From 8230e484ac163cbc252a8ca2cd74c169ea561240 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Mon, 22 May 2023 13:09:15 -0400 Subject: [PATCH] Make static mode compatible with provisioner This commit adds two features for the static mode: - Support configuring a single Gateway resource to watch. - Support not reporting the GatewayClass status, so that it doesn't conflict with the status updates done by the provisioner. Needed by https://github.com/nginxinc/nginx-kubernetes-gateway/issues/634 --- cmd/gateway/commands.go | 73 +++++++++- cmd/gateway/commands_test.go | 107 ++++++++++++--- cmd/gateway/validation.go | 35 +++++ cmd/gateway/validation_test.go | 137 ++++++++++++++++++- docs/cli-help.md | 8 +- docs/gateway-api-compatibility.md | 1 - internal/config/config.go | 4 +- internal/controller/reconciler.go | 4 +- internal/controller/register_test.go | 15 +- internal/manager/filter/filter.go | 25 ++++ internal/manager/filter/filter_test.go | 62 +++++++++ internal/manager/filter/gatewayclass.go | 23 ---- internal/manager/filter/gatewayclass_test.go | 49 ------- internal/manager/manager.go | 70 +++++++--- internal/manager/manager_test.go | 67 +++++++++ internal/status/status_suit_test.go | 2 +- internal/status/updater.go | 4 +- internal/status/updater_test.go | 76 ++++++++-- 18 files changed, 615 insertions(+), 147 deletions(-) create mode 100644 internal/manager/filter/filter.go create mode 100644 internal/manager/filter/filter_test.go delete mode 100644 internal/manager/filter/gatewayclass.go delete mode 100644 internal/manager/filter/gatewayclass_test.go create mode 100644 internal/manager/manager_test.go diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index b1b745947..d07d1e993 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -6,6 +6,7 @@ import ( "os" "github.com/spf13/cobra" + "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -36,7 +37,7 @@ var ( ) // stringValidatingValue is a string flag value with custom validation logic. -// stringValidatingValue implements the pflag.Value interface. +// it implements the pflag.Value interface. type stringValidatingValue struct { validator func(v string) error value string @@ -58,6 +59,34 @@ func (v *stringValidatingValue) Type() string { return "string" } +// namespacedNameValue is a string flag value that represents a namespaced name. +// it implements the pflag.Value interface. +type namespacedNameValue struct { + value types.NamespacedName +} + +func (v *namespacedNameValue) String() string { + if (v.value == types.NamespacedName{}) { + // if we don't do that, the default value in the help message will be printed as "/" + return "" + } + return v.value.String() +} + +func (v *namespacedNameValue) Set(param string) error { + nsname, err := parseNamespacedResourceName(param) + if err != nil { + return err + } + + v.value = nsname + return nil +} + +func (v *namespacedNameValue) Type() string { + return "string" +} + func createRootCommand() *cobra.Command { rootCmd := &cobra.Command{ Use: "gateway", @@ -84,7 +113,13 @@ func createRootCommand() *cobra.Command { } func createStaticModeCommand() *cobra.Command { - return &cobra.Command{ + const gatewayFlag = "gateway" + + // flag values + gateway := namespacedNameValue{} + var updateGCStatus bool + + cmd := &cobra.Command{ Use: "static-mode", Short: "Configure NGINX in the scope of a single Gateway resource", RunE: func(cmd *cobra.Command, args []string) error { @@ -100,11 +135,18 @@ func createStaticModeCommand() *cobra.Command { return fmt.Errorf("error validating POD_IP environment variable: %w", err) } + var gwNsName *types.NamespacedName + if cmd.Flags().Changed(gatewayFlag) { + gwNsName = &gateway.value + } + conf := config.Config{ - GatewayCtlrName: gatewayCtlrName.value, - Logger: logger, - GatewayClassName: gatewayClassName.value, - PodIP: podIP, + GatewayCtlrName: gatewayCtlrName.value, + Logger: logger, + GatewayClassName: gatewayClassName.value, + PodIP: podIP, + GatewayNsName: gwNsName, + UpdateGatewayClassStatus: updateGCStatus, } if err := manager.Start(conf); err != nil { @@ -114,6 +156,25 @@ func createStaticModeCommand() *cobra.Command { return nil }, } + + cmd.Flags().Var( + &gateway, + gatewayFlag, + "The namespaced name of the Gateway resource to use. "+ + "Must be of the form: NAMESPACE/NAME. "+ + "If not specified, the control plane will process all Gateways for the configured GatewayClass. "+ + "However, among them, it will choose the oldest resource by creation timestamp. If the timestamps are "+ + "equal, it will choose the resource that appears first in alphabetical order by {namespace}/{name}.", + ) + + cmd.Flags().BoolVar( + &updateGCStatus, + "update-gatewayclass-status", + true, + "Update the status of the GatewayClass resource.", + ) + + return cmd } func createProvisionerModeCommand() *cobra.Command { diff --git a/cmd/gateway/commands_test.go b/cmd/gateway/commands_test.go index a03a2107c..425f66a2e 100644 --- a/cmd/gateway/commands_test.go +++ b/cmd/gateway/commands_test.go @@ -5,15 +5,40 @@ import ( "testing" . "github.com/onsi/gomega" + "github.com/spf13/cobra" ) +type flagTestCase struct { + name string + expectedErrPrefix string + args []string + wantErr bool +} + +func testFlag(t *testing.T, cmd *cobra.Command, test flagTestCase) { + g := NewGomegaWithT(t) + // discard any output generated by cobra + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + // override RunE to avoid executing the command + cmd.RunE = func(cmd *cobra.Command, args []string) error { + return nil + } + + cmd.SetArgs(test.args) + err := cmd.Execute() + + if test.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(HavePrefix(test.expectedErrPrefix)) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } +} + func TestRootCmdFlagValidation(t *testing.T) { - tests := []struct { - name string - expectedErrPrefix string - args []string - wantErr bool - }{ + tests := []flagTestCase{ { name: "valid flags", args: []string{ @@ -79,22 +104,66 @@ func TestRootCmdFlagValidation(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - g := NewGomegaWithT(t) - rootCmd := createRootCommand() - // discard any output generated by cobra - rootCmd.SetOut(io.Discard) - rootCmd.SetErr(io.Discard) + testFlag(t, rootCmd, test) + }) + } +} - rootCmd.SetArgs(test.args) - err := rootCmd.Execute() +func TestStaticModeCmdFlagValidation(t *testing.T) { + tests := []flagTestCase{ + { + name: "valid flags", + args: []string{ + "--gateway=nginx-gateway/nginx", + "--update-gatewayclass-status=true", + }, + wantErr: false, + }, + { + name: "valid flags, not set", + args: nil, + wantErr: false, + }, + { + name: "gateway is set to empty string", + args: []string{ + "--gateway=", + }, + wantErr: true, + expectedErrPrefix: `invalid argument "" for "--gateway" flag: must be set`, + }, + { + name: "gateway is invalid", + args: []string{ + "--gateway=nginx-gateway", // no namespace + }, + wantErr: true, + expectedErrPrefix: `invalid argument "nginx-gateway" for "--gateway" flag: invalid format; ` + + "must be NAMESPACE/NAME", + }, + { + name: "update-gatewayclass-status is set to empty string", + args: []string{ + "--update-gatewayclass-status=", + }, + wantErr: true, + expectedErrPrefix: `invalid argument "" for "--update-gatewayclass-status" flag: strconv.ParseBool`, + }, + { + name: "update-gatewayclass-status is invalid", + args: []string{ + "--update-gatewayclass-status=invalid", // not a boolean + }, + wantErr: true, + expectedErrPrefix: `invalid argument "invalid" for "--update-gatewayclass-status" flag: strconv.ParseBool`, + }, + } - if test.wantErr { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(HavePrefix(test.expectedErrPrefix)) - } else { - g.Expect(err).ToNot(HaveOccurred()) - } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + cmd := createStaticModeCommand() + testFlag(t, cmd, test) }) } } diff --git a/cmd/gateway/validation.go b/cmd/gateway/validation.go index 157ce6994..6540252a9 100644 --- a/cmd/gateway/validation.go +++ b/cmd/gateway/validation.go @@ -7,6 +7,7 @@ import ( "regexp" "strings" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation" ) @@ -54,6 +55,40 @@ func validateResourceName(value string) error { return nil } +func validateNamespaceName(value string) error { + // used by Kubernetes to validate resource namespace names + messages := validation.IsDNS1123Label(value) + if len(messages) > 0 { + msg := strings.Join(messages, "; ") + return fmt.Errorf("invalid format: %s", msg) + } + + return nil +} + +func parseNamespacedResourceName(value string) (types.NamespacedName, error) { + if value == "" { + return types.NamespacedName{}, errors.New("must be set") + } + + parts := strings.Split(value, "/") + if len(parts) != 2 { + return types.NamespacedName{}, errors.New("invalid format; must be NAMESPACE/NAME") + } + + if err := validateNamespaceName(parts[0]); err != nil { + return types.NamespacedName{}, fmt.Errorf("invalid namespace name: %w", err) + } + if err := validateResourceName(parts[1]); err != nil { + return types.NamespacedName{}, fmt.Errorf("invalid resource name: %w", err) + } + + return types.NamespacedName{ + Namespace: parts[0], + Name: parts[1], + }, nil +} + func validateIP(ip string) error { if ip == "" { return errors.New("IP address must be set") diff --git a/cmd/gateway/validation_test.go b/cmd/gateway/validation_test.go index e8d5e8648..8836fa5b1 100644 --- a/cmd/gateway/validation_test.go +++ b/cmd/gateway/validation_test.go @@ -4,6 +4,7 @@ import ( "testing" . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/types" ) func TestValidateGatewayControllerName(t *testing.T) { @@ -81,8 +82,13 @@ func TestValidateResourceName(t *testing.T) { expErr: false, }, { - name: "valid - with dash and numbers", - value: "my-gateway-123", + name: "valid - with dot", + value: "my.gateway", + expErr: false, + }, + { + name: "valid - with numbers", + value: "mygateway123", expErr: false, }, { @@ -122,6 +128,133 @@ func TestValidateResourceName(t *testing.T) { } } +func TestValidateNamespaceName(t *testing.T) { + tests := []struct { + name string + value string + expErr bool + }{ + { + name: "valid", + value: "mynamespace", + expErr: false, + }, + { + name: "valid - with dash", + value: "my-namespace", + expErr: false, + }, + { + name: "valid - with numbers", + value: "mynamespace123", + expErr: false, + }, + { + name: "invalid - empty", + value: "", + expErr: true, + }, + { + name: "invalid - invalid character '.'", + value: "my.namespace", + expErr: true, + }, + { + name: "invalid - invalid character '/'", + value: "my/namespace", + expErr: true, + }, + { + name: "invalid - invalid character '_'", + value: "my_namespace", + expErr: true, + }, + { + name: "invalid - invalid character '@'", + value: "my@namespace", + expErr: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + err := validateNamespaceName(test.value) + + if test.expErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + +func TestParseNamespacedResourceName(t *testing.T) { + tests := []struct { + name string + value string + expectedErrPrefix string + expectedNsName types.NamespacedName + expectErr bool + }{ + { + name: "valid", + value: "test/my-gateway", + expectedNsName: types.NamespacedName{ + Namespace: "test", + Name: "my-gateway", + }, + expectErr: false, + }, + { + name: "empty", + value: "", + expectedNsName: types.NamespacedName{}, + expectErr: true, + expectedErrPrefix: "must be set", + }, + { + name: "wrong number of parts", + value: "test", + expectedNsName: types.NamespacedName{}, + expectErr: true, + expectedErrPrefix: "invalid format; must be NAMESPACE/NAME", + }, + { + name: "invalid namespace", + value: "t@st/my-gateway", + expectedNsName: types.NamespacedName{}, + expectErr: true, + expectedErrPrefix: "invalid namespace name", + }, + { + name: "invalid name", + value: "test/my-g@teway", + expectedNsName: types.NamespacedName{}, + expectErr: true, + expectedErrPrefix: "invalid resource name", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + nsName, err := parseNamespacedResourceName(test.value) + + if test.expectErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(HavePrefix(test.expectedErrPrefix)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(nsName).To(Equal(test.expectedNsName)) + } + }) + } +} + func TestValidateIP(t *testing.T) { tests := []struct { name string diff --git a/docs/cli-help.md b/docs/cli-help.md index 31eb08cf2..41387ed11 100644 --- a/docs/cli-help.md +++ b/docs/cli-help.md @@ -4,11 +4,7 @@ This document describes the commands available in the `gateway` binary of `nginx ## Static Mode -This command configures NGINX in the scope of a single Gateway resource. In case of multiple Gateway resources created -in the cluster, NGINX Kubernetes Gateway will use a deterministic conflict resolution strategy: it will choose the -oldest resource by creation timestamp. If the timestamps are equal, NGINX Kubernetes Gateway will choose the resource -that appears first in alphabetical order by “{namespace}/{name}”. We might support multiple Gateway resources. Please -share your use case with us if you're interested in that support. +This command configures NGINX in the scope of a single Gateway resource. Usage: @@ -22,3 +18,5 @@ Flags: |-|-|-| | `gateway-ctlr-name` | `string` | The name of the Gateway controller. The controller name must be of the form: `DOMAIN/PATH`. The controller's domain is `k8s-gateway.nginx.org`. | | `gatewayclass` | `string` | The name of the GatewayClass resource. Every NGINX Gateway must have a unique corresponding GatewayClass resource. | +| `gateway` | `string` | The namespaced name of the Gateway resource to use. Must be of the form: `NAMESPACE/NAME`. If not specified, the control plane will process all Gateways for the configured GatewayClass. However, among them, it will choose the oldest resource by creation timestamp. If the timestamps are equal, it will choose the resource that appears first in alphabetical order by {namespace}/{name}. | +| `update-gatewayclass-status` | `bool` | Update the status of the GatewayClass resource. (default true) | diff --git a/docs/gateway-api-compatibility.md b/docs/gateway-api-compatibility.md index 4d2ea97f4..54b7e2d89 100644 --- a/docs/gateway-api-compatibility.md +++ b/docs/gateway-api-compatibility.md @@ -50,7 +50,6 @@ Fields: > Status: Partially supported. NGINX Kubernetes Gateway supports only a single Gateway resource. The Gateway resource must reference NGINX Kubernetes Gateway's corresponding GatewayClass. -In case of multiple Gateway resources created in the cluster, NGINX Kubernetes Gateway will use a deterministic conflict resolution strategy. See [static-mode](./cli-help.md#static-mode) command for more info. Fields: diff --git a/internal/config/config.go b/internal/config/config.go index 7de2f3e0e..0bcd1c09f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -10,9 +10,11 @@ type Config struct { 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 + GatewayNsName *types.NamespacedName // GatewayClassName is the name of the GatewayClass resource that the Gateway will use. GatewayClassName string // PodIP is the IP address of this Pod. PodIP string + // UpdateGatewayClassStatus enables updating the status of the GatewayClass resource. + UpdateGatewayClassStatus bool } diff --git a/internal/controller/reconciler.go b/internal/controller/reconciler.go index 7274e1e3d..b2a87997e 100644 --- a/internal/controller/reconciler.go +++ b/internal/controller/reconciler.go @@ -16,7 +16,7 @@ import ( // NamespacedNameFilterFunc is a function that returns true if the resource should be processed by the reconciler. // If the function returns false, the reconciler will log the returned string. -type NamespacedNameFilterFunc func(nsname types.NamespacedName) (bool, string) +type NamespacedNameFilterFunc func(nsname types.NamespacedName) (shouldProcess bool, msg string) // ReconcilerConfig is the configuration for the reconciler. type ReconcilerConfig struct { @@ -67,7 +67,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco logger.Info("Reconciling the resource") if r.cfg.NamespacedNameFilter != nil { - if allow, msg := r.cfg.NamespacedNameFilter(req.NamespacedName); !allow { + if shouldProcess, msg := r.cfg.NamespacedNameFilter(req.NamespacedName); !shouldProcess { logger.Info(msg) return reconcile.Result{}, nil } diff --git a/internal/controller/register_test.go b/internal/controller/register_test.go index c60f6599c..0a60fef04 100644 --- a/internal/controller/register_test.go +++ b/internal/controller/register_test.go @@ -8,8 +8,9 @@ import ( . "github.com/onsi/gomega" "github.com/onsi/gomega/gcustom" - "github.com/onsi/gomega/types" + gtypes "github.com/onsi/gomega/types" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -17,7 +18,6 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/controller" "github.com/nginxinc/nginx-kubernetes-gateway/internal/controller/controllerfakes" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/filter" "github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/index" "github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/predicate" ) @@ -81,12 +81,15 @@ func TestRegister(t *testing.T) { } objectType := &v1beta1.HTTPRoute{} - namespacedNameFilter := filter.CreateFilterForGatewayClass("test") + nsNameFilter := func(nsname types.NamespacedName) (bool, string) { + return true, "" + } + fieldIndexes := index.CreateEndpointSliceFieldIndices() eventCh := make(chan<- interface{}) - beSameFunctionPointer := func(expected interface{}) types.GomegaMatcher { + beSameFunctionPointer := func(expected interface{}) gtypes.GomegaMatcher { return gcustom.MakeMatcher(func(f interface{}) (bool, error) { // comparing functions is not allowed in Go, so we're comparing the pointers return reflect.ValueOf(expected).Pointer() == reflect.ValueOf(f).Pointer(), nil @@ -101,7 +104,7 @@ func TestRegister(t *testing.T) { g.Expect(c.Getter).To(BeIdenticalTo(test.fakes.mgr.GetClient())) g.Expect(c.ObjectType).To(BeIdenticalTo(objectType)) g.Expect(c.EventCh).To(BeIdenticalTo(eventCh)) - g.Expect(c.NamespacedNameFilter).Should(beSameFunctionPointer(namespacedNameFilter)) + g.Expect(c.NamespacedNameFilter).Should(beSameFunctionPointer(nsNameFilter)) return controller.NewReconciler(c) } @@ -111,7 +114,7 @@ func TestRegister(t *testing.T) { objectType, test.fakes.mgr, eventCh, - controller.WithNamespacedNameFilter(namespacedNameFilter), + controller.WithNamespacedNameFilter(nsNameFilter), controller.WithK8sPredicate(predicate.ServicePortsChangedPredicate{}), controller.WithFieldIndices(fieldIndexes), controller.WithNewReconciler(newReconciler), diff --git a/internal/manager/filter/filter.go b/internal/manager/filter/filter.go new file mode 100644 index 000000000..12002ca17 --- /dev/null +++ b/internal/manager/filter/filter.go @@ -0,0 +1,25 @@ +package filter + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/types" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/controller" +) + +// CreateSingleResourceFilter creates a filter function that filters out all resources except the one +// with the given namespaced name. +func CreateSingleResourceFilter(targetNsName types.NamespacedName) controller.NamespacedNameFilterFunc { + return func(nsname types.NamespacedName) (shouldProcess bool, msg string) { + if nsname != targetNsName { + msg := fmt.Sprintf( + "Resource is ignored because this controller only supports a single resource %s/%s of that type", + targetNsName.Namespace, + targetNsName.Name, + ) + return false, msg + } + return true, "" + } +} diff --git a/internal/manager/filter/filter_test.go b/internal/manager/filter/filter_test.go new file mode 100644 index 000000000..fadfd2b9e --- /dev/null +++ b/internal/manager/filter/filter_test.go @@ -0,0 +1,62 @@ +package filter + +import ( + "testing" + + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/types" +) + +func TestCreateSingleResourceFilter(t *testing.T) { + targetNsName := types.NamespacedName{Namespace: "test", Name: "resource"} + + filter := CreateSingleResourceFilter(targetNsName) + if filter == nil { + t.Fatal("TestCreateSingleResourceFilter() returned nil filter") + } + + const expectedMsg = "Resource is ignored because this controller only supports a single resource " + + "test/resource of that type" + + tests := []struct { + name string + nsname types.NamespacedName + expectedMsg string + expectedShouldProcess bool + }{ + { + name: "match", + nsname: targetNsName, + expectedShouldProcess: true, + expectedMsg: "", + }, + { + name: "wrong namespace", + nsname: types.NamespacedName{Namespace: targetNsName.Namespace, Name: "other-name"}, + expectedShouldProcess: false, + expectedMsg: expectedMsg, + }, + { + name: "wrong name", + nsname: types.NamespacedName{Namespace: "other-ns", Name: targetNsName.Name}, + expectedShouldProcess: false, + expectedMsg: expectedMsg, + }, + { + name: "wrong namespace and name", + nsname: types.NamespacedName{Namespace: "other-ns", Name: "other-name"}, + expectedShouldProcess: false, + expectedMsg: expectedMsg, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + shouldProcess, msg := filter(test.nsname) + g.Expect(shouldProcess).To(Equal(test.expectedShouldProcess)) + g.Expect(msg).To(Equal(test.expectedMsg)) + }) + } +} diff --git a/internal/manager/filter/gatewayclass.go b/internal/manager/filter/gatewayclass.go deleted file mode 100644 index 3cd21b3f4..000000000 --- a/internal/manager/filter/gatewayclass.go +++ /dev/null @@ -1,23 +0,0 @@ -package filter - -import ( - "fmt" - - "k8s.io/apimachinery/pkg/types" - - "github.com/nginxinc/nginx-kubernetes-gateway/internal/controller" -) - -// CreateFilterForGatewayClass creates a filter function that filters out all GatewayClass resources except the one -// with the given name. -func CreateFilterForGatewayClass(gcName string) controller.NamespacedNameFilterFunc { - return func(nsname types.NamespacedName) (bool, string) { - if nsname.Name != gcName { - return false, fmt.Sprintf( - "GatewayClass is ignored because this controller only supports the GatewayClass %s", - gcName, - ) - } - return true, "" - } -} diff --git a/internal/manager/filter/gatewayclass_test.go b/internal/manager/filter/gatewayclass_test.go deleted file mode 100644 index d8c9c309e..000000000 --- a/internal/manager/filter/gatewayclass_test.go +++ /dev/null @@ -1,49 +0,0 @@ -package filter - -import ( - "testing" - - "k8s.io/apimachinery/pkg/types" -) - -func TestCreateFilterForGatewayClass(t *testing.T) { - const gcName = "my-gc" - - filter := CreateFilterForGatewayClass(gcName) - if filter == nil { - t.Fatal("CreateFilterForGatewayClass() returned nil") - } - - tests := []struct { - nsname types.NamespacedName - expected bool - }{ - { - nsname: types.NamespacedName{Name: gcName}, - expected: true, - }, - { - nsname: types.NamespacedName{Name: gcName, Namespace: "doesn't matter"}, - expected: true, - }, - { - nsname: types.NamespacedName{Name: "some-gc"}, - expected: false, - }, - } - - for _, test := range tests { - result, msg := filter(test.nsname) - - if result != test.expected { - t.Errorf("filter(%#v) returned %v but expected %v", test.nsname, result, test.expected) - } - - if result && msg != "" { - t.Errorf("filter(%#v) returned a non-empty message %q", test.nsname, msg) - } - if !result && msg == "" { - t.Errorf("filter(%#v) returned an empty message", test.nsname) - } - } -} diff --git a/internal/manager/manager.go b/internal/manager/manager.go index de9d5a6f8..a2bfcda03 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -8,6 +8,7 @@ import ( discoveryV1 "k8s.io/api/discovery/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" ctlr "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -67,6 +68,9 @@ func Start(cfg config.Config) error { return fmt.Errorf("cannot build runtime manager: %w", err) } + // Note: for any new object type or a change to the existing one, + // make sure to also update prepareFirstEventBatchPreparerArgs() + // FIXME(pleshakov): Make the comment above redundant. controllerRegCfgs := []struct { objectType client.Object options []controller.Option @@ -74,11 +78,21 @@ func Start(cfg config.Config) error { { objectType: &gatewayv1beta1.GatewayClass{}, options: []controller.Option{ - controller.WithNamespacedNameFilter(filter.CreateFilterForGatewayClass(cfg.GatewayClassName)), + controller.WithNamespacedNameFilter(filter.CreateSingleResourceFilter( + types.NamespacedName{Name: cfg.GatewayClassName}, + )), }, }, { objectType: &gatewayv1beta1.Gateway{}, + options: func() []controller.Option { + if cfg.GatewayNsName != nil { + return []controller.Option{ + controller.WithNamespacedNameFilter(filter.CreateSingleResourceFilter(*cfg.GatewayNsName)), + } + } + return nil + }(), }, { objectType: &gatewayv1beta1.HTTPRoute{}, @@ -134,12 +148,13 @@ func Start(cfg config.Config) error { nginxFileMgr := file.NewManagerImpl() nginxRuntimeMgr := ngxruntime.NewManagerImpl() statusUpdater := status.NewUpdater(status.UpdaterConfig{ - GatewayCtlrName: cfg.GatewayCtlrName, - GatewayClassName: cfg.GatewayClassName, - Client: mgr.GetClient(), - PodIP: cfg.PodIP, - Logger: cfg.Logger.WithName("statusUpdater"), - Clock: status.NewRealClock(), + GatewayCtlrName: cfg.GatewayCtlrName, + GatewayClassName: cfg.GatewayClassName, + Client: mgr.GetClient(), + PodIP: cfg.PodIP, + Logger: cfg.Logger.WithName("statusUpdater"), + Clock: status.NewRealClock(), + UpdateGatewayClassStatus: cfg.UpdateGatewayClassStatus, }) eventHandler := events.NewEventHandlerImpl(events.EventHandlerConfig{ @@ -153,19 +168,8 @@ func Start(cfg config.Config) error { StatusUpdater: statusUpdater, }) - firstBatchPreparer := events.NewFirstEventBatchPreparerImpl( - mgr.GetCache(), - []client.Object{ - &gatewayv1beta1.GatewayClass{ObjectMeta: metav1.ObjectMeta{Name: cfg.GatewayClassName}}, - }, - []client.ObjectList{ - &apiv1.ServiceList{}, - &apiv1.SecretList{}, - &discoveryV1.EndpointSliceList{}, - &gatewayv1beta1.GatewayList{}, - &gatewayv1beta1.HTTPRouteList{}, - }, - ) + objects, objectLists := prepareFirstEventBatchPreparerArgs(cfg.GatewayClassName, cfg.GatewayNsName) + firstBatchPreparer := events.NewFirstEventBatchPreparerImpl(mgr.GetCache(), objects, objectLists) eventLoop := events.NewEventLoop( eventCh, @@ -181,3 +185,29 @@ func Start(cfg config.Config) error { logger.Info("Starting manager") return mgr.Start(ctx) } + +func prepareFirstEventBatchPreparerArgs( + gcName string, + gwNsName *types.NamespacedName, +) ([]client.Object, []client.ObjectList) { + objects := []client.Object{ + &gatewayv1beta1.GatewayClass{ObjectMeta: metav1.ObjectMeta{Name: gcName}}, + } + objectLists := []client.ObjectList{ + &apiv1.ServiceList{}, + &apiv1.SecretList{}, + &discoveryV1.EndpointSliceList{}, + &gatewayv1beta1.HTTPRouteList{}, + } + + if gwNsName == nil { + objectLists = append(objectLists, &gatewayv1beta1.GatewayList{}) + } else { + objects = append( + objects, + &gatewayv1beta1.Gateway{ObjectMeta: metav1.ObjectMeta{Name: gwNsName.Name, Namespace: gwNsName.Namespace}}, + ) + } + + return objects, objectLists +} diff --git a/internal/manager/manager_test.go b/internal/manager/manager_test.go new file mode 100644 index 000000000..7f5105108 --- /dev/null +++ b/internal/manager/manager_test.go @@ -0,0 +1,67 @@ +package manager + +import ( + "testing" + + . "github.com/onsi/gomega" + apiv1 "k8s.io/api/core/v1" + discoveryV1 "k8s.io/api/discovery/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" +) + +func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { + const gcName = "nginx" + + tests := []struct { + name string + gwNsName *types.NamespacedName + expectedObjects []client.Object + expectedObjectLists []client.ObjectList + }{ + { + name: "gwNsName is nil", + gwNsName: nil, + expectedObjects: []client.Object{ + &gatewayv1beta1.GatewayClass{ObjectMeta: metav1.ObjectMeta{Name: "nginx"}}, + }, + expectedObjectLists: []client.ObjectList{ + &apiv1.ServiceList{}, + &apiv1.SecretList{}, + &discoveryV1.EndpointSliceList{}, + &gatewayv1beta1.HTTPRouteList{}, + &gatewayv1beta1.GatewayList{}, + }, + }, + { + name: "gwNsName is not nil", + gwNsName: &types.NamespacedName{ + Namespace: "test", + Name: "my-gateway", + }, + expectedObjects: []client.Object{ + &gatewayv1beta1.GatewayClass{ObjectMeta: metav1.ObjectMeta{Name: "nginx"}}, + &gatewayv1beta1.Gateway{ObjectMeta: metav1.ObjectMeta{Name: "my-gateway", Namespace: "test"}}, + }, + expectedObjectLists: []client.ObjectList{ + &apiv1.ServiceList{}, + &apiv1.SecretList{}, + &discoveryV1.EndpointSliceList{}, + &gatewayv1beta1.HTTPRouteList{}, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + objects, objectLists := prepareFirstEventBatchPreparerArgs(gcName, test.gwNsName) + + g.Expect(objects).To(ConsistOf(test.expectedObjects)) + g.Expect(objectLists).To(ConsistOf(test.expectedObjectLists)) + }) + } +} diff --git a/internal/status/status_suit_test.go b/internal/status/status_suit_test.go index de296f304..b2dad4481 100644 --- a/internal/status/status_suit_test.go +++ b/internal/status/status_suit_test.go @@ -7,7 +7,7 @@ import ( . "github.com/onsi/gomega" ) -func TestState(t *testing.T) { +func TestStatus(t *testing.T) { RegisterFailHandler(Fail) RunSpecs(t, "Status Suite") } diff --git a/internal/status/updater.go b/internal/status/updater.go index 0859bb4f3..88d1ebd21 100644 --- a/internal/status/updater.go +++ b/internal/status/updater.go @@ -34,6 +34,8 @@ type UpdaterConfig struct { GatewayClassName string // PodIP is the IP address of this Pod. PodIP string + // UpdateGatewayClassStatus enables updating the status of the GatewayClass resource. + UpdateGatewayClassStatus bool } // updaterImpl updates statuses of the Gateway API resources. @@ -88,7 +90,7 @@ 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 { + if upd.cfg.UpdateGatewayClassStatus && statuses.GatewayClassStatus != nil { upd.update( ctx, types.NamespacedName{Name: upd.cfg.GatewayClassName}, diff --git a/internal/status/updater_test.go b/internal/status/updater_test.go index aac7df883..c3560b1a4 100644 --- a/internal/status/updater_test.go +++ b/internal/status/updater_test.go @@ -26,9 +26,9 @@ var _ = Describe("Updater", func() { const gcName = "my-class" var ( - updater status.Updater client client.Client fakeClockTime metav1.Time + fakeClock *statusfakes.FakeClock gatewayCtrlName string ) @@ -45,19 +45,10 @@ var _ = Describe("Updater", func() { // We use it because updating the status in the FakeClient and then getting the resource back // involves encoding and decoding the resource to/from JSON, which uses RFC 3339 for metav1.Time. fakeClockTime = metav1.NewTime(time.Now()).Rfc3339Copy() - fakeClock := &statusfakes.FakeClock{} + fakeClock = &statusfakes.FakeClock{} fakeClock.NowReturns(fakeClockTime) gatewayCtrlName = "test.example.com" - - updater = status.NewUpdater(status.UpdaterConfig{ - GatewayCtlrName: gatewayCtrlName, - GatewayClassName: gcName, - Client: client, - Logger: zap.New(), - Clock: fakeClock, - PodIP: "1.2.3.4", - }) }) Describe("Process status updates", Ordered, func() { @@ -67,6 +58,7 @@ var _ = Describe("Updater", func() { } var ( + updater status.Updater gc *v1beta1.GatewayClass gw, ignoredGw *v1beta1.Gateway hr *v1beta1.HTTPRoute @@ -213,6 +205,16 @@ var _ = Describe("Updater", func() { ) BeforeAll(func() { + updater = status.NewUpdater(status.UpdaterConfig{ + GatewayCtlrName: gatewayCtrlName, + GatewayClassName: gcName, + Client: client, + Logger: zap.New(), + Clock: fakeClock, + PodIP: "1.2.3.4", + UpdateGatewayClassStatus: true, + }) + gc = &v1beta1.GatewayClass{ ObjectMeta: metav1.ObjectMeta{ Name: gcName, @@ -393,4 +395,56 @@ var _ = Describe("Updater", func() { }) }) }) + + Describe("Skip GatewayClass updates", Ordered, func() { + var ( + updater status.Updater + gc *v1beta1.GatewayClass + ) + + BeforeAll(func() { + updater = status.NewUpdater(status.UpdaterConfig{ + GatewayCtlrName: gatewayCtrlName, + GatewayClassName: gcName, + Client: client, + Logger: zap.New(), + Clock: fakeClock, + PodIP: "1.2.3.4", + UpdateGatewayClassStatus: false, + }) + + gc = &v1beta1.GatewayClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: gcName, + }, + TypeMeta: metav1.TypeMeta{ + Kind: "GatewayClass", + APIVersion: "gateway.networking.k8s.io/v1beta1", + }, + } + }) + + It("should create resources in the API server", func() { + Expect(client.Create(context.Background(), gc)).Should(Succeed()) + }) + + It("should not update GatewayClass status", func() { + updater.Update( + context.Background(), + state.Statuses{ + GatewayClassStatus: &state.GatewayClassStatus{ + ObservedGeneration: 1, + Conditions: status.CreateTestConditions("Test"), + }, + }, + ) + + latestGc := &v1beta1.GatewayClass{} + + err := client.Get(context.Background(), types.NamespacedName{Name: gcName}, latestGc) + Expect(err).Should(Not(HaveOccurred())) + + Expect(latestGc.Status).To(BeZero()) + }) + }) })