fix(k3s): authorize galactica on kyber and surface client sync failures - #1738
Conversation
Why: scp stderr was redirected to /dev/null and wrapped in an if-then with no else branch, so any sync failure (host key mismatch, missing authorized key, network drop) made make switch look successful while silently leaving ~/.kube/config-kyber absent. Now logs success and pipes scp stderr to the activation output on failure, while still exiting 0 so a missing key on a fresh machine does not break home-manager activation.
|
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR improves error handling in the k3s client activation script. The ChangesK3s Client Error Visibility
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Mesa DescriptionTL;DRFixes What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves error visibility in the activate-client.sh script by capturing scp errors into a temporary file and logging them upon failure, rather than discarding them to /dev/null. Corresponding tests were added to verify these changes. However, one test case was identified as ineffective because it fails to capture the multi-line scp command structure; a suggestion was made to use grep -A 1 to correctly validate the removal of the stderr redirection.
|
|
||
| Describe 'failure visibility' | ||
| It 'does not silently discard scp stderr' | ||
| When run bash -c "grep 'scp ' '$SCRIPT'" |
There was a problem hiding this comment.
The test 'does not silently discard scp stderr' is currently ineffective. Because grep 'scp ' only returns the first line of the multi-line scp command, it does not see the redirection (which was on the following line due to the backslash). Consequently, this test would pass even if 2>/dev/null were still present in the script. Using grep -A 1 ensures the redirection line is included in the check.
| When run bash -c "grep 'scp ' '$SCRIPT'" | |
| When run bash -c "grep -A 1 'scp ' '$SCRIPT'" |
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| Describe 'failure visibility' | ||
| It 'does not silently discard scp stderr' | ||
| When run bash -c "grep 'scp ' '$SCRIPT'" | ||
| The output should not include '2>/dev/null' |
There was a problem hiding this comment.
Regression test is too narrow to catch a recurrence: this assertion only looks at lines containing scp (the if scp ... line), but the original 2>/dev/null was on the continuation line "${REMOTE_HOST}:..." "$LOCAL_KUBECONFIG" 2>/dev/null; then — which has no scp token. Reintroducing the bug in the same shape would leave this grep output unchanged and the test would still pass.
Consider checking the whole script instead, e.g.:
It 'does not silently discard scp stderr'
When run bash -c "! grep -n '2>/dev/null' '$SCRIPT'"
The status should be success
Endor a multi-line grep that spans the scp invocation (grep -Pzo 'scp[^;]*2>/dev/null').
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="spec/activate_k3s_client_spec.sh">
<violation number="1" location="spec/activate_k3s_client_spec.sh:50">
P2: This regression test only greps the first line of the multi-line `scp` invocation, so it can miss a reintroduced `2>/dev/null` on the continuation line. Include the following line (or scan the whole script) before asserting stderr is not discarded.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| Describe 'failure visibility' | ||
| It 'does not silently discard scp stderr' | ||
| When run bash -c "grep 'scp ' '$SCRIPT'" |
There was a problem hiding this comment.
P2: This regression test only greps the first line of the multi-line scp invocation, so it can miss a reintroduced 2>/dev/null on the continuation line. Include the following line (or scan the whole script) before asserting stderr is not discarded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/activate_k3s_client_spec.sh, line 50:
<comment>This regression test only greps the first line of the multi-line `scp` invocation, so it can miss a reintroduced `2>/dev/null` on the continuation line. Include the following line (or scan the whole script) before asserting stderr is not discarded.</comment>
<file context>
@@ -44,4 +44,16 @@ When run bash -c "grep 'chmod 600' '$SCRIPT'"
+
+Describe 'failure visibility'
+It 'does not silently discard scp stderr'
+When run bash -c "grep 'scp ' '$SCRIPT'"
+The output should not include '2>/dev/null'
+End
</file context>
| When run bash -c "grep 'scp ' '$SCRIPT'" | |
| When run bash -c "grep -A 1 'scp ' '$SCRIPT'" |
Why: scp from galactica to kyber failed with publickey denial because galactica's pubkey was not in kyber's authorized_keys, so the silent sync after switch was a no-op. The pubkey was already duplicated across three secrets.nix files, so factor it out at the same time. - Extract galactica/kyber/matic pubkeys to named-hosts/pubkeys.nix and inherit from each per-host secrets.nix. - config/k3s/activate.sh idempotently appends galactica's pubkey to ~/.ssh/authorized_keys on kyber, with a placeholder guard so the unsubstituted template is a no-op. - config/k3s/default.nix templates the pubkey through pkgs.replaceVars, sourcing it from the same shared file.
shunkakinoki
changed the title
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="spec/activate_k3s_spec.sh">
<violation number="1" location="spec/activate_k3s_spec.sh:74">
P2: Behavioral checks run a duplicated helper in the spec instead of the real `ensure_authorized_key` implementation, which can hide regressions in `config/k3s/activate.sh`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| The output should include 'grep -qxF' | ||
| End | ||
|
|
||
| ensure_authorized_key_function() { |
There was a problem hiding this comment.
P2: Behavioral checks run a duplicated helper in the spec instead of the real ensure_authorized_key implementation, which can hide regressions in config/k3s/activate.sh.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/activate_k3s_spec.sh, line 74:
<comment>Behavioral checks run a duplicated helper in the spec instead of the real `ensure_authorized_key` implementation, which can hide regressions in `config/k3s/activate.sh`.</comment>
<file context>
@@ -49,4 +49,100 @@ When run bash -c "grep -A 1 '! -f' '$SCRIPT'"
+The output should include 'grep -qxF'
+End
+
+ensure_authorized_key_function() {
+ cat <<'BASH'
+ensure_authorized_key() {
</file context>
The bash -c snippets quote with single quotes intentionally so the inner $tmp/$HOME expand at the right moment. Add SC2016 to the existing per-file disable list to keep CI green.
1 participant
Summary
Two bugs prevented
kubectlfrom working on galactica aftermake switch:config/k3s/activate-client.shdiscarded scp stderr (2>/dev/null) and usedif scp; then ...; fiwith no else branch. Any sync failure left~/.kube/config-kyberabsent while activation looked clean.~/.ssh/authorized_keys. Nothing in the repo put it there, so a fresh sync always hitPermission denied (publickey).While fixing #2 the galactica/kyber/matic pubkeys turned out to be hardcoded in three
secrets.nixfiles, so they got pulled into a single shared module.Changes
named-hosts/pubkeys.nix(new): single source of truth for the three host pubkeys.named-hosts/{galactica,kyber,matic}/secrets.nix:inheritfrompubkeys.nixinstead of duplicating the strings.config/k3s/default.nix: read galactica's pubkey frompubkeys.nixand template it intoactivate.shviapkgs.replaceVars(matching the existingk3s.servicepattern).config/k3s/activate.sh: newensure_authorized_keyhelper appends galactica's key to kyber's~/.ssh/authorized_keysidempotently, with a placeholder guard so the unsubstituted@galacticaAuthorizedKey@template is a no-op.config/k3s/activate-client.sh: capture scp stderr to a temp file, log a one-line success on the happy path, on failure printk3s-client: failed to fetch kubeconfig from ...plus the indented scp output. Still exits 0 so a not-yet-authorized fresh machine does not break home-manager activation.spec/activate_k3s_spec.sh(+10 examples): placeholder declared, helper exists, fixed-string match used, idempotency, happy-path append, preserve-existing, no-op on unsubstituted placeholder.spec/activate_k3s_client_spec.sh(+2 examples): reject2>/dev/null, require failure-log string.Test plan
nix flake check --no-build(galactica, runner, default eval clean)shellspec spec/activate_k3s_spec.sh spec/activate_k3s_client_spec.sh-> 25 examples, 0 failuresshellcheckclean on both activate scriptsPermission denied (publickey)instead of vanishingmake build && make switch, confirm a singlek3s-server: authorized galactica SSH key for kubeconfig syncline and that~/.ssh/authorized_keyscontains galactica's pubkey exactly once after multiple runsmake switch, confirmk3s-client: kubeconfig synced from kyber.tail950b36.ts.netandkubectl get nodessucceeds