Skip to content

Event function source warning #10407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shettyvarun268 wants to merge 10 commits into from
Closed

Event function source warning #10407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
42e515f
Making Http and Callable Dart functions Public when deployed
shettyvarun268 Apr 20, 2026
67797d4
Addressing Review comments
shettyvarun268 Apr 21, 2026
be02ebd
eliminating setInvoker to maintain consistency
shettyvarun268 Apr 21, 2026
cef854f
additional test case
shettyvarun268 Apr 21, 2026
ce2ddd4
lint fixes
shettyvarun268 Apr 21, 2026
de54c6a
Handling null case
shettyvarun268 Apr 22, 2026
5555438
Adding Test cases
shettyvarun268 Apr 22, 2026
70bcd03
Additional test cases
shettyvarun268 Apr 22, 2026
abc06a7
linting erros
shettyvarun268 Apr 22, 2026
09529c8
Warning for Transatlantic hops
shettyvarun268 Apr 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 111 additions & 1 deletion src/deploy/functions/release/fabricator.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,16 @@ describe("Fabricator", () => {
expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, "service", ["custom@"]);
});

it("sets invoker to private on Node updates when explicitly configured as private", async () => {
gcfv2.updateFunction.resolves({ name: "op", done: false });
poller.pollOperation.resolves({ serviceConfig: { service: "service" } });
run.setInvokerUpdate.resolves();
const ep = endpoint({ httpsTrigger: { invoker: ["private"] } }, { platform: "gcfv2" });

await fab.updateV2Function(ep, new scraper.SourceTokenScraper());
expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, "service", ["private"]);
});

it("sets explicit invoker on dataConnectGraphqlTrigger", async () => {
gcfv2.updateFunction.resolves({ name: "op", done: false });
poller.pollOperation.resolves({ serviceConfig: { service: "service" } });
Expand Down Expand Up @@ -974,6 +984,16 @@ describe("Fabricator", () => {
expect(run.setInvokerUpdate).to.not.have.been.called;
});

it("updates invoker to public on Node updates when explicitly null", async () => {
gcfv2.updateFunction.resolves({ name: "op", done: false });
poller.pollOperation.resolves({ serviceConfig: { service: "service" } });
run.setInvokerUpdate.resolves();
const ep = endpoint({ httpsTrigger: { invoker: null } }, { platform: "gcfv2" });

await fab.updateV2Function(ep, new scraper.SourceTokenScraper());
expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, "service", ["public"]);
});

it("doesn't set invoker on non-http functions", async () => {
gcfv2.updateFunction.resolves({ name: "op", done: false });
poller.pollOperation.resolves({ serviceConfig: { service: "service" } });
Expand Down Expand Up @@ -1767,7 +1787,7 @@ describe("Fabricator", () => {
describe("createRunFunction", () => {
it("creates a Cloud Run service with correct configuration", async () => {
runv2.createService.resolves({ uri: "https://service", name: "service" } as any);
run.setInvokerUpdate.resolves();
run.setInvokerCreate.resolves();

const ep = endpoint(
{ httpsTrigger: {} },
Expand Down Expand Up @@ -1805,6 +1825,44 @@ describe("Fabricator", () => {
}),
);
});

it("always sets callable triggers to public on creation", async () => {
runv2.createService.resolves({ uri: "https://service", name: "service" } as any);
run.setInvokerCreate.resolves();

const ep = endpoint(
{ callableTrigger: {} },
{
platform: "run",
baseImageUri: "gcr.io/base",
command: ["cmd"],
args: ["arg"],
},
);
await fab.createRunFunction(ep);

expect(run.setInvokerCreate).to.have.been.calledWith(ep.project, sinon.match.string, [
"public",
]);
});

it("does not set invoker on creation when HTTPS configuration is private", async () => {
runv2.createService.resolves({ uri: "https://service", name: "service" } as any);
run.setInvokerCreate.resolves();

const ep = endpoint(
{ httpsTrigger: { invoker: ["private"] } },
{
platform: "run",
baseImageUri: "gcr.io/base",
command: ["cmd"],
args: ["arg"],
},
);
await fab.createRunFunction(ep);

expect(run.setInvokerCreate).to.not.have.been.called;
});
});

describe("updateRunFunction", () => {
Expand Down Expand Up @@ -1839,6 +1897,58 @@ describe("Fabricator", () => {
}),
);
});

it("does not update invoker for callable functions", async () => {
runv2.updateService.resolves({ uri: "https://service", name: "service" } as any);
run.setInvokerUpdate.resolves();

const ep = endpoint({ callableTrigger: {} }, { platform: "run" });
const update = { endpoint: ep };

await fab.updateRunFunction(update);

expect(run.setInvokerUpdate).to.not.have.been.called;
});

it("updates invoker to public for HTTPS functions when explicitly null", async () => {
runv2.updateService.resolves({ uri: "https://service", name: "service" } as any);
run.setInvokerUpdate.resolves();

const ep = endpoint({ httpsTrigger: { invoker: null } }, { platform: "run" });
const update = { endpoint: ep };

await fab.updateRunFunction(update);

expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, sinon.match.string, [
"public",
]);
});

it("does not update invoker for HTTPS functions when invoker is omitted (undefined)", async () => {
runv2.updateService.resolves({ uri: "https://service", name: "service" } as any);
run.setInvokerUpdate.resolves();

const ep = endpoint({ httpsTrigger: {} }, { platform: "run" });
const update = { endpoint: ep };

await fab.updateRunFunction(update);

expect(run.setInvokerUpdate).to.not.have.been.called;
});

it("updates invoker for HTTPS functions to private when explicitly configured as private", async () => {
runv2.updateService.resolves({ uri: "https://service", name: "service" } as any);
run.setInvokerUpdate.resolves();

const ep = endpoint({ httpsTrigger: { invoker: ["private"] } }, { platform: "run" });
const update = { endpoint: ep };

await fab.updateRunFunction(update);

expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, sinon.match.string, [
"private",
]);
});
});

describe("deleteRunFunction", () => {
Expand Down
48 changes: 29 additions & 19 deletions src/deploy/functions/release/fabricator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,8 @@ export class Fabricator {
.catch(rethrowAs(endpoint, "delete"));
}

// We use createRunFunction and updateRunFunction specifically to deploy Dart functions
// directly over to Cloud Run! This avoids cluttering normal Gen 2 Node function hooks.
async createRunFunction(endpoint: backend.Endpoint): Promise<void> {
const storageSource = this.sources[endpoint.codebase!]?.storage;
if (!storageSource) {
Expand Down Expand Up @@ -692,7 +694,20 @@ export class Fabricator {
})
.catch(rethrowAs(endpoint, "create"));

await this.setInvoker(endpoint);
const serviceName = `projects/${endpoint.project}/locations/${endpoint.region}/services/${endpoint.runServiceId}`;
if (backend.isHttpsTriggered(endpoint)) {
const invoker = endpoint.httpsTrigger.invoker || ["public"];
if (!invoker.includes("private")) {
await this.executor
.run(() => run.setInvokerCreate(endpoint.project, serviceName, invoker))
.catch(rethrowAs(endpoint, "set invoker"));
}
} else if (backend.isCallableTriggered(endpoint)) {
// Callable functions should always be public
await this.executor
.run(() => run.setInvokerCreate(endpoint.project, serviceName, ["public"]))
.catch(rethrowAs(endpoint, "set invoker"));
}
}

async updateRunFunction(update: planner.EndpointUpdate): Promise<void> {
Expand Down Expand Up @@ -722,7 +737,19 @@ export class Fabricator {
})
.catch(rethrowAs(endpoint, "update"));

await this.setInvoker(endpoint);
const serviceName = `projects/${endpoint.project}/locations/${endpoint.region}/services/${endpoint.runServiceId}`;
// We check for null vs undefined to respect settings people make on the Google Console.
// If it's omitted (undefined), we don't touch policies. If it is explicitly null, we make it public.
let invoker: string[] | undefined;
if (backend.isHttpsTriggered(endpoint)) {
invoker = endpoint.httpsTrigger.invoker === null ? ["public"] : endpoint.httpsTrigger.invoker;
}

if (invoker) {
await this.executor
.run(() => run.setInvokerUpdate(endpoint.project, serviceName, invoker!))
.catch(rethrowAs(endpoint, "set invoker"));
}
}

async deleteRunFunction(endpoint: backend.Endpoint): Promise<void> {
Expand All @@ -740,23 +767,6 @@ export class Fabricator {
.catch(rethrowAs(endpoint, "delete"));
}

async setInvoker(endpoint: backend.Endpoint): Promise<void> {
if (backend.isHttpsTriggered(endpoint)) {
const invoker = endpoint.httpsTrigger.invoker || ["public"];
if (!invoker.includes("private")) {
await this.executor
.run(() =>
run.setInvokerUpdate(
endpoint.project,
`projects/${endpoint.project}/locations/${endpoint.region}/services/${endpoint.runServiceId}`,
invoker,
),
)
.catch(rethrowAs(endpoint, "set invoker"));
}
}
}

async setRunTraits(serviceName: string, endpoint: backend.Endpoint): Promise<void> {
await this.functionExecutor
.run(async () => {
Expand Down
62 changes: 62 additions & 0 deletions src/deploy/functions/triggerRegionHelper.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import * as sinon from "sinon";
import * as backend from "./backend";
import * as storage from "../../gcp/storage";
import * as triggerRegionHelper from "./triggerRegionHelper";
import * as utils from "../../utils";
import * as firestore from "../../gcp/firestore";
import * as firestoreService from "./services/firestore";

const SPEC = {
region: "us-west1",
Expand All @@ -14,9 +17,13 @@ const SPEC = {
describe("TriggerRegionHelper", () => {
describe("ensureTriggerRegions", () => {
let storageStub: sinon.SinonStub;
let firestoreStub: sinon.SinonStub;

beforeEach(() => {
storageStub = sinon.stub(storage, "getBucket").throws("unexpected call to storage.getBucket");
firestoreStub = sinon
.stub(firestore, "getDatabase")
.throws("unexpected call to firestore.getDatabase");
});

afterEach(() => {
Expand Down Expand Up @@ -117,5 +124,60 @@ describe("TriggerRegionHelper", () => {
"A function in region europe-west4 cannot listen to a bucket in region us",
);
});

it("should warn on transatlantic latency hops", async () => {
firestoreStub.resolves({ locationId: "europe-west1" });
const wantFn: backend.Endpoint = {
id: "wantFn",
entryPoint: "wantFn",
platform: "gcfv2",
eventTrigger: {
eventType: "google.cloud.firestore.document.v1.written",
eventFilters: { database: "(default)" },
retry: false,
region: "europe-west1",
},
...SPEC,
region: "us-central1",
};

const logLabeledWarningStub = sinon.stub(utils, "logLabeledWarning");

await triggerRegionHelper.ensureTriggerRegions(backend.of(wantFn));

expect(logLabeledWarningStub).to.have.been.calledOnceWith(
"functions",
`Function wantFn located in us-central1 uses a trigger located in europe-west1. ` +
`To avoid unnecessary cross-region network hops, you should explicitly assign this function to europe-west1.`,
);

logLabeledWarningStub.restore();
});

it("should not warn when regions match", async () => {
firestoreStub.resolves({ locationId: "us-central1" });
const wantFn: backend.Endpoint = {
id: "wantFn",
entryPoint: "wantFn",
platform: "gcfv2",
eventTrigger: {
eventType: "google.cloud.firestore.document.v1.written",
eventFilters: { database: "(default)" },
retry: false,
region: "us-central1",
},
...SPEC,
region: "us-central1",
};

const logLabeledWarningStub = sinon.stub(utils, "logLabeledWarning");
firestoreService.clearCache();

await triggerRegionHelper.ensureTriggerRegions(backend.of(wantFn));

expect(logLabeledWarningStub).to.not.have.been.called;

logLabeledWarningStub.restore();
});
});
});
19 changes: 19 additions & 0 deletions src/deploy/functions/triggerRegionHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as backend from "./backend";
import { serviceForEndpoint } from "./services";
import * as utils from "../../utils";

/**
* Ensures the trigger regions are set and correct
Expand All @@ -16,4 +17,22 @@ export async function ensureTriggerRegions(want: backend.Backend): Promise<void>
regionLookups.push(serviceForEndpoint(ep).ensureTriggerRegion(ep));
}
await Promise.all(regionLookups);

// Warn if an event function defaults to or is assigned to us-central1 but its trigger is elsewhere,
// to avoid unnecessary cross-region network hops. We ignore nam5 since it covers us-central1.
for (const ep of backend.allEndpoints(want)) {
if (
ep.region === "us-central1" &&
backend.isEventTriggered(ep) &&
ep.eventTrigger?.region &&
ep.eventTrigger.region !== "us-central1" &&
ep.eventTrigger.region !== "nam5"
Comment on lines +28 to +29
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 22, 2026

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"

) {
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}.`,
);
Comment on lines +31 to +35
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 22, 2026

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}.`,
      );

}
}
}
Loading