From 8066f9fe8b8cb93850458e7be560d9aa0f49b778 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 18 Oct 2023 16:35:33 -0700 Subject: [PATCH 1/6] Add partial validity condition and tests --- .../static/state/conditions/conditions.go | 14 ++ internal/mode/static/state/graph/httproute.go | 6 +- .../mode/static/state/graph/httproute_test.go | 127 +++++++++++++++++- 3 files changed, 139 insertions(+), 8 deletions(-) diff --git a/internal/mode/static/state/conditions/conditions.go b/internal/mode/static/state/conditions/conditions.go index 0c3b33c104..7e719eed73 100644 --- a/internal/mode/static/state/conditions/conditions.go +++ b/internal/mode/static/state/conditions/conditions.go @@ -57,6 +57,9 @@ 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. + RouteConditionPartiallyInvalid v1beta1.RouteConditionType = "PartiallyInvalid" ) // DeduplicateConditions removes duplicate conditions based on the condition type. @@ -151,6 +154,17 @@ func NewRouteUnsupportedValue(msg string) conditions.Condition { } } +// NewRoutePartiallyInvalid returns a Condition that indicates that the HTTPRoute contains a combination +// of both valid and invalid rules. +func NewRoutePartiallyInvalid(msg string) conditions.Condition { + return conditions.Condition{ + Type: string(RouteConditionPartiallyInvalid), + Status: metav1.ConditionTrue, + Reason: string(v1beta1.RouteReasonUnsupportedValue), + Message: 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..b6fb03e446 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -233,10 +233,8 @@ 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)) + msg = "Dropped Rule(s): " + 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..7c02ceccbd 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -355,7 +355,21 @@ func TestBuildRoute(t *testing.T) { addFilterToPath(hrInvalidFilters, "/filter", invalidFilter) hrInvalidValidRules := createHTTPRoute("hr", gatewayNsName.Name, "example.com", invalidPath, "/filter", "/") - addFilterToPath(hrInvalidValidRules, "/filter", invalidFilter) + + hrInvalidValidRulesWithInvalidFilter := createHTTPRoute( + "hr", + gatewayNsName.Name, + "example.com", + invalidPath, "/filter", "/") + addFilterToPath(hrInvalidValidRulesWithInvalidFilter, "/filter", invalidFilter) + + hrInvalidValidFilter := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter", "/") + addFilterToPath(hrInvalidValidFilter, "/filter", validFilter) + addFilterToPath(hrInvalidValidFilter, "/", invalidFilter) + + hrInvalidValidRuleAndFilter := createHTTPRoute("hr", gatewayNsName.Name, "example.com", invalidPath, "/filter", "/") + addFilterToPath(hrInvalidValidRuleAndFilter, "/filter", validFilter) + addFilterToPath(hrInvalidValidRuleAndFilter, "/", invalidFilter) validatorInvalidFieldsInRule := &validationfakes.FakeHTTPFieldsValidator{ ValidatePathInMatchStub: func(path string) error { @@ -495,8 +509,44 @@ func TestBuildRoute(t *testing.T) { }, }, Conditions: []conditions.Condition{ - staticConds.NewTODO( - `Some rules are invalid: ` + + staticConds.NewRoutePartiallyInvalid( + `Dropped Rule(s): ` + + `spec.rules[0].matches[0].path.value: Invalid value: "/invalid": invalid path`, + ), + }, + Rules: []Rule{ + { + ValidMatches: false, + ValidFilters: true, + }, + { + ValidMatches: true, + ValidFilters: true, + }, + { + ValidMatches: true, + ValidFilters: true, + }, + }, + }, + name: "invalid and valid rules", + }, + + { + validator: validatorInvalidFieldsInRule, + hr: hrInvalidValidRulesWithInvalidFilter, + expected: &Route{ + Source: hrInvalidValidRulesWithInvalidFilter, + Valid: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + }, + }, + Conditions: []conditions.Condition{ + staticConds.NewRoutePartiallyInvalid( + `Dropped Rule(s): ` + `[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 +567,76 @@ func TestBuildRoute(t *testing.T) { }, }, }, - name: "invalid with invalid and valid rules", + name: "invalid filter with invalid and valid rules", + }, + { + validator: validatorInvalidFieldsInRule, + hr: hrInvalidValidFilter, + expected: &Route{ + Source: hrInvalidValidFilter, + Valid: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + }, + }, + Conditions: []conditions.Condition{ + staticConds.NewRoutePartiallyInvalid( + `Dropped Rule(s): ` + + `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: "invalid and valid filter with valid rules", + }, + { + validator: validatorInvalidFieldsInRule, + hr: hrInvalidValidRuleAndFilter, + expected: &Route{ + Source: hrInvalidValidRuleAndFilter, + Valid: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + }, + }, + Conditions: []conditions.Condition{ + staticConds.NewRoutePartiallyInvalid( + `Dropped Rule(s): ` + + `[spec.rules[0].matches[0].path.value: Invalid value: "/invalid": invalid path, ` + + `spec.rules[2].filters[0].requestRedirect.hostname: Invalid value: ` + + `"invalid.example.com": invalid hostname]`, + ), + }, + Rules: []Rule{ + { + ValidMatches: false, + ValidFilters: true, + }, + { + ValidMatches: true, + ValidFilters: true, + }, + { + ValidMatches: true, + ValidFilters: false, + }, + }, + }, + name: "invalid filter and rules with valid filter and rules", }, } From 8619e41461b3222964fb3c897a4d60c8968fa61f Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 18 Oct 2023 16:43:45 -0700 Subject: [PATCH 2/6] Update gateway api combatibility --- docs/gateway-api-compatibility.md | 1 + 1 file changed, 1 insertion(+) 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 From 25d5cbfb015a73f52d5ec86cab256ba270b8baa7 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 19 Oct 2023 10:49:37 -0700 Subject: [PATCH 3/6] Add some review feedback --- internal/mode/static/state/conditions/conditions.go | 6 +++++- internal/mode/static/state/graph/httproute.go | 1 - internal/mode/static/state/graph/httproute_test.go | 12 ++++-------- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/internal/mode/static/state/conditions/conditions.go b/internal/mode/static/state/conditions/conditions.go index 7e719eed73..f7822d4d86 100644 --- a/internal/mode/static/state/conditions/conditions.go +++ b/internal/mode/static/state/conditions/conditions.go @@ -156,12 +156,16 @@ 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: msg, + Message: "Dropped Rule(s): " + msg, } } diff --git a/internal/mode/static/state/graph/httproute.go b/internal/mode/static/state/graph/httproute.go index b6fb03e446..c60e2bb15d 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -233,7 +233,6 @@ func buildRoute( msg := allRulesErrs.ToAggregate().Error() if atLeastOneValid { - msg = "Dropped Rule(s): " + msg r.Conditions = append(r.Conditions, staticConds.NewRoutePartiallyInvalid(msg)) } else { msg = "All rules are invalid: " + msg diff --git a/internal/mode/static/state/graph/httproute_test.go b/internal/mode/static/state/graph/httproute_test.go index 7c02ceccbd..fa539d1fc8 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -510,8 +510,7 @@ func TestBuildRoute(t *testing.T) { }, Conditions: []conditions.Condition{ staticConds.NewRoutePartiallyInvalid( - `Dropped Rule(s): ` + - `spec.rules[0].matches[0].path.value: Invalid value: "/invalid": invalid path`, + `spec.rules[0].matches[0].path.value: Invalid value: "/invalid": invalid path`, ), }, Rules: []Rule{ @@ -546,8 +545,7 @@ func TestBuildRoute(t *testing.T) { }, Conditions: []conditions.Condition{ staticConds.NewRoutePartiallyInvalid( - `Dropped Rule(s): ` + - `[spec.rules[0].matches[0].path.value: Invalid value: "/invalid": invalid path, ` + + `[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]`, ), @@ -583,8 +581,7 @@ func TestBuildRoute(t *testing.T) { }, Conditions: []conditions.Condition{ staticConds.NewRoutePartiallyInvalid( - `Dropped Rule(s): ` + - `spec.rules[1].filters[0].requestRedirect.hostname: Invalid value: ` + + `spec.rules[1].filters[0].requestRedirect.hostname: Invalid value: ` + `"invalid.example.com": invalid hostname`, ), }, @@ -615,8 +612,7 @@ func TestBuildRoute(t *testing.T) { }, Conditions: []conditions.Condition{ staticConds.NewRoutePartiallyInvalid( - `Dropped Rule(s): ` + - `[spec.rules[0].matches[0].path.value: Invalid value: "/invalid": invalid path, ` + + `[spec.rules[0].matches[0].path.value: Invalid value: "/invalid": invalid path, ` + `spec.rules[2].filters[0].requestRedirect.hostname: Invalid value: ` + `"invalid.example.com": invalid hostname]`, ), From eee6bf667da2dde06526b7e59f94befa08a64cf9 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 19 Oct 2023 10:56:46 -0700 Subject: [PATCH 4/6] Add FIXME and issue relating to it --- internal/mode/static/state/conditions/conditions.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/mode/static/state/conditions/conditions.go b/internal/mode/static/state/conditions/conditions.go index f7822d4d86..1dbbb9ec72 100644 --- a/internal/mode/static/state/conditions/conditions.go +++ b/internal/mode/static/state/conditions/conditions.go @@ -59,6 +59,9 @@ const ( "is programmed again" // RouteConditionPartiallyInvalid is a condition which indicates that the Route contains // a combination of both valid and invalid rules. + // + // FIXME(Jee): Update to Gateway sig v1 version when released. + // https://github.com/nginxinc/nginx-gateway-fabric/issues/1168 RouteConditionPartiallyInvalid v1beta1.RouteConditionType = "PartiallyInvalid" ) From 34e8a0f0c6c4d793d6789c57b1570d7e0e0821e9 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 19 Oct 2023 13:31:33 -0700 Subject: [PATCH 5/6] Fix naming and remove an uncessary test case --- .../mode/static/state/graph/httproute_test.go | 74 ++++--------------- 1 file changed, 15 insertions(+), 59 deletions(-) diff --git a/internal/mode/static/state/graph/httproute_test.go b/internal/mode/static/state/graph/httproute_test.go index fa539d1fc8..85d5cb8167 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -354,22 +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", "/") + hrDroppedInvalidMatches := createHTTPRoute("hr", gatewayNsName.Name, "example.com", invalidPath, "/") - hrInvalidValidRulesWithInvalidFilter := createHTTPRoute( + hrDroppedInvalidMatchesAndInvalidFilter := createHTTPRoute( "hr", gatewayNsName.Name, "example.com", invalidPath, "/filter", "/") - addFilterToPath(hrInvalidValidRulesWithInvalidFilter, "/filter", invalidFilter) + addFilterToPath(hrDroppedInvalidMatchesAndInvalidFilter, "/filter", invalidFilter) - hrInvalidValidFilter := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter", "/") - addFilterToPath(hrInvalidValidFilter, "/filter", validFilter) - addFilterToPath(hrInvalidValidFilter, "/", invalidFilter) - - hrInvalidValidRuleAndFilter := createHTTPRoute("hr", gatewayNsName.Name, "example.com", invalidPath, "/filter", "/") - addFilterToPath(hrInvalidValidRuleAndFilter, "/filter", validFilter) - addFilterToPath(hrInvalidValidRuleAndFilter, "/", invalidFilter) + hrDroppedInvalidFilters := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter", "/") + addFilterToPath(hrDroppedInvalidFilters, "/filter", validFilter) + addFilterToPath(hrDroppedInvalidFilters, "/", invalidFilter) validatorInvalidFieldsInRule := &validationfakes.FakeHTTPFieldsValidator{ ValidatePathInMatchStub: func(path string) error { @@ -498,9 +494,9 @@ func TestBuildRoute(t *testing.T) { }, { validator: validatorInvalidFieldsInRule, - hr: hrInvalidValidRules, + hr: hrDroppedInvalidMatches, expected: &Route{ - Source: hrInvalidValidRules, + Source: hrDroppedInvalidMatches, Valid: true, ParentRefs: []ParentRef{ { @@ -522,20 +518,16 @@ func TestBuildRoute(t *testing.T) { ValidMatches: true, ValidFilters: true, }, - { - ValidMatches: true, - ValidFilters: true, - }, }, }, - name: "invalid and valid rules", + name: "dropped invalid rule with invalid matches", }, { validator: validatorInvalidFieldsInRule, - hr: hrInvalidValidRulesWithInvalidFilter, + hr: hrDroppedInvalidMatchesAndInvalidFilter, expected: &Route{ - Source: hrInvalidValidRulesWithInvalidFilter, + Source: hrDroppedInvalidMatchesAndInvalidFilter, Valid: true, ParentRefs: []ParentRef{ { @@ -565,13 +557,13 @@ func TestBuildRoute(t *testing.T) { }, }, }, - name: "invalid filter with invalid and valid rules", + name: "dropped invalid rule with invalid filters and invalid rule with invalid matches", }, { validator: validatorInvalidFieldsInRule, - hr: hrInvalidValidFilter, + hr: hrDroppedInvalidFilters, expected: &Route{ - Source: hrInvalidValidFilter, + Source: hrDroppedInvalidFilters, Valid: true, ParentRefs: []ParentRef{ { @@ -596,43 +588,7 @@ func TestBuildRoute(t *testing.T) { }, }, }, - name: "invalid and valid filter with valid rules", - }, - { - validator: validatorInvalidFieldsInRule, - hr: hrInvalidValidRuleAndFilter, - expected: &Route{ - Source: hrInvalidValidRuleAndFilter, - 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[2].filters[0].requestRedirect.hostname: Invalid value: ` + - `"invalid.example.com": invalid hostname]`, - ), - }, - Rules: []Rule{ - { - ValidMatches: false, - ValidFilters: true, - }, - { - ValidMatches: true, - ValidFilters: true, - }, - { - ValidMatches: true, - ValidFilters: false, - }, - }, - }, - name: "invalid filter and rules with valid filter and rules", + name: "dropped invalid rule with invalid filters", }, } From 1cf60cef62ff607ed4e600554b93ac8647004f81 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 19 Oct 2023 14:01:05 -0700 Subject: [PATCH 6/6] Add small touchups --- internal/mode/static/state/conditions/conditions.go | 2 +- internal/mode/static/state/graph/httproute_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/mode/static/state/conditions/conditions.go b/internal/mode/static/state/conditions/conditions.go index 1dbbb9ec72..2154a3197e 100644 --- a/internal/mode/static/state/conditions/conditions.go +++ b/internal/mode/static/state/conditions/conditions.go @@ -60,7 +60,7 @@ const ( // RouteConditionPartiallyInvalid is a condition which indicates that the Route contains // a combination of both valid and invalid rules. // - // FIXME(Jee): Update to Gateway sig v1 version when released. + // FIXME(bjee19): Update to Gateway sig v1 version when released. // https://github.com/nginxinc/nginx-gateway-fabric/issues/1168 RouteConditionPartiallyInvalid v1beta1.RouteConditionType = "PartiallyInvalid" ) diff --git a/internal/mode/static/state/graph/httproute_test.go b/internal/mode/static/state/graph/httproute_test.go index 85d5cb8167..31cf5f76b7 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -356,12 +356,12 @@ func TestBuildRoute(t *testing.T) { hrDroppedInvalidMatches := createHTTPRoute("hr", gatewayNsName.Name, "example.com", invalidPath, "/") - hrDroppedInvalidMatchesAndInvalidFilter := createHTTPRoute( + hrDroppedInvalidMatchesAndInvalidFilters := createHTTPRoute( "hr", gatewayNsName.Name, "example.com", invalidPath, "/filter", "/") - addFilterToPath(hrDroppedInvalidMatchesAndInvalidFilter, "/filter", invalidFilter) + addFilterToPath(hrDroppedInvalidMatchesAndInvalidFilters, "/filter", invalidFilter) hrDroppedInvalidFilters := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter", "/") addFilterToPath(hrDroppedInvalidFilters, "/filter", validFilter) @@ -525,9 +525,9 @@ func TestBuildRoute(t *testing.T) { { validator: validatorInvalidFieldsInRule, - hr: hrDroppedInvalidMatchesAndInvalidFilter, + hr: hrDroppedInvalidMatchesAndInvalidFilters, expected: &Route{ - Source: hrDroppedInvalidMatchesAndInvalidFilter, + Source: hrDroppedInvalidMatchesAndInvalidFilters, Valid: true, ParentRefs: []ParentRef{ {