-
Notifications
You must be signed in to change notification settings - Fork 951
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,7 +123,7 @@ | |
try { | ||
await fn(); | ||
this.logOpSuccess(op, endpoint); | ||
} catch (err: any) { | ||
result.error = err as Error; | ||
} | ||
result.durationMs = timer.stop(); | ||
|
@@ -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 GitHub Actions / lint (20)
|
||
await this.deleteTrigger(endpoint); | ||
} | ||
Check failure on line 219 in src/deploy/functions/release/fabricator.ts GitHub Actions / lint (20)
|
||
catch(err) { | ||
/** | ||
* In cases where a function has been partially deleted, | ||
Check failure on line 222 in src/deploy/functions/release/fabricator.ts GitHub Actions / lint (20)
|
||
* we may encounter errors on subsequent attempts to delete it. | ||
* | ||
Check failure on line 224 in src/deploy/functions/release/fabricator.ts GitHub Actions / lint (20)
|
||
* 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 GitHub Actions / lint (20)
|
||
*/ | ||
if ( | ||
err instanceof reporter.DeploymentError && | ||
Check failure on line 232 in src/deploy/functions/release/fabricator.ts GitHub Actions / lint (20)
|
||
err.op == "delete schedule" && | ||
Check failure on line 233 in src/deploy/functions/release/fabricator.ts GitHub Actions / lint (20)
Check failure on line 233 in src/deploy/functions/release/fabricator.ts GitHub Actions / lint (20)
Check failure on line 233 in src/deploy/functions/release/fabricator.ts GitHub Actions / unit (18)
|
||
(err.original as FirebaseError).message == "HTTP Error: 404, Job not found." | ||
Check failure on line 234 in src/deploy/functions/release/fabricator.ts GitHub Actions / lint (20)
Check failure on line 234 in src/deploy/functions/release/fabricator.ts GitHub Actions / lint (20)
Check failure on line 234 in src/deploy/functions/release/fabricator.ts GitHub Actions / unit (18)
|
||
) { | ||
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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
@@ -223,7 +254,7 @@ | |
} | ||
|
||
async createV1Function(endpoint: backend.Endpoint, scraper: SourceTokenScraper): Promise<void> { | ||
const sourceUrl = this.sources[endpoint.codebase!]?.sourceUrl; | ||
if (!sourceUrl) { | ||
logger.debug("Precondition failed. Cannot create a GCF function without sourceUrl"); | ||
throw new Error("Precondition failed"); | ||
|
@@ -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}`, | ||
operationResourceName: op.name, | ||
onPoll: scraper.poller, | ||
}); | ||
|
@@ -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 GitHub Actions / lint (20)
|
||
) { | ||
// Auth Blocking functions should always be public | ||
await this.executor | ||
|
@@ -291,7 +322,7 @@ | |
} | ||
|
||
async createV2Function(endpoint: backend.Endpoint, scraper: SourceTokenScraper): Promise<void> { | ||
const storageSource = this.sources[endpoint.codebase!]?.storage; | ||
if (!storageSource) { | ||
logger.debug("Precondition failed. Cannot create a GCFv2 function without storage"); | ||
throw new Error("Precondition failed"); | ||
|
@@ -307,15 +338,15 @@ | |
.run(async () => { | ||
try { | ||
await pubsub.createTopic({ name: topic }); | ||
} catch (err: any) { | ||
// Pub/Sub uses HTTP 409 (CONFLICT) with a status message of | ||
// ALREADY_EXISTS if the topic already exists. | ||
if (err.status === 409) { | ||
return; | ||
} | ||
throw new FirebaseError("Unexpected error creating Pub/Sub topic", { | ||
original: err as Error, | ||
status: err.status, | ||
}); | ||
} | ||
}) | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.