From b0a62a8a1b5066efcd1f509c7036e2c99cf1dc0b Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Mon, 22 Sep 2025 12:10:52 +0100 Subject: [PATCH 1/3] Add nginx image validation on initial config --- internal/controller/handler.go | 7 ++ internal/controller/nginx/agent/command.go | 69 +++++++++-- .../controller/nginx/agent/command_test.go | 114 ++++++++++++------ internal/controller/nginx/agent/deployment.go | 7 ++ internal/controller/provisioner/objects.go | 71 ++++++----- 5 files changed, 185 insertions(+), 83 deletions(-) diff --git a/internal/controller/handler.go b/internal/controller/handler.go index f79e9dc268..39be0f158d 100644 --- a/internal/controller/handler.go +++ b/internal/controller/handler.go @@ -209,6 +209,13 @@ func (h *eventHandlerImpl) sendNginxConfig(ctx context.Context, logger logr.Logg panic("expected deployment, got nil") } + nginxImage, _ := provisioner.DetermineNginxImageName( + gw.EffectiveNginxProxy, + h.cfg.plus, + h.cfg.gatewayPodConfig.Version, + ) + deployment.SetImageVersion(nginxImage) + cfg := dataplane.BuildConfiguration(ctx, logger, gr, gw, h.cfg.serviceResolver, h.cfg.plus) depCtx, getErr := h.getDeploymentContext(ctx) if getErr != nil { diff --git a/internal/controller/nginx/agent/command.go b/internal/controller/nginx/agent/command.go index cc89a47fd5..36b93b4871 100644 --- a/internal/controller/nginx/agent/command.go +++ b/internal/controller/nginx/agent/command.go @@ -86,7 +86,7 @@ func (cs *commandService) CreateConnection( podName := resource.GetContainerInfo().GetHostname() cs.logger.Info(fmt.Sprintf("Creating connection for nginx pod: %s", podName)) - owner, err := cs.getPodOwner(podName) + owner, _, err := cs.getPodOwner(podName) if err != nil { response := &pb.CreateConnectionResponse{ Response: &pb.CommandResponse{ @@ -281,6 +281,17 @@ func (cs *commandService) setInitialConfig( deployment.FileLock.Lock() defer deployment.FileLock.Unlock() + _, pod, err := cs.getPodOwner(conn.PodName) + if err != nil { + cs.logAndSendErrorStatus(deployment, conn, err) + + return grpcStatus.Error(codes.Internal, err.Error()) + } + if err := cs.validatePodImageVersion(pod, deployment.imageVersion); err != nil { + cs.logAndSendErrorStatus(deployment, conn, err) + return grpcStatus.Errorf(codes.FailedPrecondition, "nginx image version validation failed: %s", err.Error()) + } + fileOverviews, configVersion := deployment.GetFileOverviews() cs.logger.Info("Sending initial configuration to agent", "pod", conn.PodName, "configVersion", configVersion) @@ -443,7 +454,7 @@ func buildPlusAPIRequest(action *pb.NGINXPlusAction, instanceID string) *pb.Mana } } -func (cs *commandService) getPodOwner(podName string) (types.NamespacedName, error) { +func (cs *commandService) getPodOwner(podName string) (types.NamespacedName, *v1.Pod, error) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() @@ -452,30 +463,31 @@ func (cs *commandService) getPodOwner(podName string) (types.NamespacedName, err FieldSelector: fields.SelectorFromSet(fields.Set{"metadata.name": podName}), } if err := cs.k8sReader.List(ctx, &pods, listOpts); err != nil { - return types.NamespacedName{}, fmt.Errorf("error listing pods: %w", err) + return types.NamespacedName{}, nil, fmt.Errorf("error listing pods: %w", err) } if len(pods.Items) == 0 { - return types.NamespacedName{}, fmt.Errorf("no pods found with name %q", podName) + return types.NamespacedName{}, nil, fmt.Errorf("no pods found with name %q", podName) } if len(pods.Items) > 1 { - return types.NamespacedName{}, fmt.Errorf("should only be one pod with name %q", podName) + return types.NamespacedName{}, nil, fmt.Errorf("should only be one pod with name %q", podName) } - pod := pods.Items[0] + pod := &pods.Items[0] podOwnerRefs := pod.GetOwnerReferences() if len(podOwnerRefs) != 1 { - return types.NamespacedName{}, fmt.Errorf("expected one owner reference of the nginx Pod, got %d", len(podOwnerRefs)) + tooManyOwnersError := "expected one owner reference of the nginx Pod, got %d" + return types.NamespacedName{}, nil, fmt.Errorf(tooManyOwnersError, len(podOwnerRefs)) } if podOwnerRefs[0].Kind != "ReplicaSet" && podOwnerRefs[0].Kind != "DaemonSet" { err := fmt.Errorf("expected pod owner reference to be ReplicaSet or DaemonSet, got %s", podOwnerRefs[0].Kind) - return types.NamespacedName{}, err + return types.NamespacedName{}, nil, err } if podOwnerRefs[0].Kind == "DaemonSet" { - return types.NamespacedName{Namespace: pod.Namespace, Name: podOwnerRefs[0].Name}, nil + return types.NamespacedName{Namespace: pod.Namespace, Name: podOwnerRefs[0].Name}, pod, nil } var replicaSet appsv1.ReplicaSet @@ -497,16 +509,49 @@ func (cs *commandService) getPodOwner(podName string) (types.NamespacedName, err return true, nil }, ); err != nil { - return types.NamespacedName{}, fmt.Errorf("failed to get nginx Pod's ReplicaSet: %w", replicaSetErr) + return types.NamespacedName{}, nil, fmt.Errorf("failed to get nginx Pod's ReplicaSet: %w", replicaSetErr) } replicaOwnerRefs := replicaSet.GetOwnerReferences() if len(replicaOwnerRefs) != 1 { err := fmt.Errorf("expected one owner reference of the nginx ReplicaSet, got %d", len(replicaOwnerRefs)) - return types.NamespacedName{}, err + return types.NamespacedName{}, nil, err } - return types.NamespacedName{Namespace: pod.Namespace, Name: replicaOwnerRefs[0].Name}, nil + return types.NamespacedName{Namespace: pod.Namespace, Name: replicaOwnerRefs[0].Name}, pod, nil +} + +// validatePodImageVersion checks if the pod's nginx container image version matches the expected version +// from its deployment. Returns an error if versions don't match. +func (cs *commandService) validatePodImageVersion( + pod *v1.Pod, + expectedImage string, +) error { + findNginxContainerImage := func(containers []v1.Container) string { + for _, container := range containers { + if container.Name == "nginx" { + return container.Image + } + } + return "" + } + + // Find the nginx container in the pod + podNginxImage := findNginxContainerImage(pod.Spec.Containers) + if podNginxImage == "" { + return fmt.Errorf("nginx container not found in pod %q", pod.Name) + } + + // Compare images + if podNginxImage != expectedImage { + return fmt.Errorf("nginx image version mismatch: pod has %q but expected %q", podNginxImage, expectedImage) + } + + cs.logger.V(1).Info("Pod nginx image version validated successfully", + "podName", pod.Name, + "image", podNginxImage) + + return nil } // UpdateDataPlaneStatus is called by agent on startup and upon any change in agent metadata, diff --git a/internal/controller/nginx/agent/command_test.go b/internal/controller/nginx/agent/command_test.go index 634cc0eddb..b7365aa1c8 100644 --- a/internal/controller/nginx/agent/command_test.go +++ b/internal/controller/nginx/agent/command_test.go @@ -85,6 +85,48 @@ func createGrpcContextWithCancel() (context.Context, context.CancelFunc) { }), cancel } +func getDefaultPodList() []runtime.Object { + pod := &v1.PodList{ + Items: []v1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-pod", + Namespace: "test", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "ReplicaSet", + Name: "nginx-replicaset", + }, + }, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Image: "nginx:v1.0.0", + Name: "nginx", + }, + }, + }, + }, + }, + } + + replicaSet := &appsv1.ReplicaSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-replicaset", + Namespace: "test", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Deployment", + Name: "nginx-deployment", + }, + }, + }, + } + + return []runtime.Object{pod, replicaSet} +} + func TestCreateConnection(t *testing.T) { t.Parallel() @@ -165,40 +207,9 @@ func TestCreateConnection(t *testing.T) { connTracker := agentgrpcfakes.FakeConnectionsTracker{} var objs []runtime.Object - if test.errString == "" { - pod := &v1.PodList{ - Items: []v1.Pod{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "nginx-pod", - Namespace: "test", - OwnerReferences: []metav1.OwnerReference{ - { - Kind: "ReplicaSet", - Name: "nginx-replicaset", - }, - }, - }, - }, - }, - } - - replicaSet := &appsv1.ReplicaSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "nginx-replicaset", - Namespace: "test", - OwnerReferences: []metav1.OwnerReference{ - { - Kind: "Deployment", - Name: "nginx-deployment", - }, - }, - }, - } - - objs = []runtime.Object{pod, replicaSet} + if test.errString != "error getting pod owner" { + objs = getDefaultPodList() } - fakeClient, err := createFakeK8sClient(objs...) g.Expect(err).ToNot(HaveOccurred()) @@ -298,10 +309,13 @@ func TestSubscribe(t *testing.T) { } connTracker.GetConnectionReturns(conn) + fakeClient, err := createFakeK8sClient(getDefaultPodList()...) + g.Expect(err).ToNot(HaveOccurred()) + store := NewDeploymentStore(&connTracker) cs := newCommandService( logr.Discard(), - fake.NewFakeClient(), + fakeClient, store, &connTracker, status.NewQueue(), @@ -329,6 +343,7 @@ func TestSubscribe(t *testing.T) { }, } deployment.SetFiles(files) + deployment.SetImageVersion("nginx:v1.0.0") initialAction := &pb.NGINXPlusAction{ Action: &pb.NGINXPlusAction_UpdateHttpUpstreamServers{}, @@ -439,11 +454,14 @@ func TestSubscribe_Reset(t *testing.T) { } connTracker.GetConnectionReturns(conn) + fakeClient, err := createFakeK8sClient(getDefaultPodList()...) + g.Expect(err).ToNot(HaveOccurred()) + store := NewDeploymentStore(&connTracker) resetChan := make(chan struct{}) cs := newCommandService( logr.Discard(), - fake.NewFakeClient(), + fakeClient, store, &connTracker, status.NewQueue(), @@ -471,6 +489,7 @@ func TestSubscribe_Reset(t *testing.T) { }, } deployment.SetFiles(files) + deployment.SetImageVersion("nginx:v1.0.0") ctx, cancel := createGrpcContextWithCancel() defer cancel() @@ -590,9 +609,10 @@ func TestSetInitialConfig_Errors(t *testing.T) { t.Parallel() tests := []struct { - name string setup func(msgr *messengerfakes.FakeMessenger, deployment *Deployment) + name string errString string + podList []runtime.Object }{ { name: "error sending initial config", @@ -652,6 +672,13 @@ func TestSetInitialConfig_Errors(t *testing.T) { }, errString: "api apply error", }, + { + name: "error validating nginx version", + setup: func(_ *messengerfakes.FakeMessenger, deployment *Deployment) { + deployment.SetImageVersion("nginx:v2.0.0") + }, + errString: "nginx image version mismatch: pod has \"nginx:v1.0.0\" but expected \"nginx:v2.0.0\"", + }, } for _, test := range tests { @@ -662,9 +689,17 @@ func TestSetInitialConfig_Errors(t *testing.T) { connTracker := agentgrpcfakes.FakeConnectionsTracker{} msgr := &messengerfakes.FakeMessenger{} + podList := test.podList + if len(podList) == 0 { + podList = getDefaultPodList() + } + + fakeClient, err := createFakeK8sClient(podList...) + g.Expect(err).ToNot(HaveOccurred()) + cs := newCommandService( logr.Discard(), - fake.NewFakeClient(), + fakeClient, NewDeploymentStore(&connTracker), &connTracker, status.NewQueue(), @@ -678,12 +713,13 @@ func TestSetInitialConfig_Errors(t *testing.T) { } deployment := newDeployment(&broadcastfakes.FakeBroadcaster{}) + deployment.SetImageVersion("nginx:v1.0.0") if test.setup != nil { test.setup(msgr, deployment) } - err := cs.setInitialConfig(context.Background(), deployment, conn, msgr) + err = cs.setInitialConfig(context.Background(), deployment, conn, msgr) g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring(test.errString)) @@ -882,7 +918,7 @@ func TestGetPodOwner(t *testing.T) { nil, ) - owner, err := cs.getPodOwner(test.podName) + owner, _, err := cs.getPodOwner(test.podName) if test.errString != "" { g.Expect(err).To(HaveOccurred()) diff --git a/internal/controller/nginx/agent/deployment.go b/internal/controller/nginx/agent/deployment.go index 7e9aa3aba5..242d430811 100644 --- a/internal/controller/nginx/agent/deployment.go +++ b/internal/controller/nginx/agent/deployment.go @@ -40,6 +40,8 @@ type Deployment struct { broadcaster broadcast.Broadcaster + imageVersion string + configVersion string // error that is set if a ConfigApply call failed for a Pod. This is needed // because if subsequent upstream API calls are made within the same update event, @@ -73,6 +75,11 @@ func (d *Deployment) GetBroadcaster() broadcast.Broadcaster { return d.broadcaster } +// GetBroadcaster returns the deployment's broadcaster. +func (d *Deployment) SetImageVersion(imageVersion string) { + d.imageVersion = imageVersion +} + // SetLatestConfigError sets the latest config apply error for the deployment. func (d *Deployment) SetLatestConfigError(err error) { d.errLock.Lock() diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go index 475a3e7319..c9fb112e36 100644 --- a/internal/controller/provisioner/objects.go +++ b/internal/controller/provisioner/objects.go @@ -1123,38 +1123,7 @@ func (p *NginxProvisioner) buildNginxPodTemplateSpec( } func (p *NginxProvisioner) buildImage(nProxyCfg *graph.EffectiveNginxProxy) (string, corev1.PullPolicy) { - image := defaultNginxImagePath - if p.cfg.Plus { - image = defaultNginxPlusImagePath - } - tag := p.cfg.GatewayPodConfig.Version - pullPolicy := defaultImagePullPolicy - - getImageAndPullPolicy := func(container ngfAPIv1alpha2.ContainerSpec) (string, string, corev1.PullPolicy) { - if container.Image != nil { - if container.Image.Repository != nil { - image = *container.Image.Repository - } - if container.Image.Tag != nil { - tag = *container.Image.Tag - } - if container.Image.PullPolicy != nil { - pullPolicy = corev1.PullPolicy(*container.Image.PullPolicy) - } - } - - return image, tag, pullPolicy - } - - if nProxyCfg != nil && nProxyCfg.Kubernetes != nil { - if nProxyCfg.Kubernetes.Deployment != nil { - image, tag, pullPolicy = getImageAndPullPolicy(nProxyCfg.Kubernetes.Deployment.Container) - } else if nProxyCfg.Kubernetes.DaemonSet != nil { - image, tag, pullPolicy = getImageAndPullPolicy(nProxyCfg.Kubernetes.DaemonSet.Container) - } - } - - return fmt.Sprintf("%s:%s", image, tag), pullPolicy + return DetermineNginxImageName(nProxyCfg, p.cfg.Plus, p.cfg.GatewayPodConfig.Version) } func buildNginxDeploymentHPA( @@ -1395,3 +1364,41 @@ func (p *NginxProvisioner) buildReadinessProbe(nProxyCfg *graph.EffectiveNginxPr return probe } + +func DetermineNginxImageName( + nProxyCfg *graph.EffectiveNginxProxy, + isPlus bool, version string, +) (string, corev1.PullPolicy) { + image := defaultNginxImagePath + if isPlus { + image = defaultNginxPlusImagePath + } + tag := version + pullPolicy := defaultImagePullPolicy + + getImageAndPullPolicy := func(container ngfAPIv1alpha2.ContainerSpec) (string, string, corev1.PullPolicy) { + if container.Image != nil { + if container.Image.Repository != nil { + image = *container.Image.Repository + } + if container.Image.Tag != nil { + tag = *container.Image.Tag + } + if container.Image.PullPolicy != nil { + pullPolicy = corev1.PullPolicy(*container.Image.PullPolicy) + } + } + + return image, tag, pullPolicy + } + + if nProxyCfg != nil && nProxyCfg.Kubernetes != nil { + if nProxyCfg.Kubernetes.Deployment != nil { + image, tag, pullPolicy = getImageAndPullPolicy(nProxyCfg.Kubernetes.Deployment.Container) + } else if nProxyCfg.Kubernetes.DaemonSet != nil { + image, tag, pullPolicy = getImageAndPullPolicy(nProxyCfg.Kubernetes.DaemonSet.Container) + } + } + + return fmt.Sprintf("%s:%s", image, tag), pullPolicy +} From ac38be6985b28b4b3e2b7094fe9b0ebbc465d18a Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Tue, 23 Sep 2025 16:50:30 +0100 Subject: [PATCH 2/3] Review feedback --- internal/controller/nginx/agent/command.go | 22 ++++++++++--------- internal/controller/nginx/agent/deployment.go | 5 ++++- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/internal/controller/nginx/agent/command.go b/internal/controller/nginx/agent/command.go index 36b93b4871..3d338465e9 100644 --- a/internal/controller/nginx/agent/command.go +++ b/internal/controller/nginx/agent/command.go @@ -521,23 +521,25 @@ func (cs *commandService) getPodOwner(podName string) (types.NamespacedName, *v1 return types.NamespacedName{Namespace: pod.Namespace, Name: replicaOwnerRefs[0].Name}, pod, nil } +// findContainerImage returns the image of the first container with the given name. +// Returns empty string if container is not found. +func findContainerImage(containers []v1.Container, containerName string) string { + for _, container := range containers { + if container.Name == containerName { + return container.Image + } + } + return "" +} + // validatePodImageVersion checks if the pod's nginx container image version matches the expected version // from its deployment. Returns an error if versions don't match. func (cs *commandService) validatePodImageVersion( pod *v1.Pod, expectedImage string, ) error { - findNginxContainerImage := func(containers []v1.Container) string { - for _, container := range containers { - if container.Name == "nginx" { - return container.Image - } - } - return "" - } - // Find the nginx container in the pod - podNginxImage := findNginxContainerImage(pod.Spec.Containers) + podNginxImage := findContainerImage(pod.Spec.Containers, "nginx") if podNginxImage == "" { return fmt.Errorf("nginx container not found in pod %q", pod.Name) } diff --git a/internal/controller/nginx/agent/deployment.go b/internal/controller/nginx/agent/deployment.go index 242d430811..e994b853ca 100644 --- a/internal/controller/nginx/agent/deployment.go +++ b/internal/controller/nginx/agent/deployment.go @@ -75,8 +75,11 @@ func (d *Deployment) GetBroadcaster() broadcast.Broadcaster { return d.broadcaster } -// GetBroadcaster returns the deployment's broadcaster. +// SetImageVersion sets the deployment's image version. func (d *Deployment) SetImageVersion(imageVersion string) { + d.errLock.Lock() + defer d.errLock.Unlock() + d.imageVersion = imageVersion } From 86734c1f40defc4b8bdb969c42ca0fc9c62ed532 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Tue, 23 Sep 2025 18:40:42 +0100 Subject: [PATCH 3/3] Remove unnecessary function, use file lock --- internal/controller/nginx/agent/command.go | 21 +++++++------------ internal/controller/nginx/agent/deployment.go | 4 ++-- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/internal/controller/nginx/agent/command.go b/internal/controller/nginx/agent/command.go index 3d338465e9..f3095f7398 100644 --- a/internal/controller/nginx/agent/command.go +++ b/internal/controller/nginx/agent/command.go @@ -521,25 +521,20 @@ func (cs *commandService) getPodOwner(podName string) (types.NamespacedName, *v1 return types.NamespacedName{Namespace: pod.Namespace, Name: replicaOwnerRefs[0].Name}, pod, nil } -// findContainerImage returns the image of the first container with the given name. -// Returns empty string if container is not found. -func findContainerImage(containers []v1.Container, containerName string) string { - for _, container := range containers { - if container.Name == containerName { - return container.Image - } - } - return "" -} - // validatePodImageVersion checks if the pod's nginx container image version matches the expected version // from its deployment. Returns an error if versions don't match. func (cs *commandService) validatePodImageVersion( pod *v1.Pod, expectedImage string, ) error { - // Find the nginx container in the pod - podNginxImage := findContainerImage(pod.Spec.Containers, "nginx") + var podNginxImage string + + for _, container := range pod.Spec.Containers { + if container.Name == "nginx" { + podNginxImage = container.Image + break + } + } if podNginxImage == "" { return fmt.Errorf("nginx container not found in pod %q", pod.Name) } diff --git a/internal/controller/nginx/agent/deployment.go b/internal/controller/nginx/agent/deployment.go index e994b853ca..93f8b692cf 100644 --- a/internal/controller/nginx/agent/deployment.go +++ b/internal/controller/nginx/agent/deployment.go @@ -77,8 +77,8 @@ func (d *Deployment) GetBroadcaster() broadcast.Broadcaster { // SetImageVersion sets the deployment's image version. func (d *Deployment) SetImageVersion(imageVersion string) { - d.errLock.Lock() - defer d.errLock.Unlock() + d.FileLock.Lock() + defer d.FileLock.Unlock() d.imageVersion = imageVersion }