From c91771f22f45a685792920bd1b937aff1fc4e863 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 24 May 2022 16:08:45 -0700 Subject: [PATCH] Support processing multiple HTTP listeners Problem: The Gateway needs to process a Gateway resource. Currently, that is already supported by the 'state' package. However, that support is limited. For example, the package doesn't allow processing multiple listeners. More over, the approach used in the 'state' package is hard to extend to support multiple listeners. This prompts development of a better solution. As a first deliverable, we want to support processing multiple HTTP listeners assuming they all bind to port 80. We also assume that the Gateway only supports a single Gateway resource with known namespace and name. Solution - replace the existing types of the state package with the new ones: - Introduce Configuration, which is an internal representation of the Gateway configuration. - Introduce Statuses, which holds status-related information about processed resources. - Introduce ChangeProcessor, which processes changes to Gateway API resources and returns new Configuration and Statuses. --- README.md | 7 +- cmd/gateway/main.go | 6 + deploy/manifests/gateway.yaml | 5 +- deploy/manifests/nginx-gateway.yaml | 1 + examples/advanced-routing/cafe-routes.yaml | 12 + examples/cafe-example/cafe-routes.yaml | 12 + internal/config/config.go | 4 + internal/events/loop.go | 142 +- internal/events/loop_test.go | 242 ++-- internal/implementations/gateway/gateway.go | 52 +- internal/manager/manager.go | 11 +- .../config/configfakes/fake_generator.go | 82 +- internal/nginx/config/generator.go | 42 +- internal/nginx/config/generator_test.go | 26 +- internal/nginx/config/http.go | 4 + internal/nginx/config/template.go | 26 +- internal/nginx/config/template_test.go | 22 +- internal/nginx/config/warnings.go | 8 + internal/nginx/config/warnings_test.go | 86 ++ internal/nginx/file/filefakes/fake_manager.go | 152 +-- internal/nginx/file/manager.go | 21 +- internal/state/change_processor.go | 111 ++ internal/state/change_processor_test.go | 452 +++++++ internal/state/clock.go | 37 - internal/state/clock_test.go | 16 - internal/state/configuration.go | 522 ++------ internal/state/configuration_test.go | 1167 +++++------------ internal/state/graph.go | 225 ++++ internal/state/graph_test.go | 615 +++++++++ internal/state/helpers.go | 13 + internal/state/helpers_test.go | 20 + ...ate_suit_test.go => newstate_suit_test.go} | 6 +- internal/state/services.go | 5 + internal/state/sort.go | 34 - .../state/statefakes/fake_change_processor.go | 194 +++ .../state/statefakes/fake_configuration.go | 197 --- internal/state/statuses.go | 73 ++ internal/state/statuses_test.go | 56 + internal/state/store.go | 18 + internal/status/clock.go | 26 + internal/status/gateway.go | 68 + internal/status/gateway_test.go | 73 ++ internal/status/httproute.go | 77 ++ internal/status/httproute_test.go | 78 ++ internal/status/statusfakes/fake_clock.go | 103 ++ internal/status/statusfakes/fake_updater.go | 59 +- internal/status/updater.go | 88 +- internal/status/updater_test.go | 239 ++-- pkg/sdk/gateway_controller.go | 2 +- pkg/sdk/interfaces.go | 2 +- 50 files changed, 3365 insertions(+), 2174 deletions(-) create mode 100644 internal/state/change_processor.go create mode 100644 internal/state/change_processor_test.go delete mode 100644 internal/state/clock.go delete mode 100644 internal/state/clock_test.go create mode 100644 internal/state/graph.go create mode 100644 internal/state/graph_test.go create mode 100644 internal/state/helpers.go create mode 100644 internal/state/helpers_test.go rename internal/state/{state_suit_test.go => newstate_suit_test.go} (84%) delete mode 100644 internal/state/sort.go create mode 100644 internal/state/statefakes/fake_change_processor.go delete mode 100644 internal/state/statefakes/fake_configuration.go create mode 100644 internal/state/statuses.go create mode 100644 internal/state/statuses_test.go create mode 100644 internal/state/store.go create mode 100644 internal/status/clock.go create mode 100644 internal/status/gateway.go create mode 100644 internal/status/gateway_test.go create mode 100644 internal/status/httproute.go create mode 100644 internal/status/httproute_test.go create mode 100644 internal/status/statusfakes/fake_clock.go diff --git a/README.md b/README.md index d79b9cefa0..1a764270c0 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,6 @@ Before you can build and run the NGINX Kubernetes Gateway, make sure you have th Set the `PREFIX` variable to the name of the registry you'd like to push the image to. By default, the image will be named `nginx-kubernetes-gateway:0.0.1`. - 1. Push the image to your container registry: ``` @@ -55,7 +54,6 @@ You can deploy NGINX Kubernetes Gateway on an existing Kubernetes 1.16+ cluster. Make sure to substitute the image name with the name of the image you built. - 1. Install the Gateway CRDs: ``` @@ -89,6 +87,11 @@ You can deploy NGINX Kubernetes Gateway on an existing Kubernetes 1.16+ cluster. NAME READY STATUS RESTARTS AGE nginx-gateway-5d4f4c7db7-xk2kq 2/2 Running 0 112s ``` +1. Create the Gateway resource: + + ``` + kubectl apply -f deploy/manifests/gateway.yaml + ``` ## Expose NGINX Kubernetes Gateway diff --git a/cmd/gateway/main.go b/cmd/gateway/main.go index 21486d88dd..5365a875d1 100644 --- a/cmd/gateway/main.go +++ b/cmd/gateway/main.go @@ -5,6 +5,7 @@ import ( "os" flag "github.com/spf13/pflag" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/log/zap" "github.com/nginxinc/nginx-kubernetes-gateway/internal/config" @@ -36,6 +37,11 @@ func main() { conf := config.Config{ GatewayCtlrName: *gatewayCtlrName, Logger: logger, + // FIXME(pleshakov) Allow the cluster operator to customize this value + GatewayNsName: types.NamespacedName{ + Namespace: "nginx-gateway", + Name: "gateway", + }, } MustValidateArguments( diff --git a/deploy/manifests/gateway.yaml b/deploy/manifests/gateway.yaml index c7618ba877..68294a3524 100644 --- a/deploy/manifests/gateway.yaml +++ b/deploy/manifests/gateway.yaml @@ -8,7 +8,6 @@ metadata: spec: gatewayClassName: nginx listeners: - - name: my-listener - hostname: example.com - port: 8080 + - name: http + port: 80 protocol: HTTP diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index 329854855f..b879896fb3 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -36,6 +36,7 @@ rules: - gateway.networking.k8s.io resources: - httproutes/status + - gateways/status verbs: - update --- diff --git a/examples/advanced-routing/cafe-routes.yaml b/examples/advanced-routing/cafe-routes.yaml index d93a840919..99964a5a7c 100644 --- a/examples/advanced-routing/cafe-routes.yaml +++ b/examples/advanced-routing/cafe-routes.yaml @@ -3,6 +3,10 @@ kind: HTTPRoute metadata: name: cafe spec: + parentRefs: + - name: gateway + namespace: nginx-gateway + sectionName: http hostnames: - "cafe.example.com" rules: @@ -15,6 +19,10 @@ kind: HTTPRoute metadata: name: coffee spec: + parentRefs: + - name: gateway + namespace: nginx-gateway + sectionName: http hostnames: - "cafe.example.com" rules: @@ -37,6 +45,10 @@ kind: HTTPRoute metadata: name: tea spec: + parentRefs: + - name: gateway + namespace: nginx-gateway + sectionName: http hostnames: - "cafe.example.com" rules: diff --git a/examples/cafe-example/cafe-routes.yaml b/examples/cafe-example/cafe-routes.yaml index 56b5f80f47..a3566d7e20 100644 --- a/examples/cafe-example/cafe-routes.yaml +++ b/examples/cafe-example/cafe-routes.yaml @@ -3,6 +3,10 @@ kind: HTTPRoute metadata: name: cafe spec: + parentRefs: + - name: gateway + namespace: nginx-gateway + sectionName: http hostnames: - "cafe.example.com" rules: @@ -15,6 +19,10 @@ kind: HTTPRoute metadata: name: coffee spec: + parentRefs: + - name: gateway + namespace: nginx-gateway + sectionName: http hostnames: - "cafe.example.com" rules: @@ -31,6 +39,10 @@ kind: HTTPRoute metadata: name: tea spec: + parentRefs: + - name: gateway + namespace: nginx-gateway + sectionName: http hostnames: - "cafe.example.com" rules: diff --git a/internal/config/config.go b/internal/config/config.go index 8b1e89a8ef..e23d93c16c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -2,9 +2,13 @@ package config import ( "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/types" ) type Config struct { GatewayCtlrName string Logger logr.Logger + // GatewayNsName is the namespaced name of a Gateway resource that the Gateway will use. + // The Gateway will ignore all other Gateway resources. + GatewayNsName types.NamespacedName } diff --git a/internal/events/loop.go b/internal/events/loop.go index 80ac2717c8..06028a2d85 100644 --- a/internal/events/loop.go +++ b/internal/events/loop.go @@ -17,36 +17,36 @@ import ( // EventLoop is the main event loop of the Gateway. type EventLoop struct { - conf state.Configuration + processor state.ChangeProcessor serviceStore state.ServiceStore generator config.Generator eventCh <-chan interface{} logger logr.Logger - statusUpdater status.Updater nginxFileMgr file.Manager nginxRuntimeMgr runtime.Manager + statusUpdater status.Updater } // NewEventLoop creates a new EventLoop. func NewEventLoop( - conf state.Configuration, + processor state.ChangeProcessor, serviceStore state.ServiceStore, generator config.Generator, eventCh <-chan interface{}, - statusUpdater status.Updater, logger logr.Logger, nginxFileMgr file.Manager, nginxRuntimeMgr runtime.Manager, + statusUpdater status.Updater, ) *EventLoop { return &EventLoop{ - conf: conf, + processor: processor, serviceStore: serviceStore, generator: generator, eventCh: eventCh, - statusUpdater: statusUpdater, logger: logger.WithName("eventLoop"), nginxFileMgr: nginxFileMgr, nginxRuntimeMgr: nginxRuntimeMgr, + statusUpdater: statusUpdater, } } @@ -58,112 +58,88 @@ func (el *EventLoop) Start(ctx context.Context) error { for { select { case <-ctx.Done(): + // although we always return nil, Start must return it to satisfy + // "sigs.k8s.io/controller-runtime/pkg/manager".Runnable return nil case e := <-el.eventCh: - err := el.handleEvent(ctx, e) - if err != nil { - return err - } + el.handleEvent(ctx, e) } } } // FIXME(pleshakov): think about how to avoid using an interface{} here -func (el *EventLoop) handleEvent(ctx context.Context, event interface{}) error { - var changes []state.Change - var updates []state.StatusUpdate - var err error - +func (el *EventLoop) handleEvent(ctx context.Context, event interface{}) { switch e := event.(type) { case *UpsertEvent: - changes, updates, err = el.propagateUpsert(e) + el.propagateUpsert(e) case *DeleteEvent: - changes, updates, err = el.propagateDelete(e) + el.propagateDelete(e) default: - // FIXME(pleshakov): panic because it is a coding error - return fmt.Errorf("unknown event type %T", e) + panic(fmt.Errorf("unknown event type %T", e)) + } + + changed, conf, statuses := el.processor.Process() + if !changed { + return } + err := el.updateNginx(ctx, conf) + if err != nil { + el.logger.Error(err, "Failed to update NGINX configuration") + } + + el.statusUpdater.Update(ctx, statuses) +} + +func (el *EventLoop) updateNginx(ctx context.Context, conf state.Configuration) error { + cfg, warnings := el.generator.Generate(conf) + + // For now, we keep all http servers in one config + // We might rethink that. For example, we can write each server to its file + // or group servers in some way. + err := el.nginxFileMgr.WriteHTTPServersConfig("http-servers", cfg) if err != nil { return err } - el.processChangesAndStatusUpdates(ctx, changes, updates) - return nil + for obj, objWarnings := range warnings { + for _, w := range objWarnings { + // FIXME(pleshakov): report warnings via Object status + el.logger.Info("got warning while generating config", + "kind", obj.GetObjectKind().GroupVersionKind().Kind, + "namespace", obj.GetNamespace(), + "name", obj.GetName(), + "warning", w) + } + } + + return el.nginxRuntimeMgr.Reload(ctx) } -func (el *EventLoop) propagateUpsert(e *UpsertEvent) ([]state.Change, []state.StatusUpdate, error) { +func (el *EventLoop) propagateUpsert(e *UpsertEvent) { switch r := e.Resource.(type) { + case *v1alpha2.Gateway: + el.processor.CaptureUpsertChange(r) case *v1alpha2.HTTPRoute: - changes, statusUpdates := el.conf.UpsertHTTPRoute(r) - return changes, statusUpdates, nil + el.processor.CaptureUpsertChange(r) case *apiv1.Service: - el.serviceStore.Upsert(r) // FIXME(pleshakov): make sure the affected hosts are updated - return nil, nil, nil + el.serviceStore.Upsert(r) + default: + panic(fmt.Errorf("unknown resource type %T", e.Resource)) } - - // FIXME(pleshakov): panic because it is a coding error - return nil, nil, fmt.Errorf("unknown resource type %T", e.Resource) } -func (el *EventLoop) propagateDelete(e *DeleteEvent) ([]state.Change, []state.StatusUpdate, error) { +func (el *EventLoop) propagateDelete(e *DeleteEvent) { switch e.Type.(type) { + case *v1alpha2.Gateway: + el.processor.CaptureDeleteChange(e.Type, e.NamespacedName) case *v1alpha2.HTTPRoute: - changes, statusUpdates := el.conf.DeleteHTTPRoute(e.NamespacedName) - return changes, statusUpdates, nil + el.processor.CaptureDeleteChange(e.Type, e.NamespacedName) case *apiv1.Service: - el.serviceStore.Delete(e.NamespacedName) // FIXME(pleshakov): make sure the affected hosts are updated - return nil, nil, nil - } - - // FIXME(pleshakov): panic because it is a coding error - return nil, nil, fmt.Errorf("unknown resource type %T", e.Type) -} - -func (el *EventLoop) processChangesAndStatusUpdates(ctx context.Context, changes []state.Change, updates []state.StatusUpdate) { - for _, c := range changes { - el.logger.Info("Processing a change", - "host", c.Host.Value) - - if c.Op == state.Upsert { - cfg, warnings := el.generator.GenerateForHost(c.Host) - - for obj, objWarnings := range warnings { - for _, w := range objWarnings { - // FIXME(pleshakov): report warnings via Object status - el.logger.Info("got warning while generating config", - "kind", obj.GetObjectKind().GroupVersionKind().Kind, - "namespace", obj.GetNamespace(), - "name", obj.GetName(), - "warning", w) - } - } - - el.logger.Info("Writing configuration", - "host", c.Host.Value) - - err := el.nginxFileMgr.WriteServerConfig(c.Host.Value, cfg) - if err != nil { - el.logger.Error(err, "Failed to write configuration", - "host", c.Host.Value) - } - } else { - err := el.nginxFileMgr.DeleteServerConfig(c.Host.Value) - if err != nil { - el.logger.Error(err, "Failed to delete configuration", - "host", c.Host.Value) - } - } - } - - if len(changes) > 0 { - err := el.nginxRuntimeMgr.Reload(ctx) - if err != nil { - el.logger.Error(err, "Failed to reload NGINX") - } + el.serviceStore.Delete(e.NamespacedName) + default: + panic(fmt.Errorf("unknown resource type %T", e.Type)) } - - el.statusUpdater.ProcessStatusUpdates(ctx, updates) } diff --git a/internal/events/loop_test.go b/internal/events/loop_test.go index f2f7545960..7758717a00 100644 --- a/internal/events/loop_test.go +++ b/internal/events/loop_test.go @@ -36,38 +36,42 @@ func (r *unsupportedResource) DeepCopyObject() runtime.Object { } var _ = Describe("EventLoop", func() { - var ctrl *events.EventLoop - var fakeConf *statefakes.FakeConfiguration - var fakeUpdater *statusfakes.FakeUpdater - var fakeServiceStore *statefakes.FakeServiceStore - var fakeGenerator *configfakes.FakeGenerator - var fakeNginxFimeMgr *filefakes.FakeManager - var fakeNginxRuntimeMgr *runtimefakes.FakeManager - var cancel context.CancelFunc - var eventCh chan interface{} - var errorCh chan error + var ( + fakeProcessor *statefakes.FakeChangeProcessor + fakeServiceStore *statefakes.FakeServiceStore + fakeGenerator *configfakes.FakeGenerator + fakeNginxFimeMgr *filefakes.FakeManager + fakeNginxRuntimeMgr *runtimefakes.FakeManager + fakeStatusUpdater *statusfakes.FakeUpdater + cancel context.CancelFunc + eventCh chan interface{} + errorCh chan error + start func() + ) BeforeEach(func() { - fakeConf = &statefakes.FakeConfiguration{} + fakeProcessor = &statefakes.FakeChangeProcessor{} eventCh = make(chan interface{}) - fakeUpdater = &statusfakes.FakeUpdater{} fakeServiceStore = &statefakes.FakeServiceStore{} fakeGenerator = &configfakes.FakeGenerator{} fakeNginxFimeMgr = &filefakes.FakeManager{} fakeNginxRuntimeMgr = &runtimefakes.FakeManager{} - ctrl = events.NewEventLoop(fakeConf, fakeServiceStore, fakeGenerator, eventCh, fakeUpdater, zap.New(), fakeNginxFimeMgr, fakeNginxRuntimeMgr) + fakeStatusUpdater = &statusfakes.FakeUpdater{} + ctrl := events.NewEventLoop(fakeProcessor, fakeServiceStore, fakeGenerator, eventCh, zap.New(), fakeNginxFimeMgr, fakeNginxRuntimeMgr, fakeStatusUpdater) var ctx context.Context - ctx, cancel = context.WithCancel(context.Background()) errorCh = make(chan error) - - go func() { + start = func() { errorCh <- ctrl.Start(ctx) - }() + } }) - Describe("Process HTTPRoute events", func() { + Describe("Process Gateway API resource events", func() { + BeforeEach(func() { + go start() + }) + AfterEach(func() { cancel() @@ -76,97 +80,75 @@ var _ = Describe("EventLoop", func() { Expect(err).To(BeNil()) }) - It("should process upsert event", func() { - fakeChanges := []state.Change{ - { - Op: state.Upsert, - Host: state.Host{ - Value: "example.com", - }, - }, - } - fakeStatusUpdates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{}, - Status: nil, - }, - } - fakeConf.UpsertHTTPRouteReturns(fakeChanges, fakeStatusUpdates) + DescribeTable("Upsert events", + func(e *events.UpsertEvent) { + fakeConf := state.Configuration{} + changed := true + fakeStatuses := state.Statuses{} + fakeProcessor.ProcessReturns(changed, fakeConf, fakeStatuses) - fakeCfg := []byte("fake") - fakeGenerator.GenerateForHostReturns(fakeCfg, config.Warnings{}) + fakeCfg := []byte("fake") + fakeGenerator.GenerateReturns(fakeCfg, config.Warnings{}) - hr := &v1alpha2.HTTPRoute{} - - eventCh <- &events.UpsertEvent{ - Resource: hr, - } + eventCh <- e - Eventually(fakeConf.UpsertHTTPRouteCallCount).Should(Equal(1)) - Eventually(func() *v1alpha2.HTTPRoute { - return fakeConf.UpsertHTTPRouteArgsForCall(0) - }).Should(Equal(hr)) + Eventually(fakeProcessor.CaptureUpsertChangeCallCount).Should(Equal(1)) + Expect(fakeProcessor.CaptureUpsertChangeArgsForCall(0)).Should(Equal(e.Resource)) + Eventually(fakeProcessor.ProcessCallCount).Should(Equal(1)) - Eventually(fakeUpdater.ProcessStatusUpdatesCallCount).Should(Equal(1)) - Eventually(func() []state.StatusUpdate { - _, updates := fakeUpdater.ProcessStatusUpdatesArgsForCall(0) - return updates - }).Should(Equal(fakeStatusUpdates)) + Eventually(fakeGenerator.GenerateCallCount).Should(Equal(1)) + Expect(fakeGenerator.GenerateArgsForCall(0)).Should(Equal(fakeConf)) - Eventually(fakeGenerator.GenerateForHostCallCount).Should(Equal(1)) - Expect(fakeGenerator.GenerateForHostArgsForCall(0)).Should(Equal(fakeChanges[0].Host)) + Eventually(fakeNginxFimeMgr.WriteHTTPServersConfigCallCount).Should(Equal(1)) + name, cfg := fakeNginxFimeMgr.WriteHTTPServersConfigArgsForCall(0) + Expect(name).Should(Equal("http-servers")) + Expect(cfg).Should(Equal(fakeCfg)) - Eventually(fakeNginxFimeMgr.WriteServerConfigCallCount).Should(Equal(1)) - host, cfg := fakeNginxFimeMgr.WriteServerConfigArgsForCall(0) - Expect(host).Should(Equal("example.com")) - Expect(cfg).Should(Equal(fakeCfg)) + Eventually(fakeNginxRuntimeMgr.ReloadCallCount).Should(Equal(1)) - Eventually(fakeNginxRuntimeMgr.ReloadCallCount).Should(Equal(1)) - }) + Eventually(fakeStatusUpdater.UpdateCallCount).Should(Equal(1)) + _, statuses := fakeStatusUpdater.UpdateArgsForCall(0) + Expect(statuses).Should(Equal(fakeStatuses)) + }, + Entry("HTTPRoute", &events.UpsertEvent{Resource: &v1alpha2.HTTPRoute{}}), + Entry("Gateway", &events.UpsertEvent{Resource: &v1alpha2.Gateway{}}), + ) - It("should process delete event", func() { - fakeChanges := []state.Change{ - { - Op: state.Delete, - Host: state.Host{ - Value: "example.com", - }, - }, - } - fakeStatusUpdates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{}, - Status: nil, - }, - } - fakeConf.DeleteHTTPRouteReturns(fakeChanges, fakeStatusUpdates) + DescribeTable("Delete events", + func(e *events.DeleteEvent) { + fakeConf := state.Configuration{} + changed := true + fakeProcessor.ProcessReturns(changed, fakeConf, state.Statuses{}) - nsname := types.NamespacedName{Namespace: "test", Name: "route"} + fakeCfg := []byte("fake") + fakeGenerator.GenerateReturns(fakeCfg, config.Warnings{}) - eventCh <- &events.DeleteEvent{ - NamespacedName: nsname, - Type: &v1alpha2.HTTPRoute{}, - } + eventCh <- e - Eventually(fakeConf.DeleteHTTPRouteCallCount).Should(Equal(1)) - Eventually(func() types.NamespacedName { - return fakeConf.DeleteHTTPRouteArgsForCall(0) - }).Should(Equal(nsname)) + Eventually(fakeProcessor.CaptureDeleteChangeCallCount).Should(Equal(1)) + passedObj, passedNsName := fakeProcessor.CaptureDeleteChangeArgsForCall(0) + Expect(passedObj).Should(Equal(e.Type)) + Expect(passedNsName).Should(Equal(e.NamespacedName)) - Eventually(fakeUpdater.ProcessStatusUpdatesCallCount).Should(Equal(1)) - Eventually(func() []state.StatusUpdate { - _, updates := fakeUpdater.ProcessStatusUpdatesArgsForCall(0) - return updates - }).Should(Equal(fakeStatusUpdates)) + Eventually(fakeProcessor.ProcessCallCount).Should(Equal(1)) - Eventually(fakeNginxFimeMgr.DeleteServerConfigCallCount).Should(Equal(1)) - Expect(fakeNginxFimeMgr.DeleteServerConfigArgsForCall(0)).Should(Equal("example.com")) + Eventually(fakeNginxFimeMgr.WriteHTTPServersConfigCallCount).Should(Equal(1)) + name, cfg := fakeNginxFimeMgr.WriteHTTPServersConfigArgsForCall(0) + Expect(name).Should(Equal("http-servers")) + Expect(cfg).Should(Equal(fakeCfg)) - Eventually(fakeNginxRuntimeMgr.ReloadCallCount).Should(Equal(1)) - }) + Eventually(fakeNginxRuntimeMgr.ReloadCallCount).Should(Equal(1)) + }, + Entry("HTTPRoute", &events.DeleteEvent{Type: &v1alpha2.HTTPRoute{}, NamespacedName: types.NamespacedName{Namespace: "test", Name: "route"}}), + Entry("Gateway", &events.DeleteEvent{Type: &v1alpha2.Gateway{}, NamespacedName: types.NamespacedName{Namespace: "test", Name: "gateway"}}), + ) }) Describe("Process Service events", func() { + BeforeEach(func() { + go start() + }) + AfterEach(func() { cancel() @@ -183,9 +165,9 @@ var _ = Describe("EventLoop", func() { } Eventually(fakeServiceStore.UpsertCallCount).Should(Equal(1)) - Eventually(func() *apiv1.Service { - return fakeServiceStore.UpsertArgsForCall(0) - }).Should(Equal(svc)) + Expect(fakeServiceStore.UpsertArgsForCall(0)).Should(Equal(svc)) + + Eventually(fakeProcessor.ProcessCallCount).Should(Equal(1)) }) It("should process delete event", func() { @@ -197,61 +179,9 @@ var _ = Describe("EventLoop", func() { } Eventually(fakeServiceStore.DeleteCallCount).Should(Equal(1)) - Eventually(func() types.NamespacedName { - return fakeServiceStore.DeleteArgsForCall(0) - }).Should(Equal(nsname)) - }) - }) - - Describe("Processing events common cases", func() { - AfterEach(func() { - cancel() - - var err error - Eventually(errorCh).Should(Receive(&err)) - Expect(err).To(BeNil()) - }) + Expect(fakeServiceStore.DeleteArgsForCall(0)).Should(Equal(nsname)) - It("should reload once in case of multiple changes", func() { - fakeChanges := []state.Change{ - { - Op: state.Delete, - Host: state.Host{ - Value: "one.example.com", - }, - }, - { - Op: state.Upsert, - Host: state.Host{ - Value: "two.example.com", - }, - }, - } - fakeConf.DeleteHTTPRouteReturns(fakeChanges, nil) - - fakeCfg := []byte("fake") - fakeGenerator.GenerateForHostReturns(fakeCfg, config.Warnings{}) - - nsname := types.NamespacedName{Namespace: "test", Name: "route"} - - // the exact event doesn't matter. what matters is the generated changes return by DeleteHTTPRouteReturns - eventCh <- &events.DeleteEvent{ - NamespacedName: nsname, - Type: &v1alpha2.HTTPRoute{}, - } - - Eventually(fakeConf.DeleteHTTPRouteCallCount).Should(Equal(1)) - Expect(fakeConf.DeleteHTTPRouteArgsForCall(0)).Should(Equal(nsname)) - - Eventually(fakeNginxFimeMgr.WriteServerConfigCallCount).Should(Equal(1)) - host, cfg := fakeNginxFimeMgr.WriteServerConfigArgsForCall(0) - Expect(host).Should(Equal("two.example.com")) - Expect(cfg).Should(Equal(fakeCfg)) - - Eventually(fakeNginxFimeMgr.DeleteServerConfigCallCount).Should(Equal(1)) - Expect(fakeNginxFimeMgr.DeleteServerConfigArgsForCall(0)).Should(Equal("one.example.com")) - - Eventually(fakeNginxRuntimeMgr.ReloadCallCount).Should(Equal(1)) + Eventually(fakeProcessor.ProcessCallCount).Should(Equal(1)) }) }) @@ -262,19 +192,19 @@ var _ = Describe("EventLoop", func() { DescribeTable("Edge cases for events", func(e interface{}) { - eventCh <- e + go func() { + eventCh <- e + }() - var err error - Eventually(errorCh).Should(Receive(&err)) - Expect(err).Should(HaveOccurred()) + Expect(start).Should(Panic()) }, - Entry("should return error for an unknown event type", + Entry("should panic for an unknown event type", &struct{}{}), - Entry("should return error for an unknown type of resource in upsert event", + Entry("should panic for an unknown type of resource in upsert event", &events.UpsertEvent{ Resource: &unsupportedResource{}, }), - Entry("should return error for an unknown type of resource in delete event", + Entry("should panic for an unknown type of resource in delete event", &events.DeleteEvent{ Type: &unsupportedResource{}, }), diff --git a/internal/implementations/gateway/gateway.go b/internal/implementations/gateway/gateway.go index 9946e5e070..6d8d5e40ee 100644 --- a/internal/implementations/gateway/gateway.go +++ b/internal/implementations/gateway/gateway.go @@ -1,20 +1,26 @@ package implementation import ( + "fmt" + "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/gateway-api/apis/v1alpha2" "github.com/nginxinc/nginx-kubernetes-gateway/internal/config" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/events" "github.com/nginxinc/nginx-kubernetes-gateway/pkg/sdk" ) type gatewayImplementation struct { - conf config.Config + conf config.Config + eventCh chan<- interface{} } -func NewGatewayImplementation(conf config.Config) sdk.GatewayImpl { +func NewGatewayImplementation(conf config.Config, eventCh chan<- interface{}) sdk.GatewayImpl { return &gatewayImplementation{ - conf: conf, + conf: conf, + eventCh: eventCh, } } @@ -22,21 +28,43 @@ func (impl *gatewayImplementation) Logger() logr.Logger { return impl.conf.Logger } -func (impl *gatewayImplementation) ControllerName() string { - return impl.conf.GatewayCtlrName -} - func (impl *gatewayImplementation) Upsert(gw *v1alpha2.Gateway) { - if gw.Name == impl.ControllerName() { - impl.Logger().Info("Found correct Gateway resource", + if gw.Namespace != impl.conf.GatewayNsName.Namespace || gw.Name != impl.conf.GatewayNsName.Name { + msg := fmt.Sprintf("Gateway was upserted but ignored because this controller only supports the Gateway %s", impl.conf.GatewayNsName) + impl.Logger().Info(msg, + "namespace", gw.Namespace, "name", gw.Name, ) return } + + impl.Logger().Info("Gateway was upserted", + "namespace", gw.Namespace, + "name", gw.Name, + ) + + impl.eventCh <- &events.UpsertEvent{ + Resource: gw, + } } -func (impl *gatewayImplementation) Remove(key string) { - impl.Logger().Info("Gateway resource was removed", - "name", key, +func (impl *gatewayImplementation) Remove(nsname types.NamespacedName) { + if nsname != impl.conf.GatewayNsName { + msg := fmt.Sprintf("Gateway was removed but ignored because this controller only supports the Gateway %s", impl.conf.GatewayNsName) + impl.Logger().Info(msg, + "namespace", nsname.Namespace, + "name", nsname.Name, + ) + return + } + + impl.Logger().Info("Gateway was removed", + "namespace", nsname.Namespace, + "name", nsname.Name, ) + + impl.eventCh <- &events.DeleteEvent{ + NamespacedName: nsname, + Type: &v1alpha2.Gateway{}, + } } diff --git a/internal/manager/manager.go b/internal/manager/manager.go index 1347117864..469155fa2e 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -12,6 +12,7 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/config" "github.com/nginxinc/nginx-kubernetes-gateway/internal/events" + gw "github.com/nginxinc/nginx-kubernetes-gateway/internal/implementations/gateway" hr "github.com/nginxinc/nginx-kubernetes-gateway/internal/implementations/httproute" svc "github.com/nginxinc/nginx-kubernetes-gateway/internal/implementations/service" ngxcfg "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/config" @@ -50,6 +51,10 @@ func Start(cfg config.Config) error { return fmt.Errorf("cannot build runtime manager: %w", err) } + err = sdk.RegisterGatewayController(mgr, gw.NewGatewayImplementation(cfg, eventCh)) + if err != nil { + return fmt.Errorf("cannot register gateway implementation: %w", err) + } err = sdk.RegisterHTTPRouteController(mgr, hr.NewHTTPRouteImplementation(cfg, eventCh)) if err != nil { return fmt.Errorf("cannot register httproute implementation: %w", err) @@ -59,13 +64,13 @@ func Start(cfg config.Config) error { return fmt.Errorf("cannot register service implementation: %w", err) } - conf := state.NewConfiguration(cfg.GatewayCtlrName, state.NewRealClock()) + processor := state.NewChangeProcessorImpl(cfg.GatewayNsName) serviceStore := state.NewServiceStore() - reporter := status.NewUpdater(mgr.GetClient(), cfg.Logger) configGenerator := ngxcfg.NewGeneratorImpl(serviceStore) nginxFileMgr := file.NewManagerImpl() nginxRuntimeMgr := ngxruntime.NewManagerImpl() - eventLoop := events.NewEventLoop(conf, serviceStore, configGenerator, eventCh, reporter, cfg.Logger, nginxFileMgr, nginxRuntimeMgr) + statusUpdater := status.NewUpdater(cfg.GatewayCtlrName, cfg.GatewayNsName, mgr.GetClient(), cfg.Logger, status.NewRealClock()) + eventLoop := events.NewEventLoop(processor, serviceStore, configGenerator, eventCh, cfg.Logger, nginxFileMgr, nginxRuntimeMgr, statusUpdater) err = mgr.Add(eventLoop) if err != nil { diff --git a/internal/nginx/config/configfakes/fake_generator.go b/internal/nginx/config/configfakes/fake_generator.go index c02af23d70..6154d27bb3 100644 --- a/internal/nginx/config/configfakes/fake_generator.go +++ b/internal/nginx/config/configfakes/fake_generator.go @@ -9,16 +9,16 @@ import ( ) type FakeGenerator struct { - GenerateForHostStub func(state.Host) ([]byte, config.Warnings) - generateForHostMutex sync.RWMutex - generateForHostArgsForCall []struct { - arg1 state.Host + GenerateStub func(state.Configuration) ([]byte, config.Warnings) + generateMutex sync.RWMutex + generateArgsForCall []struct { + arg1 state.Configuration } - generateForHostReturns struct { + generateReturns struct { result1 []byte result2 config.Warnings } - generateForHostReturnsOnCall map[int]struct { + generateReturnsOnCall map[int]struct { result1 []byte result2 config.Warnings } @@ -26,16 +26,16 @@ type FakeGenerator struct { invocationsMutex sync.RWMutex } -func (fake *FakeGenerator) GenerateForHost(arg1 state.Host) ([]byte, config.Warnings) { - fake.generateForHostMutex.Lock() - ret, specificReturn := fake.generateForHostReturnsOnCall[len(fake.generateForHostArgsForCall)] - fake.generateForHostArgsForCall = append(fake.generateForHostArgsForCall, struct { - arg1 state.Host +func (fake *FakeGenerator) Generate(arg1 state.Configuration) ([]byte, config.Warnings) { + fake.generateMutex.Lock() + ret, specificReturn := fake.generateReturnsOnCall[len(fake.generateArgsForCall)] + fake.generateArgsForCall = append(fake.generateArgsForCall, struct { + arg1 state.Configuration }{arg1}) - stub := fake.GenerateForHostStub - fakeReturns := fake.generateForHostReturns - fake.recordInvocation("GenerateForHost", []interface{}{arg1}) - fake.generateForHostMutex.Unlock() + stub := fake.GenerateStub + fakeReturns := fake.generateReturns + fake.recordInvocation("Generate", []interface{}{arg1}) + fake.generateMutex.Unlock() if stub != nil { return stub(arg1) } @@ -45,46 +45,46 @@ func (fake *FakeGenerator) GenerateForHost(arg1 state.Host) ([]byte, config.Warn return fakeReturns.result1, fakeReturns.result2 } -func (fake *FakeGenerator) GenerateForHostCallCount() int { - fake.generateForHostMutex.RLock() - defer fake.generateForHostMutex.RUnlock() - return len(fake.generateForHostArgsForCall) +func (fake *FakeGenerator) GenerateCallCount() int { + fake.generateMutex.RLock() + defer fake.generateMutex.RUnlock() + return len(fake.generateArgsForCall) } -func (fake *FakeGenerator) GenerateForHostCalls(stub func(state.Host) ([]byte, config.Warnings)) { - fake.generateForHostMutex.Lock() - defer fake.generateForHostMutex.Unlock() - fake.GenerateForHostStub = stub +func (fake *FakeGenerator) GenerateCalls(stub func(state.Configuration) ([]byte, config.Warnings)) { + fake.generateMutex.Lock() + defer fake.generateMutex.Unlock() + fake.GenerateStub = stub } -func (fake *FakeGenerator) GenerateForHostArgsForCall(i int) state.Host { - fake.generateForHostMutex.RLock() - defer fake.generateForHostMutex.RUnlock() - argsForCall := fake.generateForHostArgsForCall[i] +func (fake *FakeGenerator) GenerateArgsForCall(i int) state.Configuration { + fake.generateMutex.RLock() + defer fake.generateMutex.RUnlock() + argsForCall := fake.generateArgsForCall[i] return argsForCall.arg1 } -func (fake *FakeGenerator) GenerateForHostReturns(result1 []byte, result2 config.Warnings) { - fake.generateForHostMutex.Lock() - defer fake.generateForHostMutex.Unlock() - fake.GenerateForHostStub = nil - fake.generateForHostReturns = struct { +func (fake *FakeGenerator) GenerateReturns(result1 []byte, result2 config.Warnings) { + fake.generateMutex.Lock() + defer fake.generateMutex.Unlock() + fake.GenerateStub = nil + fake.generateReturns = struct { result1 []byte result2 config.Warnings }{result1, result2} } -func (fake *FakeGenerator) GenerateForHostReturnsOnCall(i int, result1 []byte, result2 config.Warnings) { - fake.generateForHostMutex.Lock() - defer fake.generateForHostMutex.Unlock() - fake.GenerateForHostStub = nil - if fake.generateForHostReturnsOnCall == nil { - fake.generateForHostReturnsOnCall = make(map[int]struct { +func (fake *FakeGenerator) GenerateReturnsOnCall(i int, result1 []byte, result2 config.Warnings) { + fake.generateMutex.Lock() + defer fake.generateMutex.Unlock() + fake.GenerateStub = nil + if fake.generateReturnsOnCall == nil { + fake.generateReturnsOnCall = make(map[int]struct { result1 []byte result2 config.Warnings }) } - fake.generateForHostReturnsOnCall[i] = struct { + fake.generateReturnsOnCall[i] = struct { result1 []byte result2 config.Warnings }{result1, result2} @@ -93,8 +93,8 @@ func (fake *FakeGenerator) GenerateForHostReturnsOnCall(i int, result1 []byte, r func (fake *FakeGenerator) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() - fake.generateForHostMutex.RLock() - defer fake.generateForHostMutex.RUnlock() + fake.generateMutex.RLock() + defer fake.generateMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} for key, value := range fake.invocations { copiedInvocations[key] = value diff --git a/internal/nginx/config/generator.go b/internal/nginx/config/generator.go index 91c7778c64..5293d9f30a 100644 --- a/internal/nginx/config/generator.go +++ b/internal/nginx/config/generator.go @@ -18,8 +18,8 @@ const nginx502Server = "unix:/var/lib/nginx/nginx-502-server.sock" // Generator generates NGINX configuration. type Generator interface { - // GenerateForHost generates configuration for a host. - GenerateForHost(host state.Host) ([]byte, Warnings) + // Generate generates NGINX configuration from internal representation. + Generate(configuration state.Configuration) ([]byte, Warnings) } // GeneratorImpl is an implementation of Generator @@ -36,20 +36,32 @@ func NewGeneratorImpl(serviceStore state.ServiceStore) *GeneratorImpl { } } -func (g *GeneratorImpl) GenerateForHost(host state.Host) ([]byte, Warnings) { - server, warnings := generate(host, g.serviceStore) - return g.executor.ExecuteForServer(server), warnings +func (g *GeneratorImpl) Generate(conf state.Configuration) ([]byte, Warnings) { + warnings := newWarnings() + + servers := httpServers{ + Servers: make([]server, 0, len(conf.HTTPServers)), + } + + for _, s := range conf.HTTPServers { + cfg, warns := generate(s, g.serviceStore) + + servers.Servers = append(servers.Servers, cfg) + warnings.Add(warns) + } + + return g.executor.ExecuteForHTTPServers(servers), warnings } -func generate(host state.Host, serviceStore state.ServiceStore) (server, Warnings) { +func generate(httpServer state.HTTPServer, serviceStore state.ServiceStore) (server, Warnings) { warnings := newWarnings() - locs := make([]location, 0, len(host.PathRouteGroups)) // FIXME(pleshakov): expand with g.Routes + locs := make([]location, 0, len(httpServer.PathRules)) // FIXME(pleshakov): expand with rules.Routes - for _, g := range host.PathRouteGroups { + for _, rules := range httpServer.PathRules { // number of routes in a group is always at least 1 // otherwise, it is a bug in the state.Configuration code, so it is OK to panic here - r := g.Routes[0] // FIXME(pleshakov): for now, we only handle the first route in case there are multiple routes + r := rules.MatchRules[0] // FIXME(pleshakov): for now, we only handle the first route in case there are multiple routes address, err := getBackendAddress(r.Source.Spec.Rules[r.RuleIdx].BackendRefs, r.Source.Namespace, serviceStore) if err != nil { warnings.AddWarning(r.Source, err.Error()) @@ -59,7 +71,7 @@ func generate(host state.Host, serviceStore state.ServiceStore) (server, Warning if exists && matchLocationNeeded(match) { // FIXME(kate-osborn): route index is hardcoded to 0 for now. // Once we support multiple routes we will need to change this to the index of the current route. - path := createPathForMatch(g.Path, 0) + path := createPathForMatch(rules.Path, 0) // generate location block for this rule and match mLoc := generateMatchLocation(path, address) // generate the http_matches variable value @@ -70,13 +82,13 @@ func generate(host state.Host, serviceStore state.ServiceStore) (server, Warning } loc := location{ - Path: g.Path, + Path: rules.Path, HTTPMatchVar: string(b), } locs = append(locs, loc, mLoc) } else { loc := location{ - Path: g.Path, + Path: rules.Path, ProxyPass: generateProxyPass(address), } locs = append(locs, loc) @@ -84,7 +96,7 @@ func generate(host state.Host, serviceStore state.ServiceStore) (server, Warning } return server{ - ServerName: host.Value, + ServerName: httpServer.Hostname, Locations: locs, }, warnings } @@ -168,7 +180,7 @@ func createHTTPMatch(match v1alpha2.HTTPRouteMatch, redirectPath string) httpMat if match.Headers != nil { headers := make([]string, 0, len(match.Headers)) - //FIXME(kate-osborn): For now we only support type "Exact". + // FIXME(kate-osborn): For now we only support type "Exact". for _, h := range match.Headers { if *h.Type == v1alpha2.HeaderMatchExact { headers = append(headers, createHeaderKeyValString(h)) @@ -180,7 +192,7 @@ func createHTTPMatch(match v1alpha2.HTTPRouteMatch, redirectPath string) httpMat if match.QueryParams != nil { params := make([]string, 0, len(match.QueryParams)) - //FIXME(kate-osborn): For now we only support type "Exact". + // FIXME(kate-osborn): For now we only support type "Exact". for _, p := range match.QueryParams { if *p.Type == v1alpha2.QueryParamMatchExact { params = append(params, createQueryParamKeyValString(p)) diff --git a/internal/nginx/config/generator_test.go b/internal/nginx/config/generator_test.go index d21f0f1ca9..2faa1d7de3 100644 --- a/internal/nginx/config/generator_test.go +++ b/internal/nginx/config/generator_test.go @@ -18,15 +18,21 @@ import ( func TestGenerateForHost(t *testing.T) { generator := NewGeneratorImpl(&statefakes.FakeServiceStore{}) - host := state.Host{Value: "example.com"} + conf := state.Configuration{ + HTTPServers: []state.HTTPServer{ + { + Hostname: "example.com", + }, + }, + } - cfg, warnings := generator.GenerateForHost(host) + cfg, warnings := generator.Generate(conf) if len(cfg) == 0 { - t.Errorf("GenerateForHost() generated empty config") + t.Errorf("Generate() generated empty config") } if len(warnings) > 0 { - t.Errorf("GenerateForHost() returned unexpected warnings: %v", warnings) + t.Errorf("Generate() returned unexpected warnings: %v", warnings) } } @@ -127,12 +133,12 @@ func TestGenerate(t *testing.T) { }, } - host := state.Host{ - Value: "example.com", - PathRouteGroups: []state.PathRouteGroup{ + host := state.HTTPServer{ + Hostname: "example.com", + PathRules: []state.PathRule{ { Path: "/", - Routes: []state.Route{ + MatchRules: []state.MatchRule{ { MatchIdx: 0, RuleIdx: 0, @@ -142,7 +148,7 @@ func TestGenerate(t *testing.T) { }, { Path: "/test", - Routes: []state.Route{ + MatchRules: []state.MatchRule{ { MatchIdx: 0, RuleIdx: 1, @@ -152,7 +158,7 @@ func TestGenerate(t *testing.T) { }, { Path: "/path-only", - Routes: []state.Route{ + MatchRules: []state.MatchRule{ { MatchIdx: 0, RuleIdx: 2, diff --git a/internal/nginx/config/http.go b/internal/nginx/config/http.go index 8ba8dd76f2..0326e680d9 100644 --- a/internal/nginx/config/http.go +++ b/internal/nginx/config/http.go @@ -1,5 +1,9 @@ package config +type httpServers struct { + Servers []server +} + type server struct { ServerName string Locations []location diff --git a/internal/nginx/config/template.go b/internal/nginx/config/template.go index ae740612db..7e17e81833 100644 --- a/internal/nginx/config/template.go +++ b/internal/nginx/config/template.go @@ -6,10 +6,11 @@ import ( "text/template" ) -var serverTemplate = `server { - server_name {{ .ServerName }}; +var httpServersTemplate = `{{ range $s := .Servers }} +server { + server_name {{ $s.ServerName }}; - {{ range $l := .Locations }} + {{ range $l := $s.Locations }} location {{ $l.Path }} { {{ if $l.Internal }} internal; @@ -28,31 +29,32 @@ var serverTemplate = `server { } {{ end }} } +{{ end }} ` // templateExecutor generates NGINX configuration using a template. // Template parsing or executing errors can only occur if there is a bug in the template, so they are handled with panics. -// For now, we only generate configuration with NGINX server, but in the future we will also need to generate -// the main NGINX configuration file, upstreams. +// For now, we only generate configuration with NGINX http servers, but in the future we will also need to generate +// the main NGINX configuration file, upstreams, stream servers. type templateExecutor struct { - serverTemplate *template.Template + httpServersTemplate *template.Template } func newTemplateExecutor() *templateExecutor { - t, err := template.New("server").Parse(serverTemplate) + t, err := template.New("server").Parse(httpServersTemplate) if err != nil { - panic(fmt.Errorf("failed to parse server template: %w", err)) + panic(fmt.Errorf("failed to parse http servers template: %w", err)) } - return &templateExecutor{serverTemplate: t} + return &templateExecutor{httpServersTemplate: t} } -func (e *templateExecutor) ExecuteForServer(s server) []byte { +func (e *templateExecutor) ExecuteForHTTPServers(servers httpServers) []byte { var buf bytes.Buffer - err := e.serverTemplate.Execute(&buf, s) + err := e.httpServersTemplate.Execute(&buf, servers) if err != nil { - panic(fmt.Errorf("failed to execute server template: %w", err)) + panic(fmt.Errorf("failed to execute http servers template: %w", err)) } return buf.Bytes() diff --git a/internal/nginx/config/template_test.go b/internal/nginx/config/template_test.go index bb03edad9e..2089dae4be 100644 --- a/internal/nginx/config/template_test.go +++ b/internal/nginx/config/template_test.go @@ -8,17 +8,21 @@ import ( func TestExecuteForServer(t *testing.T) { executor := newTemplateExecutor() - server := server{ - ServerName: "example.com", - Locations: []location{ + servers := httpServers{ + Servers: []server{ { - Path: "/", - ProxyPass: "http://10.0.0.1", + ServerName: "example.com", + Locations: []location{ + { + Path: "/", + ProxyPass: "http://10.0.0.1", + }, + }, }, }, } - cfg := executor.ExecuteForServer(server) + cfg := executor.ExecuteForHTTPServers(servers) // we only do a sanity check here. // the config generation logic is tested in the Generator tests. if len(cfg) == 0 { @@ -34,7 +38,7 @@ func TestNewTemplateExecutorPanics(t *testing.T) { } }() - serverTemplate = "{{ end }}" // invalid template + httpServersTemplate = "{{ end }}" // invalid template newTemplateExecutor() } @@ -51,7 +55,7 @@ func TestExecuteForServerPanics(t *testing.T) { t.Fatalf("Failed to parse template: %v", err) } - executor := &templateExecutor{serverTemplate: tmpl} + executor := &templateExecutor{httpServersTemplate: tmpl} - _ = executor.ExecuteForServer(server{}) + _ = executor.ExecuteForHTTPServers(httpServers{}) } diff --git a/internal/nginx/config/warnings.go b/internal/nginx/config/warnings.go index a1925cd865..a9d3d404da 100644 --- a/internal/nginx/config/warnings.go +++ b/internal/nginx/config/warnings.go @@ -22,3 +22,11 @@ func (w Warnings) AddWarningf(obj client.Object, msgFmt string, args ...interfac func (w Warnings) AddWarning(obj client.Object, msg string) { w[obj] = append(w[obj], msg) } + +// Add adds new Warnings to the map. +// Warnings for the same object are merged. +func (w Warnings) Add(warnings Warnings) { + for k, v := range warnings { + w[k] = append(w[k], v...) + } +} diff --git a/internal/nginx/config/warnings_test.go b/internal/nginx/config/warnings_test.go index 2d20a61a73..7cacc4fffa 100644 --- a/internal/nginx/config/warnings_test.go +++ b/internal/nginx/config/warnings_test.go @@ -44,3 +44,89 @@ func TestAddWarning(t *testing.T) { t.Errorf("AddWarning mismatch (-want +got):\n%s", diff) } } + +func TestAdd(t *testing.T) { + obj1 := &v1alpha2.HTTPRoute{} + obj2 := &v1alpha2.HTTPRoute{} + obj3 := &v1alpha2.HTTPRoute{} + + tests := []struct { + warnings Warnings + addedWarnings Warnings + expected Warnings + msg string + }{ + { + warnings: newWarnings(), + addedWarnings: newWarnings(), + expected: newWarnings(), + msg: "empty warnings", + }, + { + warnings: Warnings{ + obj1: []string{ + "first", + }, + }, + addedWarnings: newWarnings(), + expected: Warnings{ + obj1: []string{ + "first", + }, + }, + msg: "empty added warnings", + }, + { + warnings: newWarnings(), + addedWarnings: Warnings{ + obj1: []string{ + "first", + }, + }, + expected: Warnings{ + obj1: []string{ + "first", + }, + }, + msg: "empty warnings", + }, + { + warnings: Warnings{ + obj1: []string{ + "first 1", + }, + obj3: []string{ + "first 3", + }, + }, + addedWarnings: Warnings{ + obj2: []string{ + "first 2", + }, + obj3: []string{ + "second 3", + }, + }, + expected: Warnings{ + obj1: []string{ + "first 1", + }, + obj2: []string{ + "first 2", + }, + obj3: []string{ + "first 3", + "second 3", + }, + }, + msg: "adding and merging", + }, + } + + for _, test := range tests { + test.warnings.Add(test.addedWarnings) + if diff := cmp.Diff(test.expected, test.warnings); diff != "" { + t.Errorf("Add() %q mismatch (-want +got):\n%s", test.msg, diff) + } + } +} diff --git a/internal/nginx/file/filefakes/fake_manager.go b/internal/nginx/file/filefakes/fake_manager.go index c65078d846..179abdc995 100644 --- a/internal/nginx/file/filefakes/fake_manager.go +++ b/internal/nginx/file/filefakes/fake_manager.go @@ -8,110 +8,38 @@ import ( ) type FakeManager struct { - DeleteServerConfigStub func(string) error - deleteServerConfigMutex sync.RWMutex - deleteServerConfigArgsForCall []struct { - arg1 string - } - deleteServerConfigReturns struct { - result1 error - } - deleteServerConfigReturnsOnCall map[int]struct { - result1 error - } - WriteServerConfigStub func(string, []byte) error - writeServerConfigMutex sync.RWMutex - writeServerConfigArgsForCall []struct { + WriteHTTPServersConfigStub func(string, []byte) error + writeHTTPServersConfigMutex sync.RWMutex + writeHTTPServersConfigArgsForCall []struct { arg1 string arg2 []byte } - writeServerConfigReturns struct { + writeHTTPServersConfigReturns struct { result1 error } - writeServerConfigReturnsOnCall map[int]struct { + writeHTTPServersConfigReturnsOnCall map[int]struct { result1 error } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } -func (fake *FakeManager) DeleteServerConfig(arg1 string) error { - fake.deleteServerConfigMutex.Lock() - ret, specificReturn := fake.deleteServerConfigReturnsOnCall[len(fake.deleteServerConfigArgsForCall)] - fake.deleteServerConfigArgsForCall = append(fake.deleteServerConfigArgsForCall, struct { - arg1 string - }{arg1}) - stub := fake.DeleteServerConfigStub - fakeReturns := fake.deleteServerConfigReturns - fake.recordInvocation("DeleteServerConfig", []interface{}{arg1}) - fake.deleteServerConfigMutex.Unlock() - if stub != nil { - return stub(arg1) - } - if specificReturn { - return ret.result1 - } - return fakeReturns.result1 -} - -func (fake *FakeManager) DeleteServerConfigCallCount() int { - fake.deleteServerConfigMutex.RLock() - defer fake.deleteServerConfigMutex.RUnlock() - return len(fake.deleteServerConfigArgsForCall) -} - -func (fake *FakeManager) DeleteServerConfigCalls(stub func(string) error) { - fake.deleteServerConfigMutex.Lock() - defer fake.deleteServerConfigMutex.Unlock() - fake.DeleteServerConfigStub = stub -} - -func (fake *FakeManager) DeleteServerConfigArgsForCall(i int) string { - fake.deleteServerConfigMutex.RLock() - defer fake.deleteServerConfigMutex.RUnlock() - argsForCall := fake.deleteServerConfigArgsForCall[i] - return argsForCall.arg1 -} - -func (fake *FakeManager) DeleteServerConfigReturns(result1 error) { - fake.deleteServerConfigMutex.Lock() - defer fake.deleteServerConfigMutex.Unlock() - fake.DeleteServerConfigStub = nil - fake.deleteServerConfigReturns = struct { - result1 error - }{result1} -} - -func (fake *FakeManager) DeleteServerConfigReturnsOnCall(i int, result1 error) { - fake.deleteServerConfigMutex.Lock() - defer fake.deleteServerConfigMutex.Unlock() - fake.DeleteServerConfigStub = nil - if fake.deleteServerConfigReturnsOnCall == nil { - fake.deleteServerConfigReturnsOnCall = make(map[int]struct { - result1 error - }) - } - fake.deleteServerConfigReturnsOnCall[i] = struct { - result1 error - }{result1} -} - -func (fake *FakeManager) WriteServerConfig(arg1 string, arg2 []byte) error { +func (fake *FakeManager) WriteHTTPServersConfig(arg1 string, arg2 []byte) error { var arg2Copy []byte if arg2 != nil { arg2Copy = make([]byte, len(arg2)) copy(arg2Copy, arg2) } - fake.writeServerConfigMutex.Lock() - ret, specificReturn := fake.writeServerConfigReturnsOnCall[len(fake.writeServerConfigArgsForCall)] - fake.writeServerConfigArgsForCall = append(fake.writeServerConfigArgsForCall, struct { + fake.writeHTTPServersConfigMutex.Lock() + ret, specificReturn := fake.writeHTTPServersConfigReturnsOnCall[len(fake.writeHTTPServersConfigArgsForCall)] + fake.writeHTTPServersConfigArgsForCall = append(fake.writeHTTPServersConfigArgsForCall, struct { arg1 string arg2 []byte }{arg1, arg2Copy}) - stub := fake.WriteServerConfigStub - fakeReturns := fake.writeServerConfigReturns - fake.recordInvocation("WriteServerConfig", []interface{}{arg1, arg2Copy}) - fake.writeServerConfigMutex.Unlock() + stub := fake.WriteHTTPServersConfigStub + fakeReturns := fake.writeHTTPServersConfigReturns + fake.recordInvocation("WriteHTTPServersConfig", []interface{}{arg1, arg2Copy}) + fake.writeHTTPServersConfigMutex.Unlock() if stub != nil { return stub(arg1, arg2) } @@ -121,44 +49,44 @@ func (fake *FakeManager) WriteServerConfig(arg1 string, arg2 []byte) error { return fakeReturns.result1 } -func (fake *FakeManager) WriteServerConfigCallCount() int { - fake.writeServerConfigMutex.RLock() - defer fake.writeServerConfigMutex.RUnlock() - return len(fake.writeServerConfigArgsForCall) +func (fake *FakeManager) WriteHTTPServersConfigCallCount() int { + fake.writeHTTPServersConfigMutex.RLock() + defer fake.writeHTTPServersConfigMutex.RUnlock() + return len(fake.writeHTTPServersConfigArgsForCall) } -func (fake *FakeManager) WriteServerConfigCalls(stub func(string, []byte) error) { - fake.writeServerConfigMutex.Lock() - defer fake.writeServerConfigMutex.Unlock() - fake.WriteServerConfigStub = stub +func (fake *FakeManager) WriteHTTPServersConfigCalls(stub func(string, []byte) error) { + fake.writeHTTPServersConfigMutex.Lock() + defer fake.writeHTTPServersConfigMutex.Unlock() + fake.WriteHTTPServersConfigStub = stub } -func (fake *FakeManager) WriteServerConfigArgsForCall(i int) (string, []byte) { - fake.writeServerConfigMutex.RLock() - defer fake.writeServerConfigMutex.RUnlock() - argsForCall := fake.writeServerConfigArgsForCall[i] +func (fake *FakeManager) WriteHTTPServersConfigArgsForCall(i int) (string, []byte) { + fake.writeHTTPServersConfigMutex.RLock() + defer fake.writeHTTPServersConfigMutex.RUnlock() + argsForCall := fake.writeHTTPServersConfigArgsForCall[i] return argsForCall.arg1, argsForCall.arg2 } -func (fake *FakeManager) WriteServerConfigReturns(result1 error) { - fake.writeServerConfigMutex.Lock() - defer fake.writeServerConfigMutex.Unlock() - fake.WriteServerConfigStub = nil - fake.writeServerConfigReturns = struct { +func (fake *FakeManager) WriteHTTPServersConfigReturns(result1 error) { + fake.writeHTTPServersConfigMutex.Lock() + defer fake.writeHTTPServersConfigMutex.Unlock() + fake.WriteHTTPServersConfigStub = nil + fake.writeHTTPServersConfigReturns = struct { result1 error }{result1} } -func (fake *FakeManager) WriteServerConfigReturnsOnCall(i int, result1 error) { - fake.writeServerConfigMutex.Lock() - defer fake.writeServerConfigMutex.Unlock() - fake.WriteServerConfigStub = nil - if fake.writeServerConfigReturnsOnCall == nil { - fake.writeServerConfigReturnsOnCall = make(map[int]struct { +func (fake *FakeManager) WriteHTTPServersConfigReturnsOnCall(i int, result1 error) { + fake.writeHTTPServersConfigMutex.Lock() + defer fake.writeHTTPServersConfigMutex.Unlock() + fake.WriteHTTPServersConfigStub = nil + if fake.writeHTTPServersConfigReturnsOnCall == nil { + fake.writeHTTPServersConfigReturnsOnCall = make(map[int]struct { result1 error }) } - fake.writeServerConfigReturnsOnCall[i] = struct { + fake.writeHTTPServersConfigReturnsOnCall[i] = struct { result1 error }{result1} } @@ -166,10 +94,8 @@ func (fake *FakeManager) WriteServerConfigReturnsOnCall(i int, result1 error) { func (fake *FakeManager) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() - fake.deleteServerConfigMutex.RLock() - defer fake.deleteServerConfigMutex.RUnlock() - fake.writeServerConfigMutex.RLock() - defer fake.writeServerConfigMutex.RUnlock() + fake.writeHTTPServersConfigMutex.RLock() + defer fake.writeHTTPServersConfigMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} for key, value := range fake.invocations { copiedInvocations[key] = value diff --git a/internal/nginx/file/manager.go b/internal/nginx/file/manager.go index 947cde4ede..a4901271b1 100644 --- a/internal/nginx/file/manager.go +++ b/internal/nginx/file/manager.go @@ -12,12 +12,10 @@ const confdFolder = "/etc/nginx/conf.d" // Manager manages NGINX configuration files. type Manager interface { - // WriteServerConfig writes the server config on the file system. - // The name distinguishes this server config among all other server configs. For that, it must be unique. + // WriteHTTPServersConfig writes the http servers 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. - WriteServerConfig(name string, cfg []byte) error - // DeleteServerConfig deletes the corresponding configuration file for the server from the file system. - DeleteServerConfig(name string) error + WriteHTTPServersConfig(name string, cfg []byte) error } // ManagerImpl is an implementation of Manager. @@ -28,7 +26,7 @@ func NewManagerImpl() *ManagerImpl { return &ManagerImpl{} } -func (m *ManagerImpl) WriteServerConfig(name string, cfg []byte) error { +func (m *ManagerImpl) WriteHTTPServersConfig(name string, cfg []byte) error { path := getPathForServerConfig(name) file, err := os.Create(path) @@ -46,17 +44,6 @@ func (m *ManagerImpl) WriteServerConfig(name string, cfg []byte) error { return nil } -func (m *ManagerImpl) DeleteServerConfig(name string) error { - path := getPathForServerConfig(name) - - err := os.Remove(path) - if err != nil { - return fmt.Errorf("failed to remove server config %s: %w", path, err) - } - - return nil -} - func getPathForServerConfig(name string) string { return filepath.Join(confdFolder, name+".conf") } diff --git a/internal/state/change_processor.go b/internal/state/change_processor.go new file mode 100644 index 0000000000..fa88e231e3 --- /dev/null +++ b/internal/state/change_processor.go @@ -0,0 +1,111 @@ +package state + +import ( + "fmt" + "sync" + + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/gateway-api/apis/v1alpha2" +) + +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . ChangeProcessor + +// ChangeProcessor processes the changes to resources producing the internal representation of the Gateway configuration. +// ChangeProcessor only supports one Gateway resource. +type ChangeProcessor interface { + // CaptureUpsertChange captures an upsert change to a resource. + // It panics if the resource is of unsupported type or if the passed Gateway is different from the one this ChangeProcessor + // was created for. + CaptureUpsertChange(obj client.Object) + // CaptureDeleteChange captures a delete change to a resource. + // The method panics if the resource is of unsupported type or if the passed Gateway is different from the one this ChangeProcessor + // was created for. + CaptureDeleteChange(resourceType client.Object, nsname types.NamespacedName) + // Process processes any captured changes and produces an internal representation of the Gateway configuration and + // the status information about the processed resources. + // If no changes were captured, the changed return argument will be false and both the configuration and statuses + // will be empty. + Process() (changed bool, conf Configuration, statuses Statuses) +} + +type ChangeProcessorImpl struct { + store *store + changed bool + gwNsName types.NamespacedName + + lock sync.Mutex +} + +// NewChangeProcessorImpl creates a new ChangeProcessorImpl for the Gateway resource with the configured namespace name. +func NewChangeProcessorImpl(gwNsName types.NamespacedName) *ChangeProcessorImpl { + return &ChangeProcessorImpl{ + store: newStore(), + gwNsName: gwNsName, + } +} + +func (c *ChangeProcessorImpl) CaptureUpsertChange(obj client.Object) { + c.lock.Lock() + defer c.lock.Unlock() + + c.changed = true + + switch o := obj.(type) { + case *v1alpha2.Gateway: + if o.Namespace != c.gwNsName.Namespace || o.Name != c.gwNsName.Name { + panic(fmt.Errorf("gateway resource must be %s/%s, got %s/%s", c.gwNsName.Namespace, c.gwNsName.Name, o.Namespace, o.Name)) + } + // if the resource spec hasn't changed (its generation is the same), ignore the upsert + if c.store.gw != nil && c.store.gw.Generation == o.Generation { + c.changed = false + } + c.store.gw = o + case *v1alpha2.HTTPRoute: + // if the resource spec hasn't changed (its generation is the same), ignore the upsert + prev, exist := c.store.httpRoutes[getNamespacedName(obj)] + if exist && o.Generation == prev.Generation { + c.changed = false + } + c.store.httpRoutes[getNamespacedName(obj)] = o + default: + panic(fmt.Errorf("ChangeProcessor doesn't support %T", obj)) + } +} + +func (c *ChangeProcessorImpl) CaptureDeleteChange(resourceType client.Object, nsname types.NamespacedName) { + c.lock.Lock() + defer c.lock.Unlock() + + c.changed = true + + switch o := resourceType.(type) { + case *v1alpha2.Gateway: + if nsname != c.gwNsName { + panic(fmt.Errorf("gateway resource must be %s/%s, got %s/%s", c.gwNsName.Namespace, c.gwNsName.Name, o.Namespace, o.Name)) + } + c.store.gw = nil + case *v1alpha2.HTTPRoute: + delete(c.store.httpRoutes, nsname) + default: + panic(fmt.Errorf("ChangeProcessor doesn't support %T", resourceType)) + } +} + +func (c *ChangeProcessorImpl) Process() (changed bool, conf Configuration, statuses Statuses) { + c.lock.Lock() + defer c.lock.Unlock() + + if !c.changed { + return false, conf, statuses + } + + c.changed = false + + graph := buildGraph(c.store, c.gwNsName) + + conf = buildConfiguration(graph) + statuses = buildStatuses(graph) + + return true, conf, statuses +} diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go new file mode 100644 index 0000000000..c72b61568a --- /dev/null +++ b/internal/state/change_processor_test.go @@ -0,0 +1,452 @@ +package state_test + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" +) + +var _ = Describe("ChangeProcessor", func() { + Describe("Normal cases of processing changes", func() { + var ( + hr1, hr1Updated, hr2 *v1alpha2.HTTPRoute + gw, gwUpdated *v1alpha2.Gateway + processor state.ChangeProcessor + ) + + BeforeEach(OncePerOrdered, func() { + createRoute := func(name string, hostname string) *v1alpha2.HTTPRoute { + return &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: name, + }, + Spec: v1alpha2.HTTPRouteSpec{ + CommonRouteSpec: v1alpha2.CommonRouteSpec{ + ParentRefs: []v1alpha2.ParentRef{ + { + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: "gateway", + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("listener-80-1")), + }, + }, + }, + Hostnames: []v1alpha2.Hostname{ + v1alpha2.Hostname(hostname), + }, + Rules: []v1alpha2.HTTPRouteRule{ + { + Matches: []v1alpha2.HTTPRouteMatch{ + { + Path: &v1alpha2.HTTPPathMatch{ + Value: helpers.GetStringPointer("/"), + }, + }, + }, + }, + }, + }, + } + } + + hr1 = createRoute("hr-1", "foo.example.com") + + hr1Updated = hr1.DeepCopy() + hr1Updated.Generation++ + + hr2 = createRoute("hr-2", "bar.example.com") + + gw = &v1alpha2.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "gateway", + }, + Spec: v1alpha2.GatewaySpec{ + Listeners: []v1alpha2.Listener{ + { + Name: "listener-80-1", + Hostname: nil, + Port: 80, + Protocol: v1alpha2.HTTPProtocolType, + }, + }, + }, + } + + gwUpdated = gw.DeepCopy() + gwUpdated.Generation++ + + processor = state.NewChangeProcessorImpl(types.NamespacedName{Namespace: "test", Name: "gateway"}) + }) + + Describe("Process resources", Ordered, func() { + It("should return empty configuration and statuses when no upsert has occurred", func() { + changed, conf, statuses := processor.Process() + Expect(changed).To(BeFalse()) + Expect(conf).To(BeZero()) + Expect(statuses).To(BeZero()) + }) + + It("should return empty configuration and updated statuses after upserting an HTTPRoute when the Gateway doesn't exist", func() { + processor.CaptureUpsertChange(hr1) + + expectedConf := state.Configuration{HTTPServers: []state.HTTPServer{}} + expectedStatuses := state.Statuses{ + ListenerStatuses: map[string]state.ListenerStatus{}, + HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ + {Namespace: "test", Name: "hr-1"}: { + ParentStatuses: map[string]state.ParentStatus{ + "listener-80-1": {Attached: false}, + }, + }, + }, + } + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + }) + + It("should return updated configuration and statuses after the Gateway is upserted", func() { + processor.CaptureUpsertChange(gw) + + expectedConf := state.Configuration{ + HTTPServers: []state.HTTPServer{ + { + Hostname: "foo.example.com", + PathRules: []state.PathRule{ + { + Path: "/", + MatchRules: []state.MatchRule{ + { + MatchIdx: 0, + RuleIdx: 0, + Source: hr1, + }, + }, + }, + }, + }, + }, + } + expectedStatuses := state.Statuses{ + ListenerStatuses: map[string]state.ListenerStatus{ + "listener-80-1": { + Valid: true, + AttachedRoutes: 1, + }, + }, + HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ + {Namespace: "test", Name: "hr-1"}: { + ParentStatuses: map[string]state.ParentStatus{ + "listener-80-1": {Attached: true}, + }, + }, + }, + } + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + }) + + It("should return empty configuration and statuses after processing upserting the HTTPRoute without generation change", func() { + hr1UpdatedSameGen := hr1.DeepCopy() + // hr1UpdatedSameGen.Generation has not been changed + processor.CaptureUpsertChange(hr1UpdatedSameGen) + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeFalse()) + Expect(conf).To(BeZero()) + Expect(statuses).To(BeZero()) + }) + + It("should return updated configuration and statuses after upserting the HTTPRoute with generation change", func() { + processor.CaptureUpsertChange(hr1Updated) + + expectedConf := state.Configuration{ + HTTPServers: []state.HTTPServer{ + { + Hostname: "foo.example.com", + PathRules: []state.PathRule{ + { + Path: "/", + MatchRules: []state.MatchRule{ + { + MatchIdx: 0, + RuleIdx: 0, + Source: hr1Updated, + }, + }, + }, + }, + }, + }, + } + expectedStatuses := state.Statuses{ + ListenerStatuses: map[string]state.ListenerStatus{ + "listener-80-1": { + Valid: true, + AttachedRoutes: 1, + }, + }, + HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ + {Namespace: "test", Name: "hr-1"}: { + ParentStatuses: map[string]state.ParentStatus{ + "listener-80-1": {Attached: true}, + }, + }, + }, + } + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + }) + + It("should return empty configuration and statuses after processing upserting the Gateway without generation change", func() { + gwUpdatedSameGen := gw.DeepCopy() + // gwUpdatedSameGen.Generation has not been changed + processor.CaptureUpsertChange(gwUpdatedSameGen) + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeFalse()) + Expect(conf).To(BeZero()) + Expect(statuses).To(BeZero()) + }) + + It("should return updated configuration and statuses after upserting the Gateway with generation change", func() { + processor.CaptureUpsertChange(gwUpdated) + + expectedConf := state.Configuration{ + HTTPServers: []state.HTTPServer{ + { + Hostname: "foo.example.com", + PathRules: []state.PathRule{ + { + Path: "/", + MatchRules: []state.MatchRule{ + { + MatchIdx: 0, + RuleIdx: 0, + Source: hr1Updated, + }, + }, + }, + }, + }, + }, + } + expectedStatuses := state.Statuses{ + ListenerStatuses: map[string]state.ListenerStatus{ + "listener-80-1": { + Valid: true, + AttachedRoutes: 1, + }, + }, + HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ + {Namespace: "test", Name: "hr-1"}: { + ParentStatuses: map[string]state.ParentStatus{ + "listener-80-1": {Attached: true}, + }, + }, + }, + } + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + }) + + It("should return empty configuration and statuses after processing without capturing any changes", func() { + changed, conf, statuses := processor.Process() + + Expect(changed).To(BeFalse()) + Expect(conf).To(BeZero()) + Expect(statuses).To(BeZero()) + }) + + It("should return updated configuration and statuses after a second HTTPRoute is upserted", func() { + processor.CaptureUpsertChange(hr2) + + expectedConf := state.Configuration{ + HTTPServers: []state.HTTPServer{ + { + Hostname: "bar.example.com", + PathRules: []state.PathRule{ + { + Path: "/", + MatchRules: []state.MatchRule{ + { + MatchIdx: 0, + RuleIdx: 0, + Source: hr2, + }, + }, + }, + }, + }, + { + Hostname: "foo.example.com", + PathRules: []state.PathRule{ + { + Path: "/", + MatchRules: []state.MatchRule{ + { + MatchIdx: 0, + RuleIdx: 0, + Source: hr1Updated, + }, + }, + }, + }, + }, + }, + } + expectedStatuses := state.Statuses{ + ListenerStatuses: map[string]state.ListenerStatus{ + "listener-80-1": { + Valid: true, + AttachedRoutes: 2, + }, + }, + HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ + {Namespace: "test", Name: "hr-1"}: { + ParentStatuses: map[string]state.ParentStatus{ + "listener-80-1": {Attached: true}, + }, + }, + {Namespace: "test", Name: "hr-2"}: { + ParentStatuses: map[string]state.ParentStatus{ + "listener-80-1": {Attached: true}, + }, + }, + }, + } + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + }) + + It("should return updated configuration and statuses after deleting the second HTTPRoute", func() { + processor.CaptureDeleteChange(&v1alpha2.HTTPRoute{}, types.NamespacedName{Namespace: "test", Name: "hr-2"}) + + expectedConf := state.Configuration{ + HTTPServers: []state.HTTPServer{ + { + Hostname: "foo.example.com", + PathRules: []state.PathRule{ + { + Path: "/", + MatchRules: []state.MatchRule{ + { + MatchIdx: 0, + RuleIdx: 0, + Source: hr1Updated, + }, + }, + }, + }, + }, + }, + } + expectedStatuses := state.Statuses{ + ListenerStatuses: map[string]state.ListenerStatus{ + "listener-80-1": { + Valid: true, + AttachedRoutes: 1, + }, + }, + HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ + {Namespace: "test", Name: "hr-1"}: { + ParentStatuses: map[string]state.ParentStatus{ + "listener-80-1": {Attached: true}, + }, + }, + }, + } + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + }) + + It("should return empty configuration and updated statuses after deleting the Gateway", func() { + processor.CaptureDeleteChange(&v1alpha2.Gateway{}, types.NamespacedName{Namespace: "test", Name: "gateway"}) + + expectedConf := state.Configuration{HTTPServers: []state.HTTPServer{}} + expectedStatuses := state.Statuses{ + ListenerStatuses: map[string]state.ListenerStatus{}, + HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ + {Namespace: "test", Name: "hr-1"}: { + ParentStatuses: map[string]state.ParentStatus{ + "listener-80-1": {Attached: false}, + }, + }, + }, + } + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + }) + + It("should return empty configuration and statuses after deleting the first HTTPRoute", func() { + processor.CaptureDeleteChange(&v1alpha2.HTTPRoute{}, types.NamespacedName{Namespace: "test", Name: "hr-1"}) + + expectedConf := state.Configuration{HTTPServers: []state.HTTPServer{}} + expectedStatuses := state.Statuses{ + ListenerStatuses: map[string]state.ListenerStatus{}, + HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{}, + } + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + }) + }) + }) + + Describe("Edge cases with panic", func() { + var processor state.ChangeProcessor + + BeforeEach(func() { + processor = state.NewChangeProcessorImpl(types.NamespacedName{Namespace: "test", Name: "gateway"}) + }) + + DescribeTable("CaptureUpsertChange must panic", + func(obj client.Object) { + process := func() { + processor.CaptureUpsertChange(obj) + } + Expect(process).Should(Panic()) + }, + Entry("an unsupported resource", &v1alpha2.TCPRoute{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: "tcp"}}), + Entry("a wrong gateway", &v1alpha2.Gateway{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: "other-gateway"}})) + + DescribeTable("CaptureDeleteChange must panic", + func(resourceType client.Object, nsname types.NamespacedName) { + process := func() { + processor.CaptureDeleteChange(resourceType, nsname) + } + Expect(process).Should(Panic()) + }, + Entry("an unsupported resource", &v1alpha2.TCPRoute{}, types.NamespacedName{Namespace: "test", Name: "tcp"}), + Entry("a wrong gateway", &v1alpha2.Gateway{}, types.NamespacedName{Namespace: "test", Name: "other-gateway"})) + }) +}) diff --git a/internal/state/clock.go b/internal/state/clock.go deleted file mode 100644 index 58fb5bb596..0000000000 --- a/internal/state/clock.go +++ /dev/null @@ -1,37 +0,0 @@ -package state - -import "time" - -// Clock returns the current local time. -type Clock interface { - Now() time.Time -} - -// Real clock returns the current local time. -type RealClock struct { -} - -// NewRealClock creates a new RealClock. -func NewRealClock() *RealClock { - return &RealClock{} -} - -// Now returns the current local time. -func (c *RealClock) Now() time.Time { - return time.Now() -} - -// FakeClock allows you to control the returned time. -type FakeClock struct { - time time.Time -} - -// NewFakeClock creates a FakeClock. The clock will always return the specified time. -func NewFakeClock(time time.Time) *FakeClock { - return &FakeClock{time: time} -} - -// Now is a fake implementation of Now(). -func (c FakeClock) Now() time.Time { - return c.time -} diff --git a/internal/state/clock_test.go b/internal/state/clock_test.go deleted file mode 100644 index f32e2c22fe..0000000000 --- a/internal/state/clock_test.go +++ /dev/null @@ -1,16 +0,0 @@ -package state - -import ( - "testing" - "time" -) - -func TestFakeClock(t *testing.T) { - time := time.Now() - clock := NewFakeClock(time) - - result := clock.Now() - if result != time { - t.Errorf("Now() returned %v but expected %v", result, time) - } -} diff --git a/internal/state/configuration.go b/internal/state/configuration.go index 0def2ee475..864c672796 100644 --- a/internal/state/configuration.go +++ b/internal/state/configuration.go @@ -1,119 +1,39 @@ package state import ( - "fmt" - "reflect" - "strings" + "sort" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/gateway-api/apis/v1alpha2" ) -// httpListener defines an HTTP Listener. -type httpListener struct { - // hosts include all Hosts that belong to the listener. - hosts hosts - // httpRoutes include all HTTPRoute resources that belong to the listener. - httpRoutes httpRoutes +// Configuration is an internal representation of Gateway configuration. +// We can think of Configuration as an intermediate state between the Gateway API resources and the data plane (NGINX) +// configuration. +type Configuration struct { + // HTTPServers holds all HTTPServers. + // FIXME(pleshakov) We assume that all servers are HTTP and listen on port 80. + HTTPServers []HTTPServer } -type hosts map[string]Host - -func (hs hosts) Keys() []string { - keys := make([]string, 0, len(hs)) - - for k := range hs { - keys = append(keys, k) - } - - return keys -} - -type httpRoutes map[string]*v1alpha2.HTTPRoute - -func (hrs httpRoutes) Keys() []string { - keys := make([]string, 0, len(hrs)) - - for k := range hrs { - keys = append(keys, k) - } - - return keys -} - -// Host is the primary configuration unit of the internal representation. -// It corresponds to an NGINX server block with server_name Value; -// See https://nginx.org/en/docs/http/ngx_http_core_module.html#server -type Host struct { - // Value is the host value (hostname). - Value string - // PathRouteGroups include all PathRouteGroups that belong to the Host. - // We use a slice rather than a map to control the order of the routes. - PathRouteGroups []PathRouteGroup -} - -// String returns a printable representation of a Host. -func (h *Host) String() string { - var b strings.Builder - - b.WriteString(fmt.Sprintf("Host: Value: %s\n", h.Value)) - b.WriteString("PathRouteGroups:\n") - - for _, g := range h.PathRouteGroups { - b.WriteString(g.String()) - b.WriteByte('\n') - } - - return b.String() +// HTTPServer is a virtual server. +type HTTPServer struct { + // Hostname is the hostname of the server. + Hostname string + // PathRules is a collection of routing rules. + PathRules []PathRule } -// PathRouteGroup represents a collection of Routes grouped by a path. -// Among those Routes, there will be routing rules with additional matching criteria. For example, matching of headers. -// The reason we group Routes by Path is how NGINX processes requests: its primary routing rule mechanism is a location block. -// See https://nginx.org/en/docs/http/ngx_http_core_module.html#location -type PathRouteGroup struct { - // Path is the path (URI). +// PathRule represents routing rules that share a common path. +type PathRule struct { + // Path is a path. For example, '/hello'. Path string - // Routes include all Routes for that path. - // Routes are sorted based on the creation timestamp and namespace/name of the Route source (HTTPRoute). This way - // the ordering resolves the conflicts among any conflicting rules. - // Sorting is stable so that the Routes retain the order of appearance of the corresponding matches in the corresponding - // HTTPRoute resources. - // The first "fired" Route will win in the NGINX configuration. - Routes []Route + // MatchRules holds routing rules. + MatchRules []MatchRule } -// String returns a printable representation of a PathRouteGroup. -func (g *PathRouteGroup) String() string { - var b strings.Builder - - b.WriteString(fmt.Sprintf("PathRouteGroup: Path: %s\n", g.Path)) - b.WriteString("Routes:\n") - - for _, r := range g.Routes { - b.WriteString(r.String()) - b.WriteByte('\n') - } - - return b.String() -} - -type pathRoutesGroups map[string]PathRouteGroup - -func (prs pathRoutesGroups) Keys() []string { - keys := make([]string, 0, len(prs)) - - for k := range prs { - keys = append(keys, k) - } - - return keys -} - -// Route represents a Route, which corresponds to a Match in the HTTPRouteRule. If a rule doesn't define any matches, -// it is assumed that the rule is for "/" path. -type Route struct { +// MatchRule represents a routing rule. It corresponds directly to a Match in the HTTPRoute resource. +type MatchRule struct { // MatchIdx is the index of the rule in the Rule.Matches or -1 if there are no matches. MatchIdx int // RuleIdx is the index of the corresponding rule in the HTTPRoute. @@ -122,382 +42,110 @@ type Route struct { Source *v1alpha2.HTTPRoute } -// String returns a printable representation of a Route. -func (r *Route) String() string { - return fmt.Sprintf("Route: Source: %s, RuleIdx: %d, MatchIdx: %d", getResourceKey(&r.Source.ObjectMeta), r.RuleIdx, r.MatchIdx) -} - // GetMatch returns the HTTPRouteMatch of the Route and true if it exists. // If there is no Match defined on the Route, GetMatch returns an empty HTTPRouteMatch and false. -func (r *Route) GetMatch() (v1alpha2.HTTPRouteMatch, bool) { +func (r *MatchRule) GetMatch() (v1alpha2.HTTPRouteMatch, bool) { if r.MatchIdx == -1 { return v1alpha2.HTTPRouteMatch{}, false } return r.Source.Spec.Rules[r.RuleIdx].Matches[r.MatchIdx], true } -// Operation defines an operation to be performed for a Host. -type Operation int - -const ( - // Delete the config for the Host. - Delete Operation = iota - // Upsert the config for the Host. - Upsert -) +// buildConfiguration builds the Configuration from the graph. +func buildConfiguration(graph *graph) Configuration { + // FIXME(pleshakov) For now we only handle paths with prefix matches. Handle exact and regex matches + pathRulesForHosts := make(map[string]map[string]PathRule) -// Change represents a change of the Host that needs to be reflected in the NGINX config. -type Change struct { - // Op is the operation to be performed. - Op Operation - // Host is a reference to the Host associated with the Change. - Host Host -} + for _, l := range graph.Listeners { + for _, r := range l.Routes { + var hostnames []string -// StatusUpdate represents an update to the status of a resource. -type StatusUpdate struct { - // NamespacedName is the NamespacedName of the resource. - NamespacedName types.NamespacedName - // Status is the status field of the resource - // The Status include only the new conditions. This means that the status reporter component will need to merge - // the new conditions with the existing conditions of the resource. - Status interface{} -} - -//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Configuration - -// Configuration represents the configuration of the Gateway - a collection of routing rules ready to be transformed -// into NGINX configuration. -// The methods of Configuration update its internal state and return changes and status updates that correspond to that -// update of the internal state. -type Configuration interface { - // UpsertHTTPRoute upserts an HTTPRoute into the Configuration. - UpsertHTTPRoute(httpRoute *v1alpha2.HTTPRoute) ([]Change, []StatusUpdate) - // DeleteHTTPRoute deletes an HTTPRoute from the Configuration. - DeleteHTTPRoute(nsname types.NamespacedName) ([]Change, []StatusUpdate) -} - -// configurationImpl is an implementation of Configuration. -type configurationImpl struct { - // caches of valid resources - httpRoutes httpRoutes - - // internal representation of Gateway configuration - httpListeners map[string]httpListener - - gatewayCtlrName string - clock Clock -} - -// NewConfiguration creates a Configuration. -// It is expected that the client set gatewayCtlrName to a non-empty value. -func NewConfiguration(gatewayCtlrName string, clock Clock) Configuration { - c := &configurationImpl{ - httpRoutes: make(httpRoutes), - httpListeners: make(map[string]httpListener), - gatewayCtlrName: gatewayCtlrName, - clock: clock, - } - - // Until we process the GatewayClass and Gateway resources, we assume the "http" listener always exists. - c.httpListeners["http"] = httpListener{ - hosts: make(hosts), - } - - return c -} - -// UpsertHTTPRoute upserts an HTTPRoute into the Configuration. -func (c *configurationImpl) UpsertHTTPRoute(httpRoute *v1alpha2.HTTPRoute) ([]Change, []StatusUpdate) { - key := getResourceKey(&httpRoute.ObjectMeta) - - oldHR, exist := c.httpRoutes[key] - if exist && compareObjectMetas(&oldHR.ObjectMeta, &httpRoute.ObjectMeta) { - // nothing to do - the resource hasn't changed - return nil, nil - } - - c.httpRoutes[key] = httpRoute - - return c.updateListeners() -} - -// DeleteHTTPRoute deletes an HTTPRoute from the Configuration. -func (c *configurationImpl) DeleteHTTPRoute(nsname types.NamespacedName) ([]Change, []StatusUpdate) { - delete(c.httpRoutes, nsname.String()) - - return c.updateListeners() -} - -func (c *configurationImpl) updateListeners() ([]Change, []StatusUpdate) { - var changes []Change - - // for now, we support only one listener - - c.httpListeners["http"], changes = rebuildHTTPListener(c.httpListeners["http"], c.httpRoutes) - - listener := c.httpListeners["http"] - - statusUpdates := make([]StatusUpdate, 0, len(listener.httpRoutes)) - - // FIXME(pleshakov): optimize it so that we only update the status of the affected (changed) httpRoutes - // getSortedKeys is used to ensure predictable order for unit tests - for _, key := range getSortedKeys(listener.httpRoutes) { - route := listener.httpRoutes[key] - update := StatusUpdate{ - NamespacedName: types.NamespacedName{Namespace: route.Namespace, Name: route.Name}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ - { - ParentRef: v1alpha2.ParentRef{ - Name: "fake", // FIXME(pleshakov): report the parent ref properly - }, - ControllerName: v1alpha2.GatewayController(c.gatewayCtlrName), - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: listener.httpRoutes[key].Generation, - LastTransitionTime: metav1.NewTime(c.clock.Now()), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", // FIXME(pleshakov): figure out a good message - }, - }, - }, - }, - }, - }, - } - statusUpdates = append(statusUpdates, update) - } - - // FIXME(pleshakov): remove the accepted condition for the excluded (no longer handled) Routes - - return changes, statusUpdates -} - -func rebuildHTTPListener(listener httpListener, httpRoutes httpRoutes) (httpListener, []Change) { - pathRoutesForHosts := buildPathRoutesGroupsForHosts(httpRoutes) - - newHosts, newHTTPRoutes := buildHostsAndDetermineHTTPRoutes(pathRoutesForHosts) - - removedHosts, updatedHosts, addedHosts := determineChangesInHosts(listener, newHosts) - - changes := createChanges(removedHosts, updatedHosts, addedHosts, listener.hosts, newHosts) - - newListener := httpListener{ - hosts: newHosts, - httpRoutes: newHTTPRoutes, - } - - return newListener, changes -} - -func createChanges(removedHosts []string, updatedHosts []string, addedHosts []string, oldHosts hosts, newHosts hosts) []Change { - var changes []Change - - for _, h := range removedHosts { - change := Change{ - Op: Delete, - Host: oldHosts[h], - } - changes = append(changes, change) - } - - for _, h := range updatedHosts { - change := Change{ - Op: Upsert, - Host: newHosts[h], - } - changes = append(changes, change) - } - - for _, h := range addedHosts { - change := Change{ - Op: Upsert, - Host: newHosts[h], - } - changes = append(changes, change) - } - - return changes -} - -func determineChangesInHosts(listener httpListener, newHosts hosts) (removedHosts []string, updatedHosts []string, addedHosts []string) { - // getSortedKeys is used to ensure predictable order for unit tests - - // FIXME(pleshakov): consider using a data structure for sets - - for _, h := range getSortedKeys(listener.hosts) { - _, exists := newHosts[h] - if !exists { - removedHosts = append(removedHosts, h) - } - } - - for _, h := range getSortedKeys(newHosts) { - _, exists := listener.hosts[h] - if !exists { - addedHosts = append(addedHosts, h) - } - } - - for _, h := range getSortedKeys(newHosts) { - oldHost, exists := listener.hosts[h] - if !exists { - continue - } - - if !arePathRoutesEqual(oldHost.PathRouteGroups, newHosts[h].PathRouteGroups) { - updatedHosts = append(updatedHosts, h) - } - } - return removedHosts, updatedHosts, addedHosts -} - -func buildHostsAndDetermineHTTPRoutes(routeGroupsForHosts map[string]pathRoutesGroups) (hosts, httpRoutes) { - hosts := make(hosts) - routes := make(httpRoutes) - - for h, groups := range routeGroupsForHosts { - host := Host{ - Value: h, - } - - host.PathRouteGroups = make([]PathRouteGroup, 0, len(groups)) - - // getSortedKeys is used so that the order of locations in the NGINX config doesn't change on every config - // regeneration. However, that sorting will mess up the original order of the rules in the HTTPRoutes. - // The order can be important when regexes are used. - // See https://nginx.org/en/docs/http/ngx_http_core_module.html#location to learn how NGINX searches for - // a location. - // This comment is to be aware of a potential issue. However, it is not yet clear whether it is an issue. - for _, path := range getSortedKeys(groups) { - group := groups[path] - - sortRoutes(group.Routes) - - host.PathRouteGroups = append(host.PathRouteGroups, group) - - for _, r := range group.Routes { - key := getResourceKey(&r.Source.ObjectMeta) - routes[key] = r.Source + for _, h := range r.Source.Spec.Hostnames { + if _, exist := l.AcceptedHostnames[string(h)]; exist { + hostnames = append(hostnames, string(h)) + } } - } - - hosts[h] = host - } - return hosts, routes -} -func buildPathRoutesGroupsForHosts(httpRoutes httpRoutes) map[string]pathRoutesGroups { - routeGroupsForHosts := make(map[string]pathRoutesGroups) - - // for now, we take in all available HTTPRoutes - for _, hr := range httpRoutes { - // every hostname x every routing rule - for _, h := range hr.Spec.Hostnames { - groups, exist := routeGroupsForHosts[string(h)] - if !exist { - groups = make(pathRoutesGroups) - routeGroupsForHosts[string(h)] = groups + for _, h := range hostnames { + if _, exist := pathRulesForHosts[h]; !exist { + pathRulesForHosts[h] = make(map[string]PathRule) + } } - for i := range hr.Spec.Rules { - rule := &hr.Spec.Rules[i] - - if len(rule.Matches) == 0 { - group, exist := groups["/"] - if !exist { - group = PathRouteGroup{ - Path: "/", - } - } - - group.Routes = append(group.Routes, Route{ - MatchIdx: -1, - RuleIdx: i, - Source: hr, - }) - - groups["/"] = group - } else { + for i, rule := range r.Source.Spec.Rules { + for _, h := range hostnames { for j, m := range rule.Matches { - path := "/" - if m.Path != nil && m.Path.Value != nil && *m.Path.Value != "/" { - path = *m.Path.Value - } + path := getPath(m.Path) - group, exist := groups[path] + rule, exist := pathRulesForHosts[h][path] if !exist { - group = PathRouteGroup{ - Path: path, - } + rule.Path = path } - group.Routes = append(group.Routes, Route{ + rule.MatchRules = append(rule.MatchRules, MatchRule{ MatchIdx: j, RuleIdx: i, - Source: hr, + Source: r.Source, }) - groups[path] = group + pathRulesForHosts[h][path] = rule } } } } } - return routeGroupsForHosts -} + httpServers := make([]HTTPServer, 0, len(pathRulesForHosts)) -func arePathRoutesEqual(pathRoutes1, pathRoutes2 []PathRouteGroup) bool { - if len(pathRoutes1) != len(pathRoutes2) { - return false - } - - for i := 0; i < len(pathRoutes1); i++ { - if pathRoutes1[i].Path != pathRoutes2[i].Path { - return false + for h, rules := range pathRulesForHosts { + s := HTTPServer{ + Hostname: h, + PathRules: make([]PathRule, 0, len(rules)), } - if len(pathRoutes1[i].Routes) != len(pathRoutes2[i].Routes) { - return false + for _, r := range rules { + // sort matches in every PathRule based on the Source timestamp and its namespace/name + // for conflict resolution of conflicting rules + // stable sort so that the order of matches within one HTTPRoute is preserved + sort.SliceStable(r.MatchRules, func(i, j int) bool { + return lessObjectMeta(&r.MatchRules[i].Source.ObjectMeta, &r.MatchRules[j].Source.ObjectMeta) + }) + + s.PathRules = append(s.PathRules, r) } - for j := 0; j < len(pathRoutes1[i].Routes); j++ { - if !compareObjectMetas(&pathRoutes1[i].Routes[j].Source.ObjectMeta, &pathRoutes2[i].Routes[j].Source.ObjectMeta) { - return false - } + // sort rules for predictable order + sort.Slice(s.PathRules, func(i, j int) bool { + return s.PathRules[i].Path < s.PathRules[j].Path + }) - // DeepEqual might not be needed - the comparison above might be enough - idx1 := pathRoutes1[i].Routes[j].RuleIdx - rule1 := pathRoutes1[i].Routes[j].Source.Spec.Rules[idx1] + httpServers = append(httpServers, s) + } - idx2 := pathRoutes2[i].Routes[j].RuleIdx - rule2 := pathRoutes2[i].Routes[j].Source.Spec.Rules[idx2] + // sort servers for predictable order + sort.Slice(httpServers, func(i, j int) bool { + return httpServers[i].Hostname < httpServers[j].Hostname + }) - if !reflect.DeepEqual(rule1, rule2) { - return false - } - } + return Configuration{ + HTTPServers: httpServers, } - - return true } -func compareObjectMetas(meta1 *metav1.ObjectMeta, meta2 *metav1.ObjectMeta) bool { - // Two resources are different if: - // (1) They have different namespaces or names. - // (2) They have the same namespace and name (resources are the same resource) but their specs are different. - // If their specs are different, their Generations are different too. So we only test their Generations. - // note: annotations are not part of the spec, so their update doesn't affect the Generation. - return meta1.Namespace == meta2.Namespace && - meta1.Name == meta2.Name && - meta1.Generation == meta2.Generation +func lessObjectMeta(meta1 *metav1.ObjectMeta, meta2 *metav1.ObjectMeta) bool { + if meta1.CreationTimestamp.Equal(&meta2.CreationTimestamp) { + if meta1.Namespace == meta2.Namespace { + return meta1.Name < meta2.Name + } + return meta1.Namespace < meta2.Namespace + } + + return meta1.CreationTimestamp.Before(&meta2.CreationTimestamp) } -func getResourceKey(meta *metav1.ObjectMeta) string { - return fmt.Sprintf("%s/%s", meta.Namespace, meta.Name) +func getPath(path *v1alpha2.HTTPPathMatch) string { + if path == nil || path.Value == nil || *path.Value == "" { + return "/" + } + return *path.Value } diff --git a/internal/state/configuration_test.go b/internal/state/configuration_test.go index b29128d975..84f4e5106f 100644 --- a/internal/state/configuration_test.go +++ b/internal/state/configuration_test.go @@ -1,889 +1,410 @@ -package state_test +package state import ( "testing" "time" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" + "github.com/google/go-cmp/cmp" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/gateway-api/apis/v1alpha2" "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" ) -const gatewayCtlrName = v1alpha2.GatewayController("test-name") - -var _ = Describe("Configuration", func() { - Describe("Processing HTTPRoutes", func() { - var conf state.Configuration - - constTime := time.Now() - - BeforeEach(OncePerOrdered, func() { - conf = state.NewConfiguration(string(gatewayCtlrName), state.NewFakeClock(constTime)) - }) - - Describe("Process one HTTPRoute with one host", Ordered, func() { - var hr, updatedHRWithSameGen, updatedHRWithIncrementedGen *v1alpha2.HTTPRoute - - BeforeAll(func() { - hr = &v1alpha2.HTTPRoute{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "route1", - }, - Spec: v1alpha2.HTTPRouteSpec{ - Hostnames: []v1alpha2.Hostname{ - "cafe.example.com", - }, - Rules: []v1alpha2.HTTPRouteRule{ - { - // mo matches -> "/" - }, - { - Matches: []v1alpha2.HTTPRouteMatch{ - { - Path: &v1alpha2.HTTPPathMatch{ - Value: helpers.GetStringPointer("/coffee"), - }, - }, - }, - }, - }, - }, - } - - updatedHRWithSameGen = hr.DeepCopy() - updatedHRWithSameGen.Spec.Rules[1].Matches[0].Path.Value = helpers.GetStringPointer("/tea") - - updatedHRWithIncrementedGen = updatedHRWithSameGen.DeepCopy() - updatedHRWithIncrementedGen.Generation++ - }) - - It("should upsert a host and generate a status update for the new HTTPRoute", func() { - expectedChanges := []state.Change{ +func TestBuildConfiguration(t *testing.T) { + createRoute := func(name string, hostname string, paths ...string) *v1alpha2.HTTPRoute { + rules := make([]v1alpha2.HTTPRouteRule, 0, len(paths)) + for _, p := range paths { + rules = append(rules, v1alpha2.HTTPRouteRule{ + Matches: []v1alpha2.HTTPRouteMatch{ { - Op: state.Upsert, - Host: state.Host{ - Value: "cafe.example.com", - PathRouteGroups: []state.PathRouteGroup{ - { - Path: "/", - Routes: []state.Route{ - { - MatchIdx: -1, - RuleIdx: 0, - Source: hr, - }, - }, - }, - { - Path: "/coffee", - Routes: []state.Route{ - { - MatchIdx: 0, - RuleIdx: 1, - Source: hr, - }, - }, - }, - }, + Path: &v1alpha2.HTTPPathMatch{ + Value: helpers.GetStringPointer(p), }, }, - } - expectedStatusUpdates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ - { - ControllerName: gatewayCtlrName, - ParentRef: v1alpha2.ParentRef{ - Name: "fake", - }, - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: hr.Generation, - LastTransitionTime: metav1.NewTime(constTime), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", - }, - }, - }, - }, - }, - }, - }, - } - - changes, statusUpdates := conf.UpsertHTTPRoute(hr) - Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) - }) - - It("should not generate changes and status updates for the updated HTTPRoute because it has the same generation as the old one", func() { - changes, statusUpdates := conf.UpsertHTTPRoute(updatedHRWithSameGen) - Expect(changes).To(BeEmpty()) - Expect(statusUpdates).To(BeEmpty()) + }, }) - - It("should upsert the host changes and generate a status update for the updated HTTPRoute", func() { - expectedChanges := []state.Change{ - { - Op: state.Upsert, - Host: state.Host{ - Value: "cafe.example.com", - PathRouteGroups: []state.PathRouteGroup{ - { - Path: "/", - Routes: []state.Route{ - { - MatchIdx: -1, - RuleIdx: 0, - Source: updatedHRWithIncrementedGen, - }, - }, - }, - { - Path: "/tea", - Routes: []state.Route{ - { - MatchIdx: 0, - RuleIdx: 1, - Source: updatedHRWithIncrementedGen, - }, - }, - }, - }, - }, - }, - } - expectedStatusUpdates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ - { - ControllerName: gatewayCtlrName, - ParentRef: v1alpha2.ParentRef{ - Name: "fake", - }, - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: updatedHRWithIncrementedGen.Generation, - LastTransitionTime: metav1.NewTime(constTime), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", - }, - }, - }, - }, - }, + } + return &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: name, + }, + Spec: v1alpha2.HTTPRouteSpec{ + CommonRouteSpec: v1alpha2.CommonRouteSpec{ + ParentRefs: []v1alpha2.ParentRef{ + { + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: "gateway", + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("listener-80-1")), }, }, - } + }, + Hostnames: []v1alpha2.Hostname{ + v1alpha2.Hostname(hostname), + }, + Rules: rules, + }, + } + } - changes, statusUpdates := conf.UpsertHTTPRoute(updatedHRWithIncrementedGen) - Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) - }) + hr1 := createRoute("hr-1", "foo.example.com", "/") - It("should delete the host for the deleted HTTPRoute", func() { - expectedChanges := []state.Change{ - { - Op: state.Delete, - Host: state.Host{ - Value: "cafe.example.com", - PathRouteGroups: []state.PathRouteGroup{ - { - Path: "/", - Routes: []state.Route{ - { - MatchIdx: -1, - RuleIdx: 0, - Source: updatedHRWithIncrementedGen, - }, - }, - }, - { - Path: "/tea", - Routes: []state.Route{ - { - MatchIdx: 0, - RuleIdx: 1, - Source: updatedHRWithIncrementedGen, - }, - }, - }, - }, - }, - }, - } + routeHR1 := &route{ + Source: hr1, + ValidSectionNameRefs: map[string]struct{}{ + "listener-80-1": {}, + }, + InvalidSectionNameRefs: map[string]struct{}{}, + } - changes, statusUpdates := conf.DeleteHTTPRoute(types.NamespacedName{Namespace: "test", Name: "route1"}) - Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) - Expect(statusUpdates).To(BeEmpty()) - }) - }) + hr2 := createRoute("hr-2", "bar.example.com", "/") - It("should allow removing an non-exiting HTTPRoute", func() { - changes, statusUpdates := conf.DeleteHTTPRoute(types.NamespacedName{Namespace: "test", Name: "some-route"}) - Expect(changes).To(BeEmpty()) - Expect(statusUpdates).To(BeEmpty()) - }) + routeHR2 := &route{ + Source: hr2, + ValidSectionNameRefs: map[string]struct{}{ + "listener-80-1": {}, + }, + InvalidSectionNameRefs: map[string]struct{}{}, + } - Describe("Processing multiple HTTPRoutes for one host", Ordered, func() { - var hr1, hr2, hr2Updated *v1alpha2.HTTPRoute + hr3 := createRoute("hr-3", "foo.example.com", "/", "/third") - BeforeAll(func() { - hr1 = &v1alpha2.HTTPRoute{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "route1", - }, - Spec: v1alpha2.HTTPRouteSpec{ - Hostnames: []v1alpha2.Hostname{ - "cafe.example.com", - }, - Rules: []v1alpha2.HTTPRouteRule{ - { - // mo matches -> "/" - }, - }, - }, - } + routeHR3 := &route{ + Source: hr3, + ValidSectionNameRefs: map[string]struct{}{ + "listener-80-1": {}, + }, + InvalidSectionNameRefs: map[string]struct{}{}, + } - hr2 = &v1alpha2.HTTPRoute{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "route2", - }, - Spec: v1alpha2.HTTPRouteSpec{ - Hostnames: []v1alpha2.Hostname{ - "cafe.example.com", - }, - Rules: []v1alpha2.HTTPRouteRule{ - { - Matches: []v1alpha2.HTTPRouteMatch{ - { - Path: &v1alpha2.HTTPPathMatch{ - Value: helpers.GetStringPointer("/coffee"), - }, - }, - }, - }, - }, - }, - } + hr4 := createRoute("hr-4", "foo.example.com", "/fourth", "/") - hr2Updated = hr2.DeepCopy() - hr2Updated.Spec.Rules[0].Matches[0].Path.Value = helpers.GetStringPointer("/tea") - hr2Updated.Generation++ - }) + routeHR4 := &route{ + Source: hr4, + ValidSectionNameRefs: map[string]struct{}{ + "listener-80-1": {}, + }, + InvalidSectionNameRefs: map[string]struct{}{}, + } - It("should upsert a host and generate a status update for the first HTTPRoute", func() { - expectedChanges := []state.Change{ - { - Op: state.Upsert, - Host: state.Host{ - Value: "cafe.example.com", - PathRouteGroups: []state.PathRouteGroup{ - { - Path: "/", - Routes: []state.Route{ - { - MatchIdx: -1, - RuleIdx: 0, - Source: hr1, - }, - }, - }, - }, - }, + tests := []struct { + graph *graph + expected Configuration + msg string + }{ + { + graph: &graph{ + Listeners: map[string]*listener{}, + Routes: map[types.NamespacedName]*route{}, + }, + expected: Configuration{ + HTTPServers: []HTTPServer{}, + }, + msg: "empty graph", + }, + { + graph: &graph{ + Listeners: map[string]*listener{ + "listener-80-1": { + Valid: true, + Routes: map[types.NamespacedName]*route{}, + AcceptedHostnames: map[string]struct{}{}, }, - } - expectedStatusUpdates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ - { - ControllerName: gatewayCtlrName, - ParentRef: v1alpha2.ParentRef{ - Name: "fake", - }, - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: hr1.Generation, - LastTransitionTime: metav1.NewTime(constTime), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", - }, - }, - }, - }, - }, + }, + Routes: map[types.NamespacedName]*route{}, + }, + expected: Configuration{ + HTTPServers: []HTTPServer{}, + }, + msg: "listener with no routes", + }, + { + graph: &graph{ + Listeners: map[string]*listener{ + "listener-80-1": { + Valid: true, + Routes: map[types.NamespacedName]*route{ + {Namespace: "test", Name: "hr-1"}: routeHR1, + {Namespace: "test", Name: "hr-2"}: routeHR2, }, - }, - } - - changes, statusUpdates := conf.UpsertHTTPRoute(hr1) - Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) - }) - - It("should upsert the same host and generate status updates for both HTTPRoutes after adding the second", func() { - expectedChanges := []state.Change{ - { - Op: state.Upsert, - Host: state.Host{ - Value: "cafe.example.com", - PathRouteGroups: []state.PathRouteGroup{ - { - Path: "/", - Routes: []state.Route{ - { - MatchIdx: -1, - RuleIdx: 0, - Source: hr1, - }, - }, - }, - { - Path: "/coffee", - Routes: []state.Route{ - { - MatchIdx: 0, - RuleIdx: 0, - Source: hr2, - }, - }, - }, - }, + AcceptedHostnames: map[string]struct{}{ + "foo.example.com": {}, + "bar.example.com": {}, }, }, - } - expectedStatusUpdates := []state.StatusUpdate{ + }, + Routes: map[types.NamespacedName]*route{ + {Namespace: "test", Name: "hr-1"}: routeHR1, + {Namespace: "test", Name: "hr-2"}: routeHR2, + }, + }, + expected: Configuration{ + HTTPServers: []HTTPServer{ { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ + Hostname: "bar.example.com", + PathRules: []PathRule{ + { + Path: "/", + MatchRules: []MatchRule{ { - ControllerName: gatewayCtlrName, - ParentRef: v1alpha2.ParentRef{ - Name: "fake", - }, - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: hr1.Generation, - LastTransitionTime: metav1.NewTime(constTime), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", - }, - }, + MatchIdx: 0, + RuleIdx: 0, + Source: hr2, }, }, }, }, }, { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route2"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ + Hostname: "foo.example.com", + PathRules: []PathRule{ + { + Path: "/", + MatchRules: []MatchRule{ { - ControllerName: gatewayCtlrName, - ParentRef: v1alpha2.ParentRef{ - Name: "fake", - }, - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: hr2.Generation, - LastTransitionTime: metav1.NewTime(constTime), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", - }, - }, + MatchIdx: 0, + RuleIdx: 0, + Source: hr1, }, }, }, }, }, - } - - changes, statusUpdates := conf.UpsertHTTPRoute(hr2) - Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) - }) - - It("should upsert the host and generate status updates for both HTTPRoutes after updating the second", func() { - expectedChanges := []state.Change{ - { - Op: state.Upsert, - Host: state.Host{ - Value: "cafe.example.com", - PathRouteGroups: []state.PathRouteGroup{ - { - Path: "/", - Routes: []state.Route{ - { - MatchIdx: -1, - RuleIdx: 0, - Source: hr1, - }, - }, - }, - { - Path: "/tea", - Routes: []state.Route{ - { - MatchIdx: 0, - RuleIdx: 0, - Source: hr2Updated, - }, - }, - }, - }, + }, + }, + msg: "one listener with two routes for different hostnames", + }, + { + graph: &graph{ + Listeners: map[string]*listener{ + "listener-80-1": { + Valid: true, + Routes: map[types.NamespacedName]*route{ + {Namespace: "test", Name: "hr-3"}: routeHR3, + {Namespace: "test", Name: "hr-4"}: routeHR4, }, - }, - } - expectedStatusUpdates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ - { - ControllerName: gatewayCtlrName, - ParentRef: v1alpha2.ParentRef{ - Name: "fake", - }, - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: hr1.Generation, - LastTransitionTime: metav1.NewTime(constTime), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", - }, - }, - }, - }, - }, + AcceptedHostnames: map[string]struct{}{ + "foo.example.com": {}, }, }, + }, + Routes: map[types.NamespacedName]*route{ + {Namespace: "test", Name: "hr-3"}: routeHR3, + {Namespace: "test", Name: "hr-4"}: routeHR4, + }, + }, + expected: Configuration{ + HTTPServers: []HTTPServer{ { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route2"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ + Hostname: "foo.example.com", + PathRules: []PathRule{ + { + Path: "/", + MatchRules: []MatchRule{ { - ControllerName: gatewayCtlrName, - ParentRef: v1alpha2.ParentRef{ - Name: "fake", - }, - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: hr2Updated.Generation, - LastTransitionTime: metav1.NewTime(constTime), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", - }, - }, - }, - }, - }, - }, - }, - } - - changes, statusUpdates := conf.UpsertHTTPRoute(hr2Updated) - Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) - }) - - It("should upsert the host and generate a status updates for the first HTTPRoute after deleting the second", func() { - expectedChanges := []state.Change{ - { - Op: state.Upsert, - Host: state.Host{ - Value: "cafe.example.com", - PathRouteGroups: []state.PathRouteGroup{ - { - Path: "/", - Routes: []state.Route{ - { - MatchIdx: -1, - RuleIdx: 0, - Source: hr1, - }, + MatchIdx: 0, + RuleIdx: 0, + Source: hr3, }, - }, - }, - }, - }, - } - expectedStatusUpdates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ { - ControllerName: gatewayCtlrName, - ParentRef: v1alpha2.ParentRef{ - Name: "fake", - }, - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: hr1.Generation, - LastTransitionTime: metav1.NewTime(constTime), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", - }, - }, - }, - }, - }, - }, - }, - } - - changes, statusUpdates := conf.DeleteHTTPRoute(types.NamespacedName{Namespace: "test", Name: "route2"}) - Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) - }) - - It("should delete the host after deleting the first HTTPRoute", func() { - expectedChanges := []state.Change{ - { - Op: state.Delete, - Host: state.Host{ - Value: "cafe.example.com", - PathRouteGroups: []state.PathRouteGroup{ - { - Path: "/", - Routes: []state.Route{ - { - MatchIdx: -1, - RuleIdx: 0, - Source: hr1, - }, + MatchIdx: 0, + RuleIdx: 1, + Source: hr4, }, }, }, - }, - }, - } - changes, statusUpdates := conf.DeleteHTTPRoute(types.NamespacedName{Namespace: "test", Name: "route1"}) - Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) - Expect(statusUpdates).To(BeEmpty()) - }) - }) - - Describe("Processing conflicting HTTPRoutes", Ordered, func() { - var earlier, later metav1.Time - var hr1, hr2, hr1Updated *v1alpha2.HTTPRoute - - BeforeAll(func() { - earlier = metav1.Now() - later = metav1.NewTime(earlier.Add(1 * time.Second)) - - hr1 = &v1alpha2.HTTPRoute{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "route1", - CreationTimestamp: earlier, - }, - Spec: v1alpha2.HTTPRouteSpec{ - Hostnames: []v1alpha2.Hostname{ - "cafe.example.com", - }, - Rules: []v1alpha2.HTTPRouteRule{ - { - // mo matches -> "/" - }, - }, - }, - } - - hr2 = &v1alpha2.HTTPRoute{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "route2", - CreationTimestamp: earlier, - }, - Spec: v1alpha2.HTTPRouteSpec{ - Hostnames: []v1alpha2.Hostname{ - "cafe.example.com", - }, - Rules: []v1alpha2.HTTPRouteRule{ { - Matches: []v1alpha2.HTTPRouteMatch{ + Path: "/fourth", + MatchRules: []MatchRule{ { - Path: &v1alpha2.HTTPPathMatch{ - Value: helpers.GetStringPointer("/"), - }, - }, - }, - }, - }, - }, - } - - hr1Updated = hr1.DeepCopy() - hr1Updated.Generation++ - hr1Updated.CreationTimestamp = later - }) - - It("should upsert a host and generate a status update for the first HTTPRoute", func() { - expectedChanges := []state.Change{ - { - Op: state.Upsert, - Host: state.Host{ - Value: "cafe.example.com", - PathRouteGroups: []state.PathRouteGroup{ - { - Path: "/", - Routes: []state.Route{ - { - MatchIdx: -1, - RuleIdx: 0, - Source: hr1, - }, + MatchIdx: 0, + RuleIdx: 0, + Source: hr4, }, }, }, - }, - }, - } - expectedStatusUpdates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ + { + Path: "/third", + MatchRules: []MatchRule{ { - ControllerName: gatewayCtlrName, - ParentRef: v1alpha2.ParentRef{ - Name: "fake", - }, - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: hr1.Generation, - LastTransitionTime: metav1.NewTime(constTime), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", - }, - }, + MatchIdx: 0, + RuleIdx: 1, + Source: hr3, }, }, }, }, }, - } + }, + }, + msg: "one listener with two routes with the same hostname with and without collisions", + }, + } - changes, statusUpdates := conf.UpsertHTTPRoute(hr1) - Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) - }) + for _, test := range tests { + result := buildConfiguration(test.graph) + if diff := cmp.Diff(test.expected, result); diff != "" { + t.Errorf("buildConfiguration() %q mismatch (-want +got):\n%s", test.msg, diff) + } + } +} - It("should upsert the host (make the first HTTPRoute the winner for '/' rule) and generate status updates for both HTTPRoutes after adding the second", func() { - expectedChanges := []state.Change{ - { - Op: state.Upsert, - Host: state.Host{ - Value: "cafe.example.com", - PathRouteGroups: []state.PathRouteGroup{ - { - Path: "/", - Routes: []state.Route{ - { - MatchIdx: -1, - RuleIdx: 0, - Source: hr1, - }, - { - MatchIdx: 0, - RuleIdx: 0, - Source: hr2, - }, - }, - }, - }, - }, - }, - } - expectedStatusUpdates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ - { - ControllerName: gatewayCtlrName, - ParentRef: v1alpha2.ParentRef{ - Name: "fake", - }, - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: hr1.Generation, - LastTransitionTime: metav1.NewTime(constTime), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", - }, - }, - }, - }, - }, - }, - }, - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route2"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ - { - ControllerName: gatewayCtlrName, - ParentRef: v1alpha2.ParentRef{ - Name: "fake", - }, - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: hr2.Generation, - LastTransitionTime: metav1.NewTime(constTime), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", - }, - }, - }, - }, - }, - }, - }, - } +func TestLessObjectMeta(t *testing.T) { + sooner := metav1.Now() + later := metav1.NewTime(sooner.Add(10 * time.Millisecond)) - changes, statusUpdates := conf.UpsertHTTPRoute(hr2) - Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) - }) + tests := []struct { + meta1, meta2 *metav1.ObjectMeta + expected bool + msg string + }{ + { + meta1: &metav1.ObjectMeta{ + CreationTimestamp: sooner, + Namespace: "test", + Name: "myname", + }, + meta2: &metav1.ObjectMeta{ + CreationTimestamp: sooner, + Namespace: "test", + Name: "myname", + }, + expected: false, + msg: "equal", + }, + { + meta1: &metav1.ObjectMeta{ + CreationTimestamp: sooner, + Namespace: "test", + Name: "myname", + }, + meta2: &metav1.ObjectMeta{ + CreationTimestamp: later, + Namespace: "test", + Name: "myname", + }, + expected: true, + msg: "less by timestamp", + }, + { + meta1: &metav1.ObjectMeta{ + CreationTimestamp: later, + Namespace: "test", + Name: "myname", + }, + meta2: &metav1.ObjectMeta{ + CreationTimestamp: sooner, + Namespace: "test", + Name: "myname", + }, + expected: false, + msg: "greater by timestamp", + }, + { + meta1: &metav1.ObjectMeta{ + CreationTimestamp: sooner, + Namespace: "atest", + Name: "myname", + }, + meta2: &metav1.ObjectMeta{ + CreationTimestamp: sooner, + Namespace: "test", + Name: "myname", + }, + expected: true, + msg: "less by namespace", + }, + { + meta1: &metav1.ObjectMeta{ + CreationTimestamp: sooner, + Namespace: "test", + Name: "myname", + }, + meta2: &metav1.ObjectMeta{ + CreationTimestamp: sooner, + Namespace: "atest", + Name: "myname", + }, + expected: false, + msg: "greater by namespace", + }, + { + meta1: &metav1.ObjectMeta{ + CreationTimestamp: sooner, + Namespace: "test", + Name: "amyname", + }, + meta2: &metav1.ObjectMeta{ + CreationTimestamp: sooner, + Namespace: "test", + Name: "myname", + }, + expected: true, + msg: "less by name", + }, + { + meta1: &metav1.ObjectMeta{ + CreationTimestamp: sooner, + Namespace: "test", + Name: "myname", + }, + meta2: &metav1.ObjectMeta{ + CreationTimestamp: sooner, + Namespace: "test", + Name: "amyname", + }, + expected: false, + msg: "greater by name", + }, + } - It("should upsert the host (make the second HTTPRoute the winner for '/' rule) and generate status updates for both HTTPRoutes after updating the first", func() { - expectedChanges := []state.Change{ - { - Op: state.Upsert, - Host: state.Host{ - Value: "cafe.example.com", - PathRouteGroups: []state.PathRouteGroup{ - { - Path: "/", - Routes: []state.Route{ - { - MatchIdx: 0, - RuleIdx: 0, - Source: hr2, - }, - { - MatchIdx: -1, - RuleIdx: 0, - Source: hr1Updated, - }, - }, - }, - }, - }, - }, - } - expectedStatusUpdates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ - { - ControllerName: gatewayCtlrName, - ParentRef: v1alpha2.ParentRef{ - Name: "fake", - }, - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: hr1Updated.Generation, - LastTransitionTime: metav1.NewTime(constTime), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", - }, - }, - }, - }, - }, - }, - }, - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route2"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ - { - ControllerName: gatewayCtlrName, - ParentRef: v1alpha2.ParentRef{ - Name: "fake", - }, - Conditions: []metav1.Condition{ - { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: hr2.Generation, - LastTransitionTime: metav1.NewTime(constTime), - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", - }, - }, - }, - }, - }, - }, - }, - } + for _, test := range tests { + result := lessObjectMeta(test.meta1, test.meta2) + if result != test.expected { + t.Errorf("lessObjectMeta() returned %v but expected %v for the case of %q", result, test.expected, test.msg) + } + } +} - changes, statusUpdates := conf.UpsertHTTPRoute(hr1Updated) - Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) - }) - }) - }) -}) +func TestGetPath(t *testing.T) { + tests := []struct { + path *v1alpha2.HTTPPathMatch + expected string + msg string + }{ + { + path: &v1alpha2.HTTPPathMatch{Value: helpers.GetStringPointer("/abc")}, + expected: "/abc", + msg: "normal case", + }, + { + path: nil, + expected: "/", + msg: "nil path", + }, + { + path: &v1alpha2.HTTPPathMatch{Value: nil}, + expected: "/", + msg: "nil value", + }, + { + path: &v1alpha2.HTTPPathMatch{Value: helpers.GetStringPointer("")}, + expected: "/", + msg: "empty value", + }, + } + + for _, test := range tests { + result := getPath(test.path) + if result != test.expected { + t.Errorf("getPath() returned %q but expected %q for the case of %q", result, test.expected, test.msg) + } + } +} -func TestRouteGetMatch(t *testing.T) { +func TestMatchRuleGetMatch(t *testing.T) { var hr = &v1alpha2.HTTPRoute{ Spec: v1alpha2.HTTPRouteSpec{ Rules: []v1alpha2.HTTPRouteRule{ @@ -922,47 +443,47 @@ func TestRouteGetMatch(t *testing.T) { tests := []struct { name, expPath string - route state.Route + rule MatchRule matchExists bool }{ { name: "match does not exist", expPath: "", - route: state.Route{MatchIdx: -1}, + rule: MatchRule{MatchIdx: -1}, matchExists: false, }, { name: "first match in first rule", expPath: "/path-1", - route: state.Route{MatchIdx: 0, RuleIdx: 0, Source: hr}, + rule: MatchRule{MatchIdx: 0, RuleIdx: 0, Source: hr}, matchExists: true, }, { name: "second match in first rule", expPath: "/path-2", - route: state.Route{MatchIdx: 1, RuleIdx: 0, Source: hr}, + rule: MatchRule{MatchIdx: 1, RuleIdx: 0, Source: hr}, matchExists: true, }, { name: "second match in second rule", expPath: "/path-4", - route: state.Route{MatchIdx: 1, RuleIdx: 1, Source: hr}, + rule: MatchRule{MatchIdx: 1, RuleIdx: 1, Source: hr}, matchExists: true, }, } for _, tc := range tests { - actual, exists := tc.route.GetMatch() + actual, exists := tc.rule.GetMatch() if !tc.matchExists { if exists { - t.Errorf("route.GetMatch() incorrectly returned true (match exists) for test case: %q", tc.name) + t.Errorf("rule.GetMatch() incorrectly returned true (match exists) for test case: %q", tc.name) } } else { if !exists { - t.Errorf("route.GetMatch() incorrectly returned false (match does not exist) for test case: %q", tc.name) + t.Errorf("rule.GetMatch() incorrectly returned false (match does not exist) for test case: %q", tc.name) } if *actual.Path.Value != tc.expPath { - t.Errorf("route.GetMatch() returned incorrect match with path: %s, expected path: %s for test case: %q", *actual.Path.Value, tc.expPath, tc.name) + t.Errorf("rule.GetMatch() returned incorrect match with path: %s, expected path: %s for test case: %q", *actual.Path.Value, tc.expPath, tc.name) } } } diff --git a/internal/state/graph.go b/internal/state/graph.go new file mode 100644 index 0000000000..26f59f1eae --- /dev/null +++ b/internal/state/graph.go @@ -0,0 +1,225 @@ +package state + +import ( + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/gateway-api/apis/v1alpha2" +) + +// listener represents a listener of the Gateway resource. +// FIXME(pleshakov) For now, we only support HTTP listeners. +type listener struct { + // Source holds the source of the listener from the Gateway resource. + Source v1alpha2.Listener + // Valid shows whether the listener is valid. + // FIXME(pleshakov) For now, only capture true/false without any error message. + Valid bool + // Routes holds the routes attached to the listener. + Routes map[types.NamespacedName]*route + // AcceptedHostnames is an intersection between the hostnames supported by the listener and the hostnames + // from the attached routes. + AcceptedHostnames map[string]struct{} +} + +// route represents an HTTPRoute. +type route struct { + // Source is the source resource of the route. + // FIXME(pleshakov) + // For now, we assume that the source is only HTTPRoute. Later we can support more types - TLSRoute, TCPRoute and UDPRoute. + Source *v1alpha2.HTTPRoute + + // ValidSectionNameRefs includes the sectionNames from the parentRefs of the HTTPRoute that are valid -- i.e. + // the Gateway resource has a corresponding valid listener. + ValidSectionNameRefs map[string]struct{} + // ValidSectionNameRefs includes the sectionNames from the parentRefs of the HTTPRoute that are invalid. + InvalidSectionNameRefs map[string]struct{} +} + +// graph is a graph-like representation of Gateway API resources. +// It is assumed that the Gateway resource is a single resource. +type graph struct { + // Listeners holds listeners keyed by their names in the Gateway resource. + Listeners map[string]*listener + // Routes holds route resources. + Routes map[types.NamespacedName]*route +} + +// buildGraph builds a graph from a store assuming that the Gateway resource has the gwNsName namespace and name. +func buildGraph(store *store, gwNsName types.NamespacedName) *graph { + listeners := buildListeners(store.gw) + + routes := make(map[types.NamespacedName]*route) + for _, ghr := range store.httpRoutes { + ignored, r := bindHTTPRouteToListeners(ghr, gwNsName, listeners) + if !ignored { + routes[getNamespacedName(ghr)] = r + } + } + + return &graph{ + Listeners: listeners, + Routes: routes, + } +} + +// bindHTTPRouteToListeners tries to bind an HTTPRoute to listener. +// There are three possibilities: +// (1) HTTPRoute will be ignored. +// (2) HTTPRoute will be processed but not bound. +// (3) HTTPRoute will be processed and bound to a listener. +func bindHTTPRouteToListeners( + ghr *v1alpha2.HTTPRoute, + gwNsName types.NamespacedName, + listeners map[string]*listener, +) (ignored bool, r *route) { + if len(ghr.Spec.ParentRefs) == 0 { + // ignore HTTPRoute without refs + return true, nil + } + + r = &route{ + Source: ghr, + ValidSectionNameRefs: make(map[string]struct{}), + InvalidSectionNameRefs: make(map[string]struct{}), + } + + // FIXME (pleshakov) Handle the case when parent refs are duplicated + + ignored = true + + for _, p := range ghr.Spec.ParentRefs { + // Step 1 - Ensure the ref references the Gateway and has a non-empty section name + + if ignoreParentRef(p, ghr.Namespace, gwNsName) { + continue + } + + ignored = false + + name := string(*p.SectionName) + + // Step 2 - Find a listener + + // FIXME(pleshakov) + // For now, let's do simple matching. + // However, we need to also support wildcard matching. + // More over, we need to handle cases when a Route host matches multiple HTTP listeners on the same port when + // sectionName is empty and only choose one listener. + // For example: + // - Route with host foo.example.com; + // - listener 1 for port 80 with hostname foo.example.com + // - listener 2 for port 80 with hostname *.example.com; + // In this case, the Route host foo.example.com should choose listener 2, as it is a more specific match. + + l, exists := listeners[name] + if !exists { + r.InvalidSectionNameRefs[name] = struct{}{} + continue + } + + accepted := findAcceptedHostnames(l.Source.Hostname, ghr.Spec.Hostnames) + + if len(accepted) > 0 { + for _, h := range accepted { + l.AcceptedHostnames[h] = struct{}{} + } + r.ValidSectionNameRefs[name] = struct{}{} + l.Routes[getNamespacedName(ghr)] = r + } else { + r.InvalidSectionNameRefs[name] = struct{}{} + } + } + + if ignored { + return true, nil + } + + return false, r +} + +func findAcceptedHostnames(listenerHostname *v1alpha2.Hostname, routeHostnames []v1alpha2.Hostname) []string { + hostname := getHostname(listenerHostname) + + match := func(h v1alpha2.Hostname) bool { + if hostname == "" { + return true + } + return string(h) == hostname + } + + var result []string + + for _, h := range routeHostnames { + if match(h) { + result = append(result, string(h)) + } + } + + return result +} + +func ignoreParentRef(p v1alpha2.ParentRef, hrNamespace string, gwNsName types.NamespacedName) bool { + ns := hrNamespace + if p.Namespace != nil { + ns = string(*p.Namespace) + } + + // FIXME(pleshakov) Also check for Kind and APIGroup + if ns != gwNsName.Namespace || string(p.Name) != gwNsName.Name { + return true + } + + // FIXME(pleshakov) Support empty section name + if p.SectionName == nil || *p.SectionName == "" { + return true + } + + return false +} + +func buildListeners(gw *v1alpha2.Gateway) map[string]*listener { + // FIXME(pleshakov): For now we require that all HTTP listeners bind to port 80 + + listeners := make(map[string]*listener) + + if gw == nil { + return listeners + } + + usedListenerHostnames := make(map[string]*listener) + + for _, gl := range gw.Spec.Listeners { + valid := validateListener(gl) + + h := getHostname(gl.Hostname) + + // FIXME(pleshakov) This check will need to be done per each port once we support multiple ports. + if holder, exist := usedListenerHostnames[h]; exist { + valid = false + holder.Valid = false // all listeners for the same hostname become conflicted + } + + l := &listener{ + Source: gl, + Valid: valid, + Routes: make(map[types.NamespacedName]*route), + AcceptedHostnames: make(map[string]struct{}), + } + + listeners[string(gl.Name)] = l + usedListenerHostnames[h] = l + } + + return listeners +} + +func validateListener(listener v1alpha2.Listener) bool { + // FIXME(pleshakov) For now, only support HTTP on port 80. + return listener.Protocol == v1alpha2.HTTPProtocolType && listener.Port == 80 +} + +func getHostname(h *v1alpha2.Hostname) string { + if h == nil { + return "" + } + return string(*h) +} diff --git a/internal/state/graph_test.go b/internal/state/graph_test.go new file mode 100644 index 0000000000..c353772371 --- /dev/null +++ b/internal/state/graph_test.go @@ -0,0 +1,615 @@ +package state + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" +) + +func TestBuildGraph(t *testing.T) { + createRoute := func(name string, gatewayName string) *v1alpha2.HTTPRoute { + return &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: name, + }, + Spec: v1alpha2.HTTPRouteSpec{ + CommonRouteSpec: v1alpha2.CommonRouteSpec{ + ParentRefs: []v1alpha2.ParentRef{ + { + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: v1alpha2.ObjectName(gatewayName), + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("listener-80-1")), + }, + }, + }, + Hostnames: []v1alpha2.Hostname{ + v1alpha2.Hostname("foo.example.com"), + }, + Rules: []v1alpha2.HTTPRouteRule{ + { + Matches: []v1alpha2.HTTPRouteMatch{ + { + Path: &v1alpha2.HTTPPathMatch{ + Value: helpers.GetStringPointer("/"), + }, + }, + }, + }, + }, + }, + } + } + hr1 := createRoute("hr-1", "gateway") + hr2 := createRoute("hr-2", "wrong-gateway") + + store := &store{ + gw: &v1alpha2.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "gateway", + }, + Spec: v1alpha2.GatewaySpec{ + Listeners: []v1alpha2.Listener{ + { + Name: "listener-80-1", + Hostname: nil, + Port: 80, + Protocol: v1alpha2.HTTPProtocolType, + }, + }, + }, + }, + httpRoutes: map[types.NamespacedName]*v1alpha2.HTTPRoute{ + {Namespace: "test", Name: "hr-1"}: hr1, + {Namespace: "test", Name: "hr-2"}: hr2, + }, + } + + gwNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} + + routeHR1 := &route{ + Source: hr1, + ValidSectionNameRefs: map[string]struct{}{ + "listener-80-1": {}, + }, + InvalidSectionNameRefs: map[string]struct{}{}, + } + expected := &graph{ + Listeners: map[string]*listener{ + "listener-80-1": { + Source: store.gw.Spec.Listeners[0], + Valid: true, + Routes: map[types.NamespacedName]*route{ + {Namespace: "test", Name: "hr-1"}: routeHR1, + }, + AcceptedHostnames: map[string]struct{}{ + "foo.example.com": {}, + }, + }, + }, + Routes: map[types.NamespacedName]*route{ + {Namespace: "test", Name: "hr-1"}: routeHR1, + }, + } + + result := buildGraph(store, gwNsName) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("buildGraph() mismatch (-want +got):\n%s", diff) + } +} + +func TestBuildListeners(t *testing.T) { + listener801 := v1alpha2.Listener{ + Name: "listener-80-1", + Hostname: (*v1alpha2.Hostname)(helpers.GetStringPointer("foo.example.com")), + Port: 80, + Protocol: v1alpha2.HTTPProtocolType, + } + listener802 := v1alpha2.Listener{ + Name: "listener-80-2", + Hostname: (*v1alpha2.Hostname)(helpers.GetStringPointer("bar.example.com")), + Port: 80, + Protocol: v1alpha2.TCPProtocolType, // invalid protocol + } + listener803 := v1alpha2.Listener{ + Name: "listener-80-3", + Hostname: (*v1alpha2.Hostname)(helpers.GetStringPointer("bar.example.com")), + Port: 80, + Protocol: v1alpha2.HTTPProtocolType, + } + listener804 := v1alpha2.Listener{ + Name: "listener-80-4", + Hostname: (*v1alpha2.Hostname)(helpers.GetStringPointer("foo.example.com")), + Port: 80, + Protocol: v1alpha2.HTTPProtocolType, + } + + tests := []struct { + gateway *v1alpha2.Gateway + expected map[string]*listener + msg string + }{ + { + gateway: &v1alpha2.Gateway{ + Spec: v1alpha2.GatewaySpec{ + Listeners: []v1alpha2.Listener{ + listener801, + }, + }, + }, + expected: map[string]*listener{ + "listener-80-1": { + Source: listener801, + Valid: true, + Routes: map[types.NamespacedName]*route{}, + AcceptedHostnames: map[string]struct{}{}, + }, + }, + msg: "valid listener", + }, + { + gateway: &v1alpha2.Gateway{ + Spec: v1alpha2.GatewaySpec{ + Listeners: []v1alpha2.Listener{ + listener802, + }, + }, + }, + expected: map[string]*listener{ + "listener-80-2": { + Source: listener802, + Valid: false, + Routes: map[types.NamespacedName]*route{}, + AcceptedHostnames: map[string]struct{}{}, + }, + }, + msg: "invalid listener", + }, + { + gateway: &v1alpha2.Gateway{ + Spec: v1alpha2.GatewaySpec{ + Listeners: []v1alpha2.Listener{ + listener801, listener803, + }, + }, + }, + expected: map[string]*listener{ + "listener-80-1": { + Source: listener801, + Valid: true, + Routes: map[types.NamespacedName]*route{}, + AcceptedHostnames: map[string]struct{}{}, + }, + "listener-80-3": { + Source: listener803, + Valid: true, + Routes: map[types.NamespacedName]*route{}, + AcceptedHostnames: map[string]struct{}{}, + }, + }, + msg: "two valid Listeners", + }, + { + gateway: &v1alpha2.Gateway{ + Spec: v1alpha2.GatewaySpec{ + Listeners: []v1alpha2.Listener{ + listener801, listener804, + }, + }, + }, + expected: map[string]*listener{ + "listener-80-1": { + Source: listener801, + Valid: false, + Routes: map[types.NamespacedName]*route{}, + AcceptedHostnames: map[string]struct{}{}, + }, + "listener-80-4": { + Source: listener804, + Valid: false, + Routes: map[types.NamespacedName]*route{}, + AcceptedHostnames: map[string]struct{}{}, + }, + }, + msg: "collision", + }, + { + gateway: nil, + expected: map[string]*listener{}, + msg: "no gateway", + }, + } + + for _, test := range tests { + result := buildListeners(test.gateway) + if diff := cmp.Diff(test.expected, result); diff != "" { + t.Errorf("buildListeners() %q mismatch (-want +got):\n%s", test.msg, diff) + } + } +} + +func TestBindRouteToListeners(t *testing.T) { + createRoute := func(hostname string, parentRefs ...v1alpha2.ParentRef) *v1alpha2.HTTPRoute { + return &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "hr-1", + }, + Spec: v1alpha2.HTTPRouteSpec{ + CommonRouteSpec: v1alpha2.CommonRouteSpec{ + ParentRefs: parentRefs, + }, + Hostnames: []v1alpha2.Hostname{ + v1alpha2.Hostname(hostname), + }, + }, + } + } + + hrNonExistingSectionName := createRoute("foo.example.com", v1alpha2.ParentRef{ + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: "gateway", + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("listener-80-2")), + }) + + hrFoo := createRoute("foo.example.com", v1alpha2.ParentRef{ + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: "gateway", + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("listener-80-1")), + }) + + hrBar := createRoute("bar.example.com", v1alpha2.ParentRef{ + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: "gateway", + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("listener-80-1")), + }) + + // we create a new listener each time because the function under test can modify it + createListener := func() *listener { + return &listener{ + Source: v1alpha2.Listener{ + Hostname: (*v1alpha2.Hostname)(helpers.GetStringPointer("foo.example.com")), + }, + Valid: true, + Routes: map[types.NamespacedName]*route{}, + AcceptedHostnames: map[string]struct{}{}, + } + } + + createModifiedListener := func(m func(*listener)) *listener { + l := createListener() + m(l) + return l + } + + tests := []struct { + httpRoute *v1alpha2.HTTPRoute + gwNsName types.NamespacedName + listeners map[string]*listener + expectedIgnored bool + expectedRoute *route + expectedListeners map[string]*listener + msg string + }{ + { + httpRoute: createRoute("foo.example.com"), + gwNsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, + listeners: map[string]*listener{ + "listener-80-1": createListener(), + }, + expectedIgnored: true, + expectedRoute: nil, + expectedListeners: map[string]*listener{ + "listener-80-1": createListener(), + }, + msg: "HTTPRoute without parent refs", + }, + { + httpRoute: createRoute("foo.example.com", v1alpha2.ParentRef{ + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: "some-gateway", // wrong gateway + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("listener-1")), + }), + gwNsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, + listeners: map[string]*listener{ + "listener-80-1": createListener(), + }, + expectedIgnored: true, + expectedRoute: nil, + expectedListeners: map[string]*listener{ + "listener-80-1": createListener(), + }, + msg: "HTTPRoute without good parent refs", + }, + { + httpRoute: hrNonExistingSectionName, + gwNsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, + listeners: map[string]*listener{ + "listener-80-1": createListener(), + }, + expectedIgnored: false, + expectedRoute: &route{ + Source: hrNonExistingSectionName, + ValidSectionNameRefs: map[string]struct{}{}, + InvalidSectionNameRefs: map[string]struct{}{ + "listener-80-2": {}, + }, + }, + expectedListeners: map[string]*listener{ + "listener-80-1": createListener(), + }, + msg: "HTTPRoute with non-existing section name", + }, + { + httpRoute: hrFoo, + gwNsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, + listeners: map[string]*listener{ + "listener-80-1": createListener(), + }, + expectedIgnored: false, + expectedRoute: &route{ + Source: hrFoo, + ValidSectionNameRefs: map[string]struct{}{ + "listener-80-1": {}, + }, + InvalidSectionNameRefs: map[string]struct{}{}, + }, + expectedListeners: map[string]*listener{ + "listener-80-1": createModifiedListener(func(l *listener) { + l.Routes = map[types.NamespacedName]*route{ + {Namespace: "test", Name: "hr-1"}: { + Source: hrFoo, + ValidSectionNameRefs: map[string]struct{}{ + "listener-80-1": {}, + }, + InvalidSectionNameRefs: map[string]struct{}{}, + }, + } + l.AcceptedHostnames = map[string]struct{}{ + "foo.example.com": {}, + } + }), + }, + msg: "HTTPRoute with one accepted hostname", + }, + { + httpRoute: hrBar, + gwNsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, + listeners: map[string]*listener{ + "listener-80-1": createListener(), + }, + expectedIgnored: false, + expectedRoute: &route{ + Source: hrBar, + ValidSectionNameRefs: map[string]struct{}{}, + InvalidSectionNameRefs: map[string]struct{}{ + "listener-80-1": {}, + }, + }, + expectedListeners: map[string]*listener{ + "listener-80-1": createListener(), + }, + msg: "HTTPRoute with zero accepted hostnames", + }, + } + + for _, test := range tests { + ignored, route := bindHTTPRouteToListeners(test.httpRoute, test.gwNsName, test.listeners) + if diff := cmp.Diff(test.expectedIgnored, ignored); diff != "" { + t.Errorf("bindHTTPRouteToListeners() %q mismatch on ignored (-want +got):\n%s", test.msg, diff) + } + if diff := cmp.Diff(test.expectedRoute, route); diff != "" { + t.Errorf("bindHTTPRouteToListeners() %q mismatch on route (-want +got):\n%s", test.msg, diff) + } + if diff := cmp.Diff(test.expectedListeners, test.listeners); diff != "" { + t.Errorf("bindHTTPRouteToListeners() %q mismatch on listeners (-want +got):\n%s", test.msg, diff) + } + } +} + +func TestFindAcceptedHostnames(t *testing.T) { + var listenerHostnameFoo v1alpha2.Hostname = "foo.example.com" + var listenerHostnameCafe v1alpha2.Hostname = "cafe.example.com" + routeHostnames := []v1alpha2.Hostname{"foo.example.com", "bar.example.com"} + + tests := []struct { + listenerHostname *v1alpha2.Hostname + routeHostnames []v1alpha2.Hostname + expected []string + msg string + }{ + { + listenerHostname: &listenerHostnameFoo, + routeHostnames: routeHostnames, + expected: []string{"foo.example.com"}, + msg: "one match", + }, + { + listenerHostname: &listenerHostnameCafe, + routeHostnames: routeHostnames, + expected: nil, + msg: "no match", + }, + { + listenerHostname: nil, + routeHostnames: routeHostnames, + expected: []string{"foo.example.com", "bar.example.com"}, + msg: "nil listener hostname", + }, + } + + for _, test := range tests { + result := findAcceptedHostnames(test.listenerHostname, test.routeHostnames) + if diff := cmp.Diff(test.expected, result); diff != "" { + t.Errorf("findAcceptedHostnames() %q mismatch (-want +got):\n%s", test.msg, diff) + } + } + +} + +func TestIgnoreParentRef(t *testing.T) { + var gatewayNs v1alpha2.Namespace = "test" + var sectionName v1alpha2.SectionName = "first" + var emptySectionName v1alpha2.SectionName + + tests := []struct { + parentRef v1alpha2.ParentRef + hrNamespace string + gwNsName types.NamespacedName + expected bool + msg string + }{ + { + parentRef: v1alpha2.ParentRef{ + Namespace: &gatewayNs, + Name: "gateway", + SectionName: §ionName, + }, + hrNamespace: "test2", + gwNsName: types.NamespacedName{Namespace: string(gatewayNs), Name: "gateway"}, + expected: false, + msg: "normal case", + }, + { + parentRef: v1alpha2.ParentRef{ + Name: "gateway", + SectionName: §ionName, + }, + hrNamespace: "test", + gwNsName: types.NamespacedName{Namespace: string(gatewayNs), Name: "gateway"}, + expected: false, + msg: "normal case with implicit namespace", + }, + { + parentRef: v1alpha2.ParentRef{ + Namespace: &gatewayNs, + Name: "gateway", + SectionName: §ionName, + }, + hrNamespace: "test2", + gwNsName: types.NamespacedName{Namespace: "test3", Name: "gateway"}, + expected: true, + msg: "gateway namespace is different", + }, + { + parentRef: v1alpha2.ParentRef{ + Namespace: &gatewayNs, + Name: "gateway", + SectionName: §ionName, + }, + hrNamespace: "test2", + gwNsName: types.NamespacedName{Namespace: string(gatewayNs), Name: "gateway2"}, + expected: true, + msg: "gateway name is different", + }, + { + parentRef: v1alpha2.ParentRef{ + Namespace: &gatewayNs, + Name: "gateway", + SectionName: nil, + }, + hrNamespace: "test2", + gwNsName: types.NamespacedName{Namespace: string(gatewayNs), Name: "gateway"}, + expected: true, + msg: "section name is nil", + }, + { + parentRef: v1alpha2.ParentRef{ + Namespace: &gatewayNs, + Name: "gateway", + SectionName: &emptySectionName, + }, + hrNamespace: "test2", + gwNsName: types.NamespacedName{Namespace: string(gatewayNs), Name: "gateway"}, + expected: true, + msg: "section name is empty", + }, + } + + for _, test := range tests { + result := ignoreParentRef(test.parentRef, test.hrNamespace, test.gwNsName) + if result != test.expected { + t.Errorf("ignoreParentRef() returned %v but expected %v for the case of %q", result, test.expected, test.msg) + } + } +} + +func TestValidateListener(t *testing.T) { + tests := []struct { + l v1alpha2.Listener + expected bool + msg string + }{ + { + l: v1alpha2.Listener{ + Port: 80, + Protocol: v1alpha2.HTTPProtocolType, + }, + expected: true, + msg: "valid", + }, + { + l: v1alpha2.Listener{ + Port: 81, + Protocol: v1alpha2.HTTPProtocolType, + }, + expected: false, + msg: "invalid port", + }, + { + l: v1alpha2.Listener{ + Port: 80, + Protocol: v1alpha2.TCPProtocolType, + }, + expected: false, + msg: "invalid protocol", + }, + } + + for _, test := range tests { + result := validateListener(test.l) + if result != test.expected { + t.Errorf("validateListener() returned %v but expected %v for the case of %q", result, test.expected, test.msg) + } + } +} + +func TestGetHostname(t *testing.T) { + var emptyHostname v1alpha2.Hostname + var hostname v1alpha2.Hostname = "example.com" + + tests := []struct { + h *v1alpha2.Hostname + expected string + msg string + }{ + { + h: nil, + expected: "", + msg: "nil hostname", + }, + { + h: &emptyHostname, + expected: "", + msg: "empty hostname", + }, + { + h: &hostname, + expected: string(hostname), + msg: "normal hostname", + }, + } + + for _, test := range tests { + result := getHostname(test.h) + if result != test.expected { + t.Errorf("getHostname() returned %q but expected %q for the case of %q", result, test.expected, test.msg) + } + } +} diff --git a/internal/state/helpers.go b/internal/state/helpers.go new file mode 100644 index 0000000000..5934fdcb0b --- /dev/null +++ b/internal/state/helpers.go @@ -0,0 +1,13 @@ +package state + +import ( + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func getNamespacedName(obj client.Object) types.NamespacedName { + return types.NamespacedName{ + Namespace: obj.GetNamespace(), + Name: obj.GetName(), + } +} diff --git a/internal/state/helpers_test.go b/internal/state/helpers_test.go new file mode 100644 index 0000000000..2cc2c7e6ec --- /dev/null +++ b/internal/state/helpers_test.go @@ -0,0 +1,20 @@ +package state + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/gateway-api/apis/v1alpha2" +) + +func TestGetNamespacedName(t *testing.T) { + obj := &v1alpha2.HTTPRoute{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: "hr-1"}} + + expected := types.NamespacedName{Namespace: "test", Name: "hr-1"} + + result := getNamespacedName(obj) + if result != expected { + t.Errorf("getNamespacedName() returned %#v but expected %#v", result, expected) + } +} diff --git a/internal/state/state_suit_test.go b/internal/state/newstate_suit_test.go similarity index 84% rename from internal/state/state_suit_test.go rename to internal/state/newstate_suit_test.go index 5be96add9e..d6836da7cc 100644 --- a/internal/state/state_suit_test.go +++ b/internal/state/newstate_suit_test.go @@ -1,13 +1,13 @@ package state_test import ( + "testing" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - - "testing" ) func TestState(t *testing.T) { RegisterFailHandler(Fail) - RunSpecs(t, "State Suite") + RunSpecs(t, "NewState Suite") } diff --git a/internal/state/services.go b/internal/state/services.go index 1fd66aaf41..1ddde72483 100644 --- a/internal/state/services.go +++ b/internal/state/services.go @@ -4,6 +4,7 @@ import ( "fmt" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ) @@ -52,3 +53,7 @@ func (s *serviceStoreImpl) Resolve(nsname types.NamespacedName) (string, error) return svc.Spec.ClusterIP, nil } + +func getResourceKey(meta *metav1.ObjectMeta) string { + return fmt.Sprintf("%s/%s", meta.Namespace, meta.Name) +} diff --git a/internal/state/sort.go b/internal/state/sort.go deleted file mode 100644 index c97fb6b070..0000000000 --- a/internal/state/sort.go +++ /dev/null @@ -1,34 +0,0 @@ -package state - -import ( - "sort" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -func sortRoutes(routes []Route) { - // stable sort is used so that the order of matches (as defined in each HTTPRoute rule) is preserved - // this is important, because the winning match is the first match to win. - sort.SliceStable(routes, func(i, j int) bool { - return lessObjectMeta(&routes[i].Source.ObjectMeta, &routes[j].Source.ObjectMeta) - }) -} - -func lessObjectMeta(meta1 *metav1.ObjectMeta, meta2 *metav1.ObjectMeta) bool { - if meta1.CreationTimestamp.Equal(&meta2.CreationTimestamp) { - return getResourceKey(meta1) < getResourceKey(meta2) - } - - return meta1.CreationTimestamp.Before(&meta2.CreationTimestamp) -} - -type mapper interface { - Keys() []string -} - -func getSortedKeys(m mapper) []string { - keys := m.Keys() - sort.Strings(keys) - - return keys -} diff --git a/internal/state/statefakes/fake_change_processor.go b/internal/state/statefakes/fake_change_processor.go new file mode 100644 index 0000000000..bd6151f11d --- /dev/null +++ b/internal/state/statefakes/fake_change_processor.go @@ -0,0 +1,194 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package statefakes + +import ( + "sync" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type FakeChangeProcessor struct { + CaptureDeleteChangeStub func(client.Object, types.NamespacedName) + captureDeleteChangeMutex sync.RWMutex + captureDeleteChangeArgsForCall []struct { + arg1 client.Object + arg2 types.NamespacedName + } + CaptureUpsertChangeStub func(client.Object) + captureUpsertChangeMutex sync.RWMutex + captureUpsertChangeArgsForCall []struct { + arg1 client.Object + } + ProcessStub func() (bool, state.Configuration, state.Statuses) + processMutex sync.RWMutex + processArgsForCall []struct { + } + processReturns struct { + result1 bool + result2 state.Configuration + result3 state.Statuses + } + processReturnsOnCall map[int]struct { + result1 bool + result2 state.Configuration + result3 state.Statuses + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeChangeProcessor) CaptureDeleteChange(arg1 client.Object, arg2 types.NamespacedName) { + fake.captureDeleteChangeMutex.Lock() + fake.captureDeleteChangeArgsForCall = append(fake.captureDeleteChangeArgsForCall, struct { + arg1 client.Object + arg2 types.NamespacedName + }{arg1, arg2}) + stub := fake.CaptureDeleteChangeStub + fake.recordInvocation("CaptureDeleteChange", []interface{}{arg1, arg2}) + fake.captureDeleteChangeMutex.Unlock() + if stub != nil { + fake.CaptureDeleteChangeStub(arg1, arg2) + } +} + +func (fake *FakeChangeProcessor) CaptureDeleteChangeCallCount() int { + fake.captureDeleteChangeMutex.RLock() + defer fake.captureDeleteChangeMutex.RUnlock() + return len(fake.captureDeleteChangeArgsForCall) +} + +func (fake *FakeChangeProcessor) CaptureDeleteChangeCalls(stub func(client.Object, types.NamespacedName)) { + fake.captureDeleteChangeMutex.Lock() + defer fake.captureDeleteChangeMutex.Unlock() + fake.CaptureDeleteChangeStub = stub +} + +func (fake *FakeChangeProcessor) CaptureDeleteChangeArgsForCall(i int) (client.Object, types.NamespacedName) { + fake.captureDeleteChangeMutex.RLock() + defer fake.captureDeleteChangeMutex.RUnlock() + argsForCall := fake.captureDeleteChangeArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeChangeProcessor) CaptureUpsertChange(arg1 client.Object) { + fake.captureUpsertChangeMutex.Lock() + fake.captureUpsertChangeArgsForCall = append(fake.captureUpsertChangeArgsForCall, struct { + arg1 client.Object + }{arg1}) + stub := fake.CaptureUpsertChangeStub + fake.recordInvocation("CaptureUpsertChange", []interface{}{arg1}) + fake.captureUpsertChangeMutex.Unlock() + if stub != nil { + fake.CaptureUpsertChangeStub(arg1) + } +} + +func (fake *FakeChangeProcessor) CaptureUpsertChangeCallCount() int { + fake.captureUpsertChangeMutex.RLock() + defer fake.captureUpsertChangeMutex.RUnlock() + return len(fake.captureUpsertChangeArgsForCall) +} + +func (fake *FakeChangeProcessor) CaptureUpsertChangeCalls(stub func(client.Object)) { + fake.captureUpsertChangeMutex.Lock() + defer fake.captureUpsertChangeMutex.Unlock() + fake.CaptureUpsertChangeStub = stub +} + +func (fake *FakeChangeProcessor) CaptureUpsertChangeArgsForCall(i int) client.Object { + fake.captureUpsertChangeMutex.RLock() + defer fake.captureUpsertChangeMutex.RUnlock() + argsForCall := fake.captureUpsertChangeArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeChangeProcessor) Process() (bool, state.Configuration, state.Statuses) { + fake.processMutex.Lock() + ret, specificReturn := fake.processReturnsOnCall[len(fake.processArgsForCall)] + fake.processArgsForCall = append(fake.processArgsForCall, struct { + }{}) + stub := fake.ProcessStub + fakeReturns := fake.processReturns + fake.recordInvocation("Process", []interface{}{}) + fake.processMutex.Unlock() + if stub != nil { + return stub() + } + if specificReturn { + return ret.result1, ret.result2, ret.result3 + } + return fakeReturns.result1, fakeReturns.result2, fakeReturns.result3 +} + +func (fake *FakeChangeProcessor) ProcessCallCount() int { + fake.processMutex.RLock() + defer fake.processMutex.RUnlock() + return len(fake.processArgsForCall) +} + +func (fake *FakeChangeProcessor) ProcessCalls(stub func() (bool, state.Configuration, state.Statuses)) { + fake.processMutex.Lock() + defer fake.processMutex.Unlock() + fake.ProcessStub = stub +} + +func (fake *FakeChangeProcessor) ProcessReturns(result1 bool, result2 state.Configuration, result3 state.Statuses) { + fake.processMutex.Lock() + defer fake.processMutex.Unlock() + fake.ProcessStub = nil + fake.processReturns = struct { + result1 bool + result2 state.Configuration + result3 state.Statuses + }{result1, result2, result3} +} + +func (fake *FakeChangeProcessor) ProcessReturnsOnCall(i int, result1 bool, result2 state.Configuration, result3 state.Statuses) { + fake.processMutex.Lock() + defer fake.processMutex.Unlock() + fake.ProcessStub = nil + if fake.processReturnsOnCall == nil { + fake.processReturnsOnCall = make(map[int]struct { + result1 bool + result2 state.Configuration + result3 state.Statuses + }) + } + fake.processReturnsOnCall[i] = struct { + result1 bool + result2 state.Configuration + result3 state.Statuses + }{result1, result2, result3} +} + +func (fake *FakeChangeProcessor) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.captureDeleteChangeMutex.RLock() + defer fake.captureDeleteChangeMutex.RUnlock() + fake.captureUpsertChangeMutex.RLock() + defer fake.captureUpsertChangeMutex.RUnlock() + fake.processMutex.RLock() + defer fake.processMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeChangeProcessor) 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 _ state.ChangeProcessor = new(FakeChangeProcessor) diff --git a/internal/state/statefakes/fake_configuration.go b/internal/state/statefakes/fake_configuration.go deleted file mode 100644 index 0019e0f0d0..0000000000 --- a/internal/state/statefakes/fake_configuration.go +++ /dev/null @@ -1,197 +0,0 @@ -// Code generated by counterfeiter. DO NOT EDIT. -package statefakes - -import ( - "sync" - - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/gateway-api/apis/v1alpha2" -) - -type FakeConfiguration struct { - DeleteHTTPRouteStub func(types.NamespacedName) ([]state.Change, []state.StatusUpdate) - deleteHTTPRouteMutex sync.RWMutex - deleteHTTPRouteArgsForCall []struct { - arg1 types.NamespacedName - } - deleteHTTPRouteReturns struct { - result1 []state.Change - result2 []state.StatusUpdate - } - deleteHTTPRouteReturnsOnCall map[int]struct { - result1 []state.Change - result2 []state.StatusUpdate - } - UpsertHTTPRouteStub func(*v1alpha2.HTTPRoute) ([]state.Change, []state.StatusUpdate) - upsertHTTPRouteMutex sync.RWMutex - upsertHTTPRouteArgsForCall []struct { - arg1 *v1alpha2.HTTPRoute - } - upsertHTTPRouteReturns struct { - result1 []state.Change - result2 []state.StatusUpdate - } - upsertHTTPRouteReturnsOnCall map[int]struct { - result1 []state.Change - result2 []state.StatusUpdate - } - invocations map[string][][]interface{} - invocationsMutex sync.RWMutex -} - -func (fake *FakeConfiguration) DeleteHTTPRoute(arg1 types.NamespacedName) ([]state.Change, []state.StatusUpdate) { - fake.deleteHTTPRouteMutex.Lock() - ret, specificReturn := fake.deleteHTTPRouteReturnsOnCall[len(fake.deleteHTTPRouteArgsForCall)] - fake.deleteHTTPRouteArgsForCall = append(fake.deleteHTTPRouteArgsForCall, struct { - arg1 types.NamespacedName - }{arg1}) - stub := fake.DeleteHTTPRouteStub - fakeReturns := fake.deleteHTTPRouteReturns - fake.recordInvocation("DeleteHTTPRoute", []interface{}{arg1}) - fake.deleteHTTPRouteMutex.Unlock() - if stub != nil { - return stub(arg1) - } - if specificReturn { - return ret.result1, ret.result2 - } - return fakeReturns.result1, fakeReturns.result2 -} - -func (fake *FakeConfiguration) DeleteHTTPRouteCallCount() int { - fake.deleteHTTPRouteMutex.RLock() - defer fake.deleteHTTPRouteMutex.RUnlock() - return len(fake.deleteHTTPRouteArgsForCall) -} - -func (fake *FakeConfiguration) DeleteHTTPRouteCalls(stub func(types.NamespacedName) ([]state.Change, []state.StatusUpdate)) { - fake.deleteHTTPRouteMutex.Lock() - defer fake.deleteHTTPRouteMutex.Unlock() - fake.DeleteHTTPRouteStub = stub -} - -func (fake *FakeConfiguration) DeleteHTTPRouteArgsForCall(i int) types.NamespacedName { - fake.deleteHTTPRouteMutex.RLock() - defer fake.deleteHTTPRouteMutex.RUnlock() - argsForCall := fake.deleteHTTPRouteArgsForCall[i] - return argsForCall.arg1 -} - -func (fake *FakeConfiguration) DeleteHTTPRouteReturns(result1 []state.Change, result2 []state.StatusUpdate) { - fake.deleteHTTPRouteMutex.Lock() - defer fake.deleteHTTPRouteMutex.Unlock() - fake.DeleteHTTPRouteStub = nil - fake.deleteHTTPRouteReturns = struct { - result1 []state.Change - result2 []state.StatusUpdate - }{result1, result2} -} - -func (fake *FakeConfiguration) DeleteHTTPRouteReturnsOnCall(i int, result1 []state.Change, result2 []state.StatusUpdate) { - fake.deleteHTTPRouteMutex.Lock() - defer fake.deleteHTTPRouteMutex.Unlock() - fake.DeleteHTTPRouteStub = nil - if fake.deleteHTTPRouteReturnsOnCall == nil { - fake.deleteHTTPRouteReturnsOnCall = make(map[int]struct { - result1 []state.Change - result2 []state.StatusUpdate - }) - } - fake.deleteHTTPRouteReturnsOnCall[i] = struct { - result1 []state.Change - result2 []state.StatusUpdate - }{result1, result2} -} - -func (fake *FakeConfiguration) UpsertHTTPRoute(arg1 *v1alpha2.HTTPRoute) ([]state.Change, []state.StatusUpdate) { - fake.upsertHTTPRouteMutex.Lock() - ret, specificReturn := fake.upsertHTTPRouteReturnsOnCall[len(fake.upsertHTTPRouteArgsForCall)] - fake.upsertHTTPRouteArgsForCall = append(fake.upsertHTTPRouteArgsForCall, struct { - arg1 *v1alpha2.HTTPRoute - }{arg1}) - stub := fake.UpsertHTTPRouteStub - fakeReturns := fake.upsertHTTPRouteReturns - fake.recordInvocation("UpsertHTTPRoute", []interface{}{arg1}) - fake.upsertHTTPRouteMutex.Unlock() - if stub != nil { - return stub(arg1) - } - if specificReturn { - return ret.result1, ret.result2 - } - return fakeReturns.result1, fakeReturns.result2 -} - -func (fake *FakeConfiguration) UpsertHTTPRouteCallCount() int { - fake.upsertHTTPRouteMutex.RLock() - defer fake.upsertHTTPRouteMutex.RUnlock() - return len(fake.upsertHTTPRouteArgsForCall) -} - -func (fake *FakeConfiguration) UpsertHTTPRouteCalls(stub func(*v1alpha2.HTTPRoute) ([]state.Change, []state.StatusUpdate)) { - fake.upsertHTTPRouteMutex.Lock() - defer fake.upsertHTTPRouteMutex.Unlock() - fake.UpsertHTTPRouteStub = stub -} - -func (fake *FakeConfiguration) UpsertHTTPRouteArgsForCall(i int) *v1alpha2.HTTPRoute { - fake.upsertHTTPRouteMutex.RLock() - defer fake.upsertHTTPRouteMutex.RUnlock() - argsForCall := fake.upsertHTTPRouteArgsForCall[i] - return argsForCall.arg1 -} - -func (fake *FakeConfiguration) UpsertHTTPRouteReturns(result1 []state.Change, result2 []state.StatusUpdate) { - fake.upsertHTTPRouteMutex.Lock() - defer fake.upsertHTTPRouteMutex.Unlock() - fake.UpsertHTTPRouteStub = nil - fake.upsertHTTPRouteReturns = struct { - result1 []state.Change - result2 []state.StatusUpdate - }{result1, result2} -} - -func (fake *FakeConfiguration) UpsertHTTPRouteReturnsOnCall(i int, result1 []state.Change, result2 []state.StatusUpdate) { - fake.upsertHTTPRouteMutex.Lock() - defer fake.upsertHTTPRouteMutex.Unlock() - fake.UpsertHTTPRouteStub = nil - if fake.upsertHTTPRouteReturnsOnCall == nil { - fake.upsertHTTPRouteReturnsOnCall = make(map[int]struct { - result1 []state.Change - result2 []state.StatusUpdate - }) - } - fake.upsertHTTPRouteReturnsOnCall[i] = struct { - result1 []state.Change - result2 []state.StatusUpdate - }{result1, result2} -} - -func (fake *FakeConfiguration) Invocations() map[string][][]interface{} { - fake.invocationsMutex.RLock() - defer fake.invocationsMutex.RUnlock() - fake.deleteHTTPRouteMutex.RLock() - defer fake.deleteHTTPRouteMutex.RUnlock() - fake.upsertHTTPRouteMutex.RLock() - defer fake.upsertHTTPRouteMutex.RUnlock() - copiedInvocations := map[string][][]interface{}{} - for key, value := range fake.invocations { - copiedInvocations[key] = value - } - return copiedInvocations -} - -func (fake *FakeConfiguration) 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 _ state.Configuration = new(FakeConfiguration) diff --git a/internal/state/statuses.go b/internal/state/statuses.go new file mode 100644 index 0000000000..adcc8d455f --- /dev/null +++ b/internal/state/statuses.go @@ -0,0 +1,73 @@ +package state + +import "k8s.io/apimachinery/pkg/types" + +// ListenerStatuses holds the statuses of listeners where the key is the name of a listener in the Gateway resource. +type ListenerStatuses map[string]ListenerStatus + +// HTTPRouteStatuses holds the statuses of HTTPRoutes where the key is the namespaced name of an HTTPRoute. +type HTTPRouteStatuses map[types.NamespacedName]HTTPRouteStatus + +// Statuses holds the status-related information about Gateway API resources. +// It is assumed that only a singe Gateway resource is used. +type Statuses struct { + ListenerStatuses ListenerStatuses + HTTPRouteStatuses HTTPRouteStatuses +} + +// ListenerStatus holds the status-related information about a listener in the Gateway resource. +type ListenerStatus struct { + // Valid shows if the listener is valid. + Valid bool + // AttachedRoutes is the number of routes attached to the listener. + AttachedRoutes int32 +} + +// ParentStatuses holds the statuses of parents where the key is the section name in a parentRef. +type ParentStatuses map[string]ParentStatus + +type HTTPRouteStatus struct { + ParentStatuses ParentStatuses +} + +// ParentStatus holds status-related information related to how the HTTPRoute binds to a specific parentRef. +type ParentStatus struct { + // Attached is true if the route attaches to the parent (listener). + Attached bool +} + +// buildStatuses builds statuses from a graph. +func buildStatuses(graph *graph) Statuses { + statuses := Statuses{ + ListenerStatuses: make(map[string]ListenerStatus), + HTTPRouteStatuses: make(map[types.NamespacedName]HTTPRouteStatus), + } + + for name, l := range graph.Listeners { + statuses.ListenerStatuses[name] = ListenerStatus{ + Valid: l.Valid, + AttachedRoutes: int32(len(l.Routes)), + } + } + + for nsname, r := range graph.Routes { + parentStatuses := make(map[string]ParentStatus) + + for ref := range r.ValidSectionNameRefs { + parentStatuses[ref] = ParentStatus{ + Attached: true, + } + } + for ref := range r.InvalidSectionNameRefs { + parentStatuses[ref] = ParentStatus{ + Attached: false, + } + } + + statuses.HTTPRouteStatuses[nsname] = HTTPRouteStatus{ + ParentStatuses: parentStatuses, + } + } + + return statuses +} diff --git a/internal/state/statuses_test.go b/internal/state/statuses_test.go new file mode 100644 index 0000000000..35ff58edaf --- /dev/null +++ b/internal/state/statuses_test.go @@ -0,0 +1,56 @@ +package state + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "k8s.io/apimachinery/pkg/types" +) + +func TestBuildStatuses(t *testing.T) { + g := &graph{ + Listeners: map[string]*listener{ + "listener-80-1": { + Valid: true, + Routes: map[types.NamespacedName]*route{ + {Namespace: "test", Name: "hr-1"}: {}}, + }, + }, + Routes: map[types.NamespacedName]*route{ + {Namespace: "test", Name: "hr-1"}: { + ValidSectionNameRefs: map[string]struct{}{ + "listener-80-1": {}, + }, + InvalidSectionNameRefs: map[string]struct{}{ + "listener-80-2": {}, + }, + }, + }, + } + + expected := Statuses{ + ListenerStatuses: map[string]ListenerStatus{ + "listener-80-1": { + Valid: true, + AttachedRoutes: 1, + }, + }, + HTTPRouteStatuses: map[types.NamespacedName]HTTPRouteStatus{ + {Namespace: "test", Name: "hr-1"}: { + ParentStatuses: map[string]ParentStatus{ + "listener-80-1": { + Attached: true, + }, + "listener-80-2": { + Attached: false, + }, + }, + }, + }, + } + + result := buildStatuses(g) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("buildStatuses() mismatch (-want +got):\n%s", diff) + } +} diff --git a/internal/state/store.go b/internal/state/store.go new file mode 100644 index 0000000000..73e8a66c52 --- /dev/null +++ b/internal/state/store.go @@ -0,0 +1,18 @@ +package state + +import ( + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/gateway-api/apis/v1alpha2" +) + +// store contains the resources that represent the state of the Gateway. +type store struct { + gw *v1alpha2.Gateway + httpRoutes map[types.NamespacedName]*v1alpha2.HTTPRoute +} + +func newStore() *store { + return &store{ + httpRoutes: make(map[types.NamespacedName]*v1alpha2.HTTPRoute), + } +} diff --git a/internal/status/clock.go b/internal/status/clock.go new file mode 100644 index 0000000000..a9c90a2299 --- /dev/null +++ b/internal/status/clock.go @@ -0,0 +1,26 @@ +package status + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Clock + +// Clock returns the current local time. +type Clock interface { + Now() metav1.Time +} + +// Real clock returns the current local time. +type RealClock struct { +} + +// NewRealClock creates a new RealClock. +func NewRealClock() *RealClock { + return &RealClock{} +} + +// Now returns the current local time. +func (c *RealClock) Now() metav1.Time { + return metav1.Now() +} diff --git a/internal/status/gateway.go b/internal/status/gateway.go new file mode 100644 index 0000000000..8884c5e5ef --- /dev/null +++ b/internal/status/gateway.go @@ -0,0 +1,68 @@ +package status + +import ( + "sort" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" +) + +// prepareGatewayStatus prepares the status for a Gateway resource. +// FIXME(pleshakov): Be compliant with in the Gateway API. +// Currently, we only support simple valid/invalid status per each listener. +// Extend support to cover more cases. +func prepareGatewayStatus(statuses state.ListenerStatuses, transitionTime metav1.Time) v1alpha2.GatewayStatus { + listenerStatuses := make([]v1alpha2.ListenerStatus, 0, len(statuses)) + + // FIXME(pleshakov) Maintain the order from the Gateway resource + names := make([]string, 0, len(statuses)) + for name := range statuses { + names = append(names, name) + } + sort.Strings(names) + + for _, name := range names { + s := statuses[name] + + var ( + status metav1.ConditionStatus + reason v1alpha2.ListenerConditionReason + ) + + if s.Valid { + status = metav1.ConditionTrue + reason = v1alpha2.ListenerReasonReady + } else { + status = metav1.ConditionFalse + reason = v1alpha2.ListenerReasonInvalid + } + + cond := metav1.Condition{ + Type: string(v1alpha2.ListenerConditionReady), + Status: status, + // FIXME(pleshakov) Set the observed generation to the last processed generation of the Gateway resource. + ObservedGeneration: 123, + LastTransitionTime: transitionTime, + Reason: string(reason), + Message: "", // FIXME(pleshakov) Come up with a good message + } + + listenerStatuses = append(listenerStatuses, v1alpha2.ListenerStatus{ + Name: v1alpha2.SectionName(name), + SupportedKinds: []v1alpha2.RouteGroupKind{ + { + Kind: "HTTPRoute", // FIXME(pleshakov) Set it based on the listener + }, + }, + AttachedRoutes: s.AttachedRoutes, + Conditions: []metav1.Condition{cond}, + }) + } + + return v1alpha2.GatewayStatus{ + Listeners: listenerStatuses, + Conditions: nil, // FIXME(pleshakov) Create conditions for the Gateway resource. + } +} diff --git a/internal/status/gateway_test.go b/internal/status/gateway_test.go new file mode 100644 index 0000000000..db307bc80b --- /dev/null +++ b/internal/status/gateway_test.go @@ -0,0 +1,73 @@ +package status + +import ( + "testing" + "time" + + "github.com/google/go-cmp/cmp" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" +) + +func TestPrepareGatewayStatus(t *testing.T) { + statuses := state.ListenerStatuses{ + "valid-listener": { + Valid: true, + AttachedRoutes: 2, + }, + "invalid-listener": { + Valid: false, + AttachedRoutes: 1, + }, + } + + transitionTime := metav1.NewTime(time.Now()) + + expected := v1alpha2.GatewayStatus{ + Listeners: []v1alpha2.ListenerStatus{ + { + Name: "invalid-listener", + SupportedKinds: []v1alpha2.RouteGroupKind{ + { + Kind: "HTTPRoute", + }, + }, + AttachedRoutes: 1, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ListenerConditionReady), + Status: metav1.ConditionFalse, + ObservedGeneration: 123, + LastTransitionTime: transitionTime, + Reason: string(v1alpha2.ListenerReasonInvalid), + }, + }, + }, + { + Name: "valid-listener", + SupportedKinds: []v1alpha2.RouteGroupKind{ + { + Kind: "HTTPRoute", + }, + }, + AttachedRoutes: 2, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ListenerConditionReady), + Status: metav1.ConditionTrue, + ObservedGeneration: 123, + LastTransitionTime: transitionTime, + Reason: string(v1alpha2.ListenerReasonReady), + }, + }, + }, + }, + } + + result := prepareGatewayStatus(statuses, transitionTime) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("prepareGatewayStatus() mismatch (-want +got):\n%s", diff) + } +} diff --git a/internal/status/httproute.go b/internal/status/httproute.go new file mode 100644 index 0000000000..94e4b7ed23 --- /dev/null +++ b/internal/status/httproute.go @@ -0,0 +1,77 @@ +package status + +import ( + "sort" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" +) + +// prepareHTTPRouteStatus prepares the status for an HTTPRoute resource. +// FIXME(pleshakov): Be compliant with in the Gateway API. +// Currently, we only support simple attached/not attached status per each parentRef. +// Extend support to cover more cases. +func prepareHTTPRouteStatus( + status state.HTTPRouteStatus, + gwNsName types.NamespacedName, + gatewayCtlrName string, + transitionTime metav1.Time, +) v1alpha2.HTTPRouteStatus { + parents := make([]v1alpha2.RouteParentStatus, 0, len(status.ParentStatuses)) + + // FIXME(pleshakov) Maintain the order from the HTTPRoute resource + names := make([]string, 0, len(status.ParentStatuses)) + for name := range status.ParentStatuses { + names = append(names, name) + } + sort.Strings(names) + + for _, name := range names { + ps := status.ParentStatuses[name] + + var ( + status metav1.ConditionStatus + reason string // FIXME(pleshakov) use RouteConditionReason once we upgrade to v1beta1 + ) + + if ps.Attached { + status = metav1.ConditionTrue + reason = "Accepted" // FIXME(pleshakov): use RouteReasonAccepted once we upgrade to v1beta1 + } else { + status = metav1.ConditionFalse + reason = "NotAttached" // FIXME(pleshakov): use a more specific message from the defined constants (available in v1beta1) + } + + sectionName := name + + p := v1alpha2.RouteParentStatus{ + ParentRef: v1alpha2.ParentRef{ + Namespace: (*v1alpha2.Namespace)(&gwNsName.Namespace), + Name: v1alpha2.ObjectName(gwNsName.Name), + SectionName: (*v1alpha2.SectionName)(§ionName), + }, + ControllerName: v1alpha2.GatewayController(gatewayCtlrName), + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: status, + // FIXME(pleshakov) Set the observed generation to the last processed generation of the HTTPRoute resource. + ObservedGeneration: 123, + LastTransitionTime: transitionTime, + Reason: reason, + Message: "", // FIXME(pleshakov): Figure out a good message + }, + }, + } + parents = append(parents, p) + } + + return v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: parents, + }, + } +} diff --git a/internal/status/httproute_test.go b/internal/status/httproute_test.go new file mode 100644 index 0000000000..ad9513a3ec --- /dev/null +++ b/internal/status/httproute_test.go @@ -0,0 +1,78 @@ +package status + +import ( + "testing" + "time" + + "github.com/google/go-cmp/cmp" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" +) + +func TestPrepareHTTPRouteStatus(t *testing.T) { + status := state.HTTPRouteStatus{ + ParentStatuses: map[string]state.ParentStatus{ + "attached": { + Attached: true, + }, + "not-attached": { + Attached: false, + }, + }, + } + + gwNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} + gatewayCtlrName := "test.example.com" + + transitionTime := metav1.NewTime(time.Now()) + + expected := v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ParentRef: v1alpha2.ParentRef{ + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: "gateway", + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("attached")), + }, + ControllerName: v1alpha2.GatewayController(gatewayCtlrName), + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: metav1.ConditionTrue, + ObservedGeneration: 123, + LastTransitionTime: transitionTime, + Reason: "Accepted", + }, + }, + }, + { + ParentRef: v1alpha2.ParentRef{ + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: "gateway", + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("not-attached")), + }, + ControllerName: v1alpha2.GatewayController(gatewayCtlrName), + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: metav1.ConditionFalse, + ObservedGeneration: 123, + LastTransitionTime: transitionTime, + Reason: "NotAttached", + }, + }, + }, + }, + }, + } + + result := prepareHTTPRouteStatus(status, gwNsName, gatewayCtlrName, transitionTime) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("prepareHTTPRouteStatus() mismatch (-want +got):\n%s", diff) + } +} diff --git a/internal/status/statusfakes/fake_clock.go b/internal/status/statusfakes/fake_clock.go new file mode 100644 index 0000000000..f02f382c08 --- /dev/null +++ b/internal/status/statusfakes/fake_clock.go @@ -0,0 +1,103 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package statusfakes + +import ( + "sync" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/status" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +type FakeClock struct { + NowStub func() v1.Time + nowMutex sync.RWMutex + nowArgsForCall []struct { + } + nowReturns struct { + result1 v1.Time + } + nowReturnsOnCall map[int]struct { + result1 v1.Time + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeClock) Now() v1.Time { + fake.nowMutex.Lock() + ret, specificReturn := fake.nowReturnsOnCall[len(fake.nowArgsForCall)] + fake.nowArgsForCall = append(fake.nowArgsForCall, struct { + }{}) + stub := fake.NowStub + fakeReturns := fake.nowReturns + fake.recordInvocation("Now", []interface{}{}) + fake.nowMutex.Unlock() + if stub != nil { + return stub() + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeClock) NowCallCount() int { + fake.nowMutex.RLock() + defer fake.nowMutex.RUnlock() + return len(fake.nowArgsForCall) +} + +func (fake *FakeClock) NowCalls(stub func() v1.Time) { + fake.nowMutex.Lock() + defer fake.nowMutex.Unlock() + fake.NowStub = stub +} + +func (fake *FakeClock) NowReturns(result1 v1.Time) { + fake.nowMutex.Lock() + defer fake.nowMutex.Unlock() + fake.NowStub = nil + fake.nowReturns = struct { + result1 v1.Time + }{result1} +} + +func (fake *FakeClock) NowReturnsOnCall(i int, result1 v1.Time) { + fake.nowMutex.Lock() + defer fake.nowMutex.Unlock() + fake.NowStub = nil + if fake.nowReturnsOnCall == nil { + fake.nowReturnsOnCall = make(map[int]struct { + result1 v1.Time + }) + } + fake.nowReturnsOnCall[i] = struct { + result1 v1.Time + }{result1} +} + +func (fake *FakeClock) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.nowMutex.RLock() + defer fake.nowMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeClock) 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 _ status.Clock = new(FakeClock) diff --git a/internal/status/statusfakes/fake_updater.go b/internal/status/statusfakes/fake_updater.go index f9d916ee43..6f611eb919 100644 --- a/internal/status/statusfakes/fake_updater.go +++ b/internal/status/statusfakes/fake_updater.go @@ -10,59 +10,54 @@ import ( ) type FakeUpdater struct { - ProcessStatusUpdatesStub func(context.Context, []state.StatusUpdate) - processStatusUpdatesMutex sync.RWMutex - processStatusUpdatesArgsForCall []struct { + UpdateStub func(context.Context, state.Statuses) + updateMutex sync.RWMutex + updateArgsForCall []struct { arg1 context.Context - arg2 []state.StatusUpdate + arg2 state.Statuses } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } -func (fake *FakeUpdater) ProcessStatusUpdates(arg1 context.Context, arg2 []state.StatusUpdate) { - var arg2Copy []state.StatusUpdate - if arg2 != nil { - arg2Copy = make([]state.StatusUpdate, len(arg2)) - copy(arg2Copy, arg2) - } - fake.processStatusUpdatesMutex.Lock() - fake.processStatusUpdatesArgsForCall = append(fake.processStatusUpdatesArgsForCall, struct { +func (fake *FakeUpdater) Update(arg1 context.Context, arg2 state.Statuses) { + fake.updateMutex.Lock() + fake.updateArgsForCall = append(fake.updateArgsForCall, struct { arg1 context.Context - arg2 []state.StatusUpdate - }{arg1, arg2Copy}) - stub := fake.ProcessStatusUpdatesStub - fake.recordInvocation("ProcessStatusUpdates", []interface{}{arg1, arg2Copy}) - fake.processStatusUpdatesMutex.Unlock() + arg2 state.Statuses + }{arg1, arg2}) + stub := fake.UpdateStub + fake.recordInvocation("Update", []interface{}{arg1, arg2}) + fake.updateMutex.Unlock() if stub != nil { - fake.ProcessStatusUpdatesStub(arg1, arg2) + fake.UpdateStub(arg1, arg2) } } -func (fake *FakeUpdater) ProcessStatusUpdatesCallCount() int { - fake.processStatusUpdatesMutex.RLock() - defer fake.processStatusUpdatesMutex.RUnlock() - return len(fake.processStatusUpdatesArgsForCall) +func (fake *FakeUpdater) UpdateCallCount() int { + fake.updateMutex.RLock() + defer fake.updateMutex.RUnlock() + return len(fake.updateArgsForCall) } -func (fake *FakeUpdater) ProcessStatusUpdatesCalls(stub func(context.Context, []state.StatusUpdate)) { - fake.processStatusUpdatesMutex.Lock() - defer fake.processStatusUpdatesMutex.Unlock() - fake.ProcessStatusUpdatesStub = stub +func (fake *FakeUpdater) UpdateCalls(stub func(context.Context, state.Statuses)) { + fake.updateMutex.Lock() + defer fake.updateMutex.Unlock() + fake.UpdateStub = stub } -func (fake *FakeUpdater) ProcessStatusUpdatesArgsForCall(i int) (context.Context, []state.StatusUpdate) { - fake.processStatusUpdatesMutex.RLock() - defer fake.processStatusUpdatesMutex.RUnlock() - argsForCall := fake.processStatusUpdatesArgsForCall[i] +func (fake *FakeUpdater) UpdateArgsForCall(i int) (context.Context, state.Statuses) { + fake.updateMutex.RLock() + defer fake.updateMutex.RUnlock() + argsForCall := fake.updateArgsForCall[i] return argsForCall.arg1, argsForCall.arg2 } func (fake *FakeUpdater) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() - fake.processStatusUpdatesMutex.RLock() - defer fake.processStatusUpdatesMutex.RUnlock() + fake.updateMutex.RLock() + defer fake.updateMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} for key, value := range fake.invocations { copiedInvocations[key] = value diff --git a/internal/status/updater.go b/internal/status/updater.go index fede7f298e..a3fdf535c7 100644 --- a/internal/status/updater.go +++ b/internal/status/updater.go @@ -2,7 +2,6 @@ package status import ( "context" - "fmt" "github.com/go-logr/logr" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -17,11 +16,11 @@ import ( // Updater updates statuses of the Gateway API resources. type Updater interface { - // ProcessStatusUpdates updates the statuses according to the provided updates. - ProcessStatusUpdates(context.Context, []state.StatusUpdate) + // Update updates the statuses of the resources. + Update(context.Context, state.Statuses) } -// updaterImpl reports statuses for the Gateway API resources. +// updaterImpl updates statuses of the Gateway API resources. // // It has the following limitations: // @@ -29,61 +28,80 @@ type Updater interface { // multiple replicas will step on each other when trying to report statuses for the same resources. // FIXME(pleshakov): address limitation (1) // -// (2) It is synchronous, which means the status reporter can slow down the event loop. +// (2) It is not smart. It will update the status of a resource (make an API call) even if it hasn't changed. +// FIXME(pleshakov) address limitation (2) +// +// (3) It is synchronous, which means the status reporter can slow down the event loop. // Consider the following cases: // (a) Sometimes the Gateway will need to update statuses of all resources it handles, which could be ~1000. Making 1000 // status API calls sequentially will take time. // (b) k8s API can become slow or even timeout. This will increase every update status API call. // Making updaterImpl asynchronous will prevent it from adding variable delays to the event loop. -// FIXME(pleshakov) address limitation (2) +// FIXME(pleshakov) address limitation (3) // -// (3) It doesn't retry on failures. This means there is a chance that some resources will not have up-to-do statuses. +// (4) It doesn't retry on failures. This means there is a chance that some resources will not have up-to-do statuses. // Statuses are important part of the Gateway API, so we need to ensure that the Gateway always keep the resources // statuses up-to-date. -// FIXME(pleshakov): address limitation (3) +// FIXME(pleshakov): address limitation (4) // -// (4) To support new resources, updaterImpl needs to be modified. Consider making updaterImpl extendable, so that it +// (5) It doesn't clear the statuses of a resources that are no longer handled by the Gateway. For example, if +// an HTTPRoute resource no longer has the parentRef to the Gateway resources, the Gateway must update the status +// of the resource to remove the status about the removed parentRef. +// FIXME(pleshakov): address limitation (5) +// +// (6) If another controllers changes the status of the Gateway/HTTPRoute resource so that the information set by our +// Gateway is removed, our Gateway will not restore the status until the EventLoop invokes the StatusUpdater as a +// result of processing some other new change to a resource(s). +// FIXME(pleshakov): Figure out if this is something that needs to be addressed. + +// (7) To support new resources, updaterImpl needs to be modified. Consider making updaterImpl extendable, so that it // goes along the Open-closed principle. -// FIXME(pleshakov): address limitation (4) +// FIXME(pleshakov): address limitation (7) type updaterImpl struct { - client client.Client - logger logr.Logger + gatewayCtlrName string + gwNsName types.NamespacedName + client client.Client + logger logr.Logger + clock Clock } // NewUpdater creates a new Updater. -func NewUpdater(client client.Client, logger logr.Logger) Updater { +func NewUpdater( + gatewayCtlrName string, + gwNsName types.NamespacedName, + client client.Client, + logger logr.Logger, + clock Clock, +) Updater { return &updaterImpl{ - client: client, - logger: logger.WithName("statusUpdater"), + gatewayCtlrName: gatewayCtlrName, + gwNsName: gwNsName, + client: client, + logger: logger.WithName("statusUpdater"), + clock: clock, } } -// ProcessStatusUpdates updates the statuses according to the provided updates. -func (upd *updaterImpl) ProcessStatusUpdates(ctx context.Context, updates []state.StatusUpdate) { - for _, u := range updates { +func (upd *updaterImpl) Update(ctx context.Context, statuses state.Statuses) { + // FIXME(pleshakov) Merge the new Conditions in the status with the existing Conditions + // FIXME(pleshakov) Skip the status update (API call) if the status hasn't changed. + + upd.update(ctx, upd.gwNsName, &v1alpha2.Gateway{}, func(object client.Object) { + gw := object.(*v1alpha2.Gateway) + gw.Status = prepareGatewayStatus(statuses.ListenerStatuses, upd.clock.Now()) + }) + + for nsname, rs := range statuses.HTTPRouteStatuses { select { case <-ctx.Done(): return default: } - switch s := u.Status.(type) { - case *v1alpha2.HTTPRouteStatus: - upd.logger.Info("Processing a status update for HTTPRoute", - "namespace", u.NamespacedName.Namespace, - "name", u.NamespacedName.Name) - - var hr v1alpha2.HTTPRoute - - upd.update(ctx, u.NamespacedName, &hr, func(object client.Object) { - route := object.(*v1alpha2.HTTPRoute) - // FIXME(pleshakov): merge the conditions in the status with the conditions in the route.Status properly, because - // right now, we are replacing the conditions. - route.Status = *s - }) - default: - panic(fmt.Errorf("unknown status type %T", u.Status)) - } + upd.update(ctx, nsname, &v1alpha2.HTTPRoute{}, func(object client.Object) { + hr := object.(*v1alpha2.HTTPRoute) + hr.Status = prepareHTTPRouteStatus(rs, upd.gwNsName, upd.gatewayCtlrName, upd.clock.Now()) + }) } } diff --git a/internal/status/updater_test.go b/internal/status/updater_test.go index c8db418320..0cc4fb4672 100644 --- a/internal/status/updater_test.go +++ b/internal/status/updater_test.go @@ -2,6 +2,7 @@ package status_test import ( "context" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -17,11 +18,17 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" "github.com/nginxinc/nginx-kubernetes-gateway/internal/status" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/status/statusfakes" ) var _ = Describe("Updater", func() { - var updater status.Updater - var client client.Client + var ( + updater status.Updater + client client.Client + fakeClockTime metav1.Time + gatewayCtrlName string + gwNsName types.NamespacedName + ) BeforeEach(OncePerOrdered, func() { scheme := runtime.NewScheme() @@ -31,19 +38,31 @@ var _ = Describe("Updater", func() { client = fake.NewClientBuilder(). WithScheme(scheme). Build() - updater = status.NewUpdater(client, zap.New()) + + // Rfc3339Copy() removes the monotonic clock reading + // We need to remove it, because updating the status in the FakeClient and then getting the resource back + // involves encoding and decoding the resource to/from JSON, which removes the monotonic clock reading. + fakeClockTime = metav1.NewTime(time.Now()).Rfc3339Copy() + fakeClock := &statusfakes.FakeClock{} + fakeClock.NowReturns(fakeClockTime) + + gatewayCtrlName = "test.example.com" + gwNsName = types.NamespacedName{Namespace: "test", Name: "gateway"} + + updater = status.NewUpdater(gatewayCtrlName, gwNsName, client, zap.New(), fakeClock) }) - Describe("Process status update of HTTPRoute", Ordered, func() { - var hr *v1alpha2.HTTPRoute - var testTime metav1.Time + Describe("Process status updates", Ordered, func() { + var ( + hr *v1alpha2.HTTPRoute + gw *v1alpha2.Gateway - BeforeAll(func() { - // Rfc3339Copy() removes the monotonic clock reading - // it is important, because updating the status in the FakeClient and then getting the resource back - // involves encoding and decoding the resource to/from JSON, which removes the monotonic clock reading. - testTime = metav1.Now().Rfc3339Copy() + createStatuses func(bool, bool) state.Statuses + createExpectedHR func() *v1alpha2.HTTPRoute + createExpectedGw func(metav1.ConditionStatus, string) *v1alpha2.Gateway + ) + BeforeAll(func() { hr = &v1alpha2.HTTPRoute{ ObjectMeta: metav1.ObjectMeta{ Namespace: "test", @@ -54,77 +73,121 @@ var _ = Describe("Updater", func() { APIVersion: "gateway.networking.k8s.io/v1alpha2", }, } + gw = &v1alpha2.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "gateway", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "Gateway", + APIVersion: "gateway.networking.k8s.io/v1alpha2", + }, + } - Expect(client.Create(context.Background(), hr)).Should(Succeed()) - }) + createStatuses = func(listenerValid, routeAttached bool) state.Statuses { + return state.Statuses{ + ListenerStatuses: map[string]state.ListenerStatus{ + "http": { + Valid: listenerValid, + AttachedRoutes: 1, + }, + }, + HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ + {Namespace: "test", Name: "route1"}: { + ParentStatuses: map[string]state.ParentStatus{ + "http": { + Attached: routeAttached, + }, + }, + }, + }, + } + } - It("should process status update", func() { - updates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ + createExpectedHR = func() *v1alpha2.HTTPRoute { + return &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "route1", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "HTTPRoute", + APIVersion: "gateway.networking.k8s.io/v1alpha2", + }, + Status: gatewayv1alpha2.HTTPRouteStatus{ + RouteStatus: gatewayv1alpha2.RouteStatus{ + Parents: []gatewayv1alpha2.RouteParentStatus{ { - ControllerName: "test", - ParentRef: v1alpha2.ParentRef{ - Name: "fake", + ControllerName: gatewayv1alpha2.GatewayController(gatewayCtrlName), + ParentRef: gatewayv1alpha2.ParentRef{ + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: "gateway", + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("http")), }, Conditions: []metav1.Condition{ { - Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: hr.Generation, - LastTransitionTime: testTime, - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", + Type: string(gatewayv1alpha2.ConditionRouteAccepted), + Status: metav1.ConditionTrue, + ObservedGeneration: 123, + LastTransitionTime: fakeClockTime, + Reason: "Accepted", }, }, }, }, }, }, - }, + } } - updater.ProcessStatusUpdates(context.Background(), updates) - }) - - It("should have the updated status in the API server", func() { - expectedHR := &v1alpha2.HTTPRoute{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "route1", - }, - TypeMeta: metav1.TypeMeta{ - Kind: "HTTPRoute", - APIVersion: "gateway.networking.k8s.io/v1alpha2", - }, - Status: gatewayv1alpha2.HTTPRouteStatus{ - RouteStatus: gatewayv1alpha2.RouteStatus{ - Parents: []gatewayv1alpha2.RouteParentStatus{ + createExpectedGw = func(status metav1.ConditionStatus, reason string) *v1alpha2.Gateway { + return &v1alpha2.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "gateway", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "Gateway", + APIVersion: "gateway.networking.k8s.io/v1alpha2", + }, + Status: v1alpha2.GatewayStatus{ + Listeners: []gatewayv1alpha2.ListenerStatus{ { - ControllerName: "test", - ParentRef: gatewayv1alpha2.ParentRef{ - Name: "fake", + Name: "http", + SupportedKinds: []v1alpha2.RouteGroupKind{ + { + Kind: "HTTPRoute", + }, }, + AttachedRoutes: 1, Conditions: []metav1.Condition{ { - Type: string(gatewayv1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: 0, - LastTransitionTime: testTime, - Reason: string(gatewayv1alpha2.ConditionRouteAccepted), - Message: "", + Type: string(v1alpha2.ListenerConditionReady), + Status: status, + ObservedGeneration: 123, + LastTransitionTime: fakeClockTime, + Reason: reason, }, }, }, }, }, - }, + } } + }) + It("should create resources in the API server", func() { + Expect(client.Create(context.Background(), hr)).Should(Succeed()) + Expect(client.Create(context.Background(), gw)).Should(Succeed()) + }) + + It("should update statuses", func() { + updater.Update(context.Background(), createStatuses(true, true)) + }) + + It("should have the updated status of HTTPRoute in the API server", func() { latestHR := &v1alpha2.HTTPRoute{} + expectedHR := createExpectedHR() err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "route1"}, latestHR) Expect(err).Should(Not(HaveOccurred())) @@ -133,35 +196,47 @@ var _ = Describe("Updater", func() { Expect(helpers.Diff(expectedHR, latestHR)).To(BeEmpty()) }) - }) - It("should panic for unknown status type", func() { - updates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: "unsupported", - }, - } - - process := func() { - updater.ProcessStatusUpdates(context.Background(), updates) - } - Expect(process).Should(Panic()) - }) + It("should have the updated status of Gateway in the API server", func() { + latestGw := &v1alpha2.Gateway{} + expectedGw := createExpectedGw(metav1.ConditionTrue, string(v1alpha2.ListenerReasonReady)) + + err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw) + Expect(err).Should(Not(HaveOccurred())) - It("should not process updates with canceled context", func() { - updates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: "unsupported", - }, - } + expectedGw.ResourceVersion = latestGw.ResourceVersion - ctx, cancel := context.WithCancel(context.Background()) - cancel() + Expect(helpers.Diff(expectedGw, latestGw)).To(BeEmpty()) + }) - // because the ctx is canceled, ProcessStatusUpdates should return immediately without panicking - // because of the unsupported status type - updater.ProcessStatusUpdates(ctx, updates) + It("should update statuses with canceled context - function normally returns", func() { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + updater.Update(ctx, createStatuses(false, false)) + }) + + It("should not have the updated status of HTTPRoute in the API server after updating with canceled context", func() { + latestHR := &v1alpha2.HTTPRoute{} + expectedHR := createExpectedHR() + + err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "route1"}, latestHR) + Expect(err).Should(Not(HaveOccurred())) + + expectedHR.ResourceVersion = latestHR.ResourceVersion + + Expect(helpers.Diff(expectedHR, latestHR)).To(BeEmpty()) + }) + + It("should have the updated status of Gateway in the API server after updating with canceled context", func() { + latestGw := &v1alpha2.Gateway{} + expectedGw := createExpectedGw(metav1.ConditionFalse, string(v1alpha2.ListenerReasonInvalid)) + + err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw) + Expect(err).Should(Not(HaveOccurred())) + + expectedGw.ResourceVersion = latestGw.ResourceVersion + + Expect(helpers.Diff(expectedGw, latestGw)).To(BeEmpty()) + }) }) }) diff --git a/pkg/sdk/gateway_controller.go b/pkg/sdk/gateway_controller.go index 6c931e5c37..e18ad483b3 100644 --- a/pkg/sdk/gateway_controller.go +++ b/pkg/sdk/gateway_controller.go @@ -47,7 +47,7 @@ func (r *gatewayReconciler) Reconcile(ctx context.Context, req reconcile.Request } if !found { - r.impl.Remove(req.Name) + r.impl.Remove(req.NamespacedName) return reconcile.Result{}, nil } diff --git a/pkg/sdk/interfaces.go b/pkg/sdk/interfaces.go index 841d86ab08..88cf168dd9 100644 --- a/pkg/sdk/interfaces.go +++ b/pkg/sdk/interfaces.go @@ -15,7 +15,7 @@ type GatewayClassImpl interface { type GatewayImpl interface { Upsert(*v1alpha2.Gateway) - Remove(string) + Remove(types.NamespacedName) } type GatewayConfigImpl interface {