Fix controller when NEG status is empty

This commit addresses an issue where the autoneg controller would fail
to reconcile backend services when the Network Endpoint Group (NEG)
status annotation was empty. This scenario typically occurs if the
GKE NEG controller has not yet provisioned the NEGs for a service.

The root cause was that an empty backend service name was being used in
the request to the Google Cloud Compute API. This caused the API to
treat the 'get' request as a 'list' request, leading to a JSON
unmarshaling error due to the different response format. This error
prevented the controller from removing finalizers on services marked
for deletion, causing namespaces to become stuck.

The following changes have been made:

- Updated `ReconcileBackends` in `controllers/autoneg.go` to
  handle cases where the upsert backend service name is empty,
  preventing calls to `getBackendService` with an empty name.
- Added `context.Context` propagation through `ReconcileBackends`.
- Added a new unit test, `TestReconcileBackendsDeletionWithEmptyNEGStatus`,
  in `controllers/autoneg_test.go` to specifically cover the error scenario.
- Updated function signatures and calls in `controllers/service_controller.go`
  and `controllers/suite_test.go` to align with the changes in the `BackendController` interface.

Author: rohandvora

controllers/autoneg.go

@@ -201,15 +201,21 @@ func checkOperation(op *compute.Operation) error {
 
 // ReconcileBackends takes the actual and intended AutonegStatus
 // and attempts to apply the intended status or return an error
-func (b *ProdBackendController) ReconcileBackends(actual, intended AutonegStatus) (err error) {
+func (b *ProdBackendController) ReconcileBackends(ctx context.Context, actual, intended AutonegStatus) (err error) {
+	logger := log.FromContext(ctx)
+	// Determine which backends to remove and which to insert/update.
 	removes, upserts := ReconcileStatus(b.project, actual, intended)
+	logger.Info("Reconciling backends", "removes", removes, "upserts", upserts)
 
 	var forceCapacity map[int]bool = make(map[int]bool, 0)
+	// Iterate over each port that has backends to be removed.
 	for port, _removes := range removes {
+		// Iterate over each backend service to be removed.
 		for idx, remove := range _removes {
 			var oldSvc *compute.BackendService
-			oldSvc, err = b.getBackendService(remove.name, remove.region)
 			var svcUpdated = false
+			// Get the current state of the backend service.
+			oldSvc, err = b.getBackendService(remove.name, remove.region)
 			var e *errNotFound
 			if errors.As(err, &e) {
 				// If the backend service is gone, we construct a BackendService with the same name
@@ -226,16 +232,18 @@ func (b *ProdBackendController) ReconcileBackends(actual, intended AutonegStatus
 			var newSvc *compute.BackendService
 			upsert := upserts[port][idx]
 
-			if upsert.name != remove.name {
+			// Check if the same port is in the upsert map and if upsert needs to happen on a different backend service.
+			if upsert.name != "" && upsert.name != remove.name {
 				if newSvc, err = b.getBackendService(upsert.name, upsert.region); err != nil {
 					return
 				}
 			} else {
 				newSvc = oldSvc
 			}
 
-			// Remove backends in the list to be deleted
+			// Remove backends that are in the list to be deleted for this port.
 			for _, d := range remove.backends {
+				// Remove only the requested backends and keep the rest.
 				for i, be := range oldSvc.Backends {
 					if d.Group == be.Group {
 						svcUpdated = true
@@ -246,8 +254,9 @@ func (b *ProdBackendController) ReconcileBackends(actual, intended AutonegStatus
 				}
 			}
 
-			// If we are changing backend services, save the old service
-			if upsert.name != remove.name && svcUpdated {
+			// If a different service needs to be updated based on the upsert map entry for this port,
+			// then save the existing backend service and update the new service.
+			if svcUpdated && (upsert.name == "" || upsert.name != remove.name) {
 				if err = b.updateBackends(remove.name, remove.region, oldSvc, forceCapacity); err != nil {
 					return
 				}
@@ -263,7 +272,7 @@ func (b *ProdBackendController) ReconcileBackends(actual, intended AutonegStatus
 						be.MaxConnectionsPerEndpoint = u.MaxConnectionsPerEndpoint
 						if intended.AutonegSyncConfig != nil {
 							var syncConfig AutonegSyncConfig = *intended.AutonegSyncConfig
-							if syncConfig.CapacityScaler != nil && *syncConfig.CapacityScaler == true {
+							if syncConfig.CapacityScaler != nil && *syncConfig.CapacityScaler {
 								be.CapacityScaler = u.CapacityScaler
 							}
 						} else {
@@ -477,6 +486,7 @@ func getStatuses(ctx context.Context, namespace string, name string, annotations
 	if ok {
 		// Found a status, decode
 		if err = json.Unmarshal([]byte(tmp), &s.negConfig); err != nil {
+			err = fmt.Errorf("failed to decode neg annotation %s: %w", negAnnotation, err)
 			return
 		}
 	}
@@ -489,12 +499,14 @@ func getStatuses(ctx context.Context, namespace string, name string, annotations
 
 		var tempConfig AutonegConfigTemp
 		if err = json.Unmarshal([]byte(tmp), &tempConfig); err != nil {
+			err = fmt.Errorf("failed to decode autoneg annotation %s: %w", autonegAnnotation, err)
 			return
 		}
 
 		tmpSync, syncOk := annotations[autonegSyncAnnotation]
 		if syncOk {
 			if err = json.Unmarshal([]byte(tmpSync), &s.syncConfig); err != nil {
+				err = fmt.Errorf("failed to decode autoneg-sync annotation %s: %w", autonegSyncAnnotation, err)
 				return
 			}
 		}
@@ -540,6 +552,7 @@ func getStatuses(ctx context.Context, namespace string, name string, annotations
 		}
 		// Found a status, decode
 		if err = json.Unmarshal([]byte(tmp), &s.status); err != nil {
+			err = fmt.Errorf("failed to decode autoneg-status annotation %s: %w", autonegStatusAnnotation, err)
 			return
 		}
 	}
@@ -551,6 +564,7 @@ func getStatuses(ctx context.Context, namespace string, name string, annotations
 			valid = true
 
 			if err = json.Unmarshal([]byte(tmp), &s.oldConfig); err != nil {
+				err = fmt.Errorf("failed to decode %s annotation %s: %w", oldAutonegAnnotation, tmp, err)
 				return
 			}
 
@@ -579,7 +593,7 @@ func getStatuses(ctx context.Context, namespace string, name string, annotations
 					Connections: 0,
 				}
 			} else {
-				err = errors.New(fmt.Sprintf("more than one port in %s, but autoneg configuration is for one or no ports", negAnnotation))
+				err = fmt.Errorf("more than one port in %s, but autoneg configuration is for one or no ports", negAnnotation)
 				return
 			}
 		}
@@ -593,6 +607,7 @@ func getStatuses(ctx context.Context, namespace string, name string, annotations
 			}
 			// Found a status, decode
 			if err = json.Unmarshal([]byte(tmp), &s.oldStatus); err != nil {
+				err = fmt.Errorf("failed to decode %s annotation %s: %w", oldAutonegStatusAnnotation, tmp, err)
 				return
 			}
 		}

controllers/autoneg_test.go

@@ -720,6 +720,10 @@ var (
 		AutonegConfig: configBasic,
 		NEGStatus:     negStatus,
 	}
+	statusBasicWithEmptyNEGs = AutonegStatus{
+		AutonegConfig: configBasic,
+		NEGStatus:     NEGStatus{},
+	}
 	backendsBasicWithNEGs = map[string]map[string]Backends{"80": map[string]Backends{"test": Backends{name: "test", backends: []compute.Backend{
 		statusBasicWithNEGs.Backend("test", "80", getGroup(fakeProject, "zone1", fakeNeg)),
 		statusBasicWithNEGs.Backend("test", "80", getGroup(fakeProject, "zone2", fakeNeg)),
@@ -991,6 +995,7 @@ func Test_checkOperation(t *testing.T) {
 
 func TestReconcileBackendsDeletionWithMissingBackend(t *testing.T) {
 	s := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) {
+		t.Logf("Got request: %s", req.URL.String())
 		// Return not found on backend service get.
 		res.WriteHeader(http.StatusNotFound)
 	}))
@@ -1002,7 +1007,7 @@ func TestReconcileBackendsDeletionWithMissingBackend(t *testing.T) {
 		project: "test-project",
 		s:       cs,
 	}
-	err = bc.ReconcileBackends(statusBasicWithNEGs, AutonegStatus{
+	err = bc.ReconcileBackends(context.Background(), statusBasicWithNEGs, AutonegStatus{
 		// On deletion, the intended state is set to empty.
 		AutonegConfig: AutonegConfig{},
 		NEGStatus:     negStatus,
@@ -1012,6 +1017,43 @@ func TestReconcileBackendsDeletionWithMissingBackend(t *testing.T) {
 	}
 }
 
+func TestReconcileBackendsDeletionWithEmptyNEGStatus(t *testing.T) {
+	s := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) {
+		t.Logf("Got request: %s", req.URL.String())
+		// Return not found on backend service get.
+		res.WriteHeader(http.StatusNotFound)
+	}))
+	cs, err := compute.NewService(context.Background(), option.WithEndpoint(s.URL), option.WithoutAuthentication())
+	if err != nil {
+		t.Fatalf("Failed to instantiate compute service: %v", err)
+	}
+	bc := ProdBackendController{
+		project: "test-project",
+		s:       cs,
+	}
+	err = bc.ReconcileBackends(context.Background(), AutonegStatus{
+		AutonegConfig: AutonegConfig{
+			BackendServices: map[string]map[string]AutonegNEGConfig{
+				"80": {
+					"test": AutonegNEGConfig{
+						Name: "test",
+						Rate: 100,
+					},
+				},
+			},
+		},
+		NEGStatus: NEGStatus{}, // NEG status not populated by GKE NEG controller.
+	}, AutonegStatus{
+		AutonegConfig: AutonegConfig{
+			BackendServices: map[string]map[string]AutonegNEGConfig{},
+		},
+		NEGStatus: NEGStatus{},
+	})
+	if err != nil {
+		t.Errorf("ReconcileBackends() got err: %v, want none", err)
+	}
+}
+
 type fakeReader struct {
 	client.Reader
 	svcNeg *v1beta1.ServiceNetworkEndpointGroup

controllers/service_controller.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"reflect"
 	"time"
 
@@ -35,7 +36,7 @@ import (
 )
 
 type BackendController interface {
-	ReconcileBackends(AutonegStatus, AutonegStatus) error
+	ReconcileBackends(context.Context, AutonegStatus, AutonegStatus) error
 }
 
 // ServiceReconciler reconciles a Service object
@@ -104,7 +105,7 @@ func (r *ServiceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		AutonegConfig: status.config,
 		NEGStatus:     status.negStatus,
 	}
-	logger.Info("Existing status", "status", status)
+	logger.Info("Existing status", "status", fmt.Sprintf("%+v", status))
 	if status.syncConfig != nil {
 		intendedStatus.AutonegSyncConfig = status.syncConfig
 	}
@@ -124,7 +125,7 @@ func (r *ServiceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	// Reconcile differences
 	logger.Info("Applying intended status", "status", intendedStatus)
 
-	if err = r.ReconcileBackends(status.status, intendedStatus); err != nil {
+	if err = r.ReconcileBackends(ctx, status.status, intendedStatus); err != nil {
 		var e *errNotFound
 		if !(deleting && errors.As(err, &e)) {
 			r.Recorder.Event(svc, "Warning", "BackendError", err.Error())

controllers/suite_test.go

@@ -105,7 +105,7 @@ type TestBackendController struct {
 	Counter int
 }
 
-func (t *TestBackendController) ReconcileBackends(AutonegStatus, AutonegStatus) error {
+func (t *TestBackendController) ReconcileBackends(context.Context, AutonegStatus, AutonegStatus) error {
 	t.Counter++
 	fmt.Print(t.Counter)
 	return nil