Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix behaviour of aws-load-balancer-security-groups annotation #88689

Merged
merged 1 commit into from
Mar 10, 2020
Merged

fix behaviour of aws-load-balancer-security-groups annotation #88689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 10 additions & 15 deletions staging/src/k8s.io/legacy-cloud-providers/aws/aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2987,11 +2987,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 @@ -3483,36 +3478,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 @@ -3821,15 +3816,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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1641,11 +1641,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 @@ -1674,10 +1675,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