fix(networkpolicy): skip neighbor ports without a port number

[MrBeldum]

Overview

generateEgressRule and generateIngressRule dereferenced NetworkPort.Port (an optional *int32) without a nil check:

portInt32 := networkPort.Port
key := PortProtocolKey{Port: *portInt32, Protocol: protocol}

One ContainerProfile neighbor with a nil port was enough to panic GenerateNetworkPolicy. Because GeneratedNetworkPolicyStorage.GetList aggregates every workload, each cluster-wide LIST generatednetworkpolicies then returned 503, and clients that enumerate all resource types (Argo CD's cluster cache, for example) marked the whole cluster as failed.

This PR skips such port entries in both functions, the same way mergeIngressRulesByPorts / mergeEgressRulesByPorts already skip ports with a nil Port. Valid ports on the same neighbor are still emitted.

How to Test

go test ./pkg/apis/softwarecomposition/networkpolicy/v2/

The new TestGenerateRules_NilPortIsSkipped passes a neighbor with one nil-port entry and one TCP/80 entry to both generators, and checks that nothing panics and only TCP/80 is emitted. Without the fix it panics.

Related issues/PRs:

• Resolved storage: nil pointer in networkpolicy/v2 generateEgressRule when ContainerProfile egress port is nil -> GeneratedNetworkPolicy list returns 503 for the whole cluster #418

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 13f16f7f-70dd-43e3-a12b-8a1c03616698

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[matthyx]

Request changes: the panic fix is needed, but one security regression remains at pkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy.go:424-428 (also ingress 523-527).

P1 / high: do not turn invalid-only port observations into unrestricted selector rules. For a neighbor with PodSelector: {app: db} (or a namespace selector) and Ports: [{Protocol: TCP, Port: nil}], the new guard removes every port but retains the peer. GenerateNetworkPolicy keeps this rule because the peer list is nonempty (89/114), and both merge functions preserve selector rules unchanged (185/274). Empty output ports mean all ports, as documented in v1beta1/networkpolicy.go:75-81,96-102 and the Kubernetes policy contract. Applying that generated policy permits all ports/protocols for those peers; an additional valid rule cannot narrow that allowance. IP-only neighbors are instead dropped during merging.

Please omit/reject a neighbor when it supplied ports but filtering leaves no valid ports, while retaining valid ports from mixed entries and preserving deliberately empty-port behavior. Add full GenerateNetworkPolicy regression coverage for nil-only pod/namespace selector neighbors in both directions. The added nil-plus-80 helper test misses this case.

Reviewed head c9657cc6d2f4a26d373fdc561115841ecdfe3501 against main at ea2ae7b7910980d20f80d8c613bcbddb622398bd; both were rechecked unchanged immediately before submission. The target still dereferences nil ports, consistent with #418; excluding the resource from Argo CD only works around the failure. The change is small, adds no dependencies, and preserves valid entries, but is not ready to merge due to the blocker above. Independent correctness and architecture reviews agree.

Validation: go test ./pkg/apis/softwarecomposition/networkpolicy/v2/ -count=1 passed in a disposable Go 1.26.3 container with a read-only repository mount, no credentials, dropped capabilities and resource limits. A separate local TestReviewNilOnlySelector, run with -run TestReviewNilOnlySelector -count=1 -v, failed as expected: full generation emitted ingress and egress selector rules with Ports:[]. git diff --check origin/main review-424 passed. DCO and GitGuardian passed for the head; CodeRabbit reported skipped review. No unit-test CI result is listed. Full build/lint, a before-fix runtime test, live HTTP LIST/Argo CD behavior, and the upstream producer of malformed records were not validated.

History: searches across open/closed/merged PRs and issues used networkpolicy, nil port, nil, NetworkPort, generator symbols, panic/503, and generatednetworkpolicies (up to 100 results/query). No duplicate fix surfaced; this is bounded keyword search, not proof of absence. #138 fixed nil handling in egress merging, not these generators; unlike the description's claim, current egress merging defaults nil values through NewPortProtocolKey. #348 added CIDR collapsing/deduplication, not this guard. #395 concerns metadata-only LIST behavior, confirmed fixed; #419 concerns resourceVersion, a separate Argo CD symptom. Closed #85 was explicitly superseded by #86, not rejected. Closed #79 proposed concurrency; no explicit maintainer rejection rationale was found in its discussion, and its race concerns do not apply to this synchronous guard. No previous rejection of this nil-skip approach was found. Existing PR reviews/inline comments were empty and rechecked before posting.

[matthyx]

Re-reviewed d3d1fb1a6ef863987077f8f9f445dec07a482258 against main at ea2ae7b7910980d20f80d8c613bcbddb622398bd. The previous security blocker is resolved: both generators now return an empty rule and references when a supplied port list contains only nil entries, so the caller drops the neighbor. Mixed valid entries and deliberately empty selector-port behavior are preserved. New full-generation tests cover pod/namespace selectors and both directions. No remaining code correctness or architectural blocker found in this two-file change; adding diagnostics for discarded malformed observations would be optional.

Remaining merge blocker: required DCO check. The DCO result is ACTION_REQUIRED: commit d3d1fb1 lacks its author's Signed-off-by line. main requires DCO and GitGuardian; GitGuardian passed. Please complete the project's DCO remediation for that commit. I cannot approve while this required check remains unsatisfied.

The fix remains needed for #418, and the related-history conclusions from my earlier review are unchanged. Refreshed bounded nil port PR and NetworkPort issue searches found no new duplicate/superseding fix (100-result limits; not exhaustive).

Validation in an isolated, credential-free Go 1.26.3 container: go test ./pkg/apis/softwarecomposition/networkpolicy/v2/ -count=1 and go vet ./pkg/apis/softwarecomposition/networkpolicy/v2/ passed on the updated head. Replacing only the generator implementation with target main and rerunning go test ./pkg/apis/softwarecomposition/networkpolicy/v2/ -run TestGenerateRules_NilPortIsSkipped -count=1 failed with nil-pointer panics in both directions, confirming a before/after regression. git diff --check origin/main review-424-new passed. Full repository build/lint and live LIST/Argo CD behavior were not run; no unit-test CI result is listed. Head, target, open/non-draft state and new review context were rechecked before submission.

Verdict: request changes solely for the required DCO remediation; the earlier code blocker is fixed.