conformance: TLSRoute with Terminate mode
#4137
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
Welcome @phuhung273! |
k8s-ci-robot
added
size/L
|
Hi @phuhung273. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
/ok-to-test I'm not quite sure if this is specified explicitly (I don't see it mentioned in https://gateway-api.sigs.k8s.io/reference/spec/#listenertlsconfig or https://gateway-api.sigs.k8s.io/geps/gep-2907), but is |
k8s-ci-robot
added
ok-to-test
Thanks for taking a look @mikemorris. I'm not sure about that, but can see we have a current |
|
Yeah, this absolutely should have a new feature name, so that implementations can support as they are ready to. |
|
@phuhung273, thanks for getting us started! Also, while it's valid to use HTTP as the inner protocol, we should also end up testing bare TCP functions as well. |
Thank you also for taking a look @youngnick. Absolutely i will try this (although having no idea what youre saying currently 😅) Right now Im just trying to complete a simple case. This one seems useful https://github.com/projectcontour/contour/blob/main/internal/featuretests/v3/tlsroute_test.go, im trying to replicate the same. |
phuhung273
force-pushed
the
tls-terminate-conformance
branch
from
3e280d9 to
82b0822
Compare
phuhung273
changed the title
TLSRoute with Terminate modeTLSRoute with Terminate mode
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: phuhung273 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Verified with Contour, please see PR description for test output. Also added |
|
So, just clarifying: per our TLS Guide we have the following supports and cases:
I am wondering why we are considering a TLS = Terminate + TLSRoute here? Is this just an alternative to TLS = Terminate + TCPRoute? I think in this case it may be a bit misleading on which route I want / should use, if 2 do the same job. Also, we are explicitly saying on the TLSRoute GEP that we don't support TLSRoute termination (https://github.com/kubernetes-sigs/gateway-api/pull/4064/files#diff-7e6544694a096dc122ce2ef4d38e4a47bfe14b72d5ae3603af9c17f6ccf23339R33) so if we can first agree on the GEP on if we should or not, then move to Conformance I would appreciate for my own sanity 😅 Thanks! |
|
Ok can see this table in the guide Thanks @rikatz for the update. I will wait for GEP-2643 to finalize. But currently we don't have any conformance for TCPRoute in Terminate mode. So I can add one rite ? |
|
@rikatz TLSRoute support for attaching to Gateway listeners with It sounds like we may need to resolve some inconsistent documentation as mentioned in #1474? |
|
thanks Mike. I have missed those, or maybe and inconsistently left them behind. Will take a look on them, but I am wondering if it would be good/proper that we have all of this mapped on the GEP before moving with more conformance that may not reflect the final state of the GEP |
k8s-ci-robot
removed
the
needs-rebase
k8s-ci-robot
added
the
needs-rebase
@candita @phuhung273 have you seen #4330? Hopefully this could be a good start to better explain this! |
|
@phuhung273 I am planning on reviewing the PR this week, do you mind rebasing it? (we did a bunch of changes to move conformance dependencies out of main code, so you can use the mqtt library also without concerns!) Thanks! |
Co-authored-by: Mike Morris <[email protected]>
Signed-off-by: phuhung273 <[email protected]>
Signed-off-by: phuhung273 <[email protected]>
phuhung273
force-pushed
the
tls-terminate-conformance
branch
from
a16d4f5 to
b9af184
Compare
k8s-ci-robot
removed
the
needs-rebase
|
Thanks for reminding @rikatz and the conformance dependencies structure change, I've rebased. Good to see GEP-2643 finalized. |
|
/cc |
|
/assign |
| containers: | ||
| - name: mqtt-backend | ||
| # https://hub.docker.com/_/eclipse-mosquitto | ||
| image: eclipse-mosquitto:2 |
There was a problem hiding this comment.
2 final comments here:
- Please use the full image path due to recent changes on containerd and docker. So "hub.docker.com"
- As a followup, I think we should probably create a simple MQTT server on our echo-basic server to avoid people going out fetch another image.
I would like that the first change is made here, and we can open an issue for the 2nd one to implement on our echo-basic image (eg.: https://github.com/mochi-mqtt/server) before marking it as Standard.
|
|
||
| if token := c.Subscribe(topic, 0, func(_ mqtt.Client, msg mqtt.Message) { | ||
| t.Logf("Received message: %s\n", string(msg.Payload())) | ||
| wg.Done() |
There was a problem hiding this comment.
If you never receive a token back, this thread may be open forever right?
As a suggestion, instead of using a working group here, maybe use just a channel (as you did below) or use a token.WaitTimeout(5 * time.Second) ?
There was a problem hiding this comment.
Yes you're right. I don't have much experience with async so the logic was messy. Hopefully it is better now.
About the new change, im quite sure about DefaultTestTimeout in the last check because it ensures the entire test is in expected time. RequestTimeout description looks fit token.WaitTimeout but it can be infinite. Let me know what you think.
There was a problem hiding this comment.
that's fine! Thanks for taking care of all of it!
I am doing some tests locally here but some things on the message pub/sub are still failing, I will take a look what can be improved and make some suggestion on it!
Signed-off-by: phuhung273 <[email protected]>
|
@rikatz not sure why I cannot comment on your feedback regarding MQTT image. But agree it is better not requiring user to pull too many things. |
|
@phuhung273 a question, were you able to test this with some implementation? I am testing with kgateway and failing, but I also did tested with kgateway and a simple TCP service and it is failing. @davidjumani do you know if kgateway already supports TLSRoute on termination? Thanks! |
|
Yes @rikatz i tested My setup: |
|
yeah, Istio does not support tls termination with TLSRoute yet, just TCPRoute (cc @howardjohn ) I was going to try and make changes on Istio, but wanted first to see something passing this test to be sure. |
kgateway currently supports only passthrough with TLSRoute. Thanks |
Co-authored-by: Ricardo Pchevuzinske Katz <[email protected]>
|
thanks @phuhung273 this looks good to me! /lgtm |
k8s-ci-robot
added
the
lgtm
9 participants
What type of PR is this?
/kind test
/area conformance-test
What this PR does / why we need it:
This PR introduces basic same namespace conformance tests for
TLSRoutewith Terminate modeContour test
Which issue(s) this PR fixes:
Relates #3466
Does this PR introduce a user-facing change?: