Skip to content

fix(bypass): clean up stale iptables rules on daemon restart #1621

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
imshubham22apr-gif wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(bypass): clean up stale iptables rules on daemon restart #1621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 7 additions & 3 deletions pkg/controller/bypass/bypass_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,11 @@ func NewByPassController(client kubernetes.Interface) *Controller {
}

if !shouldBypass(pod) {
// TODO: add delete iptables in case we missed skip bypass during kmesh restart
nspath, _ := ns.GetPodNSpath(pod)
if nspath != "" {
// during daemon restart, reconcile to ensure stale bypass rules are cleared
_ = deleteIptables(nspath)
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 21, 2026

Choose a reason for hiding this comment

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

high

The error returned by deleteIptables is ignored. This is inconsistent with other parts of the controller where errors from addIptables and deleteIptables are logged. If deleteIptables fails, it will fail silently, and stale iptables rules might not be cleaned up as intended, which undermines the goal of this change. The error should be logged to provide visibility into potential failures.

				if err := deleteIptables(nspath); err != nil {
					log.Errorf("failed to delete stale iptables rules for pod %s/%s: %v", pod.GetNamespace(), pod.GetName(), err)
				}

Comment on lines 67 to +71
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

The new reconciliation path runs deleteIptables for every non-bypass Pod Add event (not just during informer startup). That means every newly created sidecar-injected pod will trigger netns lookup + iptables deletes even though there can’t be “stale” Kmesh rules yet, which can add avoidable overhead. Consider limiting this cleanup to the initial informer sync (startup reconciliation) or a one-time post-sync pass over existing pods, and handle/log GetPodNSpath/deleteIptables errors explicitly rather than dropping them.

Copilot uses AI. Check for mistakes.
}
return
}

Expand Down Expand Up @@ -157,7 +161,7 @@ func addIptables(ns string) error {
}
for _, args := range iptArgs {
if err := utils.Execute("iptables", args); err != nil {
return fmt.Errorf("failed to exec command: iptables %v\", err: %v", args, err)
return fmt.Errorf("failed to exec command: iptables %v, err: %v", args, err)
}
}
return nil
Expand All @@ -178,7 +182,7 @@ func deleteIptables(ns string) error {
log.Infof("Running delete iptables rule in namespace:%s", ns)
for _, args := range iptArgs {
if err := utils.Execute("iptables", args); err != nil {
err = fmt.Errorf("failed to exec command: iptables %v\", err: %v", args, err)
err = fmt.Errorf("failed to exec command: iptables %v, err: %v", args, err)
log.Error(err)
return err
Comment on lines 183 to 187
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

deleteIptables treats any iptables -D failure as an error and logs it internally. With the new AddFunc reconciliation, this is likely to emit an error for pods where the RETURN rules were never present (iptables prints to stderr when a rule is missing), causing noisy logs on restart. Make deletion idempotent by ignoring “rule not found” errors (or check with -C first), and consider letting the caller decide whether to log to avoid double/log-spam behavior.

Copilot uses AI. Check for mistakes.
}
Expand Down
23 changes: 23 additions & 0 deletions pkg/controller/bypass/bypass_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,4 +150,27 @@ func TestBypassController(t *testing.T) {
wg.Wait()
assert.Equal(t, true, enabled.Load(), "unexpected value for enabled flag")
assert.Equal(t, false, disabled.Load(), "unexpected value for disabled flag")

enabled.Store(false)
disabled.Store(false)
// case 5: Pod with sidecar but no bypass label at creation
podWithSidecarButNoBypass := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "test-pod-no-bypass",
Namespace: namespaceName,
Labels: map[string]string{}, // no bypass label
Annotations: map[string]string{
"sidecar.istio.io/status": "placeholder",
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

This test uses a hard-coded "sidecar.istio.io/status" annotation key, while the rest of the test (and PodHasSidecar) uses annotation.SidecarStatus.Name. Using the constant here avoids drift/typos if Istio’s key changes and keeps the test aligned with the production check.

Suggested change
"sidecar.istio.io/status": "placeholder",
annotation.SidecarStatus.Name: "placeholder",

Copilot uses AI. Check for mistakes.
},
},
Spec: corev1.PodSpec{
NodeName: nodeName,
},
}
wg.Add(1)
_, err = client.CoreV1().Pods(namespaceName).Create(context.TODO(), podWithSidecarButNoBypass, metav1.CreateOptions{})
assert.NoError(t, err)
wg.Wait()
assert.Equal(t, false, enabled.Load(), "unexpected value for enabled flag")
assert.Equal(t, true, disabled.Load(), "unexpected value for disabled flag")
}
Loading