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

@MitchLewis930 MitchLewis930 commented Jan 30, 2026

User description

PR_041


PR Type

Enhancement


Description

  • Add TCP and UDP state filters for socket termination operations

  • Introduce StateFilterTCP and StateFilterUDP constants with documented state masks

  • Extend SocketFilter struct with States field for granular state filtering

  • Update socket destruction logic to use state filters instead of hardcoded values

  • Apply state filters in load balancer connection termination


Diagram Walkthrough

flowchart LR
  A["SocketFilter struct"] -->|"add States field"| B["Enhanced filtering"]
  C["StateFilterTCP constant"] -->|"TCP state mask"| B
  D["StateFilterUDP constant"] -->|"UDP state mask"| B
  B -->|"used in"| E["Destroy function"]
  E -->|"applied in"| F["terminateConnectionsToBackend"]
Loading

File Walkthrough

Relevant files
Enhancement
sockets.go
Add state filter constants and update socket filtering     

pkg/datapath/sockets/sockets.go

  • Add stateMask() helper function to create bitmasks from state values
  • Define StateFilterTCP constant with documented TCP states to filter
    (excluding TCP_CLOSE and TCP_TIME_WAIT)
  • Define StateFilterUDP constant with all UDP states (0xffff)
  • Add States field to SocketFilter struct
  • Update filterAndDestroySockets() to accept and pass states parameter
  • Replace hardcoded 0xffff state filter with filter.States in Destroy()
    function
+54/-8   
termination.go
Apply state filters in connection termination                       

pkg/loadbalancer/reconciler/termination.go

  • Add states variable initialization in terminateConnectionsToBackend()
  • Set states = sockets.StateFilterUDP for UDP protocol
  • Set states = sockets.StateFilterTCP for TCP protocol
  • Pass States: states to SocketFilter in Destroy() call
+4/-0     
Tests
sockets_test.go
Update test with state filter field                                           

pkg/datapath/sockets/sockets_test.go

  • Update TestDestroy() test to include States: StateFilterUDP in
    SocketFilter initialization
+1/-0     

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>
@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Misleading documentation: StateFilterTCP documentation states TCP_CLOSE is omitted, but the mask includes
netlink.TCP_CLOSE, making the code and comments inconsistent and not self-documenting.

Referred Code
// StateFilterTCP is a mask of all states we consider for socket termination.
// Instead of destroying all states, we make some notable omissions which are
// documented below:
//
//   - TCP_CLOSE: Calls to close a socket in TCP_CLOSE state will result in
//     ENOENT, this is also confusing as it is the same err code returned
//     when a socket that doesn't exist is destroyed.
//
//   - TCP_TIME_WAIT: Socket may enter this state post close/FIN-wait states
//     to catch any leftover traffic that may not have arrived yet. This is
//     for security reasons, as well as avoiding late traffic from entering
//     a new socket bound to the same addr/port. Technically, for the socket
//     LB its not necessary as we remove the key from the rev NAT map in the
//     cil_sock_release() hook, so these sockets won't be found. On the other
//     hand we also do not need to waste time to iterate them.
var StateFilterTCP = stateMask(
	// The following states emit RST (net/ipv4/tcp.c#L3228-L3235)
	netlink.TCP_ESTABLISHED,
	netlink.TCP_CLOSE_WAIT,
	netlink.TCP_FIN_WAIT1,
	netlink.TCP_FIN_WAIT2,


 ... (clipped 13 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Limited audit context: The new socket termination path logs terminated sockets but does not include actor
identity/correlation context, so it is unclear whether this meets audit-trail requirements
for "critical actions" in your environment.

Referred Code
switch filter.Protocol {
case unix.IPPROTO_UDP, unix.IPPROTO_TCP:
redo:
	err := filterAndDestroySockets(family, filter.Protocol, filter.States, func(sock netlink.SocketID, err error) {
		if err != nil {
			errs = errors.Join(errs, fmt.Errorf("socket with filter [%v]: %w", filter, err))
			failed++
			return
		}
		if filter.MatchSocket(sock) {
			logger.Info("", logfields.Socket, sock)
			if err := destroySocket(logger, sock, family, filter.Protocol, filter.States, true); err != nil {
				errs = errors.Join(errs, fmt.Errorf("destroying socket with filter [%v]: %w", filter, err))
				failed++

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Unvalidated shift inputs: stateMask() shifts by caller-provided int values without validating bounds/non-negativity,
which could panic or produce incorrect masks if an unexpected state value is ever passed.

Referred Code
func stateMask(ms ...int) uint32 {
	var out uint32
	for _, m := range ms {
		out |= 1 << m
	}
	return out
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Filter parameter validation: The new States filter is passed through to socket iteration/destruction without local
validation, so it requires confirmation that only trusted/internal callers can influence
states and that unexpected masks cannot cause unsafe behavior.

Referred Code
func terminateConnectionsToBackend(p socketTerminationParams, l3n4Addr lb.L3n4Addr) (opSupported bool) {
	opSupported = true

	var (
		family   uint8
		protocol uint8
		states   uint32
	)
	ip := net.IP(l3n4Addr.AddrCluster.Addr().AsSlice())
	l4Addr := l3n4Addr.L4Addr

	switch l3n4Addr.Protocol {
	case lb.UDP:
		protocol = unix.IPPROTO_UDP
		states = sockets.StateFilterUDP
	case lb.TCP:
		protocol = unix.IPPROTO_TCP
		states = sockets.StateFilterTCP
	default:
		return
	}


 ... (clipped 27 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Remove contradictory TCP state from filter

Remove netlink.TCP_CLOSE from the StateFilterTCP mask to align with the
documentation, which states that this state should be omitted to prevent ENOENT
errors.

pkg/datapath/sockets/sockets.go [67-71]

 // Sockets in TCP_LISTEN are moved to closing state
 // (net/ipv4/tcp.c#L4908)
-netlink.TCP_CLOSE,
 // Following are handled without any special consideration/just closed
 netlink.TCP_SYN_SENT,
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a contradiction between the code and its accompanying comment, which would lead to unnecessary errors when trying to terminate sockets in the TCP_CLOSE state.

Medium
  • More

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants