-
Notifications
You must be signed in to change notification settings - Fork 66
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
Change GCP credential filename generation to be deterministic instead of random. #204
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree company="Demetri Maltsiniotis, LLC" |
Bump @mericstam, Please review at your convenience and let me know if this PR needs additional work or validation. Thank you, -Demetri |
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.
Not very clear anywhere but we try to follow Azure DevOps sprint number as minor version. (https://whatsprintis.it)
aed39a9
to
325ce43
Compare
…stead of uuid4() so the credentials filename is always the same (based on the service connection name.) Bump version to current sprint.
325ce43
to
f6aec71
Compare
Hi @mericstam, I've updated the version to match the current sprint, please let me know if anything else is needed. Cheers, -DM |
Hi, If you don't mind I would like to install the branch changes into out test environment. I you give me a name of a Azure DevOps test-account of your own I can share the published test extension to your account. ( you can create a completely new account if you want). That way we can make sure the extension code works after internal build and deploy. I have very limited GCP knowledge. Your efforts so far are appreciated a lot! Br |
Hi Manuel, That's no problem at all. I've taken care to test the extension to make sure there are no adverse changes. I have an AzDo instance set up at: https://dev.azure.com/demetri/ I can invite you to the account if you wish for testing, just let me know which e-mail you'd prefer. Thank you, Demetri |
Hi, I had some problems with our pipeline project in Azure Devops. That has been sorted out. There is one internal PR in queue to be tested. I will let you know as soon as that other has been tested then I will push your code to the dev feed. |
Wonderful, thank you for the update! |
Hi @dmaltsiniotis @mericstam |
We are having some internal issues with deploying extensions to the marketplace. We are working on that and hopefully it will be fixed ASAP. This PR is first in line after we fixed the pipeline and published the fix for the last deploy |
@mericstam @dmaltsiniotis is close to release now given the Azure WIF support was added recently? Thanks 😊 |
Hi @dmaltsiniotis I know this has taken a long time but now we are ready to take in your changes. please update your branch with the latest changes from main branch and then I will take the next steps to get your changes into the codebase. |
This PR attempts to address issue #145.
Problem
The behavior of the Terraform Google provider changed after the release of 4.0.0, and this change is slightly incompatible with the way the
TerraformCommandHandlerGCP
class handles the generation of Google Cloud specific 'credentials' files used for authorizing Terraform to a Google Cloud project (akin to a Service Principle in Azure nomenclature). Specifically, the issue arises during the use of multi-stage pipelines.Analysis
Historically, when a Terraform plan was saved with the -out parameter, it also embedded a file path to the credential-files.json containing the authorization credentials to the Google project. However, if the credentials-file.json was available via environment variable, the order of precedence would favor environment variable over the embedded path in the plan file. So long as the credential file existed in the "GOOGLE_CREDENTIALS" environment variable, everything would work just fine because this task would generate a new credential file on every stage/job, and set the variable accordingly.
When v4.0.0.0 was published, this order of precedence changed. Now, instead favoring the environment variable "GOOGLE_CREDENTIALS" first, the provider favors the embedded file path first. The reason this becomes an issue in multi-stage pipelines is because of the use of uuid4() to generate the credentials file:
This results in a randomly named file saved to disk. The filename that was used to generate the plan in the first stage, no longer exists on the second/apply stage, since each stage is not guaranteed to be run on the same ephemeral Azure DevOps agent. This will manifest itself is a few possible error message including:
Solution
Approach
There were a few different proposed solutions to the issue, and I ended up going with the most straightforward change that had the least impact. My goals was to have the task generate a credential filename that was the same for any given build/service connection, so that even between stages if the credential file was re-generated the embedded path in the plan file would remain valid.
My initial thought was to use some kind of MD5 or SHA-1 hashing, and hash a known value to generate the file name. After a quick analysis of the code though, I did not want to introduce any new dependencies as part of this change such as including NodeJS's 'crytpo' or other potentially bloated hashing NPM packages. Luckily, the functionality I was looking for was staring me right in the face: The already included UUID package.
Some quick history on UUID's:
The actual change
This PR updates the jsonKeyFilePath property to instead use
uuidV5()
instead ofuuidV4()
This results in a filename that is the deterministic based on the service connection name, and safe for writing to disk on any platform/OS:In this case, the value we are hashing is the serviceName variable. This could just have easily been build ID or anything else that remains constant between stages of a pipeline run that the task has access to.
The namespace (yet another UUID) I chose for uuidV5() is
uuidV5.URL
, which is a published, known, constant generally used to represent URLs (6ba7b811-9dad-11d1-80b4-00c04fd430c8
). There are four preexisting namespace hard-coded GUIDs defined by RFC 4122, in this case it doesn't matter what the namespace is, so long as it is consistent between stages, so I elected to use somewhat friendly lookinguuidV5.URL
rather than make up some arbitrary UUID value in-line:Gross, right? But I digress. I made sure to test this all thoroughly with the version of UUID in actual use by the package lock file: 3.4.0.
Testing
This took some time and effort to test. My testing methodology consisted of:
Verifying all the unit tests in the projects run (by following the CONTRIBUTING.md guide)
Publishing a self/dev version of the Task to the private marketplace (again per the guide)
Creating a new DevOps organization
Installing the self/dev VSTX package into the organization
Creating a new AzDo Project, repository, pipeline
Creating a new multi-stage deployment YAML file with some Hello World! Terraform that would publish and download a plan file
Creating a new Google Cloud project, state file bucket, service account, and service account key
Adding the service account to the AzDo organization as a GCP Terraform service account
Kicking off the pipeline, observe success.
Remove self/dev VSTX, install marketplace Terraform tasks
Re-run the pipeline from the same branch/hash/commit, observe failure.
Re-run the pipeline steps above on both:
Evidence
First, using the current marketplace version:
Pipeline runs, fist is marketplace task, second is task from my branch/PR:
In the current/failing task, notice the UUID is different for the generated credential files between the plan and apply stages.
Plan:
Apply:
Now, using the VSTX from this PR:
Using the Terraform task from this PR, we can observe that UUID for the credentials files are now consistent between stages:
Plan:
Apply:
Thank you for your consideration.