-
Notifications
You must be signed in to change notification settings - Fork 502
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.
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
docs: add ssl passthrough note in FAQ #844
docs: add ssl passthrough note in FAQ #844
Conversation
5838b30
to
174161e
Compare
174161e
to
283c7d7
Compare
283c7d7
to
22d06c3
Compare
LGTM (giving other people a chance to look) |
Should we make a corresponding spec change? |
I think something in the godoc would be a good idea, agreed. Maybe a note in the Listener docstring, or in the HTTPRoute one? Wording of the change LGTM though. |
22d06c3
to
0ededfc
Compare
0ededfc was added to add a similar note to the |
apis/v1alpha2/httproute_types.go
Outdated
// Note that SSL passthrough is not supported for these backend refs, and | ||
// if you're looking to implement that kind of functionality use TLSRoute. | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of adding this here, it might be better to add this to the Passthrough godocs here: https://github.com/kubernetes-sigs/gateway-api/blob/master/apis/v1alpha2/gateway_types.go#L337-L340
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved as suggested in 34d584e 👍, lmkwyt
site-src/faq.md
Outdated
terminating it) is not currently a supported routing option when using the | ||
[HTTPRoute][routes] API. If you need this functionality it is supported by | ||
the [TLSRoute][tlsroute] API. Also see the [TLS Guide][tlsguide] for more | ||
details about passthrough and other TLS configurations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend framing it rather as "Yes, it is supported using TLSRoute. It is not supported in HTTPRoute because it is not possible to route based on HTTP metadata without terminating TLS."
This educates our users. The current wording makes me feel "something is possible but is not supported in the API".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, 3dc2ed4 changes the language lmkwyt
38f3d39
to
f3ac23d
Compare
f3ac23d
to
3dc2ed4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on this!
Co-authored-by: Rob Scott <rob.scott87@gmail.com>
Thanks! /lgtm |
Weird, I didn't know that would remove lgtm and approved labels... Trying again. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: robscott, shaneutt 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 |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
The
HTTPRoute
API (specifically) doesn't support SSL Passthrough. An implementer recently reported this wasn't clear. There is documentation on this, but in order to help make this more clear/prominent this patch adds a note about it in the FAQ with links to the relevant docs and guides.Which issue(s) this PR fixes:
Fixes #840
Does this PR introduce a user-facing change?: