Remove CodePipeline GitHub environment variable by gdavison · Pull Request #14175 · hashicorp/terraform-provider-aws

gdavison · 2020-07-14T19:36:39Z

Removes the previously required GITHUB_TOKEN environment variable for CodePipeline. The authentication token is now passed as the OAuthToken field in the action configuration.

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #2796
Closes #4768

Release note for CHANGELOG:

resource/aws_codepipeline: Removes `GITHUB_TOKEN` environment variable

Output from acceptance testing in commercial partition:

$ make testacc TESTARGS='-run=TestAccAWSCodePipeline_'

--- PASS: TestAccAWSCodePipeline_disappears (35.93s)
--- PASS: TestAccAWSCodePipeline_emptyStageArtifacts (40.89s)
--- PASS: TestAccAWSCodePipeline_multiregion_basic (48.58s)
--- PASS: TestAccAWSCodePipeline_deployWithServiceRole (55.08s)
--- PASS: TestAccAWSCodePipeline_WithNamespace (58.69s)
--- PASS: TestAccAWSCodePipeline_basic (63.80s)
--- PASS: TestAccAWSCodePipeline_tags (75.56s)
--- PASS: TestAccAWSCodePipeline_multiregion_Update (79.32s)
--- PASS: TestAccAWSCodePipeline_multiregion_ConvertSingleRegion (94.63s)

Output from acceptance testing in GovCloud partition:

--- SKIP: TestAccAWSCodePipeline_multiregion_ConvertSingleRegion (2.37s)
--- SKIP: TestAccAWSCodePipeline_multiregion_basic (2.38s)
--- SKIP: TestAccAWSCodePipeline_multiregion_Update (2.39s)
--- PASS: TestAccAWSCodePipeline_disappears (24.64s)
--- PASS: TestAccAWSCodePipeline_emptyStageArtifacts (30.55s)
--- PASS: TestAccAWSCodePipeline_WithNamespace (32.72s)
--- PASS: TestAccAWSCodePipeline_deployWithServiceRole (35.85s)
--- PASS: TestAccAWSCodePipeline_basic (46.98s)
--- PASS: TestAccAWSCodePipeline_tags (57.72s)

gdavison · 2020-07-14T21:15:53Z


+func testAccPreCheckAWSCodePipelineAlternateRegion(t *testing.T) {
+	// There isn't a way to get the alternate region provider at PreCheck time, so hardcode it
+	if testAccGetAlternateRegion() == "us-gov-east-1" {


Not sure, but this might be better as a check on testAccGetAlternateRegionPartition() == "aws-us-gov" instead…

Wow this is annoying (on the AWS side), I'm fine with either, to be honest.

bflad

This is well implemented so far given the existing Terraform core and SDK constraints, but I see what you are running into regarding that attribute since its TypeMap. 😬 I'm guessing we will want to circle the wagons and come up with an overall plan for this, even if that means (hopefully temporarily) punting on this as part of the current major version release.

bflad · 2020-07-15T00:33:05Z

+
+const codePipelineGitHubTokenHashPrefix = "hash-"
+
+func hashCodePipelineGitHubToken(token string) string {


We are actively in the process of removing the few other instances of hashed state storage, however Terraform in general doesn't handle TypeMap and Sensitive: true in a meaningful way without making the entire map sensitive, if I remember correctly. 😖

It would be a bummer to introduce this technical debt in, but otherwise we could also be forced down the road of what I think you were suggesting the other day of potentially going against the API design slightly and implementing bespoke Terraform schema so we can control it better (and in this situation, actually implement a single sensitive attribute). We do have some slight precedence of this class of user experience enhancement with resources like aws_sns_platform_application and I believe aws_sns_topic.

I think this leaves us in a tough spot. 🙁 We can try to quickly design something better for this situation to squeeze it in for this major release in the next two weeks, which is fairly risky in my opinion, or punt on this for now and spend later cycles on this issue in general (potentially changing the Core/SDK sensitive map abilities and/or ensuring we get good feedback on a new resource design). What do you think?

I think the environment variable is a huge pain-point for practitioners, and waiting until 4.0 to remove it would be less than ideal. If we don't want to hash the value, would setting the TypeMap to Sensitive be a better option at this point? And then we could come up with a better solution.

Our potential path forward will be:

Removing the state hashing handling

Marking the configuration attribute Sensitive: true and Computed: true

Adding a new github/github_configuration configuration block so we can properly show a difference with only the oauth attribute sensitive

These will require apply-time validation to ensure they aren't both defined since ConflictsWith does not support relative references. The other configuration blocks will need to be filled in later due to time. The documentation and upgrade guide should recommend using the new configuration block(s) over the map.

bflad · 2020-07-15T00:37:50Z

 	}
 }

+func testAccPreCheckAWSCodePipelineAlternateRegion(t *testing.T) {


Ah its a bummer we cannot reuse logic similar to testAccPreCheckAWSCodePipeline here (using a real API call to automatically enable/skip the test), but I see the problem is that we don't initialize an "alternate" global instance of the provider in the testing similar to testAccProvider. I think this is okay as-is for now until we can figure out things as part of #8983 / #8316

Yeah, I briefly considered the option of rolling my own connection to test the API, but I figured the hard-coded option was quicker. Though it actually might not be that complicated

bflad · 2020-07-15T00:38:39Z


+func testAccPreCheckAWSCodePipelineAlternateRegion(t *testing.T) {
+	// There isn't a way to get the alternate region provider at PreCheck time, so hardcode it
+	if testAccGetAlternateRegion() == "us-gov-east-1" {


Wow this is annoying (on the AWS side), I'm fine with either, to be honest.

bflad · 2020-07-30T14:51:12Z

Once these pending items are commented/addressed, will provide another review:

Fixing merge conflicts
Adding a new github/github_configuration configuration block so we can properly show a difference with only the oauth attribute sensitive

…ine and adds drift detection to GitHub action OAuthToken

…nsitive

…tion` sensitive" This reverts commit 9153409.

…eline Webhook tests

bflad

LGTM 🚀

ghost · 2020-07-31T02:18:11Z

This has been released in version 3.0.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2020-08-30T17:13:45Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

gdavison requested a review from a team July 14, 2020 19:36

gdavison commented Jul 14, 2020

View reviewed changes

gdavison mentioned this pull request Jul 14, 2020

This addresses the oddity of the configuration #5764

Closed

gdavison added this to the v3.0.0 milestone Jul 14, 2020

bflad suggested changes Jul 15, 2020

View reviewed changes

gdavison force-pushed the f-remove-codepipeline-github-envvar branch from 747bc2e to 48f86f8 Compare July 15, 2020 19:15

anGie44 force-pushed the master branch from 54db70d to 48ba2f7 Compare July 21, 2020 18:26

gdavison force-pushed the f-remove-codepipeline-github-envvar branch from 48f86f8 to 4782313 Compare July 22, 2020 19:37

gdavison added 9 commits July 30, 2020 17:12

Code cleanup

48083f3

Removes GITHUB_TOKEN environment variable requirement for CodePipel…

0468506

…ine and adds drift detection to GitHub action OAuthToken

Updates usage example in documentation for Terraform 0.12

2b2a2d1

Adds disappears test and cleans up test configurations

857c35d

Updates tests for GovCloud

1e90b47

Updates PreCheck to try API call in acceptance test regions

438deca

Adds Version 3 Upgrade documentation

22e5d58

Removes hashing from GitHub OAuthToken and marks configuration se…

fa9628f

…nsitive

Revert "Removes hashing from GitHub OAuthToken and marks `configura…

2f0704c

…tion` sensitive" This reverts commit 9153409.

gdavison force-pushed the f-remove-codepipeline-github-envvar branch from d8a72a8 to 2f0704c Compare July 31, 2020 00:25

Updates provider for multiregion tests and adds OAuthToken to CodePip…

c2e885f

…eline Webhook tests

bflad approved these changes Jul 31, 2020

View reviewed changes

gdavison merged commit 25cb068 into master Jul 31, 2020

gdavison deleted the f-remove-codepipeline-github-envvar branch July 31, 2020 01:27

gdavison added a commit that referenced this pull request Jul 31, 2020

Update CHANGELOG for #14175

aec3fca

bflad mentioned this pull request Jul 31, 2020

aws_codepipeline with Github OAuth causing persistent changes #2854

Closed

fishfacemcgee mentioned this pull request Aug 13, 2020

Codepipeline OAuthToken property inside source configuration should be sensitive #4768

Closed

ghost locked and limited conversation to collaborators Aug 30, 2020


		const codePipelineGitHubTokenHashPrefix = "hash-"

		func hashCodePipelineGitHubToken(token string) string {

Conversation

gdavison commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Community Note

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bflad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bflad commented Jul 30, 2020

Uh oh!

bflad left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 31, 2020

Uh oh!

ghost commented Aug 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gdavison commented Jul 14, 2020 •

edited

Loading