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.
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
Fixes unnecessary creation of default SG and trying to delete non-provisioned SG by k8s system when annotation [service.beta.kubernetes.io/aws-load-balancer-security-groups] is present #84265
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3497,6 +3497,18 @@ func getPortSets(annotation string) (ports *portSets) { | |
return | ||
} | ||
|
||
// This function is useful in extracting the security group list from annotation | ||
func getSGListFromAnnotation(annotatedSG string) []string { | ||
sgList := []string{} | ||
for _, extraSG := range strings.Split(annotatedSG, ",") { | ||
extraSG = strings.TrimSpace(extraSG) | ||
if len(extraSG) > 0 { | ||
sgList = append(sgList, extraSG) | ||
} | ||
} | ||
return sgList | ||
} | ||
|
||
// buildELBSecurityGroupList returns list of SecurityGroups which should be | ||
// 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. | ||
|
@@ -3507,39 +3519,30 @@ func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, load | |
var err error | ||
var securityGroupID string | ||
|
||
if c.cfg.Global.ElbSecurityGroup != "" { | ||
securityGroupID = 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 | ||
} | ||
} | ||
sgList := getSGListFromAnnotation(annotations[ServiceAnnotationLoadBalancerSecurityGroups]) | ||
|
||
sgList := []string{} | ||
|
||
for _, extraSG := range strings.Split(annotations[ServiceAnnotationLoadBalancerSecurityGroups], ",") { | ||
extraSG = strings.TrimSpace(extraSG) | ||
if len(extraSG) > 0 { | ||
sgList = append(sgList, extraSG) | ||
} | ||
} | ||
// 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 | ||
} 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 | ||
} | ||
} | ||
sgList = append(sgList, securityGroupID) | ||
} | ||
|
||
for _, extraSG := range strings.Split(annotations[ServiceAnnotationLoadBalancerExtraSecurityGroups], ",") { | ||
extraSG = strings.TrimSpace(extraSG) | ||
if len(extraSG) > 0 { | ||
sgList = append(sgList, extraSG) | ||
} | ||
} | ||
extraSGList := getSGListFromAnnotation(annotations[ServiceAnnotationLoadBalancerExtraSecurityGroups]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can be replaced by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah , I was looking at the Golang doc I couldn't figure out if we have something to just append without own loop. |
||
sgList = append(sgList, extraSGList...) | ||
|
||
return sgList, nil | ||
} | ||
|
@@ -4347,6 +4350,14 @@ func (c *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName strin | |
|
||
// Collect the security groups to delete | ||
securityGroupIDs := map[string]struct{}{} | ||
annotatedSgSet := map[string]bool{} | ||
annotatedSgsList := getSGListFromAnnotation(service.Annotations[ServiceAnnotationLoadBalancerSecurityGroups]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about ServiceAnnotationLoadBalancerExtraSecurityGroups :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes Sure can be and should be added. |
||
annotatedExtraSgsList := getSGListFromAnnotation(service.Annotations[ServiceAnnotationLoadBalancerExtraSecurityGroups]) | ||
annotatedSgsList = append(annotatedSgsList, annotatedExtraSgsList...) | ||
|
||
for _, sg := range annotatedSgsList { | ||
annotatedSgSet[sg] = true | ||
} | ||
|
||
for _, sg := range response { | ||
sgID := aws.StringValue(sg.GroupId) | ||
|
@@ -4365,6 +4376,12 @@ func (c *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName strin | |
continue | ||
} | ||
|
||
// This is an extra protection of deletion of non provisioned Security Group which is annotated with `service.beta.kubernetes.io/aws-load-balancer-security-groups`. | ||
if _, ok := annotatedSgSet[sgID]; ok { | ||
klog.Warningf("Ignoring security group with annotation `service.beta.kubernetes.io/aws-load-balancer-security-groups` or service.beta.kubernetes.io/aws-load-balancer-extra-security-groups in %s", service.Name) | ||
continue | ||
} | ||
|
||
securityGroupIDs[sgID] = struct{}{} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, while whole part can be reorganized to like to make the logic more clear. But current one is ok too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point , Thanks for the tip.