Skip to content

Commit 5b79cdc

Browse files
Fix follow-up CodeRabbit review items in wg-onboard.sh.
Remove broken bash -c SSH wrappers, split dependent local declarations, quote remote argv expansions, guard assignment rollback on secret delete failure, and create tracker temp files beside the allocation TSV. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent f68f57f commit 5b79cdc

1 file changed

Lines changed: 36 additions & 28 deletions

File tree

.github/scripts/wg-onboard.sh

Lines changed: 36 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ ssh_cmd() {
169169

170170
# ---- Preflight: jumpserver connectivity + peer pool sizing -------------------
171171
log "Checking jumpserver connectivity and peer inventory on ${JUMPSERVER_HOST} ..."
172-
PEER_LISTING="$(ssh_cmd bash -c "ls -1 $(remote_quote "$CONFIG_DIR")" 2>/dev/null || true)"
172+
PEER_LISTING="$(ssh_cmd "ls -1 $(remote_quote "$CONFIG_DIR")" 2>/dev/null || true)"
173173
[[ -n "$PEER_LISTING" ]] || die "Could not list $CONFIG_DIR on jumpserver (check --wg-dir / connectivity)"
174174

175175
mapfile -t EXISTING_PEERS < <(printf '%s\n' "$PEER_LISTING" | grep -E '^peer[0-9]+$' | sort -t r -k2 -n)
@@ -202,15 +202,16 @@ log "Peer pool: peer1..peer${MAX_PEERS} (gap-fill order, create missing peers on
202202

203203
CAN_CREATE_PEERS="false"
204204
if [[ ${#EXISTING_PEERS[@]} -gt 0 ]] \
205-
|| ssh_cmd bash -c "test -f $(remote_quote "$CONFIG_DIR/templates/peer.conf") || test -f $(remote_quote "$CONFIG_DIR/peer1/peer1.conf")" 2>/dev/null; then
205+
|| ssh_cmd "test -f $(remote_quote "$CONFIG_DIR/templates/peer.conf") || test -f $(remote_quote "$CONFIG_DIR/peer1/peer1.conf")" 2>/dev/null; then
206206
CAN_CREATE_PEERS="true"
207207
else
208208
die "No peer directories or templates under $CONFIG_DIR (cannot create missing peers)"
209209
fi
210210

211211
peer_config_exists() {
212-
local peer="$1" conf="$CONFIG_DIR/$peer/$peer.conf"
213-
ssh_cmd bash -c "test -f $(remote_quote "$conf")" 2>/dev/null
212+
local peer="$1"
213+
local conf="$CONFIG_DIR/$peer/$peer.conf"
214+
ssh_cmd "test -f $(remote_quote "$conf")" 2>/dev/null
214215
}
215216

216217
# ---- Flock-guarded peer allocation/record on the jumpserver ----------------
@@ -221,17 +222,17 @@ atomic_allocate_peers() {
221222
local force_wg0="${WG0_PEER:-__none__}"
222223
local force_wg1="${WG1_PEER:-__none__}"
223224
ssh_cmd bash -s -- \
224-
$(remote_quote "$ASSIGNED_FILE") \
225-
$(remote_quote "$ASSIGN_LOCK_FILE") \
226-
$(remote_quote "$CONFIG_DIR") \
227-
$(remote_quote "$ENV_NAME") \
228-
$(remote_quote "$LABEL") \
229-
$(remote_quote "$MAX_PEERS") \
230-
$(remote_quote "$force_tf") \
231-
$(remote_quote "$force_wg0") \
232-
$(remote_quote "$force_wg1") \
233-
$(remote_quote "$DRY_RUN") \
234-
$(remote_quote "$CAN_CREATE_PEERS") \
225+
"$(remote_quote "$ASSIGNED_FILE")" \
226+
"$(remote_quote "$ASSIGN_LOCK_FILE")" \
227+
"$(remote_quote "$CONFIG_DIR")" \
228+
"$(remote_quote "$ENV_NAME")" \
229+
"$(remote_quote "$LABEL")" \
230+
"$(remote_quote "$MAX_PEERS")" \
231+
"$(remote_quote "$force_tf")" \
232+
"$(remote_quote "$force_wg0")" \
233+
"$(remote_quote "$force_wg1")" \
234+
"$(remote_quote "$DRY_RUN")" \
235+
"$(remote_quote "$CAN_CREATE_PEERS")" \
235236
<<'REMOTE_ATOMIC_ALLOCATE'
236237
set -euo pipefail
237238
@@ -555,14 +556,14 @@ atomic_rollback_assignments() {
555556
local record_tf="$1" record_wg0="$2" record_wg1="$3"
556557
[[ "$record_tf" == "true" || "$record_wg0" == "true" || "$record_wg1" == "true" ]] || return 0
557558
ssh_cmd bash -s -- \
558-
$(remote_quote "$ASSIGNED_FILE") \
559-
$(remote_quote "$ASSIGN_LOCK_FILE") \
560-
$(remote_quote "$TF_PEER") \
561-
$(remote_quote "$WG0_PEER") \
562-
$(remote_quote "$WG1_PEER") \
563-
$(remote_quote "$record_tf") \
564-
$(remote_quote "$record_wg0") \
565-
$(remote_quote "$record_wg1") \
559+
"$(remote_quote "$ASSIGNED_FILE")" \
560+
"$(remote_quote "$ASSIGN_LOCK_FILE")" \
561+
"$(remote_quote "$TF_PEER")" \
562+
"$(remote_quote "$WG0_PEER")" \
563+
"$(remote_quote "$WG1_PEER")" \
564+
"$(remote_quote "$record_tf")" \
565+
"$(remote_quote "$record_wg0")" \
566+
"$(remote_quote "$record_wg1")" \
566567
<<'REMOTE_ROLLBACK_ASSIGNMENTS'
567568
set -euo pipefail
568569
@@ -600,21 +601,26 @@ REMOTE_ROLLBACK_ASSIGNMENTS
600601
}
601602

602603
rollback_published_secrets() {
603-
local count="$1" i
604+
local count="$1" i failed="false"
604605
[[ "$count" -gt 0 ]] || return 0
605606
for ((i = 0; i < count; i++)); do
606607
if gh secret delete "${SECRET_NAMES[$i]}" --env "$ENV_NAME" --repo "$REPO" 2>/dev/null; then
607608
log "Deleted secret ${SECRET_NAMES[$i]}"
608609
else
609610
err "Failed to delete secret ${SECRET_NAMES[$i]}"
611+
failed="true"
610612
fi
611613
done
614+
[[ "$failed" != "true" ]]
612615
}
613616

614617
rollback_allocation_state() {
615618
local published_count="$1"
616619
err "Rolling back published secrets and assignments for $ENV_NAME ..."
617-
rollback_published_secrets "$published_count"
620+
if ! rollback_published_secrets "$published_count"; then
621+
err "Skipping assignment rollback because at least one GitHub secret could not be deleted; keep $ASSIGNED_FILE reserved and clean up manually."
622+
return 0
623+
fi
618624
atomic_rollback_assignments "$RECORD_TF" "$RECORD_WG0" "$RECORD_WG1" \
619625
|| err "Assignment rollback failed; fix $ASSIGNED_FILE manually for TF=$TF_PEER WG0=$WG0_PEER WG1=$WG1_PEER"
620626
}
@@ -667,12 +673,13 @@ transform_conf() {
667673
}
668674

669675
fetch_and_transform() {
670-
local peer="$1" raw conf="$CONFIG_DIR/$peer/$peer.conf"
676+
local peer="$1" raw
677+
local conf="$CONFIG_DIR/$peer/$peer.conf"
671678
if [[ "$DRY_RUN" == "true" ]] && ! peer_config_exists "$peer"; then
672679
echo "[DRY RUN] peer config for $peer not present yet"
673680
return 0
674681
fi
675-
raw="$(ssh_cmd bash -c "cat $(remote_quote "$conf") 2>/dev/null || sudo cat $(remote_quote "$conf")" 2>/dev/null || true)"
682+
raw="$(ssh_cmd "cat $(remote_quote "$conf") 2>/dev/null || sudo cat $(remote_quote "$conf")" 2>/dev/null || true)"
676683
[[ -n "$raw" ]] || die "Could not read $CONFIG_DIR/$peer/$peer.conf"
677684
grep -q '^[[:space:]]*AllowedIPs' <<<"$raw" || die "$peer.conf has no AllowedIPs line"
678685
transform_conf <<<"$raw"
@@ -740,9 +747,10 @@ exec 8>"${ALLOCATION_FILE}.lock"
740747
if ! flock -w 30 8; then
741748
die "Timed out waiting to update repo tracker lock ${ALLOCATION_FILE}.lock"
742749
fi
743-
TRACKER_TMP="$(mktemp)"
750+
TRACKER_TMP="$(mktemp "$(dirname "$ALLOCATION_FILE")/.wg-peer-allocation.XXXXXX")"
744751
awk -F '\t' -v env="$ENV_NAME" 'NR == 1 || $1 != env' "$ALLOCATION_FILE" > "$TRACKER_TMP"
745752
printf '%s\t%s\t%s\t%s\t%s\n' "$ENV_NAME" "$TF_PEER" "$WG0_PEER" "$WG1_PEER" "$(date -u +%Y-%m-%dT%H:%M:%SZ)" >> "$TRACKER_TMP"
753+
chmod --reference="$ALLOCATION_FILE" "$TRACKER_TMP" 2>/dev/null || true
746754
mv "$TRACKER_TMP" "$ALLOCATION_FILE"
747755
TRACKER_TMP=""
748756

0 commit comments

Comments
 (0)