-
Notifications
You must be signed in to change notification settings - Fork 630
Add CSRF policy support to TrafficPolicy #11302
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: Yossi Mesika <[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
Adds support for a new CSRF policy on TrafficPolicy, translating it into Envoy’s CSRF filter and providing end-to-end tests.
- Introduces a new
csrffeature package with manifests and e2e tests. - Extends the API (types, deepcopy, applyconfig, CRD) to include
CsrfPolicy. - Updates the traffic policy plugin to translate and insert the Envoy CSRF filter.
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/kubernetes/e2e/tests/kgateway_tests.go | Imports and registers the new CSRF testing suite |
| test/kubernetes/e2e/features/csrf/{types.go,suite.go,testdata/*.yaml} | New CSRF test suite, helper types, and YAML manifests |
| internal/kgateway/setup/testdata/standard/{csrf-.yaml,csrf--out.yaml} | Standard input/output test data for CSRF route & gateway |
| internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go | Translation and filter insertion logic for CSRF |
| api/v1alpha1/traffic_policy_types.go | Adds CsrfPolicy field and type definition |
| install/helm/kgateway-crds/templates/gateway.kgateway.dev_trafficpolicies.yaml | Updates CRD schema to include CSRF settings |
Comments suppressed due to low confidence (2)
test/kubernetes/e2e/features/csrf/suite.go:99
- There’s no test for the shadow-only mode (
PercentageShadowed). Add a case wherePercentageEnabledis 0 andPercentageShadowedis non-zero to verify CSRF shadow behavior.
func (s *testingSuite) TestRouteLevelCSRF() {
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go:1262
- The code references envoy_core_v3.RuntimeFractionalPercent but the core v3 package isn’t imported. Please add
github.com/envoyproxy/go-control-plane/envoy/config/core/v3aliased asenvoy_core_v3.
FilterEnabled: &envoy_core_v3.RuntimeFractionalPercent{
install/helm/kgateway-crds/templates/gateway.kgateway.dev_trafficpolicies.yaml
Show resolved
Hide resolved
Signed-off-by: Yossi Mesika <[email protected]>
Signed-off-by: Yossi Mesika <[email protected]>
Signed-off-by: Yossi Mesika <[email protected]>
Signed-off-by: Yossi Mesika <[email protected]>
Signed-off-by: Yossi Mesika <[email protected]>
| } | ||
| if _, ok := p.csrfInChain[fcn]; !ok { | ||
| p.csrfInChain[fcn] = &envoy_csrf_v3.CsrfPolicy{ | ||
| FilterEnabled: &envoy_core_v3.RuntimeFractionalPercent{ |
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.
Have we started a discussion on how to handle runtime keys?
I think we may want a doc or at least a discussion in a community meeting or something to codify it.
Namely I state this as runtimefractionals WITHOUT a key have bitten previous implementations pretty hard and I think we need to decide early if we want to try to plumb the runtime key naming in interesting ways
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.
@nfuden This is "empty" filter being added to filter chain just so that the inner policies will be applied. The typed filter config do have runtime keys (here and here). (I will push a commit that changes the keys to envoy.csrf.filter_enabled and envoy.csrf.shadow_enabled though).
As for the generic approach, I can open an issue for this to track and discuss this over it.
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go
Outdated
Show resolved
Hide resolved
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go
Outdated
Show resolved
Hide resolved
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Yossi Mesika <[email protected]>
dhaifley
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 not a write access contributor, but I have reviewed this PR and verified the end to end test appears valid and passes in my local environment. Given what I know of it, the CSRF policy implementation looks good.
Signed-off-by: Yossi Mesika <[email protected]>
Signed-off-by: Yossi Mesika <[email protected]>
Signed-off-by: Yossi Mesika <[email protected]>
sam-heilbron
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.
lgtm!
| // Example: abc matches the value xyz.abc | ||
| Suffix string `json:"suffix,omitempty"` | ||
|
|
||
| // The input string must match the Google RE2 regular expression specified here. |
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.
"must match" to me implies that some behavior will occur if it does not. Could/Should we clarify that?
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 you please help with phrasing it in clearer way?
I took this from: https://www.envoyproxy.io/docs/envoy/latest/api-v3/type/matcher/v3/string.proto#envoy-v3-api-msg-type-matcher-v3-stringmatcher
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.
+1 to be consistent with Envoy stringmatcher.
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go
Show resolved
Hide resolved
internal/kgateway/extensions2/plugins/trafficpolicy/csrf_policy.go
Outdated
Show resolved
Hide resolved
internal/kgateway/extensions2/plugins/trafficpolicy/csrf_policy.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Yossi Mesika <[email protected]>
Signed-off-by: Yossi Mesika <[email protected]>
Signed-off-by: Yossi Mesika <[email protected]>
Description
PR adds support for configuring a CSRF policy in the
TrafficPolicy.This translates into Envoy's CSRF filter.
Fixes #11186
Change Type
/kind new_feature
Changelog
Additional Notes