Annotate function triggers with __endpoint property #999

taeold · 2021-10-27T22:13:50Z

In addition to annotating function triggers with __trigger property, we add __endpoint annotation. This property will be used by the to-be-developed functions runtime to generate/declare deployment manifest that the CLI will use to deploy the function.

There are lots of code duplication between the utility functions for annotating the __trigger and __endpoint properties. I didn't try to refactor the common code since I expect that we will favor __endpoint property in the future.

inlined

Looking good. My comments are more about what the contract should actually be. It's possible that this is a hint we should be sending this through API review.

spec/v1/cloud-functions.spec.ts

spec/v2/providers/https.spec.ts

spec/v2/providers/pubsub.spec.ts

src/cloud-functions.ts

src/v2/providers/https.ts

…se-functions into dl-endpoint-prop

colerogers

Left a few questions, also do you want to add a CHANGELOG entry for these changes?

colerogers · 2021-11-09T19:44:20Z

src/v2/providers/pubsub.ts

+    },
+  };
+  copyIfPresent(endpoint.eventTrigger, opts, 'retry', 'retry');
+  func.__endpoint = endpoint;


We seem to be alternating between using Object.defineProperty() and directly setting the property in the v2 providers. Would it be beneficial to stick with one format for all the providers?

I have mixed feels. We could always use define property because it's safest, but it is a bit arcane and is only needed for endpoint types that rely on firebaseConfig (RTDB and Storage)

I've been leaning towards "don't use defineProperty() unless we have to. And we have to use defineProperty() in some cases b/c user code will throw an error if environment variables like FIREBASE_CONFIG does not exist (which we use to define a trigger, like default bucket info).

colerogers · 2021-11-09T19:46:54Z

src/v2/providers/storage.ts

+        eventTrigger: {
+          eventType: eventType,
+          eventFilters: {
+            bucket,


Yay! So much easier to add new filters 🎆

colerogers · 2021-11-09T19:55:25Z

src/cloud-functions.ts

@@ -322,9 +333,9 @@ export interface MakeCloudFunctionArgs<EventData> {
  dataConstructor?: (raw: Event) => EventData;
  eventType: string;
  handler?: (data: EventData, context: EventContext) => PromiseLike<any> | any;
-  labels?: { [key: string]: any };
+  labels?: Record<string, string>;


Is there a risk that some projects might have non-string values here? If so, are we releasing this as a breaking change?

The backend never accepted anything but a string.

So my understanding is - yes this is breaking but uses who used anything other than string would not have succeeded in deploying a function with non-string. This seems okay to do.

colerogers · 2021-11-09T19:56:36Z

src/cloud-functions.ts

  legacyEventType?: string;
-  options?: { [key: string]: any };
+  options?: DeploymentOptions;


Same question as above, what if projects don't have options that fit into DeploymentOptions?

Another good point. Fortunately, no one should be calling makeCloudFunction directly, and in our public APIs, we make sure that passed options are typed with DeploymentOption. e.g.

firebase-functions/src/providers/pubsub.ts

Line 75 in 38f1946

private options: DeploymentOptions

Think we should be okay making this change.

… into dl-endpoint-prop

inlined · 2021-11-12T17:16:41Z

src/cloud-functions.ts

@@ -322,9 +333,9 @@ export interface MakeCloudFunctionArgs<EventData> {
  dataConstructor?: (raw: Event) => EventData;
  eventType: string;
  handler?: (data: EventData, context: EventContext) => PromiseLike<any> | any;
-  labels?: { [key: string]: any };
+  labels?: Record<string, string>;


The backend never accepted anything but a string.

src/cloud-functions.ts

inlined · 2021-11-12T17:19:20Z

src/cloud-functions.ts

+    options,
+    'vpc',
+    'vpcConnectorEgressSettings',
+    (egressSettings, vpc) => {


This is confusing to me. Is this an edit in place as an optional second parameter? I'm wondering if a more explicit approach will be clearer since now convertIfPresent only works when the calls are in the right order.

Maybe something vaguely like:

const vpc: <whateverthetypeishere> = {}; convertIfPresent(vpc, options, 'connector', 'vpcConnector'); convertIfPresent(vpc, options, 'egressSettings', 'vpcConnectorEgressSettings'); if (Object.keys(vpc).length !== 0) { endpoint.vpc = vpc; }

That's fair. I agree that the suggested code is simpler!

src/common/manifest.ts

inlined · 2021-11-12T17:30:44Z

src/providers/https.ts

+    options,
+    'invoker',
+    'invoker',
+    convertInvoker


doesn't convertInvoker turn foo@ into foo@project.gserviceaccount.com and we didn't want that?

We have that kind of logic for service accounts:

firebase-functions/src/common/encoding.ts

Lines 53 to 72 in 38f1946

export function serviceAccountFromShorthand(

serviceAccount: string

): string | null {

if (serviceAccount === 'default') {

return null;

} else if (serviceAccount.endsWith('@')) {

if (!process.env.GCLOUD_PROJECT) {

throw new Error(

`Unable to determine email for service account '${serviceAccount}' because process.env.GCLOUD_PROJECT is not set.`

);

}

return `${serviceAccount}${process.env.GCLOUD_PROJECT}.iam.gserviceaccount.com`;

} else if (serviceAccount.includes('@')) {

return serviceAccount;

} else {

throw new Error(

`Invalid option for serviceAccount: '${serviceAccount}'. Valid options are 'default', a service account email, or '{serviceAccountName}@'`

);

}

}

But not for invoker - we only do light validation checks, e.g. to make sure that invoker option with public or private doesn't conflict, etc.

Let's keep the shorthand syntax in the manifest so we can stay project-independent.

src/v2/providers/pubsub.ts

inlined · 2021-11-12T17:34:03Z

src/v2/providers/pubsub.ts

+    },
+  };
+  copyIfPresent(endpoint.eventTrigger, opts, 'retry', 'retry');
+  func.__endpoint = endpoint;


I have mixed feels. We could always use define property because it's safest, but it is a bit arcane and is only needed for endpoint types that rely on firebaseConfig (RTDB and Storage)

In addition to annotating function triggers with `__trigger` property, we add `__endpoint` annotation. This property will be used by the to-be-developed functions runtime to generate/declare deployment manifest that the CLI will use to deploy the function. There are lots of code duplication between the utility functions for annotating the `__trigger` and `__endpoint` properties. I didn't try to refactor the common code since I expect that we will favor `__endpoint` property in the future.

Follows up #999 to annotate each funuctions with `__endpoint` property. Highlight of changes: * Extend unit test coverage for all v1 providers * Add `__endpoint` annotation to v1 task queue functions * Add `__requiredAPIs` annotation to task queue and scheduler functions * Explicitly set `__endpoint` to undefined in the handler namespace * No SDK-level label setting in the __endpoint annotation.

* Annotate function triggers with __endpoint property (#999) In addition to annotating function triggers with `__trigger` property, we add `__endpoint` annotation. This property will be used by the to-be-developed functions runtime to generate/declare deployment manifest that the CLI will use to deploy the function. There are lots of code duplication between the utility functions for annotating the `__trigger` and `__endpoint` properties. I didn't try to refactor the common code since I expect that we will favor `__endpoint` property in the future. * Add missing import. * More of annotate endpoint property for functions (#1009) Follows up #999 to annotate each funuctions with `__endpoint` property. Highlight of changes: * Extend unit test coverage for all v1 providers * Add `__endpoint` annotation to v1 task queue functions * Add `__requiredAPIs` annotation to task queue and scheduler functions * Explicitly set `__endpoint` to undefined in the handler namespace * No SDK-level label setting in the __endpoint annotation.

taeold added 4 commits October 26, 2021 22:33

WIP.

72c66ef

It's v1alpha1

fb5af88

Finish writing test. Cleanup.

3c58ff3

Prettier.

5b369f1

google-cla bot added the cla: yes label Oct 27, 2021

taeold added 2 commits October 27, 2021 15:16

Nits.

2db4640

Prettier

306c269

taeold requested review from inlined and colerogers October 27, 2021 22:19

Merge branch 'master' into dl-endpoint-prop

f5b0e35

inlined reviewed Oct 29, 2021

View reviewed changes

taeold added 2 commits October 28, 2021 20:49

Unroll v1alpha manifest out of dir.

584ff6d

Merge branch 'dl-endpoint-prop' of https://github.com/firebase/fireba…

11a08fc

…se-functions into dl-endpoint-prop

taeold changed the base branch from master to dl-container-contract October 29, 2021 16:22

taeold added 15 commits November 3, 2021 12:34

Prefer timeoutSeconds over timeout.

e1d9003

Prefer topic over generic resource.

69a8846

Whoops this shouldn't go.

8d7cbee

Refactor vpc option.

eb0a9b0

Finalize refactor.

105cc7c

Don't transform SA, consistent fn name.

3de212b

Unroll trigger options.

c765db5

Make callable its own trigger, fix tests.

08cde7a

No need for dynamic props.

fafcbd5

Prettier.

7864635

Better types.

4ca1d44

Few more fixes.

3f47fdc

One mo nit.

726b4eb

No getters if unncessary.

5eda9cd

Prettier.

cec015f

taeold marked this pull request as ready for review November 8, 2021 22:24

colerogers reviewed Nov 9, 2021

View reviewed changes

Merge branch 'master' of https://github.com/firebase/firebase-functions…

10f88da

… into dl-endpoint-prop

inlined approved these changes Nov 12, 2021

View reviewed changes

taeold added 2 commits November 12, 2021 17:44

Rely on default converter function.

11f18e8

Simplify logic for copying vpc options.

fe43588

taeold merged commit a6c4971 into dl-container-contract Nov 22, 2021

taeold mentioned this pull request Nov 29, 2021

More of annotate endpoint property for functions #1009

Merged

taeold mentioned this pull request Jan 14, 2022

Container contract support for Firebase Functions SDK #1014

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotate function triggers with __endpoint property #999

Annotate function triggers with __endpoint property #999

taeold commented Oct 27, 2021 •

edited

Loading

inlined left a comment

colerogers left a comment

colerogers Nov 9, 2021

inlined Nov 12, 2021

taeold Nov 13, 2021

colerogers Nov 9, 2021

colerogers Nov 9, 2021

inlined Nov 12, 2021

taeold Nov 12, 2021 •

edited

Loading

colerogers Nov 9, 2021

taeold Nov 12, 2021

inlined Nov 12, 2021

inlined Nov 12, 2021

taeold Nov 13, 2021

inlined Nov 12, 2021

taeold Nov 13, 2021

inlined Nov 15, 2021

inlined Nov 12, 2021

	export function serviceAccountFromShorthand(
	serviceAccount: string
	): string \| null {
	if (serviceAccount === 'default') {
	return null;
	} else if (serviceAccount.endsWith('@')) {
	if (!process.env.GCLOUD_PROJECT) {
	throw new Error(
	`Unable to determine email for service account '${serviceAccount}' because process.env.GCLOUD_PROJECT is not set.`
	);
	}
	return `${serviceAccount}${process.env.GCLOUD_PROJECT}.iam.gserviceaccount.com`;
	} else if (serviceAccount.includes('@')) {
	return serviceAccount;
	} else {
	throw new Error(
	`Invalid option for serviceAccount: '${serviceAccount}'. Valid options are 'default', a service account email, or '{serviceAccountName}@'`
	);
	}
	}

Annotate function triggers with __endpoint property #999

Annotate function triggers with __endpoint property #999

Conversation

taeold commented Oct 27, 2021 • edited Loading

inlined left a comment

Choose a reason for hiding this comment

colerogers 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

taeold Nov 12, 2021 • edited Loading

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

taeold commented Oct 27, 2021 •

edited

Loading

taeold Nov 12, 2021 •

edited

Loading