Review feedback

Author: ciarams87

apis/v1alpha2/nginxproxy_types.go

@@ -401,6 +401,7 @@ type Patch struct {
 	// For StrategicMerge and Merge patches, this should be a JSON object.
 	// For JSONPatch patches, this should be a JSON array of patch operations.
 	//
+	// +optional
 	// +kubebuilder:validation:XPreserveUnknownFields
 	Value *apiextv1.JSON `json:"value"`
 }

go.mod

@@ -20,8 +20,8 @@ require (
 	golang.org/x/text v0.27.0
 	google.golang.org/grpc v1.74.0
 	google.golang.org/protobuf v1.36.6
+	gopkg.in/evanphx/json-patch.v4 v4.12.0
 	k8s.io/api v0.33.3
-    gopkg.in/evanphx/json-patch.v4 v4.12.0
 	k8s.io/apiextensions-apiserver v0.33.3
 	k8s.io/apimachinery v0.33.3
 	k8s.io/client-go v0.33.3

internal/controller/provisioner/objects.go

@@ -180,11 +180,7 @@ func (p *NginxProvisioner) buildNginxResourceObjects(
 	}
 	objects = append(objects, service, deployment)
 
-	if len(errs) > 0 {
-		return objects, errors.Join(errs...)
-	}
-
-	return objects, nil
+	return objects, errors.Join(errs...)
 }
 
 func (p *NginxProvisioner) buildNginxSecrets(
@@ -570,10 +566,8 @@ func (p *NginxProvisioner) buildNginxDeployment(
 		}
 
 		// Apply DaemonSet patches
-		if nProxyCfg.Kubernetes.DaemonSet != nil {
-			if err := applyPatches(daemonSet, nProxyCfg.Kubernetes.DaemonSet.Patches); err != nil {
-				return daemonSet, fmt.Errorf("failed to apply daemonset patches: %w", err)
-			}
+		if err := applyPatches(daemonSet, nProxyCfg.Kubernetes.DaemonSet.Patches); err != nil {
+			return daemonSet, fmt.Errorf("failed to apply daemonset patches: %w", err)
 		}
 
 		return daemonSet, nil
@@ -592,19 +586,16 @@ func (p *NginxProvisioner) buildNginxDeployment(
 	var deploymentCfg ngfAPIv1alpha2.DeploymentSpec
 	if nProxyCfg != nil && nProxyCfg.Kubernetes != nil && nProxyCfg.Kubernetes.Deployment != nil {
 		deploymentCfg = *nProxyCfg.Kubernetes.Deployment
+		// Apply Deployment patches
+		if err := applyPatches(deployment, nProxyCfg.Kubernetes.Deployment.Patches); err != nil {
+			return deployment, fmt.Errorf("failed to apply deployment patches: %w", err)
+		}
 	}
 
 	if deploymentCfg.Replicas != nil {
 		deployment.Spec.Replicas = deploymentCfg.Replicas
 	}
 
-	// Apply Deployment patches
-	if nProxyCfg != nil && nProxyCfg.Kubernetes != nil && nProxyCfg.Kubernetes.Deployment != nil {
-		if err := applyPatches(deployment, nProxyCfg.Kubernetes.Deployment.Patches); err != nil {
-			return deployment, fmt.Errorf("failed to apply deployment patches: %w", err)
-		}
-	}
-
 	return deployment, nil
 }
 

internal/controller/provisioner/objects_test.go

@@ -1273,6 +1273,43 @@ func TestBuildNginxResourceObjects_Patches(t *testing.T) {
 	g.Expect(dep.Labels).To(HaveKeyWithValue("dep-patched", "true"))
 	g.Expect(dep.Spec.Replicas).To(Equal(helpers.GetPointer(int32(3))))
 
+	// Test that a later patch overrides a field set by an earlier patch
+	nProxyCfg = &graph.EffectiveNginxProxy{
+		Kubernetes: &ngfAPIv1alpha2.KubernetesSpec{
+			Service: &ngfAPIv1alpha2.ServiceSpec{
+				Patches: []ngfAPIv1alpha2.Patch{
+					{
+						Type: helpers.GetPointer(ngfAPIv1alpha2.PatchTypeStrategicMerge),
+						Value: &apiextv1.JSON{
+							Raw: []byte(`{"metadata":{"labels":{"override-label":"first"}}}`),
+						},
+					},
+					{
+						Type: helpers.GetPointer(ngfAPIv1alpha2.PatchTypeStrategicMerge),
+						Value: &apiextv1.JSON{
+							Raw: []byte(`{"metadata":{"labels":{"override-label":"second"}}}`),
+						},
+					},
+				},
+			},
+		},
+	}
+
+	objects, err = provisioner.buildNginxResourceObjects("gw-nginx", gateway, nProxyCfg)
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(objects).To(HaveLen(6))
+
+	// Find and validate service label override
+	svc = nil
+	for _, obj := range objects {
+		if s, ok := obj.(*corev1.Service); ok {
+			svc = s
+			break
+		}
+	}
+	g.Expect(svc).ToNot(BeNil())
+	g.Expect(svc.Labels).To(HaveKeyWithValue("override-label", "second"))
+
 	// Test successful daemonset patch
 	nProxyCfg = &graph.EffectiveNginxProxy{
 		Kubernetes: &ngfAPIv1alpha2.KubernetesSpec{