Skip to content

Event function source warning#10407

Closed
shettyvarun268 wants to merge 10 commits intonextfrom
event-function-source-warning
Closed

Event function source warning#10407
shettyvarun268 wants to merge 10 commits intonextfrom
event-function-source-warning

Conversation

@shettyvarun268
Copy link
Copy Markdown
Contributor

This PR implements robust cross-region latency warnings within the Firebase Functions deployment pipeline to nudge developers away from unnecessary cross-region network hops. When an event-triggered function (such as Firestore or Cloud Storage) is explicitly assigned or defaults to us-central1 while its backing event resource operates in a distinct non-US location, the CLI now issues a user-friendly warning nudge encouraging explicit collocation with the event trigger's region. This validation integrates globally following Gen 2 EventArc trigger resolution, correctly handles US multi-region databases (such as nam5) to avoid false positives, and leaves deployment executions unblocked to prevent service disruptions.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the Fabricator to manage invoker settings directly within the creation and update flows for Cloud Run functions, ensuring that null values explicitly set the invoker to public while undefined values preserve existing settings. Additionally, it adds a warning mechanism to alert users when a function in us-central1 is triggered by a resource in a different region. Feedback from the review highlights the need to exclude the us multi-region from these warnings to avoid false positives and suggests improving the warning message's guidance for multi-region triggers.

Comment on lines +28 to +29
ep.eventTrigger.region !== "us-central1" &&
ep.eventTrigger.region !== "nam5"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The cross-region trigger validation should also exclude the us multi-region when the function is in us-central1. Similar to nam5 for Firestore, the us multi-region for Cloud Storage includes us-central1. Deploying a function in us-central1 to handle events from a us bucket does not constitute a cross-region hop, and flagging it would create false positives for many standard Storage-triggered functions.

      ep.eventTrigger.region !== "us-central1" &&
      ep.eventTrigger.region !== "nam5" &&
      ep.eventTrigger.region !== "us"

Comment on lines +31 to +35
utils.logLabeledWarning(
"functions",
`Function ${ep.id} located in us-central1 uses a trigger located in ${ep.eventTrigger.region}. ` +
`To avoid unnecessary cross-region network hops, you should explicitly assign this function to ${ep.eventTrigger.region}.`,
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The warning message suggests explicitly assigning the function to the trigger's region. However, for multi-region triggers (e.g., eur4, eu, asia), users cannot assign a function to that exact location as functions require a specific region. The advice should be refined to suggest a region within or closer to the multi-region to avoid guiding users toward an invalid configuration.

      const triggerRegion = ep.eventTrigger.region;
      const suggestion = triggerRegion.includes("-") ? triggerRegion : `a region within ${triggerRegion}`;
      utils.logLabeledWarning(
        "functions",
        `Function ${ep.id} located in us-central1 uses a trigger located in ${triggerRegion}. ` +
          `To avoid unnecessary cross-region network hops, you should explicitly assign this function to ${suggestion}.`,
      );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants