Updates for PR comments

This commit is contained in:
ampsingram 2019-01-16 18:12:49 -05:00
parent 86a8ede789
commit 207a5a1267
2 changed files with 153 additions and 231 deletions

View File

@ -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)
}

View File

@ -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)
}
}
}
}