diff --git a/internal/events/handler.go b/internal/events/handler.go index f324d09658..af07d5c9ed 100644 --- a/internal/events/handler.go +++ b/internal/events/handler.go @@ -5,9 +5,6 @@ import ( "fmt" "github.com/go-logr/logr" - apiv1 "k8s.io/api/core/v1" - discoveryV1 "k8s.io/api/discovery/v1" - "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/config" "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file" @@ -15,7 +12,6 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/dataplane" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/resolver" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets" "github.com/nginxinc/nginx-kubernetes-gateway/internal/status" ) @@ -32,10 +28,6 @@ type EventHandler interface { type EventHandlerConfig struct { // Processor is the state ChangeProcessor. Processor state.ChangeProcessor - // SecretStore is the state SecretStore. - SecretStore secrets.SecretStore - // SecretMemoryManager is the state SecretMemoryManager. - SecretMemoryManager secrets.SecretDiskMemoryManager // ServiceResolver resolves Services to Endpoints. ServiceResolver resolver.ServiceResolver // Generator is the nginx config Generator. @@ -69,9 +61,9 @@ func (h *EventHandlerImpl) HandleEventBatch(ctx context.Context, batch EventBatc for _, event := range batch { switch e := event.(type) { case *UpsertEvent: - h.propagateUpsert(e) + h.cfg.Processor.CaptureUpsertChange(e.Resource) case *DeleteEvent: - h.propagateDelete(e) + h.cfg.Processor.CaptureDeleteChange(e.Type, e.NamespacedName) default: panic(fmt.Errorf("unknown event type %T", e)) } @@ -96,74 +88,15 @@ func (h *EventHandlerImpl) HandleEventBatch(ctx context.Context, batch EventBatc } func (h *EventHandlerImpl) updateNginx(ctx context.Context, conf dataplane.Configuration) error { - // Write all secrets (nuke and pave). - // This will remove all secrets in the secrets directory before writing the requested secrets. - // FIXME(kate-osborn): We may want to rethink this approach in the future and write and remove secrets individually. - // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/561 - err := h.cfg.SecretMemoryManager.WriteAllRequestedSecrets() - if err != nil { - return err - } + files := h.cfg.Generator.Generate(conf) - cfg := h.cfg.Generator.Generate(conf) - - // For now, we keep all http servers and upstreams in one config file. - // We might rethink that. For example, we can write each server to its file - // or group servers in some way. - err = h.cfg.NginxFileMgr.WriteHTTPConfig("http", cfg) - if err != nil { - return err + if err := h.cfg.NginxFileMgr.ReplaceFiles(files); err != nil { + return fmt.Errorf("failed to replace NGINX configuration files: %w", err) } - return h.cfg.NginxRuntimeMgr.Reload(ctx) -} - -func (h *EventHandlerImpl) propagateUpsert(e *UpsertEvent) { - switch r := e.Resource.(type) { - case *v1beta1.GatewayClass: - h.cfg.Processor.CaptureUpsertChange(r) - case *v1beta1.Gateway: - h.cfg.Processor.CaptureUpsertChange(r) - case *v1beta1.HTTPRoute: - h.cfg.Processor.CaptureUpsertChange(r) - case *v1beta1.ReferenceGrant: - h.cfg.Processor.CaptureUpsertChange(r) - case *apiv1.Service: - h.cfg.Processor.CaptureUpsertChange(r) - case *apiv1.Namespace: - h.cfg.Processor.CaptureUpsertChange(r) - case *apiv1.Secret: - // FIXME(kate-osborn): need to handle certificate rotation - // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/553 - h.cfg.SecretStore.Upsert(r) - case *discoveryV1.EndpointSlice: - h.cfg.Processor.CaptureUpsertChange(r) - default: - panic(fmt.Errorf("unknown resource type %T", e.Resource)) + if err := h.cfg.NginxRuntimeMgr.Reload(ctx); err != nil { + return fmt.Errorf("failed to reload NGINX: %w", err) } -} -func (h *EventHandlerImpl) propagateDelete(e *DeleteEvent) { - switch e.Type.(type) { - case *v1beta1.GatewayClass: - h.cfg.Processor.CaptureDeleteChange(e.Type, e.NamespacedName) - case *v1beta1.Gateway: - h.cfg.Processor.CaptureDeleteChange(e.Type, e.NamespacedName) - case *v1beta1.HTTPRoute: - h.cfg.Processor.CaptureDeleteChange(e.Type, e.NamespacedName) - case *v1beta1.ReferenceGrant: - h.cfg.Processor.CaptureDeleteChange(e.Type, e.NamespacedName) - case *apiv1.Service: - h.cfg.Processor.CaptureDeleteChange(e.Type, e.NamespacedName) - case *apiv1.Namespace: - h.cfg.Processor.CaptureDeleteChange(e.Type, e.NamespacedName) - case *apiv1.Secret: - // FIXME(kate-osborn): make sure that affected servers are updated - // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/553 - h.cfg.SecretStore.Delete(e.NamespacedName) - case *discoveryV1.EndpointSlice: - h.cfg.Processor.CaptureDeleteChange(e.Type, e.NamespacedName) - default: - panic(fmt.Errorf("unknown resource type %T", e.Type)) - } + return nil } diff --git a/internal/events/handler_test.go b/internal/events/handler_test.go index c0218b09b0..7c0e309fe2 100644 --- a/internal/events/handler_test.go +++ b/internal/events/handler_test.go @@ -2,64 +2,43 @@ package events_test import ( "context" - "fmt" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - apiv1 "k8s.io/api/core/v1" - discoveryV1 "k8s.io/api/discovery/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/events" "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/config/configfakes" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file" "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file/filefakes" "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/runtime/runtimefakes" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/dataplane" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/graph" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets/secretsfakes" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/statefakes" "github.com/nginxinc/nginx-kubernetes-gateway/internal/status/statusfakes" ) -type unsupportedResource struct { - metav1.ObjectMeta -} - -func (r *unsupportedResource) GetObjectKind() schema.ObjectKind { - return nil -} - -func (r *unsupportedResource) DeepCopyObject() runtime.Object { - return nil -} - var _ = Describe("EventHandler", func() { var ( - handler *events.EventHandlerImpl - fakeProcessor *statefakes.FakeChangeProcessor - fakeSecretStore *secretsfakes.FakeSecretStore - fakeSecretMemoryManager *secretsfakes.FakeSecretDiskMemoryManager - fakeGenerator *configfakes.FakeGenerator - fakeNginxFileMgr *filefakes.FakeManager - fakeNginxRuntimeMgr *runtimefakes.FakeManager - fakeStatusUpdater *statusfakes.FakeUpdater + handler *events.EventHandlerImpl + fakeProcessor *statefakes.FakeChangeProcessor + fakeGenerator *configfakes.FakeGenerator + fakeNginxFileMgr *filefakes.FakeManager + fakeNginxRuntimeMgr *runtimefakes.FakeManager + fakeStatusUpdater *statusfakes.FakeUpdater ) - expectReconfig := func(expectedConf dataplane.Configuration, expectedCfg []byte) { + expectReconfig := func(expectedConf dataplane.Configuration, expectedFiles []file.File) { Expect(fakeProcessor.ProcessCallCount()).Should(Equal(1)) Expect(fakeGenerator.GenerateCallCount()).Should(Equal(1)) Expect(fakeGenerator.GenerateArgsForCall(0)).Should(Equal(expectedConf)) - Expect(fakeNginxFileMgr.WriteHTTPConfigCallCount()).Should(Equal(1)) - name, cfg := fakeNginxFileMgr.WriteHTTPConfigArgsForCall(0) - Expect(name).Should(Equal("http")) - Expect(cfg).Should(Equal(expectedCfg)) + Expect(fakeNginxFileMgr.ReplaceFilesCallCount()).Should(Equal(1)) + files := fakeNginxFileMgr.ReplaceFilesArgsForCall(0) + Expect(files).Should(Equal(expectedFiles)) Expect(fakeNginxRuntimeMgr.ReloadCallCount()).Should(Equal(1)) @@ -68,249 +47,99 @@ var _ = Describe("EventHandler", func() { BeforeEach(func() { fakeProcessor = &statefakes.FakeChangeProcessor{} - fakeSecretMemoryManager = &secretsfakes.FakeSecretDiskMemoryManager{} - fakeSecretStore = &secretsfakes.FakeSecretStore{} fakeGenerator = &configfakes.FakeGenerator{} fakeNginxFileMgr = &filefakes.FakeManager{} fakeNginxRuntimeMgr = &runtimefakes.FakeManager{} fakeStatusUpdater = &statusfakes.FakeUpdater{} handler = events.NewEventHandlerImpl(events.EventHandlerConfig{ - Processor: fakeProcessor, - SecretStore: fakeSecretStore, - SecretMemoryManager: fakeSecretMemoryManager, - Generator: fakeGenerator, - Logger: zap.New(), - NginxFileMgr: fakeNginxFileMgr, - NginxRuntimeMgr: fakeNginxRuntimeMgr, - StatusUpdater: fakeStatusUpdater, + Processor: fakeProcessor, + Generator: fakeGenerator, + Logger: zap.New(), + NginxFileMgr: fakeNginxFileMgr, + NginxRuntimeMgr: fakeNginxRuntimeMgr, + StatusUpdater: fakeStatusUpdater, }) }) Describe("Process the Gateway API resources events", func() { - DescribeTable( - "A batch with one event", - func(e interface{}) { - changed := true - fakeProcessor.ProcessReturns(changed, &graph.Graph{}) - - fakeCfg := []byte("fake") - fakeGenerator.GenerateReturns(fakeCfg) - - batch := []interface{}{e} - - handler.HandleEventBatch(context.TODO(), batch) - - // Check that the events were captured - switch typedEvent := e.(type) { - case *events.UpsertEvent: - Expect(fakeProcessor.CaptureUpsertChangeCallCount()).Should(Equal(1)) - Expect(fakeProcessor.CaptureUpsertChangeArgsForCall(0)).Should(Equal(typedEvent.Resource)) - case *events.DeleteEvent: - Expect(fakeProcessor.CaptureDeleteChangeCallCount()).Should(Equal(1)) - passedObj, passedNsName := fakeProcessor.CaptureDeleteChangeArgsForCall(0) - Expect(passedObj).Should(Equal(typedEvent.Type)) - Expect(passedNsName).Should(Equal(typedEvent.NamespacedName)) - default: - Fail(fmt.Sprintf("unsupported event type %T", e)) - } - - // Check that a reconfig happened - expectReconfig(dataplane.Configuration{}, fakeCfg) + fakeCfgFiles := []file.File{ + { + Type: file.TypeRegular, + Path: "test.conf", }, - Entry( - "HTTPRoute upsert", - &events.UpsertEvent{Resource: &v1beta1.HTTPRoute{}}, - ), - Entry( - "Gateway upsert", - &events.UpsertEvent{Resource: &v1beta1.Gateway{}}, - ), - Entry( - "GatewayClass upsert", - &events.UpsertEvent{Resource: &v1beta1.GatewayClass{}}, - ), - Entry( - "Service upsert", - &events.UpsertEvent{Resource: &apiv1.Service{}}, - ), - Entry( - "EndpointSlice upsert", - &events.UpsertEvent{Resource: &discoveryV1.EndpointSlice{}}, - ), - - Entry( - "HTTPRoute delete", - &events.DeleteEvent{ - Type: &v1beta1.HTTPRoute{}, - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route"}, - }, - ), - Entry( - "Gateway delete", - &events.DeleteEvent{ - Type: &v1beta1.Gateway{}, - NamespacedName: types.NamespacedName{Namespace: "test", Name: "gateway"}, - }, - ), - Entry( - "GatewayClass delete", - &events.DeleteEvent{Type: &v1beta1.GatewayClass{}, NamespacedName: types.NamespacedName{Name: "class"}}, - ), - Entry( - "Service delete", - &events.DeleteEvent{ - Type: &apiv1.Service{}, - NamespacedName: types.NamespacedName{Namespace: "test", Name: "service"}, - }, - ), - Entry( - "EndpointSlice deleted", - &events.DeleteEvent{ - Type: &discoveryV1.EndpointSlice{}, - NamespacedName: types.NamespacedName{Namespace: "test", Name: "endpointslice"}, - }, - ), - ) - }) - - Describe("Process Secret events", func() { - expectNoReconfig := func() { - Expect(fakeProcessor.ProcessCallCount()).Should(Equal(1)) - Expect(fakeGenerator.GenerateCallCount()).Should(Equal(0)) - Expect(fakeNginxFileMgr.WriteHTTPConfigCallCount()).Should(Equal(0)) - Expect(fakeNginxRuntimeMgr.ReloadCallCount()).Should(Equal(0)) - Expect(fakeStatusUpdater.UpdateCallCount()).Should(Equal(0)) } - It("should process upsert event", func() { - secret := &apiv1.Secret{} - batch := []interface{}{ - &events.UpsertEvent{ - Resource: secret, - }, - } + checkUpsertEventExpectations := func(e *events.UpsertEvent) { + Expect(fakeProcessor.CaptureUpsertChangeCallCount()).Should(Equal(1)) + Expect(fakeProcessor.CaptureUpsertChangeArgsForCall(0)).Should(Equal(e.Resource)) + } - handler.HandleEventBatch(context.TODO(), batch) + checkDeleteEventExpectations := func(e *events.DeleteEvent) { + Expect(fakeProcessor.CaptureDeleteChangeCallCount()).Should(Equal(1)) + passedResourceType, passedNsName := fakeProcessor.CaptureDeleteChangeArgsForCall(0) + Expect(passedResourceType).Should(Equal(e.Type)) + Expect(passedNsName).Should(Equal(e.NamespacedName)) + } - Expect(fakeSecretStore.UpsertCallCount()).Should(Equal(1)) - Expect(fakeSecretStore.UpsertArgsForCall(0)).Should(Equal(secret)) + BeforeEach(func() { + fakeProcessor.ProcessReturns(true /* changed */, &graph.Graph{}) - expectNoReconfig() + fakeGenerator.GenerateReturns(fakeCfgFiles) }) - It("should process delete event", func() { - nsname := types.NamespacedName{Namespace: "test", Name: "secret"} - - batch := []interface{}{ - &events.DeleteEvent{ - NamespacedName: nsname, - Type: &apiv1.Secret{}, - }, - } - - handler.HandleEventBatch(context.TODO(), batch) + When("a batch has one event", func() { + It("should process Upsert", func() { + e := &events.UpsertEvent{Resource: &v1beta1.HTTPRoute{}} + batch := []interface{}{e} - Expect(fakeSecretStore.DeleteCallCount()).Should(Equal(1)) - Expect(fakeSecretStore.DeleteArgsForCall(0)).Should(Equal(nsname)) + handler.HandleEventBatch(context.Background(), batch) - expectNoReconfig() - }) - }) + checkUpsertEventExpectations(e) + expectReconfig(dataplane.Configuration{}, fakeCfgFiles) + }) - It("should process a batch with upsert and delete events for every supported resource", func() { - svc := &apiv1.Service{} - svcNsName := types.NamespacedName{Namespace: "test", Name: "service"} - secret := &apiv1.Secret{} - secretNsName := types.NamespacedName{Namespace: "test", Name: "secret"} + It("should process Delete", func() { + e := &events.DeleteEvent{ + Type: &v1beta1.HTTPRoute{}, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route"}, + } + batch := []interface{}{e} - upserts := []interface{}{ - &events.UpsertEvent{Resource: &v1beta1.HTTPRoute{}}, - &events.UpsertEvent{Resource: &v1beta1.Gateway{}}, - &events.UpsertEvent{Resource: &v1beta1.GatewayClass{}}, - &events.UpsertEvent{Resource: svc}, - &events.UpsertEvent{Resource: &discoveryV1.EndpointSlice{}}, - &events.UpsertEvent{Resource: secret}, - } - deletes := []interface{}{ - &events.DeleteEvent{ - Type: &v1beta1.HTTPRoute{}, - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route"}, - }, - &events.DeleteEvent{ - Type: &v1beta1.Gateway{}, - NamespacedName: types.NamespacedName{Namespace: "test", Name: "gateway"}, - }, - &events.DeleteEvent{Type: &v1beta1.GatewayClass{}, NamespacedName: types.NamespacedName{Name: "class"}}, - &events.DeleteEvent{Type: &apiv1.Service{}, NamespacedName: svcNsName}, - &events.DeleteEvent{ - Type: &discoveryV1.EndpointSlice{}, - NamespacedName: types.NamespacedName{Namespace: "test", Name: "endpointslice"}, - }, - &events.DeleteEvent{Type: &apiv1.Secret{}, NamespacedName: secretNsName}, - } + handler.HandleEventBatch(context.Background(), batch) - batch := make([]interface{}, 0, len(upserts)+len(deletes)) - batch = append(batch, upserts...) - batch = append(batch, deletes...) + checkDeleteEventExpectations(e) + expectReconfig(dataplane.Configuration{}, fakeCfgFiles) + }) + }) - changed := true - fakeProcessor.ProcessReturns(changed, &graph.Graph{}) + When("a batch has multiple events", func() { + It("should process events", func() { + upsertEvent := &events.UpsertEvent{Resource: &v1beta1.HTTPRoute{}} + deleteEvent := &events.DeleteEvent{ + Type: &v1beta1.HTTPRoute{}, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route"}, + } + batch := []interface{}{upsertEvent, deleteEvent} - fakeCfg := []byte("fake") - fakeGenerator.GenerateReturns(fakeCfg) + handler.HandleEventBatch(context.Background(), batch) - handler.HandleEventBatch(context.TODO(), batch) + checkUpsertEventExpectations(upsertEvent) + checkDeleteEventExpectations(deleteEvent) - // Check that the events for Gateway API resources were captured + handler.HandleEventBatch(context.Background(), batch) + }) + }) + }) - // 5, not 6, because secret upsert events do not result into CaptureUpsertChange() call - Expect(fakeProcessor.CaptureUpsertChangeCallCount()).Should(Equal(5)) - for i := 0; i < 5; i++ { - Expect(fakeProcessor.CaptureUpsertChangeArgsForCall(i)). - Should(Equal(upserts[i].(*events.UpsertEvent).Resource)) - } + It("should panic for an unknown event type", func() { + e := &struct{}{} - // 5, not 6, because secret delete events do not result into CaptureDeleteChange() call - Expect(fakeProcessor.CaptureDeleteChangeCallCount()).Should(Equal(5)) - for i := 0; i < 5; i++ { - d := deletes[i].(*events.DeleteEvent) - passedObj, passedNsName := fakeProcessor.CaptureDeleteChangeArgsForCall(i) - Expect(passedObj).Should(Equal(d.Type)) - Expect(passedNsName).Should(Equal(d.NamespacedName)) + handle := func() { + batch := []interface{}{e} + handler.HandleEventBatch(context.TODO(), batch) } - // Check Secret-related expectations - Expect(fakeSecretStore.UpsertCallCount()).Should(Equal(1)) - Expect(fakeSecretStore.UpsertArgsForCall(0)).Should(Equal(secret)) - - Expect(fakeSecretStore.DeleteCallCount()).Should(Equal(1)) - Expect(fakeSecretStore.DeleteArgsForCall(0)).Should(Equal(secretNsName)) - - // Check that a reconfig happened - expectReconfig(dataplane.Configuration{}, fakeCfg) - }) - - Describe("Edge cases", func() { - DescribeTable("Edge cases for events", - func(e interface{}) { - handle := func() { - batch := []interface{}{e} - handler.HandleEventBatch(context.TODO(), batch) - } - - Expect(handle).Should(Panic()) - }, - Entry("should panic for an unknown event type", - &struct{}{}), - Entry("should panic for an unknown type of resource in upsert event", - &events.UpsertEvent{ - Resource: &unsupportedResource{}, - }), - Entry("should panic for an unknown type of resource in delete event", - &events.DeleteEvent{ - Type: &unsupportedResource{}, - }), - ) + Expect(handle).Should(Panic()) }) }) diff --git a/internal/manager/manager.go b/internal/manager/manager.go index 994e0e40a2..3454ab26da 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -29,7 +29,6 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/relationship" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/resolver" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/validation" "github.com/nginxinc/nginx-kubernetes-gateway/internal/status" ) @@ -37,9 +36,6 @@ import ( const ( // clusterTimeout is a timeout for connections to the Kubernetes API clusterTimeout = 10 * time.Second - // secretsFolder is the folder that holds all the secrets for NGINX servers. - // nolint:gosec - secretsFolder = "/etc/nginx/secrets" ) var scheme = runtime.NewScheme() @@ -133,16 +129,12 @@ func Start(cfg config.Config) error { } } - secretStore := secrets.NewSecretStore() - secretMemoryMgr := secrets.NewSecretDiskMemoryManager(secretsFolder, secretStore) - recorderName := fmt.Sprintf("nginx-kubernetes-gateway-%s", cfg.GatewayClassName) recorder := mgr.GetEventRecorderFor(recorderName) processor := state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ GatewayCtlrName: cfg.GatewayCtlrName, GatewayClassName: cfg.GatewayClassName, - SecretMemoryManager: secretMemoryMgr, RelationshipCapturer: relationship.NewCapturerImpl(), Logger: cfg.Logger.WithName("changeProcessor"), Validators: validation.Validators{ @@ -153,7 +145,18 @@ func Start(cfg config.Config) error { }) configGenerator := ngxcfg.NewGeneratorImpl() - nginxFileMgr := file.NewManagerImpl() + + // Clear the configuration folders to ensure that no files are left over in case the control plane was restarted + // (this assumes the folders are in a shared volume). + removedPaths, err := file.ClearFolders(file.NewStdLibOSFileManager(), ngxcfg.ConfigFolders) + for _, path := range removedPaths { + logger.Info("removed configuration file", "path", path) + } + if err != nil { + return fmt.Errorf("cannot clear NGINX configuration folders: %w", err) + } + + nginxFileMgr := file.NewManagerImpl(logger.WithName("nginxFileManager"), file.NewStdLibOSFileManager()) nginxRuntimeMgr := ngxruntime.NewManagerImpl() statusUpdater := status.NewUpdater(status.UpdaterConfig{ GatewayCtlrName: cfg.GatewayCtlrName, @@ -166,15 +169,13 @@ func Start(cfg config.Config) error { }) eventHandler := events.NewEventHandlerImpl(events.EventHandlerConfig{ - Processor: processor, - SecretStore: secretStore, - SecretMemoryManager: secretMemoryMgr, - ServiceResolver: resolver.NewServiceResolverImpl(mgr.GetClient()), - Generator: configGenerator, - Logger: cfg.Logger.WithName("eventHandler"), - NginxFileMgr: nginxFileMgr, - NginxRuntimeMgr: nginxRuntimeMgr, - StatusUpdater: statusUpdater, + Processor: processor, + ServiceResolver: resolver.NewServiceResolverImpl(mgr.GetClient()), + Generator: configGenerator, + Logger: cfg.Logger.WithName("eventHandler"), + NginxFileMgr: nginxFileMgr, + NginxRuntimeMgr: nginxRuntimeMgr, + StatusUpdater: statusUpdater, }) objects, objectLists := prepareFirstEventBatchPreparerArgs(cfg.GatewayClassName, cfg.GatewayNsName) diff --git a/internal/nginx/config/configfakes/fake_generator.go b/internal/nginx/config/configfakes/fake_generator.go index 73247d7388..7628a7978d 100644 --- a/internal/nginx/config/configfakes/fake_generator.go +++ b/internal/nginx/config/configfakes/fake_generator.go @@ -5,26 +5,27 @@ import ( "sync" "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/config" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/dataplane" ) type FakeGenerator struct { - GenerateStub func(dataplane.Configuration) []byte + GenerateStub func(dataplane.Configuration) []file.File generateMutex sync.RWMutex generateArgsForCall []struct { arg1 dataplane.Configuration } generateReturns struct { - result1 []byte + result1 []file.File } generateReturnsOnCall map[int]struct { - result1 []byte + result1 []file.File } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } -func (fake *FakeGenerator) Generate(arg1 dataplane.Configuration) []byte { +func (fake *FakeGenerator) Generate(arg1 dataplane.Configuration) []file.File { fake.generateMutex.Lock() ret, specificReturn := fake.generateReturnsOnCall[len(fake.generateArgsForCall)] fake.generateArgsForCall = append(fake.generateArgsForCall, struct { @@ -49,7 +50,7 @@ func (fake *FakeGenerator) GenerateCallCount() int { return len(fake.generateArgsForCall) } -func (fake *FakeGenerator) GenerateCalls(stub func(dataplane.Configuration) []byte) { +func (fake *FakeGenerator) GenerateCalls(stub func(dataplane.Configuration) []file.File) { fake.generateMutex.Lock() defer fake.generateMutex.Unlock() fake.GenerateStub = stub @@ -62,26 +63,26 @@ func (fake *FakeGenerator) GenerateArgsForCall(i int) dataplane.Configuration { return argsForCall.arg1 } -func (fake *FakeGenerator) GenerateReturns(result1 []byte) { +func (fake *FakeGenerator) GenerateReturns(result1 []file.File) { fake.generateMutex.Lock() defer fake.generateMutex.Unlock() fake.GenerateStub = nil fake.generateReturns = struct { - result1 []byte + result1 []file.File }{result1} } -func (fake *FakeGenerator) GenerateReturnsOnCall(i int, result1 []byte) { +func (fake *FakeGenerator) GenerateReturnsOnCall(i int, result1 []file.File) { fake.generateMutex.Lock() defer fake.generateMutex.Unlock() fake.GenerateStub = nil if fake.generateReturnsOnCall == nil { fake.generateReturnsOnCall = make(map[int]struct { - result1 []byte + result1 []file.File }) } fake.generateReturnsOnCall[i] = struct { - result1 []byte + result1 []file.File }{result1} } diff --git a/internal/nginx/config/generator.go b/internal/nginx/config/generator.go index 5af89b6acc..732c09b72e 100644 --- a/internal/nginx/config/generator.go +++ b/internal/nginx/config/generator.go @@ -1,19 +1,45 @@ package config import ( + "path/filepath" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/dataplane" ) //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Generator -// Generator generates NGINX configuration. +const ( + // configFolder is the folder where NGINX configuration files are stored. + configFolder = "/etc/nginx" + + // httpFolder is the folder where NGINX HTTP configuration files are stored. + httpFolder = configFolder + "/conf.d" + // secretsFolder is the folder where secrets (like TLS certs/keys) are stored. + secretsFolder = configFolder + "/secrets" + + // httpConfigFile is the path to the configuration file with HTTP configuration. + httpConfigFile = httpFolder + "/http.conf" +) + +// ConfigFolders is a list of folders where NGINX configuration files are stored. +var ConfigFolders = []string{httpFolder, secretsFolder} + +// Generator generates NGINX configuration files. // This interface is used for testing purposes only. type Generator interface { - // Generate generates NGINX configuration from internal representation. - Generate(configuration dataplane.Configuration) []byte + // Generate generates NGINX configuration files from internal representation. + Generate(configuration dataplane.Configuration) []file.File } // GeneratorImpl is an implementation of Generator. +// +// It generates files to be written to the following locations, which must exist and available for writing: +// - httpFolder, for HTTP configuration files. +// - secretsFolder, for secrets. +// +// It also expects that the main NGINX configuration file nginx.conf is located in configFolder and nginx.conf +// includes (https://nginx.org/en/docs/ngx_core_module.html#include) the files from httpFolder. type GeneratorImpl struct{} // NewGeneratorImpl creates a new GeneratorImpl. @@ -24,17 +50,50 @@ func NewGeneratorImpl() GeneratorImpl { // executeFunc is a function that generates NGINX configuration from internal representation. type executeFunc func(configuration dataplane.Configuration) []byte -// Generate generates NGINX configuration from internal representation. +// Generate generates NGINX configuration files from internal representation. // It is the responsibility of the caller to validate the configuration before calling this function. // In case of invalid configuration, NGINX will fail to reload or could be configured with malicious configuration. // To validate, use the validators from the validation package. -func (g GeneratorImpl) Generate(conf dataplane.Configuration) []byte { - var generated []byte +func (g GeneratorImpl) Generate(conf dataplane.Configuration) []file.File { + files := make([]file.File, 0, len(conf.SSLKeyPairs)+1 /* http config */) + + for id, pair := range conf.SSLKeyPairs { + files = append(files, generatePEM(id, pair.Cert, pair.Key)) + } + + files = append(files, generateHTTPConfig(conf)) + + return files +} + +func generatePEM(id dataplane.SSLKeyPairID, cert []byte, key []byte) file.File { + c := make([]byte, 0, len(cert)+len(key)+1) + c = append(c, cert...) + c = append(c, '\n') + c = append(c, key...) + + return file.File{ + Content: c, + Path: generatePEMFileName(id), + Type: file.TypeSecret, + } +} + +func generatePEMFileName(id dataplane.SSLKeyPairID) string { + return filepath.Join(secretsFolder, string(id)+".pem") +} + +func generateHTTPConfig(conf dataplane.Configuration) file.File { + var c []byte for _, execute := range getExecuteFuncs() { - generated = append(generated, execute(conf)...) + c = append(c, execute(conf)...) } - return generated + return file.File{ + Content: c, + Path: httpConfigFile, + Type: file.TypeRegular, + } } func getExecuteFuncs() []executeFunc { diff --git a/internal/nginx/config/generator_test.go b/internal/nginx/config/generator_test.go index 34aff2412d..cc30a4e779 100644 --- a/internal/nginx/config/generator_test.go +++ b/internal/nginx/config/generator_test.go @@ -7,11 +7,10 @@ import ( "k8s.io/apimachinery/pkg/types" "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/config" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/dataplane" ) -// Note: this test only verifies that Generate() returns a byte array with upstream, server, and split_client blocks. -// It does not test the correctness of those blocks. That functionality is covered by other tests in this package. func TestGenerate(t *testing.T) { bg := dataplane.BackendGroup{ Source: types.NamespacedName{Namespace: "test", Name: "hr"}, @@ -41,7 +40,7 @@ func TestGenerate(t *testing.T) { { Hostname: "example.com", SSL: &dataplane.SSL{ - CertificatePath: "/etc/nginx/secrets/default", + KeyPairID: "test-keypair", }, Port: 443, }, @@ -53,14 +52,34 @@ func TestGenerate(t *testing.T) { }, }, BackendGroups: []dataplane.BackendGroup{bg}, + SSLKeyPairs: map[dataplane.SSLKeyPairID]dataplane.SSLKeyPair{ + "test-keypair": { + Cert: []byte("test-cert"), + Key: []byte("test-key"), + }, + }, } g := NewGomegaWithT(t) generator := config.NewGeneratorImpl() - cfg := string(generator.Generate(conf)) - g.Expect(cfg).To(ContainSubstring("listen 80")) - g.Expect(cfg).To(ContainSubstring("listen 443")) - g.Expect(cfg).To(ContainSubstring("upstream")) - g.Expect(cfg).To(ContainSubstring("split_clients")) + files := generator.Generate(conf) + + g.Expect(files).To(HaveLen(2)) + + g.Expect(files[0]).To(Equal(file.File{ + Type: file.TypeSecret, + Path: "/etc/nginx/secrets/test-keypair.pem", + Content: []byte("test-cert\ntest-key"), + })) + + g.Expect(files[1].Type).To(Equal(file.TypeRegular)) + g.Expect(files[1].Path).To(Equal("/etc/nginx/conf.d/http.conf")) + httpCfg := string(files[1].Content) // converting to string so that on failure gomega prints strings not byte arrays + // Note: this only verifies that Generate() returns a byte array with upstream, server, and split_client blocks. + // It does not test the correctness of those blocks. That functionality is covered by other tests in this package. + g.Expect(httpCfg).To(ContainSubstring("listen 80")) + g.Expect(httpCfg).To(ContainSubstring("listen 443")) + g.Expect(httpCfg).To(ContainSubstring("upstream")) + g.Expect(httpCfg).To(ContainSubstring("split_clients")) } diff --git a/internal/nginx/config/servers.go b/internal/nginx/config/servers.go index 432a175279..f2efec53b7 100644 --- a/internal/nginx/config/servers.go +++ b/internal/nginx/config/servers.go @@ -51,8 +51,8 @@ func createSSLServer(virtualServer dataplane.VirtualServer) http.Server { return http.Server{ ServerName: virtualServer.Hostname, SSL: &http.SSL{ - Certificate: virtualServer.SSL.CertificatePath, - CertificateKey: virtualServer.SSL.CertificatePath, + Certificate: generatePEMFileName(virtualServer.SSL.KeyPairID), + CertificateKey: generatePEMFileName(virtualServer.SSL.KeyPairID), }, Locations: createLocations(virtualServer.PathRules, virtualServer.Port), Port: virtualServer.Port, diff --git a/internal/nginx/config/servers_test.go b/internal/nginx/config/servers_test.go index fa253f6841..e0a5d9a818 100644 --- a/internal/nginx/config/servers_test.go +++ b/internal/nginx/config/servers_test.go @@ -41,14 +41,14 @@ func TestExecuteServers(t *testing.T) { { Hostname: "example.com", SSL: &dataplane.SSL{ - CertificatePath: "cert-path", + KeyPairID: "test-keypair", }, Port: 8443, }, { Hostname: "cafe.example.com", SSL: &dataplane.SSL{ - CertificatePath: "cert-path", + KeyPairID: "test-keypair", }, Port: 8443, }, @@ -56,14 +56,14 @@ func TestExecuteServers(t *testing.T) { } expSubStrings := map[string]int{ - "listen 8080 default_server;": 1, - "listen 8080;": 2, - "listen 8443 ssl;": 2, - "listen 8443 ssl default_server;": 1, - "server_name example.com;": 2, - "server_name cafe.example.com;": 2, - "ssl_certificate cert-path;": 2, - "ssl_certificate_key cert-path;": 2, + "listen 8080 default_server;": 1, + "listen 8080;": 2, + "listen 8443 ssl;": 2, + "listen 8443 ssl default_server;": 1, + "server_name example.com;": 2, + "server_name cafe.example.com;": 2, + "ssl_certificate /etc/nginx/secrets/test-keypair.pem;": 2, + "ssl_certificate_key /etc/nginx/secrets/test-keypair.pem;": 2, } servers := string(executeServers(conf)) @@ -165,7 +165,7 @@ func TestExecuteForDefaultServers(t *testing.T) { func TestCreateServers(t *testing.T) { const ( - certPath = "/etc/nginx/secrets/cert" + sslKeyPairID = "test-keypair" ) hr := &v1beta1.HTTPRoute{ @@ -560,7 +560,7 @@ func TestCreateServers(t *testing.T) { }, { Hostname: "cafe.example.com", - SSL: &dataplane.SSL{CertificatePath: certPath}, + SSL: &dataplane.SSL{KeyPairID: sslKeyPairID}, PathRules: cafePathRules, Port: 8443, }, @@ -681,6 +681,8 @@ func TestCreateServers(t *testing.T) { } } + expectedPEMPath := fmt.Sprintf("/etc/nginx/secrets/%s.pem", sslKeyPairID) + expectedServers := []http.Server{ { IsDefaultHTTP: true, @@ -697,9 +699,12 @@ func TestCreateServers(t *testing.T) { }, { ServerName: "cafe.example.com", - SSL: &http.SSL{Certificate: certPath, CertificateKey: certPath}, - Locations: getExpectedLocations(true), - Port: 8443, + SSL: &http.SSL{ + Certificate: expectedPEMPath, + CertificateKey: expectedPEMPath, + }, + Locations: getExpectedLocations(true), + Port: 8443, }, } diff --git a/internal/nginx/file/file_suite_test.go b/internal/nginx/file/file_suite_test.go new file mode 100644 index 0000000000..46afb4ea54 --- /dev/null +++ b/internal/nginx/file/file_suite_test.go @@ -0,0 +1,13 @@ +package file_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestFile(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "File Suite") +} diff --git a/internal/nginx/file/filefakes/fake_clear_folders_osfile_manager.go b/internal/nginx/file/filefakes/fake_clear_folders_osfile_manager.go new file mode 100644 index 0000000000..349f88997c --- /dev/null +++ b/internal/nginx/file/filefakes/fake_clear_folders_osfile_manager.go @@ -0,0 +1,191 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package filefakes + +import ( + "io/fs" + "sync" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file" +) + +type FakeClearFoldersOSFileManager struct { + ReadDirStub func(string) ([]fs.DirEntry, error) + readDirMutex sync.RWMutex + readDirArgsForCall []struct { + arg1 string + } + readDirReturns struct { + result1 []fs.DirEntry + result2 error + } + readDirReturnsOnCall map[int]struct { + result1 []fs.DirEntry + result2 error + } + RemoveStub func(string) error + removeMutex sync.RWMutex + removeArgsForCall []struct { + arg1 string + } + removeReturns struct { + result1 error + } + removeReturnsOnCall map[int]struct { + result1 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeClearFoldersOSFileManager) ReadDir(arg1 string) ([]fs.DirEntry, error) { + fake.readDirMutex.Lock() + ret, specificReturn := fake.readDirReturnsOnCall[len(fake.readDirArgsForCall)] + fake.readDirArgsForCall = append(fake.readDirArgsForCall, struct { + arg1 string + }{arg1}) + stub := fake.ReadDirStub + fakeReturns := fake.readDirReturns + fake.recordInvocation("ReadDir", []interface{}{arg1}) + fake.readDirMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *FakeClearFoldersOSFileManager) ReadDirCallCount() int { + fake.readDirMutex.RLock() + defer fake.readDirMutex.RUnlock() + return len(fake.readDirArgsForCall) +} + +func (fake *FakeClearFoldersOSFileManager) ReadDirCalls(stub func(string) ([]fs.DirEntry, error)) { + fake.readDirMutex.Lock() + defer fake.readDirMutex.Unlock() + fake.ReadDirStub = stub +} + +func (fake *FakeClearFoldersOSFileManager) ReadDirArgsForCall(i int) string { + fake.readDirMutex.RLock() + defer fake.readDirMutex.RUnlock() + argsForCall := fake.readDirArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeClearFoldersOSFileManager) ReadDirReturns(result1 []fs.DirEntry, result2 error) { + fake.readDirMutex.Lock() + defer fake.readDirMutex.Unlock() + fake.ReadDirStub = nil + fake.readDirReturns = struct { + result1 []fs.DirEntry + result2 error + }{result1, result2} +} + +func (fake *FakeClearFoldersOSFileManager) ReadDirReturnsOnCall(i int, result1 []fs.DirEntry, result2 error) { + fake.readDirMutex.Lock() + defer fake.readDirMutex.Unlock() + fake.ReadDirStub = nil + if fake.readDirReturnsOnCall == nil { + fake.readDirReturnsOnCall = make(map[int]struct { + result1 []fs.DirEntry + result2 error + }) + } + fake.readDirReturnsOnCall[i] = struct { + result1 []fs.DirEntry + result2 error + }{result1, result2} +} + +func (fake *FakeClearFoldersOSFileManager) Remove(arg1 string) error { + fake.removeMutex.Lock() + ret, specificReturn := fake.removeReturnsOnCall[len(fake.removeArgsForCall)] + fake.removeArgsForCall = append(fake.removeArgsForCall, struct { + arg1 string + }{arg1}) + stub := fake.RemoveStub + fakeReturns := fake.removeReturns + fake.recordInvocation("Remove", []interface{}{arg1}) + fake.removeMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeClearFoldersOSFileManager) RemoveCallCount() int { + fake.removeMutex.RLock() + defer fake.removeMutex.RUnlock() + return len(fake.removeArgsForCall) +} + +func (fake *FakeClearFoldersOSFileManager) RemoveCalls(stub func(string) error) { + fake.removeMutex.Lock() + defer fake.removeMutex.Unlock() + fake.RemoveStub = stub +} + +func (fake *FakeClearFoldersOSFileManager) RemoveArgsForCall(i int) string { + fake.removeMutex.RLock() + defer fake.removeMutex.RUnlock() + argsForCall := fake.removeArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeClearFoldersOSFileManager) RemoveReturns(result1 error) { + fake.removeMutex.Lock() + defer fake.removeMutex.Unlock() + fake.RemoveStub = nil + fake.removeReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeClearFoldersOSFileManager) RemoveReturnsOnCall(i int, result1 error) { + fake.removeMutex.Lock() + defer fake.removeMutex.Unlock() + fake.RemoveStub = nil + if fake.removeReturnsOnCall == nil { + fake.removeReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.removeReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeClearFoldersOSFileManager) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.readDirMutex.RLock() + defer fake.readDirMutex.RUnlock() + fake.removeMutex.RLock() + defer fake.removeMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeClearFoldersOSFileManager) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ file.ClearFoldersOSFileManager = new(FakeClearFoldersOSFileManager) diff --git a/internal/state/secrets/secretsfakes/fake_dir_entry.go b/internal/nginx/file/filefakes/fake_dir_entry.go similarity index 99% rename from internal/state/secrets/secretsfakes/fake_dir_entry.go rename to internal/nginx/file/filefakes/fake_dir_entry.go index ee1ba1342f..b51ecd7579 100644 --- a/internal/state/secrets/secretsfakes/fake_dir_entry.go +++ b/internal/nginx/file/filefakes/fake_dir_entry.go @@ -1,5 +1,5 @@ // Code generated by counterfeiter. DO NOT EDIT. -package secretsfakes +package filefakes import ( "io/fs" diff --git a/internal/nginx/file/filefakes/fake_manager.go b/internal/nginx/file/filefakes/fake_manager.go index ed2c52caac..644e5a3c0f 100644 --- a/internal/nginx/file/filefakes/fake_manager.go +++ b/internal/nginx/file/filefakes/fake_manager.go @@ -8,40 +8,38 @@ import ( ) type FakeManager struct { - WriteHTTPConfigStub func(string, []byte) error - writeHTTPConfigMutex sync.RWMutex - writeHTTPConfigArgsForCall []struct { - arg1 string - arg2 []byte + ReplaceFilesStub func([]file.File) error + replaceFilesMutex sync.RWMutex + replaceFilesArgsForCall []struct { + arg1 []file.File } - writeHTTPConfigReturns struct { + replaceFilesReturns struct { result1 error } - writeHTTPConfigReturnsOnCall map[int]struct { + replaceFilesReturnsOnCall map[int]struct { result1 error } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } -func (fake *FakeManager) WriteHTTPConfig(arg1 string, arg2 []byte) error { - var arg2Copy []byte - if arg2 != nil { - arg2Copy = make([]byte, len(arg2)) - copy(arg2Copy, arg2) +func (fake *FakeManager) ReplaceFiles(arg1 []file.File) error { + var arg1Copy []file.File + if arg1 != nil { + arg1Copy = make([]file.File, len(arg1)) + copy(arg1Copy, arg1) } - fake.writeHTTPConfigMutex.Lock() - ret, specificReturn := fake.writeHTTPConfigReturnsOnCall[len(fake.writeHTTPConfigArgsForCall)] - fake.writeHTTPConfigArgsForCall = append(fake.writeHTTPConfigArgsForCall, struct { - arg1 string - arg2 []byte - }{arg1, arg2Copy}) - stub := fake.WriteHTTPConfigStub - fakeReturns := fake.writeHTTPConfigReturns - fake.recordInvocation("WriteHTTPConfig", []interface{}{arg1, arg2Copy}) - fake.writeHTTPConfigMutex.Unlock() + fake.replaceFilesMutex.Lock() + ret, specificReturn := fake.replaceFilesReturnsOnCall[len(fake.replaceFilesArgsForCall)] + fake.replaceFilesArgsForCall = append(fake.replaceFilesArgsForCall, struct { + arg1 []file.File + }{arg1Copy}) + stub := fake.ReplaceFilesStub + fakeReturns := fake.replaceFilesReturns + fake.recordInvocation("ReplaceFiles", []interface{}{arg1Copy}) + fake.replaceFilesMutex.Unlock() if stub != nil { - return stub(arg1, arg2) + return stub(arg1) } if specificReturn { return ret.result1 @@ -49,44 +47,44 @@ func (fake *FakeManager) WriteHTTPConfig(arg1 string, arg2 []byte) error { return fakeReturns.result1 } -func (fake *FakeManager) WriteHTTPConfigCallCount() int { - fake.writeHTTPConfigMutex.RLock() - defer fake.writeHTTPConfigMutex.RUnlock() - return len(fake.writeHTTPConfigArgsForCall) +func (fake *FakeManager) ReplaceFilesCallCount() int { + fake.replaceFilesMutex.RLock() + defer fake.replaceFilesMutex.RUnlock() + return len(fake.replaceFilesArgsForCall) } -func (fake *FakeManager) WriteHTTPConfigCalls(stub func(string, []byte) error) { - fake.writeHTTPConfigMutex.Lock() - defer fake.writeHTTPConfigMutex.Unlock() - fake.WriteHTTPConfigStub = stub +func (fake *FakeManager) ReplaceFilesCalls(stub func([]file.File) error) { + fake.replaceFilesMutex.Lock() + defer fake.replaceFilesMutex.Unlock() + fake.ReplaceFilesStub = stub } -func (fake *FakeManager) WriteHTTPConfigArgsForCall(i int) (string, []byte) { - fake.writeHTTPConfigMutex.RLock() - defer fake.writeHTTPConfigMutex.RUnlock() - argsForCall := fake.writeHTTPConfigArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2 +func (fake *FakeManager) ReplaceFilesArgsForCall(i int) []file.File { + fake.replaceFilesMutex.RLock() + defer fake.replaceFilesMutex.RUnlock() + argsForCall := fake.replaceFilesArgsForCall[i] + return argsForCall.arg1 } -func (fake *FakeManager) WriteHTTPConfigReturns(result1 error) { - fake.writeHTTPConfigMutex.Lock() - defer fake.writeHTTPConfigMutex.Unlock() - fake.WriteHTTPConfigStub = nil - fake.writeHTTPConfigReturns = struct { +func (fake *FakeManager) ReplaceFilesReturns(result1 error) { + fake.replaceFilesMutex.Lock() + defer fake.replaceFilesMutex.Unlock() + fake.ReplaceFilesStub = nil + fake.replaceFilesReturns = struct { result1 error }{result1} } -func (fake *FakeManager) WriteHTTPConfigReturnsOnCall(i int, result1 error) { - fake.writeHTTPConfigMutex.Lock() - defer fake.writeHTTPConfigMutex.Unlock() - fake.WriteHTTPConfigStub = nil - if fake.writeHTTPConfigReturnsOnCall == nil { - fake.writeHTTPConfigReturnsOnCall = make(map[int]struct { +func (fake *FakeManager) ReplaceFilesReturnsOnCall(i int, result1 error) { + fake.replaceFilesMutex.Lock() + defer fake.replaceFilesMutex.Unlock() + fake.ReplaceFilesStub = nil + if fake.replaceFilesReturnsOnCall == nil { + fake.replaceFilesReturnsOnCall = make(map[int]struct { result1 error }) } - fake.writeHTTPConfigReturnsOnCall[i] = struct { + fake.replaceFilesReturnsOnCall[i] = struct { result1 error }{result1} } @@ -94,8 +92,8 @@ func (fake *FakeManager) WriteHTTPConfigReturnsOnCall(i int, result1 error) { func (fake *FakeManager) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() - fake.writeHTTPConfigMutex.RLock() - defer fake.writeHTTPConfigMutex.RUnlock() + fake.replaceFilesMutex.RLock() + defer fake.replaceFilesMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} for key, value := range fake.invocations { copiedInvocations[key] = value diff --git a/internal/state/secrets/secretsfakes/fake_file_manager.go b/internal/nginx/file/filefakes/fake_osfile_manager.go similarity index 77% rename from internal/state/secrets/secretsfakes/fake_file_manager.go rename to internal/nginx/file/filefakes/fake_osfile_manager.go index 3c4e958f30..bf03b2080f 100644 --- a/internal/state/secrets/secretsfakes/fake_file_manager.go +++ b/internal/nginx/file/filefakes/fake_osfile_manager.go @@ -1,15 +1,15 @@ // Code generated by counterfeiter. DO NOT EDIT. -package secretsfakes +package filefakes import ( "io/fs" "os" "sync" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file" ) -type FakeFileManager struct { +type FakeOSFileManager struct { ChmodStub func(*os.File, fs.FileMode) error chmodMutex sync.RWMutex chmodArgsForCall []struct { @@ -75,7 +75,7 @@ type FakeFileManager struct { invocationsMutex sync.RWMutex } -func (fake *FakeFileManager) Chmod(arg1 *os.File, arg2 fs.FileMode) error { +func (fake *FakeOSFileManager) Chmod(arg1 *os.File, arg2 fs.FileMode) error { fake.chmodMutex.Lock() ret, specificReturn := fake.chmodReturnsOnCall[len(fake.chmodArgsForCall)] fake.chmodArgsForCall = append(fake.chmodArgsForCall, struct { @@ -95,26 +95,26 @@ func (fake *FakeFileManager) Chmod(arg1 *os.File, arg2 fs.FileMode) error { return fakeReturns.result1 } -func (fake *FakeFileManager) ChmodCallCount() int { +func (fake *FakeOSFileManager) ChmodCallCount() int { fake.chmodMutex.RLock() defer fake.chmodMutex.RUnlock() return len(fake.chmodArgsForCall) } -func (fake *FakeFileManager) ChmodCalls(stub func(*os.File, fs.FileMode) error) { +func (fake *FakeOSFileManager) ChmodCalls(stub func(*os.File, fs.FileMode) error) { fake.chmodMutex.Lock() defer fake.chmodMutex.Unlock() fake.ChmodStub = stub } -func (fake *FakeFileManager) ChmodArgsForCall(i int) (*os.File, fs.FileMode) { +func (fake *FakeOSFileManager) ChmodArgsForCall(i int) (*os.File, fs.FileMode) { fake.chmodMutex.RLock() defer fake.chmodMutex.RUnlock() argsForCall := fake.chmodArgsForCall[i] return argsForCall.arg1, argsForCall.arg2 } -func (fake *FakeFileManager) ChmodReturns(result1 error) { +func (fake *FakeOSFileManager) ChmodReturns(result1 error) { fake.chmodMutex.Lock() defer fake.chmodMutex.Unlock() fake.ChmodStub = nil @@ -123,7 +123,7 @@ func (fake *FakeFileManager) ChmodReturns(result1 error) { }{result1} } -func (fake *FakeFileManager) ChmodReturnsOnCall(i int, result1 error) { +func (fake *FakeOSFileManager) ChmodReturnsOnCall(i int, result1 error) { fake.chmodMutex.Lock() defer fake.chmodMutex.Unlock() fake.ChmodStub = nil @@ -137,7 +137,7 @@ func (fake *FakeFileManager) ChmodReturnsOnCall(i int, result1 error) { }{result1} } -func (fake *FakeFileManager) Create(arg1 string) (*os.File, error) { +func (fake *FakeOSFileManager) Create(arg1 string) (*os.File, error) { fake.createMutex.Lock() ret, specificReturn := fake.createReturnsOnCall[len(fake.createArgsForCall)] fake.createArgsForCall = append(fake.createArgsForCall, struct { @@ -156,26 +156,26 @@ func (fake *FakeFileManager) Create(arg1 string) (*os.File, error) { return fakeReturns.result1, fakeReturns.result2 } -func (fake *FakeFileManager) CreateCallCount() int { +func (fake *FakeOSFileManager) CreateCallCount() int { fake.createMutex.RLock() defer fake.createMutex.RUnlock() return len(fake.createArgsForCall) } -func (fake *FakeFileManager) CreateCalls(stub func(string) (*os.File, error)) { +func (fake *FakeOSFileManager) CreateCalls(stub func(string) (*os.File, error)) { fake.createMutex.Lock() defer fake.createMutex.Unlock() fake.CreateStub = stub } -func (fake *FakeFileManager) CreateArgsForCall(i int) string { +func (fake *FakeOSFileManager) CreateArgsForCall(i int) string { fake.createMutex.RLock() defer fake.createMutex.RUnlock() argsForCall := fake.createArgsForCall[i] return argsForCall.arg1 } -func (fake *FakeFileManager) CreateReturns(result1 *os.File, result2 error) { +func (fake *FakeOSFileManager) CreateReturns(result1 *os.File, result2 error) { fake.createMutex.Lock() defer fake.createMutex.Unlock() fake.CreateStub = nil @@ -185,7 +185,7 @@ func (fake *FakeFileManager) CreateReturns(result1 *os.File, result2 error) { }{result1, result2} } -func (fake *FakeFileManager) CreateReturnsOnCall(i int, result1 *os.File, result2 error) { +func (fake *FakeOSFileManager) CreateReturnsOnCall(i int, result1 *os.File, result2 error) { fake.createMutex.Lock() defer fake.createMutex.Unlock() fake.CreateStub = nil @@ -201,7 +201,7 @@ func (fake *FakeFileManager) CreateReturnsOnCall(i int, result1 *os.File, result }{result1, result2} } -func (fake *FakeFileManager) ReadDir(arg1 string) ([]fs.DirEntry, error) { +func (fake *FakeOSFileManager) ReadDir(arg1 string) ([]fs.DirEntry, error) { fake.readDirMutex.Lock() ret, specificReturn := fake.readDirReturnsOnCall[len(fake.readDirArgsForCall)] fake.readDirArgsForCall = append(fake.readDirArgsForCall, struct { @@ -220,26 +220,26 @@ func (fake *FakeFileManager) ReadDir(arg1 string) ([]fs.DirEntry, error) { return fakeReturns.result1, fakeReturns.result2 } -func (fake *FakeFileManager) ReadDirCallCount() int { +func (fake *FakeOSFileManager) ReadDirCallCount() int { fake.readDirMutex.RLock() defer fake.readDirMutex.RUnlock() return len(fake.readDirArgsForCall) } -func (fake *FakeFileManager) ReadDirCalls(stub func(string) ([]fs.DirEntry, error)) { +func (fake *FakeOSFileManager) ReadDirCalls(stub func(string) ([]fs.DirEntry, error)) { fake.readDirMutex.Lock() defer fake.readDirMutex.Unlock() fake.ReadDirStub = stub } -func (fake *FakeFileManager) ReadDirArgsForCall(i int) string { +func (fake *FakeOSFileManager) ReadDirArgsForCall(i int) string { fake.readDirMutex.RLock() defer fake.readDirMutex.RUnlock() argsForCall := fake.readDirArgsForCall[i] return argsForCall.arg1 } -func (fake *FakeFileManager) ReadDirReturns(result1 []fs.DirEntry, result2 error) { +func (fake *FakeOSFileManager) ReadDirReturns(result1 []fs.DirEntry, result2 error) { fake.readDirMutex.Lock() defer fake.readDirMutex.Unlock() fake.ReadDirStub = nil @@ -249,7 +249,7 @@ func (fake *FakeFileManager) ReadDirReturns(result1 []fs.DirEntry, result2 error }{result1, result2} } -func (fake *FakeFileManager) ReadDirReturnsOnCall(i int, result1 []fs.DirEntry, result2 error) { +func (fake *FakeOSFileManager) ReadDirReturnsOnCall(i int, result1 []fs.DirEntry, result2 error) { fake.readDirMutex.Lock() defer fake.readDirMutex.Unlock() fake.ReadDirStub = nil @@ -265,7 +265,7 @@ func (fake *FakeFileManager) ReadDirReturnsOnCall(i int, result1 []fs.DirEntry, }{result1, result2} } -func (fake *FakeFileManager) Remove(arg1 string) error { +func (fake *FakeOSFileManager) Remove(arg1 string) error { fake.removeMutex.Lock() ret, specificReturn := fake.removeReturnsOnCall[len(fake.removeArgsForCall)] fake.removeArgsForCall = append(fake.removeArgsForCall, struct { @@ -284,26 +284,26 @@ func (fake *FakeFileManager) Remove(arg1 string) error { return fakeReturns.result1 } -func (fake *FakeFileManager) RemoveCallCount() int { +func (fake *FakeOSFileManager) RemoveCallCount() int { fake.removeMutex.RLock() defer fake.removeMutex.RUnlock() return len(fake.removeArgsForCall) } -func (fake *FakeFileManager) RemoveCalls(stub func(string) error) { +func (fake *FakeOSFileManager) RemoveCalls(stub func(string) error) { fake.removeMutex.Lock() defer fake.removeMutex.Unlock() fake.RemoveStub = stub } -func (fake *FakeFileManager) RemoveArgsForCall(i int) string { +func (fake *FakeOSFileManager) RemoveArgsForCall(i int) string { fake.removeMutex.RLock() defer fake.removeMutex.RUnlock() argsForCall := fake.removeArgsForCall[i] return argsForCall.arg1 } -func (fake *FakeFileManager) RemoveReturns(result1 error) { +func (fake *FakeOSFileManager) RemoveReturns(result1 error) { fake.removeMutex.Lock() defer fake.removeMutex.Unlock() fake.RemoveStub = nil @@ -312,7 +312,7 @@ func (fake *FakeFileManager) RemoveReturns(result1 error) { }{result1} } -func (fake *FakeFileManager) RemoveReturnsOnCall(i int, result1 error) { +func (fake *FakeOSFileManager) RemoveReturnsOnCall(i int, result1 error) { fake.removeMutex.Lock() defer fake.removeMutex.Unlock() fake.RemoveStub = nil @@ -326,7 +326,7 @@ func (fake *FakeFileManager) RemoveReturnsOnCall(i int, result1 error) { }{result1} } -func (fake *FakeFileManager) Write(arg1 *os.File, arg2 []byte) error { +func (fake *FakeOSFileManager) Write(arg1 *os.File, arg2 []byte) error { var arg2Copy []byte if arg2 != nil { arg2Copy = make([]byte, len(arg2)) @@ -351,26 +351,26 @@ func (fake *FakeFileManager) Write(arg1 *os.File, arg2 []byte) error { return fakeReturns.result1 } -func (fake *FakeFileManager) WriteCallCount() int { +func (fake *FakeOSFileManager) WriteCallCount() int { fake.writeMutex.RLock() defer fake.writeMutex.RUnlock() return len(fake.writeArgsForCall) } -func (fake *FakeFileManager) WriteCalls(stub func(*os.File, []byte) error) { +func (fake *FakeOSFileManager) WriteCalls(stub func(*os.File, []byte) error) { fake.writeMutex.Lock() defer fake.writeMutex.Unlock() fake.WriteStub = stub } -func (fake *FakeFileManager) WriteArgsForCall(i int) (*os.File, []byte) { +func (fake *FakeOSFileManager) WriteArgsForCall(i int) (*os.File, []byte) { fake.writeMutex.RLock() defer fake.writeMutex.RUnlock() argsForCall := fake.writeArgsForCall[i] return argsForCall.arg1, argsForCall.arg2 } -func (fake *FakeFileManager) WriteReturns(result1 error) { +func (fake *FakeOSFileManager) WriteReturns(result1 error) { fake.writeMutex.Lock() defer fake.writeMutex.Unlock() fake.WriteStub = nil @@ -379,7 +379,7 @@ func (fake *FakeFileManager) WriteReturns(result1 error) { }{result1} } -func (fake *FakeFileManager) WriteReturnsOnCall(i int, result1 error) { +func (fake *FakeOSFileManager) WriteReturnsOnCall(i int, result1 error) { fake.writeMutex.Lock() defer fake.writeMutex.Unlock() fake.WriteStub = nil @@ -393,7 +393,7 @@ func (fake *FakeFileManager) WriteReturnsOnCall(i int, result1 error) { }{result1} } -func (fake *FakeFileManager) Invocations() map[string][][]interface{} { +func (fake *FakeOSFileManager) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() fake.chmodMutex.RLock() @@ -413,7 +413,7 @@ func (fake *FakeFileManager) Invocations() map[string][][]interface{} { return copiedInvocations } -func (fake *FakeFileManager) recordInvocation(key string, args []interface{}) { +func (fake *FakeOSFileManager) recordInvocation(key string, args []interface{}) { fake.invocationsMutex.Lock() defer fake.invocationsMutex.Unlock() if fake.invocations == nil { @@ -425,4 +425,4 @@ func (fake *FakeFileManager) recordInvocation(key string, args []interface{}) { fake.invocations[key] = append(fake.invocations[key], args) } -var _ secrets.FileManager = new(FakeFileManager) +var _ file.OSFileManager = new(FakeOSFileManager) diff --git a/internal/nginx/file/folders.go b/internal/nginx/file/folders.go new file mode 100644 index 0000000000..4e457a0e9a --- /dev/null +++ b/internal/nginx/file/folders.go @@ -0,0 +1,41 @@ +package file + +import ( + "fmt" + "os" + "path/filepath" +) + +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 io/fs.DirEntry + +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . ClearFoldersOSFileManager + +// ClearFoldersOSFileManager is an interface that exposes File I/O operations for ClearFolders. +// Used for unit testing. +type ClearFoldersOSFileManager interface { + // ReadDir returns the directory entries for the directory. + ReadDir(dirname string) ([]os.DirEntry, error) + // Remove removes the file with given name. + Remove(name string) error +} + +// ClearFolders removes all files in the given folders and returns the removed files' full paths. +func ClearFolders(fileMgr ClearFoldersOSFileManager, paths []string) (removedFiles []string, e error) { + for _, path := range paths { + entries, err := fileMgr.ReadDir(path) + if err != nil { + return removedFiles, fmt.Errorf("failed to read directory %q: %w", path, err) + } + + for _, entry := range entries { + path := filepath.Join(path, entry.Name()) + if err := fileMgr.Remove(path); err != nil { + return removedFiles, fmt.Errorf("failed to remove %q: %w", path, err) + } + + removedFiles = append(removedFiles, path) + } + } + + return removedFiles, nil +} diff --git a/internal/nginx/file/folders_test.go b/internal/nginx/file/folders_test.go new file mode 100644 index 0000000000..d9ba1a03a8 --- /dev/null +++ b/internal/nginx/file/folders_test.go @@ -0,0 +1,90 @@ +package file_test + +import ( + "errors" + "os" + "path/filepath" + "testing" + + . "github.com/onsi/gomega" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file/filefakes" +) + +func writeFile(t *testing.T, name string, data []byte) { + t.Helper() + + //nolint:gosec // the file permission is ok for unit testing + if err := os.WriteFile(name, data, 0o644); err != nil { + t.Fatal(err) + } +} + +func TestClearFoldersRemoves(t *testing.T) { + g := NewGomegaWithT(t) + + tempDir := t.TempDir() + + path1 := filepath.Join(tempDir, "path1") + writeFile(t, path1, []byte("test")) + path2 := filepath.Join(tempDir, "path2") + writeFile(t, path2, []byte("test")) + + removedFiles, err := file.ClearFolders(file.NewStdLibOSFileManager(), []string{tempDir}) + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(removedFiles).To(ConsistOf(path1, path2)) + + entries, err := os.ReadDir(tempDir) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(entries).To(BeEmpty()) +} + +func TestClearFoldersFails(t *testing.T) { + files := []string{"file"} + + testErr := errors.New("test error") + + tests := []struct { + fileMgr *filefakes.FakeClearFoldersOSFileManager + name string + }{ + { + fileMgr: &filefakes.FakeClearFoldersOSFileManager{ + ReadDirStub: func(dirname string) ([]os.DirEntry, error) { + return nil, testErr + }, + }, + name: "ReadDir fails", + }, + { + fileMgr: &filefakes.FakeClearFoldersOSFileManager{ + ReadDirStub: func(dirname string) ([]os.DirEntry, error) { + return []os.DirEntry{ + &filefakes.FakeDirEntry{ + NameStub: func() string { + return "file" + }, + }, + }, nil + }, + RemoveStub: func(name string) error { + return testErr + }, + }, + name: "Remove fails", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + removedFiles, err := file.ClearFolders(test.fileMgr, files) + + g.Expect(err).To(MatchError(testErr)) + g.Expect(removedFiles).To(BeNil()) + }) + } +} diff --git a/internal/nginx/file/manager.go b/internal/nginx/file/manager.go index 2fbed7fdd9..e991a455d9 100644 --- a/internal/nginx/file/manager.go +++ b/internal/nginx/file/manager.go @@ -1,49 +1,150 @@ package file import ( + "errors" "fmt" + "io/fs" "os" - "path/filepath" + + "github.com/go-logr/logr" +) + +const ( + // secretFileMode defines the default file mode for files with secrets. + secretFileMode = 0o600 +) + +// Type is the type of File. +type Type int + +func (t Type) String() string { + switch t { + case TypeRegular: + return "Regular" + case TypeSecret: + return "Secret" + default: + return fmt.Sprintf("Unknown Type %d", t) + } +} + +const ( + // TypeRegular is the type for regular configuration files. + TypeRegular Type = iota + // TypeSecret is the type for secret files. + TypeSecret ) -const confdFolder = "/etc/nginx/conf.d" +// File is a file that is part of NGINX configuration to be written to the file system. +type File struct { + Path string + Content []byte + Type Type +} + +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . OSFileManager + +// OSFileManager is an interface that exposes File I/O operations for ManagerImpl. +// Used for unit testing. +type OSFileManager interface { + // ReadDir returns the directory entries for the directory. + ReadDir(dirname string) ([]fs.DirEntry, error) + // Remove file with given name. + Remove(name string) error + // Create file at the provided filepath. + Create(name string) (*os.File, error) + // Chmod sets the mode of the file. + Chmod(file *os.File, mode os.FileMode) error + // Write writes contents to the file. + Write(file *os.File, contents []byte) error +} //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Manager // Manager manages NGINX configuration files. type Manager interface { - // WriteHTTPConfig writes the http config on the file system. - // The name distinguishes this config among all other configs. For that, it must be unique. - // Note that name is not the name of the corresponding configuration file. - WriteHTTPConfig(name string, cfg []byte) error + // ReplaceFiles replaces the files on the file system with the given files removing any previous files. + ReplaceFiles(files []File) error } // ManagerImpl is an implementation of Manager. -type ManagerImpl struct{} +// Note: It is not thread safe. +type ManagerImpl struct { + logger logr.Logger + osFileManager OSFileManager + lastWrittenPaths []string +} // NewManagerImpl creates a new NewManagerImpl. -func NewManagerImpl() *ManagerImpl { - return &ManagerImpl{} +func NewManagerImpl(logger logr.Logger, osFileManager OSFileManager) *ManagerImpl { + return &ManagerImpl{ + logger: logger, + osFileManager: osFileManager, + } } -func (m *ManagerImpl) WriteHTTPConfig(name string, cfg []byte) error { - path := getPathForConfig(name) +// ReplaceFiles replaces the files on the file system with the given files removing any previous files. +// It panics if a file type is unknown. +func (m *ManagerImpl) ReplaceFiles(files []File) error { + for _, path := range m.lastWrittenPaths { + if err := m.osFileManager.Remove(path); err != nil { + return fmt.Errorf("failed to delete file %q: %w", path, err) + } - file, err := os.Create(path) - if err != nil { - return fmt.Errorf("failed to create server config %s: %w", path, err) + m.logger.Info("deleted file", "path", path) } - defer file.Close() + // In some cases, NGINX reads files in runtime, like a JWK. If you remove such file, NGINX will fail + // any request (return 500 status code) that involves reading the file. + // However, we don't have such files yet, so we're not considering this case. - _, err = file.Write(cfg) - if err != nil { - return fmt.Errorf("failed to write server config %s: %w", path, err) + m.lastWrittenPaths = make([]string, 0, len(files)) + + for _, file := range files { + if err := writeFile(m.osFileManager, file); err != nil { + return fmt.Errorf("failed to write file %q of type %v: %w", file.Path, file.Type, err) + } + + m.lastWrittenPaths = append(m.lastWrittenPaths, file.Path) + m.logger.Info("wrote file", "path", file.Path) } return nil } -func getPathForConfig(name string) string { - return filepath.Join(confdFolder, name+".conf") +func writeFile(fileMgr OSFileManager, file File) error { + ensureType(file.Type) + + f, err := fileMgr.Create(file.Path) + if err != nil { + return fmt.Errorf("failed to create file %q: %w", file.Path, err) + } + + var resultErr error + + defer func() { + if err := f.Close(); err != nil { + resultErr = errors.Join(resultErr, fmt.Errorf("failed to close file %q: %w", file.Path, err)) + } + }() + + if file.Type == TypeSecret { + if err := fileMgr.Chmod(f, secretFileMode); err != nil { + resultErr = fmt.Errorf("failed to set file mode for %q: %w", file.Path, err) + return resultErr + } + } + + if err := fileMgr.Write(f, file.Content); err != nil { + resultErr = fmt.Errorf("failed to write file %q: %w", file.Path, err) + return resultErr + } + + return resultErr +} + +func ensureType(fileType Type) { + if fileType != TypeRegular && fileType != TypeSecret { + panic(fmt.Sprintf("unknown file type %d", fileType)) + } } diff --git a/internal/nginx/file/manager_test.go b/internal/nginx/file/manager_test.go index 6bcd87d408..12252ec527 100644 --- a/internal/nginx/file/manager_test.go +++ b/internal/nginx/file/manager_test.go @@ -1,12 +1,205 @@ -package file +package file_test -import "testing" +import ( + "errors" + "os" + "path/filepath" -func TestGetPathForServerConfig(t *testing.T) { - expected := "/etc/nginx/conf.d/test.example.com.conf" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "sigs.k8s.io/controller-runtime/pkg/log/zap" - result := getPathForConfig("test.example.com") - if result != expected { - t.Errorf("getPathForConfig() returned %q but expected %q", result, expected) - } -} + "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file/filefakes" +) + +var _ = Describe("EventHandler", func() { + Describe("Replace files", Ordered, func() { + var ( + mgr *file.ManagerImpl + tmpDir string + regular1, regular2, regular3, secret file.File + ) + + ensureFiles := func(files []file.File) { + entries, err := os.ReadDir(tmpDir) + Expect(err).ShouldNot(HaveOccurred()) + Expect(entries).Should(HaveLen(len(files))) + + entriesMap := make(map[string]os.DirEntry) + for _, entry := range entries { + entriesMap[entry.Name()] = entry + } + + for _, f := range files { + _, ok := entriesMap[filepath.Base(f.Path)] + Expect(ok).Should(BeTrue()) + + info, err := os.Stat(f.Path) + Expect(err).ShouldNot(HaveOccurred()) + + Expect(info.IsDir()).To(BeFalse()) + + if f.Type == file.TypeRegular { + Expect(info.Mode()).To(Equal(os.FileMode(0o644))) + } else { + Expect(info.Mode()).To(Equal(os.FileMode(0o600))) + } + + bytes, err := os.ReadFile(f.Path) + Expect(err).ShouldNot(HaveOccurred()) + Expect(bytes).To(Equal(f.Content)) + } + } + + ensureNotExist := func(files ...file.File) { + for _, f := range files { + _, err := os.Stat(f.Path) + Expect(os.IsNotExist(err)).To(BeTrue()) + } + } + + BeforeAll(func() { + mgr = file.NewManagerImpl(zap.New(), file.NewStdLibOSFileManager()) + tmpDir = GinkgoT().TempDir() + + regular1 = file.File{ + Type: file.TypeRegular, + Path: filepath.Join(tmpDir, "regular-1.conf"), + Content: []byte("regular-1"), + } + regular2 = file.File{ + Type: file.TypeRegular, + Path: filepath.Join(tmpDir, "regular-2.conf"), + Content: []byte("regular-2"), + } + regular3 = file.File{ + Type: file.TypeRegular, + Path: filepath.Join(tmpDir, "regular-3.conf"), + Content: []byte("regular-3"), + } + secret = file.File{ + Type: file.TypeSecret, + Path: filepath.Join(tmpDir, "secret.conf"), + Content: []byte("secret"), + } + }) + + It("should write initial config", func() { + files := []file.File{regular1, regular2, secret} + + err := mgr.ReplaceFiles(files) + Expect(err).ShouldNot(HaveOccurred()) + + ensureFiles(files) + }) + + It("should write subsequent config", func() { + files := []file.File{ + regular2, // overwriting + regular3, // adding + secret, // overwriting + } + + err := mgr.ReplaceFiles(files) + Expect(err).ShouldNot(HaveOccurred()) + + ensureFiles(files) + ensureNotExist(regular1) + }) + + It("should remove all files", func() { + err := mgr.ReplaceFiles(nil) + Expect(err).ShouldNot(HaveOccurred()) + + ensureNotExist(regular2, regular3, secret) + }) + }) + + When("file type is not supported", func() { + It("should panic", func() { + mgr := file.NewManagerImpl(zap.New(), nil) + + files := []file.File{ + { + Type: 123, + Path: "unsupported.conf", + }, + } + + replace := func() { + _ = mgr.ReplaceFiles(files) + } + + Expect(replace).Should(Panic()) + }) + }) + + Describe("Edge cases with IO errors", func() { + var ( + files = []file.File{ + { + Type: file.TypeRegular, + Path: "regular.conf", + Content: []byte("regular"), + }, + { + Type: file.TypeSecret, + Path: "secret.conf", + Content: []byte("secret"), + }, + } + testErr = errors.New("test error") + ) + + DescribeTable( + "should return error on file IO error", + func(fakeOSMgr *filefakes.FakeOSFileManager) { + mgr := file.NewManagerImpl(zap.New(), fakeOSMgr) + + // special case for Remove + // to kick off removing, we need to successfully write files beforehand + if fakeOSMgr.RemoveStub != nil { + err := mgr.ReplaceFiles(files) + Expect(err).ShouldNot(HaveOccurred()) + } + + err := mgr.ReplaceFiles(files) + Expect(err).Should(HaveOccurred()) + Expect(err).To(MatchError(testErr)) + }, + Entry( + "Remove", + &filefakes.FakeOSFileManager{ + RemoveStub: func(s string) error { + return testErr + }, + }, + ), + Entry( + "Create", + &filefakes.FakeOSFileManager{ + CreateStub: func(s string) (*os.File, error) { + return nil, testErr + }, + }, + ), + Entry( + "Chmod", + &filefakes.FakeOSFileManager{ + ChmodStub: func(os *os.File, mode os.FileMode) error { + return testErr + }, + }, + ), + Entry( + "Write", + &filefakes.FakeOSFileManager{ + WriteStub: func(os *os.File, bytes []byte) error { + return testErr + }, + }, + ), + ) + }) +}) diff --git a/internal/nginx/file/os_filemanager.go b/internal/nginx/file/os_filemanager.go new file mode 100644 index 0000000000..ffac7caa7a --- /dev/null +++ b/internal/nginx/file/os_filemanager.go @@ -0,0 +1,43 @@ +package file + +import ( + "io/fs" + "os" +) + +// StdLibOSFileManager wraps the standard library's file operations. +// Clients can define an interface with all or a subset StdLibOSFileManager methods and use it in their types or +// functions, so that they can be unit tested. +// It is expected that clients generate fakes. +type StdLibOSFileManager struct{} + +func NewStdLibOSFileManager() *StdLibOSFileManager { + return &StdLibOSFileManager{} +} + +// ReadDir wraps os.ReadDir. +func (s *StdLibOSFileManager) ReadDir(dirname string) ([]fs.DirEntry, error) { + return os.ReadDir(dirname) +} + +// Remove wraps os.Remove. +func (s *StdLibOSFileManager) Remove(name string) error { + return os.Remove(name) +} + +// Write wraps os.File.Write +func (s *StdLibOSFileManager) Write(file *os.File, contents []byte) error { + _, err := file.Write(contents) + + return err +} + +// Create wraps os.Create. +func (s *StdLibOSFileManager) Create(name string) (*os.File, error) { + return os.Create(name) +} + +// Chmod wraps os.File.Chmod. +func (s *StdLibOSFileManager) Chmod(file *os.File, mode os.FileMode) error { + return file.Chmod(mode) +} diff --git a/internal/state/change_processor.go b/internal/state/change_processor.go index 72715d1320..6ec5269ec4 100644 --- a/internal/state/change_processor.go +++ b/internal/state/change_processor.go @@ -19,7 +19,6 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/graph" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/relationship" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/validation" ) @@ -50,8 +49,6 @@ type ChangeProcessor interface { // ChangeProcessorConfig holds configuration parameters for ChangeProcessorImpl. type ChangeProcessorConfig struct { - // SecretMemoryManager is the secret memory manager. - SecretMemoryManager secrets.SecretDiskMemoryManager // RelationshipCapturer captures relationships between Kubernetes API resources and Gateway API resources. RelationshipCapturer relationship.Capturer // Validators validate resources according to data-plane specific rules. @@ -70,6 +67,8 @@ type ChangeProcessorConfig struct { // ChangeProcessorImpl is an implementation of ChangeProcessor. type ChangeProcessorImpl struct { + latestGraph *graph.Graph + // clusterState holds the current state of the cluster clusterState graph.ClusterState // updater acts upon the cluster state. @@ -77,8 +76,7 @@ type ChangeProcessorImpl struct { // getAndResetClusterStateChanged tells if the cluster state has changed. getAndResetClusterStateChanged func() bool - cfg ChangeProcessorConfig - + cfg ChangeProcessorConfig lock sync.Mutex } @@ -91,6 +89,7 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { Services: make(map[types.NamespacedName]*apiv1.Service), Namespaces: make(map[types.NamespacedName]*apiv1.Namespace), ReferenceGrants: make(map[types.NamespacedName]*v1beta1.ReferenceGrant), + Secrets: make(map[types.NamespacedName]*apiv1.Secret), } extractGVK := func(obj client.Object) schema.GroupVersionKind { @@ -101,8 +100,18 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { return gvk } + processor := &ChangeProcessorImpl{ + cfg: cfg, + clusterState: clusterStore, + } + + triggerStateChange := func(objType client.Object, nsname types.NamespacedName) bool { + return processor.latestGraph != nil && processor.latestGraph.IsReferenced(objType, nsname) + } + trackingUpdater := newChangeTrackingUpdater( cfg.RelationshipCapturer, + triggerStateChange, extractGVK, []changeTrackingUpdaterObjectTypeCfg{ { @@ -140,6 +149,11 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { store: nil, trackUpsertDelete: false, }, + { + gvk: extractGVK(&apiv1.Secret{}), + store: newObjectStoreMapAdapter(clusterStore.Secrets), + trackUpsertDelete: false, + }, }, ) @@ -169,12 +183,10 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { }, ) - return &ChangeProcessorImpl{ - cfg: cfg, - getAndResetClusterStateChanged: trackingUpdater.getAndResetChangedStatus, - updater: updater, - clusterState: clusterStore, - } + processor.getAndResetClusterStateChanged = trackingUpdater.getAndResetChangedStatus + processor.updater = updater + + return processor } // Currently, changes (upserts/delete) trigger rebuilding of the configuration, even if the change doesn't change @@ -210,13 +222,12 @@ func (c *ChangeProcessorImpl) Process() (bool, *graph.Graph) { return false, nil } - graphCfg := graph.BuildGraph( + c.latestGraph = graph.BuildGraph( c.clusterState, c.cfg.GatewayCtlrName, c.cfg.GatewayClassName, - c.cfg.SecretMemoryManager, c.cfg.Validators, ) - return true, graphCfg + return true, c.latestGraph } diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go index 2c11ab3c6c..39d38e5c8c 100644 --- a/internal/state/change_processor_test.go +++ b/internal/state/change_processor_test.go @@ -23,15 +23,13 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/graph" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/relationship" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/relationship/relationshipfakes" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets/secretsfakes" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/validation" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/validation/validationfakes" ) const ( - controllerName = "my.controller" - gcName = "test-class" - certificatePath = "path/to/cert" + controllerName = "my.controller" + gcName = "test-class" ) func createRoute( @@ -106,7 +104,7 @@ func createGateway(name string) *v1beta1.Gateway { } } -func createGatewayWithTLSListener(name, certNs string) *v1beta1.Gateway { +func createGatewayWithTLSListener(name string, tlsSecret *apiv1.Secret) *v1beta1.Gateway { gw := createGateway(name) l := v1beta1.Listener{ @@ -119,8 +117,8 @@ func createGatewayWithTLSListener(name, certNs string) *v1beta1.Gateway { CertificateRefs: []v1beta1.SecretObjectReference{ { Kind: (*v1beta1.Kind)(helpers.GetPointer("Secret")), - Name: "secret", - Namespace: (*v1beta1.Namespace)(helpers.GetPointer(certNs)), + Name: v1beta1.ObjectName(tlsSecret.Name), + Namespace: (*v1beta1.Namespace)(&tlsSecret.Namespace), }, }, }, @@ -189,6 +187,56 @@ func createScheme() *runtime.Scheme { return scheme } +var ( + cert = []byte(`-----BEGIN CERTIFICATE----- +MIIDLjCCAhYCCQDAOF9tLsaXWjANBgkqhkiG9w0BAQsFADBaMQswCQYDVQQGEwJV +UzELMAkGA1UECAwCQ0ExITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0 +ZDEbMBkGA1UEAwwSY2FmZS5leGFtcGxlLmNvbSAgMB4XDTE4MDkxMjE2MTUzNVoX +DTIzMDkxMTE2MTUzNVowWDELMAkGA1UEBhMCVVMxCzAJBgNVBAgMAkNBMSEwHwYD +VQQKDBhJbnRlcm5ldCBXaWRnaXRzIFB0eSBMdGQxGTAXBgNVBAMMEGNhZmUuZXhh +bXBsZS5jb20wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCp6Kn7sy81 +p0juJ/cyk+vCAmlsfjtFM2muZNK0KtecqG2fjWQb55xQ1YFA2XOSwHAYvSdwI2jZ +ruW8qXXCL2rb4CZCFxwpVECrcxdjm3teViRXVsYImmJHPPSyQgpiobs9x7DlLc6I +BA0ZjUOyl0PqG9SJexMV73WIIa5rDVSF2r4kSkbAj4Dcj7LXeFlVXH2I5XwXCptC +n67JCg42f+k8wgzcRVp8XZkZWZVjwq9RUKDXmFB2YyN1XEWdZ0ewRuKYUJlsm692 +skOrKQj0vkoPn41EE/+TaVEpqLTRoUY3rzg7DkdzfdBizFO2dsPNFx2CW0jXkNLv +Ko25CZrOhXAHAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAKHFCcyOjZvoHswUBMdL +RdHIb383pWFynZq/LuUovsVA58B0Cg7BEfy5vWVVrq5RIkv4lZ81N29x21d1JH6r +jSnQx+DXCO/TJEV5lSCUpIGzEUYaUPgRyjsM/NUdCJ8uHVhZJ+S6FA+CnOD9rn2i +ZBePCI5rHwEXwnnl8ywij3vvQ5zHIuyBglWr/Qyui9fjPpwWUvUm4nv5SMG9zCV7 +PpuwvuatqjO1208BjfE/cZHIg8Hw9mvW9x9C+IQMIMDE7b/g6OcK7LGTLwlFxvA8 +7WjEequnayIphMhKRXVf1N349eN98Ez38fOTHTPbdJjFA/PcC+Gyme+iGt5OQdFh +yRE= +-----END CERTIFICATE-----`) + key = []byte(`-----BEGIN RSA PRIVATE KEY----- +MIIEowIBAAKCAQEAqeip+7MvNadI7if3MpPrwgJpbH47RTNprmTStCrXnKhtn41k +G+ecUNWBQNlzksBwGL0ncCNo2a7lvKl1wi9q2+AmQhccKVRAq3MXY5t7XlYkV1bG +CJpiRzz0skIKYqG7Pcew5S3OiAQNGY1DspdD6hvUiXsTFe91iCGuaw1Uhdq+JEpG +wI+A3I+y13hZVVx9iOV8FwqbQp+uyQoONn/pPMIM3EVafF2ZGVmVY8KvUVCg15hQ +dmMjdVxFnWdHsEbimFCZbJuvdrJDqykI9L5KD5+NRBP/k2lRKai00aFGN684Ow5H +c33QYsxTtnbDzRcdgltI15DS7yqNuQmazoVwBwIDAQABAoIBAQCPSdSYnQtSPyql +FfVFpTOsoOYRhf8sI+ibFxIOuRauWehhJxdm5RORpAzmCLyL5VhjtJme223gLrw2 +N99EjUKb/VOmZuDsBc6oCF6QNR58dz8cnORTewcotsJR1pn1hhlnR5HqJJBJask1 +ZEnUQfcXZrL94lo9JH3E+Uqjo1FFs8xxE8woPBqjZsV7pRUZgC3LhxnwLSExyFo4 +cxb9SOG5OmAJozStFoQ2GJOes8rJ5qfdvytgg9xbLaQL/x0kpQ62BoFMBDdqOePW +KfP5zZ6/07/vpj48yA1Q32PzobubsBLd3Kcn32jfm1E7prtWl+JeOFiOznBQFJbN +4qPVRz5hAoGBANtWyxhNCSLu4P+XgKyckljJ6F5668fNj5CzgFRqJ09zn0TlsNro +FTLZcxDqnR3HPYM42JERh2J/qDFZynRQo3cg3oeivUdBVGY8+FI1W0qdub/L9+yu +edOZTQ5XmGGp6r6jexymcJim/OsB3ZnYOpOrlD7SPmBvzNLk4MF6gxbXAoGBAMZO +0p6HbBmcP0tjFXfcKE77ImLm0sAG4uHoUx0ePj/2qrnTnOBBNE4MvgDuTJzy+caU +k8RqmdHCbHzTe6fzYq/9it8sZ77KVN1qkbIcuc+RTxA9nNh1TjsRne74Z0j1FCLk +hHcqH0ri7PYSKHTE8FvFCxZYdbuB84CmZihvxbpRAoGAIbjqaMYPTYuklCda5S79 +YSFJ1JzZe1Kja//tDw1zFcgVCKa31jAwciz0f/lSRq3HS1GGGmezhPVTiqLfeZqc +R0iKbhgbOcVVkJJ3K0yAyKwPTumxKHZ6zImZS0c0am+RY9YGq5T7YrzpzcfvpiOU +ffe3RyFT7cfCmfoOhDCtzukCgYB30oLC1RLFOrqn43vCS51zc5zoY44uBzspwwYN +TwvP/ExWMf3VJrDjBCH+T/6sysePbJEImlzM+IwytFpANfiIXEt/48Xf60Nx8gWM +uHyxZZx/NKtDw0V8vX1POnq2A5eiKa+8jRARYKJLYNdfDuwolxvG6bZhkPi/4EtT +3Y18sQKBgHtKbk+7lNJVeswXE5cUG6EDUsDe/2Ua7fXp7FcjqBEoap1LSw+6TXp0 +ZgrmKE8ARzM47+EJHUviiq/nupE15g0kJW3syhpU9zZLO7ltB0KIkO9ZRcmUjo8Q +cpLlHMAqbLJ8WYGJCkhiWxyal6hYTyWY4cVkC0xtTl/hUE9IeNKo +-----END RSA PRIVATE KEY-----`) +) + var _ = Describe("ChangeProcessor", func() { // graph outputs are large, so allow gomega to print everything on test failure format.MaxLength = 0 @@ -203,35 +251,30 @@ var _ = Describe("ChangeProcessor", func() { ControllerName: controllerName, }, } - processor state.ChangeProcessor - fakeSecretMemoryMgr *secretsfakes.FakeSecretDiskMemoryManager + processor state.ChangeProcessor ) BeforeEach(OncePerOrdered, func() { - fakeSecretMemoryMgr = &secretsfakes.FakeSecretDiskMemoryManager{} - processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ GatewayCtlrName: controllerName, GatewayClassName: gcName, - SecretMemoryManager: fakeSecretMemoryMgr, RelationshipCapturer: relationship.NewCapturerImpl(), Logger: zap.New(), Validators: createAlwaysValidValidators(), Scheme: createScheme(), }) - - fakeSecretMemoryMgr.RequestReturns(certificatePath, nil) }) Describe("Process gateway resources", Ordered, func() { var ( - gcUpdated *v1beta1.GatewayClass - hr1, hr1Updated, hr2 *v1beta1.HTTPRoute - gw1, gw1Updated, gw2 *v1beta1.Gateway - refGrant1, refGrant2 *v1beta1.ReferenceGrant - expGraph *graph.Graph - expRouteHR1, expRouteHR2 *graph.Route - hr1Name, hr2Name types.NamespacedName + gcUpdated *v1beta1.GatewayClass + diffNsTLSSecret, sameNsTLSSecret *apiv1.Secret + hr1, hr1Updated, hr2 *v1beta1.HTTPRoute + gw1, gw1Updated, gw2 *v1beta1.Gateway + refGrant1, refGrant2 *v1beta1.ReferenceGrant + expGraph *graph.Graph + expRouteHR1, expRouteHR2 *graph.Route + hr1Name, hr2Name types.NamespacedName ) BeforeAll(func() { gcUpdated = gc.DeepCopy() @@ -257,8 +300,6 @@ var _ = Describe("ChangeProcessor", func() { hr2 = createRoute("hr-2", "gateway-2", "bar.example.com") hr2Name = types.NamespacedName{Namespace: "test", Name: "hr-2"} - gw1 = createGatewayWithTLSListener("gateway-1", "cert-ns") // cert in diff namespace than gw - refGrant1 = &v1beta1.ReferenceGrant{ ObjectMeta: metav1.ObjectMeta{ Namespace: "cert-ns", @@ -301,10 +342,36 @@ var _ = Describe("ChangeProcessor", func() { }, } + sameNsTLSSecret = &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tls-secret", + Namespace: "test", + }, + Type: apiv1.SecretTypeTLS, + Data: map[string][]byte{ + apiv1.TLSCertKey: cert, + apiv1.TLSPrivateKeyKey: key, + }, + } + + diffNsTLSSecret = &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "different-ns-tls-secret", + Namespace: "cert-ns", + }, + Type: apiv1.SecretTypeTLS, + Data: map[string][]byte{ + apiv1.TLSCertKey: cert, + apiv1.TLSPrivateKeyKey: key, + }, + } + + gw1 = createGatewayWithTLSListener("gateway-1", diffNsTLSSecret) // cert in diff namespace than gw + gw1Updated = gw1.DeepCopy() gw1Updated.Generation++ - gw2 = createGatewayWithTLSListener("gateway-2", "test") + gw2 = createGatewayWithTLSListener("gateway-2", sameNsTLSSecret) }) BeforeEach(func() { expRouteHR1 = &graph.Route{ @@ -391,7 +458,7 @@ var _ = Describe("ChangeProcessor", func() { Routes: map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "hr-1"}: expRouteHR1, }, - SecretPath: "path/to/cert", + ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(diffNsTLSSecret)), }, }, Valid: true, @@ -400,11 +467,12 @@ var _ = Describe("ChangeProcessor", func() { Routes: map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "hr-1"}: expRouteHR1, }, + ReferencedSecrets: map[types.NamespacedName]*graph.Secret{}, } }) When("no upsert has occurred", func() { - It("returns empty graph", func() { + It("returns nil graph", func() { changed, graphCfg := processor.Process() Expect(changed).To(BeFalse()) Expect(graphCfg).To(BeNil()) @@ -418,7 +486,16 @@ var _ = Describe("ChangeProcessor", func() { changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) - Expect(graphCfg).To(Equal(&graph.Graph{})) + Expect(helpers.Diff(&graph.Graph{}, graphCfg)).To(BeEmpty()) + }) + }) + When("the different namespace TLS Secret is upserted", func() { + It("returns nil graph", func() { + processor.CaptureUpsertChange(diffNsTLSSecret) + + changed, graphCfg := processor.Process() + Expect(changed).To(BeFalse()) + Expect(graphCfg).To(BeNil()) }) }) When("the first Gateway is upserted", func() { @@ -446,6 +523,8 @@ var _ = Describe("ChangeProcessor", func() { FailedCondition: conditions.NewRouteInvalidGateway(), } + expGraph.ReferencedSecrets = nil + changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) @@ -463,7 +542,7 @@ var _ = Describe("ChangeProcessor", func() { Valid: false, Routes: map[types.NamespacedName]*graph.Route{}, Conditions: conditions.NewListenerRefNotPermitted( - "Certificate ref to secret cert-ns/secret not permitted by any ReferenceGrant", + "Certificate ref to secret cert-ns/different-ns-tls-secret not permitted by any ReferenceGrant", ), } @@ -483,6 +562,8 @@ var _ = Describe("ChangeProcessor", func() { ), } + expGraph.ReferencedSecrets = nil + changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) @@ -498,6 +579,9 @@ var _ = Describe("ChangeProcessor", func() { "Backend ref to Service service-ns/service not permitted by any ReferenceGrant", ), } + expGraph.ReferencedSecrets[client.ObjectKeyFromObject(diffNsTLSSecret)] = &graph.Secret{ + Source: diffNsTLSSecret, + } changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) @@ -509,13 +593,17 @@ var _ = Describe("ChangeProcessor", func() { It("returns updated graph", func() { processor.CaptureUpsertChange(refGrant2) + expGraph.ReferencedSecrets[client.ObjectKeyFromObject(diffNsTLSSecret)] = &graph.Secret{ + Source: diffNsTLSSecret, + } + changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) }) }) When("the first HTTPRoute without a generation changed is processed", func() { - It("returns empty graph", func() { + It("returns nil graph", func() { hr1UpdatedSameGen := hr1.DeepCopy() // hr1UpdatedSameGen.Generation has not been changed processor.CaptureUpsertChange(hr1UpdatedSameGen) @@ -531,6 +619,9 @@ var _ = Describe("ChangeProcessor", func() { expGraph.Gateway.Listeners["listener-443-1"].Routes[hr1Name].Source.Generation = hr1Updated.Generation expGraph.Gateway.Listeners["listener-80-1"].Routes[hr1Name].Source.Generation = hr1Updated.Generation + expGraph.ReferencedSecrets[client.ObjectKeyFromObject(diffNsTLSSecret)] = &graph.Secret{ + Source: diffNsTLSSecret, + } changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) @@ -539,7 +630,7 @@ var _ = Describe("ChangeProcessor", func() { ) }) When("the first Gateway update without generation changed is processed", func() { - It("returns empty graph", func() { + It("returns nil graph", func() { gwUpdatedSameGen := gw1.DeepCopy() // gwUpdatedSameGen.Generation has not been changed processor.CaptureUpsertChange(gwUpdatedSameGen) @@ -554,6 +645,9 @@ var _ = Describe("ChangeProcessor", func() { processor.CaptureUpsertChange(gw1Updated) expGraph.Gateway.Source.Generation = gw1Updated.Generation + expGraph.ReferencedSecrets[client.ObjectKeyFromObject(diffNsTLSSecret)] = &graph.Secret{ + Source: diffNsTLSSecret, + } changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) @@ -561,7 +655,7 @@ var _ = Describe("ChangeProcessor", func() { }) }) When("the GatewayClass update without generation change is processed", func() { - It("returns empty graph", func() { + It("returns nil graph", func() { gcUpdatedSameGen := gc.DeepCopy() // gcUpdatedSameGen.Generation has not been changed processor.CaptureUpsertChange(gcUpdatedSameGen) @@ -576,6 +670,22 @@ var _ = Describe("ChangeProcessor", func() { processor.CaptureUpsertChange(gcUpdated) expGraph.GatewayClass.Source.Generation = gcUpdated.Generation + expGraph.ReferencedSecrets[client.ObjectKeyFromObject(diffNsTLSSecret)] = &graph.Secret{ + Source: diffNsTLSSecret, + } + + changed, graphCfg := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) + }) + }) + When("the different namespace TLS secret is upserted again", func() { + It("returns populated graph", func() { + processor.CaptureUpsertChange(diffNsTLSSecret) + + expGraph.ReferencedSecrets[client.ObjectKeyFromObject(diffNsTLSSecret)] = &graph.Secret{ + Source: diffNsTLSSecret, + } changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) @@ -583,7 +693,17 @@ var _ = Describe("ChangeProcessor", func() { }) }) When("no changes are captured", func() { - It("returns empty graph", func() { + It("returns nil graph", func() { + changed, graphCfg := processor.Process() + + Expect(changed).To(BeFalse()) + Expect(graphCfg).To(BeNil()) + }) + }) + When("the same namespace TLS Secret is upserted", func() { + It("returns nil graph", func() { + processor.CaptureUpsertChange(sameNsTLSSecret) + changed, graphCfg := processor.Process() Expect(changed).To(BeFalse()) @@ -597,6 +717,9 @@ var _ = Describe("ChangeProcessor", func() { expGraph.IgnoredGateways = map[types.NamespacedName]*v1beta1.Gateway{ {Namespace: "test", Name: "gateway-2"}: gw2, } + expGraph.ReferencedSecrets[client.ObjectKeyFromObject(diffNsTLSSecret)] = &graph.Secret{ + Source: diffNsTLSSecret, + } changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) @@ -619,6 +742,9 @@ var _ = Describe("ChangeProcessor", func() { AcceptedHostnames: map[string][]string{}, FailedCondition: conditions.NewTODO("Gateway is ignored"), } + expGraph.ReferencedSecrets[client.ObjectKeyFromObject(diffNsTLSSecret)] = &graph.Secret{ + Source: diffNsTLSSecret, + } changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) @@ -641,9 +767,13 @@ var _ = Describe("ChangeProcessor", func() { delete(expGraph.Gateway.Listeners["listener-443-1"].Routes, hr1Name) expGraph.Gateway.Listeners["listener-80-1"].Routes[hr2Name] = expRouteHR2 expGraph.Gateway.Listeners["listener-443-1"].Routes[hr2Name] = expRouteHR2 - delete(expGraph.Routes, hr1Name) expGraph.Routes[hr2Name] = expRouteHR2 + sameNsTLSSecretRef := helpers.GetPointer(client.ObjectKeyFromObject(sameNsTLSSecret)) + expGraph.Gateway.Listeners["listener-443-1"].ResolvedSecret = sameNsTLSSecretRef + expGraph.ReferencedSecrets[client.ObjectKeyFromObject(sameNsTLSSecret)] = &graph.Secret{ + Source: sameNsTLSSecret, + } changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) @@ -665,6 +795,11 @@ var _ = Describe("ChangeProcessor", func() { delete(expGraph.Gateway.Listeners["listener-80-1"].Routes, hr1Name) delete(expGraph.Gateway.Listeners["listener-443-1"].Routes, hr1Name) expGraph.Routes = map[types.NamespacedName]*graph.Route{} + sameNsTLSSecretRef := helpers.GetPointer(client.ObjectKeyFromObject(sameNsTLSSecret)) + expGraph.Gateway.Listeners["listener-443-1"].ResolvedSecret = sameNsTLSSecretRef + expGraph.ReferencedSecrets[client.ObjectKeyFromObject(sameNsTLSSecret)] = &graph.Secret{ + Source: sameNsTLSSecret, + } changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) @@ -684,6 +819,7 @@ var _ = Describe("ChangeProcessor", func() { Conditions: conditions.NewGatewayInvalid("GatewayClass doesn't exist"), } expGraph.Routes = map[types.NamespacedName]*graph.Route{} + expGraph.ReferencedSecrets = nil changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) @@ -691,31 +827,27 @@ var _ = Describe("ChangeProcessor", func() { }) }) When("the second Gateway is deleted", func() { - It("returns updated graph", func() { + It("returns empty graph", func() { processor.CaptureDeleteChange( &v1beta1.Gateway{}, types.NamespacedName{Namespace: "test", Name: "gateway-2"}, ) - expGraph := &graph.Graph{} - changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) - Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) + Expect(helpers.Diff(&graph.Graph{}, graphCfg)).To(BeEmpty()) }) }) When("the first HTTPRoute is deleted", func() { - It("returns updated graph", func() { + It("returns empty graph", func() { processor.CaptureDeleteChange( &v1beta1.HTTPRoute{}, types.NamespacedName{Namespace: "test", Name: "hr-1"}, ) - expGraph := &graph.Graph{} - changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) - Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) + Expect(helpers.Diff(&graph.Graph{}, graphCfg)).To(BeEmpty()) }) }) }) @@ -1109,23 +1241,22 @@ var _ = Describe("ChangeProcessor", func() { processor *state.ChangeProcessorImpl fakeRelationshipCapturer *relationshipfakes.FakeCapturer gcNsName, gwNsName, hrNsName, hr2NsName, rgNsName types.NamespacedName - svcNsName, sliceNsName types.NamespacedName + svcNsName, sliceNsName, secretNsName types.NamespacedName gc, gcUpdated *v1beta1.GatewayClass gw1, gw1Updated, gw2 *v1beta1.Gateway hr1, hr1Updated, hr2 *v1beta1.HTTPRoute rg1, rg1Updated, rg2 *v1beta1.ReferenceGrant svc *apiv1.Service slice *discoveryV1.EndpointSlice + secret *apiv1.Secret ) BeforeEach(OncePerOrdered, func() { - fakeSecretMemoryMgr := &secretsfakes.FakeSecretDiskMemoryManager{} fakeRelationshipCapturer = &relationshipfakes.FakeCapturer{} processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ GatewayCtlrName: "test.controller", GatewayClassName: "my-class", - SecretMemoryManager: fakeSecretMemoryMgr, RelationshipCapturer: fakeRelationshipCapturer, Validators: createAlwaysValidValidators(), Scheme: createScheme(), @@ -1209,6 +1340,15 @@ var _ = Describe("ChangeProcessor", func() { rg2 = rg1.DeepCopy() rg2.Name = "rg-2" + + secretNsName = types.NamespacedName{Namespace: "test", Name: "test-secret"} + + secret = &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: secretNsName.Namespace, + Name: secretNsName.Name, + }, + } }) // Changing change - a change that makes processor.Process() report changed // Non-changing change - a change that doesn't do that @@ -1300,6 +1440,8 @@ var _ = Describe("ChangeProcessor", func() { }) }) Describe("Multiple Kubernetes API resource changes", Ordered, func() { + // Note: because secret resource is not used by the real relationship.Capturer, it is not used + // in the same way as service and endpoint slice in the tests below. It("should report changed after multiple Upserts of related resources", func() { fakeRelationshipCapturer.ExistsReturns(true) processor.CaptureUpsertChange(svc) @@ -1314,6 +1456,8 @@ var _ = Describe("ChangeProcessor", func() { processor.CaptureUpsertChange(svc) processor.CaptureUpsertChange(slice) + processor.CaptureUpsertChange(secret) + changed, _ := processor.Process() Expect(changed).To(BeFalse()) }) @@ -1328,6 +1472,7 @@ var _ = Describe("ChangeProcessor", func() { fakeRelationshipCapturer.ExistsReturns(false) processor.CaptureUpsertChange(svc) processor.CaptureUpsertChange(slice) + processor.CaptureUpsertChange(secret) changed, _ := processor.Process() Expect(changed).To(BeTrue()) @@ -1344,6 +1489,7 @@ var _ = Describe("ChangeProcessor", func() { fakeRelationshipCapturer.ExistsReturns(false) processor.CaptureUpsertChange(svc) processor.CaptureUpsertChange(slice) + processor.CaptureUpsertChange(secret) changed, _ := processor.Process() Expect(changed).To(BeTrue()) @@ -1380,6 +1526,7 @@ var _ = Describe("ChangeProcessor", func() { fakeRelationshipCapturer.ExistsReturns(false) processor.CaptureUpsertChange(svc) processor.CaptureUpsertChange(slice) + processor.CaptureUpsertChange(secret) changed, _ := processor.Process() Expect(changed).To(BeFalse()) @@ -1398,6 +1545,7 @@ var _ = Describe("ChangeProcessor", func() { processor.CaptureUpsertChange(gw1) processor.CaptureUpsertChange(hr1) processor.CaptureUpsertChange(rg1) + processor.CaptureUpsertChange(secret) changed, _ := processor.Process() Expect(changed).To(BeTrue()) @@ -1416,6 +1564,7 @@ var _ = Describe("ChangeProcessor", func() { // these are non-changing changes processor.CaptureUpsertChange(svc) processor.CaptureUpsertChange(slice) + processor.CaptureUpsertChange(secret) changed, _ := processor.Process() Expect(changed).To(BeTrue()) @@ -1435,6 +1584,7 @@ var _ = Describe("ChangeProcessor", func() { processor.CaptureUpsertChange(gw1Updated) processor.CaptureUpsertChange(hr1Updated) processor.CaptureUpsertChange(rg1Updated) + processor.CaptureUpsertChange(secret) changed, _ := processor.Process() Expect(changed).To(BeTrue()) @@ -1445,9 +1595,8 @@ var _ = Describe("ChangeProcessor", func() { Describe("Webhook validation cases", Ordered, func() { var ( - processor state.ChangeProcessor - fakeEventRecorder *record.FakeRecorder - fakeSecretMemoryMgr *secretsfakes.FakeSecretDiskMemoryManager + processor state.ChangeProcessor + fakeEventRecorder *record.FakeRecorder gc *v1beta1.GatewayClass @@ -1456,13 +1605,11 @@ var _ = Describe("ChangeProcessor", func() { hr, hrInvalid *v1beta1.HTTPRoute ) BeforeAll(func() { - fakeSecretMemoryMgr = &secretsfakes.FakeSecretDiskMemoryManager{} fakeEventRecorder = record.NewFakeRecorder(2 /* number of buffered events */) processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ GatewayCtlrName: controllerName, GatewayClassName: gcName, - SecretMemoryManager: fakeSecretMemoryMgr, RelationshipCapturer: relationship.NewCapturerImpl(), Logger: zap.New(), Validators: createAlwaysValidValidators(), @@ -1631,13 +1778,11 @@ var _ = Describe("ChangeProcessor", func() { var processor state.ChangeProcessor BeforeEach(func() { - fakeSecretMemoryMgr = &secretsfakes.FakeSecretDiskMemoryManager{} fakeEventRecorder = record.NewFakeRecorder(1 /* number of buffered events */) processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ GatewayCtlrName: controllerName, GatewayClassName: gcName, - SecretMemoryManager: fakeSecretMemoryMgr, RelationshipCapturer: relationship.NewCapturerImpl(), Logger: zap.New(), Validators: createAlwaysValidValidators(), @@ -1763,18 +1908,15 @@ var _ = Describe("ChangeProcessor", func() { Describe("Edge cases with panic", func() { var ( processor state.ChangeProcessor - fakeSecretMemoryMgr *secretsfakes.FakeSecretDiskMemoryManager fakeRelationshipCapturer *relationshipfakes.FakeCapturer ) BeforeEach(func() { - fakeSecretMemoryMgr = &secretsfakes.FakeSecretDiskMemoryManager{} fakeRelationshipCapturer = &relationshipfakes.FakeCapturer{} processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ GatewayCtlrName: "test.controller", GatewayClassName: "my-class", - SecretMemoryManager: fakeSecretMemoryMgr, RelationshipCapturer: fakeRelationshipCapturer, Validators: createAlwaysValidValidators(), Scheme: createScheme(), diff --git a/internal/state/dataplane/configuration.go b/internal/state/dataplane/configuration.go index 46f34131af..d3124fb7c6 100644 --- a/internal/state/dataplane/configuration.go +++ b/internal/state/dataplane/configuration.go @@ -5,6 +5,7 @@ import ( "fmt" "sort" + apiv1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/gateway-api/apis/v1beta1" @@ -22,6 +23,8 @@ const ( // Configuration is an intermediate representation of dataplane configuration. type Configuration struct { + // SSLKeyPairs holds all unique SSLKeyPairs. + SSLKeyPairs map[SSLKeyPairID]SSLKeyPair // HTTPServers holds all HTTPServers. HTTPServers []VirtualServer // SSLServers holds all SSLServers. @@ -32,9 +35,18 @@ type Configuration struct { BackendGroups []BackendGroup } +// SSLKeyPairID is a unique identifier for a SSLKeyPair. +// The ID is safe to use as a file name. +type SSLKeyPairID string + +// SSLKeyPair is an SSL private/public key pair. +type SSLKeyPair struct { + Cert, Key []byte +} + // VirtualServer is a virtual server. type VirtualServer struct { - // SSL holds the SSL configuration options for the server. + // SSL holds the SSL configuration for the server. SSL *SSL // Hostname is the hostname of the server. Hostname string @@ -46,6 +58,7 @@ type VirtualServer struct { Port int32 } +// Upstream is a pool of endpoints to be load balanced. type Upstream struct { // Name is the name of the Upstream. Will be unique for each service/port combination. Name string @@ -55,9 +68,9 @@ type Upstream struct { Endpoints []resolver.Endpoint } +// SSL is the SSL configuration for a server. type SSL struct { - // CertificatePath is the path to the certificate file. - CertificatePath string + KeyPairID SSLKeyPairID } // PathRule represents routing rules that share a common path. @@ -157,17 +170,43 @@ func BuildConfiguration(ctx context.Context, g *graph.Graph, resolver resolver.S upstreams := buildUpstreams(ctx, g.Gateway.Listeners, resolver) httpServers, sslServers := buildServers(g.Gateway.Listeners) backendGroups := buildBackendGroups(append(httpServers, sslServers...)) + keyPairs := buildSSLKeyPairs(g.ReferencedSecrets, g.Gateway.Listeners) config := Configuration{ HTTPServers: httpServers, SSLServers: sslServers, Upstreams: upstreams, BackendGroups: backendGroups, + SSLKeyPairs: keyPairs, } return config } +// buildSSLKeyPairs builds the SSLKeyPairs from the Secrets. It will only include Secrets that are referenced by +// valid listeners, so that we don't include unused Secrets in the configuration of the data plane. +func buildSSLKeyPairs( + secrets map[types.NamespacedName]*graph.Secret, + listeners map[string]*graph.Listener, +) map[SSLKeyPairID]SSLKeyPair { + keyPairs := make(map[SSLKeyPairID]SSLKeyPair) + + for _, l := range listeners { + if l.Valid && l.ResolvedSecret != nil { + id := generateSSLKeyPairID(*l.ResolvedSecret) + secret := secrets[*l.ResolvedSecret] + // The Data map keys are guaranteed to exist by the graph package. + // the Source field is guaranteed to be non-nil by the graph package. + keyPairs[id] = SSLKeyPair{ + Cert: secret.Source.Data[apiv1.TLSCertKey], + Key: secret.Source.Data[apiv1.TLSPrivateKeyKey], + } + } + } + + return keyPairs +} + func buildBackendGroups(servers []VirtualServer) []BackendGroup { type key struct { nsname types.NamespacedName @@ -381,8 +420,10 @@ func (hpr *hostPathRules) buildServers() []VirtualServer { panic(fmt.Sprintf("no listener found for hostname: %s", h)) } - if l.SecretPath != "" { - s.SSL = &SSL{CertificatePath: l.SecretPath} + if l.ResolvedSecret != nil { + s.SSL = &SSL{ + KeyPairID: generateSSLKeyPairID(*l.ResolvedSecret), + } } for _, r := range rules { @@ -413,8 +454,10 @@ func (hpr *hostPathRules) buildServers() []VirtualServer { Port: hpr.port, } - if l.SecretPath != "" { - s.SSL = &SSL{CertificatePath: l.SecretPath} + if l.ResolvedSecret != nil { + s.SSL = &SSL{ + KeyPairID: generateSSLKeyPairID(*l.ResolvedSecret), + } } servers = append(servers, s) @@ -580,3 +623,10 @@ func listenerHostnameMoreSpecific(host1, host2 *v1beta1.Hostname) bool { return graph.GetMoreSpecificHostname(host1Str, host2Str) == host1Str } + +// generateSSLKeyPairID generates an ID for the SSL key pair based on the Secret namespaced name. +// It is guaranteed to be unique per unique namespaced name. +// The ID is safe to use as a file name. +func generateSSLKeyPairID(secret types.NamespacedName) SSLKeyPairID { + return SSLKeyPairID(fmt.Sprintf("ssl_keypair_%s_%s", secret.Namespace, secret.Name)) +} diff --git a/internal/state/dataplane/configuration_test.go b/internal/state/dataplane/configuration_test.go index c17ddc9dae..553a5776e9 100644 --- a/internal/state/dataplane/configuration_test.go +++ b/internal/state/dataplane/configuration_test.go @@ -8,7 +8,7 @@ import ( "github.com/google/go-cmp/cmp" . "github.com/onsi/gomega" - v1 "k8s.io/api/core/v1" + apiv1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -83,7 +83,7 @@ func TestBuildConfiguration(t *testing.T) { Endpoints: fooEndpoints, } - fooSvc := &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test"}} + fooSvc := &apiv1.Service{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test"}} fakeResolver := &resolverfakes.FakeServiceResolver{} fakeResolver.ResolveReturns(fooEndpoints, nil) @@ -297,6 +297,34 @@ func TestBuildConfiguration(t *testing.T) { pathAndType{path: "/", pathType: prefix}, pathAndType{path: "/third", pathType: prefix}, ) + secret1NsName := types.NamespacedName{Namespace: "test", Name: "secret-1"} + secret1 := &graph.Secret{ + Source: &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secret1NsName.Name, + Namespace: secret1NsName.Namespace, + }, + Data: map[string][]byte{ + apiv1.TLSCertKey: []byte("cert-1"), + apiv1.TLSPrivateKeyKey: []byte("privateKey-1"), + }, + }, + } + + secret2NsName := types.NamespacedName{Namespace: "test", Name: "secret-2"} + secret2 := &graph.Secret{ + Source: &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secret2NsName.Name, + Namespace: secret2NsName.Namespace, + }, + Data: map[string][]byte{ + apiv1.TLSCertKey: []byte("cert-2"), + apiv1.TLSPrivateKeyKey: []byte("privateKey-2"), + }, + }, + } + listener80 := v1beta1.Listener{ Name: "listener-80-1", Hostname: nil, @@ -321,8 +349,8 @@ func TestBuildConfiguration(t *testing.T) { CertificateRefs: []v1beta1.SecretObjectReference{ { Kind: (*v1beta1.Kind)(helpers.GetStringPointer("Secret")), - Name: "secret", - Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("test")), + Namespace: helpers.GetPointer(v1beta1.Namespace(secret1NsName.Namespace)), + Name: v1beta1.ObjectName(secret1NsName.Name), }, }, }, @@ -338,8 +366,8 @@ func TestBuildConfiguration(t *testing.T) { CertificateRefs: []v1beta1.SecretObjectReference{ { Kind: (*v1beta1.Kind)(helpers.GetStringPointer("Secret")), - Name: "secret", - Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("test")), + Namespace: helpers.GetPointer(v1beta1.Namespace(secret2NsName.Namespace)), + Name: v1beta1.ObjectName(secret2NsName.Name), }, }, }, @@ -357,8 +385,8 @@ func TestBuildConfiguration(t *testing.T) { CertificateRefs: []v1beta1.SecretObjectReference{ { Kind: (*v1beta1.Kind)(helpers.GetStringPointer("Secret")), - Name: "secret", - Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("test")), + Namespace: helpers.GetPointer(v1beta1.Namespace(secret2NsName.Namespace)), + Name: v1beta1.ObjectName(secret2NsName.Name), }, }, }, @@ -369,12 +397,18 @@ func TestBuildConfiguration(t *testing.T) { Hostname: nil, Port: 443, Protocol: v1beta1.HTTPSProtocolType, - TLS: nil, // missing TLS config + TLS: &v1beta1.GatewayTLSConfig{ + // Mode is missing, that's why invalid + CertificateRefs: []v1beta1.SecretObjectReference{ + { + Kind: helpers.GetPointer[v1beta1.Kind]("Secret"), + Namespace: helpers.GetPointer(v1beta1.Namespace(secret1NsName.Namespace)), + Name: v1beta1.ObjectName(secret1NsName.Name), + }, + }, + }, } - // nolint:gosec - secretPath := "/etc/nginx/secrets/secret" - tests := []struct { graph *graph.Graph msg string @@ -395,6 +429,7 @@ func TestBuildConfiguration(t *testing.T) { expConf: Configuration{ HTTPServers: []VirtualServer{}, SSLServers: []VirtualServer{}, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{}, }, msg: "no listeners and routes", }, @@ -423,7 +458,8 @@ func TestBuildConfiguration(t *testing.T) { Port: 80, }, }, - SSLServers: []VirtualServer{}, + SSLServers: []VirtualServer{}, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{}, }, msg: "http listener with no routes", }, @@ -437,20 +473,24 @@ func TestBuildConfiguration(t *testing.T) { Source: &v1beta1.Gateway{}, Listeners: map[string]*graph.Listener{ "listener-443-1": { - Source: listener443, // nil hostname - Valid: true, - Routes: map[types.NamespacedName]*graph.Route{}, - SecretPath: secretPath, + Source: listener443, // nil hostname + Valid: true, + Routes: map[types.NamespacedName]*graph.Route{}, + ResolvedSecret: &secret1NsName, }, "listener-443-with-hostname": { - Source: listener443WithHostname, // non-nil hostname - Valid: true, - Routes: map[types.NamespacedName]*graph.Route{}, - SecretPath: secretPath, + Source: listener443WithHostname, // non-nil hostname + Valid: true, + Routes: map[types.NamespacedName]*graph.Route{}, + ResolvedSecret: &secret2NsName, }, }, }, Routes: map[types.NamespacedName]*graph.Route{}, + ReferencedSecrets: map[types.NamespacedName]*graph.Secret{ + secret1NsName: secret1, + secret2NsName: secret2, + }, }, expConf: Configuration{ HTTPServers: []VirtualServer{}, @@ -461,15 +501,25 @@ func TestBuildConfiguration(t *testing.T) { }, { Hostname: string(hostname), - SSL: &SSL{CertificatePath: secretPath}, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-2"}, Port: 443, }, { Hostname: wildcardHostname, - SSL: &SSL{CertificatePath: secretPath}, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, Port: 443, }, }, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{ + "ssl_keypair_test_secret-1": { + Cert: []byte("cert-1"), + Key: []byte("privateKey-1"), + }, + "ssl_keypair_test_secret-2": { + Cert: []byte("cert-2"), + Key: []byte("privateKey-2"), + }, + }, }, msg: "https listeners with no routes", }, @@ -483,8 +533,9 @@ func TestBuildConfiguration(t *testing.T) { Source: &v1beta1.Gateway{}, Listeners: map[string]*graph.Listener{ "invalid-listener": { - Source: invalidListener, - Valid: false, + Source: invalidListener, + Valid: false, + ResolvedSecret: &secret1NsName, }, }, }, @@ -492,12 +543,16 @@ func TestBuildConfiguration(t *testing.T) { {Namespace: "test", Name: "https-hr-1"}: httpsRouteHR1, {Namespace: "test", Name: "https-hr-2"}: httpsRouteHR2, }, + ReferencedSecrets: map[types.NamespacedName]*graph.Secret{ + secret1NsName: secret1, + }, }, expConf: Configuration{ HTTPServers: []VirtualServer{}, SSLServers: []VirtualServer{}, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{}, }, - msg: "invalid listener", + msg: "invalid https listener with resolved secret", }, { graph: &graph.Graph{ @@ -569,6 +624,7 @@ func TestBuildConfiguration(t *testing.T) { SSLServers: []VirtualServer{}, Upstreams: []Upstream{fooUpstream}, BackendGroups: []BackendGroup{expHR1Groups[0], expHR2Groups[0]}, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{}, }, msg: "one http listener with two routes for different hostnames", }, @@ -582,21 +638,21 @@ func TestBuildConfiguration(t *testing.T) { Source: &v1beta1.Gateway{}, Listeners: map[string]*graph.Listener{ "listener-443-1": { - Source: listener443, - Valid: true, - SecretPath: secretPath, + Source: listener443, + Valid: true, Routes: map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "https-hr-1"}: httpsRouteHR1, {Namespace: "test", Name: "https-hr-2"}: httpsRouteHR2, }, + ResolvedSecret: &secret1NsName, }, "listener-443-with-hostname": { - Source: listener443WithHostname, - Valid: true, - SecretPath: secretPath, + Source: listener443WithHostname, + Valid: true, Routes: map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "https-hr-5"}: httpsRouteHR5, }, + ResolvedSecret: &secret2NsName, }, }, }, @@ -605,6 +661,10 @@ func TestBuildConfiguration(t *testing.T) { {Namespace: "test", Name: "https-hr-2"}: httpsRouteHR2, {Namespace: "test", Name: "https-hr-5"}: httpsRouteHR5, }, + ReferencedSecrets: map[types.NamespacedName]*graph.Secret{ + secret1NsName: secret1, + secret2NsName: secret2, + }, }, expConf: Configuration{ HTTPServers: []VirtualServer{}, @@ -629,9 +689,7 @@ func TestBuildConfiguration(t *testing.T) { }, }, }, - SSL: &SSL{ - CertificatePath: secretPath, - }, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, Port: 443, }, { @@ -650,9 +708,7 @@ func TestBuildConfiguration(t *testing.T) { }, }, }, - SSL: &SSL{ - CertificatePath: secretPath, - }, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-2"}, Port: 443, }, { @@ -671,19 +727,27 @@ func TestBuildConfiguration(t *testing.T) { }, }, }, - SSL: &SSL{ - CertificatePath: secretPath, - }, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, Port: 443, }, { Hostname: wildcardHostname, - SSL: &SSL{CertificatePath: secretPath}, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, Port: 443, }, }, Upstreams: []Upstream{fooUpstream}, BackendGroups: []BackendGroup{expHTTPSHR1Groups[0], expHTTPSHR2Groups[0], expHTTPSHR5Groups[0]}, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{ + "ssl_keypair_test_secret-1": { + Cert: []byte("cert-1"), + Key: []byte("privateKey-1"), + }, + "ssl_keypair_test_secret-2": { + Cert: []byte("cert-2"), + Key: []byte("privateKey-2"), + }, + }, }, msg: "two https listeners each with routes for different hostnames", }, @@ -705,13 +769,13 @@ func TestBuildConfiguration(t *testing.T) { }, }, "listener-443-1": { - Source: listener443, - Valid: true, - SecretPath: secretPath, + Source: listener443, + Valid: true, Routes: map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "https-hr-3"}: httpsRouteHR3, {Namespace: "test", Name: "https-hr-4"}: httpsRouteHR4, }, + ResolvedSecret: &secret1NsName, }, }, }, @@ -721,6 +785,9 @@ func TestBuildConfiguration(t *testing.T) { {Namespace: "test", Name: "https-hr-3"}: httpsRouteHR3, {Namespace: "test", Name: "https-hr-4"}: httpsRouteHR4, }, + ReferencedSecrets: map[types.NamespacedName]*graph.Secret{ + secret1NsName: secret1, + }, }, expConf: Configuration{ HTTPServers: []VirtualServer{ @@ -784,9 +851,7 @@ func TestBuildConfiguration(t *testing.T) { }, { Hostname: "foo.example.com", - SSL: &SSL{ - CertificatePath: secretPath, - }, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, PathRules: []PathRule{ { Path: "/", @@ -835,7 +900,7 @@ func TestBuildConfiguration(t *testing.T) { }, { Hostname: wildcardHostname, - SSL: &SSL{CertificatePath: secretPath}, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, Port: 443, }, }, @@ -850,6 +915,12 @@ func TestBuildConfiguration(t *testing.T) { expHTTPSHR4Groups[0], expHTTPSHR4Groups[1], }, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{ + "ssl_keypair_test_secret-1": { + Cert: []byte("cert-1"), + Key: []byte("privateKey-1"), + }, + }, }, msg: "one http and one https listener with two routes with the same hostname with and without collisions", }, @@ -877,20 +948,20 @@ func TestBuildConfiguration(t *testing.T) { }, }, "listener-443-1": { - Source: listener443, - Valid: true, - SecretPath: secretPath, + Source: listener443, + Valid: true, Routes: map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "https-hr-3"}: httpsRouteHR3, }, + ResolvedSecret: &secret1NsName, }, "listener-8443": { - Source: listener8443, - Valid: true, - SecretPath: secretPath, + Source: listener8443, + Valid: true, Routes: map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "https-hr-7"}: httpsRouteHR7, }, + ResolvedSecret: &secret1NsName, }, }, }, @@ -900,6 +971,9 @@ func TestBuildConfiguration(t *testing.T) { {Namespace: "test", Name: "https-hr-3"}: httpsRouteHR3, {Namespace: "test", Name: "https-hr-7"}: httpsRouteHR7, }, + ReferencedSecrets: map[types.NamespacedName]*graph.Secret{ + secret1NsName: secret1, + }, }, expConf: Configuration{ HTTPServers: []VirtualServer{ @@ -979,9 +1053,7 @@ func TestBuildConfiguration(t *testing.T) { }, { Hostname: "foo.example.com", - SSL: &SSL{ - CertificatePath: secretPath, - }, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, PathRules: []PathRule{ { Path: "/", @@ -1012,7 +1084,7 @@ func TestBuildConfiguration(t *testing.T) { }, { Hostname: wildcardHostname, - SSL: &SSL{CertificatePath: secretPath}, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, Port: 443, }, { @@ -1021,9 +1093,7 @@ func TestBuildConfiguration(t *testing.T) { }, { Hostname: "foo.example.com", - SSL: &SSL{ - CertificatePath: secretPath, - }, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, PathRules: []PathRule{ { Path: "/", @@ -1054,7 +1124,7 @@ func TestBuildConfiguration(t *testing.T) { }, { Hostname: wildcardHostname, - SSL: &SSL{CertificatePath: secretPath}, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, Port: 8443, }, }, @@ -1069,6 +1139,12 @@ func TestBuildConfiguration(t *testing.T) { expHTTPSHR7Groups[0], expHTTPSHR7Groups[1], }, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{ + "ssl_keypair_test_secret-1": { + Cert: []byte("cert-1"), + Key: []byte("privateKey-1"), + }, + }, }, msg: "multiple http and https listener; different ports", @@ -1200,6 +1276,7 @@ func TestBuildConfiguration(t *testing.T) { SSLServers: []VirtualServer{}, Upstreams: []Upstream{fooUpstream}, BackendGroups: []BackendGroup{expHR5Groups[0], expHR5Groups[1]}, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{}, }, msg: "one http listener with one route with filters", }, @@ -1220,12 +1297,12 @@ func TestBuildConfiguration(t *testing.T) { }, }, "listener-443-1": { - Source: listener443, - Valid: true, - SecretPath: secretPath, + Source: listener443, + Valid: true, Routes: map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "https-hr-6"}: httpsRouteHR6, }, + ResolvedSecret: &secret1NsName, }, }, }, @@ -1233,6 +1310,9 @@ func TestBuildConfiguration(t *testing.T) { {Namespace: "test", Name: "hr-6"}: routeHR6, {Namespace: "test", Name: "https-hr-6"}: httpsRouteHR6, }, + ReferencedSecrets: map[types.NamespacedName]*graph.Secret{ + secret1NsName: secret1, + }, }, expConf: Configuration{ HTTPServers: []VirtualServer{ @@ -1266,9 +1346,7 @@ func TestBuildConfiguration(t *testing.T) { }, { Hostname: "foo.example.com", - SSL: &SSL{ - CertificatePath: secretPath, - }, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, PathRules: []PathRule{ { Path: "/valid", @@ -1287,7 +1365,7 @@ func TestBuildConfiguration(t *testing.T) { }, { Hostname: wildcardHostname, - SSL: &SSL{CertificatePath: secretPath}, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, Port: 443, }, }, @@ -1296,6 +1374,12 @@ func TestBuildConfiguration(t *testing.T) { expHR6Groups[0], expHTTPSHR6Groups[0], }, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{ + "ssl_keypair_test_secret-1": { + Cert: []byte("cert-1"), + Key: []byte("privateKey-1"), + }, + }, }, msg: "one http and one https listener with routes with valid and invalid rules", }, @@ -1361,6 +1445,7 @@ func TestBuildConfiguration(t *testing.T) { SSLServers: []VirtualServer{}, Upstreams: []Upstream{fooUpstream}, BackendGroups: []BackendGroup{expHR7Groups[0], expHR7Groups[1]}, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{}, }, msg: "duplicate paths with different types", }, @@ -1374,26 +1459,30 @@ func TestBuildConfiguration(t *testing.T) { Source: &v1beta1.Gateway{}, Listeners: map[string]*graph.Listener{ "listener-443-with-hostname": { - Source: listener443WithHostname, - Valid: true, - SecretPath: "secret-path-https-listener-2", + Source: listener443WithHostname, + Valid: true, Routes: map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "https-hr-5"}: httpsRouteHR5, }, + ResolvedSecret: &secret2NsName, }, "listener-443-1": { - Source: listener443, - Valid: true, - SecretPath: secretPath, + Source: listener443, + Valid: true, Routes: map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "https-hr-5"}: httpsRouteHR5, }, + ResolvedSecret: &secret1NsName, }, }, }, Routes: map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "https-hr-5"}: httpsRouteHR5, }, + ReferencedSecrets: map[types.NamespacedName]*graph.Secret{ + secret1NsName: secret1, + secret2NsName: secret2, + }, }, expConf: Configuration{ HTTPServers: []VirtualServer{}, @@ -1425,19 +1514,27 @@ func TestBuildConfiguration(t *testing.T) { }, }, }, - SSL: &SSL{ - CertificatePath: "secret-path-https-listener-2", - }, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-2"}, Port: 443, }, { Hostname: wildcardHostname, - SSL: &SSL{CertificatePath: secretPath}, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, Port: 443, }, }, Upstreams: []Upstream{fooUpstream}, BackendGroups: []BackendGroup{expHTTPSHR5Groups[0]}, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{ + "ssl_keypair_test_secret-1": { + Cert: []byte("cert-1"), + Key: []byte("privateKey-1"), + }, + "ssl_keypair_test_secret-2": { + Cert: []byte("cert-2"), + Key: []byte("privateKey-2"), + }, + }, }, msg: "two https listeners with different hostnames but same route; chooses listener with more specific hostname", }, @@ -1453,6 +1550,7 @@ func TestBuildConfiguration(t *testing.T) { g.Expect(result.Upstreams).To(ConsistOf(test.expConf.Upstreams)) g.Expect(result.HTTPServers).To(ConsistOf(test.expConf.HTTPServers)) g.Expect(result.SSLServers).To(ConsistOf(test.expConf.SSLServers)) + g.Expect(result.SSLKeyPairs).To(Equal(test.expConf.SSLKeyPairs)) }) } } @@ -1770,7 +1868,7 @@ func TestBuildUpstreams(t *testing.T) { var backends []graph.BackendRef for _, name := range serviceNames { backends = append(backends, graph.BackendRef{ - Svc: &v1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: name}}, + Svc: &apiv1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: name}}, Port: 80, Valid: name != "", }) @@ -1866,7 +1964,7 @@ func TestBuildUpstreams(t *testing.T) { } fakeResolver := &resolverfakes.FakeServiceResolver{} - fakeResolver.ResolveCalls(func(ctx context.Context, svc *v1.Service, port int32) ([]resolver.Endpoint, error) { + fakeResolver.ResolveCalls(func(ctx context.Context, svc *apiv1.Service, port int32) ([]resolver.Endpoint, error) { switch svc.Name { case "bar": return barEndpoints, nil diff --git a/internal/state/graph/gateway.go b/internal/state/graph/gateway.go index 3d5f86ed0b..50f2fdb0fb 100644 --- a/internal/state/graph/gateway.go +++ b/internal/state/graph/gateway.go @@ -10,7 +10,6 @@ import ( nkgsort "github.com/nginxinc/nginx-kubernetes-gateway/internal/sort" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets" ) // Gateway represents the winning Gateway resource. @@ -92,7 +91,7 @@ func processGateways( func buildGateway( gw *v1beta1.Gateway, - secretMemoryMgr secrets.SecretDiskMemoryManager, + secretResolver *secretResolver, gc *GatewayClass, refGrantResolver *referenceGrantResolver, ) *Gateway { @@ -112,7 +111,7 @@ func buildGateway( return &Gateway{ Source: gw, - Listeners: buildListeners(gw, secretMemoryMgr, refGrantResolver), + Listeners: buildListeners(gw, secretResolver, refGrantResolver), Valid: true, } } diff --git a/internal/state/graph/gateway_listener.go b/internal/state/graph/gateway_listener.go index b76b0d7c27..0cec44b0fa 100644 --- a/internal/state/graph/gateway_listener.go +++ b/internal/state/graph/gateway_listener.go @@ -11,7 +11,6 @@ import ( "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets" ) // Listener represents a Listener of the Gateway resource. @@ -24,8 +23,9 @@ type Listener struct { Routes map[types.NamespacedName]*Route // AllowedRouteLabelSelector is the label selector for this Listener's allowed routes, if defined. AllowedRouteLabelSelector labels.Selector - // SecretPath is the path to the secret on disk. - SecretPath string + // ResolvedSecret is the namespaced name of the Secret resolved for this listener. + // Only applicable for HTTPS listeners. + ResolvedSecret *types.NamespacedName // Conditions holds the conditions of the Listener. Conditions []conditions.Condition // Valid shows whether the Listener is valid. @@ -35,12 +35,12 @@ type Listener struct { func buildListeners( gw *v1beta1.Gateway, - secretMemoryMgr secrets.SecretDiskMemoryManager, + secretResolver *secretResolver, refGrantResolver *referenceGrantResolver, ) map[string]*Listener { listeners := make(map[string]*Listener) - listenerFactory := newListenerConfiguratorFactory(gw, secretMemoryMgr, refGrantResolver) + listenerFactory := newListenerConfiguratorFactory(gw, secretResolver, refGrantResolver) for _, gl := range gw.Spec.Listeners { configurator := listenerFactory.getConfiguratorForListener(gl) @@ -67,7 +67,7 @@ func (f *listenerConfiguratorFactory) getConfiguratorForListener(l v1beta1.Liste func newListenerConfiguratorFactory( gw *v1beta1.Gateway, - secretMemoryMgr secrets.SecretDiskMemoryManager, + secretResolver *secretResolver, refGrantResolver *referenceGrantResolver, ) *listenerConfiguratorFactory { sharedPortConflictResolver := createPortConflictResolver() @@ -107,7 +107,7 @@ func newListenerConfiguratorFactory( sharedPortConflictResolver, }, externalReferenceResolvers: []listenerExternalReferenceResolver{ - createExternalReferencesForTLSSecretsResolver(gw.Namespace, secretMemoryMgr, refGrantResolver), + createExternalReferencesForTLSSecretsResolver(gw.Namespace, secretResolver, refGrantResolver), }, }, } @@ -375,7 +375,7 @@ func createPortConflictResolver() listenerConflictResolver { func createExternalReferencesForTLSSecretsResolver( gwNs string, - secretMemoryMgr secrets.SecretDiskMemoryManager, + secretResolver *secretResolver, refGrantResolver *referenceGrantResolver, ) listenerExternalReferenceResolver { return func(l *Listener) { @@ -401,16 +401,15 @@ func createExternalReferencesForTLSSecretsResolver( } } - var err error - - l.SecretPath, err = secretMemoryMgr.Request(certRefNsName) - if err != nil { + if err := secretResolver.resolve(certRefNsName); err != nil { path := field.NewPath("tls", "certificateRefs").Index(0) // field.NotFound could be better, but it doesn't allow us to set the error message. valErr := field.Invalid(path, certRefNsName, err.Error()) l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) l.Valid = false + } else { + l.ResolvedSecret = &certRefNsName } } } diff --git a/internal/state/graph/gateway_test.go b/internal/state/graph/gateway_test.go index 9116c1ca6f..0d8744cd7d 100644 --- a/internal/state/graph/gateway_test.go +++ b/internal/state/graph/gateway_test.go @@ -2,10 +2,10 @@ package graph import ( - "errors" "testing" . "github.com/onsi/gomega" + apiv1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" @@ -14,7 +14,6 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets/secretsfakes" ) func TestProcessedGatewaysGetAllNsNames(t *testing.T) { @@ -169,13 +168,25 @@ func TestBuildGateway(t *testing.T) { }, } + secretSameNs := &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "secret", + }, + Data: map[string][]byte{ + apiv1.TLSCertKey: cert, + apiv1.TLSPrivateKeyKey: key, + }, + Type: apiv1.SecretTypeTLS, + } + gatewayTLSConfigSameNs := &v1beta1.GatewayTLSConfig{ Mode: helpers.GetPointer(v1beta1.TLSModeTerminate), CertificateRefs: []v1beta1.SecretObjectReference{ { Kind: helpers.GetPointer[v1beta1.Kind]("Secret"), - Name: "secret", - Namespace: helpers.GetPointer[v1beta1.Namespace]("test"), + Name: v1beta1.ObjectName(secretSameNs.Name), + Namespace: (*v1beta1.Namespace)(&secretSameNs.Namespace), }, }, } @@ -191,13 +202,25 @@ func TestBuildGateway(t *testing.T) { }, } + secretDiffNamespace := &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "diff-ns", + Name: "secret", + }, + Data: map[string][]byte{ + apiv1.TLSCertKey: cert, + apiv1.TLSPrivateKeyKey: key, + }, + Type: apiv1.SecretTypeTLS, + } + gatewayTLSConfigDiffNs := &v1beta1.GatewayTLSConfig{ Mode: helpers.GetPointer(v1beta1.TLSModeTerminate), CertificateRefs: []v1beta1.SecretObjectReference{ { Kind: helpers.GetPointer[v1beta1.Kind]("Secret"), - Name: "secret", - Namespace: helpers.GetPointer[v1beta1.Namespace]("diff-ns"), + Name: v1beta1.ObjectName(secretDiffNamespace.Name), + Namespace: (*v1beta1.Namespace)(&secretDiffNamespace.Namespace), }, }, } @@ -282,9 +305,6 @@ func TestBuildGateway(t *testing.T) { conflict443PortMsg = "Multiple listeners for the same port 443 specify incompatible protocols; " + "ensure only one protocol per port" - - secretPath = "/etc/nginx/secrets/test_secret" - diffNsSecretPath = "/etc/nginx/secrets/diff_ns_secret" ) type gatewayCfg struct { @@ -354,16 +374,16 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "foo-443-https-1": { - Source: foo443HTTPSListener1, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, - SecretPath: secretPath, + Source: foo443HTTPSListener1, + Valid: true, + Routes: map[types.NamespacedName]*Route{}, + ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), }, "foo-8443-https": { - Source: foo8443HTTPSListener, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, - SecretPath: secretPath, + Source: foo8443HTTPSListener, + Valid: true, + Routes: map[types.NamespacedName]*Route{}, + ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), }, }, Valid: true, @@ -418,10 +438,10 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "listener-cross-ns-secret": { - Source: crossNamespaceSecretListener, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, - SecretPath: diffNsSecretPath, + Source: crossNamespaceSecretListener, + Valid: true, + Routes: map[types.NamespacedName]*Route{}, + ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretDiffNamespace)), }, }, Valid: true, @@ -544,7 +564,7 @@ func TestBuildGateway(t *testing.T) { Valid: false, Routes: map[types.NamespacedName]*Route{}, Conditions: conditions.NewListenerInvalidCertificateRef( - `tls.certificateRefs[0]: Invalid value: test/does-not-exist: secret not found`, + `tls.certificateRefs[0]: Invalid value: test/does-not-exist: secret does not exist`, ), }, }, @@ -592,28 +612,28 @@ func TestBuildGateway(t *testing.T) { Routes: map[types.NamespacedName]*Route{}, }, "foo-443-https-1": { - Source: foo443HTTPSListener1, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, - SecretPath: secretPath, + Source: foo443HTTPSListener1, + Valid: true, + Routes: map[types.NamespacedName]*Route{}, + ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), }, "foo-8443-https": { - Source: foo8443HTTPSListener, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, - SecretPath: secretPath, + Source: foo8443HTTPSListener, + Valid: true, + Routes: map[types.NamespacedName]*Route{}, + ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), }, "bar-443-https": { - Source: bar443HTTPSListener, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, - SecretPath: secretPath, + Source: bar443HTTPSListener, + Valid: true, + Routes: map[types.NamespacedName]*Route{}, + ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), }, "bar-8443-https": { - Source: bar8443HTTPSListener, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, - SecretPath: secretPath, + Source: bar8443HTTPSListener, + Valid: true, + Routes: map[types.NamespacedName]*Route{}, + ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), }, }, Valid: true, @@ -656,25 +676,25 @@ func TestBuildGateway(t *testing.T) { Conditions: conditions.NewListenerProtocolConflict(conflict443PortMsg), }, "foo-80-https": { - Source: foo80HTTPSListener, - Valid: false, - Routes: map[types.NamespacedName]*Route{}, - Conditions: conditions.NewListenerProtocolConflict(conflict80PortMsg), - SecretPath: "/etc/nginx/secrets/test_secret", + Source: foo80HTTPSListener, + Valid: false, + Routes: map[types.NamespacedName]*Route{}, + Conditions: conditions.NewListenerProtocolConflict(conflict80PortMsg), + ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), }, "foo-443-https-1": { - Source: foo443HTTPSListener1, - Valid: false, - Routes: map[types.NamespacedName]*Route{}, - Conditions: conditions.NewListenerProtocolConflict(conflict443PortMsg), - SecretPath: "/etc/nginx/secrets/test_secret", + Source: foo443HTTPSListener1, + Valid: false, + Routes: map[types.NamespacedName]*Route{}, + Conditions: conditions.NewListenerProtocolConflict(conflict443PortMsg), + ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), }, "bar-443-https": { - Source: bar443HTTPSListener, - Valid: false, - Routes: map[types.NamespacedName]*Route{}, - Conditions: conditions.NewListenerProtocolConflict(conflict443PortMsg), - SecretPath: "/etc/nginx/secrets/test_secret", + Source: bar443HTTPSListener, + Valid: false, + Routes: map[types.NamespacedName]*Route{}, + Conditions: conditions.NewListenerProtocolConflict(conflict443PortMsg), + ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), }, }, Valid: true, @@ -729,23 +749,17 @@ func TestBuildGateway(t *testing.T) { }, } - secretMemoryMgr := &secretsfakes.FakeSecretDiskMemoryManager{} - secretMemoryMgr.RequestCalls(func(nsname types.NamespacedName) (string, error) { - switch nsname { - case types.NamespacedName{Namespace: "test", Name: "secret"}: - return secretPath, nil - case types.NamespacedName{Namespace: "diff-ns", Name: "secret"}: - return diffNsSecretPath, nil - default: - return "", errors.New("secret not found") - } - }) + secretResolver := newSecretResolver( + map[types.NamespacedName]*apiv1.Secret{ + client.ObjectKeyFromObject(secretSameNs): secretSameNs, + client.ObjectKeyFromObject(secretDiffNamespace): secretDiffNamespace, + }) for _, test := range tests { t.Run(test.name, func(t *testing.T) { g := NewGomegaWithT(t) resolver := newReferenceGrantResolver(test.refGrants) - result := buildGateway(test.gateway, secretMemoryMgr, test.gatewayClass, resolver) + result := buildGateway(test.gateway, secretResolver, test.gatewayClass, resolver) g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) }) } diff --git a/internal/state/graph/graph.go b/internal/state/graph/graph.go index 3c8a4297ea..b63571802c 100644 --- a/internal/state/graph/graph.go +++ b/internal/state/graph/graph.go @@ -3,9 +3,9 @@ package graph import ( v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/gateway-api/apis/v1beta1" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/validation" ) @@ -17,6 +17,7 @@ type ClusterState struct { Services map[types.NamespacedName]*v1.Service Namespaces map[types.NamespacedName]*v1.Namespace ReferenceGrants map[types.NamespacedName]*v1beta1.ReferenceGrant + Secrets map[types.NamespacedName]*v1.Secret } // Graph is a Graph-like representation of Gateway API resources. @@ -35,6 +36,27 @@ type Graph struct { IgnoredGateways map[types.NamespacedName]*v1beta1.Gateway // Routes holds Route resources. Routes map[types.NamespacedName]*Route + // ReferencedSecrets includes Secrets referenced by Gateway Listeners, including invalid ones. + // It is different from the other maps, because it includes entries for Secrets that do not exist + // in the cluster. We need such entries so that we can query the Graph to determine if a Secret is referenced + // by the Gateway, including the case when the Secret is newly created. + ReferencedSecrets map[types.NamespacedName]*Secret +} + +// IsReferenced returns true if the Graph references the resource. +func (g *Graph) IsReferenced(resourceType client.Object, nsname types.NamespacedName) bool { + // FIMXE(pleshakov): For now, only works with Secrets. + // Support EndpointSlices and Namespaces so that we can remove relationship.Capturer and use the Graph + // as source to determine the relationships. + // See https://github.com/nginxinc/nginx-kubernetes-gateway/issues/824 + + switch resourceType.(type) { + case *v1.Secret: + _, exists := g.ReferencedSecrets[nsname] + return exists + default: + return false + } } // BuildGraph builds a Graph from a state. @@ -42,7 +64,6 @@ func BuildGraph( state ClusterState, controllerName string, gcName string, - secretMemoryMgr secrets.SecretDiskMemoryManager, validators validation.Validators, ) *Graph { processedGwClasses, gcExists := processGatewayClasses(state.GatewayClasses, gcName, controllerName) @@ -52,10 +73,12 @@ func BuildGraph( } gc := buildGatewayClass(processedGwClasses.Winner) + secretResolver := newSecretResolver(state.Secrets) + processedGws := processGateways(state.Gateways, gcName) refGrantResolver := newReferenceGrantResolver(state.ReferenceGrants) - gw := buildGateway(processedGws.Winner, secretMemoryMgr, gc, refGrantResolver) + gw := buildGateway(processedGws.Winner, secretResolver, gc, refGrantResolver) routes := buildRoutesForGateways(validators.HTTPFieldsValidator, state.HTTPRoutes, processedGws.GetAllNsNames()) bindRoutesToListeners(routes, gw, state.Namespaces) @@ -67,6 +90,7 @@ func BuildGraph( Routes: routes, IgnoredGatewayClasses: processedGwClasses.Ignored, IgnoredGateways: processedGws.Ignored, + ReferencedSecrets: secretResolver.getResolvedSecrets(), } return g diff --git a/internal/state/graph/graph_test.go b/internal/state/graph/graph_test.go index 68c1938424..2428eba68c 100644 --- a/internal/state/graph/graph_test.go +++ b/internal/state/graph/graph_test.go @@ -12,7 +12,6 @@ import ( "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets/secretsfakes" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/validation" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/validation/validationfakes" ) @@ -21,7 +20,6 @@ func TestBuildGraph(t *testing.T) { const ( gcName = "my-class" controllerName = "my.controller" - secretPath = "/etc/nginx/secrets/test_secret" ) createValidRuleWithBackendRefs := func(refs []BackendRef) Rule { @@ -103,6 +101,18 @@ func TestBuildGraph(t *testing.T) { }, } + secret := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "secret", + }, + Data: map[string][]byte{ + v1.TLSCertKey: cert, + v1.TLSPrivateKeyKey: key, + }, + Type: v1.SecretTypeTLS, + } + createGateway := func(name string) *v1beta1.Gateway { return &v1beta1.Gateway{ ObjectMeta: metav1.ObjectMeta{ @@ -128,8 +138,8 @@ func TestBuildGraph(t *testing.T) { CertificateRefs: []v1beta1.SecretObjectReference{ { Kind: helpers.GetPointer[v1beta1.Kind]("Secret"), - Name: "secret", - Namespace: helpers.GetPointer[v1beta1.Namespace]("certificate"), + Name: v1beta1.ObjectName(secret.Name), + Namespace: helpers.GetPointer(v1beta1.Namespace(secret.Namespace)), }, }, }, @@ -208,6 +218,9 @@ func TestBuildGraph(t *testing.T) { client.ObjectKeyFromObject(rgSecret): rgSecret, client.ObjectKeyFromObject(rgService): rgService, }, + Secrets: map[types.NamespacedName]*v1.Secret{ + client.ObjectKeyFromObject(secret): secret, + }, } } @@ -243,14 +256,6 @@ func TestBuildGraph(t *testing.T) { Rules: []Rule{createValidRuleWithBackendRefs(hr3Refs)}, } - secretMemoryMgr := &secretsfakes.FakeSecretDiskMemoryManager{} - secretMemoryMgr.RequestCalls(func(nsname types.NamespacedName) (string, error) { - if (nsname == types.NamespacedName{Namespace: "certificate", Name: "secret"}) { - return secretPath, nil - } - panic("unexpected secret request") - }) - createExpectedGraphWithGatewayClass := func(gc *v1beta1.GatewayClass) *Graph { return &Graph{ GatewayClass: &GatewayClass{ @@ -273,7 +278,7 @@ func TestBuildGraph(t *testing.T) { Routes: map[types.NamespacedName]*Route{ {Namespace: "test", Name: "hr-3"}: routeHR3, }, - SecretPath: secretPath, + ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secret)), }, }, Valid: true, @@ -285,6 +290,11 @@ func TestBuildGraph(t *testing.T) { {Namespace: "test", Name: "hr-1"}: routeHR1, {Namespace: "test", Name: "hr-3"}: routeHR3, }, + ReferencedSecrets: map[types.NamespacedName]*Secret{ + client.ObjectKeyFromObject(secret): { + Source: secret, + }, + }, } } @@ -330,7 +340,6 @@ func TestBuildGraph(t *testing.T) { test.store, controllerName, gcName, - secretMemoryMgr, validation.Validators{HTTPFieldsValidator: &validationfakes.FakeHTTPFieldsValidator{}}, ) diff --git a/internal/state/graph/secret.go b/internal/state/graph/secret.go new file mode 100644 index 0000000000..19d90cf3b4 --- /dev/null +++ b/internal/state/graph/secret.go @@ -0,0 +1,81 @@ +package graph + +import ( + "crypto/tls" + "errors" + "fmt" + + apiv1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" +) + +// Secret represents a Secret resource. +type Secret struct { + // Source holds the actual Secret resource. Can be nil if the Secret does not exist. + Source *apiv1.Secret +} + +type secretEntry struct { + Secret + // err holds the corresponding error if the Secret is invalid or does not exist. + err error +} + +// secretResolver wraps the cluster Secrets so that they can be resolved (includes validation). All resolved +// Secrets are saved to be used later. +type secretResolver struct { + clusterSecrets map[types.NamespacedName]*apiv1.Secret + resolvedSecrets map[types.NamespacedName]*secretEntry +} + +func newSecretResolver(secrets map[types.NamespacedName]*apiv1.Secret) *secretResolver { + return &secretResolver{ + clusterSecrets: secrets, + resolvedSecrets: make(map[types.NamespacedName]*secretEntry), + } +} + +func (r *secretResolver) resolve(nsname types.NamespacedName) error { + if s, resolved := r.resolvedSecrets[nsname]; resolved { + return s.err + } + + secret, exist := r.clusterSecrets[nsname] + + var validationErr error + + if !exist { + validationErr = errors.New("secret does not exist") + } else if secret.Type != apiv1.SecretTypeTLS { + validationErr = fmt.Errorf("secret type must be %q not %q", apiv1.SecretTypeTLS, secret.Type) + } else { + // A TLS Secret is guaranteed to have these data fields. + _, err := tls.X509KeyPair(secret.Data[apiv1.TLSCertKey], secret.Data[apiv1.TLSPrivateKeyKey]) + if err != nil { + validationErr = fmt.Errorf("TLS secret is invalid: %w", err) + } + } + + r.resolvedSecrets[nsname] = &secretEntry{ + Secret: Secret{ + Source: secret, + }, + err: validationErr, + } + + return validationErr +} + +func (r *secretResolver) getResolvedSecrets() map[types.NamespacedName]*Secret { + if len(r.resolvedSecrets) == 0 { + return nil + } + + resolved := make(map[types.NamespacedName]*Secret) + + for nsname, entry := range r.resolvedSecrets { + resolved[nsname] = &entry.Secret + } + + return resolved +} diff --git a/internal/state/graph/secret_test.go b/internal/state/graph/secret_test.go new file mode 100644 index 0000000000..72e4a5f7b7 --- /dev/null +++ b/internal/state/graph/secret_test.go @@ -0,0 +1,220 @@ +package graph + +import ( + "fmt" + "testing" + + . "github.com/onsi/gomega" + apiv1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var ( + cert = []byte(`-----BEGIN CERTIFICATE----- +MIIDLjCCAhYCCQDAOF9tLsaXWjANBgkqhkiG9w0BAQsFADBaMQswCQYDVQQGEwJV +UzELMAkGA1UECAwCQ0ExITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0 +ZDEbMBkGA1UEAwwSY2FmZS5leGFtcGxlLmNvbSAgMB4XDTE4MDkxMjE2MTUzNVoX +DTIzMDkxMTE2MTUzNVowWDELMAkGA1UEBhMCVVMxCzAJBgNVBAgMAkNBMSEwHwYD +VQQKDBhJbnRlcm5ldCBXaWRnaXRzIFB0eSBMdGQxGTAXBgNVBAMMEGNhZmUuZXhh +bXBsZS5jb20wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCp6Kn7sy81 +p0juJ/cyk+vCAmlsfjtFM2muZNK0KtecqG2fjWQb55xQ1YFA2XOSwHAYvSdwI2jZ +ruW8qXXCL2rb4CZCFxwpVECrcxdjm3teViRXVsYImmJHPPSyQgpiobs9x7DlLc6I +BA0ZjUOyl0PqG9SJexMV73WIIa5rDVSF2r4kSkbAj4Dcj7LXeFlVXH2I5XwXCptC +n67JCg42f+k8wgzcRVp8XZkZWZVjwq9RUKDXmFB2YyN1XEWdZ0ewRuKYUJlsm692 +skOrKQj0vkoPn41EE/+TaVEpqLTRoUY3rzg7DkdzfdBizFO2dsPNFx2CW0jXkNLv +Ko25CZrOhXAHAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAKHFCcyOjZvoHswUBMdL +RdHIb383pWFynZq/LuUovsVA58B0Cg7BEfy5vWVVrq5RIkv4lZ81N29x21d1JH6r +jSnQx+DXCO/TJEV5lSCUpIGzEUYaUPgRyjsM/NUdCJ8uHVhZJ+S6FA+CnOD9rn2i +ZBePCI5rHwEXwnnl8ywij3vvQ5zHIuyBglWr/Qyui9fjPpwWUvUm4nv5SMG9zCV7 +PpuwvuatqjO1208BjfE/cZHIg8Hw9mvW9x9C+IQMIMDE7b/g6OcK7LGTLwlFxvA8 +7WjEequnayIphMhKRXVf1N349eN98Ez38fOTHTPbdJjFA/PcC+Gyme+iGt5OQdFh +yRE= +-----END CERTIFICATE-----`) + key = []byte(`-----BEGIN RSA PRIVATE KEY----- +MIIEowIBAAKCAQEAqeip+7MvNadI7if3MpPrwgJpbH47RTNprmTStCrXnKhtn41k +G+ecUNWBQNlzksBwGL0ncCNo2a7lvKl1wi9q2+AmQhccKVRAq3MXY5t7XlYkV1bG +CJpiRzz0skIKYqG7Pcew5S3OiAQNGY1DspdD6hvUiXsTFe91iCGuaw1Uhdq+JEpG +wI+A3I+y13hZVVx9iOV8FwqbQp+uyQoONn/pPMIM3EVafF2ZGVmVY8KvUVCg15hQ +dmMjdVxFnWdHsEbimFCZbJuvdrJDqykI9L5KD5+NRBP/k2lRKai00aFGN684Ow5H +c33QYsxTtnbDzRcdgltI15DS7yqNuQmazoVwBwIDAQABAoIBAQCPSdSYnQtSPyql +FfVFpTOsoOYRhf8sI+ibFxIOuRauWehhJxdm5RORpAzmCLyL5VhjtJme223gLrw2 +N99EjUKb/VOmZuDsBc6oCF6QNR58dz8cnORTewcotsJR1pn1hhlnR5HqJJBJask1 +ZEnUQfcXZrL94lo9JH3E+Uqjo1FFs8xxE8woPBqjZsV7pRUZgC3LhxnwLSExyFo4 +cxb9SOG5OmAJozStFoQ2GJOes8rJ5qfdvytgg9xbLaQL/x0kpQ62BoFMBDdqOePW +KfP5zZ6/07/vpj48yA1Q32PzobubsBLd3Kcn32jfm1E7prtWl+JeOFiOznBQFJbN +4qPVRz5hAoGBANtWyxhNCSLu4P+XgKyckljJ6F5668fNj5CzgFRqJ09zn0TlsNro +FTLZcxDqnR3HPYM42JERh2J/qDFZynRQo3cg3oeivUdBVGY8+FI1W0qdub/L9+yu +edOZTQ5XmGGp6r6jexymcJim/OsB3ZnYOpOrlD7SPmBvzNLk4MF6gxbXAoGBAMZO +0p6HbBmcP0tjFXfcKE77ImLm0sAG4uHoUx0ePj/2qrnTnOBBNE4MvgDuTJzy+caU +k8RqmdHCbHzTe6fzYq/9it8sZ77KVN1qkbIcuc+RTxA9nNh1TjsRne74Z0j1FCLk +hHcqH0ri7PYSKHTE8FvFCxZYdbuB84CmZihvxbpRAoGAIbjqaMYPTYuklCda5S79 +YSFJ1JzZe1Kja//tDw1zFcgVCKa31jAwciz0f/lSRq3HS1GGGmezhPVTiqLfeZqc +R0iKbhgbOcVVkJJ3K0yAyKwPTumxKHZ6zImZS0c0am+RY9YGq5T7YrzpzcfvpiOU +ffe3RyFT7cfCmfoOhDCtzukCgYB30oLC1RLFOrqn43vCS51zc5zoY44uBzspwwYN +TwvP/ExWMf3VJrDjBCH+T/6sysePbJEImlzM+IwytFpANfiIXEt/48Xf60Nx8gWM +uHyxZZx/NKtDw0V8vX1POnq2A5eiKa+8jRARYKJLYNdfDuwolxvG6bZhkPi/4EtT +3Y18sQKBgHtKbk+7lNJVeswXE5cUG6EDUsDe/2Ua7fXp7FcjqBEoap1LSw+6TXp0 +ZgrmKE8ARzM47+EJHUviiq/nupE15g0kJW3syhpU9zZLO7ltB0KIkO9ZRcmUjo8Q +cpLlHMAqbLJ8WYGJCkhiWxyal6hYTyWY4cVkC0xtTl/hUE9IeNKo +-----END RSA PRIVATE KEY-----`) + + invalidCert = []byte(`-----BEGIN CERTIFICATE----- +-----END CERTIFICATE-----`) + invalidKey = []byte(`-----BEGIN RSA PRIVATE KEY----- +-----END RSA PRIVATE KEY-----`) +) + +func TestSecretResolver(t *testing.T) { + var ( + validSecret1 = &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "secret-1", + }, + Data: map[string][]byte{ + apiv1.TLSCertKey: cert, + apiv1.TLSPrivateKeyKey: key, + }, + Type: apiv1.SecretTypeTLS, + } + + validSecret2 = &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "secret-2", + }, + Data: map[string][]byte{ + apiv1.TLSCertKey: cert, + apiv1.TLSPrivateKeyKey: key, + }, + Type: apiv1.SecretTypeTLS, + } + + invalidSecretType = &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "invalid-type", + }, + Data: map[string][]byte{ + apiv1.TLSCertKey: cert, + apiv1.TLSPrivateKeyKey: key, + }, + Type: apiv1.SecretTypeDockercfg, + } + + invalidSecretCert = &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "invalid-cert", + }, + Data: map[string][]byte{ + apiv1.TLSCertKey: invalidCert, + apiv1.TLSPrivateKeyKey: key, + }, + Type: apiv1.SecretTypeTLS, + } + + invalidSecretKey = &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "invalid-key", + }, + Data: map[string][]byte{ + apiv1.TLSCertKey: cert, + apiv1.TLSPrivateKeyKey: invalidKey, + }, + Type: apiv1.SecretTypeTLS, + } + + secretNotExistNsName = types.NamespacedName{ + Namespace: "test", + Name: "not-exist", + } + ) + + resolver := newSecretResolver( + map[types.NamespacedName]*apiv1.Secret{ + client.ObjectKeyFromObject(validSecret1): validSecret1, + client.ObjectKeyFromObject(validSecret2): validSecret2, // we're not going to resolve it + client.ObjectKeyFromObject(invalidSecretType): invalidSecretType, + client.ObjectKeyFromObject(invalidSecretCert): invalidSecretCert, + client.ObjectKeyFromObject(invalidSecretKey): invalidSecretKey, + }) + + tests := []struct { + name string + nsname types.NamespacedName + expectedErrMsg string + }{ + { + name: "valid secret", + nsname: client.ObjectKeyFromObject(validSecret1), + }, + { + name: "valid secret, again", + nsname: client.ObjectKeyFromObject(validSecret1), + }, + { + name: "doesn't exist", + nsname: secretNotExistNsName, + expectedErrMsg: "secret does not exist", + }, + { + name: "invalid secret type", + nsname: client.ObjectKeyFromObject(invalidSecretType), + expectedErrMsg: `secret type must be "kubernetes.io/tls" not "kubernetes.io/dockercfg"`, + }, + { + name: "invalid secret type, again", + nsname: client.ObjectKeyFromObject(invalidSecretType), + expectedErrMsg: `secret type must be "kubernetes.io/tls" not "kubernetes.io/dockercfg"`, + }, + { + name: "invalid secret cert", + nsname: client.ObjectKeyFromObject(invalidSecretCert), + expectedErrMsg: "TLS secret is invalid: x509: malformed certificate", + }, + { + name: "invalid secret key", + nsname: client.ObjectKeyFromObject(invalidSecretKey), + expectedErrMsg: "TLS secret is invalid: tls: failed to parse private key", + }, + } + + // Not running tests with t.Run(...) because the last one (getResolvedSecrets) depends on the execution of + // all cases. + + g := NewGomegaWithT(t) + + for _, test := range tests { + err := resolver.resolve(test.nsname) + if test.expectedErrMsg == "" { + g.Expect(err).To(BeNil(), fmt.Sprintf("case %q", test.name)) + } else { + g.Expect(err).To(MatchError(test.expectedErrMsg), fmt.Sprintf("case %q", test.name)) + } + } + + expectedResolved := map[types.NamespacedName]*Secret{ + client.ObjectKeyFromObject(validSecret1): { + Source: validSecret1, + }, + client.ObjectKeyFromObject(invalidSecretType): { + Source: invalidSecretType, + }, + client.ObjectKeyFromObject(invalidSecretCert): { + Source: invalidSecretCert, + }, + client.ObjectKeyFromObject(invalidSecretKey): { + Source: invalidSecretKey, + }, + secretNotExistNsName: { + Source: nil, + }, + } + + resolved := resolver.getResolvedSecrets() + g.Expect(resolved).To(Equal(expectedResolved), "getResolvedSecrets()") +} diff --git a/internal/state/secrets/file_manager.go b/internal/state/secrets/file_manager.go deleted file mode 100644 index 3daf6d50b2..0000000000 --- a/internal/state/secrets/file_manager.go +++ /dev/null @@ -1,34 +0,0 @@ -package secrets - -import ( - "io/fs" - "os" -) - -type stdLibFileManager struct{} - -func newStdLibFileManager() *stdLibFileManager { - return &stdLibFileManager{} -} - -func (s *stdLibFileManager) ReadDir(dirname string) ([]fs.DirEntry, error) { - return os.ReadDir(dirname) -} - -func (s *stdLibFileManager) Remove(name string) error { - return os.Remove(name) -} - -func (s *stdLibFileManager) Write(file *os.File, contents []byte) error { - _, err := file.Write(contents) - - return err -} - -func (s *stdLibFileManager) Create(name string) (*os.File, error) { - return os.Create(name) -} - -func (s *stdLibFileManager) Chmod(file *os.File, mode os.FileMode) error { - return file.Chmod(mode) -} diff --git a/internal/state/secrets/secrets.go b/internal/state/secrets/secrets.go deleted file mode 100644 index 81102ea16e..0000000000 --- a/internal/state/secrets/secrets.go +++ /dev/null @@ -1,230 +0,0 @@ -package secrets - -import ( - "bytes" - "crypto/tls" - "fmt" - "io/fs" - "os" - "path" - - apiv1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/types" -) - -//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . SecretStore -//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . SecretDiskMemoryManager -//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . FileManager -//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 io/fs.DirEntry - -// tlsSecretFileMode defines the default file mode for files with TLS Secrets. -const tlsSecretFileMode = 0o600 - -// SecretStore stores secrets. -type SecretStore interface { - // Upsert upserts the secret into the store. - Upsert(secret *apiv1.Secret) - // Delete deletes the secret from the store. - Delete(nsname types.NamespacedName) - // Get gets the secret from the store. - Get(nsname types.NamespacedName) *Secret -} - -type SecretStoreImpl struct { - secrets map[types.NamespacedName]*Secret -} - -// Secret is the internal representation of a Kubernetes Secret. -type Secret struct { - // Secret is the Kubernetes Secret object. - Secret *apiv1.Secret - // Valid is whether the Kubernetes Secret is valid. - Valid bool -} - -func NewSecretStore() *SecretStoreImpl { - return &SecretStoreImpl{ - secrets: make(map[types.NamespacedName]*Secret), - } -} - -func (s SecretStoreImpl) Upsert(secret *apiv1.Secret) { - nsname := types.NamespacedName{ - Namespace: secret.Namespace, - Name: secret.Name, - } - - valid := isSecretValid(secret) - s.secrets[nsname] = &Secret{Secret: secret, Valid: valid} -} - -func (s SecretStoreImpl) Delete(nsname types.NamespacedName) { - delete(s.secrets, nsname) -} - -func (s SecretStoreImpl) Get(nsname types.NamespacedName) *Secret { - return s.secrets[nsname] -} - -// SecretDiskMemoryManager manages secrets that are requested by Gateway resources. -type SecretDiskMemoryManager interface { - // Request marks the secret as requested so that it can be written to disk before reloading NGINX. - // Returns the path to the secret if it exists. - // Returns an error if the secret does not exist in the secret store or the secret is invalid. - Request(nsname types.NamespacedName) (string, error) - // WriteAllRequestedSecrets writes all requested secrets to disk. - WriteAllRequestedSecrets() error -} - -// FileManager is an interface that exposes File I/O operations. -// Used for unit testing. -type FileManager interface { - // ReadDir returns the directory entries for the directory. - ReadDir(dirname string) ([]fs.DirEntry, error) - // Remove file with given name. - Remove(name string) error - // Create file at the provided filepath. - Create(name string) (*os.File, error) - // Chmod sets the mode of the file. - Chmod(file *os.File, mode os.FileMode) error - // Write writes contents to the file. - Write(file *os.File, contents []byte) error -} - -// FIXME(kate-osborn): Is it necessary to make this concurrent-safe? -// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/441 -type SecretDiskMemoryManagerImpl struct { - requestedSecrets map[types.NamespacedName]requestedSecret - secretStore SecretStore - fileManager FileManager - secretDirectory string -} - -type requestedSecret struct { - secret *apiv1.Secret - path string -} - -// SecretDiskMemoryManagerOption is a function that modifies the configuration of the SecretDiskMemoryManager. -type SecretDiskMemoryManagerOption func(*SecretDiskMemoryManagerImpl) - -// WithSecretFileManager sets the file manager of the SecretDiskMemoryManager. -// Used to inject a fake fileManager for unit tests. -func WithSecretFileManager(fileManager FileManager) SecretDiskMemoryManagerOption { - return func(mm *SecretDiskMemoryManagerImpl) { - mm.fileManager = fileManager - } -} - -func NewSecretDiskMemoryManager( - secretDirectory string, - secretStore SecretStore, - options ...SecretDiskMemoryManagerOption, -) *SecretDiskMemoryManagerImpl { - sm := &SecretDiskMemoryManagerImpl{ - requestedSecrets: make(map[types.NamespacedName]requestedSecret), - secretStore: secretStore, - secretDirectory: secretDirectory, - fileManager: newStdLibFileManager(), - } - - for _, o := range options { - o(sm) - } - - return sm -} - -func (s *SecretDiskMemoryManagerImpl) Request(nsname types.NamespacedName) (string, error) { - secret := s.secretStore.Get(nsname) - if secret == nil { - return "", fmt.Errorf("secret %s does not exist", nsname) - } - - if !secret.Valid { - return "", fmt.Errorf( - "secret %s is not valid; must be of type %s and contain a valid X509 key pair", - nsname, - apiv1.SecretTypeTLS, - ) - } - - ss := requestedSecret{ - secret: secret.Secret, - path: path.Join(s.secretDirectory, generateFilepathForSecret(nsname)), - } - - s.requestedSecrets[nsname] = ss - - return ss.path, nil -} - -func (s *SecretDiskMemoryManagerImpl) WriteAllRequestedSecrets() error { - // Remove all existing secrets from secrets directory - dir, err := s.fileManager.ReadDir(s.secretDirectory) - if err != nil { - return fmt.Errorf("failed to remove all secrets from %s: %w", s.secretDirectory, err) - } - - for _, d := range dir { - filepath := path.Join(s.secretDirectory, d.Name()) - if err := s.fileManager.Remove(filepath); err != nil { - return fmt.Errorf("failed to remove secret %s: %w", filepath, err) - } - } - - // Write all secrets to secrets directory - for nsname, ss := range s.requestedSecrets { - - file, err := s.fileManager.Create(ss.path) - if err != nil { - return fmt.Errorf("failed to create file %s for secret %s: %w", ss.path, nsname, err) - } - - if err = s.fileManager.Chmod(file, tlsSecretFileMode); err != nil { - return fmt.Errorf( - "failed to change mode of file %s for secret %s: %w", - ss.path, - nsname, - err, - ) - } - - contents := generateCertAndKeyFileContent(ss.secret) - - err = s.fileManager.Write(file, contents) - if err != nil { - return fmt.Errorf("failed to write secret %s to file %s: %w", nsname, ss.path, err) - } - } - - // reset stored secrets - s.requestedSecrets = make(map[types.NamespacedName]requestedSecret) - - return nil -} - -func isSecretValid(secret *apiv1.Secret) bool { - if secret.Type != apiv1.SecretTypeTLS { - return false - } - - // A TLS Secret is guaranteed to have these data fields. - _, err := tls.X509KeyPair(secret.Data[apiv1.TLSCertKey], secret.Data[apiv1.TLSPrivateKeyKey]) - - return err == nil -} - -func generateCertAndKeyFileContent(secret *apiv1.Secret) []byte { - var res bytes.Buffer - - res.Write(secret.Data[apiv1.TLSCertKey]) - res.WriteString("\n") - res.Write(secret.Data[apiv1.TLSPrivateKeyKey]) - - return res.Bytes() -} - -func generateFilepathForSecret(nsname types.NamespacedName) string { - return nsname.Namespace + "_" + nsname.Name -} diff --git a/internal/state/secrets/secrets_test.go b/internal/state/secrets/secrets_test.go deleted file mode 100644 index d24e2d8ad2..0000000000 --- a/internal/state/secrets/secrets_test.go +++ /dev/null @@ -1,385 +0,0 @@ -// nolint:gosec -package secrets_test - -import ( - "errors" - "io/fs" - "os" - "path" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - apiv1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets/secretsfakes" -) - -var ( - cert = []byte(`-----BEGIN CERTIFICATE----- -MIIDLjCCAhYCCQDAOF9tLsaXWjANBgkqhkiG9w0BAQsFADBaMQswCQYDVQQGEwJV -UzELMAkGA1UECAwCQ0ExITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0 -ZDEbMBkGA1UEAwwSY2FmZS5leGFtcGxlLmNvbSAgMB4XDTE4MDkxMjE2MTUzNVoX -DTIzMDkxMTE2MTUzNVowWDELMAkGA1UEBhMCVVMxCzAJBgNVBAgMAkNBMSEwHwYD -VQQKDBhJbnRlcm5ldCBXaWRnaXRzIFB0eSBMdGQxGTAXBgNVBAMMEGNhZmUuZXhh -bXBsZS5jb20wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCp6Kn7sy81 -p0juJ/cyk+vCAmlsfjtFM2muZNK0KtecqG2fjWQb55xQ1YFA2XOSwHAYvSdwI2jZ -ruW8qXXCL2rb4CZCFxwpVECrcxdjm3teViRXVsYImmJHPPSyQgpiobs9x7DlLc6I -BA0ZjUOyl0PqG9SJexMV73WIIa5rDVSF2r4kSkbAj4Dcj7LXeFlVXH2I5XwXCptC -n67JCg42f+k8wgzcRVp8XZkZWZVjwq9RUKDXmFB2YyN1XEWdZ0ewRuKYUJlsm692 -skOrKQj0vkoPn41EE/+TaVEpqLTRoUY3rzg7DkdzfdBizFO2dsPNFx2CW0jXkNLv -Ko25CZrOhXAHAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAKHFCcyOjZvoHswUBMdL -RdHIb383pWFynZq/LuUovsVA58B0Cg7BEfy5vWVVrq5RIkv4lZ81N29x21d1JH6r -jSnQx+DXCO/TJEV5lSCUpIGzEUYaUPgRyjsM/NUdCJ8uHVhZJ+S6FA+CnOD9rn2i -ZBePCI5rHwEXwnnl8ywij3vvQ5zHIuyBglWr/Qyui9fjPpwWUvUm4nv5SMG9zCV7 -PpuwvuatqjO1208BjfE/cZHIg8Hw9mvW9x9C+IQMIMDE7b/g6OcK7LGTLwlFxvA8 -7WjEequnayIphMhKRXVf1N349eN98Ez38fOTHTPbdJjFA/PcC+Gyme+iGt5OQdFh -yRE= ------END CERTIFICATE-----`) - key = []byte(`-----BEGIN RSA PRIVATE KEY----- -MIIEowIBAAKCAQEAqeip+7MvNadI7if3MpPrwgJpbH47RTNprmTStCrXnKhtn41k -G+ecUNWBQNlzksBwGL0ncCNo2a7lvKl1wi9q2+AmQhccKVRAq3MXY5t7XlYkV1bG -CJpiRzz0skIKYqG7Pcew5S3OiAQNGY1DspdD6hvUiXsTFe91iCGuaw1Uhdq+JEpG -wI+A3I+y13hZVVx9iOV8FwqbQp+uyQoONn/pPMIM3EVafF2ZGVmVY8KvUVCg15hQ -dmMjdVxFnWdHsEbimFCZbJuvdrJDqykI9L5KD5+NRBP/k2lRKai00aFGN684Ow5H -c33QYsxTtnbDzRcdgltI15DS7yqNuQmazoVwBwIDAQABAoIBAQCPSdSYnQtSPyql -FfVFpTOsoOYRhf8sI+ibFxIOuRauWehhJxdm5RORpAzmCLyL5VhjtJme223gLrw2 -N99EjUKb/VOmZuDsBc6oCF6QNR58dz8cnORTewcotsJR1pn1hhlnR5HqJJBJask1 -ZEnUQfcXZrL94lo9JH3E+Uqjo1FFs8xxE8woPBqjZsV7pRUZgC3LhxnwLSExyFo4 -cxb9SOG5OmAJozStFoQ2GJOes8rJ5qfdvytgg9xbLaQL/x0kpQ62BoFMBDdqOePW -KfP5zZ6/07/vpj48yA1Q32PzobubsBLd3Kcn32jfm1E7prtWl+JeOFiOznBQFJbN -4qPVRz5hAoGBANtWyxhNCSLu4P+XgKyckljJ6F5668fNj5CzgFRqJ09zn0TlsNro -FTLZcxDqnR3HPYM42JERh2J/qDFZynRQo3cg3oeivUdBVGY8+FI1W0qdub/L9+yu -edOZTQ5XmGGp6r6jexymcJim/OsB3ZnYOpOrlD7SPmBvzNLk4MF6gxbXAoGBAMZO -0p6HbBmcP0tjFXfcKE77ImLm0sAG4uHoUx0ePj/2qrnTnOBBNE4MvgDuTJzy+caU -k8RqmdHCbHzTe6fzYq/9it8sZ77KVN1qkbIcuc+RTxA9nNh1TjsRne74Z0j1FCLk -hHcqH0ri7PYSKHTE8FvFCxZYdbuB84CmZihvxbpRAoGAIbjqaMYPTYuklCda5S79 -YSFJ1JzZe1Kja//tDw1zFcgVCKa31jAwciz0f/lSRq3HS1GGGmezhPVTiqLfeZqc -R0iKbhgbOcVVkJJ3K0yAyKwPTumxKHZ6zImZS0c0am+RY9YGq5T7YrzpzcfvpiOU -ffe3RyFT7cfCmfoOhDCtzukCgYB30oLC1RLFOrqn43vCS51zc5zoY44uBzspwwYN -TwvP/ExWMf3VJrDjBCH+T/6sysePbJEImlzM+IwytFpANfiIXEt/48Xf60Nx8gWM -uHyxZZx/NKtDw0V8vX1POnq2A5eiKa+8jRARYKJLYNdfDuwolxvG6bZhkPi/4EtT -3Y18sQKBgHtKbk+7lNJVeswXE5cUG6EDUsDe/2Ua7fXp7FcjqBEoap1LSw+6TXp0 -ZgrmKE8ARzM47+EJHUviiq/nupE15g0kJW3syhpU9zZLO7ltB0KIkO9ZRcmUjo8Q -cpLlHMAqbLJ8WYGJCkhiWxyal6hYTyWY4cVkC0xtTl/hUE9IeNKo ------END RSA PRIVATE KEY-----`) - - invalidCert = []byte(`-----BEGIN CERTIFICATE----- ------END CERTIFICATE-----`) - - invalidKey = []byte(`-----BEGIN RSA PRIVATE KEY----- ------END RSA PRIVATE KEY-----`) -) - -var ( - secret1 = &apiv1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "secret1", - }, - Data: map[string][]byte{ - apiv1.TLSCertKey: cert, - apiv1.TLSPrivateKeyKey: key, - }, - Type: apiv1.SecretTypeTLS, - } - - secret2 = &apiv1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "secret2", - }, - Data: map[string][]byte{ - apiv1.TLSCertKey: cert, - apiv1.TLSPrivateKeyKey: key, - }, - Type: apiv1.SecretTypeTLS, - } - - secret3 = &apiv1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "secret3", - }, - Data: map[string][]byte{ - apiv1.TLSCertKey: cert, - apiv1.TLSPrivateKeyKey: key, - }, - Type: apiv1.SecretTypeTLS, - } - - invalidSecretType = &apiv1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "invalid-type", - }, - Data: map[string][]byte{ - apiv1.TLSCertKey: cert, - apiv1.TLSPrivateKeyKey: key, - }, - Type: apiv1.SecretTypeDockercfg, - } - invalidSecretKey = &apiv1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "invalid-key", - }, - Data: map[string][]byte{ - apiv1.TLSCertKey: cert, - apiv1.TLSPrivateKeyKey: invalidKey, - }, - Type: apiv1.SecretTypeTLS, - } -) - -var _ = Describe("SecretDiskMemoryManager", func() { - var ( - fakeStore *secretsfakes.FakeSecretStore - memMgr secrets.SecretDiskMemoryManager - tmpSecretsDir string - ) - - BeforeEach(OncePerOrdered, func() { - dir, err := os.MkdirTemp("", "secrets-test") - tmpSecretsDir = dir - Expect(err).ToNot(HaveOccurred(), "failed to create temp directory for tests") - - fakeStore = &secretsfakes.FakeSecretStore{} - memMgr = secrets.NewSecretDiskMemoryManager(tmpSecretsDir, fakeStore) - }) - - AfterEach(OncePerOrdered, func() { - Expect(os.RemoveAll(tmpSecretsDir)).To(Succeed()) - }) - - Describe("Manages secrets on disk", Ordered, func() { - testRequest := func(s *apiv1.Secret, expPath string, expErr bool) { - nsname := types.NamespacedName{Namespace: s.Namespace, Name: s.Name} - actualPath, err := memMgr.Request(nsname) - - if expErr { - Expect(err).To(HaveOccurred()) - Expect(actualPath).To(BeEmpty()) - } else { - Expect(err).ToNot(HaveOccurred()) - Expect(actualPath).To(Equal(expPath)) - } - } - - It("should return an error and empty path when secret does not exist", func() { - fakeStore.GetReturns(nil) - - testRequest(secret1, "", true) - }) - It("request should return the file path for a valid secret", func() { - fakeStore.GetReturns(&secrets.Secret{Secret: secret1, Valid: true}) - expectedPath := path.Join(tmpSecretsDir, "test_secret1") - - testRequest(secret1, expectedPath, false) - }) - - It("request should return the file path for another valid secret", func() { - fakeStore.GetReturns(&secrets.Secret{Secret: secret2, Valid: true}) - expectedPath := path.Join(tmpSecretsDir, "test_secret2") - - testRequest(secret2, expectedPath, false) - }) - - It("request should return an error and empty path when secret is invalid", func() { - fakeStore.GetReturns(&secrets.Secret{Secret: invalidSecretType, Valid: false}) - - testRequest(invalidSecretType, "", true) - }) - - It("should write all requested secrets", func() { - err := memMgr.WriteAllRequestedSecrets() - Expect(err).ToNot(HaveOccurred()) - - expectedFileNames := []string{"test_secret1", "test_secret2"} - - // read all files from directory - dir, err := os.ReadDir(tmpSecretsDir) - Expect(err).ToNot(HaveOccurred()) - - // test that the files exist that we expect - Expect(dir).To(HaveLen(2)) - actualFilenames := []string{dir[0].Name(), dir[1].Name()} - Expect(actualFilenames).To(ConsistOf(expectedFileNames)) - }) - - It("request should return the file path for secret after write", func() { - fakeStore.GetReturns(&secrets.Secret{Secret: secret3, Valid: true}) - expectedPath := path.Join(tmpSecretsDir, "test_secret3") - - testRequest(secret3, expectedPath, false) - }) - - It("should write all requested secrets", func() { - err := memMgr.WriteAllRequestedSecrets() - Expect(err).ToNot(HaveOccurred()) - - // read all files from directory - dir, err := os.ReadDir(tmpSecretsDir) - Expect(err).ToNot(HaveOccurred()) - - // only the secrets stored after the last write should be written to disk. - Expect(dir).To(HaveLen(1)) - Expect(dir[0].Name()).To(Equal("test_secret3")) - }) - When("no secrets are requested", func() { - It("write all secrets should remove all existing secrets and write no additional secrets", func() { - err := memMgr.WriteAllRequestedSecrets() - Expect(err).ToNot(HaveOccurred()) - - // read all files from directory - dir, err := os.ReadDir(tmpSecretsDir) - Expect(err).ToNot(HaveOccurred()) - - // no secrets should exist - Expect(dir).To(BeEmpty()) - }) - }) - }) - Describe("Write all requested secrets", func() { - var ( - fakeFileManager *secretsfakes.FakeFileManager - fakeStore *secretsfakes.FakeSecretStore - fakeDirEntries []fs.DirEntry - memMgr *secrets.SecretDiskMemoryManagerImpl - ) - - BeforeEach(OncePerOrdered, func() { - fakeFileManager = &secretsfakes.FakeFileManager{} - fakeStore = &secretsfakes.FakeSecretStore{} - fakeDirEntries = []fs.DirEntry{&secretsfakes.FakeDirEntry{}} - memMgr = secrets.NewSecretDiskMemoryManager("", fakeStore, secrets.WithSecretFileManager(fakeFileManager)) - - // populate a requested secret - fakeStore.GetReturns(&secrets.Secret{Secret: secret1, Valid: true}) - _, err := memMgr.Request(types.NamespacedName{Namespace: secret1.Namespace, Name: secret1.Name}) - Expect(err).ToNot(HaveOccurred()) - }) - - DescribeTable("error cases", Ordered, - func(e error, preparer func(e error)) { - preparer(e) - - err := memMgr.WriteAllRequestedSecrets() - Expect(err).To(MatchError(e)) - }, - Entry("read directory error", errors.New("read dir"), - func(e error) { - fakeFileManager.ReadDirReturns(nil, e) - }), - Entry("remove file error", errors.New("remove file"), - func(e error) { - fakeFileManager.ReadDirReturns(fakeDirEntries, nil) - fakeFileManager.RemoveReturns(e) - }), - Entry("create file error", errors.New("create error"), - func(e error) { - fakeFileManager.RemoveReturns(nil) - fakeFileManager.CreateReturns(nil, e) - }), - Entry("chmod error", errors.New("chmod"), - func(e error) { - fakeFileManager.CreateReturns(&os.File{}, nil) - fakeFileManager.ChmodReturns(e) - }), - Entry("write error", errors.New("write"), - func(e error) { - fakeFileManager.ChmodReturns(nil) - fakeFileManager.WriteReturns(e) - }), - ) - }) -}) - -var _ = Describe("SecretStore", func() { - var store secrets.SecretStore - var invalidToValidSecret, validToInvalidSecret *apiv1.Secret - - BeforeEach(OncePerOrdered, func() { - store = secrets.NewSecretStore() - - invalidToValidSecret = invalidSecretType.DeepCopy() - invalidToValidSecret.Type = apiv1.SecretTypeTLS - - validToInvalidSecret = secret1.DeepCopy() - validToInvalidSecret.Data[apiv1.TLSCertKey] = invalidCert - }) - Describe("handles CRUD events on secrets", Ordered, func() { - testUpsert := func(s *apiv1.Secret, valid bool) { - store.Upsert(s) - - nsname := types.NamespacedName{Namespace: s.Namespace, Name: s.Name} - actualSecret := store.Get(nsname) - if valid { - Expect(actualSecret.Valid).To(BeTrue()) - } else { - Expect(actualSecret.Valid).To(BeFalse()) - } - Expect(actualSecret.Secret).To(Equal(s)) - } - - testDelete := func(nsname types.NamespacedName) { - store.Delete(nsname) - - s := store.Get(nsname) - Expect(s).To(BeNil()) - } - - It("adds a new valid secret", func() { - testUpsert(secret1, true) - }) - It("adds another new valid secret", func() { - testUpsert(secret2, true) - }) - It("adds a secret with an invalid type", func() { - testUpsert(invalidSecretType, false) - }) - It("adds a secret with an invalid key", func() { - testUpsert(invalidSecretKey, false) - }) - It("deletes an invalid secret", func() { - nsname := types.NamespacedName{Namespace: "test", Name: "invalid-key"} - - testDelete(nsname) - }) - It("updates an invalid secret to valid", func() { - testUpsert(invalidToValidSecret, true) - }) - It("updates an valid secret to invalid (invalid cert)", func() { - testUpsert(validToInvalidSecret, false) - }) - It("deletes a secret", func() { - nsname := types.NamespacedName{Namespace: "test", Name: "invalid-type"} - - testDelete(nsname) - }) - It("deletes a secret", func() { - nsname := types.NamespacedName{Namespace: "test", Name: "secret1"} - - testDelete(nsname) - }) - It("gets remaining secret", func() { - nsname := types.NamespacedName{Namespace: "test", Name: "secret2"} - - s := store.Get(nsname) - Expect(s.Valid).To(BeTrue()) - Expect(s.Secret).To(Equal(secret2)) - }) - It("deletes final secret", func() { - nsname := types.NamespacedName{Namespace: "test", Name: "secret2"} - - testDelete(nsname) - }) - It("does not panic when secret is deleted that does not exist", func() { - nsname := types.NamespacedName{Namespace: "test", Name: "dne"} - - store.Delete(nsname) - }) - }) -}) diff --git a/internal/state/secrets/secretsfakes/fake_secret_disk_memory_manager.go b/internal/state/secrets/secretsfakes/fake_secret_disk_memory_manager.go deleted file mode 100644 index 3fe9f5943d..0000000000 --- a/internal/state/secrets/secretsfakes/fake_secret_disk_memory_manager.go +++ /dev/null @@ -1,182 +0,0 @@ -// Code generated by counterfeiter. DO NOT EDIT. -package secretsfakes - -import ( - "sync" - - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets" - "k8s.io/apimachinery/pkg/types" -) - -type FakeSecretDiskMemoryManager struct { - RequestStub func(types.NamespacedName) (string, error) - requestMutex sync.RWMutex - requestArgsForCall []struct { - arg1 types.NamespacedName - } - requestReturns struct { - result1 string - result2 error - } - requestReturnsOnCall map[int]struct { - result1 string - result2 error - } - WriteAllRequestedSecretsStub func() error - writeAllRequestedSecretsMutex sync.RWMutex - writeAllRequestedSecretsArgsForCall []struct { - } - writeAllRequestedSecretsReturns struct { - result1 error - } - writeAllRequestedSecretsReturnsOnCall map[int]struct { - result1 error - } - invocations map[string][][]interface{} - invocationsMutex sync.RWMutex -} - -func (fake *FakeSecretDiskMemoryManager) Request(arg1 types.NamespacedName) (string, error) { - fake.requestMutex.Lock() - ret, specificReturn := fake.requestReturnsOnCall[len(fake.requestArgsForCall)] - fake.requestArgsForCall = append(fake.requestArgsForCall, struct { - arg1 types.NamespacedName - }{arg1}) - stub := fake.RequestStub - fakeReturns := fake.requestReturns - fake.recordInvocation("Request", []interface{}{arg1}) - fake.requestMutex.Unlock() - if stub != nil { - return stub(arg1) - } - if specificReturn { - return ret.result1, ret.result2 - } - return fakeReturns.result1, fakeReturns.result2 -} - -func (fake *FakeSecretDiskMemoryManager) RequestCallCount() int { - fake.requestMutex.RLock() - defer fake.requestMutex.RUnlock() - return len(fake.requestArgsForCall) -} - -func (fake *FakeSecretDiskMemoryManager) RequestCalls(stub func(types.NamespacedName) (string, error)) { - fake.requestMutex.Lock() - defer fake.requestMutex.Unlock() - fake.RequestStub = stub -} - -func (fake *FakeSecretDiskMemoryManager) RequestArgsForCall(i int) types.NamespacedName { - fake.requestMutex.RLock() - defer fake.requestMutex.RUnlock() - argsForCall := fake.requestArgsForCall[i] - return argsForCall.arg1 -} - -func (fake *FakeSecretDiskMemoryManager) RequestReturns(result1 string, result2 error) { - fake.requestMutex.Lock() - defer fake.requestMutex.Unlock() - fake.RequestStub = nil - fake.requestReturns = struct { - result1 string - result2 error - }{result1, result2} -} - -func (fake *FakeSecretDiskMemoryManager) RequestReturnsOnCall(i int, result1 string, result2 error) { - fake.requestMutex.Lock() - defer fake.requestMutex.Unlock() - fake.RequestStub = nil - if fake.requestReturnsOnCall == nil { - fake.requestReturnsOnCall = make(map[int]struct { - result1 string - result2 error - }) - } - fake.requestReturnsOnCall[i] = struct { - result1 string - result2 error - }{result1, result2} -} - -func (fake *FakeSecretDiskMemoryManager) WriteAllRequestedSecrets() error { - fake.writeAllRequestedSecretsMutex.Lock() - ret, specificReturn := fake.writeAllRequestedSecretsReturnsOnCall[len(fake.writeAllRequestedSecretsArgsForCall)] - fake.writeAllRequestedSecretsArgsForCall = append(fake.writeAllRequestedSecretsArgsForCall, struct { - }{}) - stub := fake.WriteAllRequestedSecretsStub - fakeReturns := fake.writeAllRequestedSecretsReturns - fake.recordInvocation("WriteAllRequestedSecrets", []interface{}{}) - fake.writeAllRequestedSecretsMutex.Unlock() - if stub != nil { - return stub() - } - if specificReturn { - return ret.result1 - } - return fakeReturns.result1 -} - -func (fake *FakeSecretDiskMemoryManager) WriteAllRequestedSecretsCallCount() int { - fake.writeAllRequestedSecretsMutex.RLock() - defer fake.writeAllRequestedSecretsMutex.RUnlock() - return len(fake.writeAllRequestedSecretsArgsForCall) -} - -func (fake *FakeSecretDiskMemoryManager) WriteAllRequestedSecretsCalls(stub func() error) { - fake.writeAllRequestedSecretsMutex.Lock() - defer fake.writeAllRequestedSecretsMutex.Unlock() - fake.WriteAllRequestedSecretsStub = stub -} - -func (fake *FakeSecretDiskMemoryManager) WriteAllRequestedSecretsReturns(result1 error) { - fake.writeAllRequestedSecretsMutex.Lock() - defer fake.writeAllRequestedSecretsMutex.Unlock() - fake.WriteAllRequestedSecretsStub = nil - fake.writeAllRequestedSecretsReturns = struct { - result1 error - }{result1} -} - -func (fake *FakeSecretDiskMemoryManager) WriteAllRequestedSecretsReturnsOnCall(i int, result1 error) { - fake.writeAllRequestedSecretsMutex.Lock() - defer fake.writeAllRequestedSecretsMutex.Unlock() - fake.WriteAllRequestedSecretsStub = nil - if fake.writeAllRequestedSecretsReturnsOnCall == nil { - fake.writeAllRequestedSecretsReturnsOnCall = make(map[int]struct { - result1 error - }) - } - fake.writeAllRequestedSecretsReturnsOnCall[i] = struct { - result1 error - }{result1} -} - -func (fake *FakeSecretDiskMemoryManager) Invocations() map[string][][]interface{} { - fake.invocationsMutex.RLock() - defer fake.invocationsMutex.RUnlock() - fake.requestMutex.RLock() - defer fake.requestMutex.RUnlock() - fake.writeAllRequestedSecretsMutex.RLock() - defer fake.writeAllRequestedSecretsMutex.RUnlock() - copiedInvocations := map[string][][]interface{}{} - for key, value := range fake.invocations { - copiedInvocations[key] = value - } - return copiedInvocations -} - -func (fake *FakeSecretDiskMemoryManager) recordInvocation(key string, args []interface{}) { - fake.invocationsMutex.Lock() - defer fake.invocationsMutex.Unlock() - if fake.invocations == nil { - fake.invocations = map[string][][]interface{}{} - } - if fake.invocations[key] == nil { - fake.invocations[key] = [][]interface{}{} - } - fake.invocations[key] = append(fake.invocations[key], args) -} - -var _ secrets.SecretDiskMemoryManager = new(FakeSecretDiskMemoryManager) diff --git a/internal/state/secrets/secretsfakes/fake_secret_store.go b/internal/state/secrets/secretsfakes/fake_secret_store.go deleted file mode 100644 index d6538e0085..0000000000 --- a/internal/state/secrets/secretsfakes/fake_secret_store.go +++ /dev/null @@ -1,191 +0,0 @@ -// Code generated by counterfeiter. DO NOT EDIT. -package secretsfakes - -import ( - "sync" - - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets" - v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/types" -) - -type FakeSecretStore struct { - DeleteStub func(types.NamespacedName) - deleteMutex sync.RWMutex - deleteArgsForCall []struct { - arg1 types.NamespacedName - } - GetStub func(types.NamespacedName) *secrets.Secret - getMutex sync.RWMutex - getArgsForCall []struct { - arg1 types.NamespacedName - } - getReturns struct { - result1 *secrets.Secret - } - getReturnsOnCall map[int]struct { - result1 *secrets.Secret - } - UpsertStub func(*v1.Secret) - upsertMutex sync.RWMutex - upsertArgsForCall []struct { - arg1 *v1.Secret - } - invocations map[string][][]interface{} - invocationsMutex sync.RWMutex -} - -func (fake *FakeSecretStore) Delete(arg1 types.NamespacedName) { - fake.deleteMutex.Lock() - fake.deleteArgsForCall = append(fake.deleteArgsForCall, struct { - arg1 types.NamespacedName - }{arg1}) - stub := fake.DeleteStub - fake.recordInvocation("Delete", []interface{}{arg1}) - fake.deleteMutex.Unlock() - if stub != nil { - fake.DeleteStub(arg1) - } -} - -func (fake *FakeSecretStore) DeleteCallCount() int { - fake.deleteMutex.RLock() - defer fake.deleteMutex.RUnlock() - return len(fake.deleteArgsForCall) -} - -func (fake *FakeSecretStore) DeleteCalls(stub func(types.NamespacedName)) { - fake.deleteMutex.Lock() - defer fake.deleteMutex.Unlock() - fake.DeleteStub = stub -} - -func (fake *FakeSecretStore) DeleteArgsForCall(i int) types.NamespacedName { - fake.deleteMutex.RLock() - defer fake.deleteMutex.RUnlock() - argsForCall := fake.deleteArgsForCall[i] - return argsForCall.arg1 -} - -func (fake *FakeSecretStore) Get(arg1 types.NamespacedName) *secrets.Secret { - fake.getMutex.Lock() - ret, specificReturn := fake.getReturnsOnCall[len(fake.getArgsForCall)] - fake.getArgsForCall = append(fake.getArgsForCall, struct { - arg1 types.NamespacedName - }{arg1}) - stub := fake.GetStub - fakeReturns := fake.getReturns - fake.recordInvocation("Get", []interface{}{arg1}) - fake.getMutex.Unlock() - if stub != nil { - return stub(arg1) - } - if specificReturn { - return ret.result1 - } - return fakeReturns.result1 -} - -func (fake *FakeSecretStore) GetCallCount() int { - fake.getMutex.RLock() - defer fake.getMutex.RUnlock() - return len(fake.getArgsForCall) -} - -func (fake *FakeSecretStore) GetCalls(stub func(types.NamespacedName) *secrets.Secret) { - fake.getMutex.Lock() - defer fake.getMutex.Unlock() - fake.GetStub = stub -} - -func (fake *FakeSecretStore) GetArgsForCall(i int) types.NamespacedName { - fake.getMutex.RLock() - defer fake.getMutex.RUnlock() - argsForCall := fake.getArgsForCall[i] - return argsForCall.arg1 -} - -func (fake *FakeSecretStore) GetReturns(result1 *secrets.Secret) { - fake.getMutex.Lock() - defer fake.getMutex.Unlock() - fake.GetStub = nil - fake.getReturns = struct { - result1 *secrets.Secret - }{result1} -} - -func (fake *FakeSecretStore) GetReturnsOnCall(i int, result1 *secrets.Secret) { - fake.getMutex.Lock() - defer fake.getMutex.Unlock() - fake.GetStub = nil - if fake.getReturnsOnCall == nil { - fake.getReturnsOnCall = make(map[int]struct { - result1 *secrets.Secret - }) - } - fake.getReturnsOnCall[i] = struct { - result1 *secrets.Secret - }{result1} -} - -func (fake *FakeSecretStore) Upsert(arg1 *v1.Secret) { - fake.upsertMutex.Lock() - fake.upsertArgsForCall = append(fake.upsertArgsForCall, struct { - arg1 *v1.Secret - }{arg1}) - stub := fake.UpsertStub - fake.recordInvocation("Upsert", []interface{}{arg1}) - fake.upsertMutex.Unlock() - if stub != nil { - fake.UpsertStub(arg1) - } -} - -func (fake *FakeSecretStore) UpsertCallCount() int { - fake.upsertMutex.RLock() - defer fake.upsertMutex.RUnlock() - return len(fake.upsertArgsForCall) -} - -func (fake *FakeSecretStore) UpsertCalls(stub func(*v1.Secret)) { - fake.upsertMutex.Lock() - defer fake.upsertMutex.Unlock() - fake.UpsertStub = stub -} - -func (fake *FakeSecretStore) UpsertArgsForCall(i int) *v1.Secret { - fake.upsertMutex.RLock() - defer fake.upsertMutex.RUnlock() - argsForCall := fake.upsertArgsForCall[i] - return argsForCall.arg1 -} - -func (fake *FakeSecretStore) Invocations() map[string][][]interface{} { - fake.invocationsMutex.RLock() - defer fake.invocationsMutex.RUnlock() - fake.deleteMutex.RLock() - defer fake.deleteMutex.RUnlock() - fake.getMutex.RLock() - defer fake.getMutex.RUnlock() - fake.upsertMutex.RLock() - defer fake.upsertMutex.RUnlock() - copiedInvocations := map[string][][]interface{}{} - for key, value := range fake.invocations { - copiedInvocations[key] = value - } - return copiedInvocations -} - -func (fake *FakeSecretStore) recordInvocation(key string, args []interface{}) { - fake.invocationsMutex.Lock() - defer fake.invocationsMutex.Unlock() - if fake.invocations == nil { - fake.invocations = map[string][][]interface{}{} - } - if fake.invocations[key] == nil { - fake.invocations[key] = [][]interface{}{} - } - fake.invocations[key] = append(fake.invocations[key], args) -} - -var _ secrets.SecretStore = new(FakeSecretStore) diff --git a/internal/state/store.go b/internal/state/store.go index 0284dd0371..289e7ce951 100644 --- a/internal/state/store.go +++ b/internal/state/store.go @@ -114,6 +114,9 @@ type changeTrackingUpdaterObjectTypeCfg struct { trackUpsertDelete bool } +// triggerStateChangeFunc triggers a change to the changeTrackingUpdater's store for the given object. +type triggerStateChangeFunc func(objType client.Object, nsname types.NamespacedName) bool + // changeTrackingUpdater is an Updater that tracks changes to the cluster state in the multiObjectStore. // // It only works with objects with the GVKs registered in changeTrackingUpdaterObjectTypeCfg. Otherwise, it panics. @@ -123,9 +126,11 @@ type changeTrackingUpdaterObjectTypeCfg struct { // that its generation changed. // - An object is upserted or deleted, and it is related to another object, based on the decision by // the relationship capturer. +// - An object is upserted or deleted and triggerStateChange returns true for the object. type changeTrackingUpdater struct { - store *multiObjectStore - capturer relationship.Capturer + store *multiObjectStore + capturer relationship.Capturer + triggerStateChange triggerStateChangeFunc extractGVK extractGVKFunc supportedGVKs gvkList @@ -137,6 +142,7 @@ type changeTrackingUpdater struct { func newChangeTrackingUpdater( capturer relationship.Capturer, + triggerStateChange triggerStateChangeFunc, extractGVK extractGVKFunc, objectTypeCfgs []changeTrackingUpdaterObjectTypeCfg, ) *changeTrackingUpdater { @@ -168,6 +174,7 @@ func newChangeTrackingUpdater( trackedUpsertDeleteGVKs: trackedUpsertDeleteGVKs, persistedGVKs: persistedGVKs, capturer: capturer, + triggerStateChange: triggerStateChange, } } @@ -202,7 +209,14 @@ func (s *changeTrackingUpdater) Upsert(obj client.Object) { relationshipExists := s.capturer.Exists(obj, client.ObjectKeyFromObject(obj)) - s.changed = s.changed || changingUpsert || relationshipExisted || relationshipExists + forceChanged := s.triggerStateChange(obj, client.ObjectKeyFromObject(obj)) + + // FIXME(pleshakov): Check generation in all cases to minimize the number of Graph regeneration. + // s.changed can be true even if the generation of the object did not change, because + // capturer and triggerStateChange don't take the generation into account. + // See https://github.com/nginxinc/nginx-kubernetes-gateway/issues/825 + + s.changed = s.changed || changingUpsert || relationshipExisted || relationshipExists || forceChanged } func (s *changeTrackingUpdater) delete(objType client.Object, nsname types.NamespacedName) (changed bool) { @@ -226,7 +240,9 @@ func (s *changeTrackingUpdater) Delete(objType client.Object, nsname types.Names changingDelete := s.delete(objType, nsname) - s.changed = s.changed || changingDelete || s.capturer.Exists(objType, nsname) + forceChanged := s.triggerStateChange(objType, nsname) + + s.changed = s.changed || changingDelete || s.capturer.Exists(objType, nsname) || forceChanged s.capturer.Remove(objType, nsname) }