Clarify docs on invalid serviceName #756

howardjohn · 2021-08-09T21:35:39Z

Fixes #638

What type of PR is this?

/kind documentation

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #638

Does this PR introduce a user-facing change?:

NONE

Fixes kubernetes-sigs#638

k8s-ci-robot · 2021-08-09T21:35:47Z

Hi @howardjohn. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hbagdi · 2021-08-09T21:43:14Z

apis/v1alpha2/httproute_types.go

@@ -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


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

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.

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?

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.

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?

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.

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 :) )

robscott · 2021-08-10T00:15:37Z

/ok-to-test

robscott · 2021-08-10T00:39:41Z

apis/v1alpha2/tcproute_types.go

-	// 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.


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

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

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.

youngnick · 2021-08-11T06:35:33Z

apis/v1alpha2/tlsroute_types.go

-	// 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.


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.

robscott · 2021-08-12T22:36:41Z

I think this update lines up well with all the feedback above, thanks @howardjohn!

/lgtm

robscott · 2021-08-12T22:37:31Z

/approve

k8s-ci-robot · 2021-08-12T22:37:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: howardjohn, robscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [robscott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Clarify docs on invalid serviceName

3a00280

Fixes kubernetes-sigs#638

k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 9, 2021

k8s-ci-robot requested review from bowei and jpeach August 9, 2021 21:35

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 9, 2021

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 9, 2021

hbagdi reviewed Aug 9, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 10, 2021

robscott reviewed Aug 10, 2021

View reviewed changes

youngnick reviewed Aug 11, 2021

View reviewed changes

howardjohn added 2 commits August 12, 2021 14:51

update for comments

c72256d

Wrap

a1ca6b2

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 12, 2021

k8s-ci-robot assigned robscott Aug 12, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 12, 2021

robscott added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 12, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2021

k8s-ci-robot merged commit 70377ed into kubernetes-sigs:master Aug 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify docs on invalid serviceName #756

Clarify docs on invalid serviceName #756

howardjohn commented Aug 9, 2021

k8s-ci-robot commented Aug 9, 2021

hbagdi Aug 9, 2021

robscott Aug 9, 2021

stevesloka Aug 10, 2021

robscott Aug 10, 2021

stevesloka Aug 10, 2021

jpeach Aug 10, 2021

youngnick Aug 11, 2021 •

edited

Loading

robscott commented Aug 10, 2021

robscott Aug 10, 2021

hbagdi Aug 10, 2021

youngnick Aug 11, 2021 •

edited

Loading

youngnick Aug 11, 2021

robscott commented Aug 12, 2021

robscott commented Aug 12, 2021

k8s-ci-robot commented Aug 12, 2021

	// 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

	// 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.

	// 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.

Clarify docs on invalid serviceName #756

Clarify docs on invalid serviceName #756

Conversation

howardjohn commented Aug 9, 2021

k8s-ci-robot commented Aug 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

robscott commented Aug 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Aug 12, 2021

robscott commented Aug 12, 2021

k8s-ci-robot commented Aug 12, 2021

youngnick Aug 11, 2021 •

edited

Loading

youngnick Aug 11, 2021 •

edited

Loading