Remove deployment/daemonset when necessary

sjberman · sjberman · commit 524bec106083 · 2025-05-22T16:10:51.000-06:00
diff --git a/internal/framework/controller/resource.go b/internal/framework/controller/resource.go
@@ -2,21 +2,10 @@ package controller
 
 import (
 	"fmt"
-
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
 )
 
 // CreateNginxResourceName creates the base resource name for all nginx resources
 // created by the control plane.
 func CreateNginxResourceName(prefix, suffix string) string {
 	return fmt.Sprintf("%s-%s", prefix, suffix)
 }
-
-// ObjectMetaToNamespacedName converts ObjectMeta to NamespacedName.
-func ObjectMetaToNamespacedName(meta metav1.ObjectMeta) types.NamespacedName {
-	return types.NamespacedName{
-		Namespace: meta.Namespace,
-		Name:      meta.Name,
-	}
-}
diff --git a/internal/mode/static/provisioner/handler.go b/internal/mode/static/provisioner/handler.go
@@ -268,40 +268,25 @@ func (h *eventHandler) deprovisionSecretsForAllGateways(ctx context.Context, sec
 
 		switch {
 		case strings.HasSuffix(resources.AgentTLSSecret.Name, secret):
-			if err := h.provisioner.deleteSecret(
-				ctx,
-				controller.ObjectMetaToNamespacedName(resources.AgentTLSSecret),
-			); err != nil {
+			if err := h.provisioner.deleteObject(ctx, &corev1.Secret{ObjectMeta: resources.AgentTLSSecret}); err != nil {
 				allErrs = append(allErrs, err)
 			}
 		case strings.HasSuffix(resources.PlusJWTSecret.Name, secret):
-			if err := h.provisioner.deleteSecret(
-				ctx,
-				controller.ObjectMetaToNamespacedName(resources.PlusJWTSecret),
-			); err != nil {
+			if err := h.provisioner.deleteObject(ctx, &corev1.Secret{ObjectMeta: resources.PlusJWTSecret}); err != nil {
 				allErrs = append(allErrs, err)
 			}
 		case strings.HasSuffix(resources.PlusCASecret.Name, secret):
-			if err := h.provisioner.deleteSecret(
-				ctx,
-				controller.ObjectMetaToNamespacedName(resources.PlusCASecret),
-			); err != nil {
+			if err := h.provisioner.deleteObject(ctx, &corev1.Secret{ObjectMeta: resources.PlusCASecret}); err != nil {
 				allErrs = append(allErrs, err)
 			}
 		case strings.HasSuffix(resources.PlusClientSSLSecret.Name, secret):
-			if err := h.provisioner.deleteSecret(
-				ctx,
-				controller.ObjectMetaToNamespacedName(resources.PlusClientSSLSecret),
-			); err != nil {
+			if err := h.provisioner.deleteObject(ctx, &corev1.Secret{ObjectMeta: resources.PlusClientSSLSecret}); err != nil {
 				allErrs = append(allErrs, err)
 			}
 		default:
 			for _, dockerSecret := range resources.DockerSecrets {
 				if strings.HasSuffix(dockerSecret.Name, secret) {
-					if err := h.provisioner.deleteSecret(
-						ctx,
-						controller.ObjectMetaToNamespacedName(dockerSecret),
-					); err != nil {
+					if err := h.provisioner.deleteObject(ctx, &corev1.Secret{ObjectMeta: dockerSecret}); err != nil {
 						allErrs = append(allErrs, err)
 					}
 				}
diff --git a/internal/mode/static/provisioner/provisioner.go b/internal/mode/static/provisioner/provisioner.go
@@ -265,7 +265,7 @@ func (p *NginxProvisioner) provisionNginx(
 		p.store.registerResourceInGatewayConfig(client.ObjectKeyFromObject(gateway), obj)
 	}
 
-	// if agent configmap was updated, then we'll need to restart the deployment
+	// if agent configmap was updated, then we'll need to restart the deployment/daemonset
 	if agentConfigMapUpdated && !deploymentCreated {
 		updateCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
 		defer cancel()
@@ -286,7 +286,7 @@ func (p *NginxProvisioner) provisionNginx(
 		}
 
 		p.cfg.Logger.V(1).Info(
-			"Restarting nginx deployment after agent configmap update",
+			"Restarting nginx after agent configmap update",
 			"name", object.GetName(),
 			"namespace", object.GetNamespace(),
 		)
@@ -296,7 +296,7 @@ func (p *NginxProvisioner) provisionNginx(
 				object,
 				corev1.EventTypeWarning,
 				"RestartFailed",
-				"Failed to restart nginx deployment after agent config update: %s",
+				"Failed to restart nginx after agent config update: %s",
 				err.Error(),
 			)
 			return err
@@ -361,11 +361,11 @@ func (p *NginxProvisioner) deprovisionNginx(ctx context.Context, gatewayNSName t
 
 		objects := p.buildNginxResourceObjectsForDeletion(deploymentNSName)
 
-		createCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
+		deleteCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
 		defer cancel()
 
 		for _, obj := range objects {
-			if err := p.k8sClient.Delete(createCtx, obj); err != nil && !apierrors.IsNotFound(err) {
+			if err := p.k8sClient.Delete(deleteCtx, obj); err != nil && !apierrors.IsNotFound(err) {
 				p.cfg.EventRecorder.Eventf(
 					obj,
 					corev1.EventTypeWarning,
@@ -384,6 +384,28 @@ func (p *NginxProvisioner) deprovisionNginx(ctx context.Context, gatewayNSName t
 	return nil
 }
 
+func (p *NginxProvisioner) deleteObject(ctx context.Context, obj client.Object) error {
+	if !p.isLeader() {
+		return nil
+	}
+
+	deleteCtx, cancel := context.WithTimeout(ctx, 10*time.Second)
+	defer cancel()
+
+	if err := p.k8sClient.Delete(deleteCtx, obj); err != nil && !apierrors.IsNotFound(err) {
+		p.cfg.EventRecorder.Eventf(
+			obj,
+			corev1.EventTypeWarning,
+			"DeleteFailed",
+			"Failed to delete nginx resource: %s",
+			err.Error(),
+		)
+		return err
+	}
+
+	return nil
+}
+
 // isUserSecret determines if the provided secret name is a special user secret,
 // for example an NGINX docker registry secret or NGINX Plus secret.
 func (p *NginxProvisioner) isUserSecret(name string) bool {
@@ -404,25 +426,6 @@ func (p *NginxProvisioner) isUserSecret(name string) bool {
 	return false
 }
 
-func (p *NginxProvisioner) deleteSecret(ctx context.Context, secretNSName types.NamespacedName) error {
-	if !p.isLeader() {
-		return nil
-	}
-
-	secret := &corev1.Secret{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      secretNSName.Name,
-			Namespace: secretNSName.Namespace,
-		},
-	}
-
-	if err := p.k8sClient.Delete(ctx, secret); err != nil && !apierrors.IsNotFound(err) {
-		return err
-	}
-
-	return nil
-}
-
 // RegisterGateway is called by the main event handler when a Gateway API resource event occurs
 // and the graph is built. The provisioner updates the Gateway config in the store and then:
 // - If it's a valid Gateway, create or update nginx resources associated with the Gateway, if necessary.
@@ -447,6 +450,20 @@ func (p *NginxProvisioner) RegisterGateway(
 			p.cfg.Logger.Error(err, "error building some nginx resources")
 		}
 
+		// If NGINX deployment type switched between Deployment and DaemonSet, clean up the old one.
+		nginxResources := p.store.getNginxResourcesForGateway(gatewayNSName)
+		if nginxResources != nil {
+			if needToDeleteDaemonSet(nginxResources) {
+				if err := p.deleteObject(ctx, &appsv1.DaemonSet{ObjectMeta: nginxResources.DaemonSet}); err != nil {
+					p.cfg.Logger.Error(err, "error deleting nginx resource")
+				}
+			} else if needToDeleteDeployment(nginxResources) {
+				if err := p.deleteObject(ctx, &appsv1.Deployment{ObjectMeta: nginxResources.Deployment}); err != nil {
+					p.cfg.Logger.Error(err, "error deleting nginx resource")
+				}
+			}
+		}
+
 		if err := p.provisionNginx(ctx, resourceName, gateway.Source, objects); err != nil {
 			return fmt.Errorf("error provisioning nginx resources: %w", err)
 		}
@@ -458,3 +475,31 @@ func (p *NginxProvisioner) RegisterGateway(
 
 	return nil
 }
+
+func needToDeleteDeployment(cfg *NginxResources) bool {
+	if cfg.Deployment.Name != "" {
+		if cfg.Gateway != nil && cfg.Gateway.EffectiveNginxProxy != nil &&
+			cfg.Gateway.EffectiveNginxProxy.Kubernetes != nil &&
+			cfg.Gateway.EffectiveNginxProxy.Kubernetes.DaemonSet != nil {
+			return true
+		}
+	}
+
+	return false
+}
+
+func needToDeleteDaemonSet(cfg *NginxResources) bool {
+	if cfg.DaemonSet.Name != "" && cfg.Gateway != nil {
+		if cfg.Gateway.EffectiveNginxProxy != nil &&
+			cfg.Gateway.EffectiveNginxProxy.Kubernetes != nil &&
+			cfg.Gateway.EffectiveNginxProxy.Kubernetes.Deployment != nil {
+			return true
+		} else if cfg.Gateway.EffectiveNginxProxy == nil ||
+			cfg.Gateway.EffectiveNginxProxy.Kubernetes == nil ||
+			cfg.Gateway.EffectiveNginxProxy.Kubernetes.DaemonSet == nil {
+			return true
+		}
+	}
+
+	return false
+}
diff --git a/internal/mode/static/provisioner/provisioner_test.go b/internal/mode/static/provisioner/provisioner_test.go
@@ -314,6 +314,80 @@ func TestRegisterGateway(t *testing.T) {
 	g.Expect(deploymentStore.RemoveCallCount()).To(Equal(1))
 }
 
+func TestRegisterGateway_CleansUpOldDeploymentOrDaemonSet(t *testing.T) {
+	t.Parallel()
+	g := NewWithT(t)
+
+	// Setup: Gateway switches from Deployment to DaemonSet
+	gateway := &graph.Gateway{
+		Source: &gatewayv1.Gateway{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "gw",
+				Namespace: "default",
+			},
+		},
+		Valid: true,
+		EffectiveNginxProxy: &graph.EffectiveNginxProxy{
+			Kubernetes: &ngfAPIv1alpha2.KubernetesSpec{
+				DaemonSet: &ngfAPIv1alpha2.DaemonSetSpec{},
+			},
+		},
+	}
+
+	// Create a fake deployment that should be cleaned up
+	oldDeployment := &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "gw-nginx",
+			Namespace: "default",
+		},
+	}
+	provisioner, fakeClient, _ := defaultNginxProvisioner(gateway.Source, oldDeployment)
+	// Simulate store tracking an old Deployment
+	provisioner.store.nginxResources[types.NamespacedName{Name: "gw", Namespace: "default"}] = &NginxResources{
+		Deployment: oldDeployment.ObjectMeta,
+	}
+
+	// RegisterGateway should clean up the Deployment and create a DaemonSet
+	g.Expect(provisioner.RegisterGateway(t.Context(), gateway, "gw-nginx")).To(Succeed())
+
+	// Deployment should be deleted
+	err := fakeClient.Get(t.Context(), types.NamespacedName{Name: "gw-nginx", Namespace: "default"}, &appsv1.Deployment{})
+	g.Expect(err).To(HaveOccurred())
+
+	// DaemonSet should exist
+	err = fakeClient.Get(t.Context(), types.NamespacedName{Name: "gw-nginx", Namespace: "default"}, &appsv1.DaemonSet{})
+	g.Expect(err).ToNot(HaveOccurred())
+
+	// Now test the opposite: switch from DaemonSet to Deployment
+	gateway.EffectiveNginxProxy = &graph.EffectiveNginxProxy{
+		Kubernetes: &ngfAPIv1alpha2.KubernetesSpec{
+			Deployment: &ngfAPIv1alpha2.DeploymentSpec{},
+		},
+	}
+
+	oldDaemonSet := &appsv1.DaemonSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "gw-nginx",
+			Namespace: "default",
+		},
+	}
+
+	provisioner, fakeClient, _ = defaultNginxProvisioner(gateway.Source, oldDaemonSet)
+	provisioner.store.nginxResources[types.NamespacedName{Name: "gw", Namespace: "default"}] = &NginxResources{
+		DaemonSet: oldDaemonSet.ObjectMeta,
+	}
+
+	g.Expect(provisioner.RegisterGateway(t.Context(), gateway, "gw-nginx")).To(Succeed())
+
+	// DaemonSet should be deleted
+	err = fakeClient.Get(t.Context(), types.NamespacedName{Name: "gw-nginx", Namespace: "default"}, &appsv1.DaemonSet{})
+	g.Expect(err).To(HaveOccurred())
+
+	// Deployment should exist
+	err = fakeClient.Get(t.Context(), types.NamespacedName{Name: "gw-nginx", Namespace: "default"}, &appsv1.Deployment{})
+	g.Expect(err).ToNot(HaveOccurred())
+}
+
 func TestNonLeaderProvisioner(t *testing.T) {
 	t.Parallel()
 	g := NewWithT(t)
diff --git a/internal/mode/static/provisioner/store_test.go b/internal/mode/static/provisioner/store_test.go
@@ -104,7 +104,7 @@ func TestRegisterResourceInGatewayConfig(t *testing.T) {
 	store := newStore([]string{"docker-secret"}, "agent-tls-secret", "jwt-secret", "ca-secret", "client-ssl-secret")
 	nsName := types.NamespacedName{Name: "test-gateway", Namespace: "default"}
 
-	registerAndGetResources := func(obj interface{}) *NginxResources {
+	registerAndGetResources := func(obj any) *NginxResources {
 		changed := store.registerResourceInGatewayConfig(nsName, obj)
 		g.Expect(changed).To(BeTrue(), fmt.Sprintf("failed: %T", obj))
 		g.Expect(store.nginxResources).To(HaveKey(nsName), fmt.Sprintf("failed: %T", obj))
