-
Notifications
You must be signed in to change notification settings - Fork 545
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
Adding support for service account impersonation and short lived tokens #246
Adding support for service account impersonation and short lived tokens #246
Conversation
…in preconditions script
modules/core_project_factory/main.tf
Outdated
@@ -169,6 +170,18 @@ resource "google_project_services" "project_services_authority" { | |||
depends_on = [google_project.main] | |||
} | |||
|
|||
resource "google_project_service" "iam_credentials_api" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a separate service block, we should inject this into the activate_apis
. (In particular, this would break in the case of authoritatively setting the project services.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been fixed in the latest commit and is injected in the locals block.
modules/core_project_factory/main.tf
Outdated
/************************************************************************************ | ||
Default compute service account action. Options are keep, depriviledge, delete. | ||
***********************************************************************************/ | ||
resource "null_resource" "modify_default_compute_service_account" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming the resource is likely to lead to some issues with existing projects (causing state migrations). I'd prefer if we kept these as two separate null_resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool make sense, this has been fixed in the latest commit.
modules/core_project_factory/main.tf
Outdated
] | ||
} | ||
|
||
/****************************************** | ||
Default Service Account configuration | ||
*****************************************/ | ||
resource "google_service_account" "default_service_account" { | ||
account_id = "project-service-account" | ||
account_id = "project-service-account" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to run the format/linting commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, this formatting change was the result of running terraform fmt
FYI, running make -s has a bunch of errors locally for me for files I haven't touched, perhaps I am using a different version of shellcheck. Mine is 0.4.7, here is a sample of the errors:
In ./helpers/setup-sa.sh line 171:
\ \ - serviceAccount:${SA_ID}\\
^-- SC1117: Backslash is literal in "\ ". Prefer explicit escaping: "\\ ".
^-- SC1117: Backslash is literal in "\ ". Prefer explicit escaping: "\\ ".
In ./helpers/setup-sa.sh line 172:
\ \ role: roles/billing.user" policy-tmp-$$.yml && rm policy-tmp-$$.yml.bak
^-- SC1117: Backslash is literal in "\ ". Prefer explicit escaping: "\\ ".
^-- SC1117: Backslash is literal in "\ ". Prefer explicit escaping: "\\ ".
I have run through the integration tests three times from the last commit and it looks like it is working most of the time, although I have seen this error once (and a couple times on previous commit)
Is this a known issue or something I may have introduced? |
@rjerrems that issue is a regression which exists on the master branch so it's not caused by your changes. |
Ok cool - good to know. Thanks @aaron-lane |
Hi Guys, Just giving this a nudge - let me know if there is anything that needs to be worked on for this? |
Hi Guys,
Here is a first cut at adding support for short lived tokens / service account impersonation to address this issue:
#243
I have refactored the service account deletion script (renamed to modify-service-account.sh) and updated the preconditions checking script.
Before merge I will need to work on getting tests to pass but was having some trouble getting the full test suite to run locally.
Thanks