Skip to content

Commit

Permalink
Merge pull request #88690 from Elias481/bp-83446-1.16
Browse files Browse the repository at this point in the history
fix behaviour of aws-load-balancer-security-groups annotation
  • Loading branch information
k8s-ci-robot authored Mar 10, 2020
2 parents dafe20b + da84241 commit 33f0187
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 17 deletions.
25 changes: 10 additions & 15 deletions staging/src/k8s.io/legacy-cloud-providers/aws/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -2946,11 +2946,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)
Expand Down Expand Up @@ -3442,36 +3437,36 @@ 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
sgDescription := fmt.Sprintf("Security group for Kubernetes ELB %s (%v)", loadBalancerName, serviceName)
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
Expand Down Expand Up @@ -3780,15 +3775,15 @@ 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
}
if len(securityGroupIDs) == 0 {
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)})
Expand Down
6 changes: 4 additions & 2 deletions staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1630,11 +1630,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)
})
}
}
Expand Down Expand Up @@ -1663,10 +1664,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)
})
}
}
Expand Down

0 comments on commit 33f0187

Please sign in to comment.