From 5ae7c13ad3195bc8d92e0ed2b544e86bcd06f519 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Thu, 2 Jul 2015 13:37:00 -0400 Subject: [PATCH] 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 | 88 +++++++++++++++++++++++------ pkg/cloudprovider/aws/aws_routes.go | 9 ++- pkg/cloudprovider/aws/aws_test.go | 30 +++++++--- 3 files changed, 100 insertions(+), 27 deletions(-) diff --git a/pkg/cloudprovider/aws/aws.go b/pkg/cloudprovider/aws/aws.go index 30d302f5050..623ce0bf94e 100644 --- a/pkg/cloudprovider/aws/aws.go +++ b/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 { - self := &awsInstance{ec2: ec2, awsID: awsID} +func newAWSInstance(ec2 EC2, awsID, nodeName string) *awsInstance { + 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 == "" -func (aws *AWSCloud) getAwsInstance(instanceName string) (*awsInstance, error) { +// Gets the awsInstance with node-name nodeName, or the 'self' instance if nodeName == "" +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 { diff --git a/pkg/cloudprovider/aws/aws_routes.go b/pkg/cloudprovider/aws/aws_routes.go index c28cd8a471f..3666fcab94a 100644 --- a/pkg/cloudprovider/aws/aws_routes.go +++ b/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) diff --git a/pkg/cloudprovider/aws/aws_test.go b/pkg/cloudprovider/aws/aws_test.go index 5fd13daa4e7..c765ac1c69f 100644 --- a/pkg/cloudprovider/aws/aws_test.go +++ b/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"}}, - {"a", []string{"instance1", "instance2"}}, + {"quux", []string{"instance3.ec2.internal"}}, + {"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") }