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

FAST: fix CI/CD source repositories in stage 01 #682

Merged
merged 12 commits into from
Jun 16, 2022
Merged

Conversation

imp14a
Copy link
Collaborator

@imp14a imp14a commented Jun 16, 2022

Some changes required in order to enable sourcerepo cicd type for resman stage

Permissions required in bootstrap stage for Source Repository and Cloud Build trigger
Change the SA for sourerepo module in each branch based on the cicd.tf file from bootstrap stage
Adding 'try' statement to outputs.tf, identity_provider value is null for sourcerepo type

@imp14a imp14a requested a review from ludoo June 16, 2022 02:44
@ludoo
Copy link
Collaborator

ludoo commented Jun 16, 2022

Hey Agustin, can you explain the rationale behind the two changes? Why do you need to assign owner role to resource management SA? Why are you changing the trigger service account, which would break impersonation flow in the cloud build workflow?

@ludoo
Copy link
Collaborator

ludoo commented Jun 16, 2022

Hey Agustin, can you explain the rationale behind the two changes? Why do you need to assign owner role to resource management SA? Why are you changing the trigger service account, which would break impersonation flow in the cloud build workflow?

Sorry, just woke up and needed a coffee :) Thanks for spotting the incorrect trigger service accounts! Let me check if a different role can be assigned to the resource mgmt service account instead of owner, then this is good to go!

ludoo
ludoo previously requested changes Jun 16, 2022
fast/stages/00-bootstrap/automation.tf Show resolved Hide resolved
@ludoo ludoo dismissed their stale review June 16, 2022 19:58

stale review

@ludoo
Copy link
Collaborator

ludoo commented Jun 16, 2022

So, good catch on the wrong service accounts defined in the cloud build triggers for stage 1, and sorry for the confusing review I did this morning. :)

There's one problem left to address, which is a missing role required to use the service accounts in the triggers, which is the reason why you had assigned roles/owner initially. We can't use that as it's too broad, and I tried various permutations of roles to both the Cloud Build Service identity and Cloud Build default service account with no success. There's quite a few people having had that issue in the past, we just need to find the role role to the right identity and then we can merge this.

@ludoo
Copy link
Collaborator

ludoo commented Jun 16, 2022

This is probably the doc we should follow, I'll give it a try

https://cloud.google.com/build/docs/securing-builds/configure-user-specified-service-accounts#permissions

@ludoo
Copy link
Collaborator

ludoo commented Jun 16, 2022

The latest commit makes trigger creation work, but introduces two unrelated issues.

The first one is that depending service account module outputs on its iam bindings creates cycles in some of our examples and fast stages, and tests fail; @juliocc and I need to figure out if breaking the module assumptions (removing dependencies) is the only way to address this.

The second issue is that I added a data source to get to the resource management service account email from within the stage, as that is the identity that needs the service account user role on CI/CD service accounts for trigger creation. I don't see many other options, as a) giving the role at the project level in stage 0 has security implications, b) stage 0 has no notion of the CI/CD service accounts created in stage 1. The data source depends on the provider authentication, which we don't do in our tests, so it makes the test fail. Again, I need to discuss this with @juliocc.

Bottom line, this PR works and is needed, but breaks other examples and stages, and the stage1 test. Stay tuned. :)

Copy link
Collaborator

@ludoo ludoo left a comment

Choose a reason for hiding this comment

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

I addressed the two issues mentioned above by a) exclding the problematic data source when no ci/cd repos are configured, so test work and b) reverting the SA module changes and adding explicit dependencies. We will revisit this but for now I think it's good to go.

If this works for you feel free to merge (or ping me if you don't see a merge button and I'll do it), if you think there are changes needed let's keep working on it.

Thanks a lot for spotting the issues and opening this!

@ludoo ludoo changed the title sourcerepo and cloudbuild at 01-resman FAST: fix CI/CD source repositories in stage 01 Jun 16, 2022
@imp14a
Copy link
Collaborator Author

imp14a commented Jun 16, 2022

Tested and works fine! thanks @ludoo

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.

2 participants