Further improve function initialization #3547

inlined · 2021-07-03T01:49:16Z

The primary purpose of this change is to introduce the "discovery"
subpackage of "runtimes". This package is a common utility that all
runtimes will use to discover their backend spec based on the "container
contract" being finalized currently.

The container contract says that we must interact with customer code
using tools we'd have in a docker container: a fully-contained directory
of code, a "run" command, and environment variables. Backend specs can
be discovered as:

The file /backend.yaml at the root of the container
The response to http://localhost:${ADMIN_PORT}/backend.yaml when
running the container's RUN command with ADMIN_PORT set to a free
port.

The golang runtime fulfills #2 through two steps:

The Functions SDK includes the "codegen" package that reads the
customer code and creates an "autogen" main package that runs
the customer code as the Firebase runtime (skeleton implementation
currently).
We then start up the Firebase runtime with ADMIN_PORT set and
fetch /backend.yaml

After this + a minor fix to the template code, we have working Go
deploys!

I also moved gomod parsing into its own file to keep index.ts to a
minimal set of things that aren't easily unit tested. Unfortunately
the diff is largr than it needs to be now.

The primary purpose of this change is to introduce the "discovery" subpackage of "runtimes". This package is a common utility that all runtimes will use to discover their backend spec based on the "container contract" being finalized currently. The container contract says that we must interact with customer code using tools we'd have in a docker container: a fully-contained directory of code, a "run" command, and environment variables. Backend specs can be discovered as: 1. The file /backend.yaml at the root of the container 2. The response to http://localhost:${ADMIN_PORT}/backend.yaml when running the container's RUN command with ADMIN_PORT set to a free port. The golang runtime fulfills #2 through two steps: 1. The Functions SDK includes the "codegen" package that reads the customer code and creates an "autogen" main package that runs the customer code as the Firebase runtime (skeleton implementation currently). 2. We then start up the Firebase runtime with ADMIN_PORT set and fetch /backend.yaml After this + a minor fix to the template code, we have working Go deploys! I also moved gomod parsing into its own file to keep index.ts to a minimal set of things that aren't easily unit tested. Unfortunately the diff is largr than it needs to be now.

taeold · 2021-07-05T03:25:52Z

src/deploy/functions/runtimes/discovery/parsing.ts

+    return;
+  }
+  for (const [keyAsString, value] of Object.entries(yaml)) {
+    // I don't know why Object.entries(foo)[0] isn't type of keyof foo...


Interesting read at microsoft/TypeScript#12253 (comment)

joehan

Some nitty things, but this mostly LGTM

joehan · 2021-07-07T18:29:40Z

src/deploy/functions/runtimes/discovery/parsing.ts

+
+// Use "omit" for output only fields. This allows us to fully exhaust keyof T
+// while still recognizing output-only fields
+export type Type = "string" | "number" | "boolean" | "object" | "array" | "omit";


Nit: type already has a meaning in ts, so I don't love us making a new meaning for Type. Consider KeyType instead?

Sure, why not?

joehan · 2021-07-07T18:35:04Z

src/deploy/functions/runtimes/discovery/v1alpha1.ts

+  region: string,
+  runtime: runtimes.Runtime
+): backend.Backend {
+  const bkend: backend.Backend = _.cloneDeep(yaml);


Could we avoid using lodash here? I think JSON.parse(JSON.stringify()) achieves the same things natively, and is only slightly worse in performance (https://measurethat.net/Benchmarks/Show/2751/0/lodash-clonedeep-vs-json-clone)

It leaves me feeling dirty, but so does lodash.

joehan · 2021-07-07T18:56:09Z

src/deploy/functions/runtimes/golang/index.ts

+    // Now that we've created main.go, we will have new dependencies on the packages pulled in
+    // by /support/runtime. We need to run one more `go get`. We could have possibly frontloaded
+    // this work by adding a hand curated list of dependencies in project creation, but this
+    // would be more birttle.


Typo in brittle

To make sure I understand, this would be more brittle because it would mean that project creation would fail if we couldn't get one of the dependencies?

I've added more flavor to the text. It's brittle because we wouldn't handle the case where the autogen code added a new dependency in some SDK version that comes out after the customer's CLI was published.

Actually, when I tried a second time I was able to get everything working by go getting the subpackages in the firebase init code. Removed this for now. It bothers me how fragile this code can seem.

joehan · 2021-07-07T19:04:30Z

src/test/deploy/functions/runtimes/discovery/parsing.spec.ts

+import * as parsing from "../../../../../deploy/functions/runtimes/discovery/parsing";
+
+describe("requireKeys", () => {
+  it("accepts found keys keys", () => {


repeated a word here?

joehan · 2021-07-07T19:45:41Z

src/test/deploy/functions/runtimes/discovery/parsing.spec.ts

+  };
+  for (const type of tests) {
+    const schema = { [type]: type as parsing.Type };
+    for (const [testType, val] of Object.entries(values)) {


Very clever!

joehan · 2021-07-07T19:46:26Z

src/test/deploy/functions/runtimes/discovery/parsing.spec.ts

+  for (const type of tests) {
+    const schema = { [type]: type as parsing.Type };
+    for (const [testType, val] of Object.entries(values)) {
+      it(`Handles a ${testType} when expecting a ${type}`, () => {


Nit: descriptions should start with lowercase

mbleigh · 2021-07-08T00:34:33Z

src/deploy/functions/runtimes/discovery/index.ts

+    if (!yaml.specVersion) {
+      throw new FirebaseError("Expect backend yaml to specify a version number");
+    }
+    if (yaml.specVersion === "v1alpha1") {


If we want to follow AIP stability levels this should be v1alpha not v1alpha1...do you intend for the spec version to match API version, or do you think they should be versioned separately? I can see argument for both.

I was actually wondering if we wanted semver or stability levels. As is, these things are strictly parsed, so there is no such thing as a revision bump. I'm open to other suggestions though.

mbleigh · 2021-07-08T00:35:30Z

src/deploy/functions/runtimes/discovery/index.ts

+  }
+
+  logger.debug("Found backend.yaml. Got spec:", text);
+  // TOODO(inlined): use a schema instead of manually checking everything or blindly trusting input.


Suggested change

// TOODO(inlined): use a schema instead of manually checking everything or blindly trusting input.

// TODO(inlined): use a schema instead of manually checking everything or blindly trusting input.

Removing comment entirely since I actually added validation post-hackathon.

mbleigh · 2021-07-08T00:36:50Z

src/deploy/functions/runtimes/discovery/index.ts

+      break;
+    } catch (err) {
+      // Allow us to wait until the server is listening.
+      if (/ECONNREFUSED/.exec(err?.message)) {


Probably want some amount of backoff or delay here, right?

Also, is this the correct way to check for error types? Is this available instead as err?.code without having to regexp it? Even if not, I probably prefer err?.message?.includes("ECONNREFUSED") to inline regexp exec.

Yes, I keep forgetting about err?.code

mbleigh · 2021-07-08T00:38:29Z

src/deploy/functions/runtimes/discovery/index.ts

+
+  while (true) {
+    try {
+      res = await Promise.race([fetch(`http://localhost:${port}/backend.yaml`), timedOut]);


I don't think this will work if an ECONNREFUSED actually occurs, will it? Is it safe to use the same promise in multiple Promise.race calls?

I would think you'd need to race the timeout against an async function that does the looping retry logic...

This is verified to work. Without the race + retry the curl will fail while the harness is booting.

mbleigh · 2021-07-08T00:42:42Z

src/deploy/functions/runtimes/golang/index.ts

+    try {
+      await promisify(fs.mkdir)(path.join(this.sourceDir, "autogen"));
+    } catch (err) {
+      if (!/EEXIST/.exec(err?.message)) {


Again if err?.code is available that feels better, and if not I think I prefer err?.message?.includes("EEXIST")

Agreed, using code

taeold · 2021-07-07T23:10:53Z

src/deploy/functions/runtimes/discovery/parsing.ts

+    prefix = prefix + ".";
+  }
+  for (const key of keys) {
+    if (!yaml[key]) {


Was this meant to be:

const fullKey = prefix ? prefix + "." + key : key; if (!yaml[fullKey]) ...

Prefix is used below in the error message. The use case is:

const obj = { outer: { inner: 5, } }` requireKeys("outer", obj.outer, "inner");

In this case, the error message should be about "outer.inner", but the context object is just {inner: 5} so the key shouldn't have the prefix.

taeold · 2021-07-08T01:05:11Z

src/deploy/functions/runtimes/golang/index.ts

+    const childProcess = spawn("go", ["run", "./autogen"], {
+      env: {
+        ...envs,
+        ...process.env,


Is inheriting process.env intended? I would've thought that we won't want to do this in anticipation of a future where parsing is done on the server side.

You're right, I should remove this and keep playing reverse-whack-a-mole to get all the envs I was otherwise missing.

It turns out the magic env is HOME. Without HOME set all go commands fail with the error error missing $GOPATH...

…ebase-tools into inlined.detect-backend-yaml

* Further improve function initialization The primary purpose of this change is to introduce the "discovery" subpackage of "runtimes". This package is a common utility that all runtimes will use to discover their backend spec based on the "container contract" being finalized currently. The container contract says that we must interact with customer code using tools we'd have in a docker container: a fully-contained directory of code, a "run" command, and environment variables. Backend specs can be discovered as: 1. The file /backend.yaml at the root of the container 2. The response to http://localhost:${ADMIN_PORT}/backend.yaml when running the container's RUN command with ADMIN_PORT set to a free port. The golang runtime fulfills firebase#2 through two steps: 1. The Functions SDK includes the "codegen" package that reads the customer code and creates an "autogen" main package that runs the customer code as the Firebase runtime (skeleton implementation currently). 2. We then start up the Firebase runtime with ADMIN_PORT set and fetch /backend.yaml After this + a minor fix to the template code, we have working Go deploys! I also moved gomod parsing into its own file to keep index.ts to a minimal set of things that aren't easily unit tested. Unfortunately the diff is largr than it needs to be now. * PR feedback

inlined requested review from joehan and taeold July 3, 2021 01:49

google-cla bot added the cla: yes Manual indication that this has passed CLA. label Jul 3, 2021

taeold reviewed Jul 5, 2021

View reviewed changes

joehan approved these changes Jul 7, 2021

View reviewed changes

mbleigh reviewed Jul 8, 2021

View reviewed changes

taeold reviewed Jul 8, 2021

View reviewed changes

inlined and others added 3 commits July 12, 2021 11:28

Merge branch 'master' into inlined.detect-backend-yaml

1f7f295

PR feedback

1d3cece

Merge branch 'inlined.detect-backend-yaml' of github.com:firebase/fir…

915ffd6

…ebase-tools into inlined.detect-backend-yaml

inlined added cla: yes Manual indication that this has passed CLA. and removed cla: yes Manual indication that this has passed CLA. labels Jul 12, 2021

inlined merged commit 0b0459b into master Jul 12, 2021

snyk-bot mentioned this pull request Jul 15, 2021

[Snyk] Upgrade @typescript-eslint/parser from 4.12.0 to 4.28.0 XirdigH/firebase-tools#11

Open

bkendall deleted the inlined.detect-backend-yaml branch March 18, 2022 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further improve function initialization #3547

Further improve function initialization #3547

inlined commented Jul 3, 2021

taeold Jul 5, 2021

inlined Jul 10, 2021

joehan left a comment

joehan Jul 7, 2021

inlined Jul 10, 2021

joehan Jul 7, 2021

inlined Jul 10, 2021

joehan Jul 7, 2021

joehan Jul 7, 2021

inlined Jul 10, 2021

inlined Jul 10, 2021

joehan Jul 7, 2021

inlined Jul 10, 2021

joehan Jul 7, 2021

joehan Jul 7, 2021

mbleigh Jul 8, 2021

inlined Jul 10, 2021

mbleigh Jul 8, 2021

inlined Jul 10, 2021

mbleigh Jul 8, 2021

mbleigh Jul 8, 2021

inlined Jul 10, 2021

mbleigh Jul 8, 2021

inlined Jul 10, 2021

mbleigh Jul 8, 2021

inlined Jul 10, 2021

taeold Jul 7, 2021

inlined Jul 10, 2021

taeold Jul 8, 2021

inlined Jul 10, 2021

inlined Jul 10, 2021

	// TOODO(inlined): use a schema instead of manually checking everything or blindly trusting input.
	// TODO(inlined): use a schema instead of manually checking everything or blindly trusting input.

Further improve function initialization #3547

Further improve function initialization #3547

Conversation

inlined commented Jul 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment