-
Notifications
You must be signed in to change notification settings - Fork 631
feat: initial proxy protocol support #12979
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
Conversation
Signed-off-by: Yuval Kohavi <[email protected]>
| // +kubebuilder:metadata:labels="gateway.networking.k8s.io/policy=Direct" | ||
| // ListenerPolicy is used for configuring Envoy listener-level settings that apply to all protocol types (HTTP, HTTPS, TCP, TLS). | ||
| // This includes listener filters such as PROXY protocol support. | ||
| // These policies can be applied per Gateway (and in the future maybe per listener port, to target specific ports). |
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 policies can be applied per Gateway (and in the future maybe per listener port, to target specific ports). | |
| // These policies can be applied per Gateway. |
remove or add a TODO?
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
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 introduces initial PROXY protocol support for kgateway by adding a new ListenerPolicy CRD. The implementation allows users to enable PROXY protocol on Gateway listeners, which is commonly used when kgateway sits behind a load balancer that needs to preserve client IP information.
Key Changes:
- Introduces a new
ListenerPolicyCRD withProxyProtocolconfiguration - Implements listener-level plugin that adds Envoy PROXY protocol filter
- Adds comprehensive test coverage including unit tests, translator tests, and e2e tests
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
api/v1alpha1/listener_policy_types.go |
Defines the new ListenerPolicy CRD and ProxyProtocolConfig types |
internal/kgateway/extensions2/plugins/listenerpolicy/listener_plugin.go |
Implements the listener policy plugin that applies PROXY protocol configuration to Envoy listeners |
internal/kgateway/extensions2/registry/registry.go |
Registers the new listener policy plugin |
internal/kgateway/wellknown/kgw.go |
Adds GVK/GVR definitions for ListenerPolicy |
pkg/client/clientset/versioned/typed/api/v1alpha1/listenerpolicy.go |
Generated client code for ListenerPolicy CRD |
install/helm/kgateway-crds/templates/gateway.kgateway.dev_listenerpolicies.yaml |
CRD definition for ListenerPolicy |
install/helm/kgateway/templates/role.yaml |
Adds RBAC permissions for listenerpolicies resource |
test/e2e/features/listener_policy_proxy_protocol/suite.go |
E2E test suite validating proxy protocol behavior |
test/e2e/features/listener_policy_proxy_protocol/testdata/setup.yaml |
Test manifests for nginx backend and curl client |
internal/kgateway/translator/gateway/gateway_translator_test.go |
Unit tests for HTTP, HTTPS, and TCP proxy protocol scenarios |
pkg/utils/requestutils/curl/request.go |
Adds support for PROXY protocol header in curl utility |
Comments suppressed due to low confidence (1)
internal/kgateway/translator/gateway/gateway_translator_test.go:2061
- This test function appears to be debug/development code that should be removed before merging. The function name "TestSomethin" is incomplete and the test doesn't have a clear purpose in the context of this PR. Additionally, line 2048 has a bug where
BBcliis typed asHTTPListenerPolicybut useswellknown.ListenerPolicyGVRinstead ofwellknown.HTTPListenerPolicyGVR.
settingOpts := []translatortest.SettingsOpts{
func(s *apisettings.Settings) {
s.DiscoveryNamespaceSelectors = cfgJSON
s.EnableExperimentalGatewayAPIFeatures = true
},
}
translatortest.TestTranslation(t, ctx, inputFiles, expectedOutputFile, gwNN, settingOpts...)
}
t.Run("Select all resources", func(t *testing.T) {
test(t, `[
{
"matchExpressions": [
{
"key": "kubernetes.io/metadata.name",
"operator": "In",
"values": [
"infra"
]
}
]
},
{
"matchLabels": {
"app": "a"
}
}
]`, "base.yaml", "base_select_all.yaml")
})
t.Run("Select all resources; AND matchExpressions and matchLabels", func(t *testing.T) {
test(t, `[
{
"matchExpressions": [
{
"key": "kubernetes.io/metadata.name",
"operator": "In",
"values": [
"infra"
]
}
]
},
{
"matchExpressions": [
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
lgadban
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.
I am onboard with the new ListenerPolicy CRD for this and the code mostly LGTM.
Just a few minor comments on API docs etc
| // ListenerPolicySpec defines the desired state of a listener policy. | ||
| type ListenerPolicySpec struct { | ||
| // TargetRefs specifies the target resources by reference to attach the policy to. | ||
| // Supports Gateway and XListenerSet resources. Use sectionName to target specific listeners by port. |
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.
This comment is incorrect right? Only Gateway attachment and no sectionName support
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.
right - the comment is not right - i think i fixed the CEL validation but missed that one
| // +kubebuilder:metadata:labels="gateway.networking.k8s.io/policy=Direct" | ||
| // ListenerPolicy is used for configuring Envoy listener-level settings that apply to all protocol types (HTTP, HTTPS, TCP, TLS). | ||
| // This includes features such as PROXY protocol support. | ||
| // These policies can targetRef Gateway objects. |
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 policies can targetRef Gateway objects. | |
| // These policies can only target `Gateway` objects. |
| // +kubebuilder:subresource:status | ||
| // +kubebuilder:metadata:labels="gateway.networking.k8s.io/policy=Direct" | ||
| // ListenerPolicy is used for configuring Envoy listener-level settings that apply to all protocol types (HTTP, HTTPS, TCP, TLS). | ||
| // This includes features such as PROXY protocol support. |
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.
do you think it's worth including this in the comments?
we'll need to constantly be updating it etc?
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.
i can remove
| return d.ct | ||
| } | ||
|
|
||
| func (d *listenerPolicy) Equals(in any) bool { |
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.
any reason to not compare ct in here?
I am also surprised the krtEquals linter is not complaining that it is omitted
cc @timflannagan
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.
it's not needed in equals because it doesn't impact policy. it's used to determine ordering if there are multiple policies
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.
hmm actually that merging thing can impact policy. ok i'll add
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.
I am also surprised the krtEquals linter is not complaining that it is omitted
I think creating an issue to investigate further is worthwhile imo.
| logger.Debug("applying to listener", "listener", out.Name, "policyType", fmt.Sprintf("%T", pCtx.Policy)) | ||
| pol, ok := pCtx.Policy.(*listenerPolicy) | ||
| if !ok || pol == nil { | ||
| logger.Debug("policy is not listenerPolicy type or is nil", "ok", ok, "pol", pol) |
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.
is this a realistic concern? if so, maybe this should a warn instead
| // +kubebuilder:validation:XValidation:rule="self.all(r, r.kind == 'Gateway' && (!has(r.group) || r.group == 'gateway.networking.k8s.io'))",message="targetRefs may only reference Gateway resource" | ||
| TargetRefs []LocalPolicyTargetReference `json:"targetRefs,omitempty"` | ||
|
|
||
| // TargetSelectors specifies the target selectors to select resources to attach the policy to. |
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.
| // TargetSelectors specifies the target selectors to select resources to attach the policy to. | |
| // TargetSelectors specifies the target selectors to select `Gateway` resources to attach the policy to. |
| }) | ||
| } | ||
|
|
||
| func TestSomethin(t *testing.T) { |
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.
what is this thing? 😆
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.
ah whops! i added that to debug something and forgot to remove; i'll delete
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.
was this just local formatting 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.
i'll check maybe i refreshed goldens on other tests by mistake
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.
was this just local formatting changes?
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
| go.uber.org/zap v1.27.0 | ||
| golang.org/x/exp v0.0.0-20251017212417-90e834f514db | ||
| golang.org/x/net v0.46.0 | ||
| golang.org/x/net v0.46.0 // indirect |
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.
We should move this outside of the direct dependency require block imo.
| // Marshal to Any | ||
| proxyProtocolAny, err := utils.MessageToAny(proxyProtocolConfig) | ||
| if err != nil { | ||
| logger.Error("failed to marshal proxy protocol config", | ||
| "error", err) | ||
| } |
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.
We could also return this error and propagate to the IR's Error field. AFAIK, we don't handle errors on the IR for the HLP IR though.
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.
it should never happen, i thought to panic here but ended up doing the log. maybe i should add a panic
| envoyclusterv3 "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3" | ||
| envoylistenerv3 "github.com/envoyproxy/go-control-plane/envoy/config/listener/v3" | ||
| envoyroutev3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" | ||
| "golang.org/x/net/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.
😆 nice catch.
| func NewTestingSuite(ctx context.Context, testInst *e2e.TestInstallation) suite.TestingSuite { | ||
| return &testingSuite{ | ||
| ctx: ctx, | ||
| testInstallation: testInst, | ||
| } | ||
| } |
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.
We've been trying to use base.BaseTestingSuite as much as possible (or at least there's been a lot of movement on standardizing on that). Something to teach AGENTS.md at some point too.
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.
I think curl and nginx are both available as testdefault manifests. We could use those instead of defining net new manifests as much as possible.
Signed-off-by: Yuval Kohavi <[email protected]>
fixes #11161
Description
Add proxy protocol support to listeners
Change Type
Changelog
Addition Notes:
In the future we might make HTTPListenerPolicy and
httpfield in this policy