Merge pull request #140 from akshaypatidar1999/fix/deregister-on-remove

fix: deregister NEGs on removing annotation
GoogleCloudPlatform · Oct 20, 2024 · 73a7581 · 73a7581
2 parents 2027f7a + ce5d078
commit 73a7581
Show file tree

Hide file tree

Showing 4 changed files with 116 additions and 34 deletions.
diff --git a/controllers/autoneg.go b/controllers/autoneg.go
@@ -507,16 +507,24 @@ func getStatuses(namespace string, name string, annotations map[string]string, r
 		}
 
 		s.newConfig = true
+	}
 
-		tmp, ok = annotations[autonegStatusAnnotation]
-		if ok {
-			// Found a status, decode
-			if err = json.Unmarshal([]byte(tmp), &s.status); err != nil {
-				return
-			}
+	// Does service has new auto neg status?
+	tmp, statusOk := annotations[autonegStatusAnnotation]
+	if statusOk {
+		// Status annotation found but auto neg annotation not found set empty auto neg config
+		if !newOk && r.DeregisterNEGsOnAnnotationRemoval {
+			s.config = AutonegConfig{}
+			valid = true
+			s.newConfig = true
+		}
+		// Found a status, decode
+		if err = json.Unmarshal([]byte(tmp), &s.status); err != nil {
+			return
 		}
 	}
-	if !newOk {
+
+	if !newOk && !statusOk {
 		// Is this service using autoneg in legacy mode?
 		tmp, oldOk = annotations[oldAutonegAnnotation]
 		if oldOk {
@@ -554,13 +562,18 @@ func getStatuses(namespace string, name string, annotations map[string]string, r
 				err = errors.New(fmt.Sprintf("more than one port in %s, but autoneg configuration is for one or no ports", negAnnotation))
 				return
 			}
+		}
 
-			tmp, ok = annotations[oldAutonegStatusAnnotation]
-			if ok {
-				// Found a status, decode
-				if err = json.Unmarshal([]byte(tmp), &s.oldStatus); err != nil {
-					return
-				}
+		tmp, ok = annotations[oldAutonegStatusAnnotation]
+		if ok {
+			// Status annotation found but auto neg annotation not found set empty auto neg config
+			if !oldOk && r.DeregisterNEGsOnAnnotationRemoval {
+				s.oldConfig = OldAutonegConfig{}
+				valid = true
+			}
+			// Found a status, decode
+			if err = json.Unmarshal([]byte(tmp), &s.oldStatus); err != nil {
+				return
 			}
 		}
 	}

diff --git a/controllers/autoneg_test.go b/controllers/autoneg_test.go
@@ -166,6 +166,14 @@ var statusTests = []struct {
 		true,
 		true,
 	},
+	{
+		"valid autoneg status without autoneg",
+		map[string]string{
+			autonegStatusAnnotation: validStatus,
+		},
+		true,
+		false,
+	},
 }
 
 var oldStatusTests = []struct {
@@ -256,12 +264,21 @@ var oldStatusTests = []struct {
 		true,
 		false,
 	},
+	{
+		"(legacy) valid autoneg status without autoneg",
+		map[string]string{
+			oldAutonegStatusAnnotation: validStatus,
+		},
+		true,
+		false,
+	},
 }
 
 func TestGetStatuses(t *testing.T) {
 	var serviceReconciler = ServiceReconciler{
-		ServiceNameTemplate: "{namespace}-{name}-{port}-{hash}",
-		AllowServiceName:    true,
+		ServiceNameTemplate:               "{namespace}-{name}-{port}-{hash}",
+		AllowServiceName:                  true,
+		DeregisterNEGsOnAnnotationRemoval: true,
 	}
 	for _, st := range statusTests {
 		_, valid, err := getStatuses("ns", "test", st.annotations, &serviceReconciler)
@@ -282,8 +299,9 @@ func TestGetStatuses(t *testing.T) {
 
 func TestGetOldStatuses(t *testing.T) {
 	var serviceReconciler = ServiceReconciler{
-		ServiceNameTemplate: "{namespace}-{name}-{port}-{hash}",
-		AllowServiceName:    true,
+		ServiceNameTemplate:               "{namespace}-{name}-{port}-{hash}",
+		AllowServiceName:                  true,
+		DeregisterNEGsOnAnnotationRemoval: true,
 	}
 	for _, st := range oldStatusTests {
 		_, valid, err := getStatuses("ns", "test", st.annotations, &serviceReconciler)
@@ -340,6 +358,52 @@ func TestGetStatusesServiceNameAllowed(t *testing.T) {
 	}
 }
 
+func TestGetStatusesOnlyAutonegStatusAnnotation(t *testing.T) {
+	var serviceReconciler = ServiceReconciler{
+		ServiceNameTemplate:               "{namespace}-{name}-{port}-{hash}",
+		AllowServiceName:                  true,
+		DeregisterNEGsOnAnnotationRemoval: true,
+	}
+	statuses, valid, err := getStatuses("ns", "test", map[string]string{autonegStatusAnnotation: validAutonegStatus}, &serviceReconciler)
+	if err != nil {
+		t.Errorf("Expected no error, got one: %v", err)
+	}
+	if !valid {
+		t.Errorf("Expected autoneg status config, got none")
+	}
+
+	if !statuses.newConfig {
+		t.Errorf("Expected new autoneg config")
+	}
+	if statuses.config.BackendServices != nil {
+		t.Errorf("Expected nil backend services")
+	}
+
+}
+
+func TestGetStatusesOnlyOldAutonegStatusAnnotation(t *testing.T) {
+	var serviceReconciler = ServiceReconciler{
+		ServiceNameTemplate:               "{namespace}-{name}-{port}-{hash}",
+		AllowServiceName:                  true,
+		DeregisterNEGsOnAnnotationRemoval: true,
+	}
+	statuses, valid, err := getStatuses("ns", "test", map[string]string{oldAutonegStatusAnnotation: validAutonegStatus}, &serviceReconciler)
+	if err != nil {
+		t.Errorf("Expected no error, got one: %v", err)
+	}
+	if !valid {
+		t.Errorf("Expected old autoneg status config, got none")
+	}
+
+	if statuses.newConfig {
+		t.Errorf("Expected old autoneg config")
+	}
+	if statuses.oldConfig.Name != "" {
+		t.Errorf("Expected empty old autoneg config")
+	}
+
+}
+
 func TestDefaultMaxRatePerEndpointWhenOverrideIsSet(t *testing.T) {
 	var serviceReconciler = ServiceReconciler{
 		ServiceNameTemplate:       "{namespace}-{name}-{port}",

diff --git a/controllers/service_controller.go b/controllers/service_controller.go
@@ -42,13 +42,14 @@ type ServiceReconciler struct {
 	client.Client
 	Scheme *runtime.Scheme
 	BackendController
-	Recorder                         record.EventRecorder
-	ServiceNameTemplate              string
-	AllowServiceName                 bool
-	MaxRatePerEndpointDefault        float64
-	MaxConnectionsPerEndpointDefault float64
-	AlwaysReconcile                  bool
-	ReconcileDuration                *time.Duration
+	Recorder                          record.EventRecorder
+	ServiceNameTemplate               string
+	AllowServiceName                  bool
+	MaxRatePerEndpointDefault         float64
+	MaxConnectionsPerEndpointDefault  float64
+	AlwaysReconcile                   bool
+	ReconcileDuration                 *time.Duration
+	DeregisterNEGsOnAnnotationRemoval bool
 }
 
 //+kubebuilder:rbac:groups=core,resources=services,verbs=get;list;watch;update;patch

diff --git a/main.go b/main.go
@@ -68,6 +68,7 @@ func main() {
 	var alwaysReconcile bool
 	var reconcilePeriod string
 	var namespaces string
+	var deregisterNEGsOnAnnotationRemoval bool
 	flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
 	flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
 	flag.Float64Var(&maxRatePerEndpointDefault, "max-rate-per-endpoint", 0, "Default max rate per endpoint. Can be overridden by user config.")
@@ -82,6 +83,8 @@ func main() {
 	flag.BoolVar(&allowServiceName, "enable-custom-service-names", true, "Enable using custom service names in autoneg annotation.")
 	flag.BoolVar(&alwaysReconcile, "always-reconcile", false, "Periodically reconciles even if annotation statuses don't change.")
 	flag.StringVar(&reconcilePeriod, "reconcile-period", "", "The minimum frequency at which watched resources are reconciled, e.g. 10m. Defaults to 10h if not set.")
+	flag.BoolVar(&deregisterNEGsOnAnnotationRemoval, "deregister-negs-on-annotation-removal", true, "Deregister NEGs from backend service when annotation removed.")
+
 	opts := zap.Options{
 		Development: true,
 	}
@@ -141,16 +144,17 @@ func main() {
 	}
 
 	if err = (&controllers.ServiceReconciler{
-		Client:                           mgr.GetClient(),
-		Scheme:                           mgr.GetScheme(),
-		BackendController:                controllers.NewBackendController(project, s),
-		Recorder:                         mgr.GetEventRecorderFor("autoneg-controller"),
-		ServiceNameTemplate:              serviceNameTemplate,
-		AllowServiceName:                 allowServiceName,
-		MaxRatePerEndpointDefault:        maxRatePerEndpointDefault,
-		MaxConnectionsPerEndpointDefault: maxConnectionsPerEndpointDefault,
-		AlwaysReconcile:                  alwaysReconcile,
-		ReconcileDuration:                &reconcileDuration,
+		Client:                            mgr.GetClient(),
+		Scheme:                            mgr.GetScheme(),
+		BackendController:                 controllers.NewBackendController(project, s),
+		Recorder:                          mgr.GetEventRecorderFor("autoneg-controller"),
+		ServiceNameTemplate:               serviceNameTemplate,
+		AllowServiceName:                  allowServiceName,
+		MaxRatePerEndpointDefault:         maxRatePerEndpointDefault,
+		MaxConnectionsPerEndpointDefault:  maxConnectionsPerEndpointDefault,
+		AlwaysReconcile:                   alwaysReconcile,
+		DeregisterNEGsOnAnnotationRemoval: deregisterNEGsOnAnnotationRemoval,
+		ReconcileDuration:                 &reconcileDuration,
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "Service")
 		os.Exit(1)