Skip to content

Commit ecceb93

Browse files
shaun-nxshaun-nx
authored
Merge branch 'main' into proposal/authentication-filter
2 parents 24966b8 + 96032ac commit ecceb93

File tree

2 files changed

+194
-80
lines changed

2 files changed

+194
-80
lines changed

‎internal/controller/provisioner/objects.go‎

Lines changed: 56 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -686,33 +686,69 @@ func (p *NginxProvisioner) buildNginxDeployment(
686686
}
687687
}
688688

689-
var replicas *int32
690-
if deploymentCfg.Replicas != nil {
691-
replicas = deploymentCfg.Replicas
692-
}
693-
694-
if isAutoscalingEnabled(&deploymentCfg) {
695-
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
696-
defer cancel()
697-
698-
hpa := &autoscalingv2.HorizontalPodAutoscaler{}
699-
err := p.k8sClient.Get(ctx, types.NamespacedName{
700-
Namespace: objectMeta.Namespace,
701-
Name: objectMeta.Name,
702-
}, hpa)
703-
if err == nil && hpa.Status.DesiredReplicas > 0 {
704-
// overwrite with HPA's desiredReplicas
705-
replicas = helpers.GetPointer(hpa.Status.DesiredReplicas)
706-
}
707-
}
708-
689+
// Determine replica count based on HPA status
690+
replicas := p.determineReplicas(objectMeta, deploymentCfg)
709691
if replicas != nil {
710692
deployment.Spec.Replicas = replicas
711693
}
712694

713695
return deployment, nil
714696
}
715697

698+
// determineReplicas determines the appropriate replica count for a deployment based on HPA status.
699+
//
700+
// HPA Replicas Management Strategy:
701+
//
702+
// When an HPA is managing a deployment, we must read the current deployment's replicas
703+
// from the cluster and use that value, rather than trying to set our own value or read
704+
// from HPA.Status.DesiredReplicas (which is eventually consistent and stale).
705+
//
706+
// Why we can't use HPA.Status.DesiredReplicas:
707+
// - HPA.Status updates lag behind Deployment.Spec.Replicas changes
708+
// - When HPA scales down: HPA writes Deployment.Spec → then updates its own Status
709+
// - If we read Status during this window, we get the OLD value and overwrite HPA's new value
710+
// - This creates a race condition causing pod churn
711+
//
712+
// Our approach:
713+
// - When HPA exists: Read current deployment replicas from cluster and use that
714+
// - When HPA doesn't exist yet: Set replicas for initial deployment creation
715+
// - When HPA exists but Deployment doesn't exist yet: Set replicas for initial deployment creation
716+
// - When HPA is disabled: Set replicas normally.
717+
func (p *NginxProvisioner) determineReplicas(
718+
objectMeta metav1.ObjectMeta,
719+
deploymentCfg ngfAPIv1alpha2.DeploymentSpec,
720+
) *int32 {
721+
replicas := deploymentCfg.Replicas
722+
723+
if !isAutoscalingEnabled(&deploymentCfg) {
724+
return replicas
725+
}
726+
727+
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
728+
defer cancel()
729+
730+
hpa := &autoscalingv2.HorizontalPodAutoscaler{}
731+
err := p.k8sClient.Get(ctx, types.NamespacedName{
732+
Namespace: objectMeta.Namespace,
733+
Name: objectMeta.Name,
734+
}, hpa)
735+
if err != nil {
736+
return replicas
737+
}
738+
739+
existingDeployment := &appsv1.Deployment{}
740+
err = p.k8sClient.Get(ctx, types.NamespacedName{
741+
Namespace: objectMeta.Namespace,
742+
Name: objectMeta.Name,
743+
}, existingDeployment)
744+
745+
if err == nil && existingDeployment.Spec.Replicas != nil {
746+
replicas = existingDeployment.Spec.Replicas
747+
}
748+
749+
return replicas
750+
}
751+
716752
// applyPatches applies the provided patches to the given object.
717753
func applyPatches(obj client.Object, patches []ngfAPIv1alpha2.Patch) error {
718754
if len(patches) == 0 {

‎internal/controller/provisioner/objects_test.go‎

Lines changed: 138 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -392,78 +392,156 @@ func TestBuildNginxResourceObjects_NginxProxyConfig(t *testing.T) {
392392

393393
func TestBuildNginxResourceObjects_DeploymentReplicasFromHPA(t *testing.T) {
394394
t.Parallel()
395-
g := NewWithT(t)
396395

397-
// Create a fake HPA with status.desiredReplicas set
398-
hpa := &autoscalingv2.HorizontalPodAutoscaler{
399-
ObjectMeta: metav1.ObjectMeta{
400-
Name: "gw-nginx",
401-
Namespace: "default",
396+
tests := []struct {
397+
currentReplicas *int32
398+
configReplicas *int32
399+
name string
400+
description string
401+
expectedValue int32
402+
hpaExists bool
403+
deploymentExists bool
404+
expectedNil bool
405+
}{
406+
{
407+
name: "HPA exists - use current deployment replicas",
408+
hpaExists: true,
409+
deploymentExists: true,
410+
currentReplicas: helpers.GetPointer(int32(8)),
411+
configReplicas: helpers.GetPointer(int32(5)),
412+
expectedNil: false,
413+
expectedValue: 8,
414+
description: "When HPA exists, read current deployment replicas (set by HPA)",
402415
},
403-
Status: autoscalingv2.HorizontalPodAutoscalerStatus{
404-
DesiredReplicas: 7,
416+
{
417+
name: "HPA does not exist - use configured replicas",
418+
hpaExists: false,
419+
deploymentExists: false,
420+
configReplicas: helpers.GetPointer(int32(3)),
421+
expectedNil: false,
422+
expectedValue: 3,
423+
description: "When HPA doesn't exist yet (initial creation), use configured replicas",
405424
},
406-
}
407-
408-
agentTLSSecret := &corev1.Secret{
409-
ObjectMeta: metav1.ObjectMeta{
410-
Name: agentTLSTestSecretName,
411-
Namespace: ngfNamespace,
425+
{
426+
name: "HPA enabled but doesn't exist, no configured replicas",
427+
hpaExists: false,
428+
deploymentExists: false,
429+
configReplicas: nil,
430+
expectedNil: true,
431+
description: "When HPA enabled but doesn't exist and no replicas configured, don't set replicas",
412432
},
413-
Data: map[string][]byte{"tls.crt": []byte("tls")},
414433
}
415434

416-
fakeClient := fake.NewFakeClient(agentTLSSecret, hpa)
435+
for _, tc := range tests {
436+
t.Run(tc.name, func(t *testing.T) {
437+
t.Parallel()
438+
g := NewWithT(t)
417439

418-
provisioner := &NginxProvisioner{
419-
cfg: Config{
420-
GatewayPodConfig: &config.GatewayPodConfig{
421-
Namespace: ngfNamespace,
422-
Version: "1.0.0",
423-
Image: "ngf-image",
424-
},
425-
AgentTLSSecretName: agentTLSTestSecretName,
426-
},
427-
baseLabelSelector: metav1.LabelSelector{
428-
MatchLabels: map[string]string{"app": "nginx"},
429-
},
430-
k8sClient: fakeClient,
431-
}
440+
agentTLSSecret := &corev1.Secret{
441+
ObjectMeta: metav1.ObjectMeta{
442+
Name: agentTLSTestSecretName,
443+
Namespace: ngfNamespace,
444+
},
445+
Data: map[string][]byte{"tls.crt": []byte("tls")},
446+
}
432447

433-
gateway := &gatewayv1.Gateway{
434-
ObjectMeta: metav1.ObjectMeta{
435-
Name: "gw",
436-
Namespace: "default",
437-
},
438-
Spec: gatewayv1.GatewaySpec{
439-
Listeners: []gatewayv1.Listener{{Port: 80}},
440-
},
441-
}
448+
var fakeClient client.Client
449+
switch {
450+
case tc.hpaExists && tc.deploymentExists:
451+
// Create a fake HPA and existing deployment
452+
hpa := &autoscalingv2.HorizontalPodAutoscaler{
453+
ObjectMeta: metav1.ObjectMeta{
454+
Name: "gw-nginx",
455+
Namespace: "default",
456+
},
457+
Status: autoscalingv2.HorizontalPodAutoscalerStatus{
458+
DesiredReplicas: 7,
459+
},
460+
}
461+
existingDeployment := &appsv1.Deployment{
462+
ObjectMeta: metav1.ObjectMeta{
463+
Name: "gw-nginx",
464+
Namespace: "default",
465+
},
466+
Spec: appsv1.DeploymentSpec{
467+
Replicas: tc.currentReplicas,
468+
Selector: &metav1.LabelSelector{
469+
MatchLabels: map[string]string{"app": "nginx"},
470+
},
471+
},
472+
}
473+
fakeClient = fake.NewFakeClient(agentTLSSecret, hpa, existingDeployment)
474+
case tc.hpaExists:
475+
hpa := &autoscalingv2.HorizontalPodAutoscaler{
476+
ObjectMeta: metav1.ObjectMeta{
477+
Name: "gw-nginx",
478+
Namespace: "default",
479+
},
480+
Status: autoscalingv2.HorizontalPodAutoscalerStatus{
481+
DesiredReplicas: 7,
482+
},
483+
}
484+
fakeClient = fake.NewFakeClient(agentTLSSecret, hpa)
485+
default:
486+
fakeClient = fake.NewFakeClient(agentTLSSecret)
487+
}
442488

443-
resourceName := "gw-nginx"
444-
nProxyCfg := &graph.EffectiveNginxProxy{
445-
Kubernetes: &ngfAPIv1alpha2.KubernetesSpec{
446-
Deployment: &ngfAPIv1alpha2.DeploymentSpec{
447-
Replicas: nil, // Should be overridden by HPA
448-
Autoscaling: &ngfAPIv1alpha2.AutoscalingSpec{Enable: true},
449-
},
450-
},
451-
}
489+
provisioner := &NginxProvisioner{
490+
cfg: Config{
491+
GatewayPodConfig: &config.GatewayPodConfig{
492+
Namespace: ngfNamespace,
493+
Version: "1.0.0",
494+
Image: "ngf-image",
495+
},
496+
AgentTLSSecretName: agentTLSTestSecretName,
497+
},
498+
baseLabelSelector: metav1.LabelSelector{
499+
MatchLabels: map[string]string{"app": "nginx"},
500+
},
501+
k8sClient: fakeClient,
502+
}
452503

453-
objects, err := provisioner.buildNginxResourceObjects(resourceName, gateway, nProxyCfg)
454-
g.Expect(err).ToNot(HaveOccurred())
504+
gateway := &gatewayv1.Gateway{
505+
ObjectMeta: metav1.ObjectMeta{
506+
Name: "gw",
507+
Namespace: "default",
508+
},
509+
Spec: gatewayv1.GatewaySpec{
510+
Listeners: []gatewayv1.Listener{{Port: 80}},
511+
},
512+
}
455513

456-
// Find the deployment object
457-
var deployment *appsv1.Deployment
458-
for _, obj := range objects {
459-
if d, ok := obj.(*appsv1.Deployment); ok {
460-
deployment = d
461-
break
462-
}
514+
resourceName := "gw-nginx"
515+
nProxyCfg := &graph.EffectiveNginxProxy{
516+
Kubernetes: &ngfAPIv1alpha2.KubernetesSpec{
517+
Deployment: &ngfAPIv1alpha2.DeploymentSpec{
518+
Replicas: tc.configReplicas,
519+
Autoscaling: &ngfAPIv1alpha2.AutoscalingSpec{Enable: true},
520+
},
521+
},
522+
}
523+
524+
objects, err := provisioner.buildNginxResourceObjects(resourceName, gateway, nProxyCfg)
525+
g.Expect(err).ToNot(HaveOccurred())
526+
527+
// Find the deployment object
528+
var deployment *appsv1.Deployment
529+
for _, obj := range objects {
530+
if d, ok := obj.(*appsv1.Deployment); ok {
531+
deployment = d
532+
break
533+
}
534+
}
535+
g.Expect(deployment).ToNot(BeNil())
536+
537+
if tc.expectedNil {
538+
g.Expect(deployment.Spec.Replicas).To(BeNil(), tc.description)
539+
} else {
540+
g.Expect(deployment.Spec.Replicas).ToNot(BeNil(), tc.description)
541+
g.Expect(*deployment.Spec.Replicas).To(Equal(tc.expectedValue), tc.description)
542+
}
543+
})
463544
}
464-
g.Expect(deployment).ToNot(BeNil())
465-
g.Expect(deployment.Spec.Replicas).ToNot(BeNil())
466-
g.Expect(*deployment.Spec.Replicas).To(Equal(int32(7)))
467545
}
468546

469547
func TestBuildNginxResourceObjects_Plus(t *testing.T) {

0 commit comments

Comments
 (0)