Improve App Hosting compute service account flow for source deploys #8785
Conversation
blidd-google
changed the title
src/init/features/apphosting.ts
Outdated
| ); | ||
| spinner.succeed("App Hosting compute Service account is ready"); | ||
| await ensureAppHostingComputeServiceAccount(projectId, /* serviceAccount= */ ""); | ||
| spinner.succeed("App Hosting compute service account is ready"); | ||
| } catch (err) { | ||
| if ((err as FirebaseError).status === 400) { |
There was a problem hiding this comment.
do we know for sure that a 400 error always means that the account is still being provisioned or could it be another error ? I don't know where this 400 is coming from
There was a problem hiding this comment.
actually, if i'm reading the internal chat thread correctly, 400 is from the chunk of code you've deleted in 281 - 302 ?
it seems like we'd want to do either of the following:
- keep the IAM role setting, but ignore any 400 error from it and just log a warning saying:
a) the SA is being provisioned in the background and
b) IAM roles needed for other actions may not be ready yet.
and I assume, at some later point when the user does need the iam roles, the CLI will attempt to set them again for the SA? in which case i feel like (b) is optional - OR get rid of IAM role setting (as you've done in this PR). raise any errors that come from provisioning the SA. or if no errors, we just log that the SA is being provisioned in the background.
There was a problem hiding this comment.
Unfortunately it seems the IAM API errors are somewhat non-deterministic; the addServiceAccountToRoles API call will sometimes return the 400 error when the service account doesn't exist yet, causing the init flow to error out. This is what Kiana was running into.
If it is just an error due to propagation delay, we would prefer not to interrupt the init flow, so I added the logic to ignore the 400 error. However, I think it is also valid to fail out on any error in case the issue isn't related to propagation delay.
There was a problem hiding this comment.
ok got it addServiceAccountToRoles is still being called in provisionDefaultComputeServiceAccount.
If we know that this 400 is coming from addServiceAccountToRoles, can we do this try-catch for the 400 in provisionDefaultComputeServiceAccount.
ideally, the errors should be handled as close as possible to the source. putting it here gives the reader no context, makes maintenance difficult, and could potentially try-catch errors we should not be catching.
| @@ -267,39 +266,14 @@ export async function ensureAppHostingComputeServiceAccount( | |||
| } catch (err: unknown) { | |||
| if (!(err instanceof FirebaseError)) { | |||
| throw err; | |||
| } | |||
| if (err.status === 404) { | |||
| await provisionDefaultComputeServiceAccount(projectId); | |||
| } else if (err.status === 403) { | |||
| throw new FirebaseError( | |||
| `Failed to create backend due to missing delegation permissions for ${sa}. Make sure you have the iam.serviceAccounts.actAs permission.`, | |||
| { original: err }, | |||
| ); | |||
There was a problem hiding this comment.
Since we got rid of that final throw err in apphosting.ts, i believe we need to add throw err here for any error that passed the first two if statements?
And then with that, I'm wondering if the error handling in prepare.ts is no longer needed and can be removed?
github-project-automation
bot
moved this to Approved [PR]
in [Cloud] Extensions + Functions
github-project-automation
bot
moved this from Approved [PR]
to Done
in [Cloud] Extensions + Functions
In the firebase init apphosting flow, we preemptively enable ensure the App Hosting compute service accounts exists and add the required IAM roles. However, we don't actually need the compute service account for any of the init steps. So instead of failing the init and deploy flows when a service account is still being provisioned, we optimistically proceed with the remaining steps.