Merge pull request #6011 from justinsb/aws_specify_zone

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}
+ZONE=${KUBE_AWS_ZONE:-us-west-2a}
 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...
-ZONE=${KUBE_AWS_ZONE:-us-west-2}
+ZONE=${KUBE_AWS_ZONE:-us-west-2a}
 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!
 
+## ZONE
+
+The AWS availability zone 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 = ${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=${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
         ;;
@@ -429,7 +432,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 ZONE='${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 == "" {
-		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
 }
@@ -480,12 +497,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
 }

pkg/cloudprovider/aws/aws_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package aws_cloud
 
 import (
+	"io"
 	"reflect"
 	"strings"
 	"testing"
@@ -29,27 +30,68 @@ import (
 )
 
 func TestReadAWSCloudConfig(t *testing.T) {
-	_, err1 := readAWSCloudConfig(nil)
-	if err1 == nil {
-		t.Errorf("Should error when no config reader is given")
-	}
+	tests := []struct {
+		name string
 
-	_, err2 := readAWSCloudConfig(strings.NewReader(""))
-	if err2 == nil {
-		t.Errorf("Should error when config is empty")
-	}
+		reader   io.Reader
+		metadata AWSMetadata
 
-	_, err3 := readAWSCloudConfig(strings.NewReader("[global]\n"))
-	if err3 == nil {
-		t.Errorf("Should error when no region is specified")
+		expectError bool
+		zone        string
+	}{
+		{
+			"No config reader",
+			nil, nil,
+			true, "",
+		},
+		{
+			"Empty config, no metadata",
+			strings.NewReader(""), nil,
+			true, "",
+		},
+		{
+			"No zone in config, no metadata",
+			strings.NewReader("[global]\n"), nil,
+			true, "",
+		},
+		{
+			"Zone in config, no metadata",
+			strings.NewReader("[global]\nzone = eu-west-1a"), nil,
+			false, "eu-west-1a",
+		},
+		{
+			"No zone in config, metadata does not have zone",
+			strings.NewReader("[global]\n"), &FakeMetadata{},
+			true, "",
+		},
+		{
+			"No zone in config, metadata has zone",
+			strings.NewReader("[global]\n"), &FakeMetadata{availabilityZone: "eu-west-1a"},
+			false, "eu-west-1a",
+		},
+		{
+			"Zone in config should take precedence over metadata",
+			strings.NewReader("[global]\nzone = us-east-1a"), &FakeMetadata{availabilityZone: "eu-west-1a"},
+			false, "us-east-1a",
+		},
 	}
 
-	cfg, err4 := readAWSCloudConfig(strings.NewReader("[global]\nregion = eu-west-1"))
-	if err4 != nil {
-		t.Errorf("Should succeed when a region is specified: %s", err4)
-	}
-	if cfg.Global.Region != "eu-west-1" {
-		t.Errorf("Should read region from config")
+	for _, test := range tests {
+		t.Logf("Running test case %s", test.name)
+		cfg, err := readAWSCloudConfig(test.reader, test.metadata)
+		if test.expectError {
+			if err == nil {
+				t.Errorf("Should error for case %s", test.name)
+			}
+		} else {
+			if err != nil {
+				t.Errorf("Should succeed for case: %s", test.name)
+			}
+			if cfg.Global.Zone != test.zone {
+				t.Errorf("Incorrect zone value (%s vs %s) for case: %s",
+					cfg.Global.Zone, test.zone, test.name)
+			}
+		}
 	}
 }
 
@@ -58,29 +100,68 @@ func TestNewAWSCloud(t *testing.T) {
 		return aws.Auth{"", "", ""}, nil
 	}
 
-	_, err1 := newAWSCloud(nil, fakeAuthFunc)
-	if err1 == nil {
-		t.Errorf("Should error when no config reader is given")
-	}
+	tests := []struct {
+		name string
 
-	_, err2 := newAWSCloud(strings.NewReader(
-		"[global]\nregion = blahonga"),
-		fakeAuthFunc)
-	if err2 == nil {
-		t.Errorf("Should error when config specifies invalid region")
+		reader   io.Reader
+		authFunc AuthFunc
+		metadata AWSMetadata
+
+		expectError bool
+		zone        string
+	}{
+		{
+			"No config reader",
+			nil, fakeAuthFunc, nil,
+			true, "",
+		},
+		{
+			"Config specified invalid zone",
+			strings.NewReader("[global]\nzone = blahonga"), fakeAuthFunc, nil,
+			true, "",
+		},
+		{
+			"Config specifies valid zone",
+			strings.NewReader("[global]\nzone = eu-west-1a"), fakeAuthFunc, nil,
+			false, "eu-west-1a",
+		},
+		{
+			"Gets zone from metadata when not in config",
+
+			strings.NewReader("[global]\n"),
+			fakeAuthFunc,
+			&FakeMetadata{availabilityZone: "us-east-1a"},
+
+			false, "us-east-1a",
+		},
+		{
+			"No zone in config or metadata",
+			strings.NewReader("[global]\n"), fakeAuthFunc, &FakeMetadata{},
+			true, "",
+		},
 	}
 
-	_, err3 := newAWSCloud(
-		strings.NewReader("[global]\nregion = eu-west-1"),
-		fakeAuthFunc)
-	if err3 != nil {
-		t.Errorf("Should succeed when a valid region is specified: %s", err3)
+	for _, test := range tests {
+		t.Logf("Running test case %s", test.name)
+		c, err := newAWSCloud(test.reader, test.authFunc, test.metadata)
+		if test.expectError {
+			if err == nil {
+				t.Errorf("Should error for case %s", test.name)
+			}
+		} else {
+			if err != nil {
+				t.Errorf("Should succeed for case: %s", test.name)
+			}
+			if c.availabilityZone != test.zone {
+				t.Errorf("Incorrect zone value (%s vs %s) for case: %s",
+					c.availabilityZone, test.zone, test.name)
+			}
+		}
 	}
 }
 
 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 +176,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 +192,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) {