From cc33de85c82c3612577bc7ac0f876eac3874ef6b Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 23 Jun 2022 13:52:13 -0700 Subject: [PATCH 1/5] Support GatewayClass resource This commit brings support for a GatewayClass resource. NGINX Gateway supports a single GatewayClass resource that must be configured via a new cli argument -- gatewayclass. The GatewayClass resource is required - no traffic will go through NGINX if the GatewayClass is missing. --- README.md | 6 + cmd/gateway/main.go | 7 + cmd/gateway/setup.go | 27 +++ cmd/gateway/setup_test.go | 209 ++++++++++------- deploy/manifests/nginx-gateway.yaml | 2 + internal/config/config.go | 2 + internal/events/loop.go | 4 + internal/events/loop_test.go | 2 + .../gatewayclass/gatewayclass.go | 58 +++-- .../gatewayclass/gatewayclass_test.go | 88 +++++++ .../gatewayclass/implementation_suit_test.go | 13 ++ internal/manager/manager.go | 9 +- internal/state/change_processor.go | 32 ++- internal/state/change_processor_test.go | 171 +++++++++++++- internal/state/configuration.go | 4 + internal/state/configuration_test.go | 64 ++++- internal/state/graph.go | 57 ++++- internal/state/graph_test.go | 108 ++++++++- internal/state/statuses.go | 29 ++- internal/state/statuses_test.go | 149 +++++++++--- internal/state/store.go | 1 + internal/status/gatewayclass.go | 39 ++++ internal/status/gatewayclass_test.go | 69 ++++++ internal/status/updater.go | 10 + internal/status/updater_test.go | 219 ++++++++++++------ pkg/sdk/gatewayclass_controller.go | 5 +- pkg/sdk/interfaces.go | 4 +- 27 files changed, 1142 insertions(+), 246 deletions(-) create mode 100644 internal/implementations/gatewayclass/gatewayclass_test.go create mode 100644 internal/implementations/gatewayclass/implementation_suit_test.go create mode 100644 internal/status/gatewayclass.go create mode 100644 internal/status/gatewayclass_test.go diff --git a/README.md b/README.md index babefc384e..c32e2a2c8c 100644 --- a/README.md +++ b/README.md @@ -72,6 +72,12 @@ You can deploy NGINX Kubernetes Gateway on an existing Kubernetes 1.16+ cluster. kubectl create configmap njs-modules --from-file=internal/nginx/modules/src/httpmatches.js -n nginx-gateway ``` +1. Create the GatewayClass resource: + + ``` + kubectl apply -f deploy/manifests/gatewayclass.yaml + ``` + 1. Deploy the NGINX Kubernetes Gateway: Before deploying, make sure to update the Deployment spec in `nginx-gateway.yaml` to reference the image you built. diff --git a/cmd/gateway/main.go b/cmd/gateway/main.go index 5365a875d1..d645320893 100644 --- a/cmd/gateway/main.go +++ b/cmd/gateway/main.go @@ -28,6 +28,11 @@ var ( "", fmt.Sprintf("The name of the Gateway controller. The controller name must be of the form: DOMAIN/NAMESPACE/NAME. The controller's domain is '%s'.", domain), ) + + gatewayClassName = flag.String( + "gatewayclass", + "", + "The name of the GatewayClass resource. Every NGINX Gateway must have a unique corresponding GatewayClass resource") ) func main() { @@ -42,11 +47,13 @@ func main() { Namespace: "nginx-gateway", Name: "gateway", }, + GatewayClassName: *gatewayClassName, } MustValidateArguments( flag.CommandLine, GatewayControllerParam(domain, "nginx-gateway" /* FIXME(f5yacobucci) dynamically set */), + GatewayClassParam(), ) logger.Info("Starting NGINX Kubernetes Gateway", diff --git a/cmd/gateway/setup.go b/cmd/gateway/setup.go index e703e9af30..fa322ec50a 100644 --- a/cmd/gateway/setup.go +++ b/cmd/gateway/setup.go @@ -7,6 +7,7 @@ import ( "strings" flag "github.com/spf13/pflag" + "k8s.io/apimachinery/pkg/util/validation" ) const ( @@ -65,6 +66,32 @@ func GatewayControllerParam(domain string, namespace string) ValidatorContext { } } +func GatewayClassParam() ValidatorContext { + name := "gatewayclass" + return ValidatorContext{ + name, + func(flagset *flag.FlagSet) error { + param, err := flagset.GetString(name) + if err != nil { + return err + } + + if len(param) == 0 { + return errors.New("flag must be set") + } + + // used by Kubernetes to validate resource names + messages := validation.IsDNS1123Subdomain(param) + if len(messages) > 0 { + msg := strings.Join(messages, "; ") + return fmt.Errorf("invalid format: %s", msg) + } + + return nil + }, + } +} + func ValidateArguments(flagset *flag.FlagSet, validators ...ValidatorContext) []string { var msgs []string for _, v := range validators { diff --git a/cmd/gateway/setup_test.go b/cmd/gateway/setup_test.go index 6c7b4993aa..0786e9bbdd 100644 --- a/cmd/gateway/setup_test.go +++ b/cmd/gateway/setup_test.go @@ -10,8 +10,6 @@ import ( . "github.com/nginxinc/nginx-kubernetes-gateway/cmd/gateway" ) -var domain string - func MockValidator(name string, called *int, succeed bool) ValidatorContext { return ValidatorContext{ name, @@ -122,22 +120,25 @@ var _ = Describe("Main", func() { Describe("CLI argument validation", func() { type testCase struct { - Param string - Domain string - ExpError bool + Flag string + Value string + ValidatorContext ValidatorContext + ExpError bool } + const ( + expectError = true + expectSuccess = false + ) + var mockFlags *flag.FlagSet - var gatewayCtlrName string tester := func(t testCase) { - err := mockFlags.Set(gatewayCtlrName, t.Param) + err := mockFlags.Set(t.Flag, t.Value) Expect(err).ToNot(HaveOccurred()) - v := GatewayControllerParam(domain, t.Domain) - Expect(v.V).ToNot(BeNil()) + err = t.ValidatorContext.V(mockFlags) - err = v.V(mockFlags) if t.ExpError { Expect(err).To(HaveOccurred()) } else { @@ -150,88 +151,122 @@ var _ = Describe("Main", func() { } } - BeforeEach(func() { - domain = "k8s-gateway.nginx.org" - gatewayCtlrName = "gateway-ctlr-name" - - mockFlags = flag.NewFlagSet("mock", flag.PanicOnError) - _ = mockFlags.String("gateway-ctlr-name", "", "mock gateway-ctlr-name") - err := mockFlags.Parse([]string{}) - Expect(err).ToNot(HaveOccurred()) - }) - AfterEach(func() { - mockFlags = nil - }) - It("should parse full gateway-ctlr-name", func() { - t := testCase{ - "k8s-gateway.nginx.org/nginx-gateway/my-gateway", - "nginx-gateway", - false, - } - tester(t) - }) // should parse full gateway-ctlr-name - - It("should fail with too many path elements", func() { - t := testCase{ - "k8s-gateway.nginx.org/nginx-gateway/my-gateway/broken", - "nginx-gateway", - true, + Describe("gateway-ctlr-name validation", func() { + prepareTestCase := func(value string, expError bool) testCase { + return testCase{ + Flag: "gateway-ctlr-name", + Value: value, + ValidatorContext: GatewayControllerParam("k8s-gateway.nginx.org", "nginx-gateway"), + ExpError: expError, + } } - tester(t) - }) // should fail with too many path elements - It("should fail with too few path elements", func() { - table := []testCase{ - { - Param: "nginx-gateway/my-gateway", - Domain: "nginx-gateway", - ExpError: true, - }, - { - Param: "my-gateway", - Domain: "nginx-gateway", - ExpError: true, - }, - } + BeforeEach(func() { + mockFlags = flag.NewFlagSet("mock", flag.PanicOnError) + _ = mockFlags.String("gateway-ctlr-name", "", "mock gateway-ctlr-name") + err := mockFlags.Parse([]string{}) + Expect(err).ToNot(HaveOccurred()) + }) + AfterEach(func() { + mockFlags = nil + }) - runner(table) - }) // should fail with too few path elements + It("should parse full gateway-ctlr-name", func() { + t := prepareTestCase( + "k8s-gateway.nginx.org/nginx-gateway/my-gateway", + expectSuccess, + ) + tester(t) + }) // should parse full gateway-ctlr-name - It("should verify constraints", func() { - table := []testCase{ - { - // bad domain - Param: "invalid-domain/nginx-gateway/my-gateway", - Domain: "nginx-gateway", - ExpError: true, - }, - { - // bad domain - Param: "/default/my-gateway", - Domain: "nginx-gateway", - ExpError: true, - }, - { - // bad namespace - Param: "k8s-gateway.nginx.org/default/my-gateway", - Domain: "nginx-gateway", - ExpError: true, - }, - { - // bad namespace - Param: "k8s-gateway.nginx.org//my-gateway", - Domain: "nginx-gateway", - ExpError: true, - }, - { - // bad name - Param: "k8s-gateway.nginx.org/default/", - Domain: "nginx-gateway", - ExpError: true, - }, + It("should fail with too many path elements", func() { + t := prepareTestCase( + "k8s-gateway.nginx.org/nginx-gateway/my-gateway", + expectSuccess) + tester(t) + }) // should fail with too many path elements + + It("should fail with too few path elements", func() { + table := []testCase{ + prepareTestCase( + "nginx-gateway/my-gateway", + expectError, + ), + prepareTestCase( + "my-gateway", + expectError, + ), + } + + runner(table) + }) // should fail with too few path elements + + It("should verify constraints", func() { + table := []testCase{ + prepareTestCase( + // bad domain + "invalid-domain/nginx-gateway/my-gateway", + expectError, + ), + prepareTestCase( + // bad domain + "/default/my-gateway", + expectError, + ), + prepareTestCase( + // bad namespace + "k8s-gateway.nginx.org/default/my-gateway", + expectError), + prepareTestCase( + // bad namespace + "k8s-gateway.nginx.org//my-gateway", + expectError, + ), + prepareTestCase( + // bad name + "k8s-gateway.nginx.org/default/", + expectError, + ), + } + + runner(table) + }) // should verify constraints + }) // gateway-ctlr-name validation + + Describe("gatewayclass validation", func() { + prepareTestCase := func(value string, expError bool) testCase { + return testCase{ + Flag: "gatewayclass", + Value: value, + ValidatorContext: GatewayClassParam(), + ExpError: expError, + } } - runner(table) - }) // should verify constraints + BeforeEach(func() { + mockFlags = flag.NewFlagSet("mock", flag.PanicOnError) + _ = mockFlags.String("gatewayclass", "", "mock gatewayclass") + err := mockFlags.Parse([]string{}) + Expect(err).ToNot(HaveOccurred()) + }) + AfterEach(func() { + mockFlags = nil + }) + + It("should succeed on valid name", func() { + t := prepareTestCase( + "nginx", + expectSuccess, + ) + tester(t) + }) // should succeed on valid name + + It("should fail with invalid name", func() { + t := prepareTestCase( + "$nginx", + expectError) + tester(t) + }) // should fail with invalid name" + }) // gatewayclass validation }) // CLI argument validation }) // end Main diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index b879896fb3..d7bedf4c3b 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -37,6 +37,7 @@ rules: resources: - httproutes/status - gateways/status + - gatewayclasses/status verbs: - update --- @@ -97,6 +98,7 @@ spec: # Note: CAP_KILL is needed for sending HUP signal to NGINX main process args: - --gateway-ctlr-name=k8s-gateway.nginx.org/nginx-gateway/gateway + - --gatewayclass=nginx - image: nginx:1.21.3 imagePullPolicy: IfNotPresent name: nginx diff --git a/internal/config/config.go b/internal/config/config.go index e23d93c16c..fadddcc27f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -11,4 +11,6 @@ type Config struct { // 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 + // GatewayClassName is the name of the GatewayClass resource that the Gateway will use. + GatewayClassName string } diff --git a/internal/events/loop.go b/internal/events/loop.go index 06028a2d85..95f2c4cbdd 100644 --- a/internal/events/loop.go +++ b/internal/events/loop.go @@ -118,6 +118,8 @@ func (el *EventLoop) updateNginx(ctx context.Context, conf state.Configuration) func (el *EventLoop) propagateUpsert(e *UpsertEvent) { switch r := e.Resource.(type) { + case *v1alpha2.GatewayClass: + el.processor.CaptureUpsertChange(r) case *v1alpha2.Gateway: el.processor.CaptureUpsertChange(r) case *v1alpha2.HTTPRoute: @@ -132,6 +134,8 @@ func (el *EventLoop) propagateUpsert(e *UpsertEvent) { func (el *EventLoop) propagateDelete(e *DeleteEvent) { switch e.Type.(type) { + case *v1alpha2.GatewayClass: + el.processor.CaptureDeleteChange(e.Type, e.NamespacedName) case *v1alpha2.Gateway: el.processor.CaptureDeleteChange(e.Type, e.NamespacedName) case *v1alpha2.HTTPRoute: diff --git a/internal/events/loop_test.go b/internal/events/loop_test.go index 7758717a00..f364d0ce9f 100644 --- a/internal/events/loop_test.go +++ b/internal/events/loop_test.go @@ -112,6 +112,7 @@ var _ = Describe("EventLoop", func() { }, Entry("HTTPRoute", &events.UpsertEvent{Resource: &v1alpha2.HTTPRoute{}}), Entry("Gateway", &events.UpsertEvent{Resource: &v1alpha2.Gateway{}}), + Entry("GatewayClass", &events.UpsertEvent{Resource: &v1alpha2.GatewayClass{}}), ) DescribeTable("Delete events", @@ -141,6 +142,7 @@ var _ = Describe("EventLoop", func() { }, 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"}}), + Entry("GatewayClass", &events.DeleteEvent{Type: &v1alpha2.GatewayClass{}, NamespacedName: types.NamespacedName{Name: "class"}}), ) }) diff --git a/internal/implementations/gatewayclass/gatewayclass.go b/internal/implementations/gatewayclass/gatewayclass.go index e8ec954ef4..4bec0f11f7 100644 --- a/internal/implementations/gatewayclass/gatewayclass.go +++ b/internal/implementations/gatewayclass/gatewayclass.go @@ -1,44 +1,64 @@ 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 gatewayClassImplementation struct { - conf config.Config + logger logr.Logger + gatewayClassName string + eventCh chan<- interface{} } -func NewGatewayClassImplementation(conf config.Config) sdk.GatewayClassImpl { +func NewGatewayClassImplementation(conf config.Config, eventCh chan<- interface{}) sdk.GatewayClassImpl { return &gatewayClassImplementation{ - conf: conf, + logger: conf.Logger, + gatewayClassName: conf.GatewayClassName, + eventCh: eventCh, } } -func (impl *gatewayClassImplementation) Logger() logr.Logger { - return impl.conf.Logger -} - -func (impl *gatewayClassImplementation) ControllerName() string { - return impl.conf.GatewayCtlrName -} - func (impl *gatewayClassImplementation) Upsert(gc *v1alpha2.GatewayClass) { - if string(gc.Spec.ControllerName) != impl.ControllerName() { - impl.Logger().Info("Wrong ControllerName in the GatewayClass resource", - "expected", impl.ControllerName(), - "got", gc.Spec.ControllerName) + if gc.Name != impl.gatewayClassName { + msg := fmt.Sprintf("GatewayClass was upserted but ignored because this controller only supports the GatewayClass %s", impl.gatewayClassName) + impl.logger.Info(msg, + "name", gc.Name, + ) return } - impl.Logger().Info("Processing GatewayClass resource", + impl.eventCh <- &events.UpsertEvent{ + Resource: gc, + } + + impl.logger.Info("GatewayClass was upserted", "name", gc.Name) } -func (impl *gatewayClassImplementation) Remove(key string) { - impl.Logger().Info("GatewayClass resource was removed", - "name", key) +func (impl *gatewayClassImplementation) Remove(nsname types.NamespacedName) { + // GatewayClass is a cluster scoped resource - no namespace. + + if nsname.Name != impl.gatewayClassName { + msg := fmt.Sprintf("GatewayClass was removed but ignored because this controller only supports the GatewayClass %s", impl.gatewayClassName) + impl.logger.Info(msg, + "name", nsname.Name, + ) + return + } + + impl.logger.Info("GatewayClass was removed", + "name", nsname.Name) + + impl.eventCh <- &events.DeleteEvent{ + NamespacedName: nsname, + Type: &v1alpha2.GatewayClass{}, + } } diff --git a/internal/implementations/gatewayclass/gatewayclass_test.go b/internal/implementations/gatewayclass/gatewayclass_test.go new file mode 100644 index 0000000000..32c515256f --- /dev/null +++ b/internal/implementations/gatewayclass/gatewayclass_test.go @@ -0,0 +1,88 @@ +package implementation_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/log/zap" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/config" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/events" + implementation "github.com/nginxinc/nginx-kubernetes-gateway/internal/implementations/gatewayclass" + "github.com/nginxinc/nginx-kubernetes-gateway/pkg/sdk" +) + +var _ = Describe("GatewayClassImplementation", func() { + var ( + eventCh chan interface{} + impl sdk.GatewayClassImpl + ) + + const ( + className = "my-class" + unrelatedClassName = "not-my-class" + ) + + BeforeEach(func() { + eventCh = make(chan interface{}) + + impl = implementation.NewGatewayClassImplementation(config.Config{ + Logger: zap.New(), + GatewayClassName: className, + }, eventCh) + }) + + Describe("Implementation processes GatewayClass", func() { + It("should process upsert", func() { + gc := &v1alpha2.GatewayClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: className, + }, + } + + go func() { + impl.Upsert(gc) + }() + + Eventually(eventCh).Should(Receive(Equal(&events.UpsertEvent{Resource: gc}))) + }) + + It("should process remove", func() { + nsname := types.NamespacedName{Name: className} + + go func() { + impl.Remove(nsname) + }() + + Eventually(eventCh).Should(Receive(Equal( + &events.DeleteEvent{ + NamespacedName: nsname, + Type: &v1alpha2.GatewayClass{}, + }))) + }) + }) + + Describe("Implementation ignores unrelated GatewayClass", func() { + It("should ignore upsert", func() { + gc := &v1alpha2.GatewayClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: unrelatedClassName, + }, + } + + impl.Upsert(gc) + + Expect(eventCh).ShouldNot(Receive()) + }) + + It("should ignore remove", func() { + nsname := types.NamespacedName{Name: unrelatedClassName} + + impl.Remove(nsname) + + Expect(eventCh).ShouldNot(Receive()) + }) + }) +}) diff --git a/internal/implementations/gatewayclass/implementation_suit_test.go b/internal/implementations/gatewayclass/implementation_suit_test.go new file mode 100644 index 0000000000..5bba5d8086 --- /dev/null +++ b/internal/implementations/gatewayclass/implementation_suit_test.go @@ -0,0 +1,13 @@ +package implementation_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestState(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Implementation Suite") +} diff --git a/internal/manager/manager.go b/internal/manager/manager.go index 469155fa2e..a766319e5f 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -13,6 +13,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" + gc "github.com/nginxinc/nginx-kubernetes-gateway/internal/implementations/gatewayclass" 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" @@ -51,6 +52,10 @@ func Start(cfg config.Config) error { return fmt.Errorf("cannot build runtime manager: %w", err) } + err = sdk.RegisterGatewayClassController(mgr, gc.NewGatewayClassImplementation(cfg, eventCh)) + if err != nil { + return fmt.Errorf("cannot register gatewayclass implementation: %w", err) + } err = sdk.RegisterGatewayController(mgr, gw.NewGatewayImplementation(cfg, eventCh)) if err != nil { return fmt.Errorf("cannot register gateway implementation: %w", err) @@ -64,12 +69,12 @@ func Start(cfg config.Config) error { return fmt.Errorf("cannot register service implementation: %w", err) } - processor := state.NewChangeProcessorImpl(cfg.GatewayNsName) + processor := state.NewChangeProcessorImpl(cfg.GatewayNsName, cfg.GatewayCtlrName, cfg.GatewayClassName) serviceStore := state.NewServiceStore() configGenerator := ngxcfg.NewGeneratorImpl(serviceStore) nginxFileMgr := file.NewManagerImpl() nginxRuntimeMgr := ngxruntime.NewManagerImpl() - statusUpdater := status.NewUpdater(cfg.GatewayCtlrName, cfg.GatewayNsName, mgr.GetClient(), cfg.Logger, status.NewRealClock()) + statusUpdater := status.NewUpdater(cfg.GatewayCtlrName, cfg.GatewayNsName, cfg.GatewayClassName, mgr.GetClient(), cfg.Logger, status.NewRealClock()) eventLoop := events.NewEventLoop(processor, serviceStore, configGenerator, eventCh, cfg.Logger, nginxFileMgr, nginxRuntimeMgr, statusUpdater) err = mgr.Add(eventLoop) diff --git a/internal/state/change_processor.go b/internal/state/change_processor.go index fa88e231e3..17f8cb4d3b 100644 --- a/internal/state/change_processor.go +++ b/internal/state/change_processor.go @@ -30,18 +30,22 @@ type ChangeProcessor interface { } type ChangeProcessorImpl struct { - store *store - changed bool - gwNsName types.NamespacedName + store *store + changed bool + gwNsName types.NamespacedName + controllerName string + gcName string lock sync.Mutex } // NewChangeProcessorImpl creates a new ChangeProcessorImpl for the Gateway resource with the configured namespace name. -func NewChangeProcessorImpl(gwNsName types.NamespacedName) *ChangeProcessorImpl { +func NewChangeProcessorImpl(gwNsName types.NamespacedName, controllerName string, gcName string) *ChangeProcessorImpl { return &ChangeProcessorImpl{ - store: newStore(), - gwNsName: gwNsName, + store: newStore(), + gwNsName: gwNsName, + controllerName: controllerName, + gcName: gcName, } } @@ -52,6 +56,15 @@ func (c *ChangeProcessorImpl) CaptureUpsertChange(obj client.Object) { c.changed = true switch o := obj.(type) { + case *v1alpha2.GatewayClass: + if o.Name != c.gcName { + panic(fmt.Errorf("gatewayclass resource must be %s, got %s", c.gcName, o.Name)) + } + // if the resource spec hasn't changed (its generation is the same), ignore the upsert + if c.store.gc != nil && c.store.gc.Generation == o.Generation { + c.changed = false + } + c.store.gc = o 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)) @@ -80,6 +93,11 @@ func (c *ChangeProcessorImpl) CaptureDeleteChange(resourceType client.Object, ns c.changed = true switch o := resourceType.(type) { + case *v1alpha2.GatewayClass: + if nsname.Name != c.gcName { + panic(fmt.Errorf("gatewayclass resource must be %s, got %s", c.gcName, nsname.Name)) + } + c.store.gc = nil 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)) @@ -102,7 +120,7 @@ func (c *ChangeProcessorImpl) Process() (changed bool, conf Configuration, statu c.changed = false - graph := buildGraph(c.store, c.gwNsName) + graph := buildGraph(c.store, c.gwNsName, c.controllerName, c.gcName) conf = buildConfiguration(graph) statuses = buildStatuses(graph) diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go index c72b61568a..550d40ec29 100644 --- a/internal/state/change_processor_test.go +++ b/internal/state/change_processor_test.go @@ -14,13 +14,32 @@ import ( var _ = Describe("ChangeProcessor", func() { Describe("Normal cases of processing changes", func() { + const ( + controllerName = "my.controller" + gcName = "test-class" + ) + var ( + gc, gcUpdated *v1alpha2.GatewayClass hr1, hr1Updated, hr2 *v1alpha2.HTTPRoute gw, gwUpdated *v1alpha2.Gateway processor state.ChangeProcessor ) BeforeEach(OncePerOrdered, func() { + gc = &v1alpha2.GatewayClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: gcName, + Generation: 1, + }, + Spec: v1alpha2.GatewayClassSpec{ + ControllerName: controllerName, + }, + } + + gcUpdated = gc.DeepCopy() + gcUpdated.Generation++ + createRoute := func(name string, hostname string) *v1alpha2.HTTPRoute { return &v1alpha2.HTTPRoute{ ObjectMeta: metav1.ObjectMeta{ @@ -68,6 +87,7 @@ var _ = Describe("ChangeProcessor", func() { Name: "gateway", }, Spec: v1alpha2.GatewaySpec{ + GatewayClassName: gcName, Listeners: []v1alpha2.Listener{ { Name: "listener-80-1", @@ -82,7 +102,7 @@ var _ = Describe("ChangeProcessor", func() { gwUpdated = gw.DeepCopy() gwUpdated.Generation++ - processor = state.NewChangeProcessorImpl(types.NamespacedName{Namespace: "test", Name: "gateway"}) + processor = state.NewChangeProcessorImpl(types.NamespacedName{Namespace: "test", Name: "gateway"}, controllerName, gcName) }) Describe("Process resources", Ordered, func() { @@ -93,10 +113,10 @@ var _ = Describe("ChangeProcessor", func() { Expect(statuses).To(BeZero()) }) - It("should return empty configuration and updated statuses after upserting an HTTPRoute when the Gateway doesn't exist", func() { + It("should return empty configuration and updated statuses after upserting an HTTPRoute when the Gateway and GatewayClass don't exist", func() { processor.CaptureUpsertChange(hr1) - expectedConf := state.Configuration{HTTPServers: []state.HTTPServer{}} + expectedConf := state.Configuration{} expectedStatuses := state.Statuses{ ListenerStatuses: map[string]state.ListenerStatus{}, HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ @@ -114,9 +134,35 @@ var _ = Describe("ChangeProcessor", func() { Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) }) - It("should return updated configuration and statuses after the Gateway is upserted", func() { + It("should return empty configuration and updated statuses after upserting the Gateway when the GatewayClass doesn't exist", func() { processor.CaptureUpsertChange(gw) + expectedConf := state.Configuration{} + expectedStatuses := state.Statuses{ + ListenerStatuses: map[string]state.ListenerStatus{ + "listener-80-1": { + Valid: false, + AttachedRoutes: 1, + }, + }, + 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 GatewayClass is upserted", func() { + processor.CaptureUpsertChange(gc) + expectedConf := state.Configuration{ HTTPServers: []state.HTTPServer{ { @@ -137,6 +183,10 @@ var _ = Describe("ChangeProcessor", func() { }, } expectedStatuses := state.Statuses{ + GatewayClassStatus: &state.GatewayClassStatus{ + Valid: true, + ObservedGeneration: gc.Generation, + }, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { Valid: true, @@ -192,6 +242,10 @@ var _ = Describe("ChangeProcessor", func() { }, } expectedStatuses := state.Statuses{ + GatewayClassStatus: &state.GatewayClassStatus{ + Valid: true, + ObservedGeneration: gc.Generation, + }, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { Valid: true, @@ -247,6 +301,69 @@ var _ = Describe("ChangeProcessor", func() { }, } expectedStatuses := state.Statuses{ + GatewayClassStatus: &state.GatewayClassStatus{ + Valid: true, + ObservedGeneration: gc.Generation, + }, + 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 GatewayClass without generation change", func() { + gcUpdatedSameGen := gc.DeepCopy() + // gcUpdatedSameGen.Generation has not been changed + processor.CaptureUpsertChange(gcUpdatedSameGen) + + 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 GatewayClass with generation change", func() { + processor.CaptureUpsertChange(gcUpdated) + + 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{ + GatewayClassStatus: &state.GatewayClassStatus{ + Valid: true, + ObservedGeneration: gcUpdated.Generation, + }, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { Valid: true, @@ -314,6 +431,10 @@ var _ = Describe("ChangeProcessor", func() { }, } expectedStatuses := state.Statuses{ + GatewayClassStatus: &state.GatewayClassStatus{ + Valid: true, + ObservedGeneration: gcUpdated.Generation, + }, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { Valid: true, @@ -363,6 +484,10 @@ var _ = Describe("ChangeProcessor", func() { }, } expectedStatuses := state.Statuses{ + GatewayClassStatus: &state.GatewayClassStatus{ + Valid: true, + ObservedGeneration: gcUpdated.Generation, + }, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { Valid: true, @@ -384,10 +509,36 @@ var _ = Describe("ChangeProcessor", func() { Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) }) + It("should return empty configuration and updated statuses after deleting the GatewayClass", func() { + processor.CaptureDeleteChange(&v1alpha2.GatewayClass{}, types.NamespacedName{Name: gcName}) + + expectedConf := state.Configuration{} + expectedStatuses := state.Statuses{ + ListenerStatuses: map[string]state.ListenerStatus{ + "listener-80-1": { + Valid: false, + AttachedRoutes: 1, + }, + }, + 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 updated statuses after deleting the Gateway", func() { processor.CaptureDeleteChange(&v1alpha2.Gateway{}, types.NamespacedName{Namespace: "test", Name: "gateway"}) - expectedConf := state.Configuration{HTTPServers: []state.HTTPServer{}} + expectedConf := state.Configuration{} expectedStatuses := state.Statuses{ ListenerStatuses: map[string]state.ListenerStatus{}, HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ @@ -408,7 +559,7 @@ var _ = Describe("ChangeProcessor", func() { 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{}} + expectedConf := state.Configuration{} expectedStatuses := state.Statuses{ ListenerStatuses: map[string]state.ListenerStatus{}, HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{}, @@ -426,7 +577,7 @@ var _ = Describe("ChangeProcessor", func() { var processor state.ChangeProcessor BeforeEach(func() { - processor = state.NewChangeProcessorImpl(types.NamespacedName{Namespace: "test", Name: "gateway"}) + processor = state.NewChangeProcessorImpl(types.NamespacedName{Namespace: "test", Name: "gateway"}, "test.controller", "my-class") }) DescribeTable("CaptureUpsertChange must panic", @@ -437,7 +588,8 @@ var _ = Describe("ChangeProcessor", func() { 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"}})) + Entry("a wrong gateway", &v1alpha2.Gateway{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: "other-gateway"}}), + Entry("a wrong gatewayclass", &v1alpha2.GatewayClass{ObjectMeta: metav1.ObjectMeta{Name: "wrong-class"}})) DescribeTable("CaptureDeleteChange must panic", func(resourceType client.Object, nsname types.NamespacedName) { @@ -447,6 +599,7 @@ var _ = Describe("ChangeProcessor", func() { 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"})) + Entry("a wrong gateway", &v1alpha2.Gateway{}, types.NamespacedName{Namespace: "test", Name: "other-gateway"}), + Entry("a wrong gatewayclass", &v1alpha2.GatewayClass{}, types.NamespacedName{Name: "wrong-class"})) }) }) diff --git a/internal/state/configuration.go b/internal/state/configuration.go index 3cced8eb79..9af96edcfa 100644 --- a/internal/state/configuration.go +++ b/internal/state/configuration.go @@ -50,6 +50,10 @@ func (r *MatchRule) GetMatch() v1alpha2.HTTPRouteMatch { // buildConfiguration builds the Configuration from the graph. func buildConfiguration(graph *graph) Configuration { + if graph.GatewayClass == nil || !graph.GatewayClass.Valid { + return Configuration{} + } + // FIXME(pleshakov) For now we only handle paths with prefix matches. Handle exact and regex matches pathRulesForHosts := make(map[string]map[string]PathRule) diff --git a/internal/state/configuration_test.go b/internal/state/configuration_test.go index 6dcbf50f6e..fd5ac968cf 100644 --- a/internal/state/configuration_test.go +++ b/internal/state/configuration_test.go @@ -96,16 +96,24 @@ func TestBuildConfiguration(t *testing.T) { }{ { graph: &graph{ + GatewayClass: &gatewayClass{ + Source: &v1alpha2.GatewayClass{}, + Valid: true, + }, Listeners: map[string]*listener{}, Routes: map[types.NamespacedName]*route{}, }, expected: Configuration{ HTTPServers: []HTTPServer{}, }, - msg: "empty graph", + msg: "no listeners and routes", }, { graph: &graph{ + GatewayClass: &gatewayClass{ + Source: &v1alpha2.GatewayClass{}, + Valid: true, + }, Listeners: map[string]*listener{ "listener-80-1": { Valid: true, @@ -122,6 +130,10 @@ func TestBuildConfiguration(t *testing.T) { }, { graph: &graph{ + GatewayClass: &gatewayClass{ + Source: &v1alpha2.GatewayClass{}, + Valid: true, + }, Listeners: map[string]*listener{ "listener-80-1": { Valid: true, @@ -178,6 +190,10 @@ func TestBuildConfiguration(t *testing.T) { }, { graph: &graph{ + GatewayClass: &gatewayClass{ + Source: &v1alpha2.GatewayClass{}, + Valid: true, + }, Listeners: map[string]*listener{ "listener-80-1": { Valid: true, @@ -241,6 +257,52 @@ func TestBuildConfiguration(t *testing.T) { }, msg: "one listener with two routes with the same hostname with and without collisions", }, + { + graph: &graph{ + GatewayClass: &gatewayClass{ + Source: &v1alpha2.GatewayClass{}, + Valid: false, + ErrorMsg: "error", + }, + Listeners: map[string]*listener{ + "listener-80-1": { + 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, + }, + }, + expected: Configuration{}, + msg: "invalid gatewayclass", + }, + { + graph: &graph{ + GatewayClass: nil, + Listeners: map[string]*listener{ + "listener-80-1": { + 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, + }, + }, + expected: Configuration{}, + msg: "missing gatewayclass", + }, } for _, test := range tests { diff --git a/internal/state/graph.go b/internal/state/graph.go index 26f59f1eae..fa4be1f41d 100644 --- a/internal/state/graph.go +++ b/internal/state/graph.go @@ -1,6 +1,8 @@ package state import ( + "fmt" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/gateway-api/apis/v1alpha2" ) @@ -34,9 +36,21 @@ type route struct { InvalidSectionNameRefs map[string]struct{} } +// gatewayClass represents the GatewayClass resource. +type gatewayClass struct { + // Source is the source resource. + Source *v1alpha2.GatewayClass + // Valid shows whether the GatewayClass is valid. + Valid bool + // ErrorMsg explains the error when the resource is not valid. + ErrorMsg string +} + // graph is a graph-like representation of Gateway API resources. // It is assumed that the Gateway resource is a single resource. type graph struct { + // GatewayClass holds the GatewayClass resource. + GatewayClass *gatewayClass // Listeners holds listeners keyed by their names in the Gateway resource. Listeners map[string]*listener // Routes holds route resources. @@ -44,8 +58,10 @@ type graph struct { } // 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) +func buildGraph(store *store, gwNsName types.NamespacedName, controllerName string, gcName string) *graph { + gc := buildGatewayClass(store.gc, controllerName) + + listeners := buildListeners(store.gw, gcName) routes := make(map[types.NamespacedName]*route) for _, ghr := range store.httpRoutes { @@ -56,8 +72,28 @@ func buildGraph(store *store, gwNsName types.NamespacedName) *graph { } return &graph{ - Listeners: listeners, - Routes: routes, + GatewayClass: gc, + Listeners: listeners, + Routes: routes, + } +} + +func buildGatewayClass(gc *v1alpha2.GatewayClass, controllerName string) *gatewayClass { + if gc == nil { + return nil + } + + var errorMsg string + + err := validateGatewayClass(gc, controllerName) + if err != nil { + errorMsg = err.Error() + } + + return &gatewayClass{ + Source: gc, + Valid: err == nil, + ErrorMsg: errorMsg, } } @@ -176,12 +212,11 @@ func ignoreParentRef(p v1alpha2.ParentRef, hrNamespace string, gwNsName types.Na return false } -func buildListeners(gw *v1alpha2.Gateway) map[string]*listener { +func buildListeners(gw *v1alpha2.Gateway, gcName string) 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 { + if gw == nil || string(gw.Spec.GatewayClassName) != gcName { return listeners } @@ -223,3 +258,11 @@ func getHostname(h *v1alpha2.Hostname) string { } return string(*h) } + +func validateGatewayClass(gc *v1alpha2.GatewayClass, controllerName string) error { + if string(gc.Spec.ControllerName) != controllerName { + return fmt.Errorf("Spec.ControllerName must be %s got %s", controllerName, gc.Spec.ControllerName) + } + + return nil +} diff --git a/internal/state/graph_test.go b/internal/state/graph_test.go index c353772371..a25f284031 100644 --- a/internal/state/graph_test.go +++ b/internal/state/graph_test.go @@ -12,6 +12,10 @@ import ( ) func TestBuildGraph(t *testing.T) { + const ( + gcName = "my-class" + controllerName = "my.controller" + ) createRoute := func(name string, gatewayName string) *v1alpha2.HTTPRoute { return &v1alpha2.HTTPRoute{ ObjectMeta: metav1.ObjectMeta{ @@ -29,7 +33,7 @@ func TestBuildGraph(t *testing.T) { }, }, Hostnames: []v1alpha2.Hostname{ - v1alpha2.Hostname("foo.example.com"), + "foo.example.com", }, Rules: []v1alpha2.HTTPRouteRule{ { @@ -49,12 +53,18 @@ func TestBuildGraph(t *testing.T) { hr2 := createRoute("hr-2", "wrong-gateway") store := &store{ + gc: &v1alpha2.GatewayClass{ + Spec: v1alpha2.GatewayClassSpec{ + ControllerName: controllerName, + }, + }, gw: &v1alpha2.Gateway{ ObjectMeta: metav1.ObjectMeta{ Namespace: "test", Name: "gateway", }, Spec: v1alpha2.GatewaySpec{ + GatewayClassName: gcName, Listeners: []v1alpha2.Listener{ { Name: "listener-80-1", @@ -81,6 +91,10 @@ func TestBuildGraph(t *testing.T) { InvalidSectionNameRefs: map[string]struct{}{}, } expected := &graph{ + GatewayClass: &gatewayClass{ + Source: store.gc, + Valid: true, + }, Listeners: map[string]*listener{ "listener-80-1": { Source: store.gw.Spec.Listeners[0], @@ -98,13 +112,67 @@ func TestBuildGraph(t *testing.T) { }, } - result := buildGraph(store, gwNsName) + result := buildGraph(store, gwNsName, controllerName, gcName) if diff := cmp.Diff(expected, result); diff != "" { t.Errorf("buildGraph() mismatch (-want +got):\n%s", diff) } } +func TestBuildGatewayClass(t *testing.T) { + const controllerName = "my.controller" + + validGC := &v1alpha2.GatewayClass{ + Spec: v1alpha2.GatewayClassSpec{ + ControllerName: "my.controller", + }, + } + invalidGC := &v1alpha2.GatewayClass{ + Spec: v1alpha2.GatewayClassSpec{ + ControllerName: "wrong.controller", + }, + } + + tests := []struct { + gc *v1alpha2.GatewayClass + expected *gatewayClass + msg string + }{ + { + gc: nil, + expected: nil, + msg: "no gatewayclass", + }, + { + gc: validGC, + expected: &gatewayClass{ + Source: validGC, + Valid: true, + ErrorMsg: "", + }, + msg: "valid gatewayclass", + }, + { + gc: invalidGC, + expected: &gatewayClass{ + Source: invalidGC, + Valid: false, + ErrorMsg: "Spec.ControllerName must be my.controller got wrong.controller", + }, + msg: "invalid gatewayclass", + }, + } + + for _, test := range tests { + result := buildGatewayClass(test.gc, controllerName) + if diff := cmp.Diff(test.expected, result); diff != "" { + t.Errorf("buildGatewayClass() '%s' mismatch (-want +got):\n%s", test.msg, diff) + } + } +} + func TestBuildListeners(t *testing.T) { + const gcName = "my-gateway-class" + listener801 := v1alpha2.Listener{ Name: "listener-80-1", Hostname: (*v1alpha2.Hostname)(helpers.GetStringPointer("foo.example.com")), @@ -138,6 +206,7 @@ func TestBuildListeners(t *testing.T) { { gateway: &v1alpha2.Gateway{ Spec: v1alpha2.GatewaySpec{ + GatewayClassName: gcName, Listeners: []v1alpha2.Listener{ listener801, }, @@ -156,6 +225,7 @@ func TestBuildListeners(t *testing.T) { { gateway: &v1alpha2.Gateway{ Spec: v1alpha2.GatewaySpec{ + GatewayClassName: gcName, Listeners: []v1alpha2.Listener{ listener802, }, @@ -174,6 +244,7 @@ func TestBuildListeners(t *testing.T) { { gateway: &v1alpha2.Gateway{ Spec: v1alpha2.GatewaySpec{ + GatewayClassName: gcName, Listeners: []v1alpha2.Listener{ listener801, listener803, }, @@ -198,6 +269,7 @@ func TestBuildListeners(t *testing.T) { { gateway: &v1alpha2.Gateway{ Spec: v1alpha2.GatewaySpec{ + GatewayClassName: gcName, Listeners: []v1alpha2.Listener{ listener801, listener804, }, @@ -224,10 +296,22 @@ func TestBuildListeners(t *testing.T) { expected: map[string]*listener{}, msg: "no gateway", }, + { + gateway: &v1alpha2.Gateway{ + Spec: v1alpha2.GatewaySpec{ + GatewayClassName: "wrong-class", + Listeners: []v1alpha2.Listener{ + listener801, listener804, + }, + }, + }, + expected: map[string]*listener{}, + msg: "wrong gatewayclass", + }, } for _, test := range tests { - result := buildListeners(test.gateway) + result := buildListeners(test.gateway, gcName) if diff := cmp.Diff(test.expected, result); diff != "" { t.Errorf("buildListeners() %q mismatch (-want +got):\n%s", test.msg, diff) } @@ -613,3 +697,21 @@ func TestGetHostname(t *testing.T) { } } } + +func TestValidateGatewayClass(t *testing.T) { + gc := &v1alpha2.GatewayClass{ + Spec: v1alpha2.GatewayClassSpec{ + ControllerName: "test.controller", + }, + } + + err := validateGatewayClass(gc, "test.controller") + if err != nil { + t.Errorf("validateGatewayClass() returned unexpected error %v", err) + } + + err = validateGatewayClass(gc, "unmatched.controller") + if err == nil { + t.Errorf("validateGatewayClass() didn't return an error") + } +} diff --git a/internal/state/statuses.go b/internal/state/statuses.go index adcc8d455f..9bb9775b91 100644 --- a/internal/state/statuses.go +++ b/internal/state/statuses.go @@ -11,8 +11,9 @@ 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 + GatewayClassStatus *GatewayClassStatus + ListenerStatuses ListenerStatuses + HTTPRouteStatuses HTTPRouteStatuses } // ListenerStatus holds the status-related information about a listener in the Gateway resource. @@ -36,6 +37,16 @@ type ParentStatus struct { Attached bool } +// GatewayClassStatus holds status-related infortmation about the GatewayClass resource. +type GatewayClassStatus struct { + // Valid shows if the resource is valid. + Valid bool + // ErrorMsg describe the error when the resource is invalid. + ErrorMsg string + // ObservedGeneration is the generation of the resource that was processed. + ObservedGeneration int64 +} + // buildStatuses builds statuses from a graph. func buildStatuses(graph *graph) Statuses { statuses := Statuses{ @@ -43,9 +54,19 @@ func buildStatuses(graph *graph) Statuses { HTTPRouteStatuses: make(map[types.NamespacedName]HTTPRouteStatus), } + if graph.GatewayClass != nil { + statuses.GatewayClassStatus = &GatewayClassStatus{ + Valid: graph.GatewayClass.Valid, + ErrorMsg: graph.GatewayClass.ErrorMsg, + ObservedGeneration: graph.GatewayClass.Source.Generation, + } + } + + gcValidAndExist := graph.GatewayClass != nil && graph.GatewayClass.Valid + for name, l := range graph.Listeners { statuses.ListenerStatuses[name] = ListenerStatus{ - Valid: l.Valid, + Valid: l.Valid && gcValidAndExist, AttachedRoutes: int32(len(l.Routes)), } } @@ -55,7 +76,7 @@ func buildStatuses(graph *graph) Statuses { for ref := range r.ValidSectionNameRefs { parentStatuses[ref] = ParentStatus{ - Attached: true, + Attached: gcValidAndExist, // Attached only when GatewayClass is valid and exists } } for ref := range r.InvalidSectionNameRefs { diff --git a/internal/state/statuses_test.go b/internal/state/statuses_test.go index 35ff58edaf..ebc989cfa0 100644 --- a/internal/state/statuses_test.go +++ b/internal/state/statuses_test.go @@ -4,53 +4,146 @@ 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" ) 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"}: {}}, + 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": {}, }, }, - Routes: map[types.NamespacedName]*route{ - {Namespace: "test", Name: "hr-1"}: { - ValidSectionNameRefs: map[string]struct{}{ - "listener-80-1": {}, + } + + tests := []struct { + graph *graph + expected Statuses + msg string + }{ + { + graph: &graph{ + GatewayClass: &gatewayClass{ + Source: &v1alpha2.GatewayClass{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + }, + Valid: true, }, - InvalidSectionNameRefs: map[string]struct{}{ - "listener-80-2": {}, + Listeners: listeners, + Routes: routes, + }, + expected: Statuses{ + GatewayClassStatus: &GatewayClassStatus{ + Valid: true, + ObservedGeneration: 1, + }, + 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, + }, + }, + }, }, }, + msg: "normal case", }, - } - - expected := Statuses{ - ListenerStatuses: map[string]ListenerStatus{ - "listener-80-1": { - Valid: true, - AttachedRoutes: 1, + { + graph: &graph{ + GatewayClass: nil, + Listeners: listeners, + Routes: routes, }, + expected: Statuses{ + ListenerStatuses: map[string]ListenerStatus{ + "listener-80-1": { + Valid: false, + AttachedRoutes: 1, + }, + }, + HTTPRouteStatuses: map[types.NamespacedName]HTTPRouteStatus{ + {Namespace: "test", Name: "hr-1"}: { + ParentStatuses: map[string]ParentStatus{ + "listener-80-1": { + Attached: false, + }, + "listener-80-2": { + Attached: false, + }, + }, + }, + }, + }, + msg: "gatewayclass doesn't exist", }, - HTTPRouteStatuses: map[types.NamespacedName]HTTPRouteStatus{ - {Namespace: "test", Name: "hr-1"}: { - ParentStatuses: map[string]ParentStatus{ + { + graph: &graph{ + GatewayClass: &gatewayClass{ + Source: &v1alpha2.GatewayClass{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + }, + Valid: false, + ErrorMsg: "error", + }, + Listeners: listeners, + Routes: routes, + }, + expected: Statuses{ + GatewayClassStatus: &GatewayClassStatus{ + Valid: false, + ErrorMsg: "error", + ObservedGeneration: 1, + }, + ListenerStatuses: map[string]ListenerStatus{ "listener-80-1": { - Attached: true, + Valid: false, + AttachedRoutes: 1, }, - "listener-80-2": { - Attached: false, + }, + HTTPRouteStatuses: map[types.NamespacedName]HTTPRouteStatus{ + {Namespace: "test", Name: "hr-1"}: { + ParentStatuses: map[string]ParentStatus{ + "listener-80-1": { + Attached: false, + }, + "listener-80-2": { + Attached: false, + }, + }, }, }, }, + msg: "gatewayclass is not valid", }, } - result := buildStatuses(g) - if diff := cmp.Diff(expected, result); diff != "" { - t.Errorf("buildStatuses() mismatch (-want +got):\n%s", diff) + for _, test := range tests { + result := buildStatuses(test.graph) + if diff := cmp.Diff(test.expected, result); diff != "" { + t.Errorf("buildStatuses() '%v' mismatch (-want +got):\n%s", test.msg, diff) + } } } diff --git a/internal/state/store.go b/internal/state/store.go index 73e8a66c52..845612c765 100644 --- a/internal/state/store.go +++ b/internal/state/store.go @@ -7,6 +7,7 @@ import ( // store contains the resources that represent the state of the Gateway. type store struct { + gc *v1alpha2.GatewayClass gw *v1alpha2.Gateway httpRoutes map[types.NamespacedName]*v1alpha2.HTTPRoute } diff --git a/internal/status/gatewayclass.go b/internal/status/gatewayclass.go new file mode 100644 index 0000000000..c2e53afb63 --- /dev/null +++ b/internal/status/gatewayclass.go @@ -0,0 +1,39 @@ +package status + +import ( + "fmt" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" +) + +// prepareGatewayClassStatus prepares the status for the GatewayClass resource. +func prepareGatewayClassStatus(status state.GatewayClassStatus, transitionTime metav1.Time) v1alpha2.GatewayClassStatus { + var ( + condStatus metav1.ConditionStatus + msg string + ) + + if status.Valid { + condStatus = metav1.ConditionTrue + msg = "GatewayClass has been accepted" + } else { + condStatus = metav1.ConditionFalse + msg = fmt.Sprintf("GatewayClass has been rejected: %s", status.ErrorMsg) + } + + cond := metav1.Condition{ + Type: string(v1alpha2.GatewayClassConditionStatusAccepted), + Status: condStatus, + ObservedGeneration: status.ObservedGeneration, + LastTransitionTime: transitionTime, + Reason: string(v1alpha2.GatewayClassReasonAccepted), + Message: msg, + } + + return v1alpha2.GatewayClassStatus{ + Conditions: []metav1.Condition{cond}, + } +} diff --git a/internal/status/gatewayclass_test.go b/internal/status/gatewayclass_test.go new file mode 100644 index 0000000000..1f1e0eea67 --- /dev/null +++ b/internal/status/gatewayclass_test.go @@ -0,0 +1,69 @@ +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 TestPrepareGatewayClassStatus(t *testing.T) { + transitionTime := metav1.NewTime(time.Now()) + + tests := []struct { + status state.GatewayClassStatus + expected v1alpha2.GatewayClassStatus + msg string + }{ + { + status: state.GatewayClassStatus{ + Valid: true, + ObservedGeneration: 1, + }, + expected: v1alpha2.GatewayClassStatus{ + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.GatewayClassConditionStatusAccepted), + Status: metav1.ConditionTrue, + ObservedGeneration: 1, + LastTransitionTime: transitionTime, + Reason: string(v1alpha2.GatewayClassReasonAccepted), + Message: "GatewayClass has been accepted", + }, + }, + }, + msg: "valid GatewayClass", + }, + { + status: state.GatewayClassStatus{ + Valid: false, + ErrorMsg: "error", + ObservedGeneration: 2, + }, + expected: v1alpha2.GatewayClassStatus{ + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.GatewayClassConditionStatusAccepted), + Status: metav1.ConditionFalse, + ObservedGeneration: 2, + LastTransitionTime: transitionTime, + Reason: string(v1alpha2.GatewayClassReasonAccepted), + Message: "GatewayClass has been rejected: error", + }, + }, + }, + msg: "invalid GatewayClass", + }, + } + + for _, test := range tests { + result := prepareGatewayClassStatus(test.status, transitionTime) + if diff := cmp.Diff(test.expected, result); diff != "" { + t.Errorf("prepareGatewayClassStatus() '%s' mismatch (-want +got):\n%s", test.msg, diff) + } + } +} diff --git a/internal/status/updater.go b/internal/status/updater.go index a3fdf535c7..df19f2bac9 100644 --- a/internal/status/updater.go +++ b/internal/status/updater.go @@ -60,6 +60,7 @@ type Updater interface { type updaterImpl struct { gatewayCtlrName string gwNsName types.NamespacedName + gcName string client client.Client logger logr.Logger clock Clock @@ -69,6 +70,7 @@ type updaterImpl struct { func NewUpdater( gatewayCtlrName string, gwNsName types.NamespacedName, + gcName string, client client.Client, logger logr.Logger, clock Clock, @@ -76,6 +78,7 @@ func NewUpdater( return &updaterImpl{ gatewayCtlrName: gatewayCtlrName, gwNsName: gwNsName, + gcName: gcName, client: client, logger: logger.WithName("statusUpdater"), clock: clock, @@ -86,6 +89,13 @@ 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. + if statuses.GatewayClassStatus != nil { + upd.update(ctx, types.NamespacedName{Name: upd.gcName}, &v1alpha2.GatewayClass{}, func(object client.Object) { + gc := object.(*v1alpha2.GatewayClass) + gc.Status = prepareGatewayClassStatus(*statuses.GatewayClassStatus, upd.clock.Now()) + }) + } + upd.update(ctx, upd.gwNsName, &v1alpha2.Gateway{}, func(object client.Object) { gw := object.(*v1alpha2.Gateway) gw.Status = prepareGatewayStatus(statuses.ListenerStatuses, upd.clock.Now()) diff --git a/internal/status/updater_test.go b/internal/status/updater_test.go index 0cc4fb4672..6f3a8c62f4 100644 --- a/internal/status/updater_test.go +++ b/internal/status/updater_test.go @@ -22,6 +22,8 @@ import ( ) var _ = Describe("Updater", func() { + const gcName = "my-class" + var ( updater status.Updater client client.Client @@ -49,27 +51,28 @@ var _ = Describe("Updater", func() { gatewayCtrlName = "test.example.com" gwNsName = types.NamespacedName{Namespace: "test", Name: "gateway"} - updater = status.NewUpdater(gatewayCtrlName, gwNsName, client, zap.New(), fakeClock) + updater = status.NewUpdater(gatewayCtrlName, gwNsName, gcName, client, zap.New(), fakeClock) }) Describe("Process status updates", Ordered, func() { var ( - hr *v1alpha2.HTTPRoute + gc *v1alpha2.GatewayClass gw *v1alpha2.Gateway + hr *v1alpha2.HTTPRoute - createStatuses func(bool, bool) state.Statuses - createExpectedHR func() *v1alpha2.HTTPRoute + createStatuses func(bool) state.Statuses + createExpectedGc func(metav1.ConditionStatus, string, string) *v1alpha2.GatewayClass createExpectedGw func(metav1.ConditionStatus, string) *v1alpha2.Gateway + createExpectedHR func() *v1alpha2.HTTPRoute ) BeforeAll(func() { - hr = &v1alpha2.HTTPRoute{ + gc = &v1alpha2.GatewayClass{ ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "route1", + Name: gcName, }, TypeMeta: metav1.TypeMeta{ - Kind: "HTTPRoute", + Kind: "GatewayClass", APIVersion: "gateway.networking.k8s.io/v1alpha2", }, } @@ -83,12 +86,32 @@ var _ = Describe("Updater", func() { APIVersion: "gateway.networking.k8s.io/v1alpha2", }, } + hr = &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "route1", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "HTTPRoute", + APIVersion: "gateway.networking.k8s.io/v1alpha2", + }, + } + + createStatuses = func(valid bool) state.Statuses { + var gcErrorMsg string + if !valid { + gcErrorMsg = "error" + } - createStatuses = func(listenerValid, routeAttached bool) state.Statuses { return state.Statuses{ + GatewayClassStatus: &state.GatewayClassStatus{ + Valid: valid, + ErrorMsg: gcErrorMsg, + ObservedGeneration: 1, + }, ListenerStatuses: map[string]state.ListenerStatus{ "http": { - Valid: listenerValid, + Valid: valid, AttachedRoutes: 1, }, }, @@ -96,7 +119,7 @@ var _ = Describe("Updater", func() { {Namespace: "test", Name: "route1"}: { ParentStatuses: map[string]state.ParentStatus{ "http": { - Attached: routeAttached, + Attached: valid, }, }, }, @@ -104,97 +127,122 @@ var _ = Describe("Updater", func() { } } - createExpectedHR = func() *v1alpha2.HTTPRoute { - return &v1alpha2.HTTPRoute{ + createExpectedGc = func(status metav1.ConditionStatus, reason string, msg string) *v1alpha2.GatewayClass { + return &v1alpha2.GatewayClass{ ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "route1", + Name: gcName, }, TypeMeta: metav1.TypeMeta{ - Kind: "HTTPRoute", + Kind: "GatewayClass", APIVersion: "gateway.networking.k8s.io/v1alpha2", }, - Status: gatewayv1alpha2.HTTPRouteStatus{ - RouteStatus: gatewayv1alpha2.RouteStatus{ - Parents: []gatewayv1alpha2.RouteParentStatus{ - { - 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(gatewayv1alpha2.ConditionRouteAccepted), - Status: metav1.ConditionTrue, - ObservedGeneration: 123, - LastTransitionTime: fakeClockTime, - Reason: "Accepted", - }, - }, - }, + Status: v1alpha2.GatewayClassStatus{ + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.GatewayClassConditionStatusAccepted), + Status: status, + ObservedGeneration: 1, + LastTransitionTime: fakeClockTime, + Reason: string(v1alpha2.GatewayClassReasonAccepted), + Message: msg, }, }, }, } } + }) - 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", + 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{ + { + Name: "http", + SupportedKinds: []v1alpha2.RouteGroupKind{ + { + Kind: "HTTPRoute", + }, + }, + AttachedRoutes: 1, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ListenerConditionReady), + Status: status, + ObservedGeneration: 123, + LastTransitionTime: fakeClockTime, + Reason: reason, + }, + }, + }, }, - Status: v1alpha2.GatewayStatus{ - Listeners: []gatewayv1alpha2.ListenerStatus{ + }, + } + } + + 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{ { - Name: "http", - SupportedKinds: []v1alpha2.RouteGroupKind{ - { - Kind: "HTTPRoute", - }, + ControllerName: gatewayv1alpha2.GatewayController(gatewayCtrlName), + ParentRef: gatewayv1alpha2.ParentRef{ + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: "gateway", + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("http")), }, - AttachedRoutes: 1, Conditions: []metav1.Condition{ { - Type: string(v1alpha2.ListenerConditionReady), - Status: status, + Type: string(gatewayv1alpha2.ConditionRouteAccepted), + Status: metav1.ConditionTrue, ObservedGeneration: 123, LastTransitionTime: fakeClockTime, - Reason: reason, + Reason: "Accepted", }, }, }, }, }, - } + }, } - }) + } It("should create resources in the API server", func() { - Expect(client.Create(context.Background(), hr)).Should(Succeed()) + Expect(client.Create(context.Background(), gc)).Should(Succeed()) Expect(client.Create(context.Background(), gw)).Should(Succeed()) + Expect(client.Create(context.Background(), hr)).Should(Succeed()) }) It("should update statuses", func() { - updater.Update(context.Background(), createStatuses(true, true)) + updater.Update(context.Background(), createStatuses(true)) }) - It("should have the updated status of HTTPRoute in the API server", func() { - latestHR := &v1alpha2.HTTPRoute{} - expectedHR := createExpectedHR() + It("should have the updated status of GatewayClass in the API server", func() { + latestGc := &v1alpha2.GatewayClass{} + expectedGc := createExpectedGc(metav1.ConditionTrue, string(v1alpha2.GatewayClassConditionStatusAccepted), "GatewayClass has been accepted") - err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "route1"}, latestHR) + err := client.Get(context.Background(), types.NamespacedName{Name: gcName}, latestGc) Expect(err).Should(Not(HaveOccurred())) - expectedHR.ResourceVersion = latestHR.ResourceVersion // updating the status changes the ResourceVersion + expectedGc.ResourceVersion = latestGc.ResourceVersion // updating the status changes the ResourceVersion - Expect(helpers.Diff(expectedHR, latestHR)).To(BeEmpty()) + Expect(helpers.Diff(expectedGc, latestGc)).To(BeEmpty()) }) It("should have the updated status of Gateway in the API server", func() { @@ -209,13 +257,7 @@ var _ = Describe("Updater", func() { Expect(helpers.Diff(expectedGw, latestGw)).To(BeEmpty()) }) - 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() { + It("should have the updated status of HTTPRoute in the API server", func() { latestHR := &v1alpha2.HTTPRoute{} expectedHR := createExpectedHR() @@ -227,6 +269,24 @@ var _ = Describe("Updater", func() { Expect(helpers.Diff(expectedHR, latestHR)).To(BeEmpty()) }) + It("should update statuses with canceled context - function normally returns", func() { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + updater.Update(ctx, createStatuses(false)) + }) + + It("should have the updated status of GatewayClass in the API server after updating with canceled context", func() { + latestGc := &v1alpha2.GatewayClass{} + expectedGc := createExpectedGc(metav1.ConditionFalse, string(v1alpha2.GatewayClassConditionStatusAccepted), "GatewayClass has been rejected: error") + + err := client.Get(context.Background(), types.NamespacedName{Name: gcName}, latestGc) + Expect(err).Should(Not(HaveOccurred())) + + expectedGc.ResourceVersion = latestGc.ResourceVersion + + Expect(helpers.Diff(expectedGc, latestGc)).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)) @@ -238,5 +298,18 @@ var _ = Describe("Updater", func() { Expect(helpers.Diff(expectedGw, latestGw)).To(BeEmpty()) }) + + 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 + + // if the status was updated, we would see the route rejected (Accepted = false) + Expect(helpers.Diff(expectedHR, latestHR)).To(BeEmpty()) + }) }) }) diff --git a/pkg/sdk/gatewayclass_controller.go b/pkg/sdk/gatewayclass_controller.go index ea84c229ec..5098c5f59b 100644 --- a/pkg/sdk/gatewayclass_controller.go +++ b/pkg/sdk/gatewayclass_controller.go @@ -37,9 +37,6 @@ func (r *gatewayClassReconciler) Reconcile(ctx context.Context, req reconcile.Re var gc v1alpha2.GatewayClass found := true - // we assume that the reconcile will only be called with the GatewayClass of the known name (like 'nginx') - // this is something that has to be configured on the Manager - err := r.Get(ctx, req.NamespacedName, &gc) if err != nil { if !apierrors.IsNotFound(err) { @@ -50,7 +47,7 @@ func (r *gatewayClassReconciler) Reconcile(ctx context.Context, req reconcile.Re } 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 88cf168dd9..adbe11fca4 100644 --- a/pkg/sdk/interfaces.go +++ b/pkg/sdk/interfaces.go @@ -10,7 +10,7 @@ import ( type GatewayClassImpl interface { Upsert(gc *v1alpha2.GatewayClass) - Remove(key string) + Remove(nsname types.NamespacedName) } type GatewayImpl interface { @@ -31,5 +31,5 @@ type HTTPRouteImpl interface { type ServiceImpl interface { Upsert(svc *apiv1.Service) - Remove(name types.NamespacedName) + Remove(nsname types.NamespacedName) } From f9554027d90ae59b2b36adcbaceb4145b01b179c Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 5 Jul 2022 10:12:39 -0700 Subject: [PATCH 2/5] Fix unit test for cli arg --- cmd/gateway/setup_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/gateway/setup_test.go b/cmd/gateway/setup_test.go index 0786e9bbdd..9b8aab7aaf 100644 --- a/cmd/gateway/setup_test.go +++ b/cmd/gateway/setup_test.go @@ -181,8 +181,8 @@ var _ = Describe("Main", func() { It("should fail with too many path elements", func() { t := prepareTestCase( - "k8s-gateway.nginx.org/nginx-gateway/my-gateway", - expectSuccess) + "k8s-gateway.nginx.org/nginx-gateway/my-gateway/broken", + expectError) tester(t) }) // should fail with too many path elements From 352b766703c3c58e1d1b0ad6b21ddc490253a767 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 5 Jul 2022 10:30:25 -0700 Subject: [PATCH 3/5] Use 'when' for better readability --- internal/state/change_processor_test.go | 96 +++++++++++++------------ 1 file changed, 51 insertions(+), 45 deletions(-) diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go index 550d40ec29..bdb8b6b897 100644 --- a/internal/state/change_processor_test.go +++ b/internal/state/change_processor_test.go @@ -106,58 +106,64 @@ var _ = Describe("ChangeProcessor", func() { }) 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()) + When("no upsert has occurred", func() { + It("should return empty configuration and statuses", 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 and GatewayClass don't exist", func() { - processor.CaptureUpsertChange(hr1) - - expectedConf := state.Configuration{} - 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}, + When("GatewayClass doesn't exist", func() { + When("Gateway doesn't exist", func() { + It("should return empty configuration and updated statuses after upserting an HTTPRoute", func() { + processor.CaptureUpsertChange(hr1) + + expectedConf := state.Configuration{} + 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 updated statuses after upserting the Gateway", func() { + processor.CaptureUpsertChange(gw) + + expectedConf := state.Configuration{} + expectedStatuses := state.Statuses{ + ListenerStatuses: map[string]state.ListenerStatus{ + "listener-80-1": { + Valid: false, + AttachedRoutes: 1, }, }, - }, - } - - 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 upserting the Gateway when the GatewayClass doesn't exist", func() { - processor.CaptureUpsertChange(gw) - - expectedConf := state.Configuration{} - expectedStatuses := state.Statuses{ - ListenerStatuses: map[string]state.ListenerStatus{ - "listener-80-1": { - Valid: false, - AttachedRoutes: 1, - }, - }, - HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ - {Namespace: "test", Name: "hr-1"}: { - ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": {Attached: false}, + 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()) + 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 GatewayClass is upserted", func() { From 8f08bcc7fabdcdc7b0ef5dccd31800e4f377ec6b Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 5 Jul 2022 10:51:35 -0700 Subject: [PATCH 4/5] Add cfg for ChangeProcessorImpl --- internal/manager/manager.go | 6 +++- internal/state/change_processor.go | 44 ++++++++++++++----------- internal/state/change_processor_test.go | 15 +++++++-- 3 files changed, 43 insertions(+), 22 deletions(-) diff --git a/internal/manager/manager.go b/internal/manager/manager.go index a766319e5f..68697a2e4d 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -69,7 +69,11 @@ func Start(cfg config.Config) error { return fmt.Errorf("cannot register service implementation: %w", err) } - processor := state.NewChangeProcessorImpl(cfg.GatewayNsName, cfg.GatewayCtlrName, cfg.GatewayClassName) + processor := state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ + GatewayNsName: cfg.GatewayNsName, + GatewayCtlrName: cfg.GatewayCtlrName, + GatewayClassName: cfg.GatewayClassName, + }) serviceStore := state.NewServiceStore() configGenerator := ngxcfg.NewGeneratorImpl(serviceStore) nginxFileMgr := file.NewManagerImpl() diff --git a/internal/state/change_processor.go b/internal/state/change_processor.go index 17f8cb4d3b..93b4d84faf 100644 --- a/internal/state/change_processor.go +++ b/internal/state/change_processor.go @@ -29,23 +29,29 @@ type ChangeProcessor interface { Process() (changed bool, conf Configuration, statuses Statuses) } +// ChangeProcessorConfig holds configuration parameters for ChangeProcessorImpl. +type ChangeProcessorConfig struct { + // GatewayNsName is the namespaced name of the Gateway resource. + GatewayNsName types.NamespacedName + // GatewayCtlrName is the name of the Gateway controller. + GatewayCtlrName string + // GatewayClassName is the name of the GatewayClass resource. + GatewayClassName string +} + type ChangeProcessorImpl struct { - store *store - changed bool - gwNsName types.NamespacedName - controllerName string - gcName string + store *store + changed bool + cfg ChangeProcessorConfig lock sync.Mutex } // NewChangeProcessorImpl creates a new ChangeProcessorImpl for the Gateway resource with the configured namespace name. -func NewChangeProcessorImpl(gwNsName types.NamespacedName, controllerName string, gcName string) *ChangeProcessorImpl { +func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { return &ChangeProcessorImpl{ - store: newStore(), - gwNsName: gwNsName, - controllerName: controllerName, - gcName: gcName, + store: newStore(), + cfg: cfg, } } @@ -57,8 +63,8 @@ func (c *ChangeProcessorImpl) CaptureUpsertChange(obj client.Object) { switch o := obj.(type) { case *v1alpha2.GatewayClass: - if o.Name != c.gcName { - panic(fmt.Errorf("gatewayclass resource must be %s, got %s", c.gcName, o.Name)) + if o.Name != c.cfg.GatewayClassName { + panic(fmt.Errorf("gatewayclass resource must be %s, got %s", c.cfg.GatewayClassName, o.Name)) } // if the resource spec hasn't changed (its generation is the same), ignore the upsert if c.store.gc != nil && c.store.gc.Generation == o.Generation { @@ -66,8 +72,8 @@ func (c *ChangeProcessorImpl) CaptureUpsertChange(obj client.Object) { } c.store.gc = o 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 o.Namespace != c.cfg.GatewayNsName.Namespace || o.Name != c.cfg.GatewayNsName.Name { + panic(fmt.Errorf("gateway resource must be %s/%s, got %s/%s", c.cfg.GatewayNsName.Namespace, c.cfg.GatewayNsName.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 { @@ -94,13 +100,13 @@ func (c *ChangeProcessorImpl) CaptureDeleteChange(resourceType client.Object, ns switch o := resourceType.(type) { case *v1alpha2.GatewayClass: - if nsname.Name != c.gcName { - panic(fmt.Errorf("gatewayclass resource must be %s, got %s", c.gcName, nsname.Name)) + if nsname.Name != c.cfg.GatewayClassName { + panic(fmt.Errorf("gatewayclass resource must be %s, got %s", c.cfg.GatewayClassName, nsname.Name)) } c.store.gc = nil 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)) + if nsname != c.cfg.GatewayNsName { + panic(fmt.Errorf("gateway resource must be %s/%s, got %s/%s", c.cfg.GatewayNsName.Namespace, c.cfg.GatewayNsName.Name, o.Namespace, o.Name)) } c.store.gw = nil case *v1alpha2.HTTPRoute: @@ -120,7 +126,7 @@ func (c *ChangeProcessorImpl) Process() (changed bool, conf Configuration, statu c.changed = false - graph := buildGraph(c.store, c.gwNsName, c.controllerName, c.gcName) + graph := buildGraph(c.store, c.cfg.GatewayNsName, c.cfg.GatewayCtlrName, c.cfg.GatewayClassName) conf = buildConfiguration(graph) statuses = buildStatuses(graph) diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go index bdb8b6b897..387aba6200 100644 --- a/internal/state/change_processor_test.go +++ b/internal/state/change_processor_test.go @@ -102,7 +102,13 @@ var _ = Describe("ChangeProcessor", func() { gwUpdated = gw.DeepCopy() gwUpdated.Generation++ - processor = state.NewChangeProcessorImpl(types.NamespacedName{Namespace: "test", Name: "gateway"}, controllerName, gcName) + cfg := state.ChangeProcessorConfig{ + GatewayNsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, + GatewayCtlrName: controllerName, + GatewayClassName: gcName, + } + + processor = state.NewChangeProcessorImpl(cfg) }) Describe("Process resources", Ordered, func() { @@ -583,7 +589,12 @@ var _ = Describe("ChangeProcessor", func() { var processor state.ChangeProcessor BeforeEach(func() { - processor = state.NewChangeProcessorImpl(types.NamespacedName{Namespace: "test", Name: "gateway"}, "test.controller", "my-class") + cfg := state.ChangeProcessorConfig{ + GatewayNsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, + GatewayCtlrName: "test.controller", + GatewayClassName: "my-class", + } + processor = state.NewChangeProcessorImpl(cfg) }) DescribeTable("CaptureUpsertChange must panic", From 173acc41e5c15edbcb0cb26aa27db833c0485377 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 5 Jul 2022 11:25:41 -0700 Subject: [PATCH 5/5] Add cfg for StatusUpdater --- internal/manager/manager.go | 12 ++++++- internal/status/updater.go | 57 ++++++++++++++++----------------- internal/status/updater_test.go | 9 +++++- 3 files changed, 47 insertions(+), 31 deletions(-) diff --git a/internal/manager/manager.go b/internal/manager/manager.go index 68697a2e4d..29c598bde3 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -78,7 +78,17 @@ func Start(cfg config.Config) error { configGenerator := ngxcfg.NewGeneratorImpl(serviceStore) nginxFileMgr := file.NewManagerImpl() nginxRuntimeMgr := ngxruntime.NewManagerImpl() - statusUpdater := status.NewUpdater(cfg.GatewayCtlrName, cfg.GatewayNsName, cfg.GatewayClassName, mgr.GetClient(), cfg.Logger, status.NewRealClock()) + statusUpdater := status.NewUpdater(status.UpdaterConfig{ + GatewayNsName: cfg.GatewayNsName, + GatewayCtlrName: cfg.GatewayCtlrName, + GatewayClassName: cfg.GatewayClassName, + Client: mgr.GetClient(), + // FIXME(pleshakov) Make sure each component: + // (1) Has a dedicated named logger. + // (2) Get it from the Manager (the WithName is done here for all components). + Logger: cfg.Logger.WithName("statusUpdater"), + Clock: status.NewRealClock(), + }) eventLoop := events.NewEventLoop(processor, serviceStore, configGenerator, eventCh, cfg.Logger, nginxFileMgr, nginxRuntimeMgr, statusUpdater) err = mgr.Add(eventLoop) diff --git a/internal/status/updater.go b/internal/status/updater.go index df19f2bac9..c6a3a842c9 100644 --- a/internal/status/updater.go +++ b/internal/status/updater.go @@ -20,6 +20,22 @@ type Updater interface { Update(context.Context, state.Statuses) } +// UpdaterConfig holds configuration parameters for Updater. +type UpdaterConfig struct { + // GatewayNsName is the namespaced name of the Gateway resource. + GatewayNsName types.NamespacedName + // GatewayCtlrName is the name of the Gateway controller. + GatewayCtlrName string + // GatewayClassName is the name of the GatewayClass resource. + GatewayClassName string + // Client is a Kubernetes API client. + Client client.Client + // Logger holds a logger to be used. + Logger logr.Logger + // Clock is used as a source of time for the LastTransitionTime field in Conditions in resource statuses. + Clock Clock +} + // updaterImpl updates statuses of the Gateway API resources. // // It has the following limitations: @@ -58,30 +74,13 @@ type Updater interface { // goes along the Open-closed principle. // FIXME(pleshakov): address limitation (7) type updaterImpl struct { - gatewayCtlrName string - gwNsName types.NamespacedName - gcName string - client client.Client - logger logr.Logger - clock Clock + cfg UpdaterConfig } // NewUpdater creates a new Updater. -func NewUpdater( - gatewayCtlrName string, - gwNsName types.NamespacedName, - gcName string, - client client.Client, - logger logr.Logger, - clock Clock, -) Updater { +func NewUpdater(cfg UpdaterConfig) Updater { return &updaterImpl{ - gatewayCtlrName: gatewayCtlrName, - gwNsName: gwNsName, - gcName: gcName, - client: client, - logger: logger.WithName("statusUpdater"), - clock: clock, + cfg: cfg, } } @@ -90,15 +89,15 @@ func (upd *updaterImpl) Update(ctx context.Context, statuses state.Statuses) { // FIXME(pleshakov) Skip the status update (API call) if the status hasn't changed. if statuses.GatewayClassStatus != nil { - upd.update(ctx, types.NamespacedName{Name: upd.gcName}, &v1alpha2.GatewayClass{}, func(object client.Object) { + upd.update(ctx, types.NamespacedName{Name: upd.cfg.GatewayClassName}, &v1alpha2.GatewayClass{}, func(object client.Object) { gc := object.(*v1alpha2.GatewayClass) - gc.Status = prepareGatewayClassStatus(*statuses.GatewayClassStatus, upd.clock.Now()) + gc.Status = prepareGatewayClassStatus(*statuses.GatewayClassStatus, upd.cfg.Clock.Now()) }) } - upd.update(ctx, upd.gwNsName, &v1alpha2.Gateway{}, func(object client.Object) { + upd.update(ctx, upd.cfg.GatewayNsName, &v1alpha2.Gateway{}, func(object client.Object) { gw := object.(*v1alpha2.Gateway) - gw.Status = prepareGatewayStatus(statuses.ListenerStatuses, upd.clock.Now()) + gw.Status = prepareGatewayStatus(statuses.ListenerStatuses, upd.cfg.Clock.Now()) }) for nsname, rs := range statuses.HTTPRouteStatuses { @@ -110,7 +109,7 @@ func (upd *updaterImpl) Update(ctx context.Context, statuses state.Statuses) { 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()) + hr.Status = prepareHTTPRouteStatus(rs, upd.cfg.GatewayNsName, upd.cfg.GatewayCtlrName, upd.cfg.Clock.Now()) }) } } @@ -123,10 +122,10 @@ func (upd *updaterImpl) update(ctx context.Context, nsname types.NamespacedName, // Otherwise, the Update status API call can fail. // Note: the default client uses a cache for reads, so we're not making an unnecessary API call here. // the default is configurable in the Manager options. - err := upd.client.Get(ctx, nsname, obj) + err := upd.cfg.Client.Get(ctx, nsname, obj) if err != nil { if !apierrors.IsNotFound(err) { - upd.logger.Error(err, "Failed to get the recent version the resource when updating status", + upd.cfg.Logger.Error(err, "Failed to get the recent version the resource when updating status", "namespace", nsname.Namespace, "name", nsname.Name, "kind", obj.GetObjectKind().GroupVersionKind().Kind) @@ -136,9 +135,9 @@ func (upd *updaterImpl) update(ctx context.Context, nsname types.NamespacedName, statusSetter(obj) - err = upd.client.Status().Update(ctx, obj) + err = upd.cfg.Client.Status().Update(ctx, obj) if err != nil { - upd.logger.Error(err, "Failed to update status", + upd.cfg.Logger.Error(err, "Failed to update status", "namespace", nsname.Namespace, "name", nsname.Name, "kind", obj.GetObjectKind().GroupVersionKind().Kind) diff --git a/internal/status/updater_test.go b/internal/status/updater_test.go index 6f3a8c62f4..db1bdb95cd 100644 --- a/internal/status/updater_test.go +++ b/internal/status/updater_test.go @@ -51,7 +51,14 @@ var _ = Describe("Updater", func() { gatewayCtrlName = "test.example.com" gwNsName = types.NamespacedName{Namespace: "test", Name: "gateway"} - updater = status.NewUpdater(gatewayCtrlName, gwNsName, gcName, client, zap.New(), fakeClock) + updater = status.NewUpdater(status.UpdaterConfig{ + GatewayNsName: gwNsName, + GatewayCtlrName: gatewayCtrlName, + GatewayClassName: gcName, + Client: client, + Logger: zap.New(), + Clock: fakeClock, + }) }) Describe("Process status updates", Ordered, func() {