From d9351bd35a1181cb57625b9740e78cae7bf23003 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Fri, 5 Apr 2019 22:15:35 -0400 Subject: [PATCH 1/4] provider: Prevent panic when setting endpoints configuration Reference: https://github.com/terraform-providers/terraform-provider-aws/issues/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/terraform@v0.11.14-0.20190329073242-44702fa6c163/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/terraform@v0.11.14-0.20190329073242-44702fa6c163/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/terraform@v0.11.14-0.20190329073242-44702fa6c163/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/terraform@v0.11.14-0.20190329073242-44702fa6c163/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/terraform@v0.11.14-0.20190329073242-44702fa6c163/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/terraform@v0.11.14-0.20190329073242-44702fa6c163/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/terraform@v0.11.14-0.20190329073242-44702fa6c163/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/terraform@v0.11.14-0.20190329073242-44702fa6c163/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/terraform@v0.11.14-0.20190329073242-44702fa6c163/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/terraform@v0.11.14-0.20190329073242-44702fa6c163/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/terraform@v0.11.14-0.20190329073242-44702fa6c163/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/terraform@v0.11.14-0.20190329073242-44702fa6c163/terraform/graph.go:126 +0xbc2 github.com/hashicorp/terraform/dag.(*Walker).walkVertex(0xc001431810, 0x571f240, 0xc00143e098, 0xc00063c180) /Users/bflad/go/pkg/mod/github.com/hashicorp/terraform@v0.11.14-0.20190329073242-44702fa6c163/dag/walk.go:387 +0x33b created by github.com/hashicorp/terraform/dag.(*Walker).Update /Users/bflad/go/pkg/mod/github.com/hashicorp/terraform@v0.11.14-0.20190329073242-44702fa6c163/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) ``` --- aws/provider.go | 1 + aws/provider_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/aws/provider.go b/aws/provider.go index 863114fcce6..68a03f329df 100644 --- a/aws/provider.go +++ b/aws/provider.go @@ -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), MaxRetries: d.Get("max_retries").(int), Insecure: d.Get("insecure").(bool), SkipCredsValidation: d.Get("skip_credentials_validation").(bool), diff --git a/aws/provider_test.go b/aws/provider_test.go index 514ffdddf40..7db18a1ba45 100644 --- a/aws/provider_test.go +++ b/aws/provider_test.go @@ -5,6 +5,7 @@ import ( "log" "os" "regexp" + "strings" "testing" "github.com/aws/aws-sdk-go/aws/arn" @@ -355,3 +356,35 @@ func testSweepSkipSweepError(err error) bool { } return false } + +func TestAccAWSProvider_Endpoints(t *testing.T) { + // Capture custom provider configuration so it + // does not interfere with other tests + var providers []*schema.Provider + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProviderFactories(&providers), + Steps: []resource.TestStep{ + { + Config: fmt.Sprintf(` +provider "aws" { + skip_credentials_validation = true + skip_get_ec2_platforms = true + skip_metadata_api_check = true + skip_requesting_account_id = true + + endpoints { +%[1]s = "http://localhost" + } +} + +# Required to initialize the provider +data "aws_arn" "test" { + arn = "arn:aws:s3:::test" +} +`, strings.Join(endpointServiceNames, " = \"http://localhost\"\n")), + }, + }, + }) +} From 67dbc792ca898ccf1afdbbe8ec93eeacac44abfe Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Tue, 9 Apr 2019 22:20:02 -0400 Subject: [PATCH 2/4] provider: Ensure cognitoidp endpoint configuration correctly overwrites 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) ``` --- aws/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/config.go b/aws/config.go index 65eea07dabf..f2ec54d2081 100644 --- a/aws/config.go +++ b/aws/config.go @@ -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"])})), + cognitoidpconn: cognitoidentityprovider.New(sess.Copy(&aws.Config{Endpoint: aws.String(c.Endpoints["cognitoidp"])})), configconn: configservice.New(sess.Copy(&aws.Config{Endpoint: aws.String(c.Endpoints["configservice"])})), costandusagereportconn: costandusagereportservice.New(sess.Copy(&aws.Config{Endpoint: aws.String(c.Endpoints["cur"])})), datapipelineconn: datapipeline.New(sess.Copy(&aws.Config{Endpoint: aws.String(c.Endpoints["datapipeline"])})), From d3eb7ff3a6a4468824f907d2b4eae703c457324c Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Tue, 9 Apr 2019 22:21:03 -0400 Subject: [PATCH 3/4] tests/provider: Add acceptance testing to ensure endpoint values are properly overwritten in client configurations Output from acceptance testing: ``` --- PASS: TestAccAWSProvider_Endpoints_Deprecated (1.88s) --- PASS: TestAccAWSProvider_Endpoints (1.90s) ``` --- aws/provider_test.go | 208 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 203 insertions(+), 5 deletions(-) diff --git a/aws/provider_test.go b/aws/provider_test.go index 7db18a1ba45..80d1ec617e3 100644 --- a/aws/provider_test.go +++ b/aws/provider_test.go @@ -4,6 +4,7 @@ import ( "fmt" "log" "os" + "reflect" "regexp" "strings" "testing" @@ -357,17 +358,203 @@ func testSweepSkipSweepError(err error) bool { return false } +func testAccCheckAWSProviderEndpoints(providers *[]*schema.Provider) resource.TestCheckFunc { + return func(s *terraform.State) error { + if providers == nil { + return fmt.Errorf("no providers initialized") + } + + // Match AWSClient struct field names to endpoint configuration names + endpointFieldNameF := func(endpoint string) func(string) bool { + return func(name string) bool { + switch endpoint { + case "applicationautoscaling": + endpoint = "appautoscaling" + case "budgets": + endpoint = "budget" + case "cloudformation": + endpoint = "cf" + case "cloudhsm": + endpoint = "cloudhsmv2" + case "cognitoidentity": + endpoint = "cognito" + case "configservice": + endpoint = "config" + case "cur": + endpoint = "costandusagereport" + case "directconnect": + endpoint = "dx" + case "lexmodels": + endpoint = "lexmodel" + case "route53": + endpoint = "r53" + case "sdb": + endpoint = "simpledb" + case "serverlessrepo": + endpoint = "serverlessapplicationrepository" + case "servicecatalog": + endpoint = "sc" + case "servicediscovery": + endpoint = "sd" + case "stepfunctions": + endpoint = "sfn" + } + + switch name { + case endpoint, fmt.Sprintf("%sconn", endpoint), fmt.Sprintf("%sConn", endpoint): + return true + } + + return false + } + } + + for _, provider := range *providers { + if provider == nil || provider.Meta() == nil || provider.Meta().(*AWSClient) == nil { + continue + } + + providerClient := provider.Meta().(*AWSClient) + + for _, endpointServiceName := range endpointServiceNames { + // Skip deprecated endpoint configurations as they will override expected values + if endpointServiceName == "kinesis_analytics" || endpointServiceName == "r53" { + continue + } + + providerClientField := reflect.Indirect(reflect.ValueOf(providerClient)).FieldByNameFunc(endpointFieldNameF(endpointServiceName)) + + if !providerClientField.IsValid() { + return fmt.Errorf("unable to match AWSClient struct field name for endpoint name: %s", endpointServiceName) + } + + actualEndpoint := reflect.Indirect(reflect.Indirect(providerClientField).FieldByName("Config").FieldByName("Endpoint")).String() + expectedEndpoint := fmt.Sprintf("http://%s", endpointServiceName) + + if actualEndpoint != expectedEndpoint { + return fmt.Errorf("expected endpoint (%s) value (%s), got: %s", endpointServiceName, expectedEndpoint, actualEndpoint) + } + } + } + + return nil + } +} + +func testAccCheckAWSProviderEndpointsDeprecated(providers *[]*schema.Provider) resource.TestCheckFunc { + return func(s *terraform.State) error { + if providers == nil { + return fmt.Errorf("no providers initialized") + } + + // Match AWSClient struct field names to endpoint configuration names + endpointFieldNameF := func(endpoint string) func(string) bool { + return func(name string) bool { + switch endpoint { + case "kinesis_analytics": + endpoint = "kinesisanalytics" + } + + return name == fmt.Sprintf("%sconn", endpoint) + } + } + + for _, provider := range *providers { + if provider == nil || provider.Meta() == nil || provider.Meta().(*AWSClient) == nil { + continue + } + + providerClient := provider.Meta().(*AWSClient) + + for _, endpointServiceName := range endpointServiceNames { + // Only check deprecated endpoint configurations + if endpointServiceName != "kinesis_analytics" && endpointServiceName != "r53" { + continue + } + + providerClientField := reflect.Indirect(reflect.ValueOf(providerClient)).FieldByNameFunc(endpointFieldNameF(endpointServiceName)) + + if !providerClientField.IsValid() { + return fmt.Errorf("unable to match AWSClient struct field name for endpoint name: %s", endpointServiceName) + } + + actualEndpoint := reflect.Indirect(reflect.Indirect(providerClientField).FieldByName("Config").FieldByName("Endpoint")).String() + expectedEndpoint := fmt.Sprintf("http://%s", endpointServiceName) + + if actualEndpoint != expectedEndpoint { + return fmt.Errorf("expected endpoint (%s) value (%s), got: %s", endpointServiceName, expectedEndpoint, actualEndpoint) + } + } + } + + return nil + } +} + func TestAccAWSProvider_Endpoints(t *testing.T) { - // Capture custom provider configuration so it - // does not interfere with other tests var providers []*schema.Provider + var endpoints strings.Builder + + // Initialize each endpoint configuration with matching name and value + for _, endpointServiceName := range endpointServiceNames { + // Skip deprecated endpoint configurations as they will override expected values + if endpointServiceName == "kinesis_analytics" || endpointServiceName == "r53" { + continue + } + + endpoints.WriteString(fmt.Sprintf("%s = \"http://%s\"\n", endpointServiceName, endpointServiceName)) + } + + endpointsConfig := func(endpoints string) string { + return fmt.Sprintf(` +provider "aws" { + skip_credentials_validation = true + skip_get_ec2_platforms = true + skip_metadata_api_check = true + skip_requesting_account_id = true + + endpoints { +%[1]s + } +} + +# Required to initialize the provider +data "aws_arn" "test" { + arn = "arn:aws:s3:::test" +} +`, endpoints) + } resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, ProviderFactories: testAccProviderFactories(&providers), Steps: []resource.TestStep{ { - Config: fmt.Sprintf(` + Config: endpointsConfig(endpoints.String()), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSProviderEndpoints(&providers), + ), + }, + }, + }) +} + +func TestAccAWSProvider_Endpoints_Deprecated(t *testing.T) { + var providers []*schema.Provider + var endpointsDeprecated strings.Builder + + // Initialize each deprecated endpoint configuration with matching name and value + for _, endpointServiceName := range endpointServiceNames { + // Only configure deprecated endpoint configurations + if endpointServiceName != "kinesis_analytics" && endpointServiceName != "r53" { + continue + } + + endpointsDeprecated.WriteString(fmt.Sprintf("%s = \"http://%s\"\n", endpointServiceName, endpointServiceName)) + } + + endpointsConfig := func(endpoints string) string { + return fmt.Sprintf(` provider "aws" { skip_credentials_validation = true skip_get_ec2_platforms = true @@ -375,7 +562,7 @@ provider "aws" { skip_requesting_account_id = true endpoints { -%[1]s = "http://localhost" +%[1]s } } @@ -383,7 +570,18 @@ provider "aws" { data "aws_arn" "test" { arn = "arn:aws:s3:::test" } -`, strings.Join(endpointServiceNames, " = \"http://localhost\"\n")), +`, endpoints) + } + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProviderFactories(&providers), + Steps: []resource.TestStep{ + { + Config: endpointsConfig(endpointsDeprecated.String()), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSProviderEndpointsDeprecated(&providers), + ), }, }, }) From 3af4c3834338b55c021d4da96bc467abafaf4df2 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Tue, 9 Apr 2019 22:30:06 -0400 Subject: [PATCH 4/4] tests/provider: Refactor endpoints acceptance testing to match style of resource acceptance testing Output from acceptance testing: ``` --- PASS: TestAccAWSProvider_Endpoints_Deprecated (1.75s) --- PASS: TestAccAWSProvider_Endpoints (1.77s) ``` --- aws/provider_test.go | 136 ++++++++++++++++++------------------------- 1 file changed, 58 insertions(+), 78 deletions(-) diff --git a/aws/provider_test.go b/aws/provider_test.go index 80d1ec617e3..dbd88a02cdf 100644 --- a/aws/provider_test.go +++ b/aws/provider_test.go @@ -358,6 +358,62 @@ func testSweepSkipSweepError(err error) bool { return false } +func TestAccAWSProvider_Endpoints(t *testing.T) { + var providers []*schema.Provider + var endpoints strings.Builder + + // Initialize each endpoint configuration with matching name and value + for _, endpointServiceName := range endpointServiceNames { + // Skip deprecated endpoint configurations as they will override expected values + if endpointServiceName == "kinesis_analytics" || endpointServiceName == "r53" { + continue + } + + endpoints.WriteString(fmt.Sprintf("%s = \"http://%s\"\n", endpointServiceName, endpointServiceName)) + } + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProviderFactories(&providers), + Steps: []resource.TestStep{ + { + Config: testAccAWSProviderConfigEndpoints(endpoints.String()), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSProviderEndpoints(&providers), + ), + }, + }, + }) +} + +func TestAccAWSProvider_Endpoints_Deprecated(t *testing.T) { + var providers []*schema.Provider + var endpointsDeprecated strings.Builder + + // Initialize each deprecated endpoint configuration with matching name and value + for _, endpointServiceName := range endpointServiceNames { + // Only configure deprecated endpoint configurations + if endpointServiceName != "kinesis_analytics" && endpointServiceName != "r53" { + continue + } + + endpointsDeprecated.WriteString(fmt.Sprintf("%s = \"http://%s\"\n", endpointServiceName, endpointServiceName)) + } + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProviderFactories(&providers), + Steps: []resource.TestStep{ + { + Config: testAccAWSProviderConfigEndpoints(endpointsDeprecated.String()), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSProviderEndpointsDeprecated(&providers), + ), + }, + }, + }) +} + func testAccCheckAWSProviderEndpoints(providers *[]*schema.Provider) resource.TestCheckFunc { return func(s *terraform.State) error { if providers == nil { @@ -491,70 +547,8 @@ func testAccCheckAWSProviderEndpointsDeprecated(providers *[]*schema.Provider) r } } -func TestAccAWSProvider_Endpoints(t *testing.T) { - var providers []*schema.Provider - var endpoints strings.Builder - - // Initialize each endpoint configuration with matching name and value - for _, endpointServiceName := range endpointServiceNames { - // Skip deprecated endpoint configurations as they will override expected values - if endpointServiceName == "kinesis_analytics" || endpointServiceName == "r53" { - continue - } - - endpoints.WriteString(fmt.Sprintf("%s = \"http://%s\"\n", endpointServiceName, endpointServiceName)) - } - - endpointsConfig := func(endpoints string) string { - return fmt.Sprintf(` -provider "aws" { - skip_credentials_validation = true - skip_get_ec2_platforms = true - skip_metadata_api_check = true - skip_requesting_account_id = true - - endpoints { -%[1]s - } -} - -# Required to initialize the provider -data "aws_arn" "test" { - arn = "arn:aws:s3:::test" -} -`, endpoints) - } - - resource.ParallelTest(t, resource.TestCase{ - PreCheck: func() { testAccPreCheck(t) }, - ProviderFactories: testAccProviderFactories(&providers), - Steps: []resource.TestStep{ - { - Config: endpointsConfig(endpoints.String()), - Check: resource.ComposeTestCheckFunc( - testAccCheckAWSProviderEndpoints(&providers), - ), - }, - }, - }) -} - -func TestAccAWSProvider_Endpoints_Deprecated(t *testing.T) { - var providers []*schema.Provider - var endpointsDeprecated strings.Builder - - // Initialize each deprecated endpoint configuration with matching name and value - for _, endpointServiceName := range endpointServiceNames { - // Only configure deprecated endpoint configurations - if endpointServiceName != "kinesis_analytics" && endpointServiceName != "r53" { - continue - } - - endpointsDeprecated.WriteString(fmt.Sprintf("%s = \"http://%s\"\n", endpointServiceName, endpointServiceName)) - } - - endpointsConfig := func(endpoints string) string { - return fmt.Sprintf(` +func testAccAWSProviderConfigEndpoints(endpoints string) string { + return fmt.Sprintf(` provider "aws" { skip_credentials_validation = true skip_get_ec2_platforms = true @@ -571,18 +565,4 @@ data "aws_arn" "test" { arn = "arn:aws:s3:::test" } `, endpoints) - } - - resource.ParallelTest(t, resource.TestCase{ - PreCheck: func() { testAccPreCheck(t) }, - ProviderFactories: testAccProviderFactories(&providers), - Steps: []resource.TestStep{ - { - Config: endpointsConfig(endpointsDeprecated.String()), - Check: resource.ComposeTestCheckFunc( - testAccCheckAWSProviderEndpointsDeprecated(&providers), - ), - }, - }, - }) }