NSG bugfixes with respect to pushing metrics, nil checks,

follow-up improvements for the nsg rule management feature

Author: pranavsriram8

pkg/cloudprovider/providers/oci/instances_test.go

@@ -278,18 +278,6 @@ func (MockSecurityListManager) Delete(ctx context.Context, lbSubnets []*core.Sub
 
 type MockSecurityListManagerFactory func(mode string) MockSecurityListManager
 
-type MockNsgManager struct{}
-
-func (MockNsgManager) Add(ctx context.Context, lbService serviceComponents) error {
-	return nil
-}
-
-func (MockNsgManager) Delete(ctx context.Context, lbService serviceComponents) error {
-	return nil
-}
-
-type MockNsgManagerFactory func(mode string) MockNsgManager
-
 type MockOCIClient struct{}
 
 func (MockOCIClient) Compute() client.ComputeInterface {

pkg/cloudprovider/providers/oci/load_balancer.go

@@ -30,6 +30,7 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/sets"
 	k8sports "k8s.io/kubernetes/pkg/cluster/ports"
+	"k8s.io/utils/pointer"
 
 	providercfg "github.com/oracle/oci-cloud-controller-manager/pkg/cloudprovider/providers/oci/config"
 	"github.com/oracle/oci-cloud-controller-manager/pkg/metrics"
@@ -474,7 +475,7 @@ func (cp *CloudProvider) EnsureLoadBalancer(ctx context.Context, clusterName str
 	startTime := time.Now()
 	lbName := GetLoadBalancerName(service)
 	loadBalancerType := getLoadBalancerType(service)
-	logger := cp.logger.With("loadBalancerName", lbName, "serviceName", service.Name, "loadBalancerType", loadBalancerType)
+	logger := cp.logger.With("loadBalancerName", lbName, "serviceName", service.Name, "loadBalancerType", loadBalancerType, "serviceUid", service.UID)
 	if sa, useWI := service.Annotations[ServiceAnnotationServiceAccountName]; useWI { // When using Workload Identity
 		logger = logger.With("serviceAccount", sa, "nameSpace", service.Namespace)
 	}
@@ -522,7 +523,7 @@ func (cp *CloudProvider) EnsureLoadBalancer(ctx context.Context, clusterName str
 		metrics.SendMetricData(cp.metricPusher, getMetric(loadBalancerType, Update), time.Since(startTime).Seconds(), dimensionsMap)
 		return nil, err
 	}
-	exists := !client.IsNotFound(err)
+	lbExists := !client.IsNotFound(err)
 	lbOCID := ""
 	if lb != nil && lb.Id != nil {
 		lbOCID = *lb.Id
@@ -609,6 +610,20 @@ func (cp *CloudProvider) EnsureLoadBalancer(ctx context.Context, clusterName str
 		frontendNsgId := ""
 		backendNsgs := spec.ManagedNetworkSecurityGroup.backendNsgId
 
+		// Check if there are any NSGs which are created by CCM (and use that), but didn't get attached to LB because the LB creation failed.
+		if !lbExists {
+			frontendNsgId, _, err = cp.getFrontendNsgByName(ctx, logger, generateNsgName(service), cp.config.CompartmentID, cp.config.VCNID, fmt.Sprintf("%s", service.UID))
+			if err != nil {
+				return nil, err
+			}
+			logger.Infof("found managed NSG %s", frontendNsgId)
+			if frontendNsgId != "" {
+				spec, err = addFrontendNsgToSpec(spec, frontendNsgId)
+				if err != nil {
+					return nil, err
+				}
+			}
+		}
 		if lb != nil && lb.Id != nil && lb.NetworkSecurityGroupIds != nil {
 			nsgs := lb.NetworkSecurityGroupIds
 			for _, id := range nsgs {
@@ -620,32 +635,51 @@ func (cp *CloudProvider) EnsureLoadBalancer(ctx context.Context, clusterName str
 					metrics.SendMetricData(cp.metricPusher, getMetric(util.NSGType, Get), time.Since(startTime).Seconds(), dimensionsMap)
 				}
 				if frontendNsgId != "" {
-					if !contains(spec.NetworkSecurityGroupIds, frontendNsgId) {
-						spec.NetworkSecurityGroupIds = append(spec.NetworkSecurityGroupIds, frontendNsgId)
+					spec, err = addFrontendNsgToSpec(spec, frontendNsgId)
+					if err != nil {
+						return nil, err
 					}
 					logger.With("loadBalancerID", *lb.Id).Infof("using existing frontendNsg %s", frontendNsgId)
 					break
 				}
 			}
+
+			if frontendNsgId == "" {
+				// Check if there are any CCM created NSGs which might be manually removed by customer causing a dirty LB
+				logger.Info("Check if managed NSGs present in VCN")
+				frontendNsgId, _, err = cp.getFrontendNsgByName(ctx, logger, generateNsgName(service), cp.config.CompartmentID, cp.config.VCNID, fmt.Sprintf("%s", service.UID))
+				if err != nil {
+					return nil, err
+				}
+				logger.Infof("found managed NSG %s", frontendNsgId)
+				if frontendNsgId != "" {
+					spec, err = addFrontendNsgToSpec(spec, frontendNsgId)
+					if err != nil {
+						return nil, err
+					}
+				}
+			}
 		}
-		logger = logger.With("serviceId", service.UID)
+
 		// Create the NSG and add it to the LbSpec
 		if frontendNsgId == "" {
 			if len(spec.NetworkSecurityGroupIds) >= MaxNsgPerVnic {
-				return nil, fmt.Errorf("invalid number of Network Security Groups (Max: 5) including CCM managed nsg")
+				return nil, fmt.Errorf("invalid number of Network Security Groups (Max: 5) including managed nsg")
 			}
 			resp, err := cp.client.Networking().CreateNetworkSecurityGroup(ctx, cp.config.CompartmentID, cp.config.VCNID, generateNsgName(service), fmt.Sprintf("%s", service.UID))
 			if err != nil {
 				logger.With(zap.Error(err)).Error("Failed to create nsg")
 				errorType = util.GetError(err)
 				nsgMetricDimension = util.GetMetricDimensionForComponent(errorType, util.NSGType)
 				dimensionsMap[metrics.ComponentDimension] = nsgMetricDimension
-				dimensionsMap[metrics.ResourceOCIDDimension] = *resp.Id
 				metrics.SendMetricData(cp.metricPusher, getMetric(util.NSGType, Create), time.Since(startTime).Seconds(), dimensionsMap)
 				return nil, err
 			}
 			frontendNsgId = *resp.Id
-			spec.NetworkSecurityGroupIds = append(spec.NetworkSecurityGroupIds, frontendNsgId)
+			spec, err = addFrontendNsgToSpec(spec, frontendNsgId)
+			if err != nil {
+				return nil, err
+			}
 			logger.With("frontendNsgId", *resp.Id).
 				Info("Successfully created nsg")
 			nsgMetricDimension = util.GetMetricDimensionForComponent(util.Success, util.NSGType)
@@ -662,24 +696,27 @@ func (cp *CloudProvider) EnsureLoadBalancer(ctx context.Context, clusterName str
 					errorType = util.GetError(err)
 					nsgMetricDimension = util.GetMetricDimensionForComponent(errorType, util.NSGType)
 					dimensionsMap[metrics.ComponentDimension] = nsgMetricDimension
-					dimensionsMap[metrics.ResourceOCIDDimension] = *resp.Id
 					metrics.SendMetricData(cp.metricPusher, getMetric(util.NSGType, Get), time.Since(startTime).Seconds(), dimensionsMap)
 					return nil, err
 				}
 				freeformTags := resp.FreeformTags
 				if _, ok := freeformTags["ManagedBy"]; !ok {
 					if etag != nil {
-						freeformTags["ManagedBy"] = "OKE-CCM"
+						freeformTags["ManagedBy"] = "CCM"
 						response, err := cp.client.Networking().UpdateNetworkSecurityGroup(ctx, nsg, *etag, freeformTags)
 						if err != nil {
-							logger.With(zap.Error(err)).Error("Failed to get nsg")
+							logger.With(zap.Error(err)).Errorf("Failed to update nsg %s", nsg)
 							errorType = util.GetError(err)
 							nsgMetricDimension = util.GetMetricDimensionForComponent(errorType, util.NSGType)
 							dimensionsMap[metrics.ComponentDimension] = nsgMetricDimension
-							dimensionsMap[metrics.ResourceOCIDDimension] = *response.Id
+							dimensionsMap[metrics.ResourceOCIDDimension] = nsg
 							metrics.SendMetricData(cp.metricPusher, getMetric(util.NSGType, Update), time.Since(startTime).Seconds(), dimensionsMap)
 							return nil, err
 						}
+						nsgMetricDimension = util.GetMetricDimensionForComponent(util.Success, util.NSGType)
+						dimensionsMap[metrics.ComponentDimension] = nsgMetricDimension
+						dimensionsMap[metrics.ResourceOCIDDimension] = *response.Id
+						metrics.SendMetricData(cp.metricPusher, getMetric(util.NSGType, Update), time.Since(startTime).Seconds(), dimensionsMap)
 					}
 				}
 			}
@@ -698,7 +735,7 @@ func (cp *CloudProvider) EnsureLoadBalancer(ctx context.Context, clusterName str
 		}
 	}
 
-	if !exists {
+	if !lbExists {
 		lbStatus, newLBOCID, err := lbProvider.createLoadBalancer(ctx, spec)
 		if err != nil {
 			logger.With(zap.Error(err)).Error("Failed to provision LoadBalancer")
@@ -728,7 +765,10 @@ func (cp *CloudProvider) EnsureLoadBalancer(ctx context.Context, clusterName str
 	// was just created then these values would be equal; however, if the load balancer
 	// already existed and the default subnet ids changed, then this would ensure
 	// we are setting the security rules on the correct subnets.
-	spec.Subnets = lb.SubnetIds
+	spec, err = updateSpecWithLbSubnets(spec, lb.SubnetIds)
+	if err != nil {
+		return nil, err
+	}
 
 	// If the load balancer needs an SSL cert ensure it is present.
 	if requiresCertificate(service) {
@@ -1139,6 +1179,15 @@ func (cp *CloudProvider) EnsureLoadBalancerDeleted(ctx context.Context, clusterN
 	var nsgMetricDimension string
 
 	dimensionsMap := make(map[string]string)
+	var frontendNsgId = ""
+	uid := fmt.Sprintf("%s", service.UID)
+	var etag *string
+
+	securityRuleManagementMode, nsg, err := getRuleManagementMode(service)
+	if err != nil {
+		logger.With(zap.Error(err)).Error("failed to get rule management mode")
+		return errors.Wrap(err, "failed to get rule management mode")
+	}
 
 	lbProvider, err := cp.getLoadBalancerProvider(ctx, service)
 	if err != nil {
@@ -1148,6 +1197,33 @@ func (cp *CloudProvider) EnsureLoadBalancerDeleted(ctx context.Context, clusterN
 	if err != nil {
 		if client.IsNotFound(err) {
 			logger.Info("Could not find load balancer. Nothing to do.")
+			if securityRuleManagementMode == NSG {
+				displayName := generateNsgName(service)
+				nsg.frontendNsgId, etag, err = cp.getFrontendNsgByName(ctx, logger, displayName, cp.config.CompartmentID, cp.config.VCNID, uid)
+				if err != nil {
+					return errors.Wrap(err, "failed to get frontend NSG")
+				}
+				// Delete of NSG happens if NSG was created but LB creation fails
+				if nsg != nil && nsg.nsgRuleManagementMode == RuleManagementModeNsg && nsg.frontendNsgId != "" {
+					if etag != nil {
+						logger = logger.With("frontendNsgId", nsg.frontendNsgId)
+						logger.Infof("deleting frontend nsg %s", nsg.frontendNsgId)
+						nsgDeleted, err := cp.deleteNsg(ctx, logger, nsg.frontendNsgId, *etag)
+						if !nsgDeleted || err != nil {
+							logger.With(zap.Error(err)).Error("failed to delete nsg")
+							errorType = util.GetError(err)
+							nsgMetricDimension = util.GetMetricDimensionForComponent(errorType, util.NSGType)
+							dimensionsMap[metrics.ComponentDimension] = nsgMetricDimension
+							metrics.SendMetricData(cp.metricPusher, getMetric(util.NSGType, Delete), time.Since(startTime).Seconds(), dimensionsMap)
+							return err
+						}
+						nsgMetricDimension = util.GetMetricDimensionForComponent(util.Success, util.NSGType)
+						dimensionsMap[metrics.ComponentDimension] = nsgMetricDimension
+						metrics.SendMetricData(cp.metricPusher, getMetric(util.NSGType, Delete), time.Since(startTime).Seconds(), dimensionsMap)
+						logger.Infof("Managed nsg with id %s deleted", nsg.frontendNsgId)
+					}
+				}
+			}
 			return nil
 		}
 		errorType = util.GetError(err)
@@ -1162,18 +1238,10 @@ func (cp *CloudProvider) EnsureLoadBalancerDeleted(ctx context.Context, clusterN
 	id := *lb.Id
 	dimensionsMap[metrics.ResourceOCIDDimension] = id
 	logger = logger.With("loadBalancerID", id, "loadBalancerType", getLoadBalancerType(service))
-	securityRuleManagementMode, nsg, err := getRuleManagementMode(service)
-	if err != nil {
-		logger.With(zap.Error(err)).Error("failed to get rule management mode")
-		return errors.Wrap(err, "failed to get rule management mode")
-	}
 
-	var etag *string
-	var frontendNsgId = ""
 	if securityRuleManagementMode == NSG {
 		// List network security groups
 		nsgs := lb.NetworkSecurityGroupIds
-		uid := fmt.Sprintf("%s", service.UID)
 		for _, nsgId := range nsgs {
 			frontendNsgId, etag, err = cp.getFrontendNsg(ctx, logger, nsgId, uid)
 			if err != nil {
@@ -1183,7 +1251,7 @@ func (cp *CloudProvider) EnsureLoadBalancerDeleted(ctx context.Context, clusterN
 				metrics.SendMetricData(cp.metricPusher, getMetric(util.NSGType, Get), time.Since(startTime).Seconds(), dimensionsMap)
 			}
 			if frontendNsgId != "" {
-				logger.With("frontendNsgId", frontendNsgId).Infof("queue frontend nsg for delete %s", frontendNsgId)
+				logger = logger.With("frontendNsgId", frontendNsgId)
 				nsg.frontendNsgId = frontendNsgId
 				break
 			}
@@ -1231,8 +1299,10 @@ func (cp *CloudProvider) EnsureLoadBalancerDeleted(ctx context.Context, clusterN
 	metrics.SendMetricData(cp.metricPusher, getMetric(loadBalancerType, Delete), time.Since(startTime).Seconds(), dimensionsMap)
 
 	// Delete of NSG happens after delete of the Loadbalancer
-	if nsg != nil && nsg.nsgRuleManagementMode == RuleManagementModeNsg && frontendNsgId != "" {
+	if nsg != nil && nsg.nsgRuleManagementMode == RuleManagementModeNsg && nsg.frontendNsgId != "" {
 		if etag != nil {
+			logger = logger.With("frontendNsgId", nsg.frontendNsgId)
+			logger.Infof("deleting frontend nsg %s", nsg.frontendNsgId)
 			nsgDeleted, err := cp.deleteNsg(ctx, logger, nsg.frontendNsgId, *etag)
 			if !nsgDeleted || err != nil {
 				logger.With(zap.Error(err)).Error("failed to delete nsg")
@@ -1245,7 +1315,7 @@ func (cp *CloudProvider) EnsureLoadBalancerDeleted(ctx context.Context, clusterN
 			nsgMetricDimension = util.GetMetricDimensionForComponent(util.Success, util.NSGType)
 			dimensionsMap[metrics.ComponentDimension] = nsgMetricDimension
 			metrics.SendMetricData(cp.metricPusher, getMetric(util.NSGType, Delete), time.Since(startTime).Seconds(), dimensionsMap)
-			logger.With("frontendNsgId", nsg.frontendNsgId).Infof("ccm managed nsg with id %s deleted", nsg.frontendNsgId)
+			logger.Infof("managed nsg with id %s deleted", nsg.frontendNsgId)
 		}
 	}
 
@@ -1450,26 +1520,41 @@ func (cp *CloudProvider) checkAllBackendNodesNotReady(nodeList []*v1.Node) bool
 func (cp *CloudProvider) deleteNsg(ctx context.Context, logger *zap.SugaredLogger, id, etag string) (bool, error) {
 	opcRequestId, err := cp.client.Networking().DeleteNetworkSecurityGroup(ctx, id, etag)
 	if err != nil {
-		logger.Errorf("failed to delete nsg %s OpcRequestId %s", id, *opcRequestId)
+		logger.Errorf("failed to delete nsg %s OpcRequestId %s", id, pointer.StringDeref(opcRequestId, ""))
 		return false, err
 	}
-	logger.Infof("delete nsg OpcRequestId %s", *opcRequestId)
+	logger.Infof("delete nsg OpcRequestId %s", pointer.StringDeref(opcRequestId, ""))
 	return true, nil
 }
 
-func (cp *CloudProvider) getFrontendNsg(ctx context.Context, logger *zap.SugaredLogger, id, uid string) (string, *string, error) {
+func (cp *CloudProvider) getFrontendNsg(ctx context.Context, logger *zap.SugaredLogger, id, uid string) (frontendNsgId string, etag *string, err error) {
 	nsg, etag, err := cp.client.Networking().GetNetworkSecurityGroup(ctx, id)
-	if err != nil || etag == nil {
+	if err != nil || nsg == nil || etag == nil {
 		logger.Errorf("failed to get nsg %s", id)
 		return "", nil, err
 	}
-	logger.Infof("Get nsg %s", *nsg)
-	freeFormTags := map[string]string{"CreatedBy": "OKE-CCM", "ServiceUid": uid}
+	freeFormTags := map[string]string{"CreatedBy": "CCM", "ServiceUid": uid}
 	if reflect.DeepEqual(nsg.FreeformTags, freeFormTags) {
-		logger.Infof("Found OKE created frontend nsg %s", *nsg.Id)
-		return *nsg.Id, etag, nil
+		nsgId := pointer.StringDeref(nsg.Id, "")
+		logger.Infof("Found managed frontend nsg %s", nsgId)
+		return nsgId, etag, nil
 	} else {
-		logger.Infof("Found attached nsg %s but not OKE created", *nsg.Id)
+		logger.Infof("Found attached nsgs %s but not managed", pointer.StringDeref(nsg.Id, ""))
 		return "", nil, nil
 	}
 }
+
+func (cp *CloudProvider) getFrontendNsgByName(ctx context.Context, logger *zap.SugaredLogger, displayName, compartmentId, vcnId, uid string) (frontendNsgId string, etag *string, err error) {
+	nsgs, err := cp.client.Networking().ListNetworkSecurityGroups(ctx, displayName, compartmentId, vcnId)
+	for _, nsg := range nsgs {
+		frontendNsgId, etag, err = cp.getFrontendNsg(ctx, logger, pointer.StringDeref(nsg.Id, ""), uid)
+		if err != nil {
+			return "", nil, err
+		}
+		if frontendNsgId != "" {
+			logger.Infof("found frontend NSG %s", frontendNsgId)
+			return frontendNsgId, etag, nil
+		}
+	}
+	return "", nil, nil
+}