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.

Sign up for GitHub

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

Jump to bottom

Adding new PartiallyInvalid condition for Routes #2429

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Adding new PartiallyInvalid condition for Routes #2429

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 42 additions & 3 deletions apis/v1beta1/shared_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ const (
// This condition indicates whether the route has been accepted or rejected
// by a Gateway, and why.
//
// Possible reasons for this condition to be true are:
// Possible reasons for this condition to be True are:
//
// * "Accepted"
//
Expand Down Expand Up @@ -349,15 +349,17 @@ const (
// are incompatible filters present on a route rule (for example if
// the URLRewrite and RequestRedirect are both present on an HTTPRoute).
RouteReasonIncompatibleFilters RouteConditionReason = "IncompatibleFilters"
)

const (
// This condition indicates whether the controller was able to resolve all
// the object references for the Route.
//
// Possible reasons for this condition to be true are:
// Possible reasons for this condition to be True are:
//
// * "ResolvedRefs"
//
// Possible reasons for this condition to be false are:
// Possible reasons for this condition to be False are:
//
// * "RefNotPermitted"
// * "InvalidKind"
Expand Down Expand Up @@ -394,6 +396,43 @@ const (
RouteReasonUnsupportedProtocol RouteConditionReason = "UnsupportedProtocol"
)

const (
// This condition indicates that the Route contains a combination of both
// valid and invalid rules.
//
// When this happens, implementations MUST take one of the following
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that the two options described below are explicitly stated. In Contour we've had a somewhat inconsistent mix in our own CRD throughout history of "dropping" invalid rules, programming 5xx responses for invalid rules while preserving valid ones, attempting to program as much of an invalid rule as we can but giving warnings/errors in status about invalid attributes of a rule, etc., which all make for a bit of an inconsistent UX. Explicitly giving users feedback in status about what is happening is a step forward IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, completely agree, I think many implementations have inconsistent behavior here.

// approaches:
//
// 1) Drop Rule(s): With this approach, implementations will drop the
// invalid Route Rule(s) until they are fully valid again. The message
// for this condition MUST start with the prefix "Dropped Rule" and
// include information about which Rules have been dropped. In this
// state, the "Accepted" condition MUST be set to "True" with the latest
// generation of the resource.
// 2) Fall Back: With this approach, implementations will fall back to the
// last known good state of the entire Route. The message for this
// condition MUST start with the prefix "Fall Back" and include
Comment on lines +413 to +414
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using message as a typed/specified thing seems a bit odd/unexpected/nonstandard?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but there we don't have another way to indicate this information, and we really need to have some guidance around this before GA. Ideas welcomed, I guess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe adding new condition types with this info would work? Another condition that would only be set if PartiallyInvalid is also set

- type: PartiallyInvalid
  status: true
  reason: UnsupportedValue
  message: ...
- type: PartiallyInvalid[DropRouteRule|RouteFallBack]
  status: true
  reason: Implementation[DropsRulesOnPartialInvalidity|FallsBackOnPartialInvalidity]
  message: ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agree this is a bit wonky, but I don't love the alternatives here. @sunjayBhatia's idea to add additional conditions to more clearly reflect this state may actually be something we need to do in the future, but I'd like to see if we can survive without adding more conditions first. If we try it out and determine that it would really be best to add additional conditions, we can do that later.

// information about why the current Rule(s) are invalid. To represent
// this, the "Accepted" condition MUST be set to "True" with the
// generation of the last known good state of the resource.
//
// Reverting to the last known good state should only be done by
// implementations that have a means of restoring that state if/when they
// are restarted.
//
// This condition MUST NOT be set if a Route is fully valid, fully invalid,
// or not accepted. By extension, that means that this condition MUST only
// be set when it is "True".
//
// Possible reasons for this condition to be True are:
//
// * "UnsupportedValue"
//
// Controllers may raise this condition with other reasons, but should
// prefer to use the reasons listed above to improve interoperability.
RouteConditionPartiallyInvalid RouteConditionType = "PartiallyInvalid"
)

// RouteParentStatus describes the status of a route with respect to an
// associated Parent.
type RouteParentStatus struct {
Expand Down