-
Notifications
You must be signed in to change notification settings - Fork 530
feat(translator): add support for different hostnames for the listener and an attached TLS certificate #6582
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Jacob Neil Taylor <jacobneiltaylor@canva.com>
d14179f
to
8771b96
Compare
hey @jacobneiltaylor thanks for proactively working on this @guydc was also looking at implementing a fix for this issue, and what we had aligned on was loosing gateway/internal/gatewayapi/tls.go Line 109 in ddf1009
gateway/internal/gatewayapi/tls.go Line 90 in ddf1009
tldr - there are many cases in prod when there may be multiple certRefs and 1 invalid one (thats expired) should not block other certs from being configured |
@arkodg thanks for the context! I did go through that thread prior to opening this PR, and I think the hostname issue and the expiry issue are subtly different.
My reading of that thread (and your TLDR) is that if I have 5 I (and, I believe, @yaelSchechter) want to be able to add a In my PR, I went the route of being able to configure the validation logic of the translator as I consider this the least obtrusive method - someone has to go out of their way to specifically use this behaviour rather than changing the default behaviour of Envoy Gateway. |
sure the approach sgtm, but we cannot support the configuration via an annotation, it would need to be a strongly typed field like
|
I'll look into that alternative implementation path. As a matter of policy, is Envoy Gateway avoiding the use of |
So far, we did not use it, seeing as it's not strongly typed. Same goes for annotations. I'm +1 to Arko's suggestion. |
reg home for this new field, a better home would be in EnvoyProxy gateway/api/v1alpha1/envoyproxy_types.go Line 137 in c653290
frontendTLS / serverTLS since ClientTrafficPolicy holds client specific settings
|
Signed-off-by: Jacob Neil Taylor <jacobneiltaylor@canva.com> Update type Undo change to CTP
Signed-off-by: Jacob Neil Taylor <jacobneiltaylor@canva.com>
ee5d9b7
to
58ca98a
Compare
I've implemented the feature as you suggested, I went with |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6582 +/- ##
==========================================
+ Coverage 70.86% 71.03% +0.17%
==========================================
Files 224 225 +1
Lines 38769 39179 +410
==========================================
+ Hits 27472 27832 +360
- Misses 9708 9737 +29
- Partials 1589 1610 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Jacob Neil Taylor <jacobneiltaylor@canva.com>
Signed-off-by: Jacob Neil Taylor <jacobneiltaylor@canva.com>
What type of PR is this?
Feature enhancement
What this PR does / why we need it:
This PR adds the ability to specify an alternative hostname for validation against provided TLS certificates.
This allows Envoy Gateway to support alternative validation schemes supported by CDNs.
More information on this use case is expressed in the corresponding issue.
Which issue(s) this PR fixes:
Fixes #6442
Release Notes: Yes