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..df8d1d52c 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. + // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/306 + 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,9 @@ 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. + // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/306 + 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..aea126939 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,17 @@ 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. + // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/307 + 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 58a3c44ec..3fee9cfe0 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", }, }, },