Merge pull request #301 from oracle/sec_list_leak_fix

Bug fix: node subnet ingress security rules can sometimes leak after deletion

Author: Jeff Bornemann

pkg/cloudprovider/providers/oci/load_balancer_security_lists.go

@@ -239,7 +239,7 @@ func (s *defaultSecurityListManager) Delete(ctx context.Context, lbSubnets []*co
 		return err
 	}
 
-	return s.updateBackendRules(ctx, noSubnets, backendSubnets, nil, ports)
+	return s.updateBackendRules(ctx, noSubnets, backendSubnets, &ports, ports)
 }
 
 // frontendSecurityListManager manages only the ingress security list rules required for
@@ -408,22 +408,15 @@ func getNodeIngressRules(
 			ingressRules = append(ingressRules, rule)
 			desiredHealthChecker.Delete(*rule.Source)
 			continue
-		}
-
-		inUse, err := healthCheckPortInUse(serviceLister, int32(desiredPorts.HealthCheckerPort))
-		if err != nil {
-			// Unable to determine if this port is in use by another service, so I guess
-			// we better err on the safe side and keep the rule.
-			logger.Errorf("failed to determine if port: %d is still in use: %v", desiredPorts.HealthCheckerPort, err)
-			ingressRules = append(ingressRules, rule)
-			continue
-		}
-		if inUse {
-			// This rule is no longer needed for this service, but is still used
-			// by another service, so we must still keep it.
-			logger.Infof("Port %d still in use by another service.", desiredPorts.HealthCheckerPort)
-			ingressRules = append(ingressRules, rule)
-			continue
+		} else if *r.Max == desiredPorts.HealthCheckerPort {
+			inUse, err := healthCheckPortInUse(serviceLister, int32(desiredPorts.HealthCheckerPort))
+			if err != nil {
+				logger.Errorf("failed to determine if port: %d is still in use: %v", desiredPorts.HealthCheckerPort, err)
+				ingressRules = append(ingressRules, rule)
+			} else if inUse {
+				logger.Infof("Port %d still in use by another service.", desiredPorts.HealthCheckerPort)
+				ingressRules = append(ingressRules, rule)
+			}
 		}
 
 		// else the actual cidr no longer exists so we don't need to do

pkg/cloudprovider/providers/oci/load_balancer_security_lists_test.go

@@ -141,10 +141,11 @@ func TestGetNodeIngressRules(t *testing.T) {
 				makeIngressSecurityRule("existing", 9001),
 			},
 		}, {
-			name: "do not delete a rule which is used by another services (default) health check",
+			name: "do not delete health check rules that are used by other services",
 			securityList: &core.SecurityList{
 				IngressSecurityRules: []core.IngressSecurityRule{
 					makeIngressSecurityRule("0.0.0.0/0", lbNodesHealthCheckPort),
+					makeIngressSecurityRule("0.0.0.0/0", 80),
 				},
 			},
 			lbSubnets: []*core.Subnet{},
@@ -157,7 +158,7 @@ func TestGetNodeIngressRules(t *testing.T) {
 					ObjectMeta: metav1.ObjectMeta{Namespace: "namespace", Name: "using-default-health-check-port"},
 					Spec: v1.ServiceSpec{
 						Type:  v1.ServiceTypeLoadBalancer,
-						Ports: []v1.ServicePort{{Port: 80}},
+						Ports: []v1.ServicePort{{Port: 443}},
 					},
 				},
 			},