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

@@ -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...
+AWS_ZONE=${KUBE_AWS_ZONE:-us-west-2a}
-ZONE=${KUBE_AWS_ZONE:-us-west-2}
 MASTER_SIZE=${MASTER_SIZE:-t2.micro}
 MINION_SIZE=${MINION_SIZE:-t2.micro}
 NUM_MINIONS=${NUM_MINIONS:-4}

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...
+AWS_ZONE=${KUBE_AWS_ZONE:-us-west-2a}
-ZONE=${KUBE_AWS_ZONE:-us-west-2}
 MASTER_SIZE=${MASTER_SIZE:-t2.micro}
 MINION_SIZE=${MINION_SIZE:-t2.micro}
 NUM_MINIONS=${NUM_MINIONS:-2}

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.

cluster/aws/templates/salt-master.sh

@@ -28,7 +28,7 @@ EOF
 
 cat <<EOF > /etc/aws.conf
 [Global]
-Region = ${AWS_ZONE}
+Zone = ${AWS_ZONE}
 EOF
 
 # Auto accept all keys from minions that try to join

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}'"

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 == "" {
+	if cfg.Global.Zone == "" {
-		return nil, fmt.Errorf("no region specified in configuration file")
+		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) {
+func newAWSCloud(config io.Reader, authFunc AuthFunc, metadata AWSMetadata) (*AWSCloud, error) {
-	cfg, err := readAWSCloudConfig(config)
+	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
+	zone := cfg.Global.Zone
-	region, ok := aws.Regions[cfg.Global.Region]
+	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)},
+		ec2:              &GoamzEC2{ec2: ec2.New(auth, region)},
-		cfg:    cfg,
+		cfg:              cfg,
-		region: region,
+		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 self.availabilityZone == "" {
-	if err != nil {
+		// Should be unreachable
-		return cloudprovider.Zone{}, err
+		panic("availabilityZone not set")
 	}
 	return cloudprovider.Zone{
-		FailureDomain: availabilityZone,
+		FailureDomain: self.availabilityZone,
 		Region:        self.region.Name,
 	}, nil
 }

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" {
+	if cfg.Global.Zone != "eu-west-1a" {
-		t.Errorf("Should read region from config")
+		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"),
+		"[global]\nzone = blahonga"),
-		fakeAuthFunc)
+		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"),
+		strings.NewReader("[global]\nzone = eu-west-1a"),
-		fakeAuthFunc)
+		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
+	instances []ec2.Instance
-	availabilityZone string
 }
 
 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,
+			instances: instances,
-			availabilityZone: availabilityZone,
 		},
+		availabilityZone: availabilityZone,
 	}
 }
 
 func mockAvailabilityZone(region string, availabilityZone string) *AWSCloud {
 	return &AWSCloud{
-		ec2: &FakeEC2{
+		ec2:              &FakeEC2{},
-			availabilityZone: availabilityZone,
+		availabilityZone: availabilityZone,
-		},
+		region:           aws.Regions[region],
-		region: aws.Regions[region],
 	}
+
 }
 
 func TestList(t *testing.T) {