Skip to content

Use curl instead of oc binary copy for testing kubeconfig #30927

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
dgoodwin wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −10
Open

Use curl instead of oc binary copy for testing kubeconfig #30927

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
20 changes: 10 additions & 10 deletions test/extended/apiserver/kubeconfigs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,17 +112,17 @@ func testKubeApiserverContainer(oc *exutil.CLI, kubeconfig, masterName string) e
return fmt.Errorf("location for %s kubeconfig not found", kubeconfig)
}

framework.Logf("Copying oc binary from host to kube-apiserver container in master %q", masterName)
out, err := oc.AsAdmin().Run("debug").Args("node/"+masterName, "--", "chroot", "/host", "/bin/bash", "-euxo", "pipefail", "-c",
fmt.Sprintf(`oc --kubeconfig /etc/kubernetes/static-pod-resources/kube-apiserver-certs/secrets/node-kubeconfigs/localhost.kubeconfig -n openshift-kube-apiserver cp /usr/bin/oc kube-apiserver-%s:/tmp`, masterName)).Output()
framework.Logf("%s", out)
if err != nil {
return fmt.Errorf("%s", out)
}

// Use curl to verify the kubeconfig is present and functional, extracting
// cert paths directly from the kubeconfig file. This avoids copying the oc
// binary from the host into the container, which fails when the host OS
// (e.g., RHCOS 10 with glibc 2.38) has a newer glibc than the container.
framework.Logf("Verifying kubeconfig %q in kube-apiserver container in master %q", kubeconfig, masterName)
out, err = oc.AsAdmin().Run("exec").Args("-n", "openshift-kube-apiserver", "kube-apiserver-"+masterName, "--", "/bin/bash", "-euxo", "pipefail", "-c",
fmt.Sprintf(`/tmp/oc --kubeconfig "%s" get nodes`, kubeconfigPath)).Output()
out, err := oc.AsAdmin().Run("exec").Args("-n", "openshift-kube-apiserver", "kube-apiserver-"+masterName, "--", "/bin/bash", "-euxo", "pipefail", "-c",
fmt.Sprintf(`server=$(grep 'server:' "%[1]s" | head -1 | awk '{print $2}')
cert=$(grep 'client-certificate:' "%[1]s" | head -1 | awk '{print $2}')
key=$(grep 'client-key:' "%[1]s" | head -1 | awk '{print $2}')
ca=$(grep 'certificate-authority:' "%[1]s" | head -1 | awk '{print $2}')
curl -sf --cert "$cert" --key "$key" --cacert "$ca" "${server}/api?timeout=32s"`, kubeconfigPath)).Output()
Comment on lines +121 to +125
Copy link

@coderabbitai coderabbitai bot Mar 24, 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:

cd test/extended/apiserver && wc -l kubeconfigs.go

Repository: openshift/origin

Length of output: 78

🏁 Script executed:

cd test/extended/apiserver && sed -n '110,135p' kubeconfigs.go

Repository: openshift/origin

Length of output: 1251

🏁 Script executed:

cd test/extended/apiserver && sed -n '115,130p' kubeconfigs.go | cat -n

Repository: openshift/origin

Length of output: 1217

🏁 Script executed:

cd test/extended/apiserver && grep -B 5 -A 5 "kubeApiserverLocations" kubeconfigs.go | head -30

Repository: openshift/origin

Length of output: 1707

🏁 Script executed:

rg "verifyKubeconfig|verifyKubeapiserverKubeconfig" test/extended/apiserver/ -B 2 -A 2

Repository: openshift/origin

Length of output: 42

🌐 Web query:

curl -sf vs curl -Ssf flags difference stderr error output

💡 Result:

The difference between curl -sf and curl -Ssf (or curl -sSf) lies in how they handle stderr output for errors when running in silent mode. - curl -sf (silent + fail): Suppresses the progress meter and all error messages (no output to stderr on failure). On HTTP errors (e.g., 4xx/5xx), it fails with exit code 22 but produces no output at all, including no error message on stderr. - curl -Ssf (show-error + silent + fail): Suppresses the progress meter but shows an error message on stderr if the transfer fails (e.g., connection errors or HTTP errors due to -f). This is the common combination for scripts: quiet on success, visible error on stderr on failure for logging/monitoring (e.g., cron jobs). All diagnostic output (progress, verbose, errors) from curl goes to stderr by default. Response body goes to stdout. Examples: - Success (both): No stderr output. - HTTP 404 with -sf: Exit 22, empty stderr. - HTTP 404 with -Ssf: Exit 22, error like "curl: (22) The requested URL returned error: 404" on stderr. This behavior is consistent across recent curl versions (e.g., 8.x as of 2026).

Citations:

Improve test diagnostics by using curl -Ssf and add defensive validation for kubeconfig parsing.

The shell script extracts cert paths with grep | head -1 without validating that each field appears exactly once. While the test kubeconfigs are single-purpose, defensive validation prevents accidental mismatches. More importantly, curl -sf suppresses error output entirely, making failures opaque; switch to curl -Ssf to show errors to stderr for better test diagnostics.

Proposed hardening patch 
-		fmt.Sprintf(`server=$(grep 'server:' "%[1]s" | head -1 | awk '{print $2}')
-cert=$(grep 'client-certificate:' "%[1]s" | head -1 | awk '{print $2}')
-key=$(grep 'client-key:' "%[1]s" | head -1 | awk '{print $2}')
-ca=$(grep 'certificate-authority:' "%[1]s" | head -1 | awk '{print $2}')
-curl -sf --cert "$cert" --key "$key" --cacert "$ca" "${server}/api?timeout=32s"`, kubeconfigPath)).Output()
+		fmt.Sprintf(`cfg="%[1]s"
+for k in server client-certificate client-key certificate-authority; do
+  c=$(grep -Ec "^[[:space:]]*${k}:[[:space:]]+" "$cfg")
+  [ "$c" -eq 1 ] || { echo "expected exactly one ${k} in $cfg, got $c" >&2; exit 1; }
+done
+server=$(grep -m1 -E "^[[:space:]]*server:[[:space:]]+" "$cfg" | awk '{print $2}')
+cert=$(grep -m1 -E "^[[:space:]]*client-certificate:[[:space:]]+" "$cfg" | awk '{print $2}')
+key=$(grep -m1 -E "^[[:space:]]*client-key:[[:space:]]+" "$cfg" | awk '{print $2}')
+ca=$(grep -m1 -E "^[[:space:]]*certificate-authority:[[:space:]]+" "$cfg" | awk '{print $2}')
+curl -Ssf --cert "$cert" --key "$key" --cacert "$ca" "${server}/api?timeout=32s"`, kubeconfigPath)).Output()
📝 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
fmt.Sprintf(`server=$(grep 'server:' "%[1]s" | head -1 | awk '{print $2}')
cert=$(grep 'client-certificate:' "%[1]s" | head -1 | awk '{print $2}')
key=$(grep 'client-key:' "%[1]s" | head -1 | awk '{print $2}')
ca=$(grep 'certificate-authority:' "%[1]s" | head -1 | awk '{print $2}')
curl -sf --cert "$cert" --key "$key" --cacert "$ca" "${server}/api?timeout=32s"`, kubeconfigPath)).Output()
fmt.Sprintf(`cfg="%[1]s"
for k in server client-certificate client-key certificate-authority; do
c=$(grep -Ec "^[[:space:]]*${k}:[[:space:]]+" "$cfg")
[ "$c" -eq 1 ] || { echo "expected exactly one ${k} in $cfg, got $c" >&2; exit 1; }
done
server=$(grep -m1 -E "^[[:space:]]*server:[[:space:]]+" "$cfg" | awk '{print $2}')
cert=$(grep -m1 -E "^[[:space:]]*client-certificate:[[:space:]]+" "$cfg" | awk '{print $2}')
key=$(grep -m1 -E "^[[:space:]]*client-key:[[:space:]]+" "$cfg" | awk '{print $2}')
ca=$(grep -m1 -E "^[[:space:]]*certificate-authority:[[:space:]]+" "$cfg" | awk '{print $2}')
curl -Ssf --cert "$cert" --key "$key" --cacert "$ca" "${server}/api?timeout=32s"`, kubeconfigPath)).Output()
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/extended/apiserver/kubeconfigs.go` around lines 121 - 125, The shell
snippet that builds and runs the curl check using kubeconfigPath should be
hardened: change curl flags from -sf to -Ssf so curl prints error details to
stderr, and add defensive validation after extracting server, cert, key, and ca
(the variables produced by the grep/head pipelines that reference
kubeconfigPath) to ensure each field is present and appears exactly once (e.g.,
check non-empty and unique counts) and fail with a clear error message if
validation fails; update the formatted command string surrounding kubeconfigPath
accordingly so failures are visible and mis-parsed kubeconfigs cause immediate,
informative test failures.

framework.Logf("%s", out)
if err != nil {
return fmt.Errorf("%s", out)
Expand Down