From 583892da2db1fe5b1fb0d9800e4389e7579c5fe9 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Thu, 26 Mar 2015 12:47:49 -0700 Subject: [PATCH] Fix AWS region vs zone We were specifying a region, but naming it as a zone in util.sh The zone matters just as much as the region, e.g. for EBS volumes. We also change the config to require a Zone, not a Region. But we fallback to get the information from the metadata service. --- cluster/aws/config-default.sh | 3 +- cluster/aws/config-test.sh | 3 +- cluster/aws/options.md | 4 ++ cluster/aws/templates/salt-master.sh | 2 +- cluster/aws/util.sh | 9 ++- pkg/cloudprovider/aws/aws.go | 87 ++++++++++++++++----------- pkg/cloudprovider/aws/aws_test.go | 88 ++++++++++++++++++++-------- 7 files changed, 129 insertions(+), 67 deletions(-) diff --git a/cluster/aws/config-default.sh b/cluster/aws/config-default.sh index cace50a291..0c18ed124b 100644 --- a/cluster/aws/config-default.sh +++ b/cluster/aws/config-default.sh @@ -14,8 +14,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -# TODO: the zone isn't quite piped into all the right places... -ZONE=${KUBE_AWS_ZONE:-us-west-2} +AWS_ZONE=${KUBE_AWS_ZONE:-us-west-2a} MASTER_SIZE=${MASTER_SIZE:-t2.micro} MINION_SIZE=${MINION_SIZE:-t2.micro} NUM_MINIONS=${NUM_MINIONS:-4} diff --git a/cluster/aws/config-test.sh b/cluster/aws/config-test.sh index 9f63ce1dee..27a50b2ef8 100755 --- a/cluster/aws/config-test.sh +++ b/cluster/aws/config-test.sh @@ -14,8 +14,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -# TODO: this isn't quite piped into all the right places... -ZONE=${KUBE_AWS_ZONE:-us-west-2} +AWS_ZONE=${KUBE_AWS_ZONE:-us-west-2a} MASTER_SIZE=${MASTER_SIZE:-t2.micro} MINION_SIZE=${MINION_SIZE:-t2.micro} NUM_MINIONS=${NUM_MINIONS:-2} diff --git a/cluster/aws/options.md b/cluster/aws/options.md index fcc5e01cdc..513af1f339 100644 --- a/cluster/aws/options.md +++ b/cluster/aws/options.md @@ -5,6 +5,10 @@ specific to AWS are documented in this file, for cross-provider options see TODO This is a work-in-progress; not all options are documented yet! +## KUBE_AWS_ZONE + +The AWS availability to deploy to. Defaults to us-west-2a. + ## AWS_IMAGE The AMI to use. If not specified, the image will be selected based on the AWS region. diff --git a/cluster/aws/templates/salt-master.sh b/cluster/aws/templates/salt-master.sh index 10326aa7f8..258a9e64bb 100755 --- a/cluster/aws/templates/salt-master.sh +++ b/cluster/aws/templates/salt-master.sh @@ -28,7 +28,7 @@ EOF cat < /etc/aws.conf [Global] -Region = ${AWS_ZONE} +Zone = ${AWS_ZONE} EOF # Auto accept all keys from minions that try to join diff --git a/cluster/aws/util.sh b/cluster/aws/util.sh index 8909cef813..85be47073b 100644 --- a/cluster/aws/util.sh +++ b/cluster/aws/util.sh @@ -21,7 +21,10 @@ KUBE_ROOT=$(dirname "${BASH_SOURCE}")/../.. source "${KUBE_ROOT}/cluster/aws/${KUBE_CONFIG_FILE-"config-default.sh"}" -export AWS_DEFAULT_REGION=${ZONE} +# This removes the final character in bash (somehow) +AWS_REGION=${AWS_ZONE%?} + +export AWS_DEFAULT_REGION=${AWS_REGION} AWS_CMD="aws --output json ec2" AWS_ELB_CMD="aws --output json elb" @@ -105,7 +108,7 @@ function detect-image () { # See here: http://cloud-images.ubuntu.com/locator/ec2/ for other images # This will need to be updated from time to time as amis are deprecated if [[ -z "${AWS_IMAGE-}" ]]; then - case "${ZONE}" in + case "${AWS_REGION}" in ap-northeast-1) AWS_IMAGE=ami-93876e93 ;; @@ -428,7 +431,7 @@ function kube-up { echo "readonly NODE_INSTANCE_PREFIX='${INSTANCE_PREFIX}-minion'" echo "readonly SERVER_BINARY_TAR_URL='${SERVER_BINARY_TAR_URL}'" echo "readonly SALT_TAR_URL='${SALT_TAR_URL}'" - echo "readonly AWS_ZONE='${ZONE}'" + echo "readonly AWS_ZONE='${AWS_ZONE}'" echo "readonly MASTER_HTPASSWD='${htpasswd}'" echo "readonly PORTAL_NET='${PORTAL_NET}'" echo "readonly ENABLE_CLUSTER_MONITORING='${ENABLE_CLUSTER_MONITORING:-false}'" diff --git a/pkg/cloudprovider/aws/aws.go b/pkg/cloudprovider/aws/aws.go index c740a15afd..4e5f01bf65 100644 --- a/pkg/cloudprovider/aws/aws.go +++ b/pkg/cloudprovider/aws/aws.go @@ -38,7 +38,10 @@ import ( type EC2 interface { // Query EC2 for instances matching the filter Instances(instIds []string, filter *ec2InstanceFilter) (resp *ec2.InstancesResp, err error) +} +// Abstraction over the AWS metadata service +type AWSMetadata interface { // Query the EC2 metadata service (used to discover instance-id etc) GetMetaData(key string) ([]byte, error) } @@ -54,7 +57,7 @@ type AWSCloud struct { type AWSCloudConfig struct { Global struct { // TODO: Is there any use for this? We can get it from the instance metadata service - Region string + Zone string } } @@ -89,7 +92,11 @@ func (self *GoamzEC2) Instances(instanceIds []string, filter *ec2InstanceFilter) return self.ec2.Instances(instanceIds, goamzFilter) } -func (self *GoamzEC2) GetMetaData(key string) ([]byte, error) { +type goamzMetadata struct { +} + +// Implements AWSMetadata.GetMetaData +func (self *goamzMetadata) GetMetaData(key string) ([]byte, error) { v, err := aws.GetMetaData(key) if err != nil { return nil, fmt.Errorf("Error querying AWS metadata for key %s: %v", key, err) @@ -101,7 +108,8 @@ type AuthFunc func() (auth aws.Auth, err error) func init() { cloudprovider.RegisterCloudProvider("aws", func(config io.Reader) (cloudprovider.Interface, error) { - return newAWSCloud(config, getAuth) + metadata := &goamzMetadata{} + return newAWSCloud(config, getAuth, metadata) }) } @@ -110,7 +118,7 @@ func getAuth() (auth aws.Auth, err error) { } // readAWSCloudConfig reads an instance of AWSCloudConfig from config reader. -func readAWSCloudConfig(config io.Reader) (*AWSCloudConfig, error) { +func readAWSCloudConfig(config io.Reader, metadata AWSMetadata) (*AWSCloudConfig, error) { if config == nil { return nil, fmt.Errorf("no AWS cloud provider config file given") } @@ -121,16 +129,36 @@ func readAWSCloudConfig(config io.Reader) (*AWSCloudConfig, error) { return nil, err } - if cfg.Global.Region == "" { - return nil, fmt.Errorf("no region specified in configuration file") + if cfg.Global.Zone == "" { + if metadata != nil { + glog.Info("Zone not specified in configuration file; querying AWS metadata service") + cfg.Global.Zone, err = getAvailabilityZone(metadata) + if err != nil { + return nil, err + } + } + if cfg.Global.Zone == "" { + return nil, fmt.Errorf("no zone specified in configuration file") + } } return &cfg, nil } +func getAvailabilityZone(metadata AWSMetadata) (string, error) { + availabilityZoneBytes, err := metadata.GetMetaData("placement/availability-zone") + if err != nil { + return "", err + } + if availabilityZoneBytes == nil || len(availabilityZoneBytes) == 0 { + return "", fmt.Errorf("Unable to determine availability-zone from instance metadata") + } + return string(availabilityZoneBytes), nil +} + // newAWSCloud creates a new instance of AWSCloud. -func newAWSCloud(config io.Reader, authFunc AuthFunc) (*AWSCloud, error) { - cfg, err := readAWSCloudConfig(config) +func newAWSCloud(config io.Reader, authFunc AuthFunc, metadata AWSMetadata) (*AWSCloud, error) { + cfg, err := readAWSCloudConfig(config, metadata) if err != nil { return nil, fmt.Errorf("unable to read AWS cloud provider config file: %v", err) } @@ -140,36 +168,25 @@ func newAWSCloud(config io.Reader, authFunc AuthFunc) (*AWSCloud, error) { return nil, err } - // TODO: We can get the region very easily from the instance-metadata service - region, ok := aws.Regions[cfg.Global.Region] + zone := cfg.Global.Zone + if len(zone) <= 1 { + return nil, fmt.Errorf("invalid AWS zone in config file: %s", zone) + } + regionName := zone[:len(zone)-1] + + region, ok := aws.Regions[regionName] if !ok { - return nil, fmt.Errorf("not a valid AWS region: %s", cfg.Global.Region) + return nil, fmt.Errorf("not a valid AWS zone (unknown region): %s", zone) } return &AWSCloud{ - ec2: &GoamzEC2{ec2: ec2.New(auth, region)}, - cfg: cfg, - region: region, + ec2: &GoamzEC2{ec2: ec2.New(auth, region)}, + cfg: cfg, + region: region, + availabilityZone: zone, }, nil } -func (self *AWSCloud) getAvailabilityZone() (string, error) { - // TODO: Do we need sync.Mutex here? - availabilityZone := self.availabilityZone - if self.availabilityZone == "" { - availabilityZoneBytes, err := self.ec2.GetMetaData("placement/availability-zone") - if err != nil { - return "", err - } - if availabilityZoneBytes == nil || len(availabilityZoneBytes) == 0 { - return "", fmt.Errorf("Unable to determine availability-zone from instance metadata") - } - availabilityZone = string(availabilityZoneBytes) - self.availabilityZone = availabilityZone - } - return availabilityZone, nil -} - func (aws *AWSCloud) Clusters() (cloudprovider.Clusters, bool) { return nil, false } @@ -467,12 +484,12 @@ func getResourcesByInstanceType(instanceType string) (*api.NodeResources, error) // GetZone implements Zones.GetZone func (self *AWSCloud) GetZone() (cloudprovider.Zone, error) { - availabilityZone, err := self.getAvailabilityZone() - if err != nil { - return cloudprovider.Zone{}, err + if self.availabilityZone == "" { + // Should be unreachable + panic("availabilityZone not set") } return cloudprovider.Zone{ - FailureDomain: availabilityZone, + FailureDomain: self.availabilityZone, Region: self.region.Name, }, nil } diff --git a/pkg/cloudprovider/aws/aws_test.go b/pkg/cloudprovider/aws/aws_test.go index 1cef17c91b..641e59090a 100644 --- a/pkg/cloudprovider/aws/aws_test.go +++ b/pkg/cloudprovider/aws/aws_test.go @@ -29,27 +29,50 @@ import ( ) func TestReadAWSCloudConfig(t *testing.T) { - _, err1 := readAWSCloudConfig(nil) + _, err1 := readAWSCloudConfig(nil, nil) if err1 == nil { t.Errorf("Should error when no config reader is given") } - _, err2 := readAWSCloudConfig(strings.NewReader("")) + _, err2 := readAWSCloudConfig(strings.NewReader(""), nil) if err2 == nil { t.Errorf("Should error when config is empty") } - _, err3 := readAWSCloudConfig(strings.NewReader("[global]\n")) + _, err3 := readAWSCloudConfig(strings.NewReader("[global]\n"), nil) if err3 == nil { - t.Errorf("Should error when no region is specified") + t.Errorf("Should error when no zone is specified") } - cfg, err4 := readAWSCloudConfig(strings.NewReader("[global]\nregion = eu-west-1")) + cfg, err4 := readAWSCloudConfig(strings.NewReader("[global]\nzone = eu-west-1a"), nil) if err4 != nil { - t.Errorf("Should succeed when a region is specified: %s", err4) + t.Errorf("Should succeed when a zone is specified: %s", err4) } - if cfg.Global.Region != "eu-west-1" { - t.Errorf("Should read region from config") + if cfg.Global.Zone != "eu-west-1a" { + t.Errorf("Should read zone from config") + } + + _, err5 := readAWSCloudConfig(strings.NewReader("[global]\n"), &FakeMetadata{}) + if err5 == nil { + t.Errorf("Should error when no zone is specified in metadata") + } + + cfg, err6 := readAWSCloudConfig(strings.NewReader("[global]\n"), + &FakeMetadata{availabilityZone: "eu-west-1a"}) + if err6 != nil { + t.Errorf("Should succeed when getting zone from metadata: %s", err6) + } + if cfg.Global.Zone != "eu-west-1a" { + t.Errorf("Should read zone from metadata") + } + + cfg, err7 := readAWSCloudConfig(strings.NewReader("[global]\nzone = us-east-1a"), + &FakeMetadata{availabilityZone: "eu-west-1a"}) + if err7 != nil { + t.Errorf("Should succeed when zone is specified: %s", err7) + } + if cfg.Global.Zone != "us-east-1a" { + t.Errorf("Should prefer zone from config over metadata") } } @@ -58,29 +81,42 @@ func TestNewAWSCloud(t *testing.T) { return aws.Auth{"", "", ""}, nil } - _, err1 := newAWSCloud(nil, fakeAuthFunc) + _, err1 := newAWSCloud(nil, fakeAuthFunc, nil) if err1 == nil { t.Errorf("Should error when no config reader is given") } _, err2 := newAWSCloud(strings.NewReader( - "[global]\nregion = blahonga"), - fakeAuthFunc) + "[global]\nzone = blahonga"), + fakeAuthFunc, nil) if err2 == nil { - t.Errorf("Should error when config specifies invalid region") + t.Errorf("Should error when config specifies invalid zone") } _, err3 := newAWSCloud( - strings.NewReader("[global]\nregion = eu-west-1"), - fakeAuthFunc) + strings.NewReader("[global]\nzone = eu-west-1a"), + fakeAuthFunc, nil) if err3 != nil { - t.Errorf("Should succeed when a valid region is specified: %s", err3) + t.Errorf("Should succeed when a valid zone is specified: %s", err3) + } + + _, err4 := newAWSCloud(strings.NewReader( + "[global]\n"), + fakeAuthFunc, &FakeMetadata{availabilityZone: "us-east-1a"}) + if err4 != nil { + t.Errorf("Should success when zone is in metadata") + } + + _, err5 := newAWSCloud(strings.NewReader( + "[global]\n"), + fakeAuthFunc, &FakeMetadata{}) + if err5 == nil { + t.Errorf("Should error when AZ cannot be found in metadata") } } type FakeEC2 struct { - instances []ec2.Instance - availabilityZone string + instances []ec2.Instance } func (self *FakeEC2) Instances(instanceIds []string, filter *ec2InstanceFilter) (resp *ec2.InstancesResp, err error) { @@ -95,7 +131,11 @@ func (self *FakeEC2) Instances(instanceIds []string, filter *ec2InstanceFilter) {"", "", "", nil, matches}}}, nil } -func (self *FakeEC2) GetMetaData(key string) ([]byte, error) { +type FakeMetadata struct { + availabilityZone string +} + +func (self *FakeMetadata) GetMetaData(key string) ([]byte, error) { if key == "placement/availability-zone" { return []byte(self.availabilityZone), nil } else { @@ -107,19 +147,19 @@ func mockInstancesResp(instances []ec2.Instance) (aws *AWSCloud) { availabilityZone := "us-west-2d" return &AWSCloud{ ec2: &FakeEC2{ - instances: instances, - availabilityZone: availabilityZone, + instances: instances, }, + availabilityZone: availabilityZone, } } func mockAvailabilityZone(region string, availabilityZone string) *AWSCloud { return &AWSCloud{ - ec2: &FakeEC2{ - availabilityZone: availabilityZone, - }, - region: aws.Regions[region], + ec2: &FakeEC2{}, + availabilityZone: availabilityZone, + region: aws.Regions[region], } + } func TestList(t *testing.T) {