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

feat(execution): remove spinnaker accounts from execution context #4656

Merged
merged 4 commits into from
Feb 23, 2024

Conversation

dbyron-sf
Copy link
Contributor

Add the executions.includeAllowedAccounts flag which defaults to true. If set to true, authentication.allowedAccounts will continue to be populated by accounts in the pipeline execution json. If set to false, the pipeline execution json will show authentication.allowedAccounts = []

Testing notes:
After setting the feature flag to false,

  1. Pipelines triggered manually (by clicking play) and automatically (using a service account) continue to run. Pipelines that deploy resources into a kubernetes cluster are still able to do so
  2. If the user or service account is unauthorized to run the pipeline, the pipeline will not run

Daniel Zheng added 4 commits February 22, 2024 16:51
When true, includes accounts in the pipeline execution. When false, excludes them
to include or exclude the list of allowed accounts from the execution context
Copy link
Contributor

@xibz xibz left a comment

Choose a reason for hiding this comment

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

LGTM! One thing I was wondering, but don't have enough context is how useful it would to at least log the amount of allowed accounts. I know fiat, clouddriver, etc do some logging of accounts, but I am unsure if orca does. It would allow for us to ensure that the expected amount is there at least rather than completely missing and may be completely in the dark if there is a lack of logging in orca for this. But that can be a follow up PR since it is really minor

@dbyron-sf
Copy link
Contributor Author

Yeah, if someone wants that, let's add it in another PR. This seems like a thing where we don't actually need the info, so I'm not sure how helpful it'll be, but I won't stand in the way of logging it if someone wants to.

@dbyron-sf dbyron-sf added the ready to merge Approved and ready for merge label Feb 23, 2024
@mergify mergify bot added the auto merged Merged automatically by a bot label Feb 23, 2024
@mergify mergify bot merged commit f5fb537 into spinnaker:master Feb 23, 2024
4 checks passed
@dbyron-sf dbyron-sf deleted the remove-allowed-accounts branch February 23, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for merge target-release/1.34
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants