Clarifying how HTTP and TLS Route status should be populated #859
Conversation
|
@robscott: GitHub didn't allow me to request PR reviews from the following users: howardjohn. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
k8s-ci-robot
added
cncf-cla: yes
|
[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:
Approvers can indicate their approval by writing |
apis/v1alpha2/httproute_types.go
Outdated
| // If hostnames are specified, and none match with the criteria above, then | ||
| // the HTTPRoute is not accepted, and the implementation must raise an | ||
| // 'Accepted' Condition with a status of `False` in the corresponding | ||
| // RouteParentStatus. |
There was a problem hiding this comment.
What if the route specifies two hostnames - one satisfied the above criteria but the other one doesn't?
There was a problem hiding this comment.
must be at least one intersecting hostname for the HTTPRoute to be
Maybe this line clarifies that?
There was a problem hiding this comment.
Yep, that's what I was going for. I also hope that "and none match the criteria" helps convey that only one would need to match.
apis/v1alpha2/httproute_types.go
Outdated
| // If hostnames do not match with the criteria above, then the HTTPRoute is | ||
| // not accepted, and the implementation must raise an 'Accepted' Condition | ||
| // with a status of `False` for the target Listener(s). | ||
| // If hostnames are specified, and none match with the criteria above, then |
There was a problem hiding this comment.
| // If hostnames are specified, and none match with the criteria above, then | |
| // When hostnames specified in Route and none match with the criteria above, then |
There was a problem hiding this comment.
There's a bit of nuance here, it's really only if hostnames are specified on both the Gateway and Route. If they are absent in either place, everything should match. I'll work on clarifying that.
There was a problem hiding this comment.
Yes, that's what I was trying to capture as well but probably didn't do a good job either.
I was trying to imply gateway's listener hostname with the "criteria above" part of the sentence but that feels indirect as well.
There was a problem hiding this comment.
Thanks for catching this @hbagdi! I've updated the PR to make this more clear, PTAL.
robscott
force-pushed
the
tls-http-accepted
branch
from
88e2680
to
169b62f
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
What type of PR is this?
/kind bug
What this PR does / why we need it:
This fixes the confusing guidance (originally added by me) in HTTP and TLS Routes that suggested that conditions should be populated on Gateway Listeners. I had originally built out conditions that could be populated on Gateway and Listeners, but I'm not sure that's the right approach. I think the simplest and most straightforward answer here is to populate Route status. If Route owners can't attach to a Gateway they want to, they can simply contact the Gateway owner.
Which issue(s) this PR fixes:
Fixes #849
Does this PR introduce a user-facing change?:
/cc @youngnick @howardjohn