Ensure backup server is removed from upstreams when the Backup Service is deleted

examples/custom-resources/backup-directive/virtual-server/README.md

@@ -92,7 +92,7 @@ Note that the backup service is configured with cluster domain name of the exter
 Run the below curl command to get a response from your application. In this example we hit the `/coffee` endpoint:
 
    ```shell
-   curl --resolve cafe.example.com:$IC_HTTPS_PORT:$IC_IP https://cafe.example.com:$IC_HTTPS_PORT/tea --insecure
+   curl --resolve cafe.example.com:$IC_HTTPS_PORT:$IC_IP https://cafe.example.com:$IC_HTTPS_PORT/coffee --insecure
    ```
 
    ```shell
@@ -115,7 +115,7 @@ Run the below curl command to get a response from your application. In this exam
 2. Run the below curl command. Notice that Server name in the response is `coffee-backup-<id>` instead of `coffee-<id>`
 
    ```shell
-   curl --resolve cafe.example.com:$IC_HTTPS_PORT:$IC_IP https://cafe.example.com:$IC_HTTPS_PORT/tea --insecure
+   curl --resolve cafe.example.com:$IC_HTTPS_PORT:$IC_IP https://cafe.example.com:$IC_HTTPS_PORT/coffee --insecure
    ```
 
 3. Check response from the backup service

internal/configs/transportserver.go

@@ -186,9 +186,9 @@ func generateStreamUpstreams(transportServerEx *TransportServerEx, upstreamNamer
 		}
 
 		var backupEndpoints []string
-		if u.Backup != nil && u.BackupPort != nil {
-			backupEnpointsKey := GenerateEndpointsKey(transportServerEx.TransportServer.Namespace, *u.Backup, nil, *u.BackupPort)
-			externalNameSvcKey = GenerateExternalNameSvcKey(transportServerEx.TransportServer.Namespace, *u.Backup)
+		if u.Backup != "" && u.BackupPort != nil {
+			backupEnpointsKey := GenerateEndpointsKey(transportServerEx.TransportServer.Namespace, u.Backup, nil, *u.BackupPort)
+			externalNameSvcKey = GenerateExternalNameSvcKey(transportServerEx.TransportServer.Namespace, u.Backup)
 
 			backupEndpoints = transportServerEx.Endpoints[backupEnpointsKey]
 			_, isExternalNameSvc = transportServerEx.ExternalNameSvcs[externalNameSvcKey]

internal/configs/transportserver_test.go

@@ -598,7 +598,7 @@ func TestGenerateTransportServerConfigForBackupServiceNGINXPlus(t *testing.T) {
 
 	transportServerEx := tsEx()
 	transportServerEx.TransportServer.Spec.Upstreams[0].LoadBalancingMethod = "least_conn"
-	transportServerEx.TransportServer.Spec.Upstreams[0].Backup = createPointerFromString("backup-svc")
+	transportServerEx.TransportServer.Spec.Upstreams[0].Backup = "backup-svc"
 	transportServerEx.TransportServer.Spec.Upstreams[0].BackupPort = createPointerFromUInt16(5090)
 	transportServerEx.Endpoints = map[string][]string{
 		"default/tcp-app-svc:5001": {
@@ -752,7 +752,7 @@ func TestGenerateTransportServerConfig_DoesNotGenerateBackupOnMissingBackupPort(
 
 	transportServerEx := tsEx()
 	transportServerEx.TransportServer.Spec.Upstreams[0].LoadBalancingMethod = "least_conn"
-	transportServerEx.TransportServer.Spec.Upstreams[0].Backup = createPointerFromString("clustertwo.corp.local")
+	transportServerEx.TransportServer.Spec.Upstreams[0].Backup = "clustertwo.corp.local"
 	transportServerEx.TransportServer.Spec.Upstreams[0].BackupPort = nil
 	transportServerEx.Endpoints = map[string][]string{
 		"default/tcp-app-svc:5001": {
@@ -826,7 +826,7 @@ func TestGenerateTransportServerConfig_DoesNotGenerateBackupOnMissingBackupPortA
 
 	transportServerEx := tsEx()
 	transportServerEx.TransportServer.Spec.Upstreams[0].LoadBalancingMethod = "least_conn"
-	transportServerEx.TransportServer.Spec.Upstreams[0].Backup = nil
+	transportServerEx.TransportServer.Spec.Upstreams[0].Backup = ""
 	transportServerEx.TransportServer.Spec.Upstreams[0].BackupPort = nil
 	transportServerEx.Endpoints = map[string][]string{
 		"default/tcp-app-svc:5001": {

internal/configs/virtualserver.go

@@ -315,18 +315,18 @@ func (vsc *virtualServerConfigurator) generateBackupEndpointsForUpstream(
 	upstream conf_v1.Upstream,
 	virtualServerEx *VirtualServerEx,
 ) []string {
-	if upstream.Backup == nil || upstream.BackupPort == nil {
+	if upstream.Backup == "" || upstream.BackupPort == nil {
 		return []string{}
 	}
-	externalNameSvcKey := GenerateExternalNameSvcKey(namespace, *upstream.Backup)
+	externalNameSvcKey := GenerateExternalNameSvcKey(namespace, upstream.Backup)
 	_, isExternalNameSvc := virtualServerEx.ExternalNameSvcs[externalNameSvcKey]
 	if isExternalNameSvc && !vsc.isResolverConfigured {
 		msgFmt := "Type ExternalName service %v in upstream %v will be ignored. To use ExternaName services, a resolver must be configured in the ConfigMap"
-		vsc.addWarningf(owner, msgFmt, *upstream.Backup, upstream.Name)
+		vsc.addWarningf(owner, msgFmt, upstream.Backup, upstream.Name)
 		return []string{}
 	}
 
-	backupEndpointsKey := GenerateEndpointsKey(namespace, *upstream.Backup, upstream.Subselector, *upstream.BackupPort)
+	backupEndpointsKey := GenerateEndpointsKey(namespace, upstream.Backup, upstream.Subselector, *upstream.BackupPort)
 	backupEndpoints := virtualServerEx.Endpoints[backupEndpointsKey]
 	if len(backupEndpoints) == 0 {
 		return []string{}
@@ -2442,8 +2442,8 @@ func createUpstreamsForPlus(
 		endpoints := virtualServerEx.Endpoints[endpointsKey]
 
 		backupEndpoints := []string{}
-		if u.Backup != nil {
-			backupEndpointsKey := GenerateEndpointsKey(upstreamNamespace, *u.Backup, u.Subselector, *u.BackupPort)
+		if u.Backup != "" {
+			backupEndpointsKey := GenerateEndpointsKey(upstreamNamespace, u.Backup, u.Subselector, *u.BackupPort)
 			backupEndpoints = virtualServerEx.Endpoints[backupEndpointsKey]
 		}
 		ups := vsc.generateUpstream(virtualServerEx.VirtualServer, upstreamName, u, isExternalNameSvc, endpoints, backupEndpoints)
@@ -2467,8 +2467,8 @@ func createUpstreamsForPlus(
 
 			// BackupService
 			backupEndpoints := []string{}
-			if u.Backup != nil {
-				backupEndpointsKey := GenerateEndpointsKey(upstreamNamespace, *u.Backup, u.Subselector, *u.BackupPort)
+			if u.Backup != "" {
+				backupEndpointsKey := GenerateEndpointsKey(upstreamNamespace, u.Backup, u.Subselector, *u.BackupPort)
 				backupEndpoints = virtualServerEx.Endpoints[backupEndpointsKey]
 			}
 			ups := vsc.generateUpstream(vsr, upstreamName, u, isExternalNameSvc, endpoints, backupEndpoints)

internal/configs/virtualserver_test.go

@@ -991,13 +991,6 @@ func createPointerFromUInt16(n uint16) *uint16 {
 	return &n
 }
 
-// createPointerFromString is a helper that takes a string
-// and returns a pointer to the value. It is used for testing
-// BackupService configuration for Virtual and Transport Servers.
-func createPointerFromString(s string) *string {
-	return &s
-}
-
 // vsEx returns Virtual Server Ex config struct.
 // It's safe to modify returned config for parallel test execution.
 func vsEx() VirtualServerEx {
@@ -1185,7 +1178,7 @@ func TestGenerateVirtualServerConfigWithBackupForNGINXPlus(t *testing.T) {
 
 	virtualServerEx := vsEx()
 	virtualServerEx.VirtualServer.Spec.Upstreams[2].LBMethod = "least_conn"
-	virtualServerEx.VirtualServer.Spec.Upstreams[2].Backup = createPointerFromString("backup-svc")
+	virtualServerEx.VirtualServer.Spec.Upstreams[2].Backup = "backup-svc"
 	virtualServerEx.VirtualServer.Spec.Upstreams[2].BackupPort = createPointerFromUInt16(8090)
 	virtualServerEx.Endpoints = map[string][]string{
 		"default/tea-svc:80": {
@@ -1496,7 +1489,7 @@ func TestGenerateVirtualServerConfig_DoesNotGenerateBackupOnMissingBackupNameFor
 
 	virtualServerEx := vsEx()
 	virtualServerEx.VirtualServer.Spec.Upstreams[2].LBMethod = "least_conn"
-	virtualServerEx.VirtualServer.Spec.Upstreams[2].Backup = nil
+	virtualServerEx.VirtualServer.Spec.Upstreams[2].Backup = ""
 	virtualServerEx.VirtualServer.Spec.Upstreams[2].BackupPort = createPointerFromUInt16(8090)
 	virtualServerEx.Endpoints = map[string][]string{
 		"default/tea-svc:80": {
@@ -1802,7 +1795,7 @@ func TestGenerateVirtualServerConfig_DoesNotGenerateBackupOnMissingBackupPortFor
 
 	virtualServerEx := vsEx()
 	virtualServerEx.VirtualServer.Spec.Upstreams[2].LBMethod = "least_conn"
-	virtualServerEx.VirtualServer.Spec.Upstreams[2].Backup = createPointerFromString("backup-svc")
+	virtualServerEx.VirtualServer.Spec.Upstreams[2].Backup = "backup-svc"
 	virtualServerEx.VirtualServer.Spec.Upstreams[2].BackupPort = nil
 	virtualServerEx.Endpoints = map[string][]string{
 		"default/tea-svc:80": {
@@ -2107,7 +2100,7 @@ func TestGenerateVirtualServerConfig_DoesNotGenerateBackupOnMissingBackupPortAnd
 
 	virtualServerEx := vsEx()
 	virtualServerEx.VirtualServer.Spec.Upstreams[2].LBMethod = "least_conn"
-	virtualServerEx.VirtualServer.Spec.Upstreams[2].Backup = nil
+	virtualServerEx.VirtualServer.Spec.Upstreams[2].Backup = ""
 	virtualServerEx.VirtualServer.Spec.Upstreams[2].BackupPort = nil
 	virtualServerEx.Endpoints = map[string][]string{
 		"default/tea-svc:80": {

internal/k8s/controller.go

@@ -3072,16 +3072,16 @@ func (lbc *LoadBalancerController) createVirtualServerEx(virtualServer *conf_v1.
 	// If backup and backup port are defined it generates a backup server entry for the upstream.
 	// Backup Service is of type ExternalName.
 	generateBackupEndpoints := func(endpoints map[string][]string, u conf_v1.Upstream) {
-		if u.Backup == nil || u.BackupPort == nil {
+		if u.Backup == "" || u.BackupPort == nil {
 			return
 		}
-		backupEndpointsKey := configs.GenerateEndpointsKey(virtualServer.Namespace, *u.Backup, u.Subselector, *u.BackupPort)
-		backupEndps, external, err := lbc.getEndpointsForUpstream(virtualServer.Namespace, *u.Backup, *u.BackupPort)
+		backupEndpointsKey := configs.GenerateEndpointsKey(virtualServer.Namespace, u.Backup, u.Subselector, *u.BackupPort)
+		backupEndps, external, err := lbc.getEndpointsForUpstream(virtualServer.Namespace, u.Backup, *u.BackupPort)
 		if err != nil {
 			glog.Warningf("Error getting Endpoints for Upstream %v: %v", u.Name, err)
 		}
 		if err == nil && external {
-			externalNameSvcs[configs.GenerateExternalNameSvcKey(virtualServer.Namespace, *u.Backup)] = true
+			externalNameSvcs[configs.GenerateExternalNameSvcKey(virtualServer.Namespace, u.Backup)] = true
 		}
 		bendps := getIPAddressesFromEndpoints(backupEndps)
 		endpoints[backupEndpointsKey] = bendps
@@ -3606,14 +3606,14 @@ func (lbc *LoadBalancerController) createTransportServerEx(transportServer *conf
 		}
 
 		// If backup defined on Upstream retrieve its external name and port.
-		if u.Backup != nil && u.BackupPort != nil {
-			backupEndpointsKey := configs.GenerateEndpointsKey(transportServer.Namespace, *u.Backup, nil, *u.BackupPort)
-			backupEndps, external, err := lbc.getEndpointsForUpstream(transportServer.Namespace, *u.Backup, *u.BackupPort)
+		if u.Backup != "" && u.BackupPort != nil {
+			backupEndpointsKey := configs.GenerateEndpointsKey(transportServer.Namespace, u.Backup, nil, *u.BackupPort)
+			backupEndps, external, err := lbc.getEndpointsForUpstream(transportServer.Namespace, u.Backup, *u.BackupPort)
 			if err != nil {
 				glog.Warningf("Error getting Endpoints for Upstream %v: %v", u.Name, err)
 			}
 			if err == nil && external {
-				externalNameSvcs[configs.GenerateExternalNameSvcKey(transportServer.Namespace, *u.Backup)] = true
+				externalNameSvcs[configs.GenerateExternalNameSvcKey(transportServer.Namespace, u.Backup)] = true
 			}
 			bendps := getIPAddressesFromEndpoints(backupEndps)
 			endpoints[backupEndpointsKey] = bendps

internal/k8s/reference_checkers.go

@@ -147,7 +147,7 @@ func (rc *serviceReferenceChecker) IsReferencedByVirtualServer(svcNamespace stri
 		if rc.hasClusterIP && u.UseClusterIP {
 			continue
 		}
-		if u.Service == svcName {
+		if u.Service == svcName || u.Backup == svcName {
 			return true
 		}
 	}
@@ -178,7 +178,7 @@ func (rc *serviceReferenceChecker) IsReferencedByTransportServer(svcNamespace st
 	}
 
 	for _, u := range ts.Spec.Upstreams {
-		if u.Service == svcName {
+		if u.Service == svcName || u.Backup == svcName {
 			return true
 		}
 	}

internal/k8s/reference_checkers_test.go

@@ -513,6 +513,80 @@ func TestServiceIsReferencedByIngressAndMinion(t *testing.T) {
 	}
 }
 
+func TestBackupServiceIsReferencedByVirtualServer(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		vs                *conf_v1.VirtualServer
+		serviceNamespace  string
+		backupServiceName string
+		expected          bool
+		msg               string
+	}{
+		{
+			vs: &conf_v1.VirtualServer{
+				ObjectMeta: v1.ObjectMeta{
+					Namespace: "default",
+				},
+				Spec: conf_v1.VirtualServerSpec{
+					Upstreams: []conf_v1.Upstream{
+						{
+							Backup: "test-backup-service",
+						},
+					},
+				},
+			},
+			serviceNamespace:  "default",
+			backupServiceName: "test-backup-service",
+			expected:          true,
+			msg:               "Backup service is referenced by this VirtualServer",
+		},
+		{
+			vs: &conf_v1.VirtualServer{
+				ObjectMeta: v1.ObjectMeta{
+					Namespace: "default",
+				},
+				Spec: conf_v1.VirtualServerSpec{
+					Upstreams: []conf_v1.Upstream{
+						{
+							Backup: "test-backup-service-does-not-exist",
+						},
+					},
+				},
+			},
+			serviceNamespace:  "default",
+			backupServiceName: "test-backup-service",
+			expected:          false,
+			msg:               "Backup service is not referenced by this VirtualServer",
+		},
+		{
+			vs: &conf_v1.VirtualServer{
+				ObjectMeta: v1.ObjectMeta{
+					Namespace: "default",
+				},
+				Spec: conf_v1.VirtualServerSpec{
+					Upstreams: []conf_v1.Upstream{
+						{
+							Backup: "test-backup-service",
+						},
+					},
+				},
+			},
+			serviceNamespace:  "non-default-namespace",
+			backupServiceName: "test-backup-service",
+			expected:          false,
+			msg:               "Backup service is in different namespace",
+		},
+	}
+	for _, test := range tests {
+		rc := newServiceReferenceChecker(false)
+
+		result := rc.IsReferencedByVirtualServer(test.serviceNamespace, test.backupServiceName, test.vs)
+		if result != test.expected {
+			t.Errorf("IsReferencedByVirtualServer() returned %v but expected %v for the case of %s", result, test.expected, test.msg)
+		}
+	}
+}
+
 func TestServiceIsReferencedByVirtualServerAndVirtualServerRoutes(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
@@ -630,7 +704,7 @@ func TestServiceIsReferencedByVirtualServerAndVirtualServerRoutes(t *testing.T)
 	}
 }
 
-func TestIsServiceReferencedByTransportServer(t *testing.T) {
+func TestServiceIsReferencedByTransportServer(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		ts               *conf_v1.TransportServer
@@ -705,6 +779,81 @@ func TestIsServiceReferencedByTransportServer(t *testing.T) {
 	}
 }
 
+func TestBackupServiceIsReferencedByTransportServer(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		ts                     *conf_v1.TransportServer
+		backupServiceNamespace string
+		backupServiceName      string
+		expected               bool
+		msg                    string
+	}{
+		{
+			ts: &conf_v1.TransportServer{
+				ObjectMeta: v1.ObjectMeta{
+					Namespace: "default",
+				},
+				Spec: conf_v1.TransportServerSpec{
+					Upstreams: []conf_v1.TransportServerUpstream{
+						{
+							Backup: "test-backup-service",
+						},
+					},
+				},
+			},
+			backupServiceNamespace: "default",
+			backupServiceName:      "test-backup-service",
+			expected:               true,
+			msg:                    "Backup Service is referenced by this TransportService",
+		},
+		{
+			ts: &conf_v1.TransportServer{
+				ObjectMeta: v1.ObjectMeta{
+					Namespace: "default",
+				},
+				Spec: conf_v1.TransportServerSpec{
+					Upstreams: []conf_v1.TransportServerUpstream{
+						{
+							Backup: "test-backup-service",
+						},
+					},
+				},
+			},
+			backupServiceNamespace: "some-namespace",
+			backupServiceName:      "test-backup-service",
+			expected:               false,
+			msg:                    "wrong namespace for service in an upstream",
+		},
+		{
+			ts: &conf_v1.TransportServer{
+				ObjectMeta: v1.ObjectMeta{
+					Namespace: "default",
+				},
+				Spec: conf_v1.TransportServerSpec{
+					Upstreams: []conf_v1.TransportServerUpstream{
+						{
+							Service: "test-backup-service",
+						},
+					},
+				},
+			},
+			backupServiceNamespace: "default",
+			backupServiceName:      "random-backup-service",
+			expected:               false,
+			msg:                    "wrong name for service in an upstream",
+		},
+	}
+
+	for _, test := range tests {
+		rc := newServiceReferenceChecker(false)
+
+		result := rc.IsReferencedByTransportServer(test.backupServiceNamespace, test.backupServiceName, test.ts)
+		if result != test.expected {
+			t.Errorf("IsReferencedByTransportServer() returned %v but expected %v for the case of %s", result, test.expected, test.msg)
+		}
+	}
+}
+
 func TestPolicyIsReferencedByIngressesAndTransportServers(t *testing.T) {
 	t.Parallel()
 	rc := newPolicyReferenceChecker()