Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

chore(functions): update cloud functions to specify build SA #1282

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jul 5, 2024
Merged

chore(functions): update cloud functions to specify build SA #1282

merged 9 commits into from
Jul 5, 2024

Conversation

eeaton
Copy link
Collaborator

@eeaton eeaton commented Jun 27, 2024

to address #1269.

I haven't been able to test it because our CI test org was not impacted by the default changes to cloud build SA, and has not encountered the permissions error.

@eeaton eeaton requested review from a team, gtsorbo, rjerrems and sleighton2022 as code owners June 27, 2024 09:43
@eeaton eeaton changed the title chore(functions: update cloud functions to specify build SA chore(functions): update cloud functions to specify build SA Jun 27, 2024
daniel-cit
daniel-cit requested changes Jun 27, 2024
View reviewed changes
Copy link
Contributor

@daniel-cit daniel-cit left a comment

Choose a reason for hiding this comment

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

the build service account need to be in the full format projects/{{project}}/serviceAccounts/{{email}}

1-org/envs/shared/remote.tf Outdated Show resolved Hide resolved
1-org/envs/shared/cai_monitoring.tf Outdated Show resolved Hide resolved
@daniel-cit
Copy link
Contributor

maybe a new service account created in the SCC project would provide a better separation of concerns.
the org step service account has some org level roles that would not be need for the cloud build process of the CAI function

@daniel-cit
Copy link
Contributor

@eeaton
A quick ix for the build failure would be changing

from

    runtime_env_variables = {
      ROLES     = join(",", var.roles_to_monitor)
      SOURCE_ID = google_scc_source.cai_monitoring.id
    }

to

    runtime_env_variables = {
      ROLES            = join(",", var.roles_to_monitor)
      SOURCE_ID        = google_scc_source.cai_monitoring.id
      LOG_EXECUTION_ID = "true"
    }

here
https://github.com/eeaton/terraform-example-foundation/blob/eeaton-fix-cloudfuncitons-build-default-sa/1-org/modules/cai-monitoring/main.tf#L164C5-L167C6

Change tested locally.
It does not list changes in the plan after this fix

@eeaton
Copy link
Collaborator Author

eeaton commented Jul 3, 2024 •
edited
Loading

The failing CI related to LOG_EXECUTION_ID will be fixed in #1210. Once PR 1210 is merged, I expect the tests here will pass

@eeaton eeaton mentioned this pull request Jul 3, 2024
Closed
1 task
daniel-cit
daniel-cit approved these changes Jul 4, 2024
View reviewed changes
Copy link
Contributor

@daniel-cit daniel-cit left a comment

Choose a reason for hiding this comment

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

LGTM

@daniel-cit
Copy link
Contributor

@eeaton @apeabody The build is green 💚

@eeaton eeaton enabled auto-merge (squash) July 5, 2024 11:13
apeabody
apeabody approved these changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks @eeaton!

reviewed/approved by @daniel-cit

@eeaton eeaton merged commit 5ba5380 into terraform-google-modules:master Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@apeabody apeabody apeabody approved these changes

@daniel-cit daniel-cit daniel-cit approved these changes

@rjerrems rjerrems Awaiting requested review from rjerrems

@gtsorbo gtsorbo Awaiting requested review from gtsorbo

@sleighton2022 sleighton2022 Awaiting requested review from sleighton2022

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@eeaton @daniel-cit @apeabody
Morty Proxy This is a proxified and sanitized view of the page, visit original site.