Skip to content

Fix incorrect default deny log when NetworkPolicy is chunked #488

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.

Sign up for GitHub

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

Open
emilyhuaa wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix incorrect default deny log when NetworkPolicy is chunked #488

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30ca825
Fix incorrect default deny when NetworkPolicy is chunked
emilyhuaa Nov 19, 2025
dc8b572
Merge branch 'main' into fix-chunked-default-deny
emilyhuaa Nov 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions controllers/policyendpoints_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,7 @@ func (r *PolicyEndpointsReconciler) deriveIngressAndEgressFirewallRules(ctx cont
var ingressRules, egressRules []fwrp.EbpfFirewallRules
isIngressIsolated, isEgressIsolated := false, false
currentPE := &policyk8sawsv1.PolicyEndpoint{}
var lastPE *policyk8sawsv1.PolicyEndpoint

if policyEndpointList, ok := r.podIdentifierToPolicyEndpointMap.Load(podIdentifier); ok {
log().Infof("Total number of PolicyEndpoint resources for podIdentifier %s are %d", podIdentifier, len(policyEndpointList.([]string)))
Expand Down Expand Up @@ -404,10 +405,14 @@ func (r *PolicyEndpointsReconciler) deriveIngressAndEgressFirewallRules(ctx cont
L4Info: endPointInfo.Ports,
})
}
log().Infof("Total no.of - ingressRules %d egressRules %d", len(ingressRules), len(egressRules))
ingressIsolated, egressIsolated := r.deriveDefaultPodIsolation(ctx, currentPE, len(ingressRules), len(egressRules))
isIngressIsolated = isIngressIsolated || ingressIsolated
isEgressIsolated = isEgressIsolated || egressIsolated
lastPE = currentPE
}
log().Infof("Total no.of - ingressRules %d egressRules %d", len(ingressRules), len(egressRules))
// Check isolation after aggregating all rules from all PolicyEndpoints
Copy link
Contributor

@viveksb007 viveksb007 Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what issue was this creating?

final decision will be made once we iterate on all the PE for a PodIdentifier. It seems current behavior of doing isIngressIsolated = isIngressIsolated || ingressIsolated and this updated change, both will result in same final behavior.

Copy link
Author

@emilyhuaa emilyhuaa Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue was the log message. deriveDefaultPodIsolation() logs "Default Deny enabled on Egress" when called with egressRulesCount=0 inside the loop, even though other chunks have egress rules. The final boolean values are correct due to ||, but the premature log message was confusing customers running in standard mode who don't expect default deny for policies with explicit rules.

Copy link
Contributor

@viveksb007 viveksb007 Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, yeah log message fix makes sense. CR title Fix incorrect default deny when NetworkPolicy is chunked threw off, probably edit it to Fix incorrect Default Deny log when NP is chunked

Copy link
Contributor

@viveksb007 viveksb007 Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the actual issue could be when other Chunks haven't made it to the agent yet (as those are still getting processed by NPC), in that case Default Deny will still show up.

Copy link
Author

@emilyhuaa emilyhuaa Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, this fixes the case where all chunks exist but are being aggregated by the agent. would it be better to check if the PolicyEndpoint name indicates it's part of a chunked set (e.g. contains a chunk suffix) and suppress the log message for chunked policies? or is the sequential arrival case acceptable since it's transient during controller processing?

Copy link
Contributor

@viveksb007 viveksb007 Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

today there is no way to check if PolicyEndpoint has any siblings directly from same NetworkPolicy just by looking at its name or indirectly from other NetworkPolicies.

Doing chunk set (based on some deterministic chunk naming) makes sense from a single Network Policy perspective but pods can be targeted by multiple Network Policies and in this loop, we are iterating all PolicyEndpoints belonging to all Network Policies targeting that pod and then making the isolation decision.

if lastPE != nil {
ingressIsolated, egressIsolated := r.deriveDefaultPodIsolation(ctx, lastPE, len(ingressRules), len(egressRules))
isIngressIsolated = ingressIsolated
isEgressIsolated = egressIsolated
}
}
if len(ingressRules) > 0 {
Expand Down
Loading