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 {