-
Notifications
You must be signed in to change notification settings - Fork 631
feat: gateway tls extensions #12917
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: gateway tls extensions #12917
Conversation
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.
Pull Request Overview
This PR adds support for additional TLS extension options in Gateway listeners, allowing users to configure advanced TLS parameters such as cipher suites, ECDH curves, and TLS version constraints through Gateway API annotations.
Key changes:
- Added new TLS-related annotations (cipher suites, ECDH curves, min/max TLS versions, verify subject alt names)
- Refactored TLS option processing into a unified
ApplyTLSExtensionOptionsfunction - Extended the IR
TlsBundlestruct with new TLS configuration fields
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| api/annotations/gateway.go | Added new annotation constants for TLS extensions (cipher suites, ECDH curves, TLS versions, subject alt names) |
| pkg/pluginsdk/ir/gw2.go | Extended TlsBundle struct with new fields for TLS parameters |
| internal/kgateway/translator/sslutils/ssl_utils.go | Implemented TLS extension option processing functions and validation logic |
| internal/kgateway/translator/sslutils/ssl_utils_test.go | Added comprehensive unit tests for TLS extension option handling |
| internal/kgateway/translator/listener/gateway_listener_translator.go | Refactored to use new unified TLS option processing function |
| internal/kgateway/translator/irtranslator/fc.go | Applied TLS parameters to Envoy configuration |
| internal/kgateway/krtcollections/policy.go | Fixed type conversion for annotation key access |
| internal/kgateway/translator/gateway/gateway_translator_test.go | Updated test name to reflect broader TLS options support |
| internal/kgateway/translator/gateway/testutils/outputs/gateway-only/tls.yaml | Updated expected output to include TLS parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func ApplyCipherSuites(in string, out *ir.TlsBundle) error { | ||
| cipherSuites := strings.Split(in, ",") | ||
| out.CipherSuites = cipherSuites | ||
| return nil | ||
| } |
Copilot
AI
Nov 18, 2025
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.
The cipher suites are accepted without any validation. Consider adding validation to ensure only secure and supported cipher suites are configured. Accepting arbitrary cipher suite values could allow users to configure weak or insecure ciphers, or non-existent cipher suites that would cause Envoy configuration errors.
d8b8a7a to
91836c4
Compare
Signed-off-by: omar <[email protected]>
91836c4 to
8ba274f
Compare
sheidkamp
left a comment
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.
A few comments, and the copilot comments are worth looking at.
Would like to see e2e tests, but can be done in a followup.
| // VerifySubjectAltNames is the annotation key used to set the verify subject alt names for a TLS listener. | ||
| // The value is a comma separated list of subject alt names, e.g "example.com,www.example.com". | ||
| // Use in the TLS options field of a TLS listener. | ||
| VerifySubjectAltNames gwv1.AnnotationKey = "kgateway.dev/verify-subject-alt-names" |
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.
These are the annotations we are using for GW API with GG1:
GatewaySslCipherSuites = GatewaySslOptionsPrefix + "/cipher-suites"
GatewaySslEcdhCurves = GatewaySslOptionsPrefix + "/ecdh-curves"
GatewaySslMinimumTlsVersion = GatewaySslOptionsPrefix + "/minimum-tls-version"
GatewaySslMaximumTlsVersion = GatewaySslOptionsPrefix + "/maximum-tls-version"
GatewaySslOneWayTls = GatewaySslOptionsPrefix + "/one-way-tls"
GatewaySslVerifySubjectAltName = GatewaySslOptionsPrefix + "/verify-subject-alt-name"
The differences are (GG1/new GG2):
- minimum-tls-version/min-tls-version
- maximum-tls-version/max-tls-version
- verify-subject-alt-name/verify-subject-alt-names
The prefix will be changing from "gateway.gloo.solo.io/ssl" to "kgateway.dev", but do we want to keep the leaf names consistent? (although verify-subject-alt-names is a better annotation than verify-subject-alt-name
| if len(ssl.EcdhCurves) > 0 { | ||
| common.TlsParams.EcdhCurves = ssl.EcdhCurves | ||
| } | ||
| // TODO: add verify subject alt names (validation context) |
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.
Are we going to do this as part of this PR?
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.
No, because that involves a larger change.
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.
Should we either remove VerifySubjectAltNames from gateway.go or at least mark it as not implemented?
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.
marked as TODO
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.
created #12955 to track
internal/kgateway/translator/gateway/testutils/inputs/gateway-only/tls-alpn.yaml
Show resolved
Hide resolved
| // It is used to set the per connection buffer limit for the gateway. | ||
| // The value is a string representing the limit, e.g "64Ki". | ||
| // The limit is applied to all listeners in the gateway. | ||
| PerConnectionBufferLimit gwv1.AnnotationKey = "kgateway.dev/per-connection-buffer-limit" |
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.
maybe it's already been discussed, but why are these all annotations rather than adding them to the trafficpolicy (or other) api?
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.
PerConnectionBufferLimit is existing. Basically we didn't want to add a CRD for this option and it didn't fit anywhere. Likewise for the TLS extensions, but in this case using the gateway API tls options field instead of the actual annotations.
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
sheidkamp
left a comment
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.
One question and a small update
internal/kgateway/translator/listener/gateway_listener_translator.go
Outdated
Show resolved
Hide resolved
api/annotations/gateway.go
Outdated
| // VerifySubjectAltNames is the annotation key used to set the verify subject alt names for a TLS listener. | ||
| // The value is a comma separated list of subject alt names, e.g "example.com,www.example.com". | ||
| // Use in the TLS options field of a TLS listener. | ||
| // TODO: implement. |
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.
Can we add #12955 to the comment (and other similar TODOs)?
Description
Allow users to configure TLS parameters for TLS listeners on Gateway using the options field.
API changes:
This PR adds the following tls options:
kgateway.dev/cipher-suites: comma separated list of cipher suiteskgateway.dev/ecdh-curves: comma separated list of ecdh curveskgateway.dev/min-tls-version: minimum TLS version string, e.g. "1.2"kgateway.dev/max-tls-version: maximum TLS version string, e.g. "1.3"kgateway.dev/verify-subject-alt-names: comma separated list of SANs [placeholder, to be implemented]Change Type
/kind feature
Changelog
Additional Notes
largely referenced from solo-io#10553
resolves #12933
resolves #11232