From 86a8ede789e555047515afd50e4abc765d211c6b Mon Sep 17 00:00:00 2001 From: ampsingram Date: Thu, 20 Dec 2018 09:42:01 -0500 Subject: [PATCH 01/11] Add AWS Custom Endpoint capability #70588 Solves "Allow to override default AWS endpoint #70588" Add several new properties to AWS CloudConfig to support custom endpoints. Initialize/Parse on aws.go init() method which gets called when aws is loaded. Allows overridden endpoints per servce and region. This allows functionality on air gapped networks. This change is benign if services are not overridden in CloudConfig --- pkg/cloudprovider/providers/aws/aws.go | 142 +++++++++- pkg/cloudprovider/providers/aws/aws_test.go | 289 ++++++++++++++++++++ 2 files changed, 425 insertions(+), 6 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 97a6a4fd57..c8d8bf6e23 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -34,6 +34,7 @@ import ( "github.com/aws/aws-sdk-go/aws/credentials/ec2rolecreds" "github.com/aws/aws-sdk-go/aws/credentials/stscreds" "github.com/aws/aws-sdk-go/aws/ec2metadata" + "github.com/aws/aws-sdk-go/aws/endpoints" "github.com/aws/aws-sdk-go/aws/request" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/autoscaling" @@ -569,6 +570,126 @@ type CloudConfig struct { //yourself in an non-AWS cloud and open an issue, please indicate that in the //issue body. DisableStrictZoneCheck bool + + // Allows AWS endpoints to be overridden + // Useful in deployments to private edge nodes where amazonaws.com does not resolve + OverrideEndpoints bool + + // Delimiter to use to separate servicename from its configuration parameters + // NOTE: semi-colon ';' truncates the input line in INI files, do not use ';' + // Defaults "|" + ServicenameDelimiter string + + // Delimiter to use to separate region of occurrence, url and signing region for each override + // NOTE: semi-colon ';' truncates the input line in INI files, do not use ';' + // Defaults to "," + OverrideSeparator string + + // Delimiter to use to separate overridden services + // NOTE: semi-colon ';' truncates the input line in INI files, do not use ';' + // Defaults to "&" + ServiceDelimiter string + + // These are of format servicename ServicenameDelimiter url OverrideSeparator signing_region ServiceDelimiter nextservice + // s3|region1, https://s3.foo.bar, some signing_region & ec2|region1, https://ec2.foo.bar, signing_region + ServiceOverrides string + } +} + +const ( + ServicenameDelimiterDefault = "|" + ServicesDelimiterDefault = "&" + OverrideSeparatorDefault = "," +) + +type CustomEndpoint struct { + Endpoint string + SigningRegion string +} + +var overridesActive = false +var overrides map[string]CustomEndpoint + +func IsOverridesActive() bool { + return overridesActive +} + +func SetOverridesDefaults(cfg *CloudConfig) error { + if cfg.Global.OverrideEndpoints { + if cfg.Global.ServiceDelimiter == "" { + cfg.Global.ServiceDelimiter = ServicesDelimiterDefault + } else if cfg.Global.ServiceDelimiter == ";" { + return fmt.Errorf("semi-colon may not be used as a service delimiter, it truncates the input") + } + if cfg.Global.ServicenameDelimiter == "" { + cfg.Global.ServicenameDelimiter = ServicenameDelimiterDefault + } else if cfg.Global.ServicenameDelimiter == ";" { + return fmt.Errorf("semi-colon may not be used as a service name delimiter, it truncates the input") + } + if cfg.Global.OverrideSeparator == "" { + cfg.Global.OverrideSeparator = OverrideSeparatorDefault + } else if cfg.Global.OverrideSeparator == ";" { + return fmt.Errorf("semi-colon may not be used as a override separator, it truncates the input") + } + } + return nil +} + +func MakeRegionEndpointSignature(serviceName, region string) string { + return fmt.Sprintf("%s__%s", strings.TrimSpace(serviceName), strings.TrimSpace(region)) +} + +func ParseOverrides(cfg *CloudConfig) error { + if cfg.Global.OverrideEndpoints { + if err := SetOverridesDefaults(cfg); err != nil { + return err + } + overrides = make(map[string]CustomEndpoint) + allOverrides := strings.Split(cfg.Global.ServiceOverrides, cfg.Global.ServiceDelimiter) + for _, o := range allOverrides { + if idx := strings.Index(o, cfg.Global.ServicenameDelimiter); idx != -1 { + name := strings.TrimSpace(o[:idx]) + values := o[idx+1:] + tuple := strings.Split(values, cfg.Global.OverrideSeparator) + if len(tuple) != 3 { + return errors.New(fmt.Sprintf("3 parameters (region, url, signing region) are required for [%s] in %s", + name, o)) + } + signature := MakeRegionEndpointSignature(name, tuple[0]) + overrides[signature] = CustomEndpoint{Endpoint: strings.TrimSpace(tuple[1]), SigningRegion: strings.TrimSpace(tuple[2])} + } else { + cfg.Global.OverrideEndpoints = false + overridesActive = false + return errors.New(fmt.Sprintf("Unable to find ServicenameSeparator [%s] in %s", + cfg.Global.ServicenameDelimiter, o)) + } + } + overridesActive = true + } else { + overridesActive = false + } + return nil +} + +func loadCustomResolver() func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { + defaultResolver := endpoints.DefaultResolver() + defaultResolverFn := func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { + return defaultResolver.EndpointFor(service, region, optFns...) + } + if IsOverridesActive() { + customResolverFn := func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { + signature := MakeRegionEndpointSignature(service, region) + if ep, ok := overrides[signature]; ok { + return endpoints.ResolvedEndpoint{ + URL: ep.Endpoint, + SigningRegion: ep.SigningRegion, + }, nil + } + return defaultResolver.EndpointFor(service, region, optFns...) + } + return customResolverFn + } else { + return defaultResolverFn } } @@ -651,7 +772,8 @@ func (p *awsSDKProvider) Compute(regionName string) (EC2, error) { Region: ®ionName, Credentials: p.creds, } - awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true) + awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). + WithEndpointResolver(endpoints.ResolverFunc(loadCustomResolver())) sess, err := session.NewSession(awsConfig) if err != nil { @@ -672,7 +794,8 @@ func (p *awsSDKProvider) LoadBalancing(regionName string) (ELB, error) { Region: ®ionName, Credentials: p.creds, } - awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true) + awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). + WithEndpointResolver(endpoints.ResolverFunc(loadCustomResolver())) sess, err := session.NewSession(awsConfig) if err != nil { @@ -689,7 +812,8 @@ func (p *awsSDKProvider) LoadBalancingV2(regionName string) (ELBV2, error) { Region: ®ionName, Credentials: p.creds, } - awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true) + awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). + WithEndpointResolver(endpoints.ResolverFunc(loadCustomResolver())) sess, err := session.NewSession(awsConfig) if err != nil { @@ -707,7 +831,8 @@ func (p *awsSDKProvider) Autoscaling(regionName string) (ASG, error) { Region: ®ionName, Credentials: p.creds, } - awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true) + awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). + WithEndpointResolver(endpoints.ResolverFunc(loadCustomResolver())) sess, err := session.NewSession(awsConfig) if err != nil { @@ -721,7 +846,7 @@ func (p *awsSDKProvider) Autoscaling(regionName string) (ASG, error) { } func (p *awsSDKProvider) Metadata() (EC2Metadata, error) { - sess, err := session.NewSession(&aws.Config{}) + sess, err := session.NewSession(&aws.Config{EndpointResolver: endpoints.ResolverFunc(loadCustomResolver())}) if err != nil { return nil, fmt.Errorf("unable to initialize AWS session: %v", err) } @@ -735,7 +860,8 @@ func (p *awsSDKProvider) KeyManagement(regionName string) (KMS, error) { Region: ®ionName, Credentials: p.creds, } - awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true) + awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). + WithEndpointResolver(endpoints.ResolverFunc(loadCustomResolver())) sess, err := session.NewSession(awsConfig) if err != nil { @@ -960,6 +1086,10 @@ func init() { return nil, fmt.Errorf("unable to read AWS cloud provider config file: %v", err) } + if err = ParseOverrides(cfg); err != nil { + return nil, fmt.Errorf("unable to parse custom endpoint overrides: %v", err) + } + sess, err := session.NewSession(&aws.Config{}) if err != nil { return nil, fmt.Errorf("unable to initialize AWS session: %v", err) diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 4d12932859..6a3aefedab 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -186,6 +186,295 @@ func TestReadAWSCloudConfig(t *testing.T) { } } +func TestOverridesActiveConfig(t *testing.T) { + tests := []struct { + name string + + + reader io.Reader + aws Services + + expectError bool + active bool + servicesOverridden []string + regions []string + }{ + { + "No overrides in config", + strings.NewReader("[global]\nServiceOverrides=s3|sregion, https://s3.foo.bar, sregion"), + nil, + false, false, + []string{}, []string{}, + }, + { + "Missing Servicename Separator", + strings.NewReader("[global]\nOverrideEndpoints=true\nServiceOverrides=s3sregion, https://s3.foo.bar, sregion"), + nil, + true, false, + []string{}, []string{}, + }, + { + "Missing Service Region", + strings.NewReader("[global]\nOverrideEndpoints=true\nServiceOverrides=s3|https://s3.foo.bar, sregion"), + nil, + true, false, + []string{}, []string{}, + }, + { + "Semi-colon in service delimiter", + strings.NewReader("[global]\nOverrideEndpoints=true\nServiceDelimiter=;"), + nil, + true, false, + []string{}, []string{}, + }, + { + "Semi-colon in service name delimiter", + strings.NewReader("[global]\nOverrideEndpoints=true\nServicenameDelimiter=;"), + nil, + true, false, + []string{}, []string{}, + }, + { + "Semi-colon in service name delimiter", + strings.NewReader("[global]\nOverrideEndpoints=true\nOverrideSeparator=;"), + nil, + true, false, + []string{}, []string{}, + }, + { + "Active Overrides", + strings.NewReader("[global]\nOverrideEndpoints=true\nServiceOverrides=s3|sregion, https://s3.foo.bar, sregion"), + nil, + false, true, + []string{"s3"}, []string{"sregion"}, + }, + { + "Multiple Overriden Services", + strings.NewReader("[global]\nOverrideEndpoints=true\n" + + "ServiceOverrides=s3|sregion, https://s3.foo.bar, sregion & ec2|sregion, https://ec2.foo.bar, sregion"), + nil, + false, true, + []string{"s3", "ec2"}, []string{"sregion", "sregion"}, + }, + { + "Multiple Overriden Services in Multiple regions", + strings.NewReader("[global]\nOverrideEndpoints=true\n" + + "ServiceOverrides=s3|region1, https://s3.foo.bar, sregion & ec2|region2, https://ec2.foo.bar, sregion"), + nil, + false, true, + []string{"s3", "ec2"}, []string{"region1", "region2"}, + }, + { + "Multiple regions, Same Service", + strings.NewReader("[global]\nOverrideEndpoints=true\n" + + "ServiceOverrides=s3|region1, https://s3.foo.bar, sregion & s3|region2, https://s3.foo.bar, sregion"), + nil, + false, true, + []string{"s3", "s3"}, []string{"region1", "region2"}, + }, + } + + for _, test := range tests { + t.Logf("Running test case %s", test.name) + cfg, err := readAWSCloudConfig(test.reader) + if err == nil { + err = ParseOverrides(cfg) + } + if test.expectError { + if err == nil { + t.Errorf("Should error for case %s (cfg=%v)", test.name, cfg) + } + if IsOverridesActive() != test.active { + t.Errorf("Incorrect active flag (%v vs %v) for case: %s", + IsOverridesActive(), test.active, test.name) + } + } else { + if err != nil { + t.Errorf("Should succeed for case: %s", test.name) + } + if IsOverridesActive() != test.active { + t.Errorf("Incorrect active flag (%v vs %v) for case: %s", + IsOverridesActive(), test.active, test.name) + } + if len(overrides) != len(test.servicesOverridden) { + t.Errorf("Expected %d overridden services, received %d for case %s", + len(test.servicesOverridden), len(overrides), test.name) + } else { + for i, name := range test.servicesOverridden { + signature := MakeRegionEndpointSignature(name, test.regions[i]) + ep, ok := overrides[signature] + if !ok { + t.Errorf("Missing override for service %s in case %s", + name, test.name) + } else { + if ep.SigningRegion != "sregion" { + t.Errorf("Expected signing region 'sregion', received '%s' for case %s", + ep.SigningRegion, test.name) + } + targetName := fmt.Sprintf("https://%s.foo.bar", name) + if ep.Endpoint != targetName { + t.Errorf("Expected Endpoint '%s', received '%s' for case %s", + targetName, ep.Endpoint, test.name) + } + + fn := loadCustomResolver() + ep1, e := fn(name, test.regions[i], nil) + if e != nil { + t.Errorf("Expected a valid endpoint for %s in case %s", + name, test.name) + } else { + targetName := fmt.Sprintf("https://%s.foo.bar", name) + if ep1.URL != targetName { + t.Errorf("Expected endpoint url: %s, received %s in case %s", + targetName, ep1.URL, test.name) + } + if ep1.SigningRegion != "sregion" { + t.Errorf("Expected signing region 'sregion', received '%s' in case %s", + ep1.SigningRegion, test.name) + } + } + } + } + } + } + } +} + +func TestOverridesDefaults(t *testing.T) { + tests := []struct { + name string + + configString string + + expectError bool + active bool + servicesOverridden []string + defaults []string + }{ + { + "Bad Servicename Delimiter", + "[global]\nOverrideEndpoints=true\n" + + "ServiceOverrides=s3|sregion, https://s3.foo.bar, sregion\n" + + "ServicenameDelimiter=?", + true, false, + []string{}, + []string{";", "?", ","}, + }, + { + "Custom ServicenameDelimiter", + "[global]\nOverrideEndpoints=true\n" + + "ServiceOverrides=s3?sregion, https://s3.foo.bar, sregion\n" + + "ServicenameDelimiter=?", + false, true, + []string{"s3"}, + []string{"&", "?", ","}, + }, + { + "Custom OverrideSeparator", + "[global]\nOverrideEndpoints=true\n" + + "ServiceOverrides=s3|sregion + https://s3.foo.bar + sregion \n" + + "OverrideSeparator=+", + false, true, + []string{"s3"}, + []string{"&", "|", "+"}, + }, + { + "Custom Services Delimiter", + "[global]\nOverrideEndpoints=true\n" + + "ServiceOverrides=s3|sregion, https://s3.foo.bar, sregion + ec2|sregion, https://ec2.foo.bar , sregion\n" + + "ServiceDelimiter=+", + false, true, + []string{"s3", "ec2"}, + []string{"+", "|", ","}, + }, + { + "Active Overrides", + "[global]\nOverrideEndpoints=true\n" + + "ServiceOverrides=s3|sregion, https://s3.foo.bar , sregion & ec2|sregion, https://ec2.foo.bar, sregion", + false, true, + []string{"s3", "ec2"}, + []string{"&", "|", ","}, + }, + } + + for _, test := range tests { + t.Logf("Running test case %s", test.name) + cfg, err := readAWSCloudConfig(strings.NewReader(test.configString)) + if err == nil { + err = ParseOverrides(cfg) + } + if test.expectError { + if err == nil { + t.Errorf("Should error for case %s (cfg=%v)", test.name, cfg) + } + if IsOverridesActive() != test.active { + t.Errorf("Incorrect active flag (%v vs %v) for case: %s", + IsOverridesActive(), test.active, test.name) + } + } else { + if err != nil { + t.Errorf("Should succeed for case: %s", test.name) + } + if IsOverridesActive() != test.active { + t.Errorf("Incorrect active flag (%v vs %v) for case: %s", + IsOverridesActive(), test.active, test.name) + } + if cfg.Global.ServiceDelimiter != test.defaults[0] { + t.Errorf("Incorrect ServiceDelimter (%s vs %s) for case %s", + cfg.Global.ServiceDelimiter, test.defaults[0], test.name) + } + if cfg.Global.ServicenameDelimiter != test.defaults[1] { + t.Errorf("Incorrect ServicenameDelimiter (%s vs %s) for case %s", + cfg.Global.ServicenameDelimiter, test.defaults[1], test.name) + } + if cfg.Global.OverrideSeparator != test.defaults[2] { + t.Errorf("Incorrect OverrideSeparator (%s vs %s) for case %s", + cfg.Global.OverrideSeparator, test.defaults[2], test.name) + } + if len(overrides) != len(test.servicesOverridden) { + t.Errorf("Expected %d overridden services, received %d for case %s", + len(test.servicesOverridden), len(overrides), test.name) + } else { + for _, name := range test.servicesOverridden { + signature := MakeRegionEndpointSignature(name, "sregion") + ep, ok := overrides[signature] + if !ok { + t.Errorf("Missing override for service %s in case %s", + name, test.name) + } else { + if ep.SigningRegion != "sregion" { + t.Errorf("Expected signing region 'sregion', received '%s' for case %s", + ep.SigningRegion, test.name) + } + targetName := fmt.Sprintf("https://%s.foo.bar", name) + if ep.Endpoint != targetName { + t.Errorf("Expected Endpoint '%s', received '%s' for case %s", + targetName, ep.Endpoint, test.name) + } + + fn := loadCustomResolver() + ep1, e := fn(name, "sregion", nil) + if e != nil { + t.Errorf("Expected a valid endpoint for %s in case %s", + name, test.name) + } else { + targetName := fmt.Sprintf("https://%s.foo.bar", name) + if ep1.URL != targetName { + t.Errorf("Expected endpoint url: %s, received %s in case %s", + targetName, ep1.URL, test.name) + } + if ep1.SigningRegion != "sregion" { + t.Errorf("Expected signing region 'sregion', received '%s' in case %s", + ep1.SigningRegion, test.name) + } + } + } + } + } + } + } +} + func TestNewAWSCloud(t *testing.T) { tests := []struct { name string From 207a5a1267016dba723fd3820033a429643cea49 Mon Sep 17 00:00:00 2001 From: ampsingram Date: Wed, 16 Jan 2019 18:12:49 -0500 Subject: [PATCH 02/11] Updates for PR comments --- pkg/cloudprovider/providers/aws/aws.go | 106 +++----- pkg/cloudprovider/providers/aws/aws_test.go | 278 ++++++++------------ 2 files changed, 153 insertions(+), 231 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index c8d8bf6e23..9cde24c463 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -43,7 +43,7 @@ import ( "github.com/aws/aws-sdk-go/service/elbv2" "github.com/aws/aws-sdk-go/service/kms" "github.com/aws/aws-sdk-go/service/sts" - gcfg "gopkg.in/gcfg.v1" + "gopkg.in/gcfg.v1" "k8s.io/klog" "k8s.io/api/core/v1" @@ -56,7 +56,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" v1core "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/tools/record" - cloudprovider "k8s.io/cloud-provider" + "k8s.io/cloud-provider" "k8s.io/kubernetes/pkg/api/v1/service" "k8s.io/kubernetes/pkg/controller" kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis" @@ -571,35 +571,20 @@ type CloudConfig struct { //issue body. DisableStrictZoneCheck bool - // Allows AWS endpoints to be overridden - // Useful in deployments to private edge nodes where amazonaws.com does not resolve - OverrideEndpoints bool - - // Delimiter to use to separate servicename from its configuration parameters - // NOTE: semi-colon ';' truncates the input line in INI files, do not use ';' - // Defaults "|" - ServicenameDelimiter string - // Delimiter to use to separate region of occurrence, url and signing region for each override // NOTE: semi-colon ';' truncates the input line in INI files, do not use ';' // Defaults to "," OverrideSeparator string - // Delimiter to use to separate overridden services - // NOTE: semi-colon ';' truncates the input line in INI files, do not use ';' - // Defaults to "&" - ServiceDelimiter string - - // These are of format servicename ServicenameDelimiter url OverrideSeparator signing_region ServiceDelimiter nextservice - // s3|region1, https://s3.foo.bar, some signing_region & ec2|region1, https://ec2.foo.bar, signing_region - ServiceOverrides string + // These are of format servicename, region, url, signing_region + // s3, region1, https://s3.foo.bar, some signing_region + // ec2 region1, https://ec2.foo.bar, signing_region + ServiceOverrides []string } } const ( - ServicenameDelimiterDefault = "|" - ServicesDelimiterDefault = "&" - OverrideSeparatorDefault = "," + OverrideSeparatorDefault = "," ) type CustomEndpoint struct { @@ -610,59 +595,41 @@ type CustomEndpoint struct { var overridesActive = false var overrides map[string]CustomEndpoint -func IsOverridesActive() bool { - return overridesActive -} - -func SetOverridesDefaults(cfg *CloudConfig) error { - if cfg.Global.OverrideEndpoints { - if cfg.Global.ServiceDelimiter == "" { - cfg.Global.ServiceDelimiter = ServicesDelimiterDefault - } else if cfg.Global.ServiceDelimiter == ";" { - return fmt.Errorf("semi-colon may not be used as a service delimiter, it truncates the input") - } - if cfg.Global.ServicenameDelimiter == "" { - cfg.Global.ServicenameDelimiter = ServicenameDelimiterDefault - } else if cfg.Global.ServicenameDelimiter == ";" { - return fmt.Errorf("semi-colon may not be used as a service name delimiter, it truncates the input") - } - if cfg.Global.OverrideSeparator == "" { - cfg.Global.OverrideSeparator = OverrideSeparatorDefault - } else if cfg.Global.OverrideSeparator == ";" { - return fmt.Errorf("semi-colon may not be used as a override separator, it truncates the input") - } +func setOverridesDefaults(cfg *CloudConfig) error { + if cfg.Global.OverrideSeparator == "" { + cfg.Global.OverrideSeparator = OverrideSeparatorDefault + } else if cfg.Global.OverrideSeparator == ";" { + return fmt.Errorf("semi-colon may not be used as a override separator, it truncates the input") } return nil } -func MakeRegionEndpointSignature(serviceName, region string) string { +func makeRegionEndpointSignature(serviceName, region string) string { return fmt.Sprintf("%s__%s", strings.TrimSpace(serviceName), strings.TrimSpace(region)) } -func ParseOverrides(cfg *CloudConfig) error { - if cfg.Global.OverrideEndpoints { - if err := SetOverridesDefaults(cfg); err != nil { +func parseOverrides(cfg *CloudConfig) error { + if len(cfg.Global.ServiceOverrides) > 0 { + if err := setOverridesDefaults(cfg); err != nil { return err } overrides = make(map[string]CustomEndpoint) - allOverrides := strings.Split(cfg.Global.ServiceOverrides, cfg.Global.ServiceDelimiter) - for _, o := range allOverrides { - if idx := strings.Index(o, cfg.Global.ServicenameDelimiter); idx != -1 { - name := strings.TrimSpace(o[:idx]) - values := o[idx+1:] - tuple := strings.Split(values, cfg.Global.OverrideSeparator) - if len(tuple) != 3 { - return errors.New(fmt.Sprintf("3 parameters (region, url, signing region) are required for [%s] in %s", - name, o)) + for _, ovrd := range cfg.Global.ServiceOverrides { + tokens := strings.Split(ovrd, cfg.Global.OverrideSeparator) + if len(tokens) != 4 { + if len(tokens) > 0 { + return fmt.Errorf("4 parameters (service, region, url, signing region) are required for [%s] in %s", + tokens[0], ovrd) } - signature := MakeRegionEndpointSignature(name, tuple[0]) - overrides[signature] = CustomEndpoint{Endpoint: strings.TrimSpace(tuple[1]), SigningRegion: strings.TrimSpace(tuple[2])} - } else { - cfg.Global.OverrideEndpoints = false - overridesActive = false - return errors.New(fmt.Sprintf("Unable to find ServicenameSeparator [%s] in %s", - cfg.Global.ServicenameDelimiter, o)) + return fmt.Errorf("4 parameters (service, region, url, signing region) are required in %s", + ovrd) } + name := strings.TrimSpace(tokens[0]) + region := strings.TrimSpace(tokens[1]) + url := strings.TrimSpace(tokens[2]) + signingRegion := strings.TrimSpace(tokens[3]) + signature := makeRegionEndpointSignature(name, region) + overrides[signature] = CustomEndpoint{Endpoint: url, SigningRegion: signingRegion} } overridesActive = true } else { @@ -676,9 +643,9 @@ func loadCustomResolver() func(service, region string, optFns ...func(*endpoints defaultResolverFn := func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { return defaultResolver.EndpointFor(service, region, optFns...) } - if IsOverridesActive() { + if overridesActive { customResolverFn := func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { - signature := MakeRegionEndpointSignature(service, region) + signature := makeRegionEndpointSignature(service, region) if ep, ok := overrides[signature]; ok { return endpoints.ResolvedEndpoint{ URL: ep.Endpoint, @@ -688,9 +655,8 @@ func loadCustomResolver() func(service, region string, optFns ...func(*endpoints return defaultResolver.EndpointFor(service, region, optFns...) } return customResolverFn - } else { - return defaultResolverFn } + return defaultResolverFn } // awsSdkEC2 is an implementation of the EC2 interface, backed by aws-sdk-go @@ -846,7 +812,9 @@ func (p *awsSDKProvider) Autoscaling(regionName string) (ASG, error) { } func (p *awsSDKProvider) Metadata() (EC2Metadata, error) { - sess, err := session.NewSession(&aws.Config{EndpointResolver: endpoints.ResolverFunc(loadCustomResolver())}) + sess, err := session.NewSession(&aws.Config{ + EndpointResolver: endpoints.ResolverFunc(loadCustomResolver()), + }) if err != nil { return nil, fmt.Errorf("unable to initialize AWS session: %v", err) } @@ -1086,7 +1054,7 @@ func init() { return nil, fmt.Errorf("unable to read AWS cloud provider config file: %v", err) } - if err = ParseOverrides(cfg); err != nil { + if err = parseOverrides(cfg); err != nil { return nil, fmt.Errorf("unable to parse custom endpoint overrides: %v", err) } diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 6a3aefedab..3d70088405 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -186,91 +186,77 @@ func TestReadAWSCloudConfig(t *testing.T) { } } +type ServiceDescriptor struct { + name string + region string +} + func TestOverridesActiveConfig(t *testing.T) { tests := []struct { name string - reader io.Reader aws Services expectError bool active bool - servicesOverridden []string - regions []string + servicesOverridden []ServiceDescriptor }{ - { - "No overrides in config", - strings.NewReader("[global]\nServiceOverrides=s3|sregion, https://s3.foo.bar, sregion"), - nil, - false, false, - []string{}, []string{}, - }, { "Missing Servicename Separator", - strings.NewReader("[global]\nOverrideEndpoints=true\nServiceOverrides=s3sregion, https://s3.foo.bar, sregion"), + strings.NewReader("[global]\nServiceOverrides=s3sregion, https://s3.foo.bar, sregion"), nil, true, false, - []string{}, []string{}, + []ServiceDescriptor{}, }, { "Missing Service Region", - strings.NewReader("[global]\nOverrideEndpoints=true\nServiceOverrides=s3|https://s3.foo.bar, sregion"), + strings.NewReader("[global]\nServiceOverrides=s3, https://s3.foo.bar, sregion"), nil, true, false, - []string{}, []string{}, + []ServiceDescriptor{}, }, { - "Semi-colon in service delimiter", - strings.NewReader("[global]\nOverrideEndpoints=true\nServiceDelimiter=;"), + "Semi-colon in override delimiter", + strings.NewReader("[global]\nOverrideSeparator=;\n" + + "ServiceOverrides=s3, https://s3.foo.bar, sregion"), nil, true, false, - []string{}, []string{}, - }, - { - "Semi-colon in service name delimiter", - strings.NewReader("[global]\nOverrideEndpoints=true\nServicenameDelimiter=;"), - nil, - true, false, - []string{}, []string{}, - }, - { - "Semi-colon in service name delimiter", - strings.NewReader("[global]\nOverrideEndpoints=true\nOverrideSeparator=;"), - nil, - true, false, - []string{}, []string{}, + []ServiceDescriptor{}, }, { "Active Overrides", - strings.NewReader("[global]\nOverrideEndpoints=true\nServiceOverrides=s3|sregion, https://s3.foo.bar, sregion"), + strings.NewReader("[global]\nServiceOverrides=s3, sregion, https://s3.foo.bar, sregion"), nil, false, true, - []string{"s3"}, []string{"sregion"}, + []ServiceDescriptor{{name: "s3", region: "sregion"}}, }, { "Multiple Overriden Services", - strings.NewReader("[global]\nOverrideEndpoints=true\n" + - "ServiceOverrides=s3|sregion, https://s3.foo.bar, sregion & ec2|sregion, https://ec2.foo.bar, sregion"), + strings.NewReader("[global]\n" + + "ServiceOverrides=s3, sregion1, https://s3.foo.bar, sregion\n" + + "ServiceOverrides=ec2, sregion2, https://ec2.foo.bar, sregion"), nil, false, true, - []string{"s3", "ec2"}, []string{"sregion", "sregion"}, + []ServiceDescriptor{{"s3", "sregion1"}, {"ec2", "sregion2"}}, }, { "Multiple Overriden Services in Multiple regions", - strings.NewReader("[global]\nOverrideEndpoints=true\n" + - "ServiceOverrides=s3|region1, https://s3.foo.bar, sregion & ec2|region2, https://ec2.foo.bar, sregion"), + strings.NewReader("[global]\n" + + "ServiceOverrides=s3, region1, https://s3.foo.bar, sregion\n" + + "ServiceOverrides=ec2, region2, https://ec2.foo.bar, sregion"), nil, false, true, - []string{"s3", "ec2"}, []string{"region1", "region2"}, + []ServiceDescriptor{{"s3","region1"}, {"ec2", "region2"}}, }, { "Multiple regions, Same Service", - strings.NewReader("[global]\nOverrideEndpoints=true\n" + - "ServiceOverrides=s3|region1, https://s3.foo.bar, sregion & s3|region2, https://s3.foo.bar, sregion"), + strings.NewReader("[global]\n" + + "ServiceOverrides=s3, region1, https://s3.foo.bar, sregion\n" + + "ServiceOverrides=s3, region2, https://s3.foo.bar, sregion"), nil, false, true, - []string{"s3", "s3"}, []string{"region1", "region2"}, + []ServiceDescriptor{{"s3", "region1"}, {"s3", "region2"}}, }, } @@ -278,59 +264,60 @@ func TestOverridesActiveConfig(t *testing.T) { t.Logf("Running test case %s", test.name) cfg, err := readAWSCloudConfig(test.reader) if err == nil { - err = ParseOverrides(cfg) + err = parseOverrides(cfg) } if test.expectError { if err == nil { t.Errorf("Should error for case %s (cfg=%v)", test.name, cfg) } - if IsOverridesActive() != test.active { + if overridesActive != test.active { t.Errorf("Incorrect active flag (%v vs %v) for case: %s", - IsOverridesActive(), test.active, test.name) + overridesActive, test.active, test.name) } } else { if err != nil { t.Errorf("Should succeed for case: %s", test.name) } - if IsOverridesActive() != test.active { + if overridesActive != test.active { t.Errorf("Incorrect active flag (%v vs %v) for case: %s", - IsOverridesActive(), test.active, test.name) - } - if len(overrides) != len(test.servicesOverridden) { - t.Errorf("Expected %d overridden services, received %d for case %s", - len(test.servicesOverridden), len(overrides), test.name) + overridesActive, test.active, test.name) } else { - for i, name := range test.servicesOverridden { - signature := MakeRegionEndpointSignature(name, test.regions[i]) - ep, ok := overrides[signature] - if !ok { - t.Errorf("Missing override for service %s in case %s", - name, test.name) - } else { - if ep.SigningRegion != "sregion" { - t.Errorf("Expected signing region 'sregion', received '%s' for case %s", - ep.SigningRegion, test.name) - } - targetName := fmt.Sprintf("https://%s.foo.bar", name) - if ep.Endpoint != targetName { - t.Errorf("Expected Endpoint '%s', received '%s' for case %s", - targetName, ep.Endpoint, test.name) - } - - fn := loadCustomResolver() - ep1, e := fn(name, test.regions[i], nil) - if e != nil { - t.Errorf("Expected a valid endpoint for %s in case %s", - name, test.name) + if len(overrides) != len(test.servicesOverridden) { + t.Errorf("Expected %d overridden services, received %d for case %s", + len(test.servicesOverridden), len(overrides), test.name) + } else { + for _, sd := range test.servicesOverridden { + signature := makeRegionEndpointSignature(sd.name, sd.region) + ep, ok := overrides[signature] + if !ok { + t.Errorf("Missing override for service %s in case %s", + sd.name, test.name) } else { - targetName := fmt.Sprintf("https://%s.foo.bar", name) - if ep1.URL != targetName { - t.Errorf("Expected endpoint url: %s, received %s in case %s", - targetName, ep1.URL, test.name) + if ep.SigningRegion != "sregion" { + t.Errorf("Expected signing region 'sregion', received '%s' for case %s", + ep.SigningRegion, test.name) } - if ep1.SigningRegion != "sregion" { - t.Errorf("Expected signing region 'sregion', received '%s' in case %s", - ep1.SigningRegion, test.name) + targetName := fmt.Sprintf("https://%s.foo.bar", sd.name) + if ep.Endpoint != targetName { + t.Errorf("Expected Endpoint '%s', received '%s' for case %s", + targetName, ep.Endpoint, test.name) + } + + fn := loadCustomResolver() + ep1, e := fn(sd.name, sd.region, nil) + if e != nil { + t.Errorf("Expected a valid endpoint for %s in case %s", + sd.name, test.name) + } else { + targetName := fmt.Sprintf("https://%s.foo.bar", sd.name) + if ep1.URL != targetName { + t.Errorf("Expected endpoint url: %s, received %s in case %s", + targetName, ep1.URL, test.name) + } + if ep1.SigningRegion != "sregion" { + t.Errorf("Expected signing region 'sregion', received '%s' in case %s", + ep1.SigningRegion, test.name) + } } } } @@ -351,49 +338,23 @@ func TestOverridesDefaults(t *testing.T) { servicesOverridden []string defaults []string }{ - { - "Bad Servicename Delimiter", - "[global]\nOverrideEndpoints=true\n" + - "ServiceOverrides=s3|sregion, https://s3.foo.bar, sregion\n" + - "ServicenameDelimiter=?", - true, false, - []string{}, - []string{";", "?", ","}, - }, - { - "Custom ServicenameDelimiter", - "[global]\nOverrideEndpoints=true\n" + - "ServiceOverrides=s3?sregion, https://s3.foo.bar, sregion\n" + - "ServicenameDelimiter=?", - false, true, - []string{"s3"}, - []string{"&", "?", ","}, - }, { "Custom OverrideSeparator", - "[global]\nOverrideEndpoints=true\n" + - "ServiceOverrides=s3|sregion + https://s3.foo.bar + sregion \n" + + "[global]\n" + + "ServiceOverrides=s3 + sregion + https://s3.foo.bar + sregion \n" + "OverrideSeparator=+", false, true, []string{"s3"}, - []string{"&", "|", "+"}, - }, - { - "Custom Services Delimiter", - "[global]\nOverrideEndpoints=true\n" + - "ServiceOverrides=s3|sregion, https://s3.foo.bar, sregion + ec2|sregion, https://ec2.foo.bar , sregion\n" + - "ServiceDelimiter=+", - false, true, - []string{"s3", "ec2"}, - []string{"+", "|", ","}, + []string{"+"}, }, { "Active Overrides", - "[global]\nOverrideEndpoints=true\n" + - "ServiceOverrides=s3|sregion, https://s3.foo.bar , sregion & ec2|sregion, https://ec2.foo.bar, sregion", + "[global]\n" + + "ServiceOverrides=s3, sregion, https://s3.foo.bar , sregion\n" + + "ServiceOverrides=ec2, sregion, https://ec2.foo.bar, sregion", false, true, []string{"s3", "ec2"}, - []string{"&", "|", ","}, + []string{","}, }, } @@ -401,71 +362,64 @@ func TestOverridesDefaults(t *testing.T) { t.Logf("Running test case %s", test.name) cfg, err := readAWSCloudConfig(strings.NewReader(test.configString)) if err == nil { - err = ParseOverrides(cfg) + err = parseOverrides(cfg) } if test.expectError { if err == nil { t.Errorf("Should error for case %s (cfg=%v)", test.name, cfg) } - if IsOverridesActive() != test.active { + if overridesActive != test.active { t.Errorf("Incorrect active flag (%v vs %v) for case: %s", - IsOverridesActive(), test.active, test.name) + overridesActive, test.active, test.name) } } else { if err != nil { t.Errorf("Should succeed for case: %s", test.name) } - if IsOverridesActive() != test.active { + if overridesActive != test.active { t.Errorf("Incorrect active flag (%v vs %v) for case: %s", - IsOverridesActive(), test.active, test.name) - } - if cfg.Global.ServiceDelimiter != test.defaults[0] { - t.Errorf("Incorrect ServiceDelimter (%s vs %s) for case %s", - cfg.Global.ServiceDelimiter, test.defaults[0], test.name) - } - if cfg.Global.ServicenameDelimiter != test.defaults[1] { - t.Errorf("Incorrect ServicenameDelimiter (%s vs %s) for case %s", - cfg.Global.ServicenameDelimiter, test.defaults[1], test.name) - } - if cfg.Global.OverrideSeparator != test.defaults[2] { - t.Errorf("Incorrect OverrideSeparator (%s vs %s) for case %s", - cfg.Global.OverrideSeparator, test.defaults[2], test.name) - } - if len(overrides) != len(test.servicesOverridden) { - t.Errorf("Expected %d overridden services, received %d for case %s", - len(test.servicesOverridden), len(overrides), test.name) + overridesActive, test.active, test.name) } else { - for _, name := range test.servicesOverridden { - signature := MakeRegionEndpointSignature(name, "sregion") - ep, ok := overrides[signature] - if !ok { - t.Errorf("Missing override for service %s in case %s", - name, test.name) - } else { - if ep.SigningRegion != "sregion" { - t.Errorf("Expected signing region 'sregion', received '%s' for case %s", - ep.SigningRegion, test.name) - } - targetName := fmt.Sprintf("https://%s.foo.bar", name) - if ep.Endpoint != targetName { - t.Errorf("Expected Endpoint '%s', received '%s' for case %s", - targetName, ep.Endpoint, test.name) - } - - fn := loadCustomResolver() - ep1, e := fn(name, "sregion", nil) - if e != nil { - t.Errorf("Expected a valid endpoint for %s in case %s", + if cfg.Global.OverrideSeparator != test.defaults[0] { + t.Errorf("Incorrect OverrideSeparator (%s vs %s) for case %s", + cfg.Global.OverrideSeparator, test.defaults[0], test.name) + } + if len(overrides) != len(test.servicesOverridden) { + t.Errorf("Expected %d overridden services, received %d for case %s", + len(test.servicesOverridden), len(overrides), test.name) + } else { + for _, name := range test.servicesOverridden { + signature := makeRegionEndpointSignature(name, "sregion") + ep, ok := overrides[signature] + if !ok { + t.Errorf("Missing override for service %s in case %s", name, test.name) } else { - targetName := fmt.Sprintf("https://%s.foo.bar", name) - if ep1.URL != targetName { - t.Errorf("Expected endpoint url: %s, received %s in case %s", - targetName, ep1.URL, test.name) + if ep.SigningRegion != "sregion" { + t.Errorf("Expected signing region 'sregion', received '%s' for case %s", + ep.SigningRegion, test.name) } - if ep1.SigningRegion != "sregion" { - t.Errorf("Expected signing region 'sregion', received '%s' in case %s", - ep1.SigningRegion, test.name) + targetName := fmt.Sprintf("https://%s.foo.bar", name) + if ep.Endpoint != targetName { + t.Errorf("Expected Endpoint '%s', received '%s' for case %s", + targetName, ep.Endpoint, test.name) + } + + fn := loadCustomResolver() + ep1, e := fn(name, "sregion", nil) + if e != nil { + t.Errorf("Expected a valid endpoint for %s in case %s", + name, test.name) + } else { + targetName := fmt.Sprintf("https://%s.foo.bar", name) + if ep1.URL != targetName { + t.Errorf("Expected endpoint url: %s, received %s in case %s", + targetName, ep1.URL, test.name) + } + if ep1.SigningRegion != "sregion" { + t.Errorf("Expected signing region 'sregion', received '%s' in case %s", + ep1.SigningRegion, test.name) + } } } } From 2a6ed9a69815a2031881f4f0c11dfb5a750a4369 Mon Sep 17 00:00:00 2001 From: ampsingram Date: Wed, 16 Jan 2019 18:18:16 -0500 Subject: [PATCH 03/11] Improvement suggested by PR comment Fail early makes the code more readable --- pkg/cloudprovider/providers/aws/aws.go | 50 +++++++++++++------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 9cde24c463..86fcf0edd1 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -609,32 +609,32 @@ func makeRegionEndpointSignature(serviceName, region string) string { } func parseOverrides(cfg *CloudConfig) error { - if len(cfg.Global.ServiceOverrides) > 0 { - if err := setOverridesDefaults(cfg); err != nil { - return err - } - overrides = make(map[string]CustomEndpoint) - for _, ovrd := range cfg.Global.ServiceOverrides { - tokens := strings.Split(ovrd, cfg.Global.OverrideSeparator) - if len(tokens) != 4 { - if len(tokens) > 0 { - return fmt.Errorf("4 parameters (service, region, url, signing region) are required for [%s] in %s", - tokens[0], ovrd) - } - return fmt.Errorf("4 parameters (service, region, url, signing region) are required in %s", - ovrd) - } - name := strings.TrimSpace(tokens[0]) - region := strings.TrimSpace(tokens[1]) - url := strings.TrimSpace(tokens[2]) - signingRegion := strings.TrimSpace(tokens[3]) - signature := makeRegionEndpointSignature(name, region) - overrides[signature] = CustomEndpoint{Endpoint: url, SigningRegion: signingRegion} - } - overridesActive = true - } else { - overridesActive = false + overridesActive = false + if len(cfg.Global.ServiceOverrides) == 0 { + return nil } + if err := setOverridesDefaults(cfg); err != nil { + return err + } + overrides = make(map[string]CustomEndpoint) + for _, ovrd := range cfg.Global.ServiceOverrides { + tokens := strings.Split(ovrd, cfg.Global.OverrideSeparator) + if len(tokens) != 4 { + if len(tokens) > 0 { + return fmt.Errorf("4 parameters (service, region, url, signing region) are required for [%s] in %s", + tokens[0], ovrd) + } + return fmt.Errorf("4 parameters (service, region, url, signing region) are required in %s", + ovrd) + } + name := strings.TrimSpace(tokens[0]) + region := strings.TrimSpace(tokens[1]) + url := strings.TrimSpace(tokens[2]) + signingRegion := strings.TrimSpace(tokens[3]) + signature := makeRegionEndpointSignature(name, region) + overrides[signature] = CustomEndpoint{Endpoint: url, SigningRegion: signingRegion} + } + overridesActive = true return nil } From 6f60d57dab51b132b8d2ce7840492accf959ef69 Mon Sep 17 00:00:00 2001 From: ampsingram Date: Wed, 16 Jan 2019 18:47:09 -0500 Subject: [PATCH 04/11] Fail early, helps readability responding to a comment in the PR --- pkg/cloudprovider/providers/aws/aws.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 86fcf0edd1..34b137947f 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -643,20 +643,20 @@ func loadCustomResolver() func(service, region string, optFns ...func(*endpoints defaultResolverFn := func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { return defaultResolver.EndpointFor(service, region, optFns...) } - if overridesActive { - customResolverFn := func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { - signature := makeRegionEndpointSignature(service, region) - if ep, ok := overrides[signature]; ok { - return endpoints.ResolvedEndpoint{ - URL: ep.Endpoint, - SigningRegion: ep.SigningRegion, - }, nil - } - return defaultResolver.EndpointFor(service, region, optFns...) - } - return customResolverFn + if !overridesActive { + return defaultResolverFn } - return defaultResolverFn + customResolverFn := func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { + signature := makeRegionEndpointSignature(service, region) + if ep, ok := overrides[signature]; ok { + return endpoints.ResolvedEndpoint{ + URL: ep.Endpoint, + SigningRegion: ep.SigningRegion, + }, nil + } + return defaultResolver.EndpointFor(service, region, optFns...) + } + return customResolverFn } // awsSdkEC2 is an implementation of the EC2 interface, backed by aws-sdk-go From 0dea2459782c50c7246e32e66c7f1636416e98d7 Mon Sep 17 00:00:00 2001 From: ampsingram Date: Wed, 23 Jan 2019 10:04:37 -0500 Subject: [PATCH 05/11] Change to CloudConfig methods for validation and Resolver fn get Move to a separate section for service overrides in INI, populate a struct for each override update-bazel, update-gofmt and verify-spelling run --- pkg/cloudprovider/providers/aws/BUILD | 1 + pkg/cloudprovider/providers/aws/aws.go | 150 +++++---- pkg/cloudprovider/providers/aws/aws_test.go | 332 ++++++++++---------- 3 files changed, 243 insertions(+), 240 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/BUILD b/pkg/cloudprovider/providers/aws/BUILD index 755d731a0f..c2207efaad 100644 --- a/pkg/cloudprovider/providers/aws/BUILD +++ b/pkg/cloudprovider/providers/aws/BUILD @@ -50,6 +50,7 @@ go_library( "//vendor/github.com/aws/aws-sdk-go/aws/credentials/ec2rolecreds:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws/credentials/stscreds:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws/ec2metadata:go_default_library", + "//vendor/github.com/aws/aws-sdk-go/aws/endpoints:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws/request:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws/session:go_default_library", "//vendor/github.com/aws/aws-sdk-go/service/autoscaling:go_default_library", diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 34b137947f..66387d8573 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -570,89 +570,76 @@ type CloudConfig struct { //yourself in an non-AWS cloud and open an issue, please indicate that in the //issue body. DisableStrictZoneCheck bool - - // Delimiter to use to separate region of occurrence, url and signing region for each override - // NOTE: semi-colon ';' truncates the input line in INI files, do not use ';' - // Defaults to "," - OverrideSeparator string - - // These are of format servicename, region, url, signing_region - // s3, region1, https://s3.foo.bar, some signing_region - // ec2 region1, https://ec2.foo.bar, signing_region - ServiceOverrides []string + } + // [ServiceOverride "1"] + // Name = s3 + // Region = region1 + // Url = https://s3.foo.bar + // SigningRegion = signing_region + // + // [ServiceOverride "2"] + // Name = ec2 + // Region = region2 + // Url = https://ec2.foo.bar + // SigningRegion = signing_region + ServiceOverride map[string]*struct { + Service string + Region string + Url string + SigningRegion string } } -const ( - OverrideSeparatorDefault = "," -) - -type CustomEndpoint struct { - Endpoint string - SigningRegion string -} - -var overridesActive = false -var overrides map[string]CustomEndpoint - -func setOverridesDefaults(cfg *CloudConfig) error { - if cfg.Global.OverrideSeparator == "" { - cfg.Global.OverrideSeparator = OverrideSeparatorDefault - } else if cfg.Global.OverrideSeparator == ";" { - return fmt.Errorf("semi-colon may not be used as a override separator, it truncates the input") - } - return nil -} - -func makeRegionEndpointSignature(serviceName, region string) string { - return fmt.Sprintf("%s__%s", strings.TrimSpace(serviceName), strings.TrimSpace(region)) -} - -func parseOverrides(cfg *CloudConfig) error { - overridesActive = false - if len(cfg.Global.ServiceOverrides) == 0 { +func (cfg *CloudConfig) validateOverrides() error { + if len(cfg.ServiceOverride) == 0 { return nil } - if err := setOverridesDefaults(cfg); err != nil { - return err - } - overrides = make(map[string]CustomEndpoint) - for _, ovrd := range cfg.Global.ServiceOverrides { - tokens := strings.Split(ovrd, cfg.Global.OverrideSeparator) - if len(tokens) != 4 { - if len(tokens) > 0 { - return fmt.Errorf("4 parameters (service, region, url, signing region) are required for [%s] in %s", - tokens[0], ovrd) - } - return fmt.Errorf("4 parameters (service, region, url, signing region) are required in %s", - ovrd) + set := make(map[string]bool) + for onum, ovrd := range cfg.ServiceOverride { + name := strings.TrimSpace(ovrd.Service) + if name == "" { + return fmt.Errorf("service name is missing [Service is \"\"] in override %s", onum) + } + region := strings.TrimSpace(ovrd.Region) + if region == "" { + return fmt.Errorf("service region is missing [Region is \"\"] in override %s", onum) + } + url := strings.TrimSpace(ovrd.Url) + if url == "" { + return fmt.Errorf("url is missing [Url is \"\"] in override %s", onum) + } + signingRegion := strings.TrimSpace(ovrd.SigningRegion) + if signingRegion == "" { + return fmt.Errorf("signingRegion is missing [SigningRegion is \"\"] in override %s", onum) + } + if _, ok := set[name+"_"+region]; ok { + return fmt.Errorf("duplicate entry found for service override [%s] (%s in %s)", onum, name, region) + } else { + set[name+"_"+region] = true } - name := strings.TrimSpace(tokens[0]) - region := strings.TrimSpace(tokens[1]) - url := strings.TrimSpace(tokens[2]) - signingRegion := strings.TrimSpace(tokens[3]) - signature := makeRegionEndpointSignature(name, region) - overrides[signature] = CustomEndpoint{Endpoint: url, SigningRegion: signingRegion} } - overridesActive = true return nil } -func loadCustomResolver() func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { +func (cfg *CloudConfig) getResolver() func(service, region string, + optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { defaultResolver := endpoints.DefaultResolver() - defaultResolverFn := func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { + defaultResolverFn := func(service, region string, + optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { return defaultResolver.EndpointFor(service, region, optFns...) } - if !overridesActive { + if len(cfg.ServiceOverride) == 0 { return defaultResolverFn } - customResolverFn := func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { - signature := makeRegionEndpointSignature(service, region) - if ep, ok := overrides[signature]; ok { - return endpoints.ResolvedEndpoint{ - URL: ep.Endpoint, - SigningRegion: ep.SigningRegion, - }, nil + customResolverFn := func(service, region string, + optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { + for _, override := range cfg.ServiceOverride { + if override.Service == service && override.Region == region { + return endpoints.ResolvedEndpoint{ + URL: override.Url, + SigningRegion: override.SigningRegion, + }, nil + } } return defaultResolver.EndpointFor(service, region, optFns...) } @@ -664,16 +651,23 @@ type awsSdkEC2 struct { ec2 *ec2.EC2 } +// Interface to make the CloudConfig immutable for awsSDKProvider +type awsCloudConfigProvider interface { + getResolver() func(string, string, ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) +} + type awsSDKProvider struct { creds *credentials.Credentials + cfg awsCloudConfigProvider mutex sync.Mutex regionDelayers map[string]*CrossRequestRetryDelay } -func newAWSSDKProvider(creds *credentials.Credentials) *awsSDKProvider { +func newAWSSDKProvider(creds *credentials.Credentials, cfg *CloudConfig) *awsSDKProvider { return &awsSDKProvider{ creds: creds, + cfg: cfg, regionDelayers: make(map[string]*CrossRequestRetryDelay), } } @@ -739,7 +733,7 @@ func (p *awsSDKProvider) Compute(regionName string) (EC2, error) { Credentials: p.creds, } awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). - WithEndpointResolver(endpoints.ResolverFunc(loadCustomResolver())) + WithEndpointResolver(endpoints.ResolverFunc(p.cfg.getResolver())) sess, err := session.NewSession(awsConfig) if err != nil { @@ -761,7 +755,7 @@ func (p *awsSDKProvider) LoadBalancing(regionName string) (ELB, error) { Credentials: p.creds, } awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). - WithEndpointResolver(endpoints.ResolverFunc(loadCustomResolver())) + WithEndpointResolver(endpoints.ResolverFunc(p.cfg.getResolver())) sess, err := session.NewSession(awsConfig) if err != nil { @@ -779,7 +773,7 @@ func (p *awsSDKProvider) LoadBalancingV2(regionName string) (ELBV2, error) { Credentials: p.creds, } awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). - WithEndpointResolver(endpoints.ResolverFunc(loadCustomResolver())) + WithEndpointResolver(endpoints.ResolverFunc(p.cfg.getResolver())) sess, err := session.NewSession(awsConfig) if err != nil { @@ -798,7 +792,7 @@ func (p *awsSDKProvider) Autoscaling(regionName string) (ASG, error) { Credentials: p.creds, } awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). - WithEndpointResolver(endpoints.ResolverFunc(loadCustomResolver())) + WithEndpointResolver(endpoints.ResolverFunc(p.cfg.getResolver())) sess, err := session.NewSession(awsConfig) if err != nil { @@ -813,7 +807,7 @@ func (p *awsSDKProvider) Autoscaling(regionName string) (ASG, error) { func (p *awsSDKProvider) Metadata() (EC2Metadata, error) { sess, err := session.NewSession(&aws.Config{ - EndpointResolver: endpoints.ResolverFunc(loadCustomResolver()), + EndpointResolver: endpoints.ResolverFunc(p.cfg.getResolver()), }) if err != nil { return nil, fmt.Errorf("unable to initialize AWS session: %v", err) @@ -829,7 +823,7 @@ func (p *awsSDKProvider) KeyManagement(regionName string) (KMS, error) { Credentials: p.creds, } awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). - WithEndpointResolver(endpoints.ResolverFunc(loadCustomResolver())) + WithEndpointResolver(endpoints.ResolverFunc(p.cfg.getResolver())) sess, err := session.NewSession(awsConfig) if err != nil { @@ -1054,8 +1048,8 @@ func init() { return nil, fmt.Errorf("unable to read AWS cloud provider config file: %v", err) } - if err = parseOverrides(cfg); err != nil { - return nil, fmt.Errorf("unable to parse custom endpoint overrides: %v", err) + if err = cfg.validateOverrides(); err != nil { + return nil, fmt.Errorf("unable to validate custom endpoint overrides: %v", err) } sess, err := session.NewSession(&aws.Config{}) @@ -1083,7 +1077,7 @@ func init() { &credentials.SharedCredentialsProvider{}, }) - aws := newAWSSDKProvider(creds) + aws := newAWSSDKProvider(creds, cfg) return newAWSCloud(*cfg, aws) }) } diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 3d70088405..d26d4a20a9 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -187,7 +187,7 @@ func TestReadAWSCloudConfig(t *testing.T) { } type ServiceDescriptor struct { - name string + name string region string } @@ -203,57 +203,165 @@ func TestOverridesActiveConfig(t *testing.T) { servicesOverridden []ServiceDescriptor }{ { - "Missing Servicename Separator", - strings.NewReader("[global]\nServiceOverrides=s3sregion, https://s3.foo.bar, sregion"), + "No overrides", + strings.NewReader(` + [global] + `), + nil, + false, false, + []ServiceDescriptor{}, + }, + { + "Missing Service Name", + strings.NewReader(` + [global] + + [ServiceOverride "1"] + Region=sregion + Url=https://s3.foo.bar + SigningRegion=sregion + `), nil, true, false, []ServiceDescriptor{}, }, { "Missing Service Region", - strings.NewReader("[global]\nServiceOverrides=s3, https://s3.foo.bar, sregion"), + strings.NewReader(` + [global] + + [ServiceOverride "1"] + Service=s3 + Url=https://s3.foo.bar + SigningRegion=sregion + `), nil, true, false, []ServiceDescriptor{}, }, { - "Semi-colon in override delimiter", - strings.NewReader("[global]\nOverrideSeparator=;\n" + - "ServiceOverrides=s3, https://s3.foo.bar, sregion"), + "Missing URL", + strings.NewReader(` + [global] + + [ServiceOverride "1"] + Service=s3 + Region=sregion + SigningRegion=sregion + `), + nil, + true, false, + []ServiceDescriptor{}, + }, + { + "Missing Signing Region", + strings.NewReader(` + [global] + + [ServiceOverride "1"] + Service=s3 + Region=sregion + Url=https://s3.foo.bar + `), nil, true, false, []ServiceDescriptor{}, }, { "Active Overrides", - strings.NewReader("[global]\nServiceOverrides=s3, sregion, https://s3.foo.bar, sregion"), + strings.NewReader(` + [Global] + + [ServiceOverride "1"] + Service = s3 + Region = sregion + Url = https://s3.foo.bar + SigningRegion = sregion + `), nil, false, true, []ServiceDescriptor{{name: "s3", region: "sregion"}}, }, { - "Multiple Overriden Services", - strings.NewReader("[global]\n" + - "ServiceOverrides=s3, sregion1, https://s3.foo.bar, sregion\n" + - "ServiceOverrides=ec2, sregion2, https://ec2.foo.bar, sregion"), + "Multiple Overridden Services", + strings.NewReader(` + [Global] + vpc = vpc-abc1234567 + + [ServiceOverride "1"] + Service=s3 + Region=sregion1 + Url=https://s3.foo.bar + SigningRegion=sregion + + [ServiceOverride "2"] + Service=ec2 + Region=sregion2 + Url=https://ec2.foo.bar + SigningRegion=sregion`), nil, false, true, []ServiceDescriptor{{"s3", "sregion1"}, {"ec2", "sregion2"}}, }, { - "Multiple Overriden Services in Multiple regions", - strings.NewReader("[global]\n" + - "ServiceOverrides=s3, region1, https://s3.foo.bar, sregion\n" + - "ServiceOverrides=ec2, region2, https://ec2.foo.bar, sregion"), + "Duplicate Services", + strings.NewReader(` + [Global] + vpc = vpc-abc1234567 + + [ServiceOverride "1"] + Service=s3 + Region=sregion1 + Url=https://s3.foo.bar + SigningRegion=sregion + + [ServiceOverride "2"] + Service=s3 + Region=sregion1 + Url=https://s3.foo.bar + SigningRegion=sregion`), + nil, + true, false, + []ServiceDescriptor{}, + }, + { + "Multiple Overridden Services in Multiple regions", + strings.NewReader(` + [global] + + [ServiceOverride "1"] + Service=s3 + Region=region1 + Url=https://s3.foo.bar + SigningRegion=sregion + + [ServiceOverride "2"] + Service=ec2 + Region=region2 + Url=https://ec2.foo.bar + SigningRegion=sregion + `), nil, false, true, - []ServiceDescriptor{{"s3","region1"}, {"ec2", "region2"}}, + []ServiceDescriptor{{"s3", "region1"}, {"ec2", "region2"}}, }, { "Multiple regions, Same Service", - strings.NewReader("[global]\n" + - "ServiceOverrides=s3, region1, https://s3.foo.bar, sregion\n" + - "ServiceOverrides=s3, region2, https://s3.foo.bar, sregion"), + strings.NewReader(` + [global] + + [ServiceOverride "1"] + Service=s3 + Region=region1 + Url=https://s3.foo.bar + SigningRegion=sregion + + [ServiceOverride "2"] + Service=s3 + Region=region2 + Url=https://s3.foo.bar + SigningRegion=sregion + `), nil, false, true, []ServiceDescriptor{{"s3", "region1"}, {"s3", "region2"}}, @@ -264,162 +372,62 @@ func TestOverridesActiveConfig(t *testing.T) { t.Logf("Running test case %s", test.name) cfg, err := readAWSCloudConfig(test.reader) if err == nil { - err = parseOverrides(cfg) + err = cfg.validateOverrides() } if test.expectError { if err == nil { t.Errorf("Should error for case %s (cfg=%v)", test.name, cfg) } - if overridesActive != test.active { - t.Errorf("Incorrect active flag (%v vs %v) for case: %s", - overridesActive, test.active, test.name) - } } else { if err != nil { - t.Errorf("Should succeed for case: %s", test.name) + t.Errorf("Should succeed for case: %s, got %v", test.name, err) } - if overridesActive != test.active { - t.Errorf("Incorrect active flag (%v vs %v) for case: %s", - overridesActive, test.active, test.name) - } else { - if len(overrides) != len(test.servicesOverridden) { - t.Errorf("Expected %d overridden services, received %d for case %s", - len(test.servicesOverridden), len(overrides), test.name) - } else { - for _, sd := range test.servicesOverridden { - signature := makeRegionEndpointSignature(sd.name, sd.region) - ep, ok := overrides[signature] - if !ok { - t.Errorf("Missing override for service %s in case %s", - sd.name, test.name) - } else { - if ep.SigningRegion != "sregion" { - t.Errorf("Expected signing region 'sregion', received '%s' for case %s", - ep.SigningRegion, test.name) - } - targetName := fmt.Sprintf("https://%s.foo.bar", sd.name) - if ep.Endpoint != targetName { - t.Errorf("Expected Endpoint '%s', received '%s' for case %s", - targetName, ep.Endpoint, test.name) - } - fn := loadCustomResolver() - ep1, e := fn(sd.name, sd.region, nil) - if e != nil { - t.Errorf("Expected a valid endpoint for %s in case %s", - sd.name, test.name) - } else { - targetName := fmt.Sprintf("https://%s.foo.bar", sd.name) - if ep1.URL != targetName { - t.Errorf("Expected endpoint url: %s, received %s in case %s", - targetName, ep1.URL, test.name) - } - if ep1.SigningRegion != "sregion" { - t.Errorf("Expected signing region 'sregion', received '%s' in case %s", - ep1.SigningRegion, test.name) - } - } + if len(cfg.ServiceOverride) != len(test.servicesOverridden) { + t.Errorf("Expected %d overridden services, received %d for case %s", + len(test.servicesOverridden), len(cfg.ServiceOverride), test.name) + } else { + for _, sd := range test.servicesOverridden { + var found *struct { + Service string + Region string + Url string + SigningRegion string + } + for _, v := range cfg.ServiceOverride { + if v.Service == sd.name && v.Region == sd.region { + found = v + break } } - } - } - } - } -} + if found == nil { + t.Errorf("Missing override for service %s in case %s", + sd.name, test.name) + } else { + if found.SigningRegion != "sregion" { + t.Errorf("Expected signing region 'sregion', received '%s' for case %s", + found.SigningRegion, test.name) + } + targetName := fmt.Sprintf("https://%s.foo.bar", sd.name) + if found.Url != targetName { + t.Errorf("Expected Endpoint '%s', received '%s' for case %s", + targetName, found.Url, test.name) + } -func TestOverridesDefaults(t *testing.T) { - tests := []struct { - name string - - configString string - - expectError bool - active bool - servicesOverridden []string - defaults []string - }{ - { - "Custom OverrideSeparator", - "[global]\n" + - "ServiceOverrides=s3 + sregion + https://s3.foo.bar + sregion \n" + - "OverrideSeparator=+", - false, true, - []string{"s3"}, - []string{"+"}, - }, - { - "Active Overrides", - "[global]\n" + - "ServiceOverrides=s3, sregion, https://s3.foo.bar , sregion\n" + - "ServiceOverrides=ec2, sregion, https://ec2.foo.bar, sregion", - false, true, - []string{"s3", "ec2"}, - []string{","}, - }, - } - - for _, test := range tests { - t.Logf("Running test case %s", test.name) - cfg, err := readAWSCloudConfig(strings.NewReader(test.configString)) - if err == nil { - err = parseOverrides(cfg) - } - if test.expectError { - if err == nil { - t.Errorf("Should error for case %s (cfg=%v)", test.name, cfg) - } - if overridesActive != test.active { - t.Errorf("Incorrect active flag (%v vs %v) for case: %s", - overridesActive, test.active, test.name) - } - } else { - if err != nil { - t.Errorf("Should succeed for case: %s", test.name) - } - if overridesActive != test.active { - t.Errorf("Incorrect active flag (%v vs %v) for case: %s", - overridesActive, test.active, test.name) - } else { - if cfg.Global.OverrideSeparator != test.defaults[0] { - t.Errorf("Incorrect OverrideSeparator (%s vs %s) for case %s", - cfg.Global.OverrideSeparator, test.defaults[0], test.name) - } - if len(overrides) != len(test.servicesOverridden) { - t.Errorf("Expected %d overridden services, received %d for case %s", - len(test.servicesOverridden), len(overrides), test.name) - } else { - for _, name := range test.servicesOverridden { - signature := makeRegionEndpointSignature(name, "sregion") - ep, ok := overrides[signature] - if !ok { - t.Errorf("Missing override for service %s in case %s", - name, test.name) + fn := cfg.getResolver() + ep1, e := fn(sd.name, sd.region, nil) + if e != nil { + t.Errorf("Expected a valid endpoint for %s in case %s", + sd.name, test.name) } else { - if ep.SigningRegion != "sregion" { - t.Errorf("Expected signing region 'sregion', received '%s' for case %s", - ep.SigningRegion, test.name) + targetName := fmt.Sprintf("https://%s.foo.bar", sd.name) + if ep1.URL != targetName { + t.Errorf("Expected endpoint url: %s, received %s in case %s", + targetName, ep1.URL, test.name) } - targetName := fmt.Sprintf("https://%s.foo.bar", name) - if ep.Endpoint != targetName { - t.Errorf("Expected Endpoint '%s', received '%s' for case %s", - targetName, ep.Endpoint, test.name) - } - - fn := loadCustomResolver() - ep1, e := fn(name, "sregion", nil) - if e != nil { - t.Errorf("Expected a valid endpoint for %s in case %s", - name, test.name) - } else { - targetName := fmt.Sprintf("https://%s.foo.bar", name) - if ep1.URL != targetName { - t.Errorf("Expected endpoint url: %s, received %s in case %s", - targetName, ep1.URL, test.name) - } - if ep1.SigningRegion != "sregion" { - t.Errorf("Expected signing region 'sregion', received '%s' in case %s", - ep1.SigningRegion, test.name) - } + if ep1.SigningRegion != "sregion" { + t.Errorf("Expected signing region 'sregion', received '%s' in case %s", + ep1.SigningRegion, test.name) } } } From d6ea2296e8ac614da5ab66b0fb50abba4998cd59 Mon Sep 17 00:00:00 2001 From: ampsingram Date: Wed, 23 Jan 2019 10:30:19 -0500 Subject: [PATCH 06/11] Fix comment in CloudConfig s/Name/Service --- pkg/cloudprovider/providers/aws/aws.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 66387d8573..aec92f015f 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -572,13 +572,13 @@ type CloudConfig struct { DisableStrictZoneCheck bool } // [ServiceOverride "1"] - // Name = s3 + // Service = s3 // Region = region1 // Url = https://s3.foo.bar // SigningRegion = signing_region // // [ServiceOverride "2"] - // Name = ec2 + // Service = ec2 // Region = region2 // Url = https://ec2.foo.bar // SigningRegion = signing_region From cc835a86c9647cd7c2ea5ef81a31039837875118 Mon Sep 17 00:00:00 2001 From: ampsingram Date: Thu, 24 Jan 2019 16:07:14 +0000 Subject: [PATCH 07/11] gofmt changes --- pkg/cloudprovider/providers/aws/aws_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index d26d4a20a9..6ab4f8fe7c 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -207,8 +207,8 @@ func TestOverridesActiveConfig(t *testing.T) { strings.NewReader(` [global] `), - nil, - false, false, + nil, + false, false, []ServiceDescriptor{}, }, { From 87592b38113a91f878f4657044d28af41cccd66f Mon Sep 17 00:00:00 2001 From: ampsingram Date: Thu, 24 Jan 2019 11:58:12 -0500 Subject: [PATCH 08/11] Fix golint findings --- pkg/cloudprovider/providers/aws/aws.go | 15 +++++------ pkg/cloudprovider/providers/aws/aws_test.go | 30 ++++++++++----------- 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index aec92f015f..56c3af459b 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -574,18 +574,18 @@ type CloudConfig struct { // [ServiceOverride "1"] // Service = s3 // Region = region1 - // Url = https://s3.foo.bar + // URL = https://s3.foo.bar // SigningRegion = signing_region // // [ServiceOverride "2"] // Service = ec2 // Region = region2 - // Url = https://ec2.foo.bar + // URL = https://ec2.foo.bar // SigningRegion = signing_region ServiceOverride map[string]*struct { Service string Region string - Url string + URL string SigningRegion string } } @@ -604,9 +604,9 @@ func (cfg *CloudConfig) validateOverrides() error { if region == "" { return fmt.Errorf("service region is missing [Region is \"\"] in override %s", onum) } - url := strings.TrimSpace(ovrd.Url) + url := strings.TrimSpace(ovrd.URL) if url == "" { - return fmt.Errorf("url is missing [Url is \"\"] in override %s", onum) + return fmt.Errorf("url is missing [URL is \"\"] in override %s", onum) } signingRegion := strings.TrimSpace(ovrd.SigningRegion) if signingRegion == "" { @@ -614,9 +614,8 @@ func (cfg *CloudConfig) validateOverrides() error { } if _, ok := set[name+"_"+region]; ok { return fmt.Errorf("duplicate entry found for service override [%s] (%s in %s)", onum, name, region) - } else { - set[name+"_"+region] = true } + set[name+"_"+region] = true } return nil } @@ -636,7 +635,7 @@ func (cfg *CloudConfig) getResolver() func(service, region string, for _, override := range cfg.ServiceOverride { if override.Service == service && override.Region == region { return endpoints.ResolvedEndpoint{ - URL: override.Url, + URL: override.URL, SigningRegion: override.SigningRegion, }, nil } diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 6ab4f8fe7c..4a7ea0217c 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -218,7 +218,7 @@ func TestOverridesActiveConfig(t *testing.T) { [ServiceOverride "1"] Region=sregion - Url=https://s3.foo.bar + URL=https://s3.foo.bar SigningRegion=sregion `), nil, @@ -232,7 +232,7 @@ func TestOverridesActiveConfig(t *testing.T) { [ServiceOverride "1"] Service=s3 - Url=https://s3.foo.bar + URL=https://s3.foo.bar SigningRegion=sregion `), nil, @@ -261,7 +261,7 @@ func TestOverridesActiveConfig(t *testing.T) { [ServiceOverride "1"] Service=s3 Region=sregion - Url=https://s3.foo.bar + URL=https://s3.foo.bar `), nil, true, false, @@ -275,7 +275,7 @@ func TestOverridesActiveConfig(t *testing.T) { [ServiceOverride "1"] Service = s3 Region = sregion - Url = https://s3.foo.bar + URL = https://s3.foo.bar SigningRegion = sregion `), nil, @@ -291,13 +291,13 @@ func TestOverridesActiveConfig(t *testing.T) { [ServiceOverride "1"] Service=s3 Region=sregion1 - Url=https://s3.foo.bar + URL=https://s3.foo.bar SigningRegion=sregion [ServiceOverride "2"] Service=ec2 Region=sregion2 - Url=https://ec2.foo.bar + URL=https://ec2.foo.bar SigningRegion=sregion`), nil, false, true, @@ -312,13 +312,13 @@ func TestOverridesActiveConfig(t *testing.T) { [ServiceOverride "1"] Service=s3 Region=sregion1 - Url=https://s3.foo.bar + URL=https://s3.foo.bar SigningRegion=sregion [ServiceOverride "2"] Service=s3 Region=sregion1 - Url=https://s3.foo.bar + URL=https://s3.foo.bar SigningRegion=sregion`), nil, true, false, @@ -332,13 +332,13 @@ func TestOverridesActiveConfig(t *testing.T) { [ServiceOverride "1"] Service=s3 Region=region1 - Url=https://s3.foo.bar + URL=https://s3.foo.bar SigningRegion=sregion [ServiceOverride "2"] Service=ec2 Region=region2 - Url=https://ec2.foo.bar + URL=https://ec2.foo.bar SigningRegion=sregion `), nil, @@ -353,13 +353,13 @@ func TestOverridesActiveConfig(t *testing.T) { [ServiceOverride "1"] Service=s3 Region=region1 - Url=https://s3.foo.bar + URL=https://s3.foo.bar SigningRegion=sregion [ServiceOverride "2"] Service=s3 Region=region2 - Url=https://s3.foo.bar + URL=https://s3.foo.bar SigningRegion=sregion `), nil, @@ -391,7 +391,7 @@ func TestOverridesActiveConfig(t *testing.T) { var found *struct { Service string Region string - Url string + URL string SigningRegion string } for _, v := range cfg.ServiceOverride { @@ -409,9 +409,9 @@ func TestOverridesActiveConfig(t *testing.T) { found.SigningRegion, test.name) } targetName := fmt.Sprintf("https://%s.foo.bar", sd.name) - if found.Url != targetName { + if found.URL != targetName { t.Errorf("Expected Endpoint '%s', received '%s' for case %s", - targetName, found.Url, test.name) + targetName, found.URL, test.name) } fn := cfg.getResolver() From 28d3774004aa62738e03d1a86b0050b6180bff40 Mon Sep 17 00:00:00 2001 From: ampsingram Date: Fri, 25 Jan 2019 18:15:41 -0500 Subject: [PATCH 09/11] Add SigningMethod, make it optional, copy in verbatim An empty SigningMethod will default properly. --- pkg/cloudprovider/providers/aws/aws.go | 39 ++++++++----- pkg/cloudprovider/providers/aws/aws_test.go | 62 +++++++++++++++------ 2 files changed, 69 insertions(+), 32 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 56c3af459b..65c50db965 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -576,17 +576,20 @@ type CloudConfig struct { // Region = region1 // URL = https://s3.foo.bar // SigningRegion = signing_region + // SigningMethod = signing_method // // [ServiceOverride "2"] // Service = ec2 // Region = region2 // URL = https://ec2.foo.bar // SigningRegion = signing_region + // SigningMethod = signing_method ServiceOverride map[string]*struct { Service string Region string URL string SigningRegion string + SigningMethod string } } @@ -596,32 +599,39 @@ func (cfg *CloudConfig) validateOverrides() error { } set := make(map[string]bool) for onum, ovrd := range cfg.ServiceOverride { + // Note: gcfg does not space trim, so we have to when comparing to empty string "" name := strings.TrimSpace(ovrd.Service) if name == "" { return fmt.Errorf("service name is missing [Service is \"\"] in override %s", onum) } + // insure the map service name is space trimmed + ovrd.Service = name + region := strings.TrimSpace(ovrd.Region) if region == "" { return fmt.Errorf("service region is missing [Region is \"\"] in override %s", onum) } + // insure the map region is space trimmed + ovrd.Region = region + url := strings.TrimSpace(ovrd.URL) - if url == "" { + if url== "" { return fmt.Errorf("url is missing [URL is \"\"] in override %s", onum) } signingRegion := strings.TrimSpace(ovrd.SigningRegion) if signingRegion == "" { return fmt.Errorf("signingRegion is missing [SigningRegion is \"\"] in override %s", onum) } - if _, ok := set[name+"_"+region]; ok { + signature := name+"_"+region + if set[signature] { return fmt.Errorf("duplicate entry found for service override [%s] (%s in %s)", onum, name, region) } - set[name+"_"+region] = true + set[signature] = true } return nil } -func (cfg *CloudConfig) getResolver() func(service, region string, - optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { +func (cfg *CloudConfig) getResolver() endpoints.ResolverFunc { defaultResolver := endpoints.DefaultResolver() defaultResolverFn := func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { @@ -630,19 +640,20 @@ func (cfg *CloudConfig) getResolver() func(service, region string, if len(cfg.ServiceOverride) == 0 { return defaultResolverFn } - customResolverFn := func(service, region string, + + return func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) { for _, override := range cfg.ServiceOverride { if override.Service == service && override.Region == region { return endpoints.ResolvedEndpoint{ URL: override.URL, SigningRegion: override.SigningRegion, + SigningMethod: override.SigningMethod, }, nil } } return defaultResolver.EndpointFor(service, region, optFns...) } - return customResolverFn } // awsSdkEC2 is an implementation of the EC2 interface, backed by aws-sdk-go @@ -652,7 +663,7 @@ type awsSdkEC2 struct { // Interface to make the CloudConfig immutable for awsSDKProvider type awsCloudConfigProvider interface { - getResolver() func(string, string, ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) + getResolver() endpoints.ResolverFunc } type awsSDKProvider struct { @@ -732,7 +743,7 @@ func (p *awsSDKProvider) Compute(regionName string) (EC2, error) { Credentials: p.creds, } awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). - WithEndpointResolver(endpoints.ResolverFunc(p.cfg.getResolver())) + WithEndpointResolver(p.cfg.getResolver()) sess, err := session.NewSession(awsConfig) if err != nil { @@ -754,7 +765,7 @@ func (p *awsSDKProvider) LoadBalancing(regionName string) (ELB, error) { Credentials: p.creds, } awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). - WithEndpointResolver(endpoints.ResolverFunc(p.cfg.getResolver())) + WithEndpointResolver(p.cfg.getResolver()) sess, err := session.NewSession(awsConfig) if err != nil { @@ -772,7 +783,7 @@ func (p *awsSDKProvider) LoadBalancingV2(regionName string) (ELBV2, error) { Credentials: p.creds, } awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). - WithEndpointResolver(endpoints.ResolverFunc(p.cfg.getResolver())) + WithEndpointResolver(p.cfg.getResolver()) sess, err := session.NewSession(awsConfig) if err != nil { @@ -791,7 +802,7 @@ func (p *awsSDKProvider) Autoscaling(regionName string) (ASG, error) { Credentials: p.creds, } awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). - WithEndpointResolver(endpoints.ResolverFunc(p.cfg.getResolver())) + WithEndpointResolver(p.cfg.getResolver()) sess, err := session.NewSession(awsConfig) if err != nil { @@ -806,7 +817,7 @@ func (p *awsSDKProvider) Autoscaling(regionName string) (ASG, error) { func (p *awsSDKProvider) Metadata() (EC2Metadata, error) { sess, err := session.NewSession(&aws.Config{ - EndpointResolver: endpoints.ResolverFunc(p.cfg.getResolver()), + EndpointResolver: p.cfg.getResolver(), }) if err != nil { return nil, fmt.Errorf("unable to initialize AWS session: %v", err) @@ -822,7 +833,7 @@ func (p *awsSDKProvider) KeyManagement(regionName string) (KMS, error) { Credentials: p.creds, } awsConfig = awsConfig.WithCredentialsChainVerboseErrors(true). - WithEndpointResolver(endpoints.ResolverFunc(p.cfg.getResolver())) + WithEndpointResolver(p.cfg.getResolver()) sess, err := session.NewSession(awsConfig) if err != nil { diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 4a7ea0217c..6625676032 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -189,6 +189,7 @@ func TestReadAWSCloudConfig(t *testing.T) { type ServiceDescriptor struct { name string region string + signingRegion, signingMethod string } func TestOverridesActiveConfig(t *testing.T) { @@ -220,6 +221,7 @@ func TestOverridesActiveConfig(t *testing.T) { Region=sregion URL=https://s3.foo.bar SigningRegion=sregion + SigningMethod = sign `), nil, true, false, @@ -234,6 +236,7 @@ func TestOverridesActiveConfig(t *testing.T) { Service=s3 URL=https://s3.foo.bar SigningRegion=sregion + SigningMethod = sign `), nil, true, false, @@ -245,9 +248,10 @@ func TestOverridesActiveConfig(t *testing.T) { [global] [ServiceOverride "1"] - Service=s3 + Service="s3" Region=sregion SigningRegion=sregion + SigningMethod = sign `), nil, true, false, @@ -262,6 +266,7 @@ func TestOverridesActiveConfig(t *testing.T) { Service=s3 Region=sregion URL=https://s3.foo.bar + SigningMethod = sign `), nil, true, false, @@ -273,14 +278,15 @@ func TestOverridesActiveConfig(t *testing.T) { [Global] [ServiceOverride "1"] - Service = s3 + Service = "s3 " Region = sregion URL = https://s3.foo.bar SigningRegion = sregion + SigningMethod = v4 `), nil, false, true, - []ServiceDescriptor{{name: "s3", region: "sregion"}}, + []ServiceDescriptor{{name: "s3", region: "sregion", signingRegion: "sregion", signingMethod: "v4"}}, }, { "Multiple Overridden Services", @@ -292,16 +298,19 @@ func TestOverridesActiveConfig(t *testing.T) { Service=s3 Region=sregion1 URL=https://s3.foo.bar - SigningRegion=sregion + SigningRegion=sregion1 + SigningMethod = v4 [ServiceOverride "2"] Service=ec2 Region=sregion2 URL=https://ec2.foo.bar - SigningRegion=sregion`), + SigningRegion=sregion2 + SigningMethod = v4`), nil, false, true, - []ServiceDescriptor{{"s3", "sregion1"}, {"ec2", "sregion2"}}, + []ServiceDescriptor{{name:"s3", region: "sregion1", signingRegion: "sregion1", signingMethod: "v4"}, + {name: "ec2", region: "sregion2", signingRegion: "sregion2", signingMethod: "v4"}}, }, { "Duplicate Services", @@ -314,12 +323,14 @@ func TestOverridesActiveConfig(t *testing.T) { Region=sregion1 URL=https://s3.foo.bar SigningRegion=sregion + SigningMethod = sign [ServiceOverride "2"] Service=s3 Region=sregion1 URL=https://s3.foo.bar - SigningRegion=sregion`), + SigningRegion=sregion + SigningMethod = sign`), nil, true, false, []ServiceDescriptor{}, @@ -333,17 +344,19 @@ func TestOverridesActiveConfig(t *testing.T) { Service=s3 Region=region1 URL=https://s3.foo.bar - SigningRegion=sregion + SigningRegion=sregion1 [ServiceOverride "2"] Service=ec2 Region=region2 URL=https://ec2.foo.bar SigningRegion=sregion + SigningMethod = v4 `), nil, false, true, - []ServiceDescriptor{{"s3", "region1"}, {"ec2", "region2"}}, + []ServiceDescriptor{{name: "s3", region: "region1", signingRegion: "sregion1", signingMethod: ""}, + {name:"ec2", region: "region2", signingRegion: "sregion", signingMethod: "v4"}}, }, { "Multiple regions, Same Service", @@ -354,17 +367,21 @@ func TestOverridesActiveConfig(t *testing.T) { Service=s3 Region=region1 URL=https://s3.foo.bar - SigningRegion=sregion + SigningRegion=sregion1 + SigningMethod = v3 [ServiceOverride "2"] Service=s3 Region=region2 URL=https://s3.foo.bar - SigningRegion=sregion + SigningRegion=sregion1 + SigningMethod = v4 + `), nil, false, true, - []ServiceDescriptor{{"s3", "region1"}, {"s3", "region2"}}, + []ServiceDescriptor{{name:"s3", region: "region1", signingRegion: "sregion1", signingMethod: "v3"}, + {name:"s3", region: "region2", signingRegion: "sregion1", signingMethod: "v4"}}, }, } @@ -393,6 +410,7 @@ func TestOverridesActiveConfig(t *testing.T) { Region string URL string SigningRegion string + SigningMethod string } for _, v := range cfg.ServiceOverride { if v.Service == sd.name && v.Region == sd.region { @@ -404,9 +422,13 @@ func TestOverridesActiveConfig(t *testing.T) { t.Errorf("Missing override for service %s in case %s", sd.name, test.name) } else { - if found.SigningRegion != "sregion" { - t.Errorf("Expected signing region 'sregion', received '%s' for case %s", - found.SigningRegion, test.name) + if found.SigningRegion != sd.signingRegion { + t.Errorf("Expected signing region '%s', received '%s' for case %s", + sd.signingRegion, found.SigningRegion, test.name) + } + if found.SigningMethod != sd.signingMethod { + t.Errorf("Expected signing method '%s', received '%s' for case %s", + sd.signingMethod, found.SigningRegion, test.name) } targetName := fmt.Sprintf("https://%s.foo.bar", sd.name) if found.URL != targetName { @@ -425,9 +447,13 @@ func TestOverridesActiveConfig(t *testing.T) { t.Errorf("Expected endpoint url: %s, received %s in case %s", targetName, ep1.URL, test.name) } - if ep1.SigningRegion != "sregion" { - t.Errorf("Expected signing region 'sregion', received '%s' in case %s", - ep1.SigningRegion, test.name) + if ep1.SigningRegion != sd.signingRegion { + t.Errorf("Expected signing region '%s', received '%s' in case %s", + sd.signingRegion, ep1.SigningRegion, test.name) + } + if ep1.SigningMethod != sd.signingMethod { + t.Errorf("Expected signing method '%s', received '%s' in case %s", + sd.signingMethod, ep1.SigningRegion, test.name) } } } From 1ab6569da2e0157425cf7c48e0a2f52c6d72a278 Mon Sep 17 00:00:00 2001 From: ampsingram Date: Fri, 25 Jan 2019 18:16:48 -0500 Subject: [PATCH 10/11] format changes --- pkg/cloudprovider/providers/aws/aws.go | 4 ++-- pkg/cloudprovider/providers/aws/aws_test.go | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 65c50db965..35e7e6d50d 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -615,14 +615,14 @@ func (cfg *CloudConfig) validateOverrides() error { ovrd.Region = region url := strings.TrimSpace(ovrd.URL) - if url== "" { + if url == "" { return fmt.Errorf("url is missing [URL is \"\"] in override %s", onum) } signingRegion := strings.TrimSpace(ovrd.SigningRegion) if signingRegion == "" { return fmt.Errorf("signingRegion is missing [SigningRegion is \"\"] in override %s", onum) } - signature := name+"_"+region + signature := name + "_" + region if set[signature] { return fmt.Errorf("duplicate entry found for service override [%s] (%s in %s)", onum, name, region) } diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 6625676032..1a6333764d 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -187,8 +187,8 @@ func TestReadAWSCloudConfig(t *testing.T) { } type ServiceDescriptor struct { - name string - region string + name string + region string signingRegion, signingMethod string } @@ -309,7 +309,7 @@ func TestOverridesActiveConfig(t *testing.T) { SigningMethod = v4`), nil, false, true, - []ServiceDescriptor{{name:"s3", region: "sregion1", signingRegion: "sregion1", signingMethod: "v4"}, + []ServiceDescriptor{{name: "s3", region: "sregion1", signingRegion: "sregion1", signingMethod: "v4"}, {name: "ec2", region: "sregion2", signingRegion: "sregion2", signingMethod: "v4"}}, }, { @@ -356,7 +356,7 @@ func TestOverridesActiveConfig(t *testing.T) { nil, false, true, []ServiceDescriptor{{name: "s3", region: "region1", signingRegion: "sregion1", signingMethod: ""}, - {name:"ec2", region: "region2", signingRegion: "sregion", signingMethod: "v4"}}, + {name: "ec2", region: "region2", signingRegion: "sregion", signingMethod: "v4"}}, }, { "Multiple regions, Same Service", @@ -380,8 +380,8 @@ func TestOverridesActiveConfig(t *testing.T) { `), nil, false, true, - []ServiceDescriptor{{name:"s3", region: "region1", signingRegion: "sregion1", signingMethod: "v3"}, - {name:"s3", region: "region2", signingRegion: "sregion1", signingMethod: "v4"}}, + []ServiceDescriptor{{name: "s3", region: "region1", signingRegion: "sregion1", signingMethod: "v3"}, + {name: "s3", region: "region2", signingRegion: "sregion1", signingMethod: "v4"}}, }, } From 5daa004105b124248045971ec5414d13271251f6 Mon Sep 17 00:00:00 2001 From: ampsingram Date: Tue, 29 Jan 2019 09:45:03 -0500 Subject: [PATCH 11/11] Add SigningName as optional parameter Makes AWS testing simpler --- pkg/cloudprovider/providers/aws/aws.go | 2 ++ pkg/cloudprovider/providers/aws/aws_test.go | 10 ++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 35e7e6d50d..7cd44c739d 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -590,6 +590,7 @@ type CloudConfig struct { URL string SigningRegion string SigningMethod string + SigningName string } } @@ -649,6 +650,7 @@ func (cfg *CloudConfig) getResolver() endpoints.ResolverFunc { URL: override.URL, SigningRegion: override.SigningRegion, SigningMethod: override.SigningMethod, + SigningName: override.SigningName, }, nil } } diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 1a6333764d..62f7186af0 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -190,6 +190,7 @@ type ServiceDescriptor struct { name string region string signingRegion, signingMethod string + signingName string } func TestOverridesActiveConfig(t *testing.T) { @@ -376,12 +377,12 @@ func TestOverridesActiveConfig(t *testing.T) { URL=https://s3.foo.bar SigningRegion=sregion1 SigningMethod = v4 - + SigningName = "name" `), nil, false, true, []ServiceDescriptor{{name: "s3", region: "region1", signingRegion: "sregion1", signingMethod: "v3"}, - {name: "s3", region: "region2", signingRegion: "sregion1", signingMethod: "v4"}}, + {name: "s3", region: "region2", signingRegion: "sregion1", signingMethod: "v4", signingName: "name"}}, }, } @@ -411,6 +412,7 @@ func TestOverridesActiveConfig(t *testing.T) { URL string SigningRegion string SigningMethod string + SigningName string } for _, v := range cfg.ServiceOverride { if v.Service == sd.name && v.Region == sd.region { @@ -435,6 +437,10 @@ func TestOverridesActiveConfig(t *testing.T) { t.Errorf("Expected Endpoint '%s', received '%s' for case %s", targetName, found.URL, test.name) } + if found.SigningName != sd.signingName { + t.Errorf("Expected signing name '%s', received '%s' for case %s", + sd.signingName, found.SigningName, test.name) + } fn := cfg.getResolver() ep1, e := fn(sd.name, sd.region, nil)