diff --git a/cmd/gateway/setup_test.go b/cmd/gateway/setup_test.go index 51e050d894..558075edb9 100644 --- a/cmd/gateway/setup_test.go +++ b/cmd/gateway/setup_test.go @@ -19,7 +19,7 @@ func MockValidator(name string, called *int, succeed bool) ValidatorContext { *called++ if !succeed { - return errors.New("Mock error") + return errors.New("mock error") } return nil }, diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index 786584b379..fa7a773b8e 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -30,6 +30,12 @@ rules: verbs: - list - watch +- apiGroups: + - gateway.networking.k8s.io + resources: + - httproutes/status + verbs: + - update --- kind: ClusterRoleBinding apiVersion: rbac.authorization.k8s.io/v1 diff --git a/internal/events/loop.go b/internal/events/loop.go index 3bf3f0eab6..4c9e71ebfd 100644 --- a/internal/events/loop.go +++ b/internal/events/loop.go @@ -6,22 +6,25 @@ import ( "github.com/go-logr/logr" "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/status" "sigs.k8s.io/gateway-api/apis/v1alpha2" ) // EventLoop is the main event loop of the Gateway. type EventLoop struct { - conf state.Configuration - eventCh <-chan interface{} - logger logr.Logger + conf state.Configuration + eventCh <-chan interface{} + logger logr.Logger + statusUpdater status.Updater } // NewEventLoop creates a new EventLoop. -func NewEventLoop(conf state.Configuration, eventCh <-chan interface{}, logger logr.Logger) *EventLoop { +func NewEventLoop(conf state.Configuration, eventCh <-chan interface{}, statusUpdater status.Updater, logger logr.Logger) *EventLoop { return &EventLoop{ - conf: conf, - eventCh: eventCh, - logger: logger.WithName("eventLoop"), + conf: conf, + eventCh: eventCh, + statusUpdater: statusUpdater, + logger: logger.WithName("eventLoop"), } } @@ -35,7 +38,7 @@ func (el *EventLoop) Start(ctx context.Context) error { case <-ctx.Done(): return nil case e := <-el.eventCh: - err := el.handleEvent(e) + err := el.handleEvent(ctx, e) if err != nil { return err } @@ -44,7 +47,7 @@ func (el *EventLoop) Start(ctx context.Context) error { } // TO-DO: think about how to avoid using an interface{} here -func (el *EventLoop) handleEvent(event interface{}) error { +func (el *EventLoop) handleEvent(ctx context.Context, event interface{}) error { var changes []state.Change var updates []state.StatusUpdate var err error @@ -55,6 +58,7 @@ func (el *EventLoop) handleEvent(event interface{}) error { case *DeleteEvent: changes, updates, err = el.propagateDelete(e) default: + // TO-DO: panic return fmt.Errorf("unknown event type %T", e) } @@ -62,8 +66,7 @@ func (el *EventLoop) handleEvent(event interface{}) error { return err } - el.processChangesAndStatusUpdates(changes, updates) - + el.processChangesAndStatusUpdates(ctx, changes, updates) return nil } @@ -87,7 +90,7 @@ func (el *EventLoop) propagateDelete(e *DeleteEvent) ([]state.Change, []state.St return nil, nil, fmt.Errorf("unknown resource type %T", e.Type) } -func (el *EventLoop) processChangesAndStatusUpdates(changes []state.Change, updates []state.StatusUpdate) { +func (el *EventLoop) processChangesAndStatusUpdates(ctx context.Context, changes []state.Change, updates []state.StatusUpdate) { for _, c := range changes { el.logger.Info("Processing a change", "host", c.Host.Value) @@ -96,13 +99,5 @@ func (el *EventLoop) processChangesAndStatusUpdates(changes []state.Change, upda fmt.Printf("%+v\n", c) } - for _, u := range updates { - // TO-DO: in the next iteration, the update will include the namespace/name of the resource instead of - // runtime.Object, so it will be easy to get the resource namespace/name and include it in the log output - el.logger.Info("Processing a status update", - "gvk", u.Object.GetObjectKind().GroupVersionKind().String()) - - // TO-DO: This code is temporary. We will remove it once we have a component that updates statuses. - fmt.Printf("%+v\n", u) - } + el.statusUpdater.ProcessStatusUpdates(ctx, updates) } diff --git a/internal/events/loop_test.go b/internal/events/loop_test.go index 7f48eb7b0b..24a7498865 100644 --- a/internal/events/loop_test.go +++ b/internal/events/loop_test.go @@ -4,7 +4,9 @@ import ( "context" "github.com/nginxinc/nginx-gateway-kubernetes/internal/events" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" "github.com/nginxinc/nginx-gateway-kubernetes/internal/state/statefakes" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/status/statusfakes" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -30,6 +32,7 @@ func (r *unsupportedResource) DeepCopyObject() runtime.Object { var _ = Describe("EventLoop", func() { var ctrl *events.EventLoop var fakeConf *statefakes.FakeConfiguration + var fakeUpdater *statusfakes.FakeUpdater var cancel context.CancelFunc var eventCh chan interface{} var errorCh chan error @@ -37,7 +40,8 @@ var _ = Describe("EventLoop", func() { BeforeEach(func() { fakeConf = &statefakes.FakeConfiguration{} eventCh = make(chan interface{}) - ctrl = events.NewEventLoop(fakeConf, eventCh, zap.New()) + fakeUpdater = &statusfakes.FakeUpdater{} + ctrl = events.NewEventLoop(fakeConf, eventCh, fakeUpdater, zap.New()) var ctx context.Context @@ -59,6 +63,16 @@ var _ = Describe("EventLoop", func() { }) It("should process upsert event", func() { + fakeStatusUpdates := []state.StatusUpdate{ + { + NamespacedName: types.NamespacedName{}, + Status: nil, + }, + } + // for now, we pass nil, because we don't need to test how EventLoop processes changes yet. We will start + // testing once we have NGINX Configuration Manager component. + fakeConf.UpsertHTTPRouteReturns(nil, fakeStatusUpdates) + hr := &v1alpha2.HTTPRoute{} eventCh <- &events.UpsertEvent{ @@ -69,9 +83,25 @@ var _ = Describe("EventLoop", func() { Eventually(func() *v1alpha2.HTTPRoute { return fakeConf.UpsertHTTPRouteArgsForCall(0) }).Should(Equal(hr)) + + Eventually(fakeUpdater.ProcessStatusUpdatesCallCount()).Should(Equal(1)) + Eventually(func() []state.StatusUpdate { + _, updates := fakeUpdater.ProcessStatusUpdatesArgsForCall(0) + return updates + }).Should(Equal(fakeStatusUpdates)) }) It("should process delete event", func() { + fakeStatusUpdates := []state.StatusUpdate{ + { + NamespacedName: types.NamespacedName{}, + Status: nil, + }, + } + // for now, we pass nil, because we don't need to test how EventLoop processes changes yet. We will start + // testing once we have NGINX Configuration Manager component. + fakeConf.DeleteHTTPRouteReturns(nil, fakeStatusUpdates) + nsname := types.NamespacedName{Namespace: "test", Name: "route"} eventCh <- &events.DeleteEvent{ @@ -83,6 +113,12 @@ var _ = Describe("EventLoop", func() { Eventually(func() types.NamespacedName { return fakeConf.DeleteHTTPRouteArgsForCall(0) }).Should(Equal(nsname)) + + Eventually(fakeUpdater.ProcessStatusUpdatesCallCount()).Should(Equal(1)) + Eventually(func() []state.StatusUpdate { + _, updates := fakeUpdater.ProcessStatusUpdatesArgsForCall(0) + return updates + }).Should(Equal(fakeStatusUpdates)) }) }) diff --git a/internal/helpers/helpers.go b/internal/helpers/helpers.go new file mode 100644 index 0000000000..3196bdd0ce --- /dev/null +++ b/internal/helpers/helpers.go @@ -0,0 +1,15 @@ +package helpers + +import "github.com/google/go-cmp/cmp" + +// Diff prints the diff between two structs. +// It is useful in testing to compare two structs when they are large. In such a case, without Diff it will be difficult +// to pinpoint the difference between the two structs. +func Diff(x, y interface{}) string { + r := cmp.Diff(x, y) + + if r != "" { + return "(-want +got)\n" + r + } + return r +} diff --git a/internal/manager/manager.go b/internal/manager/manager.go index dc2a61ce5a..a88df40d4d 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -2,6 +2,7 @@ package manager import ( "fmt" + "time" "github.com/nginxinc/nginx-gateway-kubernetes/internal/config" "github.com/nginxinc/nginx-gateway-kubernetes/internal/events" @@ -10,6 +11,7 @@ import ( gcfg "github.com/nginxinc/nginx-gateway-kubernetes/internal/implementations/gatewayconfig" hr "github.com/nginxinc/nginx-gateway-kubernetes/internal/implementations/httproute" "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/status" nginxgwv1alpha1 "github.com/nginxinc/nginx-gateway-kubernetes/pkg/apis/gateway/v1alpha1" "github.com/nginxinc/nginx-gateway-kubernetes/pkg/sdk" @@ -19,6 +21,9 @@ import ( gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" ) +// clusterTimeout is a timeout for connections to the Kubernetes API +const clusterTimeout = 10 * time.Second + var scheme = runtime.NewScheme() func init() { @@ -35,7 +40,10 @@ func Start(cfg config.Config) error { eventCh := make(chan interface{}) - mgr, err := manager.New(ctlr.GetConfigOrDie(), options) + clusterCfg := ctlr.GetConfigOrDie() + clusterCfg.Timeout = clusterTimeout + + mgr, err := manager.New(clusterCfg, options) if err != nil { return fmt.Errorf("cannot build runtime manager: %w", err) } @@ -58,11 +66,12 @@ func Start(cfg config.Config) error { } conf := state.NewConfiguration(cfg.GatewayCtlrName, state.NewRealClock()) - mainCtrl := events.NewEventLoop(conf, eventCh, cfg.Logger) + reporter := status.NewUpdater(mgr.GetClient(), cfg.Logger) + eventLoop := events.NewEventLoop(conf, eventCh, reporter, cfg.Logger) - err = mgr.Add(mainCtrl) + err = mgr.Add(eventLoop) if err != nil { - return fmt.Errorf("cannot register main controller") + return fmt.Errorf("cannot register event loop: %w", err) } ctx := ctlr.SetupSignalHandler() diff --git a/internal/state/configuration.go b/internal/state/configuration.go index 667082307e..586bb1dce4 100644 --- a/internal/state/configuration.go +++ b/internal/state/configuration.go @@ -6,7 +6,6 @@ import ( "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/gateway-api/apis/v1alpha2" ) @@ -148,8 +147,8 @@ type Change struct { // StatusUpdate represents an update to the status of a resource. type StatusUpdate struct { - // Object is the resource. - Object runtime.Object + // NamespacedName is the NamespacedName of the resource. + NamespacedName types.NamespacedName // Status is the status field of the resource // The Status include only the new conditions. This means that the status reporter component will need to merge // the new conditions with the existing conditions of the resource. @@ -235,12 +234,16 @@ func (c *configurationImpl) updateListeners() ([]Change, []StatusUpdate) { // TO-DO: optimize it so that we only update the status of the affected (changed) httpRoutes // getSortedKeys is used to ensure predictable order for unit tests for _, key := range getSortedKeys(listener.httpRoutes) { + route := listener.httpRoutes[key] update := StatusUpdate{ - Object: listener.httpRoutes[key], + NamespacedName: types.NamespacedName{Namespace: route.Namespace, Name: route.Name}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { + ParentRef: v1alpha2.ParentRef{ + Name: "fake", // TO-DO: report the parent ref properly + }, ControllerName: v1alpha2.GatewayController(c.gatewayCtlrName), Conditions: []metav1.Condition{ { diff --git a/internal/state/configuration_test.go b/internal/state/configuration_test.go index 98eff5a46c..b37ac40596 100644 --- a/internal/state/configuration_test.go +++ b/internal/state/configuration_test.go @@ -3,7 +3,7 @@ package state_test import ( "time" - "github.com/google/go-cmp/cmp" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/helpers" "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" . "github.com/onsi/ginkgo/v2" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -95,12 +95,15 @@ var _ = Describe("Configuration", func() { } expectedStatusUpdates := []state.StatusUpdate{ { - Object: hr, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { ControllerName: gatewayCtlrName, + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), @@ -119,8 +122,8 @@ var _ = Describe("Configuration", func() { } changes, statusUpdates := conf.UpsertHTTPRoute(hr) - Expect(diff(expectedChanges, changes)).To(BeEmpty()) - Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) }) It("should not generate changes and status updates for the updated HTTPRoute because it has the same generation as the old one", func() { @@ -162,12 +165,15 @@ var _ = Describe("Configuration", func() { } expectedStatusUpdates := []state.StatusUpdate{ { - Object: updatedHRWithIncrementedGen, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { ControllerName: gatewayCtlrName, + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), @@ -186,8 +192,8 @@ var _ = Describe("Configuration", func() { } changes, statusUpdates := conf.UpsertHTTPRoute(updatedHRWithIncrementedGen) - Expect(diff(expectedChanges, changes)).To(BeEmpty()) - Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) }) It("should delete the host for the deleted HTTPRoute", func() { @@ -223,7 +229,7 @@ var _ = Describe("Configuration", func() { } changes, statusUpdates := conf.DeleteHTTPRoute(types.NamespacedName{Namespace: "test", Name: "route1"}) - Expect(diff(expectedChanges, changes)).To(BeEmpty()) + Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) Expect(statusUpdates).To(BeEmpty()) }) }) @@ -306,12 +312,15 @@ var _ = Describe("Configuration", func() { } expectedStatusUpdates := []state.StatusUpdate{ { - Object: hr1, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { ControllerName: gatewayCtlrName, + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), @@ -330,8 +339,8 @@ var _ = Describe("Configuration", func() { } changes, statusUpdates := conf.UpsertHTTPRoute(hr1) - Expect(diff(expectedChanges, changes)).To(BeEmpty()) - Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) }) It("should upsert the same host and generate status updates for both HTTPRoutes after adding the second", func() { @@ -367,12 +376,15 @@ var _ = Describe("Configuration", func() { } expectedStatusUpdates := []state.StatusUpdate{ { - Object: hr1, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { ControllerName: gatewayCtlrName, + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), @@ -389,12 +401,15 @@ var _ = Describe("Configuration", func() { }, }, { - Object: hr2, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route2"}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { ControllerName: gatewayCtlrName, + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), @@ -413,8 +428,8 @@ var _ = Describe("Configuration", func() { } changes, statusUpdates := conf.UpsertHTTPRoute(hr2) - Expect(diff(expectedChanges, changes)).To(BeEmpty()) - Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) }) It("should upsert the host and generate status updates for both HTTPRoutes after updating the second", func() { @@ -450,12 +465,15 @@ var _ = Describe("Configuration", func() { } expectedStatusUpdates := []state.StatusUpdate{ { - Object: hr1, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { ControllerName: gatewayCtlrName, + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), @@ -472,12 +490,15 @@ var _ = Describe("Configuration", func() { }, }, { - Object: hr2Updated, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route2"}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { ControllerName: gatewayCtlrName, + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), @@ -496,8 +517,8 @@ var _ = Describe("Configuration", func() { } changes, statusUpdates := conf.UpsertHTTPRoute(hr2Updated) - Expect(diff(expectedChanges, changes)).To(BeEmpty()) - Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) }) It("should upsert the host and generate a status updates for the first HTTPRoute after deleting the second", func() { @@ -523,12 +544,15 @@ var _ = Describe("Configuration", func() { } expectedStatusUpdates := []state.StatusUpdate{ { - Object: hr1, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { ControllerName: gatewayCtlrName, + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), @@ -547,8 +571,8 @@ var _ = Describe("Configuration", func() { } changes, statusUpdates := conf.DeleteHTTPRoute(types.NamespacedName{Namespace: "test", Name: "route2"}) - Expect(diff(expectedChanges, changes)).To(BeEmpty()) - Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) }) It("should delete the host after deleting the first HTTPRoute", func() { @@ -573,7 +597,7 @@ var _ = Describe("Configuration", func() { }, } changes, statusUpdates := conf.DeleteHTTPRoute(types.NamespacedName{Namespace: "test", Name: "route1"}) - Expect(diff(expectedChanges, changes)).To(BeEmpty()) + Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) Expect(statusUpdates).To(BeEmpty()) }) }) @@ -656,12 +680,15 @@ var _ = Describe("Configuration", func() { } expectedStatusUpdates := []state.StatusUpdate{ { - Object: hr1, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { ControllerName: gatewayCtlrName, + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), @@ -680,8 +707,8 @@ var _ = Describe("Configuration", func() { } changes, statusUpdates := conf.UpsertHTTPRoute(hr1) - Expect(diff(expectedChanges, changes)).To(BeEmpty()) - Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) }) It("should upsert the host (make the first HTTPRoute the winner for '/' rule) and generate status updates for both HTTPRoutes after adding the second", func() { @@ -712,12 +739,15 @@ var _ = Describe("Configuration", func() { } expectedStatusUpdates := []state.StatusUpdate{ { - Object: hr1, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { ControllerName: gatewayCtlrName, + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), @@ -734,12 +764,15 @@ var _ = Describe("Configuration", func() { }, }, { - Object: hr2, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route2"}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { ControllerName: gatewayCtlrName, + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), @@ -758,8 +791,8 @@ var _ = Describe("Configuration", func() { } changes, statusUpdates := conf.UpsertHTTPRoute(hr2) - Expect(diff(expectedChanges, changes)).To(BeEmpty()) - Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) }) It("should upsert the host (make the second HTTPRoute the winner for '/' rule) and generate status updates for both HTTPRoutes after updating the first", func() { @@ -790,12 +823,15 @@ var _ = Describe("Configuration", func() { } expectedStatusUpdates := []state.StatusUpdate{ { - Object: hr1Updated, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { ControllerName: gatewayCtlrName, + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), @@ -812,12 +848,15 @@ var _ = Describe("Configuration", func() { }, }, { - Object: hr2, + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route2"}, Status: &v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ Parents: []v1alpha2.RouteParentStatus{ { ControllerName: gatewayCtlrName, + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), @@ -836,25 +875,13 @@ var _ = Describe("Configuration", func() { } changes, statusUpdates := conf.UpsertHTTPRoute(hr1Updated) - Expect(diff(expectedChanges, changes)).To(BeEmpty()) - Expect(diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) + Expect(helpers.Diff(expectedChanges, changes)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatusUpdates, statusUpdates)).To(BeEmpty()) }) }) }) }) -// diff prints the diff between two structs. -// It is useful when the structs are large. In such a case, without diff it will be difficult to pinpoint the difference -// between the two structs. -func diff(x, y interface{}) string { - r := cmp.Diff(x, y) - - if r != "" { - return "(-want +got)\n" + r - } - return r -} - func getStringPointer(s string) *string { return &s } diff --git a/internal/status/status_suit_test.go b/internal/status/status_suit_test.go new file mode 100644 index 0000000000..6de1286f1d --- /dev/null +++ b/internal/status/status_suit_test.go @@ -0,0 +1,13 @@ +package status + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "testing" +) + +func TestState(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Status Suite") +} diff --git a/internal/status/statusfakes/fake_updater.go b/internal/status/statusfakes/fake_updater.go new file mode 100644 index 0000000000..71781c7f27 --- /dev/null +++ b/internal/status/statusfakes/fake_updater.go @@ -0,0 +1,85 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package statusfakes + +import ( + "context" + "sync" + + "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/status" +) + +type FakeUpdater struct { + ProcessStatusUpdatesStub func(context.Context, []state.StatusUpdate) + processStatusUpdatesMutex sync.RWMutex + processStatusUpdatesArgsForCall []struct { + arg1 context.Context + arg2 []state.StatusUpdate + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeUpdater) ProcessStatusUpdates(arg1 context.Context, arg2 []state.StatusUpdate) { + var arg2Copy []state.StatusUpdate + if arg2 != nil { + arg2Copy = make([]state.StatusUpdate, len(arg2)) + copy(arg2Copy, arg2) + } + fake.processStatusUpdatesMutex.Lock() + fake.processStatusUpdatesArgsForCall = append(fake.processStatusUpdatesArgsForCall, struct { + arg1 context.Context + arg2 []state.StatusUpdate + }{arg1, arg2Copy}) + stub := fake.ProcessStatusUpdatesStub + fake.recordInvocation("ProcessStatusUpdates", []interface{}{arg1, arg2Copy}) + fake.processStatusUpdatesMutex.Unlock() + if stub != nil { + fake.ProcessStatusUpdatesStub(arg1, arg2) + } +} + +func (fake *FakeUpdater) ProcessStatusUpdatesCallCount() int { + fake.processStatusUpdatesMutex.RLock() + defer fake.processStatusUpdatesMutex.RUnlock() + return len(fake.processStatusUpdatesArgsForCall) +} + +func (fake *FakeUpdater) ProcessStatusUpdatesCalls(stub func(context.Context, []state.StatusUpdate)) { + fake.processStatusUpdatesMutex.Lock() + defer fake.processStatusUpdatesMutex.Unlock() + fake.ProcessStatusUpdatesStub = stub +} + +func (fake *FakeUpdater) ProcessStatusUpdatesArgsForCall(i int) (context.Context, []state.StatusUpdate) { + fake.processStatusUpdatesMutex.RLock() + defer fake.processStatusUpdatesMutex.RUnlock() + argsForCall := fake.processStatusUpdatesArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeUpdater) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.processStatusUpdatesMutex.RLock() + defer fake.processStatusUpdatesMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeUpdater) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ status.Updater = new(FakeUpdater) diff --git a/internal/status/updater.go b/internal/status/updater.go new file mode 100644 index 0000000000..cd9c39de35 --- /dev/null +++ b/internal/status/updater.go @@ -0,0 +1,117 @@ +package status + +import ( + "context" + "fmt" + + "github.com/go-logr/logr" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/gateway-api/apis/v1alpha2" +) + +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Updater + +// Updater updates statuses of the Gateway API resources. +type Updater interface { + // ProcessStatusUpdates updates the statuses according to the provided updates. + ProcessStatusUpdates(context.Context, []state.StatusUpdate) +} + +// updaterImpl reports statuses for the Gateway API resources. +// +// It has the following limitations: +// +// (1) It doesn't understand the leader election. Only the leader must report the statuses of the resources. Otherwise, +// multiple replicas will step on each other when trying to report statuses for the same resources. +// TO-DO: address limitation (1) +// +// (2) It is synchronous, which means the status reporter can slow down the event loop. +// Consider the following cases: +// (a) Sometimes the Gateway will need to update statuses of all resources it handles, which could be ~1000. Making 1000 +// status API calls sequentially will take time. +// (b) k8s API can become slow or even timeout. This will increase every update status API call. +// Making updaterImpl asynchronous will prevent it from adding variable delays to the event loop. +// TO-DO: address limitation (2) +// +// (3) It doesn't retry on failures. This means there is a chance that some resources will not have up-to-do statuses. +// Statuses are important part of the Gateway API, so we need to ensure that the Gateway always keep the resources +// statuses up-to-date. +// TO-DO: address limitation (3) +// +// (4) To support new resources, updaterImpl needs to be modified. Consider making updaterImpl extendable, so that it +// goes along the Open-closed principle. +// TO-DO: address limitation (4) +type updaterImpl struct { + client client.Client + logger logr.Logger +} + +// NewUpdater creates a new Updater. +func NewUpdater(client client.Client, logger logr.Logger) Updater { + return &updaterImpl{ + client: client, + logger: logger.WithName("statusUpdater"), + } +} + +// ProcessStatusUpdates updates the statuses according to the provided updates. +func (upd *updaterImpl) ProcessStatusUpdates(ctx context.Context, updates []state.StatusUpdate) { + for _, u := range updates { + select { + case <-ctx.Done(): + return + default: + } + + switch s := u.Status.(type) { + case *v1alpha2.HTTPRouteStatus: + upd.logger.Info("Processing a status update for HTTPRoute", + "namespace", u.NamespacedName.Namespace, + "name", u.NamespacedName.Name) + + var hr v1alpha2.HTTPRoute + + upd.update(ctx, u.NamespacedName, &hr, func(object client.Object) { + route := object.(*v1alpha2.HTTPRoute) + // TO-DO: merge the conditions in the status with the conditions in the route.Status properly, because + // right now, we are replacing the conditions. + route.Status = *s + }) + default: + panic(fmt.Errorf("unknown status type %T", u.Status)) + } + } +} + +func (upd *updaterImpl) update(ctx context.Context, nsname types.NamespacedName, obj client.Object, statusSetter func(client.Object)) { + // The function handles errors by reporting them in the logs. + // TO-DO: figure out appropriate log level for these errors. Perhaps 3? + + // We need to get the latest version of the resource. + // 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) + if err != nil { + if !apierrors.IsNotFound(err) { + upd.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) + } + return + } + + statusSetter(obj) + + err = upd.client.Status().Update(ctx, obj) + if err != nil { + upd.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 new file mode 100644 index 0000000000..71d8aa4ea8 --- /dev/null +++ b/internal/status/updater_test.go @@ -0,0 +1,166 @@ +package status_test + +import ( + "context" + + "github.com/nginxinc/nginx-gateway-kubernetes/internal/helpers" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/status" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" +) + +var _ = Describe("Updater", func() { + var updater status.Updater + var client client.Client + + BeforeEach(OncePerOrdered, func() { + scheme := runtime.NewScheme() + + Expect(gatewayv1alpha2.AddToScheme(scheme)).Should(Succeed()) + + client = fake.NewClientBuilder(). + WithScheme(scheme). + Build() + updater = status.NewUpdater(client, zap.New()) + }) + + Describe("Process status update of HTTPRoute", Ordered, func() { + var hr *v1alpha2.HTTPRoute + var testTime metav1.Time + + BeforeAll(func() { + // Rfc3339Copy() removes the monotonic clock reading + // it is important, because updating the status in the FakeClient and then getting the resource back + // involves encoding and decoding the resource to/from JSON, which removes the monotonic clock reading. + testTime = metav1.Now().Rfc3339Copy() + + hr = &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "route1", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "HTTPRoute", + APIVersion: "gateway.networking.k8s.io/v1alpha2", + }, + } + + Expect(client.Create(context.Background(), hr)).Should(Succeed()) + }) + + It("should process status update", func() { + updates := []state.StatusUpdate{ + { + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, + Status: &v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ControllerName: "test", + ParentRef: v1alpha2.ParentRef{ + Name: "fake", + }, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: hr.Generation, + LastTransitionTime: testTime, + Reason: string(v1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + }, + } + + updater.ProcessStatusUpdates(context.Background(), updates) + }) + + It("should have the updated status in the API server", func() { + expectedHR := &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "route1", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "HTTPRoute", + APIVersion: "gateway.networking.k8s.io/v1alpha2", + }, + Status: gatewayv1alpha2.HTTPRouteStatus{ + RouteStatus: gatewayv1alpha2.RouteStatus{ + Parents: []gatewayv1alpha2.RouteParentStatus{ + { + ControllerName: "test", + ParentRef: gatewayv1alpha2.ParentRef{ + Name: "fake", + }, + Conditions: []metav1.Condition{ + { + Type: string(gatewayv1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: 0, + LastTransitionTime: testTime, + Reason: string(gatewayv1alpha2.ConditionRouteAccepted), + Message: "", + }, + }, + }, + }, + }, + }, + } + + latestHR := &v1alpha2.HTTPRoute{} + + err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "route1"}, latestHR) + Expect(err).Should(Not(HaveOccurred())) + + expectedHR.ResourceVersion = latestHR.ResourceVersion // updating the status changes the ResourceVersion + + Expect(helpers.Diff(expectedHR, latestHR)).To(BeEmpty()) + }) + }) + + It("should panic for unknown status type", func() { + updates := []state.StatusUpdate{ + { + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, + Status: "unsupported", + }, + } + + process := func() { + updater.ProcessStatusUpdates(context.Background(), updates) + } + Expect(process).Should(Panic()) + }) + + It("should not process updates with canceled context", func() { + updates := []state.StatusUpdate{ + { + NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, + Status: "unsupported", + }, + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + // because the ctx is canceled, ProcessStatusUpdates should return immediately without panicking + // because of the unsupported status type + updater.ProcessStatusUpdates(ctx, updates) + }) +})