Skip to content
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.

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

fix: prevent failure when deleting cloud function with missing schedule #7787

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 32 additions & 1 deletion src/deploy/functions/release/fabricator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@
try {
await fn();
this.logOpSuccess(op, endpoint);
} catch (err: any) {

Check warning on line 126 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
result.error = err as Error;
}
result.durationMs = timer.stop();
Expand Down Expand Up @@ -214,7 +214,38 @@
}

async deleteEndpoint(endpoint: backend.Endpoint): Promise<void> {
await this.deleteTrigger(endpoint);
try{

Check failure on line 217 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Insert `·`

Check failure on line 217 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / unit (18)

Insert `·`
await this.deleteTrigger(endpoint);
}

Check failure on line 219 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Replace `⏎····catch` with `·catch·`

Check failure on line 219 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / unit (18)

Replace `⏎····catch` with `·catch·`
catch(err) {
/**
* In cases where a function has been partially deleted,

Check failure on line 222 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Delete `·`

Check failure on line 222 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / unit (18)

Delete `·`
* we may encounter errors on subsequent attempts to delete it.
*

Check failure on line 224 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Delete `·`

Check failure on line 224 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / unit (18)

Delete `·`
* For example, if an account lacking the `pubsub.topics.delete` permission
* attempts to delete a function with a Pub/Sub trigger, the trigger will be
* deleted but the topic will not. This will cause the function deletion
* to fail on subsequent attempts.
*

Check failure on line 229 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Delete `·`

Check failure on line 229 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / unit (18)

Delete `·`
*/
if (
err instanceof reporter.DeploymentError &&

Check failure on line 232 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Replace `··········err·instanceof·reporter.DeploymentError·&&·` with `········err·instanceof·reporter.DeploymentError·&&`

Check failure on line 232 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / unit (18)

Replace `··········err·instanceof·reporter.DeploymentError·&&·` with `········err·instanceof·reporter.DeploymentError·&&`
err.op == "delete schedule" &&

Check failure on line 233 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Replace `··········err.op·==·"delete·schedule"·&&·` with `········err.op·==·"delete·schedule"·&&`

Check failure on line 233 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Expected '===' and instead saw '=='

Check failure on line 233 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / unit (18)

Replace `··········err.op·==·"delete·schedule"·&&·` with `········err.op·==·"delete·schedule"·&&`

Check failure on line 233 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / unit (18)

Expected '===' and instead saw '=='
(err.original as FirebaseError).message == "HTTP Error: 404, Job not found."

Check failure on line 234 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Delete `··`

Check failure on line 234 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Expected '===' and instead saw '=='

Check failure on line 234 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / unit (18)

Delete `··`

Check failure on line 234 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / unit (18)

Expected '===' and instead saw '=='
Copy link
Contributor

Choose a reason for hiding this comment

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

Drive by: Probably safer to just check if err.status == 404 - relying on the exact string is vulnerable to breaking in the future.

Copy link
Author

Choose a reason for hiding this comment

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

I considered that; however, I wasn't sure whether the backend here may re-use the 404 status code for a scenario where we would not want to continue cleanup.

I can adjust this either way based on guidance from someone familiar with the possible failure modes here.

) {
logger.warn(`Cloud Scheduler job for ${endpoint.id} not found. It may have been deleted out of band. Continuing to delete function.`);
}
else if (
err instanceof reporter.DeploymentError &&
err.op == "delete topic" &&
err.message.startsWith("Failed to delete topic function")
Copy link
Contributor

Choose a reason for hiding this comment

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

This concerns me - if the topic deletion fails for a generic reason, I don't like orphaning the resource. If we can check for specific failures (ie 404s), I'd be more comfortable with this

) {
logger.warn(`Failed to delete topic function for ${endpoint.id}. Ensure the account has 'pubsub.topics.delete' permission. You may need to manually delete the topic. Continuing to delete function.`);
}
else {
throw err;
}
}
if (endpoint.platform === "gcfv1") {
await this.deleteV1Function(endpoint);
} else {
Expand All @@ -223,7 +254,7 @@
}

async createV1Function(endpoint: backend.Endpoint, scraper: SourceTokenScraper): Promise<void> {
const sourceUrl = this.sources[endpoint.codebase!]?.sourceUrl;

Check warning on line 257 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Forbidden non-null assertion
if (!sourceUrl) {
logger.debug("Precondition failed. Cannot create a GCF function without sourceUrl");
throw new Error("Precondition failed");
Expand All @@ -242,7 +273,7 @@
const op: { name: string } = await gcf.createFunction(apiFunction);
return poller.pollOperation<gcf.CloudFunction>({
...gcfV1PollerOptions,
pollerName: `create-${endpoint.codebase}-${endpoint.region}-${endpoint.id}`,

Check warning on line 276 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Invalid type "string | undefined" of template literal expression
operationResourceName: op.name,
onPoll: scraper.poller,
});
Expand Down Expand Up @@ -279,7 +310,7 @@
}
} else if (
backend.isBlockingTriggered(endpoint) &&
AUTH_BLOCKING_EVENTS.includes(endpoint.blockingTrigger.eventType as any)

Check warning on line 313 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe argument of type `any` assigned to a parameter of type `"providers/cloud.auth/eventTypes/user.beforeCreate" | "providers/cloud.auth/eventTypes/user.beforeSignIn"`

Check warning on line 313 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
) {
// Auth Blocking functions should always be public
await this.executor
Expand All @@ -291,7 +322,7 @@
}

async createV2Function(endpoint: backend.Endpoint, scraper: SourceTokenScraper): Promise<void> {
const storageSource = this.sources[endpoint.codebase!]?.storage;

Check warning on line 325 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Forbidden non-null assertion
if (!storageSource) {
logger.debug("Precondition failed. Cannot create a GCFv2 function without storage");
throw new Error("Precondition failed");
Expand All @@ -307,15 +338,15 @@
.run(async () => {
try {
await pubsub.createTopic({ name: topic });
} catch (err: any) {

Check warning on line 341 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
// Pub/Sub uses HTTP 409 (CONFLICT) with a status message of
// ALREADY_EXISTS if the topic already exists.
if (err.status === 409) {

Check warning on line 344 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .status on an `any` value
return;
}
throw new FirebaseError("Unexpected error creating Pub/Sub topic", {
original: err as Error,
status: err.status,

Check warning on line 349 in src/deploy/functions/release/fabricator.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe assignment of an `any` value
});
}
})
Expand Down
6 changes: 5 additions & 1 deletion src/gcp/cloudscheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,11 @@ export async function createOrReplaceJob(job: Job): Promise<any> {
const jobName = job.name.split("/").pop();
const existingJob = await getJob(job.name);
// if no job is found, create one
if (existingJob.status === 404) {
if (existingJob.status === 403) {
throw new FirebaseError(`Failed to create scheduler job ${job.name}: Ensure you have the 'cloudscheduler.jobs.get' permission`);

}
else if (existingJob.status === 404) {
let newJob;
try {
newJob = await createJob(job);
Expand Down
Loading