From 4b71875d671fbb32fbb7f7e410df93494c8b7084 Mon Sep 17 00:00:00 2001 From: Yusuke KUOKA Date: Sat, 7 May 2016 01:15:55 +0900 Subject: [PATCH] Error out when there is ambiguity in cluster.yaml. Which configuration did you want, single AZ or multi AZ? ref https://github.com/coreos/coreos-kubernetes/pull/439#issuecomment-217309099 --- multi-node/aws/pkg/config/config.go | 86 ++++++++++++++----- multi-node/aws/pkg/config/config_test.go | 32 +++++-- multi-node/aws/pkg/config/tls_config_test.go | 2 +- .../aws/pkg/config/user_data_config_test.go | 2 +- 4 files changed, 93 insertions(+), 29 deletions(-) diff --git a/multi-node/aws/pkg/config/config.go b/multi-node/aws/pkg/config/config.go index 9fd4b119ad..db83079fc2 100644 --- a/multi-node/aws/pkg/config/config.go +++ b/multi-node/aws/pkg/config/config.go @@ -32,7 +32,6 @@ func newDefaultCluster() *Cluster { ClusterName: "kubernetes", ReleaseChannel: "alpha", VPCCIDR: "10.0.0.0/16", - InstanceCIDR: "10.0.0.0/24", ControllerIP: "10.0.0.50", PodCIDR: "10.2.0.0/16", ServiceCIDR: "10.3.0.0/24", @@ -75,7 +74,20 @@ func ClusterFromBytes(data []byte) (*Cluster, error) { // as it will with RecordSets c.HostedZone = WithTrailingDot(c.HostedZone) + // If the user specified no subnets, we assume that a single AZ configuration with the default instanceCIDR is demanded + if len(c.Subnets) == 0 && c.InstanceCIDR == "" { + c.InstanceCIDR = "10.0.0.0/24" + } + + if err := c.valid(); err != nil { + return nil, fmt.Errorf("invalid cluster: %v", err) + } + + // For backward-compatibility if len(c.Subnets) == 0 { + if c.InstanceCIDR == "" { + c.InstanceCIDR = "10.0.0.0/24" + } c.Subnets = []Subnet{ { AvailabilityZone: c.AvailabilityZone, @@ -83,10 +95,6 @@ func ClusterFromBytes(data []byte) (*Cluster, error) { }, } } - - if err := c.valid(); err != nil { - return nil, fmt.Errorf("invalid cluster: %v", err) - } return c, nil } @@ -403,35 +411,71 @@ func (cfg Cluster) valid() error { return fmt.Errorf("invalid controllerIP: %s", cfg.ControllerIP) } - var instanceCIDRs = make([]*net.IPNet, 0) - for i, subnet := range cfg.Subnets { - if subnet.AvailabilityZone == "" { - return fmt.Errorf("availabilityZone must be set for subnet #%d", i) + if len(cfg.Subnets) == 0 { + if cfg.AvailabilityZone == "" { + return fmt.Errorf("availabilityZone must be set") } - _, instanceCIDR, err := net.ParseCIDR(subnet.InstanceCIDR) + _, instanceCIDR, err := net.ParseCIDR(cfg.InstanceCIDR) if err != nil { - return fmt.Errorf("invalid instanceCIDR for subnet #%d: %v", i, err) + return fmt.Errorf("invalid instanceCIDR: %v", err) } - instanceCIDRs = append(instanceCIDRs, instanceCIDR) if !vpcNet.Contains(instanceCIDR.IP) { - return fmt.Errorf("vpcCIDR (%s) does not contain instanceCIDR (%s) for subnet #%d", + return fmt.Errorf("vpcCIDR (%s) does not contain instanceCIDR (%s)", cfg.VPCCIDR, cfg.InstanceCIDR, - i, ) } - if i == 0 && !instanceCIDR.Contains(controllerIPAddr) { + if !instanceCIDR.Contains(controllerIPAddr) { return fmt.Errorf("instanceCIDR (%s) does not contain controllerIP (%s)", - subnet.InstanceCIDR, + cfg.InstanceCIDR, + cfg.ControllerIP, + ) + } + } else { + if cfg.InstanceCIDR != "" { + return fmt.Errorf("The top-level instanceCIDR(%s) must be empty when subnets are specified", cfg.InstanceCIDR) + } + if cfg.AvailabilityZone != "" { + return fmt.Errorf("The top-level availabilityZone(%s) must be empty when subnets are specified", cfg.AvailabilityZone) + } + + var instanceCIDRs = make([]*net.IPNet, 0) + for i, subnet := range cfg.Subnets { + if subnet.AvailabilityZone == "" { + return fmt.Errorf("availabilityZone must be set for subnet #%d", i) + } + _, instanceCIDR, err := net.ParseCIDR(subnet.InstanceCIDR) + if err != nil { + return fmt.Errorf("invalid instanceCIDR for subnet #%d: %v", i, err) + } + instanceCIDRs = append(instanceCIDRs, instanceCIDR) + if !vpcNet.Contains(instanceCIDR.IP) { + return fmt.Errorf("vpcCIDR (%s) does not contain instanceCIDR (%s) for subnet #%d", + cfg.VPCCIDR, + cfg.InstanceCIDR, + i, + ) + } + } + + controllerInstanceCidrExists := false + for _, a := range instanceCIDRs { + if a.Contains(controllerIPAddr) { + controllerInstanceCidrExists = true + } + } + if !controllerInstanceCidrExists { + return fmt.Errorf("No instanceCIDRs in Subnets (%v) contain controllerIP (%s)", + instanceCIDRs, cfg.ControllerIP, ) } - } - for i, a := range instanceCIDRs { - for j, b := range instanceCIDRs[i+1:] { - if i > 0 && cidrOverlap(a, b) { - return fmt.Errorf("CIDR of subnet %d (%s) overlaps with CIDR of subnet %d (%s)", i, a, j, b) + for i, a := range instanceCIDRs { + for j, b := range instanceCIDRs[i+1:] { + if i > 0 && cidrOverlap(a, b) { + return fmt.Errorf("CIDR of subnet %d (%s) overlaps with CIDR of subnet %d (%s)", i, a, j, b) + } } } } diff --git a/multi-node/aws/pkg/config/config_test.go b/multi-node/aws/pkg/config/config_test.go index 4b3734d006..f9b5f8c870 100644 --- a/multi-node/aws/pkg/config/config_test.go +++ b/multi-node/aws/pkg/config/config_test.go @@ -9,11 +9,16 @@ import ( const minimalConfigYaml = `externalDNSName: test.staging.core-os.net keyName: test-key-name region: us-west-1 -availabilityZone: us-west-1c clusterName: test-cluster-name kmsKeyArn: "arn:aws:kms:us-west-1:xxxxxxxxx:key/xxxxxxxxxxxxxxxxxxx" ` +const availabilityZoneConfig = ` +availabilityZone: us-west-1c +` + +const singleAzConfigYaml = minimalConfigYaml + availabilityZoneConfig + var goodNetworkingConfigs = []string{ ``, //Tests validity of default network config values ` @@ -116,14 +121,14 @@ hostedZone: "whatever.com" func TestNetworkValidation(t *testing.T) { for _, networkConfig := range goodNetworkingConfigs { - configBody := minimalConfigYaml + networkConfig + configBody := singleAzConfigYaml + networkConfig if _, err := ClusterFromBytes([]byte(configBody)); err != nil { t.Errorf("Correct config tested invalid: %s\n%s", err, networkConfig) } } for _, networkConfig := range incorrectNetworkingConfigs { - configBody := minimalConfigYaml + networkConfig + configBody := singleAzConfigYaml + networkConfig if _, err := ClusterFromBytes([]byte(configBody)); err == nil { t.Errorf("Incorrect config tested valid, expected error:\n%s", networkConfig) } @@ -170,7 +175,7 @@ dnsServiceIP: 10.6.142.100 } for _, testConfig := range testConfigs { - configBody := minimalConfigYaml + testConfig.NetworkConfig + configBody := singleAzConfigYaml + testConfig.NetworkConfig cluster, err := ClusterFromBytes([]byte(configBody)) if err != nil { t.Errorf("Unexpected error parsing config: %v\n %s", err, configBody) @@ -286,7 +291,7 @@ releaseChannel: non-existant #this release channel will never exist } for _, conf := range validConfigs { - confBody := minimalConfigYaml + conf.conf + confBody := singleAzConfigYaml + conf.conf c, err := ClusterFromBytes([]byte(confBody)) if err != nil { t.Errorf("failed to parse config %s: %v", confBody, err) @@ -302,7 +307,7 @@ releaseChannel: non-existant #this release channel will never exist } for _, conf := range invalidConfigs { - confBody := minimalConfigYaml + conf + confBody := singleAzConfigYaml + conf _, err := ClusterFromBytes([]byte(confBody)) if err == nil { t.Errorf("expected error parsing invalid config: %s", confBody) @@ -399,7 +404,22 @@ availabilityZone: "ap-northeast-1a" invalidConfigs := []string{ ` +# You can't specify both the top-level availability zone and subnets +# (It doesn't make sense. Which configuration did you want, single or multi AZ one?) availabilityZone: "ap-northeast-1a" +subnets: + - availabilityZone: "ap-northeast-1b" + instanceCIDR: "10.0.0.0/24" +`, + ` +# You can't specify both the top-level instanceCIDR and subnets +# (It doesn't make sense. Which configuration did you want, single or multi AZ one?) +instanceCIDR: "10.0.0.0/24" +subnets: +- availabilityZone: "ap-northeast-1b" + instanceCIDR: "10.0.1.0/24" +`, + ` subnets: # Missing AZ like this # - availabilityZone: "ap-northeast-1a" diff --git a/multi-node/aws/pkg/config/tls_config_test.go b/multi-node/aws/pkg/config/tls_config_test.go index a8af14a639..90877dba3d 100644 --- a/multi-node/aws/pkg/config/tls_config_test.go +++ b/multi-node/aws/pkg/config/tls_config_test.go @@ -9,7 +9,7 @@ import ( ) func genTLSAssets(t *testing.T) *RawTLSAssets { - cluster, err := ClusterFromBytes([]byte(minimalConfigYaml)) + cluster, err := ClusterFromBytes([]byte(singleAzConfigYaml)) if err != nil { t.Fatalf("failed generating config: %v", err) } diff --git a/multi-node/aws/pkg/config/user_data_config_test.go b/multi-node/aws/pkg/config/user_data_config_test.go index 3e8b231a5c..00ae0c49d6 100644 --- a/multi-node/aws/pkg/config/user_data_config_test.go +++ b/multi-node/aws/pkg/config/user_data_config_test.go @@ -19,7 +19,7 @@ func (d *dummyEncryptService) Encrypt(input *kms.EncryptInput) (*kms.EncryptOutp } func TestCloudConfigTemplating(t *testing.T) { - cluster, err := ClusterFromBytes([]byte(minimalConfigYaml)) + cluster, err := ClusterFromBytes([]byte(singleAzConfigYaml)) if err != nil { t.Fatalf("Unable to load cluster config: %v", err) }