From eca8cd46a5b96ae8678b11a9dffc2988b9e275af Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 2 Jun 2022 17:12:22 -0600 Subject: [PATCH 1/4] Update status reporting to use new types This commit updates status reporting to use the Statuses type, introduced in 0a39e3f, as a source of status information about resources. --- deploy/manifests/nginx-gateway.yaml | 1 + internal/events/loop.go | 12 +- internal/events/loop_test.go | 12 +- internal/manager/manager.go | 4 +- internal/newstate/change_processor.go | 9 + internal/newstate/change_processor_test.go | 129 ++++++++++- internal/status/clock.go | 37 +++ internal/status/clock_test.go | 16 ++ internal/status/gateway.go | 65 ++++++ internal/status/gateway_test.go | 74 ++++++ internal/status/httproute.go | 73 ++++++ internal/status/httproute_test.go | 79 +++++++ internal/status/statusfakes/fake_updater.go | 61 +++-- internal/status/updater.go | 90 +++++--- internal/status/updater_test.go | 238 +++++++++++++------- 15 files changed, 734 insertions(+), 166 deletions(-) create mode 100644 internal/status/clock.go create mode 100644 internal/status/clock_test.go create mode 100644 internal/status/gateway.go create mode 100644 internal/status/gateway_test.go create mode 100644 internal/status/httproute.go create mode 100644 internal/status/httproute_test.go diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index 329854855f..b879896fb3 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -36,6 +36,7 @@ rules: - gateway.networking.k8s.io resources: - httproutes/status + - gateways/status verbs: - update --- diff --git a/internal/events/loop.go b/internal/events/loop.go index 487a2f69d8..a984aea634 100644 --- a/internal/events/loop.go +++ b/internal/events/loop.go @@ -13,6 +13,7 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file" "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/runtime" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/status" ) // EventLoop is the main event loop of the Gateway. @@ -24,6 +25,7 @@ type EventLoop struct { logger logr.Logger nginxFileMgr file.Manager nginxRuntimeMgr runtime.Manager + statusUpdater status.Updater } // NewEventLoop creates a new EventLoop. @@ -35,6 +37,7 @@ func NewEventLoop( logger logr.Logger, nginxFileMgr file.Manager, nginxRuntimeMgr runtime.Manager, + statusUpdater status.Updater, ) *EventLoop { return &EventLoop{ processor: processor, @@ -44,6 +47,7 @@ func NewEventLoop( logger: logger.WithName("eventLoop"), nginxFileMgr: nginxFileMgr, nginxRuntimeMgr: nginxRuntimeMgr, + statusUpdater: statusUpdater, } } @@ -85,13 +89,7 @@ func (el *EventLoop) handleEvent(ctx context.Context, event interface{}) { el.logger.Error(err, "Failed to update NGINX configuration") } - // FIXME(pleshakov) Update resource statuses instead of printing to stdout - for name, s := range statuses.ListenerStatuses { - fmt.Printf("Listener %q, Statuses: %v\n", name, s) - } - for nsname, s := range statuses.HTTPRouteStatuses { - fmt.Printf("HTTPRoute %q, Statuses: %v\n", nsname, s) - } + el.statusUpdater.Update(ctx, statuses) } func (el *EventLoop) updateNginx(ctx context.Context, conf newstate.Configuration) error { diff --git a/internal/events/loop_test.go b/internal/events/loop_test.go index 8d0467efa4..089dea826d 100644 --- a/internal/events/loop_test.go +++ b/internal/events/loop_test.go @@ -21,6 +21,7 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file/filefakes" "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/runtime/runtimefakes" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/statefakes" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/status/statusfakes" ) type unsupportedResource struct { @@ -42,6 +43,7 @@ var _ = Describe("EventLoop", func() { fakeGenerator *configfakes.FakeGenerator fakeNginxFimeMgr *filefakes.FakeManager fakeNginxRuntimeMgr *runtimefakes.FakeManager + fakeStatusUpdater *statusfakes.FakeUpdater cancel context.CancelFunc eventCh chan interface{} errorCh chan error @@ -55,7 +57,8 @@ var _ = Describe("EventLoop", func() { fakeGenerator = &configfakes.FakeGenerator{} fakeNginxFimeMgr = &filefakes.FakeManager{} fakeNginxRuntimeMgr = &runtimefakes.FakeManager{} - ctrl := events.NewEventLoop(fakeProcessor, fakeServiceStore, fakeGenerator, eventCh, zap.New(), fakeNginxFimeMgr, fakeNginxRuntimeMgr) + fakeStatusUpdater = &statusfakes.FakeUpdater{} + ctrl := events.NewEventLoop(fakeProcessor, fakeServiceStore, fakeGenerator, eventCh, zap.New(), fakeNginxFimeMgr, fakeNginxRuntimeMgr, fakeStatusUpdater) var ctx context.Context ctx, cancel = context.WithCancel(context.Background()) @@ -82,7 +85,8 @@ var _ = Describe("EventLoop", func() { func(e *events.UpsertEvent) { fakeConf := newstate.Configuration{} changed := true - fakeProcessor.ProcessReturns(changed, fakeConf, newstate.Statuses{}) + fakeStatuses := newstate.Statuses{} + fakeProcessor.ProcessReturns(changed, fakeConf, fakeStatuses) fakeCfg := []byte("fake") fakeGenerator.GenerateReturns(fakeCfg, config.Warnings{}) @@ -102,6 +106,10 @@ var _ = Describe("EventLoop", func() { Expect(cfg).Should(Equal(fakeCfg)) Eventually(fakeNginxRuntimeMgr.ReloadCallCount).Should(Equal(1)) + + Eventually(fakeStatusUpdater.UpdateCallCount).Should(Equal(1)) + _, statuses := fakeStatusUpdater.UpdateArgsForCall(0) + Expect(statuses).Should(Equal(fakeStatuses)) }, Entry("HTTPRoute", &events.UpsertEvent{Resource: &v1alpha2.HTTPRoute{}}), Entry("Gateway", &events.UpsertEvent{Resource: &v1alpha2.Gateway{}}), diff --git a/internal/manager/manager.go b/internal/manager/manager.go index 0df1ff82d9..bb42fea453 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -20,6 +20,7 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file" ngxruntime "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/runtime" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/status" "github.com/nginxinc/nginx-kubernetes-gateway/pkg/sdk" ) @@ -69,7 +70,8 @@ func Start(cfg config.Config) error { configGenerator := ngxcfg.NewGeneratorImpl(serviceStore) nginxFileMgr := file.NewManagerImpl() nginxRuntimeMgr := ngxruntime.NewManagerImpl() - eventLoop := events.NewEventLoop(processor, serviceStore, configGenerator, eventCh, cfg.Logger, nginxFileMgr, nginxRuntimeMgr) + statusUpdater := status.NewUpdater(cfg.GatewayCtlrName, cfg.GatewayNsName, mgr.GetClient(), cfg.Logger, status.NewRealClock()) + eventLoop := events.NewEventLoop(processor, serviceStore, configGenerator, eventCh, cfg.Logger, nginxFileMgr, nginxRuntimeMgr, statusUpdater) err = mgr.Add(eventLoop) if err != nil { diff --git a/internal/newstate/change_processor.go b/internal/newstate/change_processor.go index 4e8daa4109..f17d1b9aff 100644 --- a/internal/newstate/change_processor.go +++ b/internal/newstate/change_processor.go @@ -56,8 +56,17 @@ func (c *ChangeProcessorImpl) CaptureUpsertChange(obj client.Object) { if o.Namespace != c.gwNsName.Namespace || o.Name != c.gwNsName.Name { panic(fmt.Errorf("gateway resource must be %s/%s, got %s/%s", c.gwNsName.Namespace, c.gwNsName.Name, o.Namespace, o.Name)) } + // if the resource spec hasn't changed (its generation is the same), ignore the upsert + if c.store.gw != nil && c.store.gw.Generation == o.Generation { + c.changed = false + } c.store.gw = o case *v1alpha2.HTTPRoute: + // if the resource spec hasn't changed (its generation is the same), ignore the upsert + prev, exist := c.store.httpRoutes[getNamespacedName(obj)] + if exist && o.Generation == prev.Generation { + c.changed = false + } c.store.httpRoutes[getNamespacedName(obj)] = o default: panic(fmt.Errorf("ChangeProcessor doesn't support %T", obj)) diff --git a/internal/newstate/change_processor_test.go b/internal/newstate/change_processor_test.go index d04a0d4168..28e831cf2f 100644 --- a/internal/newstate/change_processor_test.go +++ b/internal/newstate/change_processor_test.go @@ -14,9 +14,11 @@ import ( var _ = Describe("ChangeProcessor", func() { Describe("Normal cases of processing changes", func() { - var hr1, hr2 *v1alpha2.HTTPRoute - var gw *v1alpha2.Gateway - var processor newstate.ChangeProcessor + var ( + hr1, hr1Updated, hr2 *v1alpha2.HTTPRoute + gw, gwUpdated *v1alpha2.Gateway + processor newstate.ChangeProcessor + ) BeforeEach(OncePerOrdered, func() { createRoute := func(name string, hostname string) *v1alpha2.HTTPRoute { @@ -54,6 +56,10 @@ var _ = Describe("ChangeProcessor", func() { } hr1 = createRoute("hr-1", "foo.example.com") + + hr1Updated = hr1.DeepCopy() + hr1Updated.Generation++ + hr2 = createRoute("hr-2", "bar.example.com") gw = &v1alpha2.Gateway{ @@ -73,6 +79,9 @@ var _ = Describe("ChangeProcessor", func() { }, } + gwUpdated = gw.DeepCopy() + gwUpdated.Generation++ + processor = newstate.NewChangeProcessorImpl(types.NamespacedName{Namespace: "test", Name: "gateway"}) }) @@ -149,6 +158,116 @@ var _ = Describe("ChangeProcessor", func() { Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) }) + It("should return empty configuration and statuses after processing upserting the HTTPRoute without generation change", func() { + hr1UpdatedSameGen := hr1.DeepCopy() + // hr1UpdatedSameGen.Generation has not been changed + processor.CaptureUpsertChange(hr1UpdatedSameGen) + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeFalse()) + Expect(conf).To(BeZero()) + Expect(statuses).To(BeZero()) + }) + + It("should return updated configuration and statuses after upserting the HTTPRoute with generation change", func() { + processor.CaptureUpsertChange(hr1Updated) + + expectedConf := newstate.Configuration{ + HTTPServers: []newstate.HTTPServer{ + { + Hostname: "foo.example.com", + PathRules: []newstate.PathRule{ + { + Path: "/", + MatchRules: []newstate.MatchRule{ + { + MatchIdx: 0, + RuleIdx: 0, + Source: hr1Updated, + }, + }, + }, + }, + }, + }, + } + expectedStatuses := newstate.Statuses{ + ListenerStatuses: map[string]newstate.ListenerStatus{ + "listener-80-1": { + Valid: true, + AttachedRoutes: 1, + }, + }, + HTTPRouteStatuses: map[types.NamespacedName]newstate.HTTPRouteStatus{ + {Namespace: "test", Name: "hr-1"}: { + ParentStatuses: map[string]newstate.ParentStatus{ + "listener-80-1": {Attached: true}, + }, + }, + }, + } + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + }) + + It("should return empty configuration and statuses after processing upserting the Gateway without generation change", func() { + gwUpdatedSameGen := gw.DeepCopy() + // gwUpdatedSameGen.Generation has not been changed + processor.CaptureUpsertChange(gwUpdatedSameGen) + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeFalse()) + Expect(conf).To(BeZero()) + Expect(statuses).To(BeZero()) + }) + + It("should return updated configuration and statuses after upserting the Gateway with generation change", func() { + processor.CaptureUpsertChange(gwUpdated) + + expectedConf := newstate.Configuration{ + HTTPServers: []newstate.HTTPServer{ + { + Hostname: "foo.example.com", + PathRules: []newstate.PathRule{ + { + Path: "/", + MatchRules: []newstate.MatchRule{ + { + MatchIdx: 0, + RuleIdx: 0, + Source: hr1Updated, + }, + }, + }, + }, + }, + }, + } + expectedStatuses := newstate.Statuses{ + ListenerStatuses: map[string]newstate.ListenerStatus{ + "listener-80-1": { + Valid: true, + AttachedRoutes: 1, + }, + }, + HTTPRouteStatuses: map[types.NamespacedName]newstate.HTTPRouteStatus{ + {Namespace: "test", Name: "hr-1"}: { + ParentStatuses: map[string]newstate.ParentStatus{ + "listener-80-1": {Attached: true}, + }, + }, + }, + } + + changed, conf, statuses := processor.Process() + Expect(changed).To(BeTrue()) + Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) + Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + }) + It("should return empty configuration and statuses after processing without capturing any changes", func() { changed, conf, statuses := processor.Process() @@ -186,7 +305,7 @@ var _ = Describe("ChangeProcessor", func() { { MatchIdx: 0, RuleIdx: 0, - Source: hr1, + Source: hr1Updated, }, }, }, @@ -235,7 +354,7 @@ var _ = Describe("ChangeProcessor", func() { { MatchIdx: 0, RuleIdx: 0, - Source: hr1, + Source: hr1Updated, }, }, }, diff --git a/internal/status/clock.go b/internal/status/clock.go new file mode 100644 index 0000000000..5e29c81069 --- /dev/null +++ b/internal/status/clock.go @@ -0,0 +1,37 @@ +package status + +import "time" + +// Clock returns the current local time. +type Clock interface { + Now() time.Time +} + +// Real clock returns the current local time. +type RealClock struct { +} + +// NewRealClock creates a new RealClock. +func NewRealClock() *RealClock { + return &RealClock{} +} + +// Now returns the current local time. +func (c *RealClock) Now() time.Time { + return time.Now() +} + +// FakeClock allows you to control the returned time. +type FakeClock struct { + time time.Time +} + +// NewFakeClock creates a FakeClock. The clock will always return the specified time. +func NewFakeClock(time time.Time) *FakeClock { + return &FakeClock{time: time} +} + +// Now is a fake implementation of Now(). +func (c FakeClock) Now() time.Time { + return c.time +} diff --git a/internal/status/clock_test.go b/internal/status/clock_test.go new file mode 100644 index 0000000000..fb44085128 --- /dev/null +++ b/internal/status/clock_test.go @@ -0,0 +1,16 @@ +package status + +import ( + "testing" + "time" +) + +func TestFakeClock(t *testing.T) { + time := time.Now() + clock := NewFakeClock(time) + + result := clock.Now() + if result != time { + t.Errorf("Now() returned %v but expected %v", result, time) + } +} diff --git a/internal/status/gateway.go b/internal/status/gateway.go new file mode 100644 index 0000000000..d1e97d9667 --- /dev/null +++ b/internal/status/gateway.go @@ -0,0 +1,65 @@ +package status + +import ( + "sort" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/newstate" +) + +// prepareGatewayStatus prepares the status for a Gateway resource. +// FIXME(pleshakov): Be compliant with in the Gateway API. +// Currently, we only support simple valid/invalid status per each listener. +// Extend support to cover more cases. +func prepareGatewayStatus(statuses newstate.ListenerStatuses, clock Clock) v1alpha2.GatewayStatus { + listenerStatuses := make([]v1alpha2.ListenerStatus, 0, len(statuses)) + + // FIXME(pleshakov) Maintain the order from the Gateway resource + names := make([]string, 0, len(statuses)) + for name := range statuses { + names = append(names, name) + } + sort.Strings(names) + + for _, name := range names { + s := statuses[name] + + var status, reason string + + if s.Valid { + status = "True" + reason = string(v1alpha2.ListenerReasonReady) + } else { + status = "False" + reason = string(v1alpha2.ListenerReasonInvalid) + } + + cond := metav1.Condition{ + Type: string(v1alpha2.ListenerConditionReady), + Status: metav1.ConditionStatus(status), + // FIXME(pleshakov) Set the observed generation to the last processed generation of the Gateway resource. + ObservedGeneration: 123, + LastTransitionTime: metav1.NewTime(clock.Now()), + Reason: reason, + Message: "", // FIXME(pleshakov) Come up with a good message + } + + listenerStatuses = append(listenerStatuses, v1alpha2.ListenerStatus{ + Name: v1alpha2.SectionName(name), + SupportedKinds: []v1alpha2.RouteGroupKind{ + { + Kind: "HTTPRoute", // FIXME(pleshakov) Set it based on the listener + }, + }, + AttachedRoutes: s.AttachedRoutes, + Conditions: []metav1.Condition{cond}, + }) + } + + return v1alpha2.GatewayStatus{ + Listeners: listenerStatuses, + Conditions: nil, // FIXME(pleshakov) Create conditions for the Gateway resource. + } +} diff --git a/internal/status/gateway_test.go b/internal/status/gateway_test.go new file mode 100644 index 0000000000..ad179793ee --- /dev/null +++ b/internal/status/gateway_test.go @@ -0,0 +1,74 @@ +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/newstate" +) + +func TestPrepareGatewayStatus(t *testing.T) { + statuses := newstate.ListenerStatuses{ + "valid-listener": { + Valid: true, + AttachedRoutes: 2, + }, + "invalid-listener": { + Valid: false, + AttachedRoutes: 1, + }, + } + + expectedTime := time.Now() + clock := NewFakeClock(expectedTime) + + expected := v1alpha2.GatewayStatus{ + Listeners: []v1alpha2.ListenerStatus{ + { + Name: "invalid-listener", + SupportedKinds: []v1alpha2.RouteGroupKind{ + { + Kind: "HTTPRoute", + }, + }, + AttachedRoutes: 1, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ListenerConditionReady), + Status: "False", + ObservedGeneration: 123, + LastTransitionTime: metav1.Time{Time: expectedTime}, + Reason: string(v1alpha2.ListenerReasonInvalid), + }, + }, + }, + { + Name: "valid-listener", + SupportedKinds: []v1alpha2.RouteGroupKind{ + { + Kind: "HTTPRoute", + }, + }, + AttachedRoutes: 2, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ListenerConditionReady), + Status: "True", + ObservedGeneration: 123, + LastTransitionTime: metav1.Time{Time: expectedTime}, + Reason: string(v1alpha2.ListenerReasonReady), + }, + }, + }, + }, + } + + result := prepareGatewayStatus(statuses, clock) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("prepareGatewayStatus() mismatch (-want +got):\n%s", diff) + } +} diff --git a/internal/status/httproute.go b/internal/status/httproute.go new file mode 100644 index 0000000000..35a31c0582 --- /dev/null +++ b/internal/status/httproute.go @@ -0,0 +1,73 @@ +package status + +import ( + "sort" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/newstate" +) + +// prepareHTTPRouteStatus prepares the status for an HTTPRoute resource. +// FIXME(pleshakov): Be compliant with in the Gateway API. +// Currently, we only support simple attached/not attached status per each parentRef. +// Extend support to cover more cases. +func prepareHTTPRouteStatus( + status newstate.HTTPRouteStatus, + gwNsName types.NamespacedName, + gatewayCtlrName string, + clock Clock, +) v1alpha2.HTTPRouteStatus { + parents := make([]v1alpha2.RouteParentStatus, 0, len(status.ParentStatuses)) + + // FIXME(pleshakov) Maintain the order from the HTTPRoute resource + names := make([]string, 0, len(status.ParentStatuses)) + for name := range status.ParentStatuses { + names = append(names, name) + } + sort.Strings(names) + + for _, name := range names { + ps := status.ParentStatuses[name] + + var status, reason string + if ps.Attached { + status = "True" + reason = "Attached" + } else { + status = "False" + reason = "Not attached" + } + + sectionName := name + + p := v1alpha2.RouteParentStatus{ + ParentRef: v1alpha2.ParentRef{ + Namespace: (*v1alpha2.Namespace)(&gwNsName.Namespace), + Name: v1alpha2.ObjectName(gwNsName.Name), + SectionName: (*v1alpha2.SectionName)(§ionName), + }, + ControllerName: v1alpha2.GatewayController(gatewayCtlrName), + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: metav1.ConditionStatus(status), + // FIXME(pleshakov) Set the observed generation to the last processed generation of the HTTPRoute resource. + ObservedGeneration: 123, + LastTransitionTime: metav1.NewTime(clock.Now()), + Reason: reason, + Message: "", // FIXME(pleshakov): Figure out a good message + }, + }, + } + parents = append(parents, p) + } + + return v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: parents, + }, + } +} diff --git a/internal/status/httproute_test.go b/internal/status/httproute_test.go new file mode 100644 index 0000000000..21cadd08d6 --- /dev/null +++ b/internal/status/httproute_test.go @@ -0,0 +1,79 @@ +package status + +import ( + "testing" + "time" + + "github.com/google/go-cmp/cmp" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/newstate" +) + +func TestPrepareHTTPRouteStatus(t *testing.T) { + status := newstate.HTTPRouteStatus{ + ParentStatuses: map[string]newstate.ParentStatus{ + "attached": { + Attached: true, + }, + "not-attached": { + Attached: false, + }, + }, + } + + gwNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} + gatewayCtlrName := "test.example.com" + + expectedTime := time.Now() + clock := NewFakeClock(expectedTime) + + expected := v1alpha2.HTTPRouteStatus{ + RouteStatus: v1alpha2.RouteStatus{ + Parents: []v1alpha2.RouteParentStatus{ + { + ParentRef: v1alpha2.ParentRef{ + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: "gateway", + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("attached")), + }, + ControllerName: v1alpha2.GatewayController(gatewayCtlrName), + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "True", + ObservedGeneration: 123, + LastTransitionTime: metav1.Time{Time: expectedTime}, + Reason: "Attached", + }, + }, + }, + { + ParentRef: v1alpha2.ParentRef{ + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: "gateway", + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("not-attached")), + }, + ControllerName: v1alpha2.GatewayController(gatewayCtlrName), + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.ConditionRouteAccepted), + Status: "False", + ObservedGeneration: 123, + LastTransitionTime: metav1.Time{Time: expectedTime}, + Reason: "Not attached", + }, + }, + }, + }, + }, + } + + result := prepareHTTPRouteStatus(status, gwNsName, gatewayCtlrName, clock) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("prepareHTTPRouteStatus() mismatch (-want +got):\n%s", diff) + } +} diff --git a/internal/status/statusfakes/fake_updater.go b/internal/status/statusfakes/fake_updater.go index f9d916ee43..5f31b6eaf9 100644 --- a/internal/status/statusfakes/fake_updater.go +++ b/internal/status/statusfakes/fake_updater.go @@ -5,64 +5,59 @@ import ( "context" "sync" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/newstate" "github.com/nginxinc/nginx-kubernetes-gateway/internal/status" ) type FakeUpdater struct { - ProcessStatusUpdatesStub func(context.Context, []state.StatusUpdate) - processStatusUpdatesMutex sync.RWMutex - processStatusUpdatesArgsForCall []struct { + UpdateStub func(context.Context, newstate.Statuses) + updateMutex sync.RWMutex + updateArgsForCall []struct { arg1 context.Context - arg2 []state.StatusUpdate + arg2 newstate.Statuses } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } -func (fake *FakeUpdater) ProcessStatusUpdates(arg1 context.Context, arg2 []state.StatusUpdate) { - var arg2Copy []state.StatusUpdate - if arg2 != nil { - arg2Copy = make([]state.StatusUpdate, len(arg2)) - copy(arg2Copy, arg2) - } - fake.processStatusUpdatesMutex.Lock() - fake.processStatusUpdatesArgsForCall = append(fake.processStatusUpdatesArgsForCall, struct { +func (fake *FakeUpdater) Update(arg1 context.Context, arg2 newstate.Statuses) { + fake.updateMutex.Lock() + fake.updateArgsForCall = append(fake.updateArgsForCall, struct { arg1 context.Context - arg2 []state.StatusUpdate - }{arg1, arg2Copy}) - stub := fake.ProcessStatusUpdatesStub - fake.recordInvocation("ProcessStatusUpdates", []interface{}{arg1, arg2Copy}) - fake.processStatusUpdatesMutex.Unlock() + arg2 newstate.Statuses + }{arg1, arg2}) + stub := fake.UpdateStub + fake.recordInvocation("Update", []interface{}{arg1, arg2}) + fake.updateMutex.Unlock() if stub != nil { - fake.ProcessStatusUpdatesStub(arg1, arg2) + fake.UpdateStub(arg1, arg2) } } -func (fake *FakeUpdater) ProcessStatusUpdatesCallCount() int { - fake.processStatusUpdatesMutex.RLock() - defer fake.processStatusUpdatesMutex.RUnlock() - return len(fake.processStatusUpdatesArgsForCall) +func (fake *FakeUpdater) UpdateCallCount() int { + fake.updateMutex.RLock() + defer fake.updateMutex.RUnlock() + return len(fake.updateArgsForCall) } -func (fake *FakeUpdater) ProcessStatusUpdatesCalls(stub func(context.Context, []state.StatusUpdate)) { - fake.processStatusUpdatesMutex.Lock() - defer fake.processStatusUpdatesMutex.Unlock() - fake.ProcessStatusUpdatesStub = stub +func (fake *FakeUpdater) UpdateCalls(stub func(context.Context, newstate.Statuses)) { + fake.updateMutex.Lock() + defer fake.updateMutex.Unlock() + fake.UpdateStub = stub } -func (fake *FakeUpdater) ProcessStatusUpdatesArgsForCall(i int) (context.Context, []state.StatusUpdate) { - fake.processStatusUpdatesMutex.RLock() - defer fake.processStatusUpdatesMutex.RUnlock() - argsForCall := fake.processStatusUpdatesArgsForCall[i] +func (fake *FakeUpdater) UpdateArgsForCall(i int) (context.Context, newstate.Statuses) { + fake.updateMutex.RLock() + defer fake.updateMutex.RUnlock() + argsForCall := fake.updateArgsForCall[i] return argsForCall.arg1, argsForCall.arg2 } func (fake *FakeUpdater) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() - fake.processStatusUpdatesMutex.RLock() - defer fake.processStatusUpdatesMutex.RUnlock() + fake.updateMutex.RLock() + defer fake.updateMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} for key, value := range fake.invocations { copiedInvocations[key] = value diff --git a/internal/status/updater.go b/internal/status/updater.go index fede7f298e..6246f22e02 100644 --- a/internal/status/updater.go +++ b/internal/status/updater.go @@ -2,7 +2,6 @@ package status import ( "context" - "fmt" "github.com/go-logr/logr" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -10,18 +9,18 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/gateway-api/apis/v1alpha2" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/newstate" ) //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) + // Update updates the statuses of the resources. + Update(context.Context, newstate.Statuses) } -// updaterImpl reports statuses for the Gateway API resources. +// updaterImpl updates statuses of the Gateway API resources. // // It has the following limitations: // @@ -29,61 +28,80 @@ type Updater interface { // multiple replicas will step on each other when trying to report statuses for the same resources. // FIXME(pleshakov): address limitation (1) // -// (2) It is synchronous, which means the status reporter can slow down the event loop. +// (2) It is not smart. It will update the status of a resource (make an API call) even if it hasn't changed. +// FIXME(pleshakov) address limitation (2) +// +// (3) It is synchronous, which means the status reporter can slow down the event loop. // Consider the following cases: // (a) Sometimes the Gateway will need to update statuses of all resources it handles, which could be ~1000. Making 1000 // status API calls sequentially will take time. // (b) k8s API can become slow or even timeout. This will increase every update status API call. // Making updaterImpl asynchronous will prevent it from adding variable delays to the event loop. -// FIXME(pleshakov) address limitation (2) +// FIXME(pleshakov) address limitation (3) // -// (3) It doesn't retry on failures. This means there is a chance that some resources will not have up-to-do statuses. +// (4) It doesn't retry on failures. This means there is a chance that some resources will not have up-to-do statuses. // Statuses are important part of the Gateway API, so we need to ensure that the Gateway always keep the resources // statuses up-to-date. -// FIXME(pleshakov): address limitation (3) +// FIXME(pleshakov): address limitation (4) // -// (4) To support new resources, updaterImpl needs to be modified. Consider making updaterImpl extendable, so that it +// (5) It doesn't clear the statuses of a resources that are no longer handled by the Gateway. For example, if +// an HTTPRoute resource no longer has the parentRef to the Gateway resources, the Gateway must update the status +// of the resource to remove the status about the removed parentRef. +// FIXME(pleshakov): address limitation (5) +// +// (6) If another controllers changes the status of the Gateway/HTTPRoute resource so that the information set by our +// Gateway is removed, our Gateway will not restore the status until the EventLoop invokes the StatusUpdater as a +// result of processing some other new change to a resource(s). +// FIXME(pleshakov): Figure out if this is something that needs to be addressed. + +// (7) To support new resources, updaterImpl needs to be modified. Consider making updaterImpl extendable, so that it // goes along the Open-closed principle. -// FIXME(pleshakov): address limitation (4) +// FIXME(pleshakov): address limitation (7) type updaterImpl struct { - client client.Client - logger logr.Logger + gatewayCtlrName string + gwNsName types.NamespacedName + client client.Client + logger logr.Logger + clock Clock } // NewUpdater creates a new Updater. -func NewUpdater(client client.Client, logger logr.Logger) Updater { +func NewUpdater( + gatewayCtlrName string, + gwNsName types.NamespacedName, + client client.Client, + logger logr.Logger, + clock Clock, +) Updater { return &updaterImpl{ - client: client, - logger: logger.WithName("statusUpdater"), + gatewayCtlrName: gatewayCtlrName, + gwNsName: gwNsName, + client: client, + logger: logger.WithName("statusUpdater"), + clock: clock, } } -// ProcessStatusUpdates updates the statuses according to the provided updates. -func (upd *updaterImpl) ProcessStatusUpdates(ctx context.Context, updates []state.StatusUpdate) { - for _, u := range updates { +func (upd *updaterImpl) Update(ctx context.Context, statuses newstate.Statuses) { + // FIXME(pleshakov) Merge the new Conditions in the status with the existing Conditions + // FIXME(pleshakov) Skip the status update (API call) if the status hasn't changed. + + upd.update(ctx, upd.gwNsName, &v1alpha2.Gateway{}, func(object client.Object) { + gw := object.(*v1alpha2.Gateway) + gw.Status = prepareGatewayStatus(statuses.ListenerStatuses, upd.clock) + }) + + for nsname, rs := range statuses.HTTPRouteStatuses { select { case <-ctx.Done(): return default: } - switch s := u.Status.(type) { - case *v1alpha2.HTTPRouteStatus: - upd.logger.Info("Processing a status update for HTTPRoute", - "namespace", u.NamespacedName.Namespace, - "name", u.NamespacedName.Name) - - var hr v1alpha2.HTTPRoute - - upd.update(ctx, u.NamespacedName, &hr, func(object client.Object) { - route := object.(*v1alpha2.HTTPRoute) - // FIXME(pleshakov): merge the conditions in the status with the conditions in the route.Status properly, because - // right now, we are replacing the conditions. - route.Status = *s - }) - default: - panic(fmt.Errorf("unknown status type %T", u.Status)) - } + upd.update(ctx, nsname, &v1alpha2.HTTPRoute{}, func(object client.Object) { + hr := object.(*v1alpha2.HTTPRoute) + hr.Status = prepareHTTPRouteStatus(rs, upd.gwNsName, upd.gatewayCtlrName, upd.clock) + }) } } diff --git a/internal/status/updater_test.go b/internal/status/updater_test.go index c8db418320..217e1ccb94 100644 --- a/internal/status/updater_test.go +++ b/internal/status/updater_test.go @@ -2,6 +2,7 @@ package status_test import ( "context" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -15,13 +16,19 @@ import ( gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/newstate" "github.com/nginxinc/nginx-kubernetes-gateway/internal/status" ) var _ = Describe("Updater", func() { - var updater status.Updater - var client client.Client + var ( + updater status.Updater + client client.Client + fakeClockTime metav1.Time + fakeClock *status.FakeClock + gatewayCtrlName string + gwNsName types.NamespacedName + ) BeforeEach(OncePerOrdered, func() { scheme := runtime.NewScheme() @@ -31,19 +38,30 @@ var _ = Describe("Updater", func() { client = fake.NewClientBuilder(). WithScheme(scheme). Build() - updater = status.NewUpdater(client, zap.New()) + + // Rfc3339Copy() removes the monotonic clock reading + // We need to remove it, because updating the status in the FakeClient and then getting the resource back + // involves encoding and decoding the resource to/from JSON, which removes the monotonic clock reading. + fakeClockTime = metav1.NewTime(time.Now()).Rfc3339Copy() + fakeClock = status.NewFakeClock(fakeClockTime.Time) + + gatewayCtrlName = "test.example.com" + gwNsName = types.NamespacedName{Namespace: "test", Name: "gateway"} + + updater = status.NewUpdater(gatewayCtrlName, gwNsName, client, zap.New(), fakeClock) }) - Describe("Process status update of HTTPRoute", Ordered, func() { - var hr *v1alpha2.HTTPRoute - var testTime metav1.Time + Describe("Process status updates", Ordered, func() { + var ( + hr *v1alpha2.HTTPRoute + gw *v1alpha2.Gateway - BeforeAll(func() { - // Rfc3339Copy() removes the monotonic clock reading - // it is important, because updating the status in the FakeClient and then getting the resource back - // involves encoding and decoding the resource to/from JSON, which removes the monotonic clock reading. - testTime = metav1.Now().Rfc3339Copy() + createStatuses func(bool, bool) newstate.Statuses + createExpectedHR func() *v1alpha2.HTTPRoute + createExpectedGw func(metav1.ConditionStatus, string) *v1alpha2.Gateway + ) + BeforeAll(func() { hr = &v1alpha2.HTTPRoute{ ObjectMeta: metav1.ObjectMeta{ Namespace: "test", @@ -54,77 +72,121 @@ var _ = Describe("Updater", func() { APIVersion: "gateway.networking.k8s.io/v1alpha2", }, } + gw = &v1alpha2.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "gateway", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "Gateway", + APIVersion: "gateway.networking.k8s.io/v1alpha2", + }, + } - Expect(client.Create(context.Background(), hr)).Should(Succeed()) - }) + createStatuses = func(listenerValid, routeAttached bool) newstate.Statuses { + return newstate.Statuses{ + ListenerStatuses: map[string]newstate.ListenerStatus{ + "http": { + Valid: listenerValid, + AttachedRoutes: 1, + }, + }, + HTTPRouteStatuses: map[types.NamespacedName]newstate.HTTPRouteStatus{ + {Namespace: "test", Name: "route1"}: { + ParentStatuses: map[string]newstate.ParentStatus{ + "http": { + Attached: routeAttached, + }, + }, + }, + }, + } + } - It("should process status update", func() { - updates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: &v1alpha2.HTTPRouteStatus{ - RouteStatus: v1alpha2.RouteStatus{ - Parents: []v1alpha2.RouteParentStatus{ + createExpectedHR = func() *v1alpha2.HTTPRoute { + return &v1alpha2.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "route1", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "HTTPRoute", + APIVersion: "gateway.networking.k8s.io/v1alpha2", + }, + Status: gatewayv1alpha2.HTTPRouteStatus{ + RouteStatus: gatewayv1alpha2.RouteStatus{ + Parents: []gatewayv1alpha2.RouteParentStatus{ { - ControllerName: "test", - ParentRef: v1alpha2.ParentRef{ - Name: "fake", + ControllerName: gatewayv1alpha2.GatewayController(gatewayCtrlName), + ParentRef: gatewayv1alpha2.ParentRef{ + Namespace: (*v1alpha2.Namespace)(helpers.GetStringPointer("test")), + Name: "gateway", + SectionName: (*v1alpha2.SectionName)(helpers.GetStringPointer("http")), }, Conditions: []metav1.Condition{ { - Type: string(v1alpha2.ConditionRouteAccepted), + Type: string(gatewayv1alpha2.ConditionRouteAccepted), Status: "True", - ObservedGeneration: hr.Generation, - LastTransitionTime: testTime, - Reason: string(v1alpha2.ConditionRouteAccepted), - Message: "", + ObservedGeneration: 123, + LastTransitionTime: fakeClockTime, + Reason: "Attached", }, }, }, }, }, }, - }, + } } - updater.ProcessStatusUpdates(context.Background(), updates) - }) - - It("should have the updated status in the API server", func() { - expectedHR := &v1alpha2.HTTPRoute{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "route1", - }, - TypeMeta: metav1.TypeMeta{ - Kind: "HTTPRoute", - APIVersion: "gateway.networking.k8s.io/v1alpha2", - }, - Status: gatewayv1alpha2.HTTPRouteStatus{ - RouteStatus: gatewayv1alpha2.RouteStatus{ - Parents: []gatewayv1alpha2.RouteParentStatus{ + createExpectedGw = func(status metav1.ConditionStatus, reason string) *v1alpha2.Gateway { + return &v1alpha2.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "gateway", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "Gateway", + APIVersion: "gateway.networking.k8s.io/v1alpha2", + }, + Status: v1alpha2.GatewayStatus{ + Listeners: []gatewayv1alpha2.ListenerStatus{ { - ControllerName: "test", - ParentRef: gatewayv1alpha2.ParentRef{ - Name: "fake", + Name: "http", + SupportedKinds: []v1alpha2.RouteGroupKind{ + { + Kind: "HTTPRoute", + }, }, + AttachedRoutes: 1, Conditions: []metav1.Condition{ { - Type: string(gatewayv1alpha2.ConditionRouteAccepted), - Status: "True", - ObservedGeneration: 0, - LastTransitionTime: testTime, - Reason: string(gatewayv1alpha2.ConditionRouteAccepted), - Message: "", + Type: string(v1alpha2.ListenerConditionReady), + Status: status, + ObservedGeneration: 123, + LastTransitionTime: fakeClockTime, + Reason: reason, }, }, }, }, }, - }, + } } + }) + It("should create resources in the API server", func() { + Expect(client.Create(context.Background(), hr)).Should(Succeed()) + Expect(client.Create(context.Background(), gw)).Should(Succeed()) + }) + + It("should update statuses", func() { + updater.Update(context.Background(), createStatuses(true, true)) + }) + + It("should have the updated status of HTTPRoute in the API server", func() { latestHR := &v1alpha2.HTTPRoute{} + expectedHR := createExpectedHR() err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "route1"}, latestHR) Expect(err).Should(Not(HaveOccurred())) @@ -133,35 +195,47 @@ var _ = Describe("Updater", func() { Expect(helpers.Diff(expectedHR, latestHR)).To(BeEmpty()) }) - }) - It("should panic for unknown status type", func() { - updates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: "unsupported", - }, - } - - process := func() { - updater.ProcessStatusUpdates(context.Background(), updates) - } - Expect(process).Should(Panic()) - }) + It("should have the updated status of Gateway in the API server", func() { + latestGw := &v1alpha2.Gateway{} + expectedGw := createExpectedGw("True", string(v1alpha2.ListenerReasonReady)) + + err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw) + Expect(err).Should(Not(HaveOccurred())) - It("should not process updates with canceled context", func() { - updates := []state.StatusUpdate{ - { - NamespacedName: types.NamespacedName{Namespace: "test", Name: "route1"}, - Status: "unsupported", - }, - } + expectedGw.ResourceVersion = latestGw.ResourceVersion - ctx, cancel := context.WithCancel(context.Background()) - cancel() + Expect(helpers.Diff(expectedGw, latestGw)).To(BeEmpty()) + }) - // because the ctx is canceled, ProcessStatusUpdates should return immediately without panicking - // because of the unsupported status type - updater.ProcessStatusUpdates(ctx, updates) + It("should update statuses with canceled context - function normally returns", func() { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + updater.Update(ctx, createStatuses(false, false)) + }) + + It("should not have the updated status of HTTPRoute in the API server after updating with canceled context", func() { + latestHR := &v1alpha2.HTTPRoute{} + expectedHR := createExpectedHR() + + err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "route1"}, latestHR) + Expect(err).Should(Not(HaveOccurred())) + + expectedHR.ResourceVersion = latestHR.ResourceVersion + + Expect(helpers.Diff(expectedHR, latestHR)).To(BeEmpty()) + }) + + It("should have the updated status of Gateway in the API server after updating with canceled context", func() { + latestGw := &v1alpha2.Gateway{} + expectedGw := createExpectedGw("False", string(v1alpha2.ListenerReasonInvalid)) + + err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw) + Expect(err).Should(Not(HaveOccurred())) + + expectedGw.ResourceVersion = latestGw.ResourceVersion + + Expect(helpers.Diff(expectedGw, latestGw)).To(BeEmpty()) + }) }) }) From 3ebd7b98fb76ab830f3a87fcf1f9b932b5d00d6a Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 21 Jun 2022 12:57:46 -0700 Subject: [PATCH 2/4] Generate a fake clock --- internal/status/clock.go | 27 ++---- internal/status/clock_test.go | 16 ---- internal/status/gateway.go | 4 +- internal/status/gateway_test.go | 9 +- internal/status/httproute.go | 4 +- internal/status/httproute_test.go | 9 +- internal/status/statusfakes/fake_clock.go | 103 ++++++++++++++++++++++ internal/status/updater.go | 4 +- internal/status/updater_test.go | 5 +- 9 files changed, 128 insertions(+), 53 deletions(-) delete mode 100644 internal/status/clock_test.go create mode 100644 internal/status/statusfakes/fake_clock.go diff --git a/internal/status/clock.go b/internal/status/clock.go index 5e29c81069..a9c90a2299 100644 --- a/internal/status/clock.go +++ b/internal/status/clock.go @@ -1,10 +1,14 @@ package status -import "time" +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Clock // Clock returns the current local time. type Clock interface { - Now() time.Time + Now() metav1.Time } // Real clock returns the current local time. @@ -17,21 +21,6 @@ func NewRealClock() *RealClock { } // Now returns the current local time. -func (c *RealClock) Now() time.Time { - return time.Now() -} - -// FakeClock allows you to control the returned time. -type FakeClock struct { - time time.Time -} - -// NewFakeClock creates a FakeClock. The clock will always return the specified time. -func NewFakeClock(time time.Time) *FakeClock { - return &FakeClock{time: time} -} - -// Now is a fake implementation of Now(). -func (c FakeClock) Now() time.Time { - return c.time +func (c *RealClock) Now() metav1.Time { + return metav1.Now() } diff --git a/internal/status/clock_test.go b/internal/status/clock_test.go deleted file mode 100644 index fb44085128..0000000000 --- a/internal/status/clock_test.go +++ /dev/null @@ -1,16 +0,0 @@ -package status - -import ( - "testing" - "time" -) - -func TestFakeClock(t *testing.T) { - time := time.Now() - clock := NewFakeClock(time) - - result := clock.Now() - if result != time { - t.Errorf("Now() returned %v but expected %v", result, time) - } -} diff --git a/internal/status/gateway.go b/internal/status/gateway.go index d1e97d9667..9869f88ada 100644 --- a/internal/status/gateway.go +++ b/internal/status/gateway.go @@ -13,7 +13,7 @@ import ( // FIXME(pleshakov): Be compliant with in the Gateway API. // Currently, we only support simple valid/invalid status per each listener. // Extend support to cover more cases. -func prepareGatewayStatus(statuses newstate.ListenerStatuses, clock Clock) v1alpha2.GatewayStatus { +func prepareGatewayStatus(statuses newstate.ListenerStatuses, transitionTime metav1.Time) v1alpha2.GatewayStatus { listenerStatuses := make([]v1alpha2.ListenerStatus, 0, len(statuses)) // FIXME(pleshakov) Maintain the order from the Gateway resource @@ -41,7 +41,7 @@ func prepareGatewayStatus(statuses newstate.ListenerStatuses, clock Clock) v1alp Status: metav1.ConditionStatus(status), // FIXME(pleshakov) Set the observed generation to the last processed generation of the Gateway resource. ObservedGeneration: 123, - LastTransitionTime: metav1.NewTime(clock.Now()), + LastTransitionTime: transitionTime, Reason: reason, Message: "", // FIXME(pleshakov) Come up with a good message } diff --git a/internal/status/gateway_test.go b/internal/status/gateway_test.go index ad179793ee..2fa29e7d6e 100644 --- a/internal/status/gateway_test.go +++ b/internal/status/gateway_test.go @@ -23,8 +23,7 @@ func TestPrepareGatewayStatus(t *testing.T) { }, } - expectedTime := time.Now() - clock := NewFakeClock(expectedTime) + transitionTime := metav1.NewTime(time.Now()) expected := v1alpha2.GatewayStatus{ Listeners: []v1alpha2.ListenerStatus{ @@ -41,7 +40,7 @@ func TestPrepareGatewayStatus(t *testing.T) { Type: string(v1alpha2.ListenerConditionReady), Status: "False", ObservedGeneration: 123, - LastTransitionTime: metav1.Time{Time: expectedTime}, + LastTransitionTime: transitionTime, Reason: string(v1alpha2.ListenerReasonInvalid), }, }, @@ -59,7 +58,7 @@ func TestPrepareGatewayStatus(t *testing.T) { Type: string(v1alpha2.ListenerConditionReady), Status: "True", ObservedGeneration: 123, - LastTransitionTime: metav1.Time{Time: expectedTime}, + LastTransitionTime: transitionTime, Reason: string(v1alpha2.ListenerReasonReady), }, }, @@ -67,7 +66,7 @@ func TestPrepareGatewayStatus(t *testing.T) { }, } - result := prepareGatewayStatus(statuses, clock) + result := prepareGatewayStatus(statuses, transitionTime) if diff := cmp.Diff(expected, result); diff != "" { t.Errorf("prepareGatewayStatus() mismatch (-want +got):\n%s", diff) } diff --git a/internal/status/httproute.go b/internal/status/httproute.go index 35a31c0582..4fff72d1d2 100644 --- a/internal/status/httproute.go +++ b/internal/status/httproute.go @@ -18,7 +18,7 @@ func prepareHTTPRouteStatus( status newstate.HTTPRouteStatus, gwNsName types.NamespacedName, gatewayCtlrName string, - clock Clock, + transitionTime metav1.Time, ) v1alpha2.HTTPRouteStatus { parents := make([]v1alpha2.RouteParentStatus, 0, len(status.ParentStatuses)) @@ -56,7 +56,7 @@ func prepareHTTPRouteStatus( Status: metav1.ConditionStatus(status), // FIXME(pleshakov) Set the observed generation to the last processed generation of the HTTPRoute resource. ObservedGeneration: 123, - LastTransitionTime: metav1.NewTime(clock.Now()), + LastTransitionTime: transitionTime, Reason: reason, Message: "", // FIXME(pleshakov): Figure out a good message }, diff --git a/internal/status/httproute_test.go b/internal/status/httproute_test.go index 21cadd08d6..2e33906e51 100644 --- a/internal/status/httproute_test.go +++ b/internal/status/httproute_test.go @@ -28,8 +28,7 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { gwNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} gatewayCtlrName := "test.example.com" - expectedTime := time.Now() - clock := NewFakeClock(expectedTime) + transitionTime := metav1.NewTime(time.Now()) expected := v1alpha2.HTTPRouteStatus{ RouteStatus: v1alpha2.RouteStatus{ @@ -46,7 +45,7 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { Type: string(v1alpha2.ConditionRouteAccepted), Status: "True", ObservedGeneration: 123, - LastTransitionTime: metav1.Time{Time: expectedTime}, + LastTransitionTime: transitionTime, Reason: "Attached", }, }, @@ -63,7 +62,7 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { Type: string(v1alpha2.ConditionRouteAccepted), Status: "False", ObservedGeneration: 123, - LastTransitionTime: metav1.Time{Time: expectedTime}, + LastTransitionTime: transitionTime, Reason: "Not attached", }, }, @@ -72,7 +71,7 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { }, } - result := prepareHTTPRouteStatus(status, gwNsName, gatewayCtlrName, clock) + result := prepareHTTPRouteStatus(status, gwNsName, gatewayCtlrName, transitionTime) if diff := cmp.Diff(expected, result); diff != "" { t.Errorf("prepareHTTPRouteStatus() mismatch (-want +got):\n%s", diff) } diff --git a/internal/status/statusfakes/fake_clock.go b/internal/status/statusfakes/fake_clock.go new file mode 100644 index 0000000000..f02f382c08 --- /dev/null +++ b/internal/status/statusfakes/fake_clock.go @@ -0,0 +1,103 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package statusfakes + +import ( + "sync" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/status" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +type FakeClock struct { + NowStub func() v1.Time + nowMutex sync.RWMutex + nowArgsForCall []struct { + } + nowReturns struct { + result1 v1.Time + } + nowReturnsOnCall map[int]struct { + result1 v1.Time + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeClock) Now() v1.Time { + fake.nowMutex.Lock() + ret, specificReturn := fake.nowReturnsOnCall[len(fake.nowArgsForCall)] + fake.nowArgsForCall = append(fake.nowArgsForCall, struct { + }{}) + stub := fake.NowStub + fakeReturns := fake.nowReturns + fake.recordInvocation("Now", []interface{}{}) + fake.nowMutex.Unlock() + if stub != nil { + return stub() + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeClock) NowCallCount() int { + fake.nowMutex.RLock() + defer fake.nowMutex.RUnlock() + return len(fake.nowArgsForCall) +} + +func (fake *FakeClock) NowCalls(stub func() v1.Time) { + fake.nowMutex.Lock() + defer fake.nowMutex.Unlock() + fake.NowStub = stub +} + +func (fake *FakeClock) NowReturns(result1 v1.Time) { + fake.nowMutex.Lock() + defer fake.nowMutex.Unlock() + fake.NowStub = nil + fake.nowReturns = struct { + result1 v1.Time + }{result1} +} + +func (fake *FakeClock) NowReturnsOnCall(i int, result1 v1.Time) { + fake.nowMutex.Lock() + defer fake.nowMutex.Unlock() + fake.NowStub = nil + if fake.nowReturnsOnCall == nil { + fake.nowReturnsOnCall = make(map[int]struct { + result1 v1.Time + }) + } + fake.nowReturnsOnCall[i] = struct { + result1 v1.Time + }{result1} +} + +func (fake *FakeClock) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.nowMutex.RLock() + defer fake.nowMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeClock) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ status.Clock = new(FakeClock) diff --git a/internal/status/updater.go b/internal/status/updater.go index 6246f22e02..9b4471f2ba 100644 --- a/internal/status/updater.go +++ b/internal/status/updater.go @@ -88,7 +88,7 @@ func (upd *updaterImpl) Update(ctx context.Context, statuses newstate.Statuses) upd.update(ctx, upd.gwNsName, &v1alpha2.Gateway{}, func(object client.Object) { gw := object.(*v1alpha2.Gateway) - gw.Status = prepareGatewayStatus(statuses.ListenerStatuses, upd.clock) + gw.Status = prepareGatewayStatus(statuses.ListenerStatuses, upd.clock.Now()) }) for nsname, rs := range statuses.HTTPRouteStatuses { @@ -100,7 +100,7 @@ func (upd *updaterImpl) Update(ctx context.Context, statuses newstate.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) + hr.Status = prepareHTTPRouteStatus(rs, upd.gwNsName, upd.gatewayCtlrName, upd.clock.Now()) }) } } diff --git a/internal/status/updater_test.go b/internal/status/updater_test.go index 217e1ccb94..7982e74258 100644 --- a/internal/status/updater_test.go +++ b/internal/status/updater_test.go @@ -18,6 +18,7 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" "github.com/nginxinc/nginx-kubernetes-gateway/internal/newstate" "github.com/nginxinc/nginx-kubernetes-gateway/internal/status" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/status/statusfakes" ) var _ = Describe("Updater", func() { @@ -25,7 +26,6 @@ var _ = Describe("Updater", func() { updater status.Updater client client.Client fakeClockTime metav1.Time - fakeClock *status.FakeClock gatewayCtrlName string gwNsName types.NamespacedName ) @@ -43,7 +43,8 @@ var _ = Describe("Updater", func() { // We need to remove it, because updating the status in the FakeClient and then getting the resource back // involves encoding and decoding the resource to/from JSON, which removes the monotonic clock reading. fakeClockTime = metav1.NewTime(time.Now()).Rfc3339Copy() - fakeClock = status.NewFakeClock(fakeClockTime.Time) + fakeClock := &statusfakes.FakeClock{} + fakeClock.NowReturns(fakeClockTime) gatewayCtrlName = "test.example.com" gwNsName = types.NamespacedName{Namespace: "test", Name: "gateway"} From 9de3166ab8689124f9d69ff477a3524d34a484b6 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 21 Jun 2022 14:38:01 -0700 Subject: [PATCH 3/4] Use constants --- internal/status/gateway.go | 15 +++++++++------ internal/status/gateway_test.go | 4 ++-- internal/status/httproute.go | 16 ++++++++++------ internal/status/httproute_test.go | 6 +++--- internal/status/updater_test.go | 8 ++++---- 5 files changed, 28 insertions(+), 21 deletions(-) diff --git a/internal/status/gateway.go b/internal/status/gateway.go index 9869f88ada..df1dd25022 100644 --- a/internal/status/gateway.go +++ b/internal/status/gateway.go @@ -26,14 +26,17 @@ func prepareGatewayStatus(statuses newstate.ListenerStatuses, transitionTime met for _, name := range names { s := statuses[name] - var status, reason string + var ( + status metav1.ConditionStatus + reason v1alpha2.ListenerConditionReason + ) if s.Valid { - status = "True" - reason = string(v1alpha2.ListenerReasonReady) + status = metav1.ConditionTrue + reason = v1alpha2.ListenerReasonReady } else { - status = "False" - reason = string(v1alpha2.ListenerReasonInvalid) + status = metav1.ConditionFalse + reason = v1alpha2.ListenerReasonInvalid } cond := metav1.Condition{ @@ -42,7 +45,7 @@ func prepareGatewayStatus(statuses newstate.ListenerStatuses, transitionTime met // FIXME(pleshakov) Set the observed generation to the last processed generation of the Gateway resource. ObservedGeneration: 123, LastTransitionTime: transitionTime, - Reason: reason, + Reason: string(reason), Message: "", // FIXME(pleshakov) Come up with a good message } diff --git a/internal/status/gateway_test.go b/internal/status/gateway_test.go index 2fa29e7d6e..2dda340cf8 100644 --- a/internal/status/gateway_test.go +++ b/internal/status/gateway_test.go @@ -38,7 +38,7 @@ func TestPrepareGatewayStatus(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(v1alpha2.ListenerConditionReady), - Status: "False", + Status: metav1.ConditionFalse, ObservedGeneration: 123, LastTransitionTime: transitionTime, Reason: string(v1alpha2.ListenerReasonInvalid), @@ -56,7 +56,7 @@ func TestPrepareGatewayStatus(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(v1alpha2.ListenerConditionReady), - Status: "True", + Status: metav1.ConditionTrue, ObservedGeneration: 123, LastTransitionTime: transitionTime, Reason: string(v1alpha2.ListenerReasonReady), diff --git a/internal/status/httproute.go b/internal/status/httproute.go index 4fff72d1d2..50629b693c 100644 --- a/internal/status/httproute.go +++ b/internal/status/httproute.go @@ -32,13 +32,17 @@ func prepareHTTPRouteStatus( for _, name := range names { ps := status.ParentStatuses[name] - var status, reason string + var ( + status metav1.ConditionStatus + reason string // FIXME(pleshakov) use RouteConditionReason once we upgrade to v1beta1 + ) + if ps.Attached { - status = "True" - reason = "Attached" + status = metav1.ConditionTrue + reason = "Accepted" // FIXME(pleshakov): use RouteReasonAccepted once we upgrade to v1beta1 } else { - status = "False" - reason = "Not attached" + status = metav1.ConditionFalse + reason = "Not attached" // FIXME(pleshakov): use a more specific message from the defined constants (available in v1beta1) } sectionName := name @@ -53,7 +57,7 @@ func prepareHTTPRouteStatus( Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), - Status: metav1.ConditionStatus(status), + Status: status, // FIXME(pleshakov) Set the observed generation to the last processed generation of the HTTPRoute resource. ObservedGeneration: 123, LastTransitionTime: transitionTime, diff --git a/internal/status/httproute_test.go b/internal/status/httproute_test.go index 2e33906e51..5564e8616b 100644 --- a/internal/status/httproute_test.go +++ b/internal/status/httproute_test.go @@ -43,10 +43,10 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), - Status: "True", + Status: metav1.ConditionTrue, ObservedGeneration: 123, LastTransitionTime: transitionTime, - Reason: "Attached", + Reason: "Accepted", }, }, }, @@ -60,7 +60,7 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(v1alpha2.ConditionRouteAccepted), - Status: "False", + Status: metav1.ConditionFalse, ObservedGeneration: 123, LastTransitionTime: transitionTime, Reason: "Not attached", diff --git a/internal/status/updater_test.go b/internal/status/updater_test.go index 7982e74258..ddff0277ed 100644 --- a/internal/status/updater_test.go +++ b/internal/status/updater_test.go @@ -127,10 +127,10 @@ var _ = Describe("Updater", func() { Conditions: []metav1.Condition{ { Type: string(gatewayv1alpha2.ConditionRouteAccepted), - Status: "True", + Status: metav1.ConditionTrue, ObservedGeneration: 123, LastTransitionTime: fakeClockTime, - Reason: "Attached", + Reason: "Accepted", }, }, }, @@ -199,7 +199,7 @@ var _ = Describe("Updater", func() { It("should have the updated status of Gateway in the API server", func() { latestGw := &v1alpha2.Gateway{} - expectedGw := createExpectedGw("True", string(v1alpha2.ListenerReasonReady)) + expectedGw := createExpectedGw(metav1.ConditionTrue, string(v1alpha2.ListenerReasonReady)) err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw) Expect(err).Should(Not(HaveOccurred())) @@ -229,7 +229,7 @@ var _ = Describe("Updater", func() { It("should have the updated status of Gateway in the API server after updating with canceled context", func() { latestGw := &v1alpha2.Gateway{} - expectedGw := createExpectedGw("False", string(v1alpha2.ListenerReasonInvalid)) + expectedGw := createExpectedGw(metav1.ConditionFalse, string(v1alpha2.ListenerReasonInvalid)) err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw) Expect(err).Should(Not(HaveOccurred())) From 210472cf5e0e10a77ac21f1cc302642f0f58082e Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 21 Jun 2022 14:44:03 -0700 Subject: [PATCH 4/4] Fix linting --- internal/status/gateway.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/status/gateway.go b/internal/status/gateway.go index df1dd25022..3c33ce23f6 100644 --- a/internal/status/gateway.go +++ b/internal/status/gateway.go @@ -41,7 +41,7 @@ func prepareGatewayStatus(statuses newstate.ListenerStatuses, transitionTime met cond := metav1.Condition{ Type: string(v1alpha2.ListenerConditionReady), - Status: metav1.ConditionStatus(status), + Status: status, // FIXME(pleshakov) Set the observed generation to the last processed generation of the Gateway resource. ObservedGeneration: 123, LastTransitionTime: transitionTime,