From 4c82cc02aa50f7bd2da24e02399c610c011c421f Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Mon, 7 Nov 2022 17:34:21 -0500 Subject: [PATCH 1/2] Support NoMatchingListenerHostname reason in HTTPRoute status If an HTTPRoute references a listener with the hostname that does not match any hostnames of the HTTPRoute, NGINX Kubernetes Gateway will set the condition "Accepted" with the status "False" and the reason "NoMatchingListenerHostname" in the status of the HTTPRoute. The commit introduces a new type RouteCondition. It is used to capture status-related conditions when building the graph. It will be used to capture other conditions too, including when building configuration. Note: the implementation required implementing a few additional conditions. In one case, to wait until the corresponding reasons (v1beta1.RouteConditionReason) are added to the Gateway API and in the other case, to not increase the scope, those conditions are left to be implemented (FIXMEs). --- docs/gateway-api-compatibility.md | 4 +- internal/state/change_processor_test.go | 128 +++++++++++++++----- internal/state/conditions/httproute.go | 72 +++++++++++ internal/state/conditions/httproute_test.go | 53 ++++++++ internal/state/configuration_test.go | 5 +- internal/state/graph.go | 18 ++- internal/state/graph_test.go | 25 ++-- internal/state/statuses.go | 33 ++++- internal/state/statuses_test.go | 71 +++++++++-- internal/status/httproute.go | 46 +++---- internal/status/httproute_test.go | 76 ++++++++++-- internal/status/updater_test.go | 11 +- 12 files changed, 435 insertions(+), 107 deletions(-) create mode 100644 internal/state/conditions/httproute.go create mode 100644 internal/state/conditions/httproute_test.go diff --git a/docs/gateway-api-compatibility.md b/docs/gateway-api-compatibility.md index fdae4a808..716f09615 100644 --- a/docs/gateway-api-compatibility.md +++ b/docs/gateway-api-compatibility.md @@ -96,7 +96,9 @@ Fields: * `parents` * `parentRef` - supported. * `controllerName` - supported. - * `conditions` - partially supported. + * `conditions` - partially supported. Supported (Condition/Status/Reason): + * `Accepted/True/Accepted` + * `Accepted/False/NoMatchingListenerHostname` ### TLSRoute diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go index 5aaffbbb5..e0c4d8464 100644 --- a/internal/state/change_processor_test.go +++ b/internal/state/change_processor_test.go @@ -2,6 +2,7 @@ package state_test import ( "context" + "sort" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -17,6 +18,7 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" "github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/index" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/relationship" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/relationship/relationshipfakes" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/statefakes" @@ -228,6 +230,24 @@ var _ = Describe("ChangeProcessor", func() { gw2 = createGatewayWithTLSListener("gateway-2") }) + + assertStatuses := func(expected, result state.Statuses) { + sortConditions := func(statuses state.HTTPRouteStatuses) { + for _, status := range statuses { + for _, ps := range status.ParentStatuses { + sort.Slice(ps.Conditions, func(i, j int) bool { + return ps.Conditions[i].Type < ps.Conditions[j].Type + }) + } + } + } + + sortConditions(expected.HTTPRouteStatuses) + sortConditions(result.HTTPRouteStatuses) + + ExpectWithOffset(1, helpers.Diff(expected, result)).To(BeEmpty()) + } + When("no upsert has occurred", func() { It("returns empty configuration and statuses", func() { changed, conf, statuses := processor.Process(context.TODO()) @@ -279,8 +299,18 @@ var _ = Describe("ChangeProcessor", func() { {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1.Generation, ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": {Attached: false}, - "listener-443-1": {Attached: false}, + "listener-80-1": { + Conditions: append( + conditions.NewDefaultRouteConditions(), + conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist"), + ), + }, + "listener-443-1": { + Conditions: append( + conditions.NewDefaultRouteConditions(), + conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist"), + ), + }, }, }, }, @@ -289,7 +319,7 @@ var _ = Describe("ChangeProcessor", func() { changed, conf, statuses := processor.Process(context.TODO()) Expect(changed).To(BeTrue()) Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) }) }) }) @@ -367,8 +397,12 @@ var _ = Describe("ChangeProcessor", func() { {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1.Generation, ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": {Attached: true}, - "listener-443-1": {Attached: true}, + "listener-80-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, + "listener-443-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, }, }, }, @@ -377,7 +411,7 @@ var _ = Describe("ChangeProcessor", func() { changed, conf, statuses := processor.Process(context.TODO()) Expect(changed).To(BeTrue()) Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) }) }) When("the first HTTPRoute without a generation changed is processed", func() { @@ -465,8 +499,12 @@ var _ = Describe("ChangeProcessor", func() { {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1Updated.Generation, ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": {Attached: true}, - "listener-443-1": {Attached: true}, + "listener-80-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, + "listener-443-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, }, }, }, @@ -475,7 +513,7 @@ var _ = Describe("ChangeProcessor", func() { changed, conf, statuses := processor.Process(context.TODO()) Expect(changed).To(BeTrue()) Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) }, ) }) @@ -564,8 +602,12 @@ var _ = Describe("ChangeProcessor", func() { {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1Updated.Generation, ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": {Attached: true}, - "listener-443-1": {Attached: true}, + "listener-80-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, + "listener-443-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, }, }, }, @@ -574,7 +616,7 @@ var _ = Describe("ChangeProcessor", func() { changed, conf, statuses := processor.Process(context.TODO()) Expect(changed).To(BeTrue()) Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) }) }) When("the GatewayClass update without generation change is processed", func() { @@ -662,8 +704,12 @@ var _ = Describe("ChangeProcessor", func() { {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1Updated.Generation, ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": {Attached: true}, - "listener-443-1": {Attached: true}, + "listener-80-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, + "listener-443-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, }, }, }, @@ -672,7 +718,7 @@ var _ = Describe("ChangeProcessor", func() { changed, conf, statuses := processor.Process(context.TODO()) Expect(changed).To(BeTrue()) Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) }) }) When("no changes are captured", func() { @@ -763,8 +809,12 @@ var _ = Describe("ChangeProcessor", func() { {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1Updated.Generation, ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": {Attached: true}, - "listener-443-1": {Attached: true}, + "listener-80-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, + "listener-443-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, }, }, }, @@ -773,7 +823,7 @@ var _ = Describe("ChangeProcessor", func() { changed, conf, statuses := processor.Process(context.TODO()) Expect(changed).To(BeTrue()) Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) }) }) When("the second HTTPRoute is upserted", func() { @@ -853,15 +903,29 @@ var _ = Describe("ChangeProcessor", func() { {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1Updated.Generation, ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": {Attached: true}, - "listener-443-1": {Attached: true}, + "listener-80-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, + "listener-443-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, }, }, {Namespace: "test", Name: "hr-2"}: { ObservedGeneration: hr2.Generation, ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": {Attached: false}, - "listener-443-1": {Attached: false}, + "listener-80-1": { + Conditions: append( + conditions.NewDefaultRouteConditions(), + conditions.NewRouteTODO("Gateway is ignored"), + ), + }, + "listener-443-1": { + Conditions: append( + conditions.NewDefaultRouteConditions(), + conditions.NewRouteTODO("Gateway is ignored"), + ), + }, }, }, }, @@ -870,7 +934,7 @@ var _ = Describe("ChangeProcessor", func() { changed, conf, statuses := processor.Process(context.TODO()) Expect(changed).To(BeTrue()) Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) }) }) When("the first Gateway is deleted", func() { @@ -949,8 +1013,12 @@ var _ = Describe("ChangeProcessor", func() { {Namespace: "test", Name: "hr-2"}: { ObservedGeneration: hr2.Generation, ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": {Attached: true}, - "listener-443-1": {Attached: true}, + "listener-80-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, + "listener-443-1": { + Conditions: conditions.NewDefaultRouteConditions(), + }, }, }, }, @@ -959,7 +1027,7 @@ var _ = Describe("ChangeProcessor", func() { changed, conf, statuses := processor.Process(context.TODO()) Expect(changed).To(BeTrue()) Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) }) }) When("the second HTTPRoute is deleted", func() { @@ -1003,7 +1071,7 @@ var _ = Describe("ChangeProcessor", func() { changed, conf, statuses := processor.Process(context.TODO()) Expect(changed).To(BeTrue()) Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) }) }) When("the GatewayClass is deleted", func() { @@ -1035,7 +1103,7 @@ var _ = Describe("ChangeProcessor", func() { changed, conf, statuses := processor.Process(context.TODO()) Expect(changed).To(BeTrue()) Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) }) }) When("the second Gateway is deleted", func() { @@ -1054,7 +1122,7 @@ var _ = Describe("ChangeProcessor", func() { changed, conf, statuses := processor.Process(context.TODO()) Expect(changed).To(BeTrue()) Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) }) }) When("the first HTTPRoute is deleted", func() { @@ -1073,7 +1141,7 @@ var _ = Describe("ChangeProcessor", func() { changed, conf, statuses := processor.Process(context.TODO()) Expect(changed).To(BeTrue()) Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) - Expect(helpers.Diff(expectedStatuses, statuses)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) }) }) }) diff --git a/internal/state/conditions/httproute.go b/internal/state/conditions/httproute.go new file mode 100644 index 000000000..d7c7464bd --- /dev/null +++ b/internal/state/conditions/httproute.go @@ -0,0 +1,72 @@ +package conditions + +import ( + "fmt" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/gateway-api/apis/v1beta1" +) + +// RouteCondition defines a condition to be reported in the status of an HTTPRoute. +type RouteCondition struct { + Type v1beta1.RouteConditionType + Status metav1.ConditionStatus + Reason v1beta1.RouteConditionReason + Message string +} + +// DeduplicateRouteConditions removes duplicate conditions based on the condition type. +// The last condition wins. +func DeduplicateRouteConditions(conds []RouteCondition) []RouteCondition { + uniqueConds := make(map[v1beta1.RouteConditionType]RouteCondition) + + for _, cond := range conds { + uniqueConds[cond.Type] = cond + } + + result := make([]RouteCondition, 0, len(uniqueConds)) + + for _, cond := range uniqueConds { + result = append(result, cond) + } + + return result +} + +// NewDefaultRouteConditions returns the default conditions that must be present in the status of an HTTPRoute. +func NewDefaultRouteConditions() []RouteCondition { + return []RouteCondition{ + NewRouteAccepted(), + } +} + +// NewRouteNoMatchingListenerHostname returns a RouteCondition that indicates that the hostname of the listener +// does not match the hostnames of the HTTPRoute. +func NewRouteNoMatchingListenerHostname() RouteCondition { + return RouteCondition{ + Type: v1beta1.RouteConditionAccepted, + Status: metav1.ConditionFalse, + Reason: v1beta1.RouteReasonNoMatchingListenerHostname, + Message: "Listener hostname does not match the HTTPRoute hostnames", + } +} + +// NewRouteAccepted returns a RouteCondition that indicates that the HTTPRoute is accepted. +func NewRouteAccepted() RouteCondition { + return RouteCondition{ + Type: v1beta1.RouteConditionAccepted, + Status: metav1.ConditionTrue, + Reason: v1beta1.RouteReasonAccepted, + Message: "The route is accepted", + } +} + +// NewRouteTODO returns a RouteCondition that can be used as a placeholder for a condition that is not yet implemented. +func NewRouteTODO(msg string) RouteCondition { + return RouteCondition{ + Type: "TODO", + Status: metav1.ConditionTrue, + Reason: "TODO", + Message: fmt.Sprintf("The condition for this has not been implemented yet: %s", msg), + } +} diff --git a/internal/state/conditions/httproute_test.go b/internal/state/conditions/httproute_test.go new file mode 100644 index 000000000..d514ae7c0 --- /dev/null +++ b/internal/state/conditions/httproute_test.go @@ -0,0 +1,53 @@ +package conditions + +import ( + "testing" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestDeduplicateDeduplicateRouteConditions(t *testing.T) { + g := NewGomegaWithT(t) + + conds := []RouteCondition{ + { + Type: "Type1", + Status: metav1.ConditionTrue, + }, + { + Type: "Type1", + Status: metav1.ConditionFalse, + }, + { + Type: "Type2", + Status: metav1.ConditionFalse, + }, + { + Type: "Type2", + Status: metav1.ConditionTrue, + }, + { + Type: "Type3", + Status: metav1.ConditionTrue, + }, + } + + expected := []RouteCondition{ + { + Type: "Type1", + Status: metav1.ConditionFalse, + }, + { + Type: "Type2", + Status: metav1.ConditionTrue, + }, + { + Type: "Type3", + Status: metav1.ConditionTrue, + }, + } + + result := DeduplicateRouteConditions(conds) + g.Expect(result).Should(ConsistOf(expected)) +} diff --git a/internal/state/configuration_test.go b/internal/state/configuration_test.go index b2577fc2a..c5bbc8422 100644 --- a/internal/state/configuration_test.go +++ b/internal/state/configuration_test.go @@ -14,6 +14,7 @@ import ( "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/resolver" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/resolver/resolverfakes" ) @@ -100,7 +101,7 @@ func TestBuildConfiguration(t *testing.T) { createInternalRoute := func(source *v1beta1.HTTPRoute, validSectionName string, groups ...BackendGroup) *route { r := &route{ Source: source, - InvalidSectionNameRefs: make(map[string]struct{}), + InvalidSectionNameRefs: make(map[string]conditions.RouteCondition), ValidSectionNameRefs: map[string]struct{}{validSectionName: {}}, BackendGroups: groups, } @@ -179,7 +180,7 @@ func TestBuildConfiguration(t *testing.T) { routeHR5 := &route{ Source: hr5, - InvalidSectionNameRefs: make(map[string]struct{}), + InvalidSectionNameRefs: make(map[string]conditions.RouteCondition), ValidSectionNameRefs: map[string]struct{}{"listener-80-1": {}}, BackendGroups: []BackendGroup{hr5BackendGroup}, } diff --git a/internal/state/graph.go b/internal/state/graph.go index a507208c9..6350ece6b 100644 --- a/internal/state/graph.go +++ b/internal/state/graph.go @@ -7,6 +7,8 @@ import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/gateway-api/apis/v1beta1" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" ) // gateway represents the winning Gateway resource. @@ -28,8 +30,9 @@ type route struct { // ValidSectionNameRefs includes the sectionNames from the parentRefs of the HTTPRoute that are valid -- i.e. // the Gateway resource has a corresponding valid listener. ValidSectionNameRefs map[string]struct{} - // ValidSectionNameRefs includes the sectionNames from the parentRefs of the HTTPRoute that are invalid. - InvalidSectionNameRefs map[string]struct{} + // InvalidSectionNameRefs includes the sectionNames from the parentRefs of the HTTPRoute that are invalid. + // The RouteCondition describes why the sectionName is invalid. + InvalidSectionNameRefs map[string]conditions.RouteCondition // BackendGroups includes the backend groups of the HTTPRoute. // There's one BackendGroup per rule in the HTTPRoute. // The BackendGroups are stored in order of the rules. @@ -189,7 +192,7 @@ func bindHTTPRouteToListeners( r = &route{ Source: ghr, ValidSectionNameRefs: make(map[string]struct{}), - InvalidSectionNameRefs: make(map[string]struct{}), + InvalidSectionNameRefs: make(map[string]conditions.RouteCondition), } // FIXME (pleshakov) Handle the case when parent refs are duplicated @@ -233,7 +236,9 @@ func bindHTTPRouteToListeners( l, exists := listeners[name] if !exists { - r.InvalidSectionNameRefs[name] = struct{}{} + // FIXME(pleshakov): Add a proper condition once it is available in the Gateway API. + // See also https://github.com/kubernetes-sigs/gateway-api/discussions/1445 + r.InvalidSectionNameRefs[name] = conditions.NewRouteTODO("listener is not found") continue } @@ -246,7 +251,7 @@ func bindHTTPRouteToListeners( r.ValidSectionNameRefs[name] = struct{}{} l.Routes[client.ObjectKeyFromObject(ghr)] = r } else { - r.InvalidSectionNameRefs[name] = struct{}{} + r.InvalidSectionNameRefs[name] = conditions.NewRouteNoMatchingListenerHostname() } continue @@ -257,7 +262,8 @@ func bindHTTPRouteToListeners( key := types.NamespacedName{Namespace: ns, Name: string(p.Name)} if _, exist := ignoredGws[key]; exist { - r.InvalidSectionNameRefs[name] = struct{}{} + // FIXME(pleshakov): Add a proper condition. + r.InvalidSectionNameRefs[name] = conditions.NewRouteTODO("Gateway is ignored") processed = true continue diff --git a/internal/state/graph_test.go b/internal/state/graph_test.go index d85bfde75..16bfa5d94 100644 --- a/internal/state/graph_test.go +++ b/internal/state/graph_test.go @@ -11,6 +11,7 @@ import ( "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" ) var testSecret = &v1.Secret{ @@ -230,7 +231,7 @@ func TestBuildGraph(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-80-1": {}, }, - InvalidSectionNameRefs: map[string]struct{}{}, + InvalidSectionNameRefs: map[string]conditions.RouteCondition{}, BackendGroups: []BackendGroup{hr1Group}, } @@ -239,7 +240,7 @@ func TestBuildGraph(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-443-1": {}, }, - InvalidSectionNameRefs: map[string]struct{}{}, + InvalidSectionNameRefs: map[string]conditions.RouteCondition{}, BackendGroups: []BackendGroup{hr3Group}, } @@ -878,8 +879,8 @@ func TestBindRouteToListeners(t *testing.T) { expectedRoute: &route{ Source: hrNonExistingSectionName, ValidSectionNameRefs: map[string]struct{}{}, - InvalidSectionNameRefs: map[string]struct{}{ - "listener-80-2": {}, + InvalidSectionNameRefs: map[string]conditions.RouteCondition{ + "listener-80-2": conditions.NewRouteTODO("listener is not found"), }, }, expectedListeners: map[string]*listener{ @@ -914,7 +915,7 @@ func TestBindRouteToListeners(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-80-1": {}, }, - InvalidSectionNameRefs: map[string]struct{}{}, + InvalidSectionNameRefs: map[string]conditions.RouteCondition{}, }, expectedListeners: map[string]*listener{ "listener-80-1": createModifiedListener(func(l *listener) { @@ -924,7 +925,7 @@ func TestBindRouteToListeners(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-80-1": {}, }, - InvalidSectionNameRefs: map[string]struct{}{}, + InvalidSectionNameRefs: map[string]conditions.RouteCondition{}, }, } l.AcceptedHostnames = map[string]struct{}{ @@ -947,7 +948,7 @@ func TestBindRouteToListeners(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-80-1": {}, }, - InvalidSectionNameRefs: map[string]struct{}{}, + InvalidSectionNameRefs: map[string]conditions.RouteCondition{}, }, expectedListeners: map[string]*listener{ "listener-80-1": createModifiedListener(func(l *listener) { @@ -957,7 +958,7 @@ func TestBindRouteToListeners(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-80-1": {}, }, - InvalidSectionNameRefs: map[string]struct{}{}, + InvalidSectionNameRefs: map[string]conditions.RouteCondition{}, }, } l.AcceptedHostnames = map[string]struct{}{ @@ -978,8 +979,8 @@ func TestBindRouteToListeners(t *testing.T) { expectedRoute: &route{ Source: hrBar, ValidSectionNameRefs: map[string]struct{}{}, - InvalidSectionNameRefs: map[string]struct{}{ - "listener-80-1": {}, + InvalidSectionNameRefs: map[string]conditions.RouteCondition{ + "listener-80-1": conditions.NewRouteNoMatchingListenerHostname(), }, }, expectedListeners: map[string]*listener{ @@ -1000,8 +1001,8 @@ func TestBindRouteToListeners(t *testing.T) { expectedRoute: &route{ Source: hrIgnoredGateway, ValidSectionNameRefs: map[string]struct{}{}, - InvalidSectionNameRefs: map[string]struct{}{ - "listener-80-1": {}, + InvalidSectionNameRefs: map[string]conditions.RouteCondition{ + "listener-80-1": conditions.NewRouteTODO("Gateway is ignored"), }, }, expectedListeners: map[string]*listener{ diff --git a/internal/state/statuses.go b/internal/state/statuses.go index ee99de00c..889874d66 100644 --- a/internal/state/statuses.go +++ b/internal/state/statuses.go @@ -3,6 +3,8 @@ package state import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" ) // ListenerStatuses holds the statuses of listeners where the key is the name of a listener in the Gateway resource. @@ -54,8 +56,8 @@ type HTTPRouteStatus struct { // ParentStatus holds status-related information related to how the HTTPRoute binds to a specific parentRef. type ParentStatus struct { - // Attached is true if the route attaches to the parent (listener). - Attached bool + // Conditions is the list of conditions that are relevant to the parentRef. + Conditions []conditions.RouteCondition } // GatewayClassStatus holds status-related infortmation about the GatewayClass resource. @@ -110,12 +112,20 @@ func buildStatuses(graph *graph) Statuses { for ref := range r.ValidSectionNameRefs { parentStatuses[ref] = ParentStatus{ - Attached: gcValidAndExist, // Attached only when GatewayClass is valid and exists + Conditions: conditions.DeduplicateRouteConditions( + buildBaseRouteConditions(gcValidAndExist), + ), } } - for ref := range r.InvalidSectionNameRefs { + for ref, cond := range r.InvalidSectionNameRefs { + baseConds := buildBaseRouteConditions(gcValidAndExist) + + conds := make([]conditions.RouteCondition, 0, len(baseConds)+1) + conds = append(conds, baseConds...) + conds = append(conds, cond) + parentStatuses[ref] = ParentStatus{ - Attached: false, + Conditions: conditions.DeduplicateRouteConditions(conds), } } @@ -127,3 +137,16 @@ func buildStatuses(graph *graph) Statuses { return statuses } + +func buildBaseRouteConditions(gcValidAndExist bool) []conditions.RouteCondition { + conds := conditions.NewDefaultRouteConditions() + + // FIXME(pleshakov): Figure out appropriate conditions for the cases when: + // (1) GatewayClass is invalid. + // (2) GatewayClass does not exist. + if !gcValidAndExist { + conds = append(conds, conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist")) + } + + return conds +} diff --git a/internal/state/statuses_test.go b/internal/state/statuses_test.go index 78171611f..6c20e5884 100644 --- a/internal/state/statuses_test.go +++ b/internal/state/statuses_test.go @@ -1,15 +1,23 @@ package state import ( + "sort" "testing" "github.com/google/go-cmp/cmp" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/gateway-api/apis/v1beta1" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" ) func TestBuildStatuses(t *testing.T) { + invalidCondition := conditions.RouteCondition{ + Type: "Test", + Status: metav1.ConditionTrue, + } + listeners := map[string]*listener{ "listener-80-1": { Valid: true, @@ -29,8 +37,8 @@ func TestBuildStatuses(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-80-1": {}, }, - InvalidSectionNameRefs: map[string]struct{}{ - "listener-80-2": {}, + InvalidSectionNameRefs: map[string]conditions.RouteCondition{ + "listener-80-2": invalidCondition, }, }, } @@ -42,9 +50,9 @@ func TestBuildStatuses(t *testing.T) { Generation: 4, }, }, - InvalidSectionNameRefs: map[string]struct{}{ - "listener-80-2": {}, - "listener-80-1": {}, + InvalidSectionNameRefs: map[string]conditions.RouteCondition{ + "listener-80-2": invalidCondition, + "listener-80-1": invalidCondition, }, }, } @@ -108,10 +116,13 @@ func TestBuildStatuses(t *testing.T) { ObservedGeneration: 3, ParentStatuses: map[string]ParentStatus{ "listener-80-1": { - Attached: true, + Conditions: conditions.NewDefaultRouteConditions(), }, "listener-80-2": { - Attached: false, + Conditions: append( + conditions.NewDefaultRouteConditions(), + invalidCondition, + ), }, }, }, @@ -150,10 +161,17 @@ func TestBuildStatuses(t *testing.T) { ObservedGeneration: 3, ParentStatuses: map[string]ParentStatus{ "listener-80-1": { - Attached: false, + Conditions: append( + conditions.NewDefaultRouteConditions(), + conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist"), + ), }, "listener-80-2": { - Attached: false, + Conditions: append( + conditions.NewDefaultRouteConditions(), + conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist"), + invalidCondition, + ), }, }, }, @@ -202,10 +220,17 @@ func TestBuildStatuses(t *testing.T) { ObservedGeneration: 3, ParentStatuses: map[string]ParentStatus{ "listener-80-1": { - Attached: false, + Conditions: append( + conditions.NewDefaultRouteConditions(), + conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist"), + ), }, "listener-80-2": { - Attached: false, + Conditions: append( + conditions.NewDefaultRouteConditions(), + conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist"), + invalidCondition, + ), }, }, }, @@ -237,10 +262,16 @@ func TestBuildStatuses(t *testing.T) { ObservedGeneration: 4, ParentStatuses: map[string]ParentStatus{ "listener-80-1": { - Attached: false, + Conditions: append( + conditions.NewDefaultRouteConditions(), + invalidCondition, + ), }, "listener-80-2": { - Attached: false, + Conditions: append( + conditions.NewDefaultRouteConditions(), + invalidCondition, + ), }, }, }, @@ -250,8 +281,22 @@ func TestBuildStatuses(t *testing.T) { }, } + sortConditions := func(statuses Statuses) { + for _, rs := range statuses.HTTPRouteStatuses { + for _, ps := range rs.ParentStatuses { + sort.Slice(ps.Conditions, func(i, j int) bool { + return ps.Conditions[i].Type < ps.Conditions[j].Type + }) + } + } + } + for _, test := range tests { result := buildStatuses(test.graph) + + sortConditions(result) + sortConditions(test.expected) + if diff := cmp.Diff(test.expected, result); diff != "" { t.Errorf("buildStatuses() '%v' mismatch (-want +got):\n%s", test.msg, diff) } diff --git a/internal/status/httproute.go b/internal/status/httproute.go index 367f83f37..f2c928f09 100644 --- a/internal/status/httproute.go +++ b/internal/status/httproute.go @@ -8,6 +8,7 @@ import ( "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" ) // prepareHTTPRouteStatus prepares the status for an HTTPRoute resource. @@ -32,19 +33,6 @@ func prepareHTTPRouteStatus( for _, name := range names { ps := status.ParentStatuses[name] - var ( - conditionStatus metav1.ConditionStatus - reason string // FIXME(pleshakov) use RouteConditionReason - ) - - if ps.Attached { - conditionStatus = metav1.ConditionTrue - reason = "Accepted" // FIXME(pleshakov): use RouteReasonAccepted - } else { - conditionStatus = metav1.ConditionFalse - reason = "NotAttached" // FIXME(pleshakov): use a more specific message from the defined constants - } - sectionName := name p := v1beta1.RouteParentStatus{ @@ -54,16 +42,7 @@ func prepareHTTPRouteStatus( SectionName: (*v1beta1.SectionName)(§ionName), }, ControllerName: v1beta1.GatewayController(gatewayCtlrName), - Conditions: []metav1.Condition{ - { - Type: string(v1beta1.RouteConditionAccepted), - Status: conditionStatus, - ObservedGeneration: status.ObservedGeneration, - LastTransitionTime: transitionTime, - Reason: reason, - Message: "", // FIXME(pleshakov): Figure out a good message - }, - }, + Conditions: convertRouteConditions(ps.Conditions, status.ObservedGeneration, transitionTime), } parents = append(parents, p) } @@ -74,3 +53,24 @@ func prepareHTTPRouteStatus( }, } } + +func convertRouteConditions( + routeConds []conditions.RouteCondition, + observedGeneration int64, + transitionTime metav1.Time, +) []metav1.Condition { + conds := make([]metav1.Condition, len(routeConds)) + + for i := range routeConds { + conds[i] = metav1.Condition{ + Type: string(routeConds[i].Type), + Status: routeConds[i].Status, + ObservedGeneration: observedGeneration, + LastTransitionTime: transitionTime, + Reason: string(routeConds[i].Reason), + Message: routeConds[i].Message, + } + } + + return conds +} diff --git a/internal/status/httproute_test.go b/internal/status/httproute_test.go index 7b2965661..41850864c 100644 --- a/internal/status/httproute_test.go +++ b/internal/status/httproute_test.go @@ -4,24 +4,36 @@ import ( "testing" "time" - "github.com/google/go-cmp/cmp" + . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" ) func TestPrepareHTTPRouteStatus(t *testing.T) { + g := NewGomegaWithT(t) + + acceptedTrue := conditions.RouteCondition{ + Type: v1beta1.RouteConditionAccepted, + Status: metav1.ConditionTrue, + } + acceptedFalse := conditions.RouteCondition{ + Type: v1beta1.RouteConditionAccepted, + Status: metav1.ConditionFalse, + } + status := state.HTTPRouteStatus{ ObservedGeneration: 1, ParentStatuses: map[string]state.ParentStatus{ - "attached": { - Attached: true, + "accepted": { + Conditions: []conditions.RouteCondition{acceptedTrue}, }, - "not-attached": { - Attached: false, + "not-accepted": { + Conditions: []conditions.RouteCondition{acceptedFalse}, }, }, } @@ -38,7 +50,7 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { ParentRef: v1beta1.ParentReference{ Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("test")), Name: "gateway", - SectionName: (*v1beta1.SectionName)(helpers.GetStringPointer("attached")), + SectionName: (*v1beta1.SectionName)(helpers.GetStringPointer("accepted")), }, ControllerName: v1beta1.GatewayController(gatewayCtlrName), Conditions: []metav1.Condition{ @@ -47,7 +59,6 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { Status: metav1.ConditionTrue, ObservedGeneration: 1, LastTransitionTime: transitionTime, - Reason: "Accepted", }, }, }, @@ -55,7 +66,7 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { ParentRef: v1beta1.ParentReference{ Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("test")), Name: "gateway", - SectionName: (*v1beta1.SectionName)(helpers.GetStringPointer("not-attached")), + SectionName: (*v1beta1.SectionName)(helpers.GetStringPointer("not-accepted")), }, ControllerName: v1beta1.GatewayController(gatewayCtlrName), Conditions: []metav1.Condition{ @@ -64,16 +75,57 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { Status: metav1.ConditionFalse, ObservedGeneration: 1, LastTransitionTime: transitionTime, - Reason: "NotAttached", }, }, }, }, }, } - result := prepareHTTPRouteStatus(status, gwNsName, gatewayCtlrName, transitionTime) - if diff := cmp.Diff(expected, result); diff != "" { - t.Errorf("prepareHTTPRouteStatus() mismatch (-want +got):\n%s", diff) + g.Expect(helpers.Diff(expected, result)).To(BeEmpty()) +} + +func TestConvertRouteConditions(t *testing.T) { + g := NewGomegaWithT(t) + + routeConds := []conditions.RouteCondition{ + { + Type: "Test", + Status: metav1.ConditionTrue, + Reason: "reason1", + Message: "message1", + }, + { + Type: "Test", + Status: metav1.ConditionFalse, + Reason: "reason2", + Message: "message2", + }, } + + var generation int64 = 1 + transitionTime := metav1.NewTime(time.Now()) + + expected := []metav1.Condition{ + { + Type: "Test", + Status: metav1.ConditionTrue, + ObservedGeneration: generation, + LastTransitionTime: transitionTime, + Reason: "reason1", + Message: "message1", + }, + { + Type: "Test", + Status: metav1.ConditionFalse, + ObservedGeneration: generation, + LastTransitionTime: transitionTime, + Reason: "reason2", + Message: "message2", + }, + } + + result := convertRouteConditions(routeConds, generation, transitionTime) + + g.Expect(helpers.Diff(expected, result)).To(BeEmpty()) } diff --git a/internal/status/updater_test.go b/internal/status/updater_test.go index e5c833495..3dec7e401 100644 --- a/internal/status/updater_test.go +++ b/internal/status/updater_test.go @@ -17,6 +17,7 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" "github.com/nginxinc/nginx-kubernetes-gateway/internal/status" "github.com/nginxinc/nginx-kubernetes-gateway/internal/status/statusfakes" ) @@ -95,7 +96,12 @@ var _ = Describe("Updater", func() { ObservedGeneration: 5, ParentStatuses: map[string]state.ParentStatus{ "http": { - Attached: valid, + Conditions: []conditions.RouteCondition{ + { + Type: "Test", + Status: metav1.ConditionTrue, + }, + }, }, }, }, @@ -214,11 +220,10 @@ var _ = Describe("Updater", func() { }, Conditions: []metav1.Condition{ { - Type: string(gatewayv1beta1.RouteConditionAccepted), + Type: "Test", Status: metav1.ConditionTrue, ObservedGeneration: 5, LastTransitionTime: fakeClockTime, - Reason: "Accepted", }, }, }, From cf204aa0712e04e17ed6007454a39239b9fd2fe4 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 16 Nov 2022 12:43:21 -0500 Subject: [PATCH 2/2] Add links to issues for new FIXMEs --- internal/state/graph.go | 3 ++- internal/state/statuses.go | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/state/graph.go b/internal/state/graph.go index 6350ece6b..df8d1d52c 100644 --- a/internal/state/graph.go +++ b/internal/state/graph.go @@ -237,7 +237,7 @@ func bindHTTPRouteToListeners( l, exists := listeners[name] if !exists { // FIXME(pleshakov): Add a proper condition once it is available in the Gateway API. - // See also https://github.com/kubernetes-sigs/gateway-api/discussions/1445 + // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/306 r.InvalidSectionNameRefs[name] = conditions.NewRouteTODO("listener is not found") continue } @@ -263,6 +263,7 @@ func bindHTTPRouteToListeners( if _, exist := ignoredGws[key]; exist { // FIXME(pleshakov): Add a proper condition. + // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/306 r.InvalidSectionNameRefs[name] = conditions.NewRouteTODO("Gateway is ignored") processed = true diff --git a/internal/state/statuses.go b/internal/state/statuses.go index 889874d66..aea126939 100644 --- a/internal/state/statuses.go +++ b/internal/state/statuses.go @@ -144,6 +144,7 @@ func buildBaseRouteConditions(gcValidAndExist bool) []conditions.RouteCondition // FIXME(pleshakov): Figure out appropriate conditions for the cases when: // (1) GatewayClass is invalid. // (2) GatewayClass does not exist. + // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/307 if !gcValidAndExist { conds = append(conds, conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist")) }