[Fixes: mosip/mosip-infra#1895] Added script to register/import Ranch…#267
Open
Ivanmeneges wants to merge 7 commits into
Open
[Fixes: mosip/mosip-infra#1895] Added script to register/import Ranch…#267Ivanmeneges wants to merge 7 commits into
Ivanmeneges wants to merge 7 commits into
Conversation
…er cluster via API This script allows the infra pipeline to register or import a downstream cluster in Rancher using the API without manual intervention. It handles the creation of the cluster and its registration token, providing the necessary import command for Terraform. Signed-off-by: Ivanmeneges <ivan.anil016@gmail.com>
Contributor
WalkthroughThe PR adds a Rancher registration script that resolves an import command for a named cluster and wires it into the Terraform workflow behind optional dispatch inputs. When enabled, the workflow exports Terraform variables from the generated command. ChangesRancher Import Flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant Script
participant RancherAPI
participant Terraform
Workflow->>Script: run registration step when enabled
Script->>RancherAPI: lookup cluster and poll registration tokens
RancherAPI-->>Script: import command data
Script-->>Workflow: quoted import URL
Workflow->>Terraform: export TF_VAR_rancher_import_url and TF_VAR_enable_rancher_import
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/scripts/rancher-register-cluster.sh (1)
64-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a request timeout to
curl.
api()has no--max-time(or--connect-timeout). A hung Rancher endpoint can block a poll attempt indefinitely, and in CI this stalls the pipeline rather than failing fast and retrying.♻️ Suggested change
- local args=(-fsSL -X "$method" + local args=(-fsSL --connect-timeout 10 --max-time 30 -X "$method" -H "Authorization: Bearer ${RANCHER_TOKEN}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/rancher-register-cluster.sh around lines 64 - 73, The api() helper in rancher-register-cluster.sh currently calls curl without any timeout, so a stalled Rancher request can hang the poll loop indefinitely. Update api() to pass a curl timeout option such as --max-time and/or --connect-timeout, keeping the existing method/path/body handling and the optional INSECURE flag intact, so failed requests return promptly and the retry logic can continue.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/rancher-register-cluster.sh:
- Around line 45-54: The argument parsing in rancher-register-cluster.sh can
dereference an unbound $2 under set -u when --rancher-url, --token, or
--cluster-name is provided without a value. Update the while/case handling to
use a safe fallback such as ${2:-} before assigning in the RANCHER_URL,
RANCHER_TOKEN, and CLUSTER_NAME branches, then route missing values through the
existing validation/error path so die reports the friendly usage message instead
of a shell abort.
- Around line 154-173: The polling loop in rancher-register-cluster.sh can still
exit early under set -euo pipefail when fetch_registration_token_list or the jq
token count command fails, even though the loop is meant to retry. Update the
retry block in the polling loop to tolerate transient API/pipeline failures by
guarding the TOKEN_LIST assignment and TOKEN_COUNT computation the same way
read_token_fields_from_list is already guarded, while keeping
token_fields_ready, try_create_registration_token, and the existing retry flow
unchanged.
- Around line 176-192: The import command normalization in
rancher-register-cluster.sh still leaves the --insecure branch in a
Terraform-invalid form because INSECURE_CMD is preserved as a curl-pipe command
instead of a plain manifest URL. Update the logic around IMPORT_CMD so both the
INSECURE_CMD and normal paths resolve to the same quoted kubectl apply -f
https://...yaml shape using manifestUrl/token (or normalize the final string
before output), ensuring the value matches the regex expected by
terraform/infra/variables.tf.
- Around line 141-149: The optional registration token creation in
try_create_registration_token is using the singular clusterregistrationtoken
endpoint, which makes the POST path ineffective and hides real HTTP errors.
Update the api POST call to use the plural clusterregistrationtokens collection
endpoint so it matches the GET collection path and actually attempts creation;
keep the existing logging in place for accepted versus skipped/denied outcomes.
---
Nitpick comments:
In @.github/scripts/rancher-register-cluster.sh:
- Around line 64-73: The api() helper in rancher-register-cluster.sh currently
calls curl without any timeout, so a stalled Rancher request can hang the poll
loop indefinitely. Update api() to pass a curl timeout option such as --max-time
and/or --connect-timeout, keeping the existing method/path/body handling and the
optional INSECURE flag intact, so failed requests return promptly and the retry
logic can continue.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 42b6c978-653c-43a5-b332-48797bbb0349
📒 Files selected for processing (1)
.github/scripts/rancher-register-cluster.sh
Added inputs for Rancher cluster registration in Terraform workflow. Signed-off-by: Ivanmeneges <ivan.anil016@gmail.com>
Guard flag parsing under set -u, use plural registration-token POST path, retry on transient API failures, normalize import URL for Terraform, and fix workflow script path under .github/scripts/. Signed-off-by: ivan <ivan.anil016@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/terraform.yml (2)
372-375: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider passing the Rancher token via env instead of a CLI argument.
RANCHER_TOKENis already exported as an env var (Line 363) but is also passed as--token "$RANCHER_TOKEN"on the command line, which can leak via process listings (ps//proc). Since the script receivesRANCHER_TOKENin its environment already, prefer having it read the token from env directly and drop the--tokenflag, consistent with how other Rancher automation in this repo consumesRANCHER_TOKENfrom env.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/terraform.yml around lines 372 - 375, The Rancher token is being passed twice, and the CLI argument in the import command can expose secrets via process listings. Update the terraform workflow command that calls rancher-register-cluster.sh so it relies on the already-exported RANCHER_TOKEN environment variable and removes the --token argument. Make sure the script entrypoint and any token handling in rancher-register-cluster.sh continue to read the token from env, consistent with the rest of the Rancher automation.
376-377: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate
IMPORT_CMDbefore exporting Terraform variables.If the registration script succeeds (exit 0) but emits an empty last line,
TF_VAR_rancher_import_urlwill be set to an empty string whileTF_VAR_enable_rancher_importis unconditionally set totrue, leaving Terraform with an inconsistent/invalid import config. Consider assertingIMPORT_CMDis non-empty (and optionally matches the expectedkubectl apply -f ...shape) before writing toGITHUB_ENV.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/terraform.yml around lines 376 - 377, The workflow step that exports Terraform vars is accepting an empty IMPORT_CMD, which can leave TF_VAR_rancher_import_url invalid while TF_VAR_enable_rancher_import is still set. Update the shell logic around the IMPORT_CMD handling to verify it is non-empty before writing either variable to GITHUB_ENV, and only enable rancher import when the command looks valid. Keep the fix localized to the terraform workflow step that captures registration output and exports TF_VAR_rancher_import_url / TF_VAR_enable_rancher_import.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/terraform.yml:
- Around line 359-379: The `Generate Rancher import URL via API` step is
vulnerable to template/shell injection because `github.ref_name` and
`inputs.RANCHER_CLUSTER_NAME` are interpolated directly inside the `run:`
script. Move both values into `env:` on the same step, then use shell variables
in the script for `CLUSTER_NAME` and the error message instead of raw `${{ ...
}}` expressions. Keep the fix localized to this workflow step and the
`rancher-register-cluster.sh` invocation so the bash parser never sees untrusted
input as part of the script text.
---
Nitpick comments:
In @.github/workflows/terraform.yml:
- Around line 372-375: The Rancher token is being passed twice, and the CLI
argument in the import command can expose secrets via process listings. Update
the terraform workflow command that calls rancher-register-cluster.sh so it
relies on the already-exported RANCHER_TOKEN environment variable and removes
the --token argument. Make sure the script entrypoint and any token handling in
rancher-register-cluster.sh continue to read the token from env, consistent with
the rest of the Rancher automation.
- Around line 376-377: The workflow step that exports Terraform vars is
accepting an empty IMPORT_CMD, which can leave TF_VAR_rancher_import_url invalid
while TF_VAR_enable_rancher_import is still set. Update the shell logic around
the IMPORT_CMD handling to verify it is non-empty before writing either variable
to GITHUB_ENV, and only enable rancher import when the command looks valid. Keep
the fix localized to the terraform workflow step that captures registration
output and exports TF_VAR_rancher_import_url / TF_VAR_enable_rancher_import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6b65708a-01ad-4d05-8d8c-aa22e98569df
📒 Files selected for processing (2)
.github/scripts/rancher-register-cluster.sh.github/workflows/terraform.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/scripts/rancher-register-cluster.sh
Pass ref name and cluster name through env vars instead of interpolating user-controlled values directly into the run script. Signed-off-by: ivan <ivan.anil016@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
URL-encode query params, build JSON bodies with jq, surface Rancher error responses, add curl timeouts, validate URL and cluster name, detect duplicate cluster matches, and require a Terraform-compatible import URL before retry exit. Signed-off-by: ivan <ivan.anil016@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid duplicate cluster creation under parallel runs, safe array reads, temp file cleanup, and validate import URLs match the configured Rancher host. Signed-off-by: ivan <ivan.anil016@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Capture the mktemp path when registering the RETURN trap so cleanup does not reference an unbound local variable in the caller scope. Signed-off-by: ivan <ivan.anil016@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/rancher-register-cluster.sh:
- Around line 220-226: The try_create_registration_token() flow is leaking the
token-create response because api POST writes the registration-token JSON to
stdout. Update the call site in try_create_registration_token so the POST
response is suppressed or redirected while still preserving the success/failure
check, and keep stdout reserved for the final command line only. Use the
existing api helper and json_registration_token_body invocation to locate the
fix.
- Around line 287-290: Failure diagnostics in the token summary are leaking
import credentials because `jq` prints `.status.command` and
`.status.manifestUrl` from `TOKEN_LIST`. Update the diagnostic block in
`rancher-register-cluster.sh` to redact or omit those fields before logging,
while keeping the rest of the summary from the same `TOKEN_LIST`/`jq` path so
failures still show useful context without exposing `/v3/import/<token>.yaml`
URLs.
- Around line 189-199: The token selection logic in pick_best_token_json should
prefer a ready token over the default-token fallback. Update the jq selection so
it first returns the newest entry from $d with a non-empty status.manifestUrl,
status.command, or status.token, and only falls back to default-token if no
ready token exists. Keep the behavior centered in pick_best_token_json so the
polling flow uses the best available token data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8db3ec07-238b-4741-b4d0-fcbcbf97918d
📒 Files selected for processing (2)
.github/scripts/rancher-register-cluster.sh.github/workflows/terraform.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/terraform.yml
Comment on lines
+189
to
+199
| pick_best_token_json() { | ||
| local list="$1" | ||
| jq -c ' | ||
| (.data // []) as $d | | ||
| if ($d | length) == 0 then empty | ||
| else | ||
| ([$d[] | select(.name == "default-token")] | .[0]) // | ||
| ([$d[] | select((.status.manifestUrl // .status.command // .status.token // "") != "")] | .[-1]) // | ||
| $d[-1] | ||
| end | ||
| ' <<<"$list" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prefer ready token data before the default-token fallback.
pick_best_token_json currently selects default-token even when its status fields are empty, so a ready non-default token can be ignored and the poll can fail unnecessarily.
Suggested fix
pick_best_token_json() {
local list="$1"
jq -c '
+ def usable:
+ ((.status.manifestUrl // .manifestUrl //
+ .status.command // .command //
+ .status.insecureCommand // .insecureCommand //
+ .status.token // .token // "") != "");
(.data // []) as $d |
if ($d | length) == 0 then empty
else
- ([$d[] | select(.name == "default-token")] | .[0]) //
- ([$d[] | select((.status.manifestUrl // .status.command // .status.token // "") != "")] | .[-1]) //
+ ([$d[] | select((.name == "default-token") and usable)] | .[0]) //
+ ([$d[] | select(usable)] | .[-1]) //
+ ([$d[] | select(.name == "default-token")] | .[0]) //
$d[-1]
end
' <<<"$list"
}📝 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
| pick_best_token_json() { | |
| local list="$1" | |
| jq -c ' | |
| (.data // []) as $d | | |
| if ($d | length) == 0 then empty | |
| else | |
| ([$d[] | select(.name == "default-token")] | .[0]) // | |
| ([$d[] | select((.status.manifestUrl // .status.command // .status.token // "") != "")] | .[-1]) // | |
| $d[-1] | |
| end | |
| ' <<<"$list" | |
| pick_best_token_json() { | |
| local list="$1" | |
| jq -c ' | |
| def usable: | |
| ((.status.manifestUrl // .manifestUrl // | |
| .status.command // .command // | |
| .status.insecureCommand // .insecureCommand // | |
| .status.token // .token // "") != ""); | |
| (.data // []) as $d | | |
| if ($d | length) == 0 then empty | |
| else | |
| ([$d[] | select((.name == "default-token") and usable)] | .[0]) // | |
| ([$d[] | select(usable)] | .[-1]) // | |
| ([$d[] | select(.name == "default-token")] | .[0]) // | |
| $d[-1] | |
| end | |
| ' <<<"$list" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/rancher-register-cluster.sh around lines 189 - 199, The
token selection logic in pick_best_token_json should prefer a ready token over
the default-token fallback. Update the jq selection so it first returns the
newest entry from $d with a non-empty status.manifestUrl, status.command, or
status.token, and only falls back to default-token if no ready token exists.
Keep the behavior centered in pick_best_token_json so the polling flow uses the
best available token data.
Comment on lines
+220
to
+226
| try_create_registration_token() { | ||
| log "Attempting to create a registration token (optional) ..." | ||
| if api POST "/v3/clusterregistrationtokens" "$(json_registration_token_body)"; then | ||
| log "Registration token create request accepted" | ||
| else | ||
| log "Registration token create skipped or denied (default-token may already exist)" | ||
| fi |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Discard the token-create API body from stdout.
api POST emits the created registration-token JSON, which can include import command/token material and also violates the script’s stdout contract before the final command line.
Suggested fix
try_create_registration_token() {
log "Attempting to create a registration token (optional) ..."
- if api POST "/v3/clusterregistrationtokens" "$(json_registration_token_body)"; then
+ if api POST "/v3/clusterregistrationtokens" "$(json_registration_token_body)" >/dev/null; then
log "Registration token create request accepted"
else
log "Registration token create skipped or denied (default-token may already exist)"
fi
}📝 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
| try_create_registration_token() { | |
| log "Attempting to create a registration token (optional) ..." | |
| if api POST "/v3/clusterregistrationtokens" "$(json_registration_token_body)"; then | |
| log "Registration token create request accepted" | |
| else | |
| log "Registration token create skipped or denied (default-token may already exist)" | |
| fi | |
| try_create_registration_token() { | |
| log "Attempting to create a registration token (optional) ..." | |
| if api POST "/v3/clusterregistrationtokens" "$(json_registration_token_body)" >/dev/null; then | |
| log "Registration token create request accepted" | |
| else | |
| log "Registration token create skipped or denied (default-token may already exist)" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/rancher-register-cluster.sh around lines 220 - 226, The
try_create_registration_token() flow is leaking the token-create response
because api POST writes the registration-token JSON to stdout. Update the call
site in try_create_registration_token so the POST response is suppressed or
redirected while still preserving the success/failure check, and keep stdout
reserved for the final command line only. Use the existing api helper and
json_registration_token_body invocation to locate the fix.
Comment on lines
+287
to
+290
| err "Could not determine import command. Token summary:" | ||
| if [[ -n "$TOKEN_LIST" ]]; then | ||
| jq '{count: (.data|length), tokens: [.data[]? | {name, command: .status.command, manifestUrl: .status.manifestUrl, token: (if .status.token then "set" else "empty" end)}]}' \ | ||
| <<<"$TOKEN_LIST" >&2 || true |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact import URLs in failure diagnostics.
.status.command and .status.manifestUrl can contain the registration token in /v3/import/<token>.yaml; printing them to stderr can expose import credentials in CI logs.
Suggested fix
err "Could not determine import command. Token summary:"
if [[ -n "$TOKEN_LIST" ]]; then
- jq '{count: (.data|length), tokens: [.data[]? | {name, command: .status.command, manifestUrl: .status.manifestUrl, token: (if .status.token then "set" else "empty" end)}]}' \
+ jq '
+ def redact:
+ if type == "string" then
+ gsub("/v3/import/[^[:space:]\"|]+\\.yaml"; "/v3/import/<redacted>.yaml")
+ else . end;
+ {count: (.data|length), tokens: [.data[]? | {
+ name,
+ command: (.status.command | redact),
+ manifestUrl: (.status.manifestUrl | redact),
+ token: (if .status.token then "set" else "empty" end)
+ }]}
+ ' \
<<<"$TOKEN_LIST" >&2 || true
fi📝 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
| err "Could not determine import command. Token summary:" | |
| if [[ -n "$TOKEN_LIST" ]]; then | |
| jq '{count: (.data|length), tokens: [.data[]? | {name, command: .status.command, manifestUrl: .status.manifestUrl, token: (if .status.token then "set" else "empty" end)}]}' \ | |
| <<<"$TOKEN_LIST" >&2 || true | |
| err "Could not determine import command. Token summary:" | |
| if [[ -n "$TOKEN_LIST" ]]; then | |
| jq ' | |
| def redact: | |
| if type == "string" then | |
| gsub("/v3/import/[^[:space:]\"|]+\\.yaml"; "/v3/import/<redacted>.yaml") | |
| else . end; | |
| {count: (.data|length), tokens: [.data[]? | { | |
| name, | |
| command: (.status.command | redact), | |
| manifestUrl: (.status.manifestUrl | redact), | |
| token: (if .status.token then "set" else "empty" end) | |
| }]} | |
| ' \ | |
| <<<"$TOKEN_LIST" >&2 || true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/rancher-register-cluster.sh around lines 287 - 290, Failure
diagnostics in the token summary are leaking import credentials because `jq`
prints `.status.command` and `.status.manifestUrl` from `TOKEN_LIST`. Update the
diagnostic block in `rancher-register-cluster.sh` to redact or omit those fields
before logging, while keeping the rest of the summary from the same
`TOKEN_LIST`/`jq` path so failures still show useful context without exposing
`/v3/import/<token>.yaml` URLs.
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
1 participant
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.
…er cluster via API
This script allows the infra pipeline to register or import a downstream cluster in Rancher using the API without manual intervention. It handles the creation of the cluster and its registration token, providing the necessary import command for Terraform.
Summary by CodeRabbit