From 1cdfc9ad84884982fe92b3eee1cea7a9f288d797 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Thu, 25 Feb 2016 11:17:42 -0500 Subject: [PATCH] AWS: Find the correct security group by looking at tags Like everything else AWS, we differentiate between k8s-owned security groups and k8s-not-owned security groups using tags. When we are setting up the ingress rule for ELBs, pick the security group that is tagged over any others. We continue to tolerate a single security group being untagged, but having multiple security groups without tagging is now an error, as it leads to undefined behaviour. We also log at startup if the cluster tag is not defined. Fix #21986 --- pkg/cloudprovider/providers/aws/aws.go | 97 ++++++++++++++++++++------ 1 file changed, 75 insertions(+), 22 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index d4f9565785..37f20a7d0d 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -655,6 +655,10 @@ func newAWSCloud(config io.Reader, awsServices AWSServices) (*AWSCloud, error) { } } + if filterTags[TagNameKubernetesCluster] == "" { + glog.Errorf("Tag %q not found; Kuberentes may behave unexpectedly.", TagNameKubernetesCluster) + } + awsCloud.filterTags = filterTags if len(filterTags) > 0 { glog.Infof("AWS cloud filtering on tags: %v", filterTags) @@ -1496,6 +1500,7 @@ func (s *AWSCloud) findSecurityGroup(securityGroupId string) (*ec2.SecurityGroup describeSecurityGroupsRequest := &ec2.DescribeSecurityGroupsInput{ GroupIds: []*string{&securityGroupId}, } + // We don't apply our tag filters because we are retrieving by ID groups, err := s.ec2.DescribeSecurityGroups(describeSecurityGroupsRequest) if err != nil { @@ -1731,7 +1736,8 @@ func (s *AWSCloud) ensureClusterTags(resourceID string, tags []*ec2.Tag) error { return nil } -// Makes sure the security group exists +// Makes sure the security group exists. +// For multi-cluster isolation, name must be globally unique, for example derived from the service UUID. // Returns the security group id or error func (s *AWSCloud) ensureSecurityGroup(name string, description string, vpcID string) (string, error) { groupID := "" @@ -2046,30 +2052,64 @@ func toStatus(lb *elb.LoadBalancerDescription) *api.LoadBalancerStatus { } // Returns the first security group for an instance, or nil -// We only create instances with one security group, so we warn if there are multiple or none -func findSecurityGroupForInstance(instance *ec2.Instance) *string { - var securityGroupId *string - for _, securityGroup := range instance.SecurityGroups { - if securityGroup == nil || securityGroup.GroupId == nil { - // Not expected, but avoid panic - glog.Warning("Unexpected empty security group for instance: ", orEmpty(instance.InstanceId)) +// We only create instances with one security group, so we don't expect multiple security groups. +// However, if there are multiple security groups, we will choose the one tagged with our cluster filter. +// Otherwise we will return an error. +func findSecurityGroupForInstance(instance *ec2.Instance, taggedSecurityGroups map[string]*ec2.SecurityGroup) (*ec2.GroupIdentifier, error) { + instanceID := aws.StringValue(instance.InstanceId) + var best *ec2.GroupIdentifier + for _, group := range instance.SecurityGroups { + groupID := aws.StringValue(group.GroupId) + if groupID == "" { + glog.Warningf("Ignoring security group without id for instance %q: %v", instanceID, group) + continue + } + if best == nil { + best = group continue } - if securityGroupId != nil { - // We create instances with one SG - glog.Warningf("Multiple security groups found for instance (%s); will use first group (%s)", orEmpty(instance.InstanceId), *securityGroupId) - continue + _, bestIsTagged := taggedSecurityGroups[*best.GroupId] + _, groupIsTagged := taggedSecurityGroups[groupID] + + if bestIsTagged && !groupIsTagged { + // best is still best + } else if groupIsTagged && !bestIsTagged { + best = group } else { - securityGroupId = securityGroup.GroupId + // We create instances with one SG + // If users create multiple SGs, they must tag one of them as being k8s owned + return nil, fmt.Errorf("Multiple security groups found for instance (%s); ensure the k8s security group is tagged", instanceID) } } - if securityGroupId == nil { - glog.Warning("No security group found for instance ", orEmpty(instance.InstanceId)) + if best == nil { + glog.Warning("No security group found for instance ", instanceID) } - return securityGroupId + return best, nil +} + +// Return all the security groups that are tagged as being part of our cluster +func (s *AWSCloud) getTaggedSecurityGroups() (map[string]*ec2.SecurityGroup, error) { + request := &ec2.DescribeSecurityGroupsInput{} + filters := []*ec2.Filter{} + request.Filters = s.addFilters(filters) + groups, err := s.ec2.DescribeSecurityGroups(request) + if err != nil { + return nil, fmt.Errorf("error querying security groups: %v", err) + } + + m := make(map[string]*ec2.SecurityGroup) + for _, group := range groups { + id := aws.StringValue(group.GroupId) + if id == "" { + glog.Warningf("Ignoring group without id: %v", group) + continue + } + m[id] = group + } + return m, nil } // Open security group ingress rules on the instances so that the load balancer can talk to them @@ -2105,6 +2145,11 @@ func (s *AWSCloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalan return fmt.Errorf("error querying security groups: %v", err) } + taggedSecurityGroups, err := s.getTaggedSecurityGroups() + if err != nil { + return fmt.Errorf("error querying for tagged security groups: %v", err) + } + // Open the firewall from the load balancer to the instance // We don't actually have a trivial way to know in advance which security group the instance is in // (it is probably the minion security group, but we don't easily have that). @@ -2115,24 +2160,32 @@ func (s *AWSCloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalan // Scan instances for groups we want open for _, instance := range allInstances { - securityGroupId := findSecurityGroupForInstance(instance) - if isNilOrEmpty(securityGroupId) { + securityGroup, err := findSecurityGroupForInstance(instance, taggedSecurityGroups) + if err != nil { + return err + } + + if securityGroup == nil { glog.Warning("Ignoring instance without security group: ", orEmpty(instance.InstanceId)) continue } + id := aws.StringValue(securityGroup.GroupId) + if id == "" { + glog.Warningf("found security group without id: %v", securityGroup) + continue + } - instanceSecurityGroupIds[*securityGroupId] = true + instanceSecurityGroupIds[id] = true } // Compare to actual groups for _, actualGroup := range actualGroups { - if isNilOrEmpty(actualGroup.GroupId) { + actualGroupID := aws.StringValue(actualGroup.GroupId) + if actualGroupID == "" { glog.Warning("Ignoring group without ID: ", actualGroup) continue } - actualGroupID := *actualGroup.GroupId - adding, found := instanceSecurityGroupIds[actualGroupID] if found && adding { // We don't need to make a change; the permission is already in place