Fix bug where user mounted files were being removed by agent (#4178)

Fix bug where user mounted files were being removed by agent.

Problem: User mounted files are being removed by nginx agent.

Solution: Mark user mounted files as unmanaged so nginx agent doesn't remove them. To do so we use agent's UpdateOverview function to get all files referenced in the nginx conf. We compare that with the user added volumeMounts and mark any files that were user mounted as unmanaged, ensuring nginx agent doesn't remove them.

Testing: Added unit tests and manual testing.

Author: bjee19

internal/controller/handler.go

@@ -241,8 +241,20 @@ func (h *eventHandlerImpl) sendNginxConfig(ctx context.Context, logger logr.Logg
 
 		h.setLatestConfiguration(gw, &cfg)
 
+		vm := []v1.VolumeMount{}
+		if gw.EffectiveNginxProxy != nil &&
+			gw.EffectiveNginxProxy.Kubernetes != nil {
+			if gw.EffectiveNginxProxy.Kubernetes.Deployment != nil {
+				vm = gw.EffectiveNginxProxy.Kubernetes.Deployment.Container.VolumeMounts
+			}
+
+			if gw.EffectiveNginxProxy.Kubernetes.DaemonSet != nil {
+				vm = gw.EffectiveNginxProxy.Kubernetes.DaemonSet.Container.VolumeMounts
+			}
+		}
+
 		deployment.FileLock.Lock()
-		h.updateNginxConf(deployment, cfg)
+		h.updateNginxConf(deployment, cfg, vm)
 		deployment.FileLock.Unlock()
 
 		configErr := deployment.GetLatestConfigError()
@@ -454,9 +466,10 @@ func (h *eventHandlerImpl) parseAndCaptureEvent(ctx context.Context, logger logr
 func (h *eventHandlerImpl) updateNginxConf(
 	deployment *agent.Deployment,
 	conf dataplane.Configuration,
+	volumeMounts []v1.VolumeMount,
 ) {
 	files := h.cfg.generator.Generate(conf)
-	h.cfg.nginxUpdater.UpdateConfig(deployment, files)
+	h.cfg.nginxUpdater.UpdateConfig(deployment, files, volumeMounts)
 
 	// If using NGINX Plus, update upstream servers using the API.
 	if h.cfg.plus {

internal/controller/handler_test.go

@@ -22,6 +22,7 @@ import (
 	gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"
 
 	ngfAPI "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1"
+	"github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha2"
 	"github.com/nginx/nginx-gateway-fabric/v2/internal/controller/config"
 	"github.com/nginx/nginx-gateway-fabric/v2/internal/controller/licensing/licensingfakes"
 	"github.com/nginx/nginx-gateway-fabric/v2/internal/controller/metrics/collectors"
@@ -66,7 +67,7 @@ var _ = Describe("eventHandler", func() {
 		Expect(fakeGenerator.GenerateArgsForCall(0)).Should(Equal(expectedConf))
 
 		Expect(fakeNginxUpdater.UpdateConfigCallCount()).Should(Equal(1))
-		_, files := fakeNginxUpdater.UpdateConfigArgsForCall(0)
+		_, files, _ := fakeNginxUpdater.UpdateConfigArgsForCall(0)
 		Expect(expectedFiles).To(Equal(files))
 
 		Eventually(
@@ -642,6 +643,104 @@ var _ = Describe("eventHandler", func() {
 
 		Expect(handler.GetLatestConfiguration()).To(BeEmpty())
 	})
+
+	It("should process events with volume mounts from Deployment", func() {
+		// Create a gateway with EffectiveNginxProxy containing Deployment VolumeMounts
+		gatewayWithVolumeMounts := &graph.Graph{
+			Gateways: map[types.NamespacedName]*graph.Gateway{
+				{Namespace: "test", Name: "gateway"}: {
+					Valid: true,
+					Source: &gatewayv1.Gateway{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "gateway",
+							Namespace: "test",
+						},
+					},
+					DeploymentName: types.NamespacedName{
+						Namespace: "test",
+						Name:      controller.CreateNginxResourceName("gateway", "nginx"),
+					},
+					EffectiveNginxProxy: &graph.EffectiveNginxProxy{
+						Kubernetes: &v1alpha2.KubernetesSpec{
+							Deployment: &v1alpha2.DeploymentSpec{
+								Container: v1alpha2.ContainerSpec{
+									VolumeMounts: []v1.VolumeMount{
+										{
+											Name:      "test-volume",
+											MountPath: "/etc/test",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		}
+
+		fakeProcessor.ProcessReturns(gatewayWithVolumeMounts)
+
+		e := &events.UpsertEvent{Resource: &gatewayv1.HTTPRoute{}}
+		batch := []interface{}{e}
+
+		handler.HandleEventBatch(context.Background(), logr.Discard(), batch)
+
+		// Verify that UpdateConfig was called with the volume mounts
+		Expect(fakeNginxUpdater.UpdateConfigCallCount()).Should(Equal(1))
+		_, _, volumeMounts := fakeNginxUpdater.UpdateConfigArgsForCall(0)
+		Expect(volumeMounts).To(HaveLen(1))
+		Expect(volumeMounts[0].Name).To(Equal("test-volume"))
+		Expect(volumeMounts[0].MountPath).To(Equal("/etc/test"))
+	})
+
+	It("should process events with volume mounts from DaemonSet", func() {
+		// Create a gateway with EffectiveNginxProxy containing DaemonSet VolumeMounts
+		gatewayWithVolumeMounts := &graph.Graph{
+			Gateways: map[types.NamespacedName]*graph.Gateway{
+				{Namespace: "test", Name: "gateway"}: {
+					Valid: true,
+					Source: &gatewayv1.Gateway{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "gateway",
+							Namespace: "test",
+						},
+					},
+					DeploymentName: types.NamespacedName{
+						Namespace: "test",
+						Name:      controller.CreateNginxResourceName("gateway", "nginx"),
+					},
+					EffectiveNginxProxy: &graph.EffectiveNginxProxy{
+						Kubernetes: &v1alpha2.KubernetesSpec{
+							DaemonSet: &v1alpha2.DaemonSetSpec{
+								Container: v1alpha2.ContainerSpec{
+									VolumeMounts: []v1.VolumeMount{
+										{
+											Name:      "daemon-volume",
+											MountPath: "/var/daemon",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		}
+
+		fakeProcessor.ProcessReturns(gatewayWithVolumeMounts)
+
+		e := &events.UpsertEvent{Resource: &gatewayv1.HTTPRoute{}}
+		batch := []interface{}{e}
+
+		handler.HandleEventBatch(context.Background(), logr.Discard(), batch)
+
+		// Verify that UpdateConfig was called with the volume mounts
+		Expect(fakeNginxUpdater.UpdateConfigCallCount()).Should(Equal(1))
+		_, _, volumeMounts := fakeNginxUpdater.UpdateConfigArgsForCall(0)
+		Expect(volumeMounts).To(HaveLen(1))
+		Expect(volumeMounts[0].Name).To(Equal("daemon-volume"))
+		Expect(volumeMounts[0].MountPath).To(Equal("/var/daemon"))
+	})
 })
 
 var _ = Describe("getGatewayAddresses", func() {

internal/controller/nginx/agent/agent.go

@@ -10,6 +10,7 @@ import (
 	"github.com/go-logr/logr"
 	pb "github.com/nginx/agent/v3/api/grpc/mpi/v1"
 	"google.golang.org/protobuf/types/known/structpb"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -29,7 +30,7 @@ const retryUpstreamTimeout = 5 * time.Second
 
 // NginxUpdater is an interface for updating NGINX using the NGINX agent.
 type NginxUpdater interface {
-	UpdateConfig(deployment *Deployment, files []File)
+	UpdateConfig(deployment *Deployment, files []File, volumeMounts []v1.VolumeMount)
 	UpdateUpstreamServers(deployment *Deployment, conf dataplane.Configuration)
 }
 
@@ -87,8 +88,9 @@ func NewNginxUpdater(
 func (n *NginxUpdaterImpl) UpdateConfig(
 	deployment *Deployment,
 	files []File,
+	volumeMounts []v1.VolumeMount,
 ) {
-	msg := deployment.SetFiles(files)
+	msg := deployment.SetFiles(files, volumeMounts)
 	if msg == nil {
 		n.logger.V(1).Info("No changes to nginx configuration files, not sending to agent")
 		return

internal/controller/nginx/agent/agent_test.go

@@ -9,6 +9,7 @@ import (
 	pb "github.com/nginx/agent/v3/api/grpc/mpi/v1"
 	. "github.com/onsi/gomega"
 	"google.golang.org/protobuf/types/known/structpb"
+	v1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	"github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/agent/broadcast/broadcastfakes"
@@ -63,7 +64,7 @@ func TestUpdateConfig(t *testing.T) {
 				deployment.SetPodErrorStatus("pod1", testErr)
 			}
 
-			updater.UpdateConfig(deployment, []File{file})
+			updater.UpdateConfig(deployment, []File{file}, []v1.VolumeMount{})
 
 			g.Expect(fakeBroadcaster.SendCallCount()).To(Equal(1))
 			fileContents, _ := deployment.GetFile(file.Meta.Name, file.Meta.Hash)
@@ -74,7 +75,7 @@ func TestUpdateConfig(t *testing.T) {
 				// ensure that the error is cleared after the next config is applied
 				deployment.SetPodErrorStatus("pod1", nil)
 				file.Meta.Hash = "5678"
-				updater.UpdateConfig(deployment, []File{file})
+				updater.UpdateConfig(deployment, []File{file}, []v1.VolumeMount{})
 				g.Expect(deployment.GetLatestConfigError()).ToNot(HaveOccurred())
 			} else {
 				g.Expect(deployment.GetLatestConfigError()).ToNot(HaveOccurred())
@@ -105,10 +106,10 @@ func TestUpdateConfig_NoChange(t *testing.T) {
 	}
 
 	// Set the initial files on the deployment
-	deployment.SetFiles([]File{file})
+	deployment.SetFiles([]File{file}, []v1.VolumeMount{})
 
 	// Call UpdateConfig with the same files
-	updater.UpdateConfig(deployment, []File{file})
+	updater.UpdateConfig(deployment, []File{file}, []v1.VolumeMount{})
 
 	// Verify that no new configuration was sent
 	g.Expect(fakeBroadcaster.SendCallCount()).To(Equal(0))

internal/controller/nginx/agent/agentfakes/fake_nginx_updater.go

@@ -342,7 +342,7 @@ func TestSubscribe(t *testing.T) {
 			Contents: []byte("file contents"),
 		},
 	}
-	deployment.SetFiles(files)
+	deployment.SetFiles(files, []v1.VolumeMount{})
 	deployment.SetImageVersion("nginx:v1.0.0")
 
 	initialAction := &pb.NGINXPlusAction{
@@ -488,7 +488,7 @@ func TestSubscribe_Reset(t *testing.T) {
 			Contents: []byte("file contents"),
 		},
 	}
-	deployment.SetFiles(files)
+	deployment.SetFiles(files, []v1.VolumeMount{})
 	deployment.SetImageVersion("nginx:v1.0.0")
 
 	ctx, cancel := createGrpcContextWithCancel()

internal/controller/nginx/agent/command_test.go

@@ -4,10 +4,12 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 	"sync"
 
 	pb "github.com/nginx/agent/v3/api/grpc/mpi/v1"
 	filesHelper "github.com/nginx/agent/v3/pkg/files"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 
 	"github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/agent/broadcast"
@@ -58,6 +60,8 @@ type Deployment struct {
 	fileOverviews    []*pb.File
 	files            []File
 
+	latestFileNames []string
+
 	FileLock sync.RWMutex
 	errLock  sync.RWMutex
 }
@@ -187,16 +191,31 @@ func (d *Deployment) GetFile(name, hash string) ([]byte, string) {
 
 // SetFiles updates the nginx files and fileOverviews for the deployment and returns the message to send.
 // The deployment FileLock MUST already be locked before calling this function.
-func (d *Deployment) SetFiles(files []File) *broadcast.NginxAgentMessage {
+func (d *Deployment) SetFiles(files []File, volumeMounts []v1.VolumeMount) *broadcast.NginxAgentMessage {
 	d.files = files
 
 	fileOverviews := make([]*pb.File, 0, len(files))
 	for _, file := range files {
 		fileOverviews = append(fileOverviews, &pb.File{FileMeta: file.Meta})
 	}
 
+	// To avoid duplicates, use a set for volume ignore files
+	volumeIgnoreSet := make(map[string]struct{}, len(d.latestFileNames))
+	for _, vm := range volumeMounts {
+		for _, f := range d.latestFileNames {
+			if strings.HasPrefix(f, vm.MountPath) {
+				volumeIgnoreSet[f] = struct{}{}
+			}
+		}
+	}
+
+	volumeIgnoreFiles := make([]string, 0, len(volumeIgnoreSet))
+	for f := range volumeIgnoreSet {
+		volumeIgnoreFiles = append(volumeIgnoreFiles, f)
+	}
+
 	// add ignored files to the overview as 'unmanaged' so agent doesn't touch them
-	for _, f := range ignoreFiles {
+	for _, f := range append(ignoreFiles, volumeIgnoreFiles...) {
 		meta := &pb.FileMeta{
 			Name:        f,
 			Permissions: fileMode,