Skip to content

Commit ca25b10

Browse files
ciarams87ciarams87
committed
Preserve annotations on service; ignore replicas on HPA
1 parent 5c78a60 commit ca25b10

File tree

4 files changed

+150
-6
lines changed

4 files changed

+150
-6
lines changed

‎internal/controller/provisioner/objects.go‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -691,6 +691,9 @@ func (p *NginxProvisioner) buildNginxDeployment(
691691
replicas = deploymentCfg.Replicas
692692
}
693693

694+
// When HPA is enabled, we should not set the replicas field to allow HPA exclusive control.
695+
// Setting replicas causes a race condition where NGF and HPA fight over the replica count.
696+
// See: https://github.com/nginx/nginx-gateway-fabric/issues/4007
694697
if isAutoscalingEnabled(&deploymentCfg) {
695698
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
696699
defer cancel()
@@ -700,9 +703,14 @@ func (p *NginxProvisioner) buildNginxDeployment(
700703
Namespace: objectMeta.Namespace,
701704
Name: objectMeta.Name,
702705
}, hpa)
703-
if err == nil && hpa.Status.DesiredReplicas > 0 {
704-
// overwrite with HPA's desiredReplicas
705-
replicas = helpers.GetPointer(hpa.Status.DesiredReplicas)
706+
if err == nil {
707+
// HPA exists - do not set replicas field, let HPA manage it
708+
replicas = nil
709+
} else if replicas == nil && deploymentCfg.Autoscaling.MinReplicas != nil {
710+
// HPA doesn't exist yet - set initial replicas
711+
// This handles the initial deployment before HPA takes over
712+
// Use minReplicas as the initial value
713+
replicas = deploymentCfg.Autoscaling.MinReplicas
706714
}
707715
}
708716

‎internal/controller/provisioner/objects_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -462,8 +462,8 @@ func TestBuildNginxResourceObjects_DeploymentReplicasFromHPA(t *testing.T) {
462462
}
463463
}
464464
g.Expect(deployment).ToNot(BeNil())
465-
g.Expect(deployment.Spec.Replicas).ToNot(BeNil())
466-
g.Expect(*deployment.Spec.Replicas).To(Equal(int32(7)))
465+
g.Expect(deployment.Spec.Replicas).To(BeNil(),
466+
"Deployment replicas should be nil when HPA exists to allow HPA exclusive control")
467467
}
468468

469469
func TestBuildNginxResourceObjects_Plus(t *testing.T) {

‎internal/controller/provisioner/setter.go‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,23 @@ func serviceSpecSetter(
8383
objectMeta metav1.ObjectMeta,
8484
) controllerutil.MutateFn {
8585
return func() error {
86+
// Preserve all existing annotations from the service, then overwrite with NGF-managed ones
87+
// This allows external controllers (MetalLB, external-dns, cloud providers) to add
88+
// annotations without NGF removing them during reconciliation.
89+
// See: https://github.com/nginx/nginx-gateway-fabric/issues/4012
90+
mergedAnnotations := make(map[string]string)
91+
92+
// Start with all existing annotations
93+
if service.Annotations != nil {
94+
maps.Copy(mergedAnnotations, service.Annotations)
95+
}
96+
97+
// Overwrite with NGF-managed annotations (from Gateway Infrastructure or NginxProxy)
98+
// NGF-managed annotations take precedence
99+
maps.Copy(mergedAnnotations, objectMeta.Annotations)
100+
86101
service.Labels = objectMeta.Labels
87-
service.Annotations = objectMeta.Annotations
102+
service.Annotations = mergedAnnotations
88103
service.Spec = spec
89104
return nil
90105
}

‎internal/controller/provisioner/setter_test.go‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
package provisioner
2+
3+
import (
4+
"testing"
5+
6+
. "github.com/onsi/gomega"
7+
corev1 "k8s.io/api/core/v1"
8+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
9+
)
10+
11+
func TestServiceSpecSetter_PreservesExistingAnnotations(t *testing.T) {
12+
tests := []struct {
13+
existingAnnotations map[string]string
14+
ngfManagedAnnotations map[string]string
15+
expectedAnnotations map[string]string
16+
name string
17+
}{
18+
{
19+
name: "preserves external annotations and adds NGF annotations",
20+
existingAnnotations: map[string]string{
21+
"metallb.universe.tf/address-pool": "production-public-ips",
22+
"external-dns.alpha.kubernetes.io/hostname": "example.com",
23+
},
24+
ngfManagedAnnotations: map[string]string{
25+
"gateway.nginx.org/managed": "true",
26+
},
27+
expectedAnnotations: map[string]string{
28+
"metallb.universe.tf/address-pool": "production-public-ips",
29+
"external-dns.alpha.kubernetes.io/hostname": "example.com",
30+
"gateway.nginx.org/managed": "true",
31+
},
32+
},
33+
{
34+
name: "NGF annotations take precedence",
35+
existingAnnotations: map[string]string{
36+
"custom.annotation": "old-value",
37+
"metallb.universe.tf/address-pool": "staging",
38+
},
39+
ngfManagedAnnotations: map[string]string{
40+
"custom.annotation": "new-value",
41+
},
42+
expectedAnnotations: map[string]string{
43+
"custom.annotation": "new-value",
44+
"metallb.universe.tf/address-pool": "staging",
45+
},
46+
},
47+
{
48+
name: "new service with no existing annotations",
49+
existingAnnotations: nil,
50+
ngfManagedAnnotations: map[string]string{
51+
"gateway.nginx.org/managed": "true",
52+
},
53+
expectedAnnotations: map[string]string{
54+
"gateway.nginx.org/managed": "true",
55+
},
56+
},
57+
{
58+
name: "preserves external annotations with patched annotations",
59+
existingAnnotations: map[string]string{
60+
"metallb.universe.tf/ip-allocated-from-pool": "production",
61+
"metallb.universe.tf/loadBalancerIPs": "192.168.1.100",
62+
},
63+
ngfManagedAnnotations: map[string]string{
64+
"gateway.nginx.org/managed": "true",
65+
"custom.patched.io/example": "patched-value",
66+
"service.beta.kubernetes.io/aws-load-balancer-type": "nlb",
67+
},
68+
expectedAnnotations: map[string]string{
69+
"metallb.universe.tf/ip-allocated-from-pool": "production",
70+
"metallb.universe.tf/loadBalancerIPs": "192.168.1.100",
71+
"gateway.nginx.org/managed": "true",
72+
"custom.patched.io/example": "patched-value",
73+
"service.beta.kubernetes.io/aws-load-balancer-type": "nlb",
74+
},
75+
},
76+
}
77+
78+
for _, tt := range tests {
79+
t.Run(tt.name, func(t *testing.T) {
80+
g := NewWithT(t)
81+
82+
// Create existing service with annotations
83+
existingService := &corev1.Service{
84+
ObjectMeta: metav1.ObjectMeta{
85+
Name: "test-service",
86+
Namespace: "default",
87+
Annotations: tt.existingAnnotations,
88+
},
89+
}
90+
91+
// Create desired object metadata with NGF-managed annotations
92+
desiredMeta := metav1.ObjectMeta{
93+
Labels: map[string]string{
94+
"app": "nginx-gateway",
95+
},
96+
Annotations: tt.ngfManagedAnnotations,
97+
}
98+
99+
// Create desired spec
100+
desiredSpec := corev1.ServiceSpec{
101+
Type: corev1.ServiceTypeLoadBalancer,
102+
Ports: []corev1.ServicePort{
103+
{
104+
Name: "http",
105+
Port: 80,
106+
Protocol: corev1.ProtocolTCP,
107+
},
108+
},
109+
}
110+
111+
// Execute the setter
112+
setter := serviceSpecSetter(existingService, desiredSpec, desiredMeta)
113+
err := setter()
114+
115+
g.Expect(err).ToNot(HaveOccurred())
116+
g.Expect(existingService.Annotations).To(Equal(tt.expectedAnnotations))
117+
g.Expect(existingService.Labels).To(Equal(desiredMeta.Labels))
118+
g.Expect(existingService.Spec).To(Equal(desiredSpec))
119+
})
120+
}
121+
}

0 commit comments

Comments
 (0)