Fix combineRulesEndpoints to correctly handle allow-all port semantics#205
Fix combineRulesEndpoints to correctly handle allow-all port semantics#205Supreeth095 wants to merge 1 commit intomainfrom
Conversation
When combining rules for the same CIDR, if any rule allows all ports (empty Ports slice), the combined rule should allow all ports. Previously, the function would incorrectly append the empty slice, resulting in policies being more restrictive than intended. Fixes #197
|
Hi @Supreeth095 do we have any idea on when this change might be merged and deployed to the controlplane? |
There was a problem hiding this comment.
Pull request overview
Fixes combineRulesEndpoints so that when combining EndpointInfo entries for the same CIDR, an “allow all ports” rule (empty Ports slice) results in the combined rule allowing all ports, avoiding unintended over-restriction in generated PolicyEndpoints.
Changes:
- Update
combineRulesEndpointsto treat an emptyPortsslice as “allow all ports” and make it dominate during combination. - Add a unit test covering allow-all port semantics for combined rules.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/policyendpoints/manager.go | Adjusts rule-combination logic so empty Ports correctly yields “allow all ports” when merging same-CIDR entries. |
| pkg/policyendpoints/manager_test.go | Adds a test validating allow-all port behavior during endpoint combination. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tempIEP.Ports = append(combinedMap[string(iep.CIDR)].Ports, iep.Ports...) | ||
| } | ||
| // If tempIEP.Ports is already empty (allow all), keep it empty | ||
| tempIEP.Except = append(combinedMap[string(iep.CIDR)].Except, iep.Except...) |
There was a problem hiding this comment.
tempIEP.Except = append(...) effectively unions exception lists when combining entries. For ipBlock semantics, unioning Except is overly restrictive (the union of two ipBlocks with the same CIDR corresponds to intersecting their exception sets, not unioning). If you keep combining, exceptions should be handled in a semantics-preserving way (or avoid combining when Except differs).
| tempIEP.Except = append(combinedMap[string(iep.CIDR)].Except, iep.Except...) | |
| // For ipBlock semantics, combining rules with the same CIDR must intersect their | |
| // exception sets: (CIDR - E1) ∪ (CIDR - E2) == CIDR - (E1 ∩ E2). | |
| existingExcept := sets.NewString(tempIEP.Except...) | |
| newExcept := sets.NewString(iep.Except...) | |
| intersectExcept := existingExcept.Intersection(newExcept) | |
| tempIEP.Except = intersectExcept.UnsortedList() |
| result := combineRulesEndpoints(tt.inputEndpoints) | ||
| assert.Equal(t, 1, len(result), "Should combine to single CIDR entry") | ||
|
|
||
| if tt.wantPortsEmpty { | ||
| assert.Empty(t, result[0].Ports, tt.description) | ||
| } else { | ||
| assert.NotEmpty(t, result[0].Ports, tt.description) | ||
| } |
There was a problem hiding this comment.
The new test only checks whether the combined Ports slice is empty/non-empty. For the "only specific ports" case it would be stronger to assert the exact number of ports (and ideally their protocol/port values) so the test fails if ports are accidentally dropped or duplicated during combination.
| for _, iep := range ingressEndpoints { | ||
| if _, ok := combinedMap[string(iep.CIDR)]; ok { | ||
| tempIEP := combinedMap[string(iep.CIDR)] |
There was a problem hiding this comment.
combineRulesEndpoints currently combines endpoints solely by CIDR. If two rules share a CIDR but have different Except lists, merging them into a single EndpointInfo changes NetworkPolicy semantics (e.g., it can drop an allow that should apply to an excepted range). Consider including Except (normalized/sorted) in the grouping key, or skipping combination when Except differs, so only compatible rules are merged.
When combining rules for the same CIDR, if any rule allows all ports (empty Ports slice), the combined rule should allow all ports.
Previously, the function would incorrectly append the empty slice, resulting in policies being more restrictive than intended.
Fixes #197 #193
What type of PR is this?
Which issue does this PR fix:
What does this PR do / Why do we need it:
If an issue # is not available please add steps to reproduce and the controller logs:
Testing done on this change:
Automation added to e2e:
Will this PR introduce any new dependencies?:
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.