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

staging/src/k8s.io/legacy-cloud-providers/aws/aws.go

@@ -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 != "" {
+	sgList := getSGListFromAnnotation(annotations[ServiceAnnotationLoadBalancerSecurityGroups])
-		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 := []string{}
+	// 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
-	for _, extraSG := range strings.Split(annotations[ServiceAnnotationLoadBalancerSecurityGroups], ",") {
-		extraSG = strings.TrimSpace(extraSG)
-		if len(extraSG) > 0 {
-			sgList = append(sgList, extraSG)
-		}
-	}
 
 	// 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], ",") {
+	extraSGList := getSGListFromAnnotation(annotations[ServiceAnnotationLoadBalancerExtraSecurityGroups])
-		extraSG = strings.TrimSpace(extraSG)
+	sgList = append(sgList, extraSGList...)
-		if len(extraSG) > 0 {
-			sgList = append(sgList, extraSG)
-		}
-	}
 
 	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])
+		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{}{}
 		}