[occm] Delete unused SG rules with manage-security-groups (#2287)

* Apply LB SG to nodes only once

Code applying SG to nodes was incorrectly put in a loop over Services
ports. This commit fixes that.

* Delete unused SG rules with manage-security-groups

This commit makes sure unnecessary SG rules are removed when we're
reconciling a Service.

Co-Authored-By: Sven Haardiek <[email protected]>

---------

Co-authored-by: Sven Haardiek <[email protected]>

Author: dulek

pkg/openstack/loadbalancer.go

@@ -422,6 +422,15 @@ func getSecurityGroupName(service *corev1.Service) string {
 	return securityGroupName
 }
 
+func getSecurityGroupRules(client *gophercloud.ServiceClient, opts rules.ListOpts) ([]rules.SecGroupRule, error) {
+	mc := metrics.NewMetricContext("security_group_rule", "list")
+	page, err := rules.List(client, opts).AllPages()
+	if mc.ObserveRequest(err) != nil {
+		return nil, err
+	}
+	return rules.ExtractRules(page)
+}
+
 func getListenerProtocol(protocol corev1.Protocol, svcConf *serviceConfig) listeners.Protocol {
 	// Make neutron-lbaas code work
 	if svcConf != nil {
@@ -2060,30 +2069,16 @@ func (lbaas *LbaasV2) listSubnetsForNetwork(networkID string, tweak ...TweakSubN
 }
 
 // group, if it not present.
-func (lbaas *LbaasV2) ensureSecurityRule(
-	direction rules.RuleDirection,
-	protocol rules.RuleProtocol,
-	etherType rules.RuleEtherType,
-	remoteIPPrefix, secGroupID string,
-	portRangeMin, portRangeMax int,
-) error {
-	sgRuleCreateOpts := rules.CreateOpts{
-		Direction:      direction,
-		Protocol:       protocol,
-		PortRangeMax:   portRangeMin,
-		PortRangeMin:   portRangeMax,
-		RemoteIPPrefix: remoteIPPrefix,
-		SecGroupID:     secGroupID,
-		EtherType:      etherType,
-	}
-
+func (lbaas *LbaasV2) ensureSecurityRule(sgRuleCreateOpts rules.CreateOpts) error {
 	mc := metrics.NewMetricContext("security_group_rule", "create")
 	_, err := rules.Create(lbaas.network, sgRuleCreateOpts).Extract()
 	if err != nil && cpoerrors.IsConflictError(err) {
 		// Conflict means the SG rule already exists, so ignoring that error.
+		klog.Warningf("Security group rule already found when trying to create it. This indicates concurrent "+
+			"updates to the SG %s and is unexpected", sgRuleCreateOpts.SecGroupID)
 		return mc.ObserveRequest(nil)
 	} else if mc.ObserveRequest(err) != nil {
-		return fmt.Errorf("failed to create rule for security group %s: %v", secGroupID, err)
+		return fmt.Errorf("failed to create rule for security group %s: %v", sgRuleCreateOpts.SecGroupID, err)
 	}
 	return nil
 }
@@ -2174,6 +2169,50 @@ func (lbaas *LbaasV2) UpdateLoadBalancer(ctx context.Context, clusterName string
 	return mc.ObserveReconcile(err)
 }
 
+func compareSecurityGroupRuleAndCreateOpts(rule rules.SecGroupRule, opts rules.CreateOpts) bool {
+	return rule.Direction == string(opts.Direction) &&
+		rule.Protocol == string(opts.Protocol) &&
+		rule.EtherType == string(opts.EtherType) &&
+		rule.RemoteIPPrefix == opts.RemoteIPPrefix &&
+		rule.PortRangeMin == opts.PortRangeMin &&
+		rule.PortRangeMax == opts.PortRangeMax
+}
+
+func getRulesToCreateAndDelete(wantedRules []rules.CreateOpts, existingRules []rules.SecGroupRule) ([]rules.CreateOpts, []rules.SecGroupRule) {
+	toCreate := make([]rules.CreateOpts, 0, len(wantedRules))     // Max is all rules need creation
+	toDelete := make([]rules.SecGroupRule, 0, len(existingRules)) // Max will be all the existing rules to be deleted
+	// Surely this can be done in a more efficient way. Is it worth optimizing if most of
+	// the time we'll deal with just 1 or 2 elements in each array? I doubt it.
+	for _, existingRule := range existingRules {
+		found := false
+		for _, wantedRule := range wantedRules {
+			if compareSecurityGroupRuleAndCreateOpts(existingRule, wantedRule) {
+				found = true
+				break
+			}
+		}
+		if !found {
+			// in existingRules but not in wantedRules, delete
+			toDelete = append(toDelete, existingRule)
+		}
+	}
+	for _, wantedRule := range wantedRules {
+		found := false
+		for _, existingRule := range existingRules {
+			if compareSecurityGroupRuleAndCreateOpts(existingRule, wantedRule) {
+				found = true
+				break
+			}
+		}
+		if !found {
+			// in wantedRules but not in exisitngRules, create
+			toCreate = append(toCreate, wantedRule)
+		}
+	}
+
+	return toCreate, toDelete
+}
+
 // ensureAndUpdateOctaviaSecurityGroup handles the creation and update of the security group and the securiry rules for the octavia load balancer
 func (lbaas *LbaasV2) ensureAndUpdateOctaviaSecurityGroup(clusterName string, apiService *corev1.Service, nodes []*corev1.Node, memberSubnetID string) error {
 	// get service ports
@@ -2220,47 +2259,75 @@ func (lbaas *LbaasV2) ensureAndUpdateOctaviaSecurityGroup(clusterName string, ap
 		etherType = rules.EtherType6
 	}
 
+	existingRules, err := getSecurityGroupRules(lbaas.network, rules.ListOpts{SecGroupID: lbSecGroupID})
+	if err != nil {
+		return fmt.Errorf(
+			"failed to find security group rules in %s: %v", lbSecGroupID, err)
+	}
+
+	// List of the security group rules wanted in the SG.
+	// Number of Ports plus the potential HealthCheckNodePort.
+	wantedRules := make([]rules.CreateOpts, 0, len(ports)+1)
+
 	if apiService.Spec.HealthCheckNodePort != 0 {
-		err = lbaas.ensureSecurityRule(
-			rules.DirIngress,
-			rules.ProtocolTCP,
-			etherType,
-			subnet.CIDR,
-			lbSecGroupID,
-			int(apiService.Spec.HealthCheckNodePort),
-			int(apiService.Spec.HealthCheckNodePort),
+		wantedRules = append(wantedRules,
+			rules.CreateOpts{
+				Direction:      rules.DirIngress,
+				Protocol:       rules.ProtocolTCP,
+				EtherType:      etherType,
+				RemoteIPPrefix: subnet.CIDR,
+				SecGroupID:     lbSecGroupID,
+				PortRangeMin:   int(apiService.Spec.HealthCheckNodePort),
+				PortRangeMax:   int(apiService.Spec.HealthCheckNodePort),
+			},
 		)
-		if err != nil {
-			return fmt.Errorf(
-				"failed to apply security rule for health check node port, %w",
-				err)
-		}
 	}
 
-	// ensure rules for node security group
 	for _, port := range ports {
 		if port.NodePort == 0 { // It's 0 when AllocateLoadBalancerNodePorts=False
 			continue
 		}
-		err = lbaas.ensureSecurityRule(
-			rules.DirIngress,
-			rules.RuleProtocol(port.Protocol),
-			etherType,
-			subnet.CIDR,
-			lbSecGroupID,
-			int(port.NodePort),
-			int(port.NodePort),
+		wantedRules = append(wantedRules,
+			rules.CreateOpts{
+				Direction:      rules.DirIngress,
+				Protocol:       rules.RuleProtocol(port.Protocol),
+				EtherType:      etherType,
+				RemoteIPPrefix: subnet.CIDR,
+				SecGroupID:     lbSecGroupID,
+				PortRangeMin:   int(port.NodePort),
+				PortRangeMax:   int(port.NodePort),
+			},
 		)
+	}
+
+	toCreate, toDelete := getRulesToCreateAndDelete(wantedRules, existingRules)
+
+	// create new rules
+	for _, opts := range toCreate {
+		err := lbaas.ensureSecurityRule(opts)
 		if err != nil {
-			return fmt.Errorf(
-				"failed to apply security rule for port %d, %w",
-				port.NodePort, err)
+			return fmt.Errorf("failed to apply security rule (%v), %w", opts, err)
 		}
+	}
 
-		if err := applyNodeSecurityGroupIDForLB(lbaas.compute, lbaas.network, nodes, lbSecGroupID); err != nil {
-			return err
+	// delete unneeded rules
+	for _, existingRule := range toDelete {
+		klog.Infof("Deleting rule %s from security group %s (%s)", existingRule.ID, existingRule.SecGroupID, lbSecGroupName)
+		mc := metrics.NewMetricContext("security_group_rule", "delete")
+		err := rules.Delete(lbaas.network, existingRule.ID).ExtractErr()
+		if err != nil && cpoerrors.IsNotFound(err) {
+			// ignore 404
+			klog.Warningf("Security group rule %s found missing when trying to delete it. This indicates concurrent "+
+				"updates to the SG %s and is unexpected", existingRule.ID, existingRule.SecGroupID)
+			return mc.ObserveRequest(nil)
+		} else if mc.ObserveRequest(err) != nil {
+			return fmt.Errorf("failed to delete security group rule %s: %w", existingRule.ID, err)
 		}
 	}
+
+	if err := applyNodeSecurityGroupIDForLB(lbaas.compute, lbaas.network, nodes, lbSecGroupID); err != nil {
+		return err
+	}
 	return nil
 }