clarify requirement for port in forwardTo #685
Conversation
k8s-ci-robot
added
the
kind/documentation
|
@hbagdi: GitHub didn't allow me to request PR reviews from the following users: stevesloka. 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: hbagdi 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 |
k8s-ci-robot
added
the
approved
|
Please see #578 (comment) for some additional context. |
|
/retest |
| // when forwarding to a backendRef or serviceName. | ||
| // Port is required when the request is being forwarded to a Kubernetes | ||
| // Service. Port is optional with BackendRef to enable use cases where | ||
| // the port should be read from the referrant resource. |
There was a problem hiding this comment.
This seems like something we should enforce with webhook validation. What should we do if BackendRef points to a Service?
There was a problem hiding this comment.
This seems like something we should enforce with webhook validation.
Absolutely, please see #578 (comment). With this patch and the validation bit, the user experience should be good.
What should we do if BackendRef points to a Service?
I didn't consider that part because I wrongly assumed that was not allowed. Is there a good reason to allow such a thing in the first place? At this level of detail, I personally prefer having only one way to do something.
There was a problem hiding this comment.
I didn't consider that part because I wrongly assumed that was not allowed. Is there a good reason to allow such a thing in the first place?
One potential example - say we end up adding the ability to forward traffic to a Service in another namespace. We have 3 options:
- Add a
backendRef.Namespacefield, this supports everything if Service can be referenced by BackendRef - Add a
serviceNamespacefield, this only supports Service - Add both a
serviceNamespacefield andbackendRef.Namespacefield
I'm pretty sure we want to avoid 3, and 2 could eventually lead to 3.
At this level of detail, I personally prefer having only one way to do something.
This may be a good argument against the serviceName shortcut altogether. I need to think about it more.
There was a problem hiding this comment.
This is devolving into a separate conversation on if we should have serviceName shortcut or not.
Let's take a look at the following two examples:
forwardTo:
- serviceName: foo
port: 80forwardTo:
- backendRef:
kind: Service
name: foo
port: 80The easy path is shorter but not significantly so. The two additional lines also help with creating awareness around this extension point. Given how many corner cases exist with this special case, I think it would be best if we drop serviceName special case from here and rest of the API as well.
@jpeach @bowei @robscott @danehans Would love to hear your take on this.
There was a problem hiding this comment.
Would it be confusing if backendRef.kind defaulted to Service? That would make the difference even smaller:
forwardTo:
- serviceName: foo
port: 80forwardTo:
- backendRef:
name: foo
port: 80Given that we've required port to be specified when the backend is a Service, there's no longer a simple serviceName only short form anyway.
There was a problem hiding this comment.
I'd rather take one extra line of YAML over solving the corner cases the shortcut is introducing.
Would love to hear from others.
There was a problem hiding this comment.
I agree with @hbagdi on this one. I don't see the serviceName shortcut adding enough value to make the effort worthwhile.
There was a problem hiding this comment.
If we are thinking about having backendRef default to Service then why not do the following:
forwardTo:
- backendName: foo
port: 80
kind: <defaults to Service>This still provides the shortcut, provides only 1 way of doing things, and is also the minimal number of lines. I hate to be so focused on number of YAML lines, but forwardTo is also one of the most heavily used fields in the API so the verbosity will be multiplied.
Using a key like backendName is also nice because backend is a proper term for thing that's being pointed to. I have already heard people call it "the forwardTo" which feels a bit off.
|
@hbagdi: PR needs rebase. 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
the
needs-rebase
|
@youngnick @robscott @mark-church I've introduced a small GEP to arrive at a decision here: #719. |
|
/close |
|
@hbagdi: Closed this PR. 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. |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
Clarify when port is required and when it is optional.
Which issue(s) this PR fixes:
For #578
Does this PR introduce a user-facing change?:
/cc @robscott @stevesloka @danehans