-
Notifications
You must be signed in to change notification settings - Fork 101
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
add support for TLS route #2211
add support for TLS route #2211
Conversation
Is there a need for separate nginx servers for each hostname? For example, if the graph has a L4Route with hostnames "app.example.com" and "cafe.example.com", the code will currently create a map and two nginx servers like this:
Rather it might be better to do it like this
|
having separate servers will help with handling edge cases like below. Let's say an application developer creates this TLSRoute: apiVersion: gateway.networking.k8s.io/v1alpha2
kind: TLSRoute
metadata:
name: first
spec:
parentRefs:
- name: gateway
hostnames:
- app.example.com
rules:
- backendRefs:
- name: my-backend-1
port: 443 Another application developer creates this TLSRoute after the first one was created: apiVersion: gateway.networking.k8s.io/v1alpha2
kind: TLSRoute
metadata:
name: second
spec:
parentRefs:
- name: gateway
hostnames:
- app.example.com
- cafe.example.com
rules:
- backendRefs:
- name: my-backend-2
port: 443 Based on this https://gateway-api.sigs.k8s.io/guides/api-design/#conflicts, NGF needs to be configured Having a server per hostname makes handling this edge case easy |
thanks! |
@pleshakov I'm assuming the second TLSRoute in this example would be created with a different name? Otherwise, the second TLSRoute would just update the first... |
good catch. updated |
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.
@sarthyparty can you also add a test case, or modify an existing test case, to TestBuildConfiguration? Anytime we modify the BuildConfiguration function, we update or add to TestBuildConfiguration. This ensures that we don't have any regressions when we add something new to the config.
abba57e
to
974ab89
Compare
878a2d4
to
3298bb9
Compare
974ab89
to
e6906d4
Compare
e6906d4
to
97acc79
Compare
I'm guessing the |
It will after the rebase and some additional work. @sarthyparty is working on that now |
3a6a527
to
94e7237
Compare
581050c
to
8448d09
Compare
Before merging, can we give this PR a better title? Focus on the fact that it's related to TLSRoute |
Yep, but keep in mind this is just going to be merged to the feature branch. |
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.
leaving a partial review
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.
Just a few more test changes
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.
🚀 🚀
Good Work @sarthyparty |
🚀 🚀 🎉 🎉 |
8448d09
to
6756a2f
Compare
209d517
to
cadacd4
Compare
Proposed changes
Problem: TLSRoute was not supported by NGF.
Solution: Watched for changes to TLSRoutes, added validation and tests, added TLSRoute to graph, and converted Kubernetes TLSRoute spec to nginx config.
Testing: Full code coverage to all code added and enabled one of the TLS Passthrough tests.
Closes #686
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.