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

Add support for service account in functions.runWith #770

Conversation

egor-miasnikov
Copy link
Contributor

@egor-miasnikov egor-miasnikov commented Aug 28, 2020

Description

This PR adds support for the service account inside runWith.
My corresponding PR in firebase-tools: #2580

Code sample

functions
      .runWith({
        serviceAccountEmail: '[email protected]'
      })
      .auth.user()
      .onCreate((user) => user);

@samtstern
Copy link
Contributor

@egor-miasnikov thank you for this pull request! All API changes to Firebase SDKs require us to go through an internal API review process, so my apologies in advance this may take a bit longer than expected.

@mbleigh do you want to sponsor this one as well?

@mbleigh
Copy link
Contributor

mbleigh commented Aug 31, 2020

@egor-miasnikov thanks for the PR! I'll need to sponsor this through internal API review, which will take a bit of time, but a few things:

  1. You'll need to make a corresponding change in firebase/firebase-tools to support the new option (see here for recent example)
  2. As with the new vpcConnector setting, I'd like to make it possible for this to be project-agnostic. So when you add the firebase-tools PR, let's have a pattern of:
    1. If serviceAccountEmail is a full email, pass it through as-is.
    2. If serviceAccountEmail does not contain an @, automatically append @${project_id}.iam.gserviceaccount.com to the value.

This way I can deploy to multiple projects with the same value so long as I've set it to the name of a service account that exists in each project.

@samtstern
Copy link
Contributor

@mbleigh the firebase-tools PR is already up:
firebase/firebase-tools#2580

@egor-miasnikov
Copy link
Contributor Author

egor-miasnikov commented Aug 31, 2020

Hey @mbleigh thank you for you comments! point 1 already done, thanks @samtstern ;) Regarding point 2 I propose validate user input by '@' and if ok set as-is otherwise skip input and based on current functionality will be set default service account from the project. Or if we are going to generate email based on name maybe we would like to rename property to 'serviceAccount', what do you think? But I think it will be not so obviously relatively what need to set in it:)
PS: in all my projects where I used to use firebase we controled environment separately and inside the function we use 'some-sa@{project_id}.iam.gserviceaccount.com'

@egor-miasnikov egor-miasnikov force-pushed the add-support-for-service-account-in-runwith branch from 801177b to 7e0284b Compare September 3, 2020 19:23
@mdreizin
Copy link

@mbleigh Did you have a chance to review this PR?

@mdreizin
Copy link

mdreizin commented Oct 2, 2020

@mbleigh @samtstern Could anybody from Firebase Team also look at this PR as well? Has it passed internal API review process?

@samtstern
Copy link
Contributor

@mdreizin sorry about the silence here! I'm going to look into this and see what's going on. @mbleigh definitely wrote up the API proposal and sent it out for approval but I don't see the final decision anywhere (which is odd, it shouldn't take this long).

@egor-miasnikov
Copy link
Contributor Author

Hey @samtstern @mbleigh, any updates with the internal proposal?

@joehan
Copy link
Contributor

joehan commented Nov 30, 2020

Hey all, sorry for the delay on this, the API review slipped past us for a bit, but it is approved now! There were a few other changes to the API requested, so I'm going to make those this week and do some testing. I'm hoping to get this released sometime later this week if all goes well. Thanks for the contribution and patience!

@joehan joehan requested a review from samtstern December 3, 2020 21:09
if (options.serviceAccount) {
if (options.serviceAccount === 'default') {
// Do nothing, since this is equivalent to not setting serviceAccount.
} else if (options.serviceAccount.includes('@')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The approved API for this was actually to explicitly put a trailing @ to indicate that it is a named service account for the current project. So the else should be moved up to an else if that comes before this something like:

    else if (options.serviceAccount.endsWith('@')) {
      if (!process.env.GCLOUD_PROJECT) {
        throw new Error(
          `Unable to determine email for service account '${options.serviceAccount}' because process.env.GCLOUD_PROJECT is not set.`
        );
      }
      trigger.serviceAccountEmail = `${options.serviceAccount}${process.env.GCLOUD_PROJECT}.iam.gserviceaccount.com`;
    }

@joehan
Copy link
Contributor

joehan commented Dec 4, 2020

@mbleigh Fixed this up and retested all 4 scenarios ('default', 'service-account@', '[email protected]', and 'something-invalid'): the first three work as expected, and the fourth throws an error listing the valid options. PTAL!

@joehan joehan requested a review from mbleigh December 4, 2020 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants