feat: add allowICMP annotation for port-filter mode

[kvaps]

Summary

Follow-up to #11. In port-filter mode the previous PR drops all ICMP toward the backend pod IP, which breaks ping, PMTU discovery (ICMP "fragmentation needed"), and ICMP unreachable signalling. This adds an opt-in annotation to accept ICMP for services that need it.

• New annotation networking.cozystack.io/allowICMP: "true" (default false). Only meaningful in port-filter mode (wholeIP: "false").
• New nft set icmp_allowed_pods and a rule in port_filter placed between the ct established,related accept and the per-port drop: ip daddr @icmp_allowed_pods meta l4proto icmp accept.
• New ProxyProcessor methods EnsureICMPAllow / DeleteICMPAllow / CleanupICMPAllow, mirroring the port-filter API shape.
• Controller wiring consolidated into reconcilePortFilter / clearPortFilter helpers — fewer scattered if/else blocks for both port-filter and ICMP state.
• Unit test for the new allowICMP predicate.
• README documents the annotation and updates the Datapath section.

Why per-pod set, not per-rule

Same reason as filtered_pods/allowed_ports: the chain runs at priority filter (0), after ingress_dnat has rewritten daddr from LB IP to pod IP. Keying on pod IP keeps the chain rule count constant and the per-service state in nft sets, which is consistent with the rest of the design.

Test plan

• wholeIP: "false" + allowICMP: "true": ping VM works, port filter still in effect for TCP/UDP
• wholeIP: "false" without allowICMP: ICMP dropped (default)
• wholeIP: "true": passthrough, ICMP works
• Toggle allowICMP true→false: pod removed from icmp_allowed_pods
• Service deletion cleans up icmp_allowed_pods entry

Summary by CodeRabbit

Release Notes

• New Features

  • Added optional networking.cozystack.io/allowICMP service annotation to selectively allow ICMP traffic to managed pods in port-filter mode.

• Documentation

  • Updated documentation explaining default ICMP drop behavior in port-filter mode and how the new annotation enables ICMP acceptance.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ad28621d-6bb7-4a9a-86a6-d92e89b60c31

📥 Commits

Reviewing files that changed from the base of the PR and between 4a8d111 and 2131467.

📒 Files selected for processing (6)

• README.md
• pkg/controllers/services_controller.go
• pkg/controllers/services_controller_test.go
• pkg/proxy/dummy.go
• pkg/proxy/interface.go
• pkg/proxy/nft.go

---

📝 Walkthrough

Walkthrough

This change introduces an optional ICMP allowlist feature for port-filter networking mode. The implementation adds service annotation support (networking.cozystack.io/allowICMP), refactors service controller logic into centralized helpers, extends the ProxyProcessor interface with ICMP management methods, and implements ICMP allow/deny functionality in both dummy and nftables proxy processors.

Changes

Cohort / File(s)	Summary
Documentation README.md	Added documentation for optional networking.cozystack.io/allowICMP annotation in port-filter mode; describes ICMP drop behavior and conditional acceptance flow via icmp_allowed_pods.
Service Controller Refactor pkg/controllers/services_controller.go, pkg/controllers/services_controller_test.go	Refactored port-filter logic into centralized helpers (reconcilePortFilter, clearPortFilter); integrated allowICMP annotation parsing and conditional ICMP-allow rule management; added unit test validating annotation parsing across multiple scenarios.
Proxy Interface pkg/proxy/interface.go	Extended ProxyProcessor interface with three methods: EnsureICMPAllow, DeleteICMPAllow, and CleanupICMPAllow to manage ICMP allowlist entries.
Proxy Implementations pkg/proxy/dummy.go, pkg/proxy/nft.go	Dummy processor provides no-op implementations with diagnostic output; nftables processor creates icmp_allowed_pods set, installs ICMP accept rules, and implements reconciliation logic with batched set operations.

Sequence Diagram

sequenceDiagram
    participant Svc as Service with<br/>allowICMP annotation
    participant Ctrl as Services<br/>Controller
    participant PP as ProxyProcessor
    participant NFT as nftables<br/>port_filter chain

    Svc->>Ctrl: Add/Update Service
    Ctrl->>Ctrl: Extract allowICMP<br/>annotation value
    
    alt allowICMP: "true" & port-filter mode
        Ctrl->>PP: EnsureICMPAllow(svcIP, podIP)
        PP->>NFT: Add podIP to<br/>icmp_allowed_pods set
        NFT->>NFT: ICMP packets to podIP<br/>match accept rule
    else allowICMP absent/false & port-filter mode
        Ctrl->>PP: DeleteICMPAllow(svcIP, podIP)
        PP->>NFT: Remove podIP from<br/>icmp_allowed_pods set
        NFT->>NFT: ICMP packets to podIP<br/>hit drop rule
    end

    Svc->>Ctrl: Delete Service
    Ctrl->>PP: CleanupICMPAllow(keep map)
    PP->>NFT: Reconcile set:<br/>remove unlisted entries

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 A fluffy bouncing through the nets,
Where ICMP pings now pass begets,
With annotations marking the way,
Allowed pods get to ICMP-play,
The port-filter chain, no longer gray!

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/allow-icmp

---

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 and usage tips.}

[gemini-code-assist]

Code Review

This pull request introduces the networking.cozystack.io/allowICMP annotation, enabling ICMP traffic for services in port-filter mode to support ping and Path MTU Discovery. The ServicesController has been refactored to use reconcilePortFilter and clearPortFilter helpers for managing port-filtering and ICMP rules. Additionally, the NFTProxyProcessor now includes an icmp_allowed_pods nftables set and a rule to accept ICMP traffic for authorized pods. I have no feedback to provide.