Improve App Hosting compute service account flow for source deploys #8785
Conversation
| ); | ||
| 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.
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.