Skip to content
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

provider: Prevent panic when setting endpoints configuration #8226

Merged
merged 4 commits into from
Apr 10, 2019

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Apr 6, 2019

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" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #8214
Closes #8265

Output from acceptance testing before code change:

=== CONT  TestAccAWSProvider_Endpoints
panic: assignment to entry in nil map

goroutine 614 [running]:
github.com/terraform-providers/terraform-provider-aws/aws.providerConfigure(0xc000c2fd50, 0x0, 0xc00098e300, 0xc000c2fd50, 0x0)
	/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/provider.go:1011 +0xde1
github.com/hashicorp/terraform/helper/schema.(*Provider).Configure(0xc000af7650, 0xc0006de000, 0xc, 0x653dec0)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/helper/schema/provider.go:264 +0xcd
github.com/hashicorp/terraform/terraform.(*BuiltinEvalContext).ConfigureProvider(0xc000a31860, 0xc00142c880, 0xc, 0xc0006de000, 0x0, 0x31)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval_context_builtin.go:136 +0x14a
github.com/hashicorp/terraform/terraform.(*EvalConfigProvider).Eval(0xc0009a4b40, 0x6543da0, 0xc000a31860, 0x2, 0x2, 0x59a0521, 0x4)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval_provider.go:48 +0x53
github.com/hashicorp/terraform/terraform.EvalRaw(0x645b160, 0xc0009a4b40, 0x6543da0, 0xc000a31860, 0xc000114000, 0x2, 0xc000660150, 0x2b)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval.go:53 +0x11a
github.com/hashicorp/terraform/terraform.(*EvalSequence).Eval(0xc0009a4b60, 0x6543da0, 0xc000a31860, 0x2, 0x2, 0x59a0521, 0x4)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval_sequence.go:14 +0x9c
github.com/hashicorp/terraform/terraform.EvalRaw(0x645b560, 0xc0009a4b60, 0x6543da0, 0xc000a31860, 0x2b, 0x0, 0x0, 0x2b)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval.go:53 +0x11a
github.com/hashicorp/terraform/terraform.(*EvalOpFilter).Eval(0xc00142f560, 0x6543da0, 0xc000a31860, 0x2, 0x2, 0x59a0521, 0x4)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval_filter_operation.go:37 +0x4c
github.com/hashicorp/terraform/terraform.EvalRaw(0x645b440, 0xc00142f560, 0x6543da0, 0xc000a31860, 0x0, 0x0, 0x0, 0x0)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval.go:53 +0x11a
github.com/hashicorp/terraform/terraform.(*EvalSequence).Eval(0xc0009a4b80, 0x6543da0, 0xc000a31860, 0x2, 0x2, 0x59a0521, 0x4)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval_sequence.go:14 +0x9c
github.com/hashicorp/terraform/terraform.EvalRaw(0x645b560, 0xc0009a4b80, 0x6543da0, 0xc000a31860, 0x50696e0, 0x9b05605, 0x4c8c7a0, 0xc00142b700)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval.go:53 +0x11a
github.com/hashicorp/terraform/terraform.Eval(0x645b560, 0xc0009a4b80, 0x6543da0, 0xc000a31860, 0xc0009a4b80, 0x645b560, 0xc0009a4b80, 0xc000075db0)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval.go:34 +0x4d
github.com/hashicorp/terraform/terraform.(*Graph).walk.func1(0x571f240, 0xc00143e098, 0x0, 0x0)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/graph.go:126 +0xbc2
github.com/hashicorp/terraform/dag.(*Walker).walkVertex(0xc001431810, 0x571f240, 0xc00143e098, 0xc00063c180)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/dag/walk.go:387 +0x33b
created by github.com/hashicorp/terraform/dag.(*Walker).Update
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/dag/walk.go:310 +0xa4f
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	3.940s

Output from acceptance testing after code change:

--- PASS: TestAccAWSProvider_Endpoints (1.68s)

Reference: #8214

Output from acceptance testing before code change:

```
=== CONT  TestAccAWSProvider_Endpoints
panic: assignment to entry in nil map

goroutine 614 [running]:
github.com/terraform-providers/terraform-provider-aws/aws.providerConfigure(0xc000c2fd50, 0x0, 0xc00098e300, 0xc000c2fd50, 0x0)
	/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/provider.go:1011 +0xde1
github.com/hashicorp/terraform/helper/schema.(*Provider).Configure(0xc000af7650, 0xc0006de000, 0xc, 0x653dec0)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/helper/schema/provider.go:264 +0xcd
github.com/hashicorp/terraform/terraform.(*BuiltinEvalContext).ConfigureProvider(0xc000a31860, 0xc00142c880, 0xc, 0xc0006de000, 0x0, 0x31)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval_context_builtin.go:136 +0x14a
github.com/hashicorp/terraform/terraform.(*EvalConfigProvider).Eval(0xc0009a4b40, 0x6543da0, 0xc000a31860, 0x2, 0x2, 0x59a0521, 0x4)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval_provider.go:48 +0x53
github.com/hashicorp/terraform/terraform.EvalRaw(0x645b160, 0xc0009a4b40, 0x6543da0, 0xc000a31860, 0xc000114000, 0x2, 0xc000660150, 0x2b)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval.go:53 +0x11a
github.com/hashicorp/terraform/terraform.(*EvalSequence).Eval(0xc0009a4b60, 0x6543da0, 0xc000a31860, 0x2, 0x2, 0x59a0521, 0x4)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval_sequence.go:14 +0x9c
github.com/hashicorp/terraform/terraform.EvalRaw(0x645b560, 0xc0009a4b60, 0x6543da0, 0xc000a31860, 0x2b, 0x0, 0x0, 0x2b)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval.go:53 +0x11a
github.com/hashicorp/terraform/terraform.(*EvalOpFilter).Eval(0xc00142f560, 0x6543da0, 0xc000a31860, 0x2, 0x2, 0x59a0521, 0x4)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval_filter_operation.go:37 +0x4c
github.com/hashicorp/terraform/terraform.EvalRaw(0x645b440, 0xc00142f560, 0x6543da0, 0xc000a31860, 0x0, 0x0, 0x0, 0x0)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval.go:53 +0x11a
github.com/hashicorp/terraform/terraform.(*EvalSequence).Eval(0xc0009a4b80, 0x6543da0, 0xc000a31860, 0x2, 0x2, 0x59a0521, 0x4)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval_sequence.go:14 +0x9c
github.com/hashicorp/terraform/terraform.EvalRaw(0x645b560, 0xc0009a4b80, 0x6543da0, 0xc000a31860, 0x50696e0, 0x9b05605, 0x4c8c7a0, 0xc00142b700)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval.go:53 +0x11a
github.com/hashicorp/terraform/terraform.Eval(0x645b560, 0xc0009a4b80, 0x6543da0, 0xc000a31860, 0xc0009a4b80, 0x645b560, 0xc0009a4b80, 0xc000075db0)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/eval.go:34 +0x4d
github.com/hashicorp/terraform/terraform.(*Graph).walk.func1(0x571f240, 0xc00143e098, 0x0, 0x0)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/terraform/graph.go:126 +0xbc2
github.com/hashicorp/terraform/dag.(*Walker).walkVertex(0xc001431810, 0x571f240, 0xc00143e098, 0xc00063c180)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/dag/walk.go:387 +0x33b
created by github.com/hashicorp/terraform/dag.(*Walker).Update
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/dag/walk.go:310 +0xa4f
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	3.940s
```

Output from acceptance testing after code change:

```
--- PASS: TestAccAWSProvider_Endpoints (1.68s)
```
@bflad bflad added bug Addresses a defect in current functionality. crash Results from or addresses a Terraform crash or kernel panic. regression Pertains to a degraded workflow resulting from an upstream patch or internal enhancement. provider Pertains to the provider itself, rather than any interaction with AWS. labels Apr 6, 2019
@bflad bflad added this to the v2.6.0 milestone Apr 6, 2019
@bflad bflad requested a review from a team April 6, 2019 02:17
@ghost ghost added size/S Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Apr 6, 2019
bflad added 2 commits April 9, 2019 22:20
…es cognitoidentityprovider client endpoint

Previous output from acceptance testing:

```
--- FAIL: TestAccAWSProvider_Endpoints (1.84s)
    testing.go:538: Step 0 error: Check failed: Check 1/1 error: expected endpoint (cognitoidp) value (http://cognitoidp), got:
```

Output from acceptance testing:

```
--- PASS: TestAccAWSProvider_Endpoints (1.90s)
```
…properly overwritten in client configurations

Output from acceptance testing:

```
--- PASS: TestAccAWSProvider_Endpoints_Deprecated (1.88s)
--- PASS: TestAccAWSProvider_Endpoints (1.90s)
```
@ghost ghost added size/L Managed by automation to categorize the size of a PR. and removed size/S Managed by automation to categorize the size of a PR. labels Apr 10, 2019
@bflad
Copy link
Contributor Author

bflad commented Apr 10, 2019

The new acceptance testing here will now also check each AWS service client Config.Endpoint value to ensure it matches the test configuration.

…of resource acceptance testing

Output from acceptance testing:

```
--- PASS: TestAccAWSProvider_Endpoints_Deprecated (1.75s)
--- PASS: TestAccAWSProvider_Endpoints (1.77s)
```
@@ -354,7 +354,7 @@ func (c *Config) Client() (interface{}, error) {
codedeployconn: codedeploy.New(sess.Copy(&aws.Config{Endpoint: aws.String(c.Endpoints["codedeploy"])})),
codepipelineconn: codepipeline.New(sess.Copy(&aws.Config{Endpoint: aws.String(c.Endpoints["codepipeline"])})),
cognitoconn: cognitoidentity.New(sess.Copy(&aws.Config{Endpoint: aws.String(c.Endpoints["cognitoidentity"])})),
cognitoidpconn: cognitoidentityprovider.New(sess.Copy(&aws.Config{Endpoint: aws.String(c.Endpoints["cognitoidentityprovider"])})),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bug discovered by the new acceptance testing. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

grahamlyons added a commit to mergermarket/tf_aws_secrets_access_policy that referenced this pull request Apr 10, 2019
This issue hashicorp/terraform-provider-aws#8226 prevents
us from setting the dummy endpoints for the tests so pin until it's
fixed.
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Nicely done on the updated acceptance tests. Have validation for each of the endpoints is a good call.
👍

@@ -968,6 +968,7 @@ func providerConfigure(d *schema.ResourceData) (interface{}, error) {
Profile: d.Get("profile").(string),
Token: d.Get("token").(string),
Region: d.Get("region").(string),
Endpoints: make(map[string]string),
Copy link
Contributor

Choose a reason for hiding this comment

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

✔️

})
}

func testAccCheckAWSProviderEndpoints(providers *[]*schema.Provider) resource.TestCheckFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function signature threw me for a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it is a little bizarre! Dealing with pointers here so we can instantiate the slice during runtime. See also: testAccCheckEbsSnapshotCopyExistsWithProviders

endpoint = "sfn"
}

switch name {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thought about recommending an if statement here, but the multiple || is too much, and I don't feel strongly enough to say use a regex match 💯

@nywilken
Copy link
Contributor

FWIW I tested against the reported issues and confirmed that the fix works as expected.

@bflad bflad merged commit 7a59511 into master Apr 10, 2019
@bflad bflad deleted the b-provider-endpoints-panic branch April 10, 2019 15:20
bflad added a commit that referenced this pull request Apr 10, 2019
@bflad
Copy link
Contributor Author

bflad commented Apr 11, 2019

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

@ghost
Copy link

ghost commented Mar 30, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. crash Results from or addresses a Terraform crash or kernel panic. provider Pertains to the provider itself, rather than any interaction with AWS. regression Pertains to a degraded workflow resulting from an upstream patch or internal enhancement. size/L Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
2 participants