Skip to content
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

feat(*) add support for TLSRoute #2476

Merged
merged 2 commits into from
May 13, 2022
Merged

feat(*) add support for TLSRoute #2476

merged 2 commits into from
May 13, 2022

Conversation

rainest
Copy link
Contributor

@rainest rainest commented May 10, 2022

What this PR does / why we need it:
It's TLSRoute 🤷

Basically the same as the other non-HTTP routes, but it populates route.snis instead of route.destination.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #2088

Special notes for your reviewer:

  • This does not implement loading certificates, which aren't actually loaded via TLSRoute. For now, TLSRoutes will always use the default proxy certificate. TLSRoute code is not expected to change when we add support for Gateway.CertificateRefs, as we'll still just add SNI+certificate pairs independent of route creation: Implement Gateway certificateRef #2474
  • Passthrough is not implemented per the spec, though it is available by setting the protocols annotation to tls_passthrough manually. The spec handles it in the listener, which raises some interesting questions. TLSRoute code will change to pull info from ParentRefs when we implement that for Listeners: Implement Listener Hostnames and associated TLS mode on matching TLSRoutes #2475

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@rainest rainest requested a review from a team as a code owner May 10, 2022 00:03
@rainest rainest temporarily deployed to Configure ci May 10, 2022 00:03 Inactive
@rainest rainest temporarily deployed to Configure ci May 10, 2022 00:03 Inactive
@rainest rainest temporarily deployed to Configure ci May 10, 2022 00:04 Inactive
@rainest rainest temporarily deployed to Configure ci May 10, 2022 00:06 Inactive
@rainest
Copy link
Contributor Author

rainest commented May 10, 2022

Apparent test race condition if TCPRoute and TLSRoute tests run concurrently, guessing from:

2022-05-10T00:22:44.1046343Z === CONT  TestTCPRouteExample
2022-05-10T00:22:44.1047641Z     examples_test.go:160: 
2022-05-10T00:22:44.1048898Z         	Error Trace:	examples_test.go:160
2022-05-10T00:22:44.1050080Z         	Error:      	Condition never satisfied
2022-05-10T00:22:44.1050732Z         	Test:       	TestTCPRouteExample
2022-05-10T00:22:44.1051427Z     examples_test.go:155: deleting tcproute example
2022-05-10T00:22:44.1974380Z     examples_test.go:156: 
2022-05-10T00:22:44.1975142Z         	Error Trace:	examples_test.go:156
2022-05-10T00:22:44.1976048Z         	            				panic.go:500
2022-05-10T00:22:44.1976832Z         	            				testing.go:864
2022-05-10T00:22:44.1977767Z         	            				examples_test.go:160
2022-05-10T00:22:44.1978308Z         	Error:      	Received unexpected error:
2022-05-10T00:22:44.1979453Z         	            	failed to deploy YAML STDOUT=(tcproute.gateway.networking.k8s.io "tcpecho" deleted
2022-05-10T00:22:44.1980790Z         	            	) STDERR=(Error from server (NotFound): error when deleting "STDIN": deployments.apps "tcpecho" not found
2022-05-10T00:22:44.1982142Z         	            	Error from server (NotFound): error when deleting "STDIN": services "tcpecho" not found
2022-05-10T00:22:44.1983636Z         	            	Error from server (NotFound): error when deleting "STDIN": gatewayclasses.gateway.networking.k8s.io "kong" not found
2022-05-10T00:22:44.2031512Z         	            	Error from server (NotFound): error when deleting "STDIN": gateways.gateway.networking.k8s.io "kong" not found
2022-05-10T00:22:44.2032181Z         	            	): exit status 1
2022-05-10T00:22:44.2032604Z         	Test:       	TestTCPRouteExample
2022-05-10T00:22:44.2033550Z --- FAIL: TestTCPRouteExample (180.39s)

@rainest rainest marked this pull request as draft May 10, 2022 00:58
@rainest rainest temporarily deployed to Configure ci May 10, 2022 20:45 Inactive
Add TLSRoute controller.

Add TLSRoute store functions.

Add TLSRoute translation to the parser.

Add TLSRoute example and test.

Add TLSRoute integration tests.

Fix copy/paste error in TCPRoute fakestore and add missing TCPRoute
fakestore test.

Disable parallel execution for example tests that had it. As these tests
do not use isolated namespaces and use similar resources (namely
Gateways and GatewayClasses) they can interfere with each other.
@rainest rainest temporarily deployed to Configure ci May 10, 2022 21:16 Inactive
@rainest rainest temporarily deployed to Configure ci May 10, 2022 22:07 Inactive
@rainest
Copy link
Contributor Author

rainest commented May 10, 2022

My initial read of "oh, the TCPRoute example test is parallel, the rest should be too! most tests are!" was backwards. None should be parallel. Those tests are unique in that they all operate in default and generally use similar Gateway/GatewayClass resources, and will delete and/or modify "shared" resources if run in parallel.

Also, apparently I've hit the threshold for tests being close enough to their global timeout that they actually hit it.

Enterprise has started failing due to a timeout often with no obvious
cause. Bumping to 25m.
@rainest rainest temporarily deployed to Configure ci May 10, 2022 22:35 Inactive
@rainest rainest temporarily deployed to Configure ci May 10, 2022 22:35 Inactive
@rainest rainest temporarily deployed to Configure ci May 10, 2022 22:57 Inactive
@rainest rainest marked this pull request as ready for review May 10, 2022 23:04
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So after reading this over my take is that this really isn't a complete implementation without certificateRefs, but that's OK. It looks like once certificateRefs are implemented where' in good shape with minimal changes to get TLSRoute set up to work with custom certs. LGTM and we can continue to iterate from here 👍

Makefile Show resolved Hide resolved
@rainest rainest enabled auto-merge (rebase) May 13, 2022 16:28
@rainest rainest merged commit 5414a49 into main May 13, 2022
@rainest rainest deleted the feat/tlsroute branch May 13, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TLSRoute Support
2 participants