Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PartiallyInvalid HTTPRoute Condition #1160

Merged
merged 6 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/gateway-api-compatibility.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
21 changes: 21 additions & 0 deletions internal/mode/static/state/conditions/conditions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
bjee19 marked this conversation as resolved.
Show resolved Hide resolved
)

// DeduplicateConditions removes duplicate conditions based on the condition type.
Expand Down Expand Up @@ -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 {
Expand Down
5 changes: 1 addition & 4 deletions internal/mode/static/state/graph/httproute.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
87 changes: 79 additions & 8 deletions internal/mode/static/state/graph/httproute_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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{
{
Expand All @@ -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]`,
),
Expand All @@ -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",
},
}

Expand Down