-
Notifications
You must be signed in to change notification settings - Fork 584
feat: support section name in policies, and add tests for all attachment types. #11272
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
updated outputs to what i think is the correct output (not current output) current failing: - for section name http1, the config needs to be disabled on the listener and enabled on the virtualHost. - section-name-gw-extauth should not be present on tls2 filter chain 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 adds edge-case tests for ext-auth policies and enhances policy targeting by introducing section names, updating how filters are attached per filter chain.
- Add a new test entry for TrafficPolicy edge cases in gateway translator tests
- Extend data structures and logic to carry and match
SectionName
for policy attachments - Refactor
trafficPolicyPluginGwPass
to track per-filter-chain state and disable/enable filters accordingly
Reviewed Changes
Copilot reviewed 55 out of 55 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
internal/kgateway/translator/gateway/gateway_translator_test.go | Added table entry for TrafficPolicy edge cases |
internal/kgateway/setup/testdata/standard/*.yaml | Updated expected YAML to include disabled: true and typedPerFilterConfig per chain |
internal/kgateway/query/httproute.go | Extended UniqueRouteName to accept an optional ruleName |
internal/kgateway/krtcollections/policy.go | Incorporated SectionName in targetRefs indexing and lookup |
internal/kgateway/extensions2/pluginutils/policy.go | Propagate SectionName into IR conversion |
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go | Refactored to use maps keyed by filter-chain and track per-chain provider state |
internal/kgateway/extensions2/plugins/trafficpolicy/extauth_policy_test.go | Updated tests to supply FilterChainName context |
install/helm/kgateway-crds/templates/*.yaml | Added sectionName field in CRD schemas |
api/v1alpha1/*.go | Added SectionName to API types and updated deep copy methods |
api/applyconfiguration/*.go | Added SectionName in apply configurations |
Comments suppressed due to low confidence (3)
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go:261
- [nitpick] The name
ProviderNeededMap
is generic. Consider renaming it to something more descriptive, e.g.FilterChainProviderMap
, to clarify its purpose.
type ProviderNeededMap struct {
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go:507
- [nitpick] The plugin name returned is
routepolicies
but this extension now handles a broader traffic policy. Consider updating it totrafficpolicies
to reflect its intent and avoid confusion.
func (p *TrafficPolicy) Name() string {
internal/kgateway/query/httproute.go:103
- The signature of
UniqueRouteName
now takes an extraruleName
argument. Consider adding unit tests for whenruleName
is non-empty to cover the new branch.
func (r *RouteInfo) UniqueRouteName(ruleIdx, matchIdx int, ruleName string) string {
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go
Outdated
Show resolved
Hide resolved
yaml updates Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
i noticed some multi listener tests are flakey due to ordering; i'll fix that; but otherwise ready for review |
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.
code review is WIP but can you we change the PR title to reflect there are actual functional changes (i.e. it's not just tests) and also i think the PR description is slightly out of order or unintentional
attachedPoliciesSlice := []ir.AttachedPolicies{ | ||
h.gw.AttachedHttpPolicies, | ||
h.attachedPolicies, | ||
} |
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.
Could you pre-merge the policies per GK here so that it is easier to invoke a Merge() on them within the loop
attachedPolicies := ir.AttachedPolicies{
Policies: map[schema.GroupKind][]ir.PolicyAtt{},
}
attachedHttpPolicies.Append(h.gw.AttachedHttpPolicies, h.attachedPolicies)
// TODO: user error - they attached a non http policy | ||
continue | ||
} | ||
for _, pol := range pols { |
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 need to MergePolicies when pass.MergePolicies != nil
?
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
…is if we like these merge semantics Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
internal/kgateway/setup/testdata/standard/extauth-gw-and-route-disable-out.yaml
Show resolved
Hide resolved
internal/kgateway/translator/gateway/testutils/outputs/delegation/traffic_policy.yaml
Show resolved
Hide resolved
Signed-off-by: Yuval Kohavi <[email protected]>
…ent types. (#11272) Signed-off-by: Yuval Kohavi <[email protected]>
…ent types. (kgateway-dev#11272) Signed-off-by: Yuval Kohavi <[email protected]>
Description
Handle attachment edge cases, and allow policies to target section names.
Change Type
Changelog
Additional Notes
Behavior after this PR:
Main implication is that HCM plugins can't attach to http plain text listeners by section name, because the http filter chain is shared. (But does work on tls).
Option to address this is to remove section name from the http listener policy.
Additional changes
getTargetingPolicies
to not return policies without a section name when a section name is requested. instead we use envoy's hierarchy to get the same data plane behaviour.