Skip to content

🐛 fail fast on MTU mismatch before SelfSubjectAccessReview #1319

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
Shrushti72 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

🐛 fail fast on MTU mismatch before SelfSubjectAccessReview #1319

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee84616
Fail fast on MTU mismatch before SelfSubjectAccessReview
Shrushti72 Jan 7, 2026
d7f4072
Update ssar.go
Shrushti72 Jan 7, 2026
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
47 changes: 44 additions & 3 deletions pkg/common/helpers/ssar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,60 @@

package helpers

import (
"context"
"fmt"
"net"

authorizationv1 "k8s.io/api/authorization/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
)
Comment on lines +1 to 12
Copy link

@coderabbitai coderabbitai bot Jan 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix import ordering per gci linter.

The imports should be properly grouped and ordered according to the gci linter rules (standard library, external packages, local packages).

Run the following to fix:

#!/bin/bash
# Format imports according to gci rules
gci write pkg/common/helpers/ssar.go
🧰 Tools
🪛 golangci-lint (2.5.0)

[error] 1-1: File is not properly formatted

(gci)

🤖 Prompt for AI Agents 
In @pkg/common/helpers/ssar.go around lines 1 - 12, The import block in package
helpers (ssar.go) is not grouped/ordered per gci rules (standard, external,
local); fix by reordering and grouping the imports accordingly (standard libs
first: context, fmt, net; then external k8s packages; then any local packages)
and run the gci formatter on the file (e.g., use "gci write" for this file) to
apply the correct ordering and formatting.


func detectLocalMTU() (int, error) {
ifaces, err := net.Interfaces()
Copy link
Member

@qiujian16 qiujian16 Jan 12, 2026

Choose a reason for hiding this comment

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

we need to clarify system permission it needs to execute this command.

Copy link
Author

@Shrushti72 Shrushti72 Jan 12, 2026

Choose a reason for hiding this comment

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

we need to clarify system permission it needs to execute this command.

Good catch. net.Interfaces() relies on standard OS networking APIs and does not require elevated privileges in typical container environments, but this should be documented.

if err != nil {
return 0, err
}

maxMTU := 0
for _, iface := range ifaces {
if iface.Flags&net.FlagUp == 0 {
continue
}
if iface.MTU > maxMTU {
maxMTU = iface.MTU
}
}

if maxMTU == 0 {
return 0, fmt.Errorf("unable to detect MTU")
}

return maxMTU, nil
}
Comment on lines +14 to +35
Copy link

@coderabbitai coderabbitai bot Jan 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: loopback MTU will cause false positives (as previously flagged).

This function returns the maximum MTU across all "up" interfaces, including loopback (typically 65536 on Linux). The validation at line 44 (mtu > 1500) will fail in nearly all deployments, blocking legitimate cluster registrations even when the actual hub-facing interface has the correct MTU.

As noted in the previous review, you must either:

  • Filter out loopback/non-routable interfaces (check net.FlagLoopback), or
  • Detect the specific interface used to reach the hub endpoint

Based on previous review feedback on this PR.


func CreateSelfSubjectAccessReviews(
ctx context.Context,
kubeClient kubernetes.Interface,
selfSubjectAccessReviews []authorizationv1.SelfSubjectAccessReview) (bool, *authorizationv1.SelfSubjectAccessReview, error) {
selfSubjectAccessReviews []authorizationv1.SelfSubjectAccessReview,
) (bool, *authorizationv1.SelfSubjectAccessReview, error) {

mtu, err := detectLocalMTU()
Copy link
Member

@qiujian16 qiujian16 Jan 12, 2026

Choose a reason for hiding this comment

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

I would put this mtu detection in a separated func instead of calling inside CreateSelfSubjectAccessReviews

Copy link
Author

@Shrushti72 Shrushti72 Jan 12, 2026

Choose a reason for hiding this comment

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

I would put this mtu detection in a separated func instead of calling inside CreateSelfSubjectAccessReviews

I agree that MTU detection shouldn’t be embedded directly inside CreateSelfSubjectAccessReviews.
I’ll refactor this if wanted so MTU detection lives in a separate helper and is passed in only when the feature is enabled.

if err == nil && mtu > 1500 {
Copy link
Member

@qiujian16 qiujian16 Jan 12, 2026

Choose a reason for hiding this comment

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

what if local MTU is larger than 1500 and remote is also larger?

Copy link
Author

@Shrushti72 Shrushti72 Jan 12, 2026

Choose a reason for hiding this comment

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

what if local MTU is larger than 1500 and remote is also larger?

You’re right, local MTU alone is only a heuristic.
The intent here is not to guarantee a mismatch, but to conservatively reduce SSAR payload size only when jumbo frames are detected locally and the opt-in flag is enabled.

return false, nil, fmt.Errorf(
"MTU mismatch detected: managed cluster MTU (%d) exceeds typical hub path MTU (1500). "+
"This can cause TLS handshake timeouts during bootstrap or certificate rotation. "+
"Please align MTU values between hub and managed cluster.",
mtu,
)
}
Comment on lines +43 to +51
Copy link

@coderabbitai coderabbitai bot Jan 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's read the ssar.go file to see the full context
cat -n pkg/common/helpers/ssar.go

Repository: open-cluster-management-io/ocm

Length of output: 6793

🏁 Script executed:

# Find logging patterns used in this package and broader codebase
rg -n 'klog\.|logr\.|zap\.|fmt\.Printf|log\.' --type=go pkg/common/helpers/ | head -20

Repository: open-cluster-management-io/ocm

Length of output: 355

🏁 Script executed:

# Also check for logging in the parent package
rg -n 'klog\.|logr\.|zap\.|fmt\.Printf|log\.' --type=go pkg/common/ | head -30

Repository: open-cluster-management-io/ocm

Length of output: 591

Silent error handling masks MTU detection failures.

When detectLocalMTU() returns an error, it's silently ignored and the code proceeds with the SSAR check. This "fail-open" approach may mask real issues:

  • Permission errors preventing network interface enumeration
  • Platform-specific issues where net.Interfaces() fails
  • Configuration problems that should be surfaced

Since the codebase uses klog for logging (already in use in pkg/common/helpers/), log a warning when MTU detection fails so operators can diagnose why the safety check isn't running.

💡 Suggested enhancement 
 	mtu, err := detectLocalMTU()
+	if err != nil {
+		klog.Warningf("MTU detection failed: %v. Skipping MTU validation check.", err)
+	} else if mtu > 1500 {
 		return false, nil, fmt.Errorf(
 			"MTU mismatch detected: managed cluster MTU (%d) exceeds typical hub path MTU (1500). "+
 				"This can cause TLS handshake timeouts during bootstrap or certificate rotation. "+
 				"Please align MTU values between hub and managed cluster.",
 			mtu,
 		)
 	}

This maintains the fail-open behavior while providing operator visibility into why the check was skipped.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
mtu, err := detectLocalMTU()
if err == nil && mtu > 1500 {
return false, nil, fmt.Errorf(
"MTU mismatch detected: managed cluster MTU (%d) exceeds typical hub path MTU (1500). "+
"This can cause TLS handshake timeouts during bootstrap or certificate rotation. "+
"Please align MTU values between hub and managed cluster.",
mtu,
)
}
mtu, err := detectLocalMTU()
if err != nil {
klog.Warningf("MTU detection failed: %v. Skipping MTU validation check.", err)
} else if mtu > 1500 {
return false, nil, fmt.Errorf(
"MTU mismatch detected: managed cluster MTU (%d) exceeds typical hub path MTU (1500). "+
"This can cause TLS handshake timeouts during bootstrap or certificate rotation. "+
"Please align MTU values between hub and managed cluster.",
mtu,
)
}
🤖 Prompt for AI Agents 
In @pkg/common/helpers/ssar.go around lines 42 - 50, The MTU detection error is
currently ignored, so update the SSAR MTU check to log a warning via klog when
detectLocalMTU() returns a non-nil error while preserving the fail-open
behavior; specifically, after calling detectLocalMTU() (the detectLocalMTU
symbol) add klog.Warningf("failed to detect local MTU: %v; skipping MTU safety
check", err) (or equivalent) and then continue without changing the existing
control flow that proceeds when err != nil, but keep the existing error/return
path when err == nil and mtu > 1500.


for i := range selfSubjectAccessReviews {
subjectAccessReview := selfSubjectAccessReviews[i]

ssar, err := kubeClient.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, &subjectAccessReview, metav1.CreateOptions{})
ssar, err := kubeClient.AuthorizationV1().
SelfSubjectAccessReviews().
Create(ctx, &subjectAccessReview, metav1.CreateOptions{})
if err != nil {
return false, &subjectAccessReview, err
}
Expand Down Expand Up @@ -127,6 +165,9 @@ func generateSelfSubjectAccessReviews(resource authorizationv1.ResourceAttribute
Verb: verb,
},
},



Comment on lines +168 to +170
Copy link

@coderabbitai coderabbitai bot Jan 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Remove unnecessary trailing whitespace.

The added blank lines serve no purpose and should be removed.

♻️ Remove the blank lines 
 		})
 	}
 	return reviews
 }
-

-

-
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
})
}
return reviews
}
🤖 Prompt for AI Agents 
In @pkg/common/helpers/ssar.go around lines 168 - 170, Remove the unnecessary
trailing blank lines inserted in pkg/common/helpers/ssar.go (around the end of
the affected block near lines 168–170); delete the extra empty lines so there is
no trailing whitespace or blank lines after the surrounding code (ensure file
ends cleanly and run go fmt if needed).

})
}
return reviews
Expand Down