Repository: nginx/nginx-gateway-fabric

Issue:
Lack of dataplane service annotation possibility prevents v1 -> v2 migration
<!--
WARNING: Prior to submitting an enhancement request, we ask that you create a discussion. If you have not yet
created a discussion related to your request, please do so now: https://github.com/nginx/nginx-gateway-fabric/discussions/new?category=ideas
-->

**Is your enhancement request related to a problem? Please describe.**
TLDR - lack of dataplane service annotation annotations required by [Azure AKS](https://learn.microsoft.com/en-us/azure/aks/internal-lb?tabs=set-service-annotations) makes migration from v1 towards v2 impossible for NGF users who create internally reachable gateways (atleast on Azure).

I ran into an issue while migrating v1 ngf to v2. The transition otherwise went smoothly, but my main problem lies with the way how Azure requires annotations be added for the `Loadbalancer` type service that would be created for the dataplane. IIUC - the v1 implementation of ngf left it to maintainer of gateway to configure/patch the service with correct annotations (link in TLDR). The v2 on the other hand seems to create these for the user, without any option to add annotations into them, which would be crucial in our context.

I really like the v2 idea and implementation here, but due to how Azure expects internally reachable load balancers to be annotated, it makes us impossible to move forward. We would like to continue using NGF, but might need to migrate to different gateway implementation incase v1 is no longer maintained (which is understandable) and v2 will not provide way for service annotations for the dataplane.

Is there any solution / potential enchancement to allow us using NGF?

**What would you like to be added**:
A way to provide annotations for the dataplane services created by NGF v2

**Why this is needed**:
Like said above, not having this feature drops possibilities to use NGF atleast in any environments that need to provide internally reachable loadbalancers.


Discussion:
@nginx-bot: Hi @stutommi! Welcome to the project! 🎉

Thanks for opening this issue!
Be sure to check out our [Contributing Guidelines](../blob/main/CONTRIBUTING.md) and the [Issue Lifecycle](../blob/main/ISSUE_LIFECYCLE.md) while you wait for someone on the team to take a look at this.

<!-- nginx-bot-comment -->@sjberman: Hi @stutommi, labels and annotations for provisioned nginx resources (such as the LoadBalancer Service) can be defined in the Gateway resource, in the `spec.infrastructure` section. See [this guide](https://docs.nginx.com/nginx-gateway-fabric/install/deploy-data-plane/#set-annotations-and-labels-on-provisioned-resources) on how to do that. Note that these labels/annotations get applied to every resource we provision (including the Deployment). Functionally, that shouldn't affect anything, but it may look odd.

In our 2.1 release later this week, we're also adding the ability to specify custom patches per resource (in the NginxProxy CRD), which can allow for setting resource-specific labels/annotations on just the Service, instead of setting them in the Gateway's infrastructure and having them propagated onto every resource we provision.

Let me know if this addresses your needs!@stutommi: Thanks for the quick reply! Both ways you described sound like valid ways forward, although I'm much happier using the way that the 2.1 release brings.

I'll test the first option tomorrow 👍😊@stutommi: Commenting here that I ended up waiting for the 2.1 release - since thinking it will anyway be the preferred solution for my problem. I can comment the outcome once the release lands.@mattmichal: > labels and annotations for provisioned nginx resources (such as the LoadBalancer Service) can be defined in the Gateway resource, in the spec.infrastructure section. See [this guide](https://docs.nginx.com/nginx-gateway-fabric/install/deploy-data-plane/#set-annotations-and-labels-on-provisioned-resources) on how to do that. Note that these labels/annotations get applied to every resource we provision (including the Deployment). Functionally, that shouldn't affect anything, but it may look odd.

This works and has the described behavior of having annotations on both the Service and Deployment which isn't as tidy as it could be.

> In our 2.1 release later this week, we're also adding the ability to specify custom patches per resource (in the NginxProxy CRD), which can allow for setting resource-specific labels/annotations on just the Service

I tested this with 2.1.0 and I found that patching the Service with annotations caused the Deployment to be updated and patching the Deployment with annotations caused the Service to be updated. Even after ensuring that the Gateway had no infrastructure annotations configured, I still observed this behavior.

```yaml
apiVersion: gateway.nginx.org/v1alpha2
kind: NginxProxy
metadata:
  annotations:
    meta.helm.sh/release-name: nginx-gateway-fabric
    meta.helm.sh/release-namespace: nginx-gateway
  labels:
    app.kubernetes.io/instance: nginx-gateway-fabric
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: nginx-gateway-fabric
    app.kubernetes.io/version: 2.1.0
    helm.sh/chart: nginx-gateway-fabric-2.1.0
  name: nginx-gateway-fabric-proxy-config
  namespace: nginx-gateway
spec:
  ipFamily: dual
  kubernetes:
    deployment:
      container:
        image:
          pullPolicy: IfNotPresent
          repository: ghcr.io/nginx/nginx-gateway-fabric/nginx
          tag: 2.1.0
      patches: []
      replicas: 1
    service:
      externalTrafficPolicy: Local
      patches:
        - type: StrategicMerge
          value:
            metadata:
              annotations:
                service.beta.kubernetes.io/azure-load-balancer-internal: 'true'
                service.beta.kubernetes.io/azure-load-balancer-internal-subnet: subnet
                service.beta.kubernetes.io/azure-load-balancer-resource-group: rg
      type: LoadBalancer
```

```yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  annotations:
    field.cattle.io/publicEndpoints: ...
    service.beta.kubernetes.io/azure-load-balancer-internal: 'true'
    service.beta.kubernetes.io/azure-load-balancer-internal-subnet: subnet
    service.beta.kubernetes.io/azure-load-balancer-resource-group: rg
  generation: 6879  # config update loop
  labels:
    app.kubernetes.io/instance: nginx-gateway-fabric
    app.kubernetes.io/managed-by: nginx-gateway-fabric-nginx
    app.kubernetes.io/name: example-nginx
    gateway.networking.k8s.io/gateway-name: example
...
```

I am using Rancher which is setting a `publicEndpoints` annotation which is causing a config update loop (generation 6879) so I could see a scenario where that is causing conflicts or other unexpected behavior.

@stutommi were you able to test? Did you observe similar behavior with annotations being applied to the Deployment incorrectly?@sjberman: Hm, could be a bug on our side. I think @ciarams87 might have seen this occur. We can investigate that.@ciarams87: The problem is that both the Service and Deployment are sharing the same [objectMeta variable](https://github.com/nginx/nginx-gateway-fabric/blob/v2.1.0/internal/controller/provisioner/objects.go#L111), which contains the same Labels map. When you patch the Service and modify its labels, the shared map that's also referenced by the Deployment is being modified.

I have opened a PR with the fix.@stutommi: I haven't had the time to test yet. I had potential time today, but figured if the issue with 2.1 is already figured and has a PR, I'll wait until it has landed.

@ciarams87 is the fix going to be made into potential quicker 2.1.1 release or waiting until 2.2.0? Just figuring out whether to install the 2.1.0 and along with the bug (which shouldn't pose any effective problems IIUC) or wait for the fix to land. :)

Solution patch:
diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go
index 6abe6f23..14b3cbcc 100644
--- a/internal/controller/provisioner/objects.go
+++ b/internal/controller/provisioner/objects.go
@@ -152,13 +152,28 @@ func (p *NginxProvisioner) buildNginxResourceObjects(
 		ports[int32(listener.Port)] = struct{}{}
 	}
 
-	service, err := buildNginxService(objectMeta, nProxyCfg, ports, selectorLabels)
+	// Create separate copies of objectMeta for service and deployment to avoid shared map references
+	serviceObjectMeta := metav1.ObjectMeta{
+		Name:        objectMeta.Name,
+		Namespace:   objectMeta.Namespace,
+		Labels:      maps.Clone(objectMeta.Labels),
+		Annotations: maps.Clone(objectMeta.Annotations),
+	}
+
+	deploymentObjectMeta := metav1.ObjectMeta{
+		Name:        objectMeta.Name,
+		Namespace:   objectMeta.Namespace,
+		Labels:      maps.Clone(objectMeta.Labels),
+		Annotations: maps.Clone(objectMeta.Annotations),
+	}
+
+	service, err := buildNginxService(serviceObjectMeta, nProxyCfg, ports, selectorLabels)
 	if err != nil {
 		errs = append(errs, err)
 	}
 
 	deployment, err := p.buildNginxDeployment(
-		objectMeta,
+		deploymentObjectMeta,
 		nProxyCfg,
 		ngxIncludesConfigMapName,
 		ngxAgentConfigMapName,



Test patch:
diff --git a/internal/controller/provisioner/objects_test.go b/internal/controller/provisioner/objects_test.go
index 600ff5d8..4af6f2de 100644
--- a/internal/controller/provisioner/objects_test.go
+++ b/internal/controller/provisioner/objects_test.go
@@ -1696,4 +1696,62 @@ func TestBuildNginxResourceObjects_Patches(t *testing.T) {
 	}
 	g.Expect(svc).ToNot(BeNil())
 	g.Expect(svc.Labels).ToNot(HaveKey("patched")) // Should not have patch-related labels
+
+	// Test that Service patches don't affect Deployment labels and vice versa (cross-contamination)
+	nProxyCfg = &graph.EffectiveNginxProxy{
+		Kubernetes: &ngfAPIv1alpha2.KubernetesSpec{
+			Service: &ngfAPIv1alpha2.ServiceSpec{
+				Patches: []ngfAPIv1alpha2.Patch{
+					{
+						Type: helpers.GetPointer(ngfAPIv1alpha2.PatchTypeStrategicMerge),
+						Value: &apiextv1.JSON{
+							Raw: []byte(`{"metadata":{"labels":{"service-only":"true"}}}`),
+						},
+					},
+				},
+			},
+			Deployment: &ngfAPIv1alpha2.DeploymentSpec{
+				Patches: []ngfAPIv1alpha2.Patch{
+					{
+						Type: helpers.GetPointer(ngfAPIv1alpha2.PatchTypeStrategicMerge),
+						Value: &apiextv1.JSON{
+							Raw: []byte(`{"metadata":{"labels":{"deployment-only":"true"}}}`),
+						},
+					},
+				},
+			},
+		},
+	}
+
+	objects, err = provisioner.buildNginxResourceObjects("gw-nginx", gateway, nProxyCfg)
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(objects).To(HaveLen(6))
+
+	// Find and validate service - should only have service-specific labels
+	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("service-only", "true"))
+	g.Expect(svc.Labels).ToNot(HaveKey("deployment-only"))
+
+	// Find and validate deployment - should only have deployment-specific labels
+	dep = nil
+	for _, obj := range objects {
+		if d, ok := obj.(*appsv1.Deployment); ok {
+			dep = d
+			break
+		}
+	}
+	g.Expect(dep).ToNot(BeNil())
+	g.Expect(dep.Labels).To(HaveKeyWithValue("deployment-only", "true"))
+	g.Expect(dep.Labels).ToNot(HaveKey("service-only"))
+
+	// Both should still have the common base labels
+	g.Expect(svc.Labels).To(HaveKeyWithValue("app", "nginx"))
+	g.Expect(dep.Labels).To(HaveKeyWithValue("app", "nginx"))
 }