From 20a682b643fdac9a7d0b4ccc4fd8a44395b5b274 Mon Sep 17 00:00:00 2001 From: Andrew Garrett Date: Wed, 24 Jan 2018 20:16:45 +0000 Subject: [PATCH] Tag Security Group created for AWS ELB with same additional tags as ELB Reuse the service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags annotation for these tags. We think that this is a simpler and less intrusive approach than adding a new annotation, and most users will want the same set of tags applied. Signed-off-by: Brett Kochendorfer --- pkg/cloudprovider/providers/aws/aws.go | 12 ++++++------ pkg/cloudprovider/providers/aws/aws_test.go | 19 ++++++++++--------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index f919cd899b8..5e526a19627 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -2815,7 +2815,7 @@ func (c *Cloud) removeSecurityGroupIngress(securityGroupID string, removePermiss // 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 (c *Cloud) ensureSecurityGroup(name string, description string) (string, error) { +func (c *Cloud) ensureSecurityGroup(name string, description string, annotations map[string]string) (string, error) { groupID := "" attempt := 0 for { @@ -2881,7 +2881,7 @@ func (c *Cloud) ensureSecurityGroup(name string, description string) (string, er return "", fmt.Errorf("created security group, but id was not returned: %s", name) } - err := c.tagging.createTags(c.ec2, groupID, ResourceLifecycleOwned, nil) + err := c.tagging.createTags(c.ec2, groupID, ResourceLifecycleOwned, getLoadBalancerAdditionalTags(annotations)) if err != nil { // If we retry, ensureClusterTags will recover from this - it // will add the missing tags. We could delete the security @@ -3097,7 +3097,7 @@ func getPortSets(annotation string) (ports *portSets) { // attached to ELB created by a service. List always consist of at least // 1 member which is an SG created for this service or a SG from the Global config. Extra groups can be // specified via annotation -func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, loadBalancerName, annotation string) ([]string, error) { +func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, loadBalancerName string, annotations map[string]string) ([]string, error) { var err error var securityGroupID string @@ -3107,7 +3107,7 @@ func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, load // Create a security group for the load balancer sgName := "k8s-elb-" + loadBalancerName sgDescription := fmt.Sprintf("Security group for Kubernetes ELB %s (%v)", loadBalancerName, serviceName) - securityGroupID, err = c.ensureSecurityGroup(sgName, sgDescription) + securityGroupID, err = c.ensureSecurityGroup(sgName, sgDescription, annotations) if err != nil { glog.Errorf("Error creating load balancer security group: %q", err) return nil, err @@ -3115,7 +3115,7 @@ func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, load } sgList := []string{securityGroupID} - for _, extraSG := range strings.Split(annotation, ",") { + for _, extraSG := range strings.Split(annotations[ServiceAnnotationLoadBalancerExtraSecurityGroups], ",") { extraSG = strings.TrimSpace(extraSG) if len(extraSG) > 0 { sgList = append(sgList, extraSG) @@ -3413,7 +3413,7 @@ func (c *Cloud) EnsureLoadBalancer(clusterName string, apiService *v1.Service, n loadBalancerName := cloudprovider.GetLoadBalancerName(apiService) serviceName := types.NamespacedName{Namespace: apiService.Namespace, Name: apiService.Name} - securityGroupIDs, err := c.buildELBSecurityGroupList(serviceName, loadBalancerName, annotations[ServiceAnnotationLoadBalancerExtraSecurityGroups]) + securityGroupIDs, err := c.buildELBSecurityGroupList(serviceName, loadBalancerName, annotations) if err != nil { return nil, err } diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index ded45896a65..abbb05c22b3 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -1124,19 +1124,20 @@ func TestLBExtraSecurityGroupsAnnotation(t *testing.T) { awsServices := newMockedFakeAWSServices(TestClusterId) c, _ := newAWSCloud(strings.NewReader("[global]"), awsServices) - sg1 := "sg-000001" - sg2 := "sg-000002" + sg1 := map[string]string{ServiceAnnotationLoadBalancerExtraSecurityGroups: "sg-000001"} + sg2 := map[string]string{ServiceAnnotationLoadBalancerExtraSecurityGroups: "sg-000002"} + sg3 := map[string]string{ServiceAnnotationLoadBalancerExtraSecurityGroups: "sg-000001, sg-000002"} tests := []struct { name string - extraSGsAnnotation string - expectedSGs []string + annotations map[string]string + expectedSGs []string }{ - {"No extra SG annotation", "", []string{}}, - {"Empty extra SGs specified", ", ,,", []string{}}, - {"SG specified", sg1, []string{sg1}}, - {"Multiple SGs specified", fmt.Sprintf("%s, %s", sg1, sg2), []string{sg1, sg2}}, + {"No extra SG annotation", map[string]string{}, []string{}}, + {"Empty extra SGs specified", map[string]string{ServiceAnnotationLoadBalancerExtraSecurityGroups: ", ,,"}, []string{}}, + {"SG specified", sg1, []string{sg1[ServiceAnnotationLoadBalancerExtraSecurityGroups]}}, + {"Multiple SGs specified", sg3, []string{sg1[ServiceAnnotationLoadBalancerExtraSecurityGroups], sg2[ServiceAnnotationLoadBalancerExtraSecurityGroups]}}, } awsServices.ec2.(*MockedFakeEC2).expectDescribeSecurityGroups(TestClusterId, "k8s-elb-aid", "cluster.test") @@ -1145,7 +1146,7 @@ func TestLBExtraSecurityGroupsAnnotation(t *testing.T) { t.Run(test.name, func(t *testing.T) { serviceName := types.NamespacedName{Namespace: "default", Name: "myservice"} - sgList, err := c.buildELBSecurityGroupList(serviceName, "aid", test.extraSGsAnnotation) + sgList, err := c.buildELBSecurityGroupList(serviceName, "aid", test.annotations) assert.NoError(t, err, "buildELBSecurityGroupList failed") extraSGs := sgList[1:] assert.True(t, sets.NewString(test.expectedSGs...).Equal(sets.NewString(extraSGs...)),