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

Clarify docs on invalid serviceName #756

Merged
merged 3 commits into from
Aug 12, 2021
Merged

Clarify docs on invalid serviceName #756

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a00280
Clarify docs on invalid serviceName
howardjohn Aug 9, 2021
c72256d
update for comments
howardjohn Aug 12, 2021
a1ca6b2
Wrap
howardjohn Aug 12, 2021
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
2 changes: 1 addition & 1 deletion apis/v1alpha2/httproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ type HTTPRouteRule struct {
Filters []HTTPRouteFilter `json:"filters,omitempty"`

// BackendRefs defines the backend(s) where matching requests should be
// sent. If unspecified, the rule performs no forwarding. If unspecified and
// sent. If unspecified or invalid, the rule performs no forwarding; if
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we further clarify what invalid means?
Does a regular k8s Service with no endpoints count as invalid?

Copy link
Member

Choose a reason for hiding this comment

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

Good point, wouldn't hurt to clarify that a Service with no endpoints should also not have traffic forwarded for it either. Maybe something like this:

A BackendRef is considered invalid when it refers to a non-existent resource or a Service that does not have any endpoints.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to clarify too if you have multiple backendrefs what happens. From the discussion in the issue, you'd lad balance across some valid and some that return 503s?

Copy link
Member

Choose a reason for hiding this comment

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

I personally think it makes more sense to leave the weighting as requested. IE if a user says that 80% of requests should go to an invalid backend, could we just make that 80% of requests 503? Although that's harsh, it may be better than the alternative of sending all traffic to an alpha endpoint, a failure that could potentially go unnoticed for a long time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just feel we need to call out whatever behavior is decided. In my head, you'd set status for the invalid but skip the invalid services for serving. But that's ok if others don't agree. 🙃

Given this model, if you only had one invalid service defined for a route, you'd serve 503s, bit would you set a status on the route?

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally think it makes more sense to leave the weighting as requested. IE if a user says that 80% of requests should go to an invalid backend, could we just make that 80% of requests 503? Although that's harsh, it may be better than the alternative of sending all traffic to an alpha endpoint, a failure that could potentially go unnoticed for a long time.

This was the consensus in the linked issue #638.

You could set an unresolved refs condition somewhere to report the inability to resolve an invalid service name.

Copy link
Contributor

@youngnick youngnick Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So, something like this?

Suggested change
// sent. If unspecified or invalid, the rule performs no forwarding; if
// sent. If unspecified or invalid (refers to a non-existent resource or a Service with no endpoints),
// the rule performs no forwarding; if

With another bit added afterwards:

	// 503 responses must be sent so that the overall weight is respected; if an invalid backend is requested
	// to have 80% of requests, then 80% of requests must get a 503 instead.

That makes sense to me. (With apologies to @howardjohn for writing your PR for you :) )

// no filters are specified that would result in a response being sent,
// a HTTP 503 status code is returned.
//
Expand Down
4 changes: 3 additions & 1 deletion apis/v1alpha2/tcproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,9 @@ type TCPRouteRule struct {
Matches []TCPRouteMatch `json:"matches,omitempty"`

// BackendRefs defines the backend(s) where matching requests should be
// sent.
// sent. If unspecified or invalid, the rule performs no forwarding; if
// no filters are specified that would result in a response being sent,
// a HTTP 503 status code is returned.
Copy link
Member

Choose a reason for hiding this comment

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

Missed this the first time, but the HTTP 503 bit doesn't make sense for TCP or other L4 protocols.

Copy link
Contributor

Choose a reason for hiding this comment

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

Close the connection for TCP and ignore the packet for UDP?

Copy link
Contributor

@youngnick youngnick Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe this?

Suggested change
// a HTTP 503 status code is returned.
// the underlying implementation must actively reject connection attempts to this backend, which
// in this case implies closing the TCP connection.

For UDP, the only option is to drop the packet on the floor.

//
// Support: Core for Kubernetes Service
// Support: Custom for any other resource
Expand Down
4 changes: 3 additions & 1 deletion apis/v1alpha2/tlsroute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,9 @@ type TLSRouteRule struct {
Matches []TLSRouteMatch `json:"matches,omitempty"`

// BackendRefs defines the backend(s) where matching requests should be
// sent.
// sent. If unspecified or invalid, the rule performs no forwarding; if
// no filters are specified that would result in a response being sent,
// a HTTP 503 status code is returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

As above with TCPRoute, it's not always given that TLSRoutes are using HTTP, so maybe something like this:

Suggested change
// a HTTP 503 status code is returned.
// the underlying implementation should actively reject connection attempts to this backend,
// either by sending a HTTP 503 status code, or closing the connection for non-HTTP TLSRoutes.

//
// Support: Core for Kubernetes Service
// Support: Custom for any other resource
Expand Down