AWS: Use private dns name for node name again

This is a partial reversion of #9728, and should fix #10612.

9728 used the AWS instance id as the node name.  But proxy, logs
and exec all used the node name as the host name for contacting the minion.

It is possible to resolve a host to the IP, and this fixes logs.  But
exec and proxy also require an SSL certificate match on the hostname,
and this is harder to fix.

So the sensible fix seems to be a minimal reversion of the changes in #9728,
and we can revisit this post 1.0.

pkg/cloudprovider/aws/aws.go

@@ -278,7 +278,7 @@ func (a *AWSCloud) CurrentNodeName(hostname string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	return selfInstance.awsID, nil
+	return selfInstance.nodeName, nil
 }
 
 // Implementation of EC2.Instances
@@ -624,7 +624,7 @@ func (aws *AWSCloud) Routes() (cloudprovider.Routes, bool) {
 
 // NodeAddresses is an implementation of Instances.NodeAddresses.
 func (aws *AWSCloud) NodeAddresses(name string) ([]api.NodeAddress, error) {
-	instance, err := aws.getInstanceById(name)
+	instance, err := aws.getInstanceByNodeName(name)
 	if err != nil {
 		return nil, err
 	}
@@ -660,7 +660,7 @@ func (aws *AWSCloud) NodeAddresses(name string) ([]api.NodeAddress, error) {
 // Note that if the instance does not exist or is no longer running, we must return ("", cloudprovider.InstanceNotFound)
 func (aws *AWSCloud) ExternalID(name string) (string, error) {
 	// We must verify that the instance still exists
-	instance, err := aws.getInstanceById(name)
+	instance, err := aws.getInstanceByNodeName(name)
 	if err != nil {
 		return "", err
 	}
@@ -673,7 +673,7 @@ func (aws *AWSCloud) ExternalID(name string) (string, error) {
 // InstanceID returns the cloud provider ID of the specified instance.
 func (aws *AWSCloud) InstanceID(name string) (string, error) {
 	// TODO: Do we need to verify it exists, or can we just construct it knowing our AZ (or via caching?)
-	inst, err := aws.getInstanceById(name)
+	inst, err := aws.getInstanceByNodeName(name)
 	if err != nil {
 		return "", err
 	}
@@ -741,9 +741,16 @@ func (s *AWSCloud) getInstancesByRegex(regex string) ([]string, error) {
 			continue
 		}
 
+		privateDNSName := orEmpty(instance.PrivateDNSName)
+		if privateDNSName == "" {
+			glog.V(2).Infof("skipping EC2 instance (no PrivateDNSName): %s",
+				orEmpty(instance.InstanceID))
+			continue
+		}
+
 		for _, tag := range instance.Tags {
 			if orEmpty(tag.Key) == "Name" && re.MatchString(orEmpty(tag.Value)) {
-				matchingInstances = append(matchingInstances, orEmpty(instance.InstanceID))
+				matchingInstances = append(matchingInstances, privateDNSName)
 				break
 			}
 		}
@@ -760,7 +767,7 @@ func (aws *AWSCloud) List(filter string) ([]string, error) {
 
 // GetNodeResources implements Instances.GetNodeResources
 func (aws *AWSCloud) GetNodeResources(name string) (*api.NodeResources, error) {
-	instance, err := aws.getInstanceById(name)
+	instance, err := aws.getInstanceByNodeName(name)
 	if err != nil {
 		return nil, err
 	}
@@ -968,6 +975,9 @@ type awsInstance struct {
 	// id in AWS
 	awsID string
 
+	// node name in k8s
+	nodeName string
+
 	mutex sync.Mutex
 
 	// We must cache because otherwise there is a race condition,
@@ -975,8 +985,8 @@ type awsInstance struct {
 	deviceMappings map[string]string
 }
 
-func newAWSInstance(ec2 EC2, awsID string) *awsInstance {
+func newAWSInstance(ec2 EC2, awsID, nodeName string) *awsInstance {
-	self := &awsInstance{ec2: ec2, awsID: awsID}
+	self := &awsInstance{ec2: ec2, awsID: awsID, nodeName: nodeName}
 
 	// We lazy-init deviceMappings
 	self.deviceMappings = nil
@@ -1216,29 +1226,34 @@ func (s *AWSCloud) getSelfAWSInstance() (*awsInstance, error) {
 		if err != nil {
 			return nil, fmt.Errorf("error fetching instance-id from ec2 metadata service: %v", err)
 		}
-		i = newAWSInstance(s.ec2, string(instanceIdBytes))
+		privateDnsNameBytes, err := metadata.GetMetaData("local-hostname")
+		if err != nil {
+			return nil, fmt.Errorf("error fetching local-hostname from ec2 metadata service: %v", err)
+		}
+
+		i = newAWSInstance(s.ec2, string(instanceIdBytes), string(privateDnsNameBytes))
 		s.selfAWSInstance = i
 	}
 
 	return i, nil
 }
 
-// Gets the awsInstance named instanceName, or the 'self' instance if instanceName == ""
+// Gets the awsInstance with node-name nodeName, or the 'self' instance if nodeName == ""
-func (aws *AWSCloud) getAwsInstance(instanceName string) (*awsInstance, error) {
+func (aws *AWSCloud) getAwsInstance(nodeName string) (*awsInstance, error) {
 	var awsInstance *awsInstance
 	var err error
-	if instanceName == "" {
+	if nodeName == "" {
 		awsInstance, err = aws.getSelfAWSInstance()
 		if err != nil {
 			return nil, fmt.Errorf("error getting self-instance: %v", err)
 		}
 	} else {
-		instance, err := aws.getInstanceById(instanceName)
+		instance, err := aws.getInstanceByNodeName(nodeName)
 		if err != nil {
-			return nil, fmt.Errorf("error finding instance: %v", err)
+			return nil, fmt.Errorf("error finding instance %s: %v", nodeName, err)
 		}
 
-		awsInstance = newAWSInstance(aws.ec2, orEmpty(instance.InstanceID))
+		awsInstance = newAWSInstance(aws.ec2, orEmpty(instance.InstanceID), orEmpty(instance.PrivateDNSName))
 	}
 
 	return awsInstance, nil
@@ -1700,7 +1715,7 @@ func (s *AWSCloud) CreateTCPLoadBalancer(name, region string, publicIP net.IP, p
 		return nil, fmt.Errorf("publicIP cannot be specified for AWS ELB")
 	}
 
-	instances, err := s.getInstancesByIds(hosts)
+	instances, err := s.getInstancesByNodeNames(hosts)
 	if err != nil {
 		return nil, err
 	}
@@ -2112,7 +2127,7 @@ func (s *AWSCloud) EnsureTCPLoadBalancerDeleted(name, region string) error {
 
 // UpdateTCPLoadBalancer implements TCPLoadBalancer.UpdateTCPLoadBalancer
 func (s *AWSCloud) UpdateTCPLoadBalancer(name, region string, hosts []string) error {
-	instances, err := s.getInstancesByIds(hosts)
+	instances, err := s.getInstancesByNodeNames(hosts)
 	if err != nil {
 		return err
 	}
@@ -2221,6 +2236,45 @@ func (a *AWSCloud) getInstanceById(instanceID string) (*ec2.Instance, error) {
 	return instances[0], nil
 }
 
+// TODO: Make efficient
+func (a *AWSCloud) getInstancesByNodeNames(nodeNames []string) ([]*ec2.Instance, error) {
+	instances := []*ec2.Instance{}
+	for _, nodeName := range nodeNames {
+		instance, err := a.getInstanceByNodeName(nodeName)
+		if err != nil {
+			return nil, err
+		}
+		if instance == nil {
+			return nil, fmt.Errorf("unable to find instance " + nodeName)
+		}
+		instances = append(instances, instance)
+	}
+	return instances, nil
+}
+
+// Returns the instance with the specified node name
+func (a *AWSCloud) getInstanceByNodeName(nodeName string) (*ec2.Instance, error) {
+	filters := []*ec2.Filter{
+		newEc2Filter("private-dns-name", nodeName),
+	}
+	filters = a.addFilters(filters)
+	request := &ec2.DescribeInstancesInput{
+		Filters: filters,
+	}
+
+	instances, err := a.ec2.DescribeInstances(request)
+	if err != nil {
+		return nil, err
+	}
+	if len(instances) == 0 {
+		return nil, fmt.Errorf("no instances found for name: %s", nodeName)
+	}
+	if len(instances) > 1 {
+		return nil, fmt.Errorf("multiple instances found for name: %s", nodeName)
+	}
+	return instances[0], nil
+}
+
 // Add additional filters, to match on our tags
 // This lets us run multiple k8s clusters in a single EC2 AZ
 func (s *AWSCloud) addFilters(filters []*ec2.Filter) []*ec2.Filter {

pkg/cloudprovider/aws/aws_routes.go

@@ -85,9 +85,14 @@ func (s *AWSCloud) configureInstanceSourceDestCheck(instanceID string, sourceDes
 // CreateRoute implements Routes.CreateRoute
 // Create the described route
 func (s *AWSCloud) CreateRoute(clusterName string, nameHint string, route *cloudprovider.Route) error {
+	instance, err := s.getInstanceByNodeName(route.TargetInstance)
+	if err != nil {
+		return err
+	}
+
 	// In addition to configuring the route itself, we also need to configure the instance to accept that traffic
 	// On AWS, this requires turning source-dest checks off
-	err := s.configureInstanceSourceDestCheck(route.TargetInstance, false)
+	err = s.configureInstanceSourceDestCheck(orEmpty(instance.InstanceID), false)
 	if err != nil {
 		return err
 	}
@@ -100,7 +105,7 @@ func (s *AWSCloud) CreateRoute(clusterName string, nameHint string, route *cloud
 	request := &ec2.CreateRouteInput{}
 	// TODO: use ClientToken for idempotency?
 	request.DestinationCIDRBlock = aws.String(route.DestinationCIDR)
-	request.InstanceID = aws.String(route.TargetInstance)
+	request.InstanceID = instance.InstanceID
 	request.RouteTableID = table.RouteTableID
 
 	_, err = s.ec2.CreateRoute(request)

pkg/cloudprovider/aws/aws_test.go

@@ -108,6 +108,7 @@ type FakeAWSServices struct {
 	availabilityZone string
 	instances        []*ec2.Instance
 	instanceId       string
+	privateDnsName   string
 
 	ec2      *FakeEC2
 	elb      *FakeELB
@@ -124,8 +125,10 @@ func NewFakeAWSServices() *FakeAWSServices {
 	s.metadata = &FakeMetadata{aws: s}
 
 	s.instanceId = "i-self"
+	s.privateDnsName = "ip-172-20-0-100.ec2.internal"
 	var selfInstance ec2.Instance
 	selfInstance.InstanceID = &s.instanceId
+	selfInstance.PrivateDNSName = &s.privateDnsName
 	s.instances = []*ec2.Instance{&selfInstance}
 
 	var tag ec2.Tag
@@ -310,6 +313,8 @@ func (self *FakeMetadata) GetMetaData(key string) ([]byte, error) {
 		return []byte(self.aws.availabilityZone), nil
 	} else if key == "instance-id" {
 		return []byte(self.aws.instanceId), nil
+	} else if key == "local-hostname" {
+		return []byte(self.aws.privateDnsName), nil
 	} else {
 		return nil, nil
 	}
@@ -448,6 +453,7 @@ func TestList(t *testing.T) {
 	}
 	instance0.Tags = []*ec2.Tag{&tag0}
 	instance0.InstanceID = aws.String("instance0")
+	instance0.PrivateDNSName = aws.String("instance0.ec2.internal")
 	state0 := ec2.InstanceState{
 		Name: aws.String("running"),
 	}
@@ -460,6 +466,7 @@ func TestList(t *testing.T) {
 	}
 	instance1.Tags = []*ec2.Tag{&tag1}
 	instance1.InstanceID = aws.String("instance1")
+	instance1.PrivateDNSName = aws.String("instance1.ec2.internal")
 	state1 := ec2.InstanceState{
 		Name: aws.String("running"),
 	}
@@ -472,6 +479,7 @@ func TestList(t *testing.T) {
 	}
 	instance2.Tags = []*ec2.Tag{&tag2}
 	instance2.InstanceID = aws.String("instance2")
+	instance2.PrivateDNSName = aws.String("instance2.ec2.internal")
 	state2 := ec2.InstanceState{
 		Name: aws.String("running"),
 	}
@@ -484,6 +492,7 @@ func TestList(t *testing.T) {
 	}
 	instance3.Tags = []*ec2.Tag{&tag3}
 	instance3.InstanceID = aws.String("instance3")
+	instance3.PrivateDNSName = aws.String("instance3.ec2.internal")
 	state3 := ec2.InstanceState{
 		Name: aws.String("running"),
 	}
@@ -497,8 +506,8 @@ func TestList(t *testing.T) {
 		expect []string
 	}{
 		{"blahonga", []string{}},
-		{"quux", []string{"instance3"}},
+		{"quux", []string{"instance3.ec2.internal"}},
-		{"a", []string{"instance1", "instance2"}},
+		{"a", []string{"instance1.ec2.internal", "instance2.ec2.internal"}},
 	}
 
 	for _, item := range table {
@@ -529,6 +538,7 @@ func TestNodeAddresses(t *testing.T) {
 
 	//0
 	instance0.InstanceID = aws.String("instance-same")
+	instance0.PrivateDNSName = aws.String("instance-same.ec2.internal")
 	instance0.PrivateIPAddress = aws.String("192.168.0.1")
 	instance0.PublicIPAddress = aws.String("1.2.3.4")
 	instance0.InstanceType = aws.String("c3.large")
@@ -539,6 +549,7 @@ func TestNodeAddresses(t *testing.T) {
 
 	//1
 	instance1.InstanceID = aws.String("instance-same")
+	instance1.PrivateDNSName = aws.String("instance-same.ec2.internal")
 	instance1.PrivateIPAddress = aws.String("192.168.0.2")
 	instance1.InstanceType = aws.String("c3.large")
 	state1 := ec2.InstanceState{
@@ -549,19 +560,19 @@ func TestNodeAddresses(t *testing.T) {
 	instances := []*ec2.Instance{&instance0, &instance1}
 
 	aws1 := mockInstancesResp([]*ec2.Instance{})
-	_, err1 := aws1.NodeAddresses("instance-mismatch")
+	_, err1 := aws1.NodeAddresses("instance-mismatch.ec2.internal")
 	if err1 == nil {
 		t.Errorf("Should error when no instance found")
 	}
 
 	aws2 := mockInstancesResp(instances)
-	_, err2 := aws2.NodeAddresses("instance-same")
+	_, err2 := aws2.NodeAddresses("instance-same.ec2.internal")
 	if err2 == nil {
 		t.Errorf("Should error when multiple instances found")
 	}
 
 	aws3 := mockInstancesResp(instances[0:1])
-	addrs3, err3 := aws3.NodeAddresses("instance-same")
+	addrs3, err3 := aws3.NodeAddresses("instance-same.ec2.internal")
 	if err3 != nil {
 		t.Errorf("Should not error when instance found")
 	}
@@ -598,6 +609,7 @@ func TestGetResources(t *testing.T) {
 
 	//0
 	instance0.InstanceID = aws.String("m3.medium")
+	instance0.PrivateDNSName = aws.String("m3-medium.ec2.internal")
 	instance0.InstanceType = aws.String("m3.medium")
 	state0 := ec2.InstanceState{
 		Name: aws.String("running"),
@@ -606,6 +618,7 @@ func TestGetResources(t *testing.T) {
 
 	//1
 	instance1.InstanceID = aws.String("r3.8xlarge")
+	instance1.PrivateDNSName = aws.String("r3-8xlarge.ec2.internal")
 	instance1.InstanceType = aws.String("r3.8xlarge")
 	state1 := ec2.InstanceState{
 		Name: aws.String("running"),
@@ -614,6 +627,7 @@ func TestGetResources(t *testing.T) {
 
 	//2
 	instance2.InstanceID = aws.String("unknown.type")
+	instance2.PrivateDNSName = aws.String("unknown-type.ec2.internal")
 	instance2.InstanceType = aws.String("unknown.type")
 	state2 := ec2.InstanceState{
 		Name: aws.String("running"),
@@ -624,7 +638,7 @@ func TestGetResources(t *testing.T) {
 
 	aws1 := mockInstancesResp(instances)
 
-	res1, err1 := aws1.GetNodeResources("m3.medium")
+	res1, err1 := aws1.GetNodeResources("m3-medium.ec2.internal")
 	if err1 != nil {
 		t.Errorf("Should not error when instance type found: %v", err1)
 	}
@@ -638,7 +652,7 @@ func TestGetResources(t *testing.T) {
 		t.Errorf("Expected %v, got %v", e1, res1)
 	}
 
-	res2, err2 := aws1.GetNodeResources("r3.8xlarge")
+	res2, err2 := aws1.GetNodeResources("r3-8xlarge.ec2.internal")
 	if err2 != nil {
 		t.Errorf("Should not error when instance type found: %v", err2)
 	}
@@ -652,7 +666,7 @@ func TestGetResources(t *testing.T) {
 		t.Errorf("Expected %v, got %v", e2, res2)
 	}
 
-	res3, err3 := aws1.GetNodeResources("unknown.type")
+	res3, err3 := aws1.GetNodeResources("unknown-type.ec2.internal")
 	if err3 != nil {
 		t.Errorf("Should not error when unknown instance type")
 	}