From ed6540d13f5d1f6f7697e2c9559e6eb449c7314a Mon Sep 17 00:00:00 2001 From: Elias Ohm Date: Thu, 3 Oct 2019 14:29:44 +0200 Subject: [PATCH] fix behaviour of aws-load-balancer-security-groups annotation --- .../k8s.io/legacy-cloud-providers/aws/aws.go | 25 ++++++++----------- .../legacy-cloud-providers/aws/aws_test.go | 6 +++-- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go b/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go index 5a6b58a33a05d..b6f780a2f05c0 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go +++ b/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go @@ -2918,11 +2918,6 @@ func isEqualUserGroupPair(l, r *ec2.UserIdGroupPair, compareGroupUserIDs bool) b // Returns true if and only if changes were made // The security group must already exist func (c *Cloud) setSecurityGroupIngress(securityGroupID string, permissions IPPermissionSet) (bool, error) { - // We do not want to make changes to the Global defined SG - if securityGroupID == c.cfg.Global.ElbSecurityGroup { - return false, nil - } - group, err := c.findSecurityGroup(securityGroupID) if err != nil { klog.Warningf("Error retrieving security group %q", err) @@ -3407,19 +3402,18 @@ func getSGListFromAnnotation(annotatedSG string) []string { // Extra groups can be specified via annotation, as can extra tags for any // new groups. The annotation "ServiceAnnotationLoadBalancerSecurityGroups" allows for // setting the security groups specified. -func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, loadBalancerName string, annotations map[string]string) ([]string, error) { +func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, loadBalancerName string, annotations map[string]string) ([]string, bool, error) { var err error var securityGroupID string + // We do not want to make changes to a Global defined SG + var setupSg = false sgList := getSGListFromAnnotation(annotations[ServiceAnnotationLoadBalancerSecurityGroups]) - // The below code changes makes sure that when we have Security Groups specified with the ServiceAnnotationLoadBalancerSecurityGroups - // annotation we don't create a new default Security Groups - // If no Security Groups have been specified with the ServiceAnnotationLoadBalancerSecurityGroups annotation, we add the default one. if len(sgList) == 0 { if c.cfg.Global.ElbSecurityGroup != "" { - securityGroupID = c.cfg.Global.ElbSecurityGroup + sgList = append(sgList, c.cfg.Global.ElbSecurityGroup) } else { // Create a security group for the load balancer sgName := "k8s-elb-" + loadBalancerName @@ -3427,16 +3421,17 @@ func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, load securityGroupID, err = c.ensureSecurityGroup(sgName, sgDescription, getLoadBalancerAdditionalTags(annotations)) if err != nil { klog.Errorf("Error creating load balancer security group: %q", err) - return nil, err + return nil, setupSg, err } + sgList = append(sgList, securityGroupID) + setupSg = true } - sgList = append(sgList, securityGroupID) } extraSGList := getSGListFromAnnotation(annotations[ServiceAnnotationLoadBalancerExtraSecurityGroups]) sgList = append(sgList, extraSGList...) - return sgList, nil + return sgList, setupSg, nil } // buildListener creates a new listener from the given port, adding an SSL certificate @@ -3745,7 +3740,7 @@ func (c *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, apiS loadBalancerName := c.GetLoadBalancerName(ctx, clusterName, apiService) serviceName := types.NamespacedName{Namespace: apiService.Namespace, Name: apiService.Name} - securityGroupIDs, err := c.buildELBSecurityGroupList(serviceName, loadBalancerName, annotations) + securityGroupIDs, setupSg, err := c.buildELBSecurityGroupList(serviceName, loadBalancerName, annotations) if err != nil { return nil, err } @@ -3753,7 +3748,7 @@ func (c *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, apiS return nil, fmt.Errorf("[BUG] ELB can't have empty list of Security Groups to be assigned, this is a Kubernetes bug, please report") } - { + if setupSg { ec2SourceRanges := []*ec2.IpRange{} for _, sourceRange := range sourceRanges.StringSlice() { ec2SourceRanges = append(ec2SourceRanges, &ec2.IpRange{CidrIp: aws.String(sourceRange)}) diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go b/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go index 8b788dd8bcba5..8aaa31f3ccb6a 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go @@ -1624,11 +1624,12 @@ 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.annotations) + sgList, setupSg, 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...)), "Security Groups expected=%q , returned=%q", test.expectedSGs, extraSGs) + assert.True(t, setupSg, "Security Groups Setup Permissions Flag expected=%t , returned=%t", true, setupSg) }) } } @@ -1657,10 +1658,11 @@ func TestLBSecurityGroupsAnnotation(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.annotations) + sgList, setupSg, err := c.buildELBSecurityGroupList(serviceName, "aid", test.annotations) assert.NoError(t, err, "buildELBSecurityGroupList failed") assert.True(t, sets.NewString(test.expectedSGs...).Equal(sets.NewString(sgList...)), "Security Groups expected=%q , returned=%q", test.expectedSGs, sgList) + assert.False(t, setupSg, "Security Groups Setup Permissions Flag expected=%t , returned=%t", false, setupSg) }) } }