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. 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-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.
Needs 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 |
We've been somewhat inconsistent about this, but we generally haven't enforced substantial retroactive edits to older GEPs, instead allowing newer GEPs to supercede and prioritizing conformance tests and docs reflecting the current state while allowing older GEPs to stay as historical documents. |
|
yeah but in this case we don't have a TLSRoute GEP at all, and my plan is to have some covering all of the features/conformance that are already in place for TLSRoute |
|
Updated the GEP proposal to add TLSRoute termination: 23c275e |
|
If it's not covered in that summary table, and we have no GEP mentioning it, then we can't just change the docs and call it done. We can add it to the TLSRoute GEP as a new area, but it then needs to be reviewed and debated. Right now I don't see the use case for TLSRoute termination. |
The GEP it was added to was a Memorandum GEP and not highly scrutinized. TLSRoute support for Terminate mode may eventually make it in as Extended, but we agreed in our last community meeting that we should not be using Memorandum GEPs to publish new features. |
| // TLSRouteModeTerminate contains metadata for the TLSRouteModeTerminate feature. | ||
| TLSRouteModeTerminateFeature = Feature{ | ||
| Name: SupportTLSRouteModeTerminate, | ||
| Channel: FeatureChannelExperimental, |
There was a problem hiding this comment.
Does TLSRouteModeTerminateFeature remain in the experimental channel after TLSRouteFeature moves to the standard channel?
There was a problem hiding this comment.
Given GEP-2643: TLSRoute (although not finalized yet) stating Support both Termination and Passthrough TLS modes for TLSRoute, TLSRouteModeTerminateFeature will be in standard channel after TLSRouteFeature moves to standard.
There was a problem hiding this comment.
Please make sure it is marked as Extended in the CRD.
There was a problem hiding this comment.
I'm not really understand your point @candita, should I make any change in CRD ?
| ExtendedFeatures: features.SetsToNamesSet(features.GatewayExtendedFeatures), | ||
| ExtendedFeatures: features.SetsToNamesSet( | ||
| features.GatewayExtendedFeatures, | ||
| features.TLSRouteExtendedFeatures, |
There was a problem hiding this comment.
I could be convinced otherwise, but I don't think TLSRouteExtendedFeatures belongs grouped with GatewayTLSConformanceProfile. We should create a new conformance profile for TLSRoute. Unless we add TLSRouteCoreFeatures to the GatewayTLSConformanceProfile as well.
There was a problem hiding this comment.
isn't it the case of features.SupportTLSRoute ?
| - name: gateway-tlsroute-terminate | ||
| namespace: gateway-conformance-infra | ||
| hostnames: | ||
| - abc.example.com |
There was a problem hiding this comment.
Would you mind using a new hostname here (and creating a new certificate for that hostname). While "abc.example.com" is not necessarily reserved for HTTPRoute, it would be good to make sure TLSRoute certificate handling is working properly with a new hostname and certificate.
There was a problem hiding this comment.
Sure I've updated to use tls.terminate.com
| t.Fatalf("unexpected error finding TLS secret: %v", err) | ||
| } | ||
| t.Run("Simple TLS request matching TLSRoute should reach infra-backend", func(t *testing.T) { | ||
| tls.MakeTLSRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, cPem, keyPem, serverStr, |
There was a problem hiding this comment.
I wonder if it would be a better test if we did not use an HTTP request. Maybe use FTP or MQTT to test this.
There was a problem hiding this comment.
+1. As we are saying about TLS with termination, I would like to see a non HTTP traffic. Maybe MQTT, Postgres, can even be some simpler TCP listener that echos the requests.
It should start as a TLS client, backend non TLS, routing via SNI, check if connection is working.
There was a problem hiding this comment.
Thank you both for your feedback. I've updated to use MQTT but also installed additional library to make connection. Is it ok to do so ? As it has nothing to do with the project and only used for testing.
phuhung273
force-pushed
the
tls-terminate-conformance
branch
2 times, most recently
from
a996737 to
aa021b6
Compare
phuhung273
force-pushed
the
tls-terminate-conformance
branch
from
aa021b6 to
c2daad7
Compare
There was a problem hiding this comment.
I think a different subdomain is good per @candita suggestion, but trying to stick to the canonical example.com testing domain rather than any real domains.
conformance/tests/tlsroute-terminate-simple-same-namespace.yaml
Outdated
Show resolved
Hide resolved
conformance/tests/tlsroute-terminate-simple-same-namespace.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Mike Morris <[email protected]>
| token := client.Connect() | ||
| if token.Wait() && token.Error() != nil { | ||
| t.Fatalf("Connection failed: %v", token.Error()) | ||
| } |
There was a problem hiding this comment.
I'm not an MQTT expert, but don't you have to subscribe and publish a message to test the transport? Doing a Connect doesn't seem enough.
There was a problem hiding this comment.
Ah no worries, I thought connect is enough. Please check latest update for the full round-trip test. Thanks for the feedback.
6 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?: