-
-
Notifications
You must be signed in to change notification settings - Fork 966
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
Comply with the Google Cloud impersonate library and implement integration test #2550
Conversation
remoteStateConfig := remote.RemoteStateConfigGCS{Bucket: bucketName} | ||
|
||
gcsClient, err := remote.CreateGCSClient(remoteStateConfig) | ||
if err != nil { |
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.
I was wondering why can't be used assert.NoError()
assert.NoError(t, err)
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.
I made it the same as other implementations such as here.
remote/remote_state_gcs.go
Outdated
opts = append(opts, option.ImpersonateCredentials( | ||
gcsConfigRemote.ImpersonateServiceAccount, | ||
gcsConfigRemote.ImpersonateServiceAccountDelegates...)) | ||
ts, err := impersonate.CredentialsTokenSource(context.Background(), impersonate.CredentialsConfig{ |
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.
Why can't be used existing context from ctx
variable?
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.
Modified to use ctx
variable.
Noticed failed integration tests:
|
Thank you for your review! |
Maybe you don't have enough environment variables set. 84a9eb5#diff-288bea63faddba4478b115bc89ce4fc23b9fab5f9a17dabce38fbb69033a0f7aR368-R373 |
Definitely GCP environment variables should be added to CICD |
Change environment variable name Restore default credentials before validate
Changed to set the contents of the service account key to an environment variable considering running in CICD. |
Sorry, I couldn't find any documentation that lists the environment variables required for integration testing. |
Is there anything I can help with to get PR reviewed & merged pls ? FYI We cannot upgrade Terragrunt because impersonation is broken in GCP. Thx a lot. |
Conflicting files
|
Conflicts resolved. |
Is this PR still waiting on the documentation update? We would like to adopt terragrunt, but this bug is a show-stopper since our backends all use impersonation to switch to a Terraform service account. |
This PR has not passing integration tests:
|
It is not passing integration tests because the CICD attached to this repo needs to be updated to include a second service account key. Yamadayutaka mentioned that above and in the test code. I assume this is something that must be done by a project admin who has access to the CircleCI account attached to this repo? |
Hi @denis256 any updates on this, we are also blocked on using the latest terraform version in our environment because of this broken impersonation thing, do anyone here know other way-out for using the latest terraform version using terragrunt ? |
test/integration_serial_test.go
Outdated
// run without impersonation | ||
tmpTerragruntGCSConfigPath := createTmpTerragruntGCSConfig(t, TEST_FIXTURE_GCS_PATH, project, TERRAFORM_REMOTE_STATE_GCP_REGION, gcsBucketName, config.DefaultTerragruntConfigPath) | ||
// run terregrunt ignore errors | ||
runTerragruntCommand(t, fmt.Sprintf("terragrunt apply -auto-approve --terragrunt-non-interactive --terragrunt-config %s --terragrunt-working-dir %s", tmpTerragruntGCSConfigPath, TEST_FIXTURE_GCS_PATH), os.Stdout, os.Stderr) |
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.
why error should be ignored? why can't be used assert.Error()
?
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.
Since the command is executed without impersonation, the command will fail due to lack of privileges.
This is expected behavior and the error should be ignored.
Lack of privileges should prevent the bucket from being created, so checking for that here.
https://github.com/gruntwork-io/terragrunt/pull/2550/files/7956720c2c9cc4a17fb6d8de61a54494e4962655#diff-288bea63faddba4478b115bc89ce4fc23b9fab5f9a17dabce38fbb69033a0f7aR393
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.
If is expected to have an error, then should be checked that the error is returned, it will help in the future if for some reason behavior of code will change and error will not be thrown, it will also be more to find that error is not returned instead if debugging why GCS bucket wasn't created.
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.
Thanks for explaining.
I have modified it to use assert.Error()
.
556674c
to
4ca6098
Compare
Do I understand correctly, the "only" thing missing is a second service account key in the Circle CI configuration (as per this comment? |
Hello, |
Sorry for the delay on our side. I've added the
|
Hi @robmorgan, Thank you for trying. The first call of terragrunt should result in an error, could you try waiting a little longer without interrupting? And perhaps the GOOGLE_APPLICATION_CREDENTIALS environment variable is set incorrectly. https://github.com/gruntwork-io/terragrunt/blob/master/.circleci/config.yml#L95-L97 For example, as follows.
|
Will take a look on this error |
For what it is worth, just expressing my interest in this feature! (eagerly been keeping an eye on this PR for a while) |
Changes from this PR merged and released as part of: #2679 If the issue still occurs, please open a separate issue |
Description
Fixes #1997.
Fixes #2477.
Based on #2052.
ScopeFullControl
for label updates. (refer to here)Test senario
Preparation
Account A
: Terragrunt test runner account. This requires storage administrator privileges.Account B
: Impersonation test runner account. This requires token creation privileges without storage admin privileges.Account A
- User credentials set up by using the Google Cloud CLI
- The attached service account, returned by the metadata server
Account B
Account A
Execution
Account B
.account A
)Result
https://gist.github.com/yamadayutaka/bf774ecf249ac17d08f41a5e397d2e85
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Comply with the Google Cloud impersonate library.