Fix control-plane join failing with empty certificate key#57
Merged
Conversation
The upload-certs command was piped through `2>/dev/null | tail -1`, which suppressed errors and masked the exit code. When kubeadm failed, we got an empty string instead of an error. Remove the pipe and stderr suppression so failures propagate properly. Extract the certificate key by matching the 64-char hex string from the output instead of blindly taking the last line. Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reviewer's GuideAdjusts the control-plane join flow to preserve kubeadm error propagation and robustly extract the certificate key from upload-certs output, avoiding failures due to empty keys. Sequence diagram for updated control-plane join certificate key extractionsequenceDiagram
participant Cluster
participant cpSSHClient
participant kubeadm
Cluster->>Cluster: generateJoinCommand(ctx, cpSSHClient, true)
Cluster->>cpSSHClient: Exec(ctx, sudo kubeadm init phase upload-certs --upload-certs)
cpSSHClient->>kubeadm: run upload-certs
kubeadm-->>cpSSHClient: certKeyOutput (logs on stderr, key on stdout)
cpSSHClient-->>Cluster: certKeyOutput
loop for each line in certKeyOutput
Cluster->>Cluster: strings.TrimSpace(line)
alt len(line) == 64 and isHex(line)
Cluster->>Cluster: set certificateKey
end
end
alt certificateKey == ""
Cluster->>Cluster: fmt.Errorf(certificate key not found in upload-certs output)
Cluster-->>Cluster: generateJoinCommand returns error
else certificateKey != ""
Cluster->>Cluster: logger.Infof(Certificate key: %s, certificateKey)
Cluster-->>Cluster: generateJoinCommand returns join command
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- When scanning for the certificate key, consider breaking out of the loop as soon as a valid 64-char hex string is found instead of overwriting
certificateKeyon each match, to avoid unintentionally preferring a later hex-looking token if kubeadm ever prints more than one.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When scanning for the certificate key, consider breaking out of the loop as soon as a valid 64-char hex string is found instead of overwriting `certificateKey` on each match, to avoid unintentionally preferring a later hex-looking token if kubeadm ever prints more than one.
## Individual Comments
### Comment 1
<location path="internal/cluster/join.go" line_range="167-168" />
<code_context>
+ certificateKey = line
+ }
+ }
if certificateKey == "" {
- return "", fmt.Errorf("certificate key is empty")
+ return "", fmt.Errorf("certificate key not found in upload-certs output: %s", certKeyOutput)
}
</code_context>
<issue_to_address>
**🚨 issue (security):** Avoid returning the full upload-certs output (including the key) in the error message
Including `certKeyOutput` here will log and propagate the certificate key, which is sensitive and should never be exposed. Instead, either omit the raw output from the error or redact the key (e.g., log only a short, non-sensitive snippet or use a structured message without the full value).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
167
to
+168
| if certificateKey == "" { | ||
| return "", fmt.Errorf("certificate key is empty") | ||
| return "", fmt.Errorf("certificate key not found in upload-certs output: %s", certKeyOutput) |
There was a problem hiding this comment.
🚨 issue (security): Avoid returning the full upload-certs output (including the key) in the error message
Including certKeyOutput here will log and propagate the certificate key, which is sensitive and should never be exposed. Instead, either omit the raw output from the error or redact the key (e.g., log only a short, non-sensitive snippet or use a structured message without the full value).
Without --config, kubeadm upload-certs auto-discovers the API server and connects to the container's podman bridge IP instead of the VM cluster IP, causing a certificate validation error (x509: certificate is valid for 10.0.0.x, not 10.89.x.x). Pass --config /etc/kubernetes/kubeadm-config.yaml so kubeadm uses the correct controlPlaneEndpoint from the init config. Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The upload-certs command was piped through
2>/dev/null | tail -1, which suppressed errors and masked the exit code. When kubeadm failed, we got an empty string instead of an error.Remove the pipe and stderr suppression so failures propagate properly. Extract the certificate key by matching the 64-char hex string from the output instead of blindly taking the last line.
Summary by Sourcery
Ensure control-plane join correctly obtains the kubeadm certificate key by using the full upload-certs output and parsing the hex key instead of relying on a piped tail command.
Bug Fixes:
Enhancements: