From 89f6452d5fe649d06f2b766aad418a5fe883f27e Mon Sep 17 00:00:00 2001 From: bjee19 <139261241+bjee19@users.noreply.github.com> Date: Thu, 19 Oct 2023 14:14:39 -0700 Subject: [PATCH] Add PartiallyInvalid HTTPRoute Condition (#1160) Replace previously used TODO Conditions with PartiallyInvalid HTTPRoute Condition. Problem: There was no HTTPRoute Condition that conveyed when a Route was PartiallyInvalid. Solution: Added the PartiallyInvalid HTTPRoute Condition. --- docs/gateway-api-compatibility.md | 1 + .../static/state/conditions/conditions.go | 21 +++++ internal/mode/static/state/graph/httproute.go | 5 +- .../mode/static/state/graph/httproute_test.go | 87 +++++++++++++++++-- 4 files changed, 102 insertions(+), 12 deletions(-) diff --git a/docs/gateway-api-compatibility.md b/docs/gateway-api-compatibility.md index f9984d7d87..32eb1a7703 100644 --- a/docs/gateway-api-compatibility.md +++ b/docs/gateway-api-compatibility.md @@ -174,6 +174,7 @@ Fields: - `ResolvedRefs/False/BackendNotFound` - `ResolvedRefs/False/UnsupportedValue` - custom reason for when one of the HTTPRoute rules has a backendRef with an unsupported value. + - `PartiallyInvalid/True/UnsupportedValue` ### ReferenceGrant diff --git a/internal/mode/static/state/conditions/conditions.go b/internal/mode/static/state/conditions/conditions.go index 0c3b33c104..2154a3197e 100644 --- a/internal/mode/static/state/conditions/conditions.go +++ b/internal/mode/static/state/conditions/conditions.go @@ -57,6 +57,12 @@ const ( RouteMessageFailedNginxReload = GatewayMessageFailedNginxReload + ". NGINX may still be configured " + "for this HTTPRoute. However, future updates to this resource will not be configured until the Gateway " + "is programmed again" + // RouteConditionPartiallyInvalid is a condition which indicates that the Route contains + // a combination of both valid and invalid rules. + // + // FIXME(bjee19): Update to Gateway sig v1 version when released. + // https://github.com/nginxinc/nginx-gateway-fabric/issues/1168 + RouteConditionPartiallyInvalid v1beta1.RouteConditionType = "PartiallyInvalid" ) // DeduplicateConditions removes duplicate conditions based on the condition type. @@ -151,6 +157,21 @@ func NewRouteUnsupportedValue(msg string) conditions.Condition { } } +// NewRoutePartiallyInvalid returns a Condition that indicates that the HTTPRoute contains a combination +// of both valid and invalid rules. +// +// // nolint:lll +// The message must start with "Dropped Rules(s)" according to the Gateway API spec +// See https://github.com/kubernetes-sigs/gateway-api/blob/37d81593e5a965ed76582dbc1a2f56bbd57c0622/apis/v1/shared_types.go#L408-L413 +func NewRoutePartiallyInvalid(msg string) conditions.Condition { + return conditions.Condition{ + Type: string(RouteConditionPartiallyInvalid), + Status: metav1.ConditionTrue, + Reason: string(v1beta1.RouteReasonUnsupportedValue), + Message: "Dropped Rule(s): " + msg, + } +} + // NewRouteInvalidListener returns a Condition that indicates that the HTTPRoute is not accepted because of an // invalid listener. func NewRouteInvalidListener() conditions.Condition { diff --git a/internal/mode/static/state/graph/httproute.go b/internal/mode/static/state/graph/httproute.go index 5a6dc0966e..c60e2bb15d 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -233,10 +233,7 @@ func buildRoute( msg := allRulesErrs.ToAggregate().Error() if atLeastOneValid { - // FIXME(pleshakov): Partial validity for HTTPRoute rules is not defined in the Gateway API spec yet. - // See https://github.com/nginxinc/nginx-gateway-fabric/issues/485 - msg = "Some rules are invalid: " + msg - r.Conditions = append(r.Conditions, staticConds.NewTODO(msg)) + r.Conditions = append(r.Conditions, staticConds.NewRoutePartiallyInvalid(msg)) } else { msg = "All rules are invalid: " + msg r.Conditions = append(r.Conditions, staticConds.NewRouteUnsupportedValue(msg)) diff --git a/internal/mode/static/state/graph/httproute_test.go b/internal/mode/static/state/graph/httproute_test.go index 4d3fa26bb4..31cf5f76b7 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -354,8 +354,18 @@ func TestBuildRoute(t *testing.T) { hrInvalidFilters := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter") addFilterToPath(hrInvalidFilters, "/filter", invalidFilter) - hrInvalidValidRules := createHTTPRoute("hr", gatewayNsName.Name, "example.com", invalidPath, "/filter", "/") - addFilterToPath(hrInvalidValidRules, "/filter", invalidFilter) + hrDroppedInvalidMatches := createHTTPRoute("hr", gatewayNsName.Name, "example.com", invalidPath, "/") + + hrDroppedInvalidMatchesAndInvalidFilters := createHTTPRoute( + "hr", + gatewayNsName.Name, + "example.com", + invalidPath, "/filter", "/") + addFilterToPath(hrDroppedInvalidMatchesAndInvalidFilters, "/filter", invalidFilter) + + hrDroppedInvalidFilters := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter", "/") + addFilterToPath(hrDroppedInvalidFilters, "/filter", validFilter) + addFilterToPath(hrDroppedInvalidFilters, "/", invalidFilter) validatorInvalidFieldsInRule := &validationfakes.FakeHTTPFieldsValidator{ ValidatePathInMatchStub: func(path string) error { @@ -484,9 +494,9 @@ func TestBuildRoute(t *testing.T) { }, { validator: validatorInvalidFieldsInRule, - hr: hrInvalidValidRules, + hr: hrDroppedInvalidMatches, expected: &Route{ - Source: hrInvalidValidRules, + Source: hrDroppedInvalidMatches, Valid: true, ParentRefs: []ParentRef{ { @@ -495,9 +505,39 @@ func TestBuildRoute(t *testing.T) { }, }, Conditions: []conditions.Condition{ - staticConds.NewTODO( - `Some rules are invalid: ` + - `[spec.rules[0].matches[0].path.value: Invalid value: "/invalid": invalid path, ` + + staticConds.NewRoutePartiallyInvalid( + `spec.rules[0].matches[0].path.value: Invalid value: "/invalid": invalid path`, + ), + }, + Rules: []Rule{ + { + ValidMatches: false, + ValidFilters: true, + }, + { + ValidMatches: true, + ValidFilters: true, + }, + }, + }, + name: "dropped invalid rule with invalid matches", + }, + + { + validator: validatorInvalidFieldsInRule, + hr: hrDroppedInvalidMatchesAndInvalidFilters, + expected: &Route{ + Source: hrDroppedInvalidMatchesAndInvalidFilters, + Valid: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + }, + }, + Conditions: []conditions.Condition{ + staticConds.NewRoutePartiallyInvalid( + `[spec.rules[0].matches[0].path.value: Invalid value: "/invalid": invalid path, ` + `spec.rules[1].filters[0].requestRedirect.hostname: Invalid value: ` + `"invalid.example.com": invalid hostname]`, ), @@ -517,7 +557,38 @@ func TestBuildRoute(t *testing.T) { }, }, }, - name: "invalid with invalid and valid rules", + name: "dropped invalid rule with invalid filters and invalid rule with invalid matches", + }, + { + validator: validatorInvalidFieldsInRule, + hr: hrDroppedInvalidFilters, + expected: &Route{ + Source: hrDroppedInvalidFilters, + Valid: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + }, + }, + Conditions: []conditions.Condition{ + staticConds.NewRoutePartiallyInvalid( + `spec.rules[1].filters[0].requestRedirect.hostname: Invalid value: ` + + `"invalid.example.com": invalid hostname`, + ), + }, + Rules: []Rule{ + { + ValidMatches: true, + ValidFilters: true, + }, + { + ValidMatches: true, + ValidFilters: false, + }, + }, + }, + name: "dropped invalid rule with invalid filters", }, }