Skip to content

Comments

cilium, socklb: Add states filter for the termination iteration#1

Open
MitchLewis930 wants to merge 1 commit intopr_041_beforefrom
pr_041_after
Open

cilium, socklb: Add states filter for the termination iteration#1
MitchLewis930 wants to merge 1 commit intopr_041_beforefrom
pr_041_after

Conversation

@MitchLewis930
Copy link

PR_041

Add a state filter to the iterator and skip TCP sockets which are in
closing or time wait state. There is no need to spend time to iterate
these. Technically, there is no harm since when the client app closes
the socket and it goes into time wait state, then upon close the socket
LB removes the socket from the revnat map in cil_sock_release.. but
then again, no need to iterate through these.

Suggested-by: Yusuke Suzuki <yusuke.suzuki@isovalent.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@MitchLewis930
Copy link
Author

@greptileai

@greptile-apps
Copy link

greptile-apps bot commented Jan 30, 2026

Greptile Overview

Greptile Summary

Added protocol-specific state filtering for socket termination operations to improve efficiency when iterating and destroying sockets connected to deleted/unhealthy backends.

Key Changes:

  • Introduced stateMask() helper function to build TCP state bitmasks
  • Added StateFilterTCP that selectively includes TCP states, explicitly excluding TCP_TIME_WAIT
  • Added StateFilterUDP constant (0xffff) that includes all UDP states
  • Extended SocketFilter struct with States field to pass state filters through the API
  • Updated Destroy(), filterAndDestroySockets(), and destroySocket() to use configurable state filters instead of hardcoded 0xffff
  • Updated test and termination logic to use new state filter constants

Critical Issue:
The documentation states TCP_CLOSE should be omitted due to ENOENT errors (lines 46-48), but the implementation includes it in StateFilterTCP (line 69). This contradiction needs clarification.

Confidence Score: 2/5

  • Not safe to merge due to critical logic inconsistency in state filter implementation
  • The PR contains a critical contradiction between documentation and implementation regarding TCP_CLOSE state handling. The comment explicitly states TCP_CLOSE should be omitted to avoid ENOENT errors, but the code includes it in the filter. This could lead to spurious errors or unexpected behavior in production.
  • pkg/datapath/sockets/sockets.go requires immediate attention to resolve the TCP_CLOSE state filter contradiction

Important Files Changed

Filename Overview
pkg/datapath/sockets/sockets.go Adds TCP/UDP state filtering for socket termination; contains critical logic inconsistency between comment and implementation regarding TCP_CLOSE state

Sequence Diagram

sequenceDiagram
    participant Reconciler as Socket Termination Loop
    participant Filter as SocketFilter
    participant Sockets as sockets.Destroy()
    participant Netlink as Netlink Socket API
    participant Kernel as Linux Kernel

    Note over Reconciler: Backend marked unhealthy/deleted
    Reconciler->>Filter: Create filter with protocol-specific states
    alt TCP Protocol
        Filter->>Filter: Set States = StateFilterTCP
    else UDP Protocol
        Filter->>Filter: Set States = StateFilterUDP (0xffff)
    end
    
    Reconciler->>Sockets: Destroy(filter)
    Sockets->>Netlink: filterAndDestroySockets(family, protocol, states)
    Netlink->>Kernel: Query sockets with state filter
    Kernel-->>Netlink: Return matching sockets
    
    loop For each matching socket
        Sockets->>Filter: MatchSocket(socketID)
        Filter->>Filter: Check destination IP/Port
        Filter->>Filter: Check RevNAT map (via DestroyCB)
        alt Socket matches filter
            Sockets->>Netlink: destroySocket(socketID, states)
            Netlink->>Kernel: SOCK_DESTROY with state filter
            Kernel-->>Netlink: ACK or ENOENT
        end
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

netlink.TCP_NEW_SYN_REC,
// Sockets in TCP_LISTEN are moved to closing state
// (net/ipv4/tcp.c#L4908)
netlink.TCP_CLOSE,
Copy link

Choose a reason for hiding this comment

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

Contradicts comment on lines 46-48 which says TCP_CLOSE should be omitted to avoid ENOENT errors

Suggested change
netlink.TCP_CLOSE,
// Following are handled without any special consideration/just closed
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/datapath/sockets/sockets.go
Line: 69:69

Comment:
Contradicts comment on lines 46-48 which says `TCP_CLOSE` should be omitted to avoid `ENOENT` errors

```suggestion
	// Following are handled without any special consideration/just closed
```

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants