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

Propogate explicit service account to eventarc #6859

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

inlined
Copy link
Member

@inlined inlined commented Mar 8, 2024

Another part of the fix to #6814. Similar to #6858, this propagates the function's service account through to the invoking service.

@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.27%. Comparing base (ec1dd69) to head (7dcdb81).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6859   +/-   ##
=======================================
  Coverage   54.27%   54.27%           
=======================================
  Files         352      352           
  Lines       24534    24536    +2     
  Branches     5082     5083    +1     
=======================================
+ Hits        13315    13317    +2     
  Misses      10006    10006           
  Partials     1213     1213           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@inlined inlined merged commit 27088cd into master Mar 13, 2024
35 checks passed
@inlined inlined deleted the inlined.eventarc-invoker branch March 13, 2024 02:33
@aablsk
Copy link

aablsk commented Mar 25, 2024

FYI: This is a breaking change that does ignore the preserve_external_changes=True parameter and it broke our deployments giving us less control over our infrastructure than we had previously.

We would have much preferred an additional parameter to set the service account for the event_arc trigger separately instead of re-using an existing service account and as such forcing the use of one service account for both the event arc trigger and the workload.

@inlined
Copy link
Member Author

inlined commented Mar 26, 2024

This is unfortunate. I can go through the process of creating an API proposal to have two separate fields. As hard-coding a possibly non-existent service account might have been considered a bug, we may instead roll forward where the trigger service account defaults to the service account used within the container and can be overridden.

Can you help me understand your use case a bit better? I assume you were using a lower-privileged service account in your functions to ensure bugs don't have a large blast radius?

@aablsk
Copy link

aablsk commented Mar 26, 2024

Dear @inlined thank you for your immediate response! 🙇

We would absolutely appreciate the API being amended to have two separate fields.

The motivation for multiple service accounts is exactly as you assumed to adhere to best practices and minimise any potential bugs or security incidents have highly limited blast radius (e.g. the service account for the CloudRun service would usually not have permissions to invoke itself). Additionally, it makes it easier to manage/maintain the service accounts permission if the service account is more narrowly scoped thus further reducing the risk of unnecessary permissions that are kept due to uncertainty of them being in use.

Our setup
Our use-case involves deploying our firebase functions with firebase-cli and then adjusting parameters that cannot be set through firebase with terraform. firebase functions are imported into terraform after the first deployment to have them in IaC as well and allow us to set parameters that cannot be set through the firebase function config. This is how we used to set the service account for the Event Arc trigger (and this would still hypothetically work). We also import the corresponding pub/sub subscription into Terraform so we can set e.g. dead letter queue behavior.

Unfortunately, when changing the service account of a Event Arc Trigger for a function it needs to be re-created, which also means that the corresponding Pub/Sub topic and Pub/Sub subscription are re-created. This means that we either would have to re-import the subscription on every deployment or we need to adhere to how firebase-cli manages the services accounts (one service account for both trigger and the service).

Thank you for your time and consideration @inlined! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants