[MOSIP-39951] Updated keycloak init scripts with keycloakExternalHost…#199
[MOSIP-39951] Updated keycloak init scripts with keycloakExternalHost…#199bhumi46 wants to merge 1 commit intomosip:developfrom
Conversation
…, keycloakInternalHost, and frontendUrl configurations. Fixed YAML indentation and removed non-existent sa_client_roles. Added helpful comments for client secrets management. Signed-off-by: bhumi46 <thisisbn46@gmail.com>
WalkthroughThe changes modify Keycloak configuration and Helm deployment scripts across four deployment files. Key updates include restructuring realm roles, introducing client scopes and secrets, renaming role references from plural to singular forms, and refactoring Helm installation commands with additional host configuration parameters. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
deploy/upgrade-init.sh (1)
2-4:⚠️ Potential issue | 🟡 MinorStale usage comment — still references
import-init.sh.The header says
# ./import-init.sh [kube_config_file]but this file isupgrade-init.sh.Proposed fix
-# Initialize Imported Keycloak with MOSIP base data +# Upgrade Keycloak with MOSIP base data # Usage: -# ./import-init.sh [kube_config_file] +# ./upgrade-init.sh [kube_config_file]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/upgrade-init.sh` around lines 2 - 4, The usage comment at the top of upgrade-init.sh is stale and still references import-init.sh; update the header comment to reflect the correct script name and usage (e.g., change "# ./import-init.sh [kube_config_file]" to "# ./upgrade-init.sh [kube_config_file]" or a generic "Usage: ./upgrade-init.sh [kube_config_file]") so the file-level comment in upgrade-init.sh accurately describes how to invoke this script.deploy/import-init-values.yaml (1)
214-223:⚠️ Potential issue | 🟡 MinorDuplicate
mosip-creser-idpass-clientdefinition (same issue as upgrade file).This client appears at lines 214–223 and again at lines 383–392 with identical content. Remove one of the duplicates.
Also applies to: 383-392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/import-init-values.yaml` around lines 214 - 223, Remove the duplicate client definition for "mosip-creser-idpass-client" in deploy/import-init-values.yaml by keeping a single entry with its mappers and saroles and deleting the redundant block; ensure the remaining block preserves the exact mappers: [] and saroles list (REGISTRATION_PROCESSOR, DATA_READ, DOCUMENT_READ, BIOMETRIC_READ, METADATA_READ, CREATE_SHARE, CREDENTIAL_REQUEST) and scan the file for any other identical duplicates (including the upgrade-related section) so only one canonical definition remains.
🧹 Nitpick comments (2)
deploy/import-init.sh (1)
21-25:helm installwill fail if the release already exists; considerhelm upgrade --install.
upgrade-init.shuseshelm upgrade --install(idempotent), but this script uses plainhelm install, which will error out ifkeycloak-importis already installed (e.g., a partial re-run). Usingupgrade --installwould make the script re-runnable.Proposed fix
- helm -n $NS install keycloak-import mosip/keycloak-init \ + helm -n $NS upgrade --install keycloak-import mosip/keycloak-init \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/import-init.sh` around lines 21 - 25, The helm invocation uses a plain "helm install keycloak-import ..." which will fail if the release already exists; change the command to use "helm upgrade --install keycloak-import ..." (keep the same release name "keycloak-import", chart "mosip/keycloak-init" and all existing --set flags and -f import-init-values.yaml --version $CHART_VERSION) so the script becomes idempotent and can be re-run safely.deploy/upgrade-init.sh (1)
21-25: Duplicated helm command block — extract into a helper function.The helm install invocation with
keycloakExternalHost,keycloakInternalHost, andfrontendUrlis repeated identically across both steps (and also inimport-init.sh). A helper function would reduce duplication and the risk of the parameters drifting out of sync.Also applies to: 34-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/upgrade-init.sh` around lines 21 - 25, Extract the repeated helm upgrade/install invocation into a single helper function (e.g., run_keycloak_helm) that accepts no args and uses the existing variables (NS, IAM_HOST, KEYCLOAK_SERVICE_NAME, CHART_VERSION) to run: helm -n $NS upgrade --install keycloak-init-upgrade mosip/keycloak-init with --set keycloakExternalHost="$IAM_HOST", --set keycloakInternalHost="$KEYCLOAK_SERVICE_NAME.$NS", --set keycloak.realms.mosip.realm_config.attributes.frontendUrl="https://$IAM_HOST/auth", -f upgrade-init-values.yaml, --version $CHART_VERSION and --wait; then replace the duplicated helm blocks in this file (and the copy in import-init.sh) with a single call to run_keycloak_helm, ensuring environment variables used by the original command remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/import-init-values.yaml`:
- Around line 3-5: The del_realms value is nested under realms_to_delete which
conflicts with the expected schema used elsewhere (see
upgrade-init-values.yaml); update the deploy/import-init-values.yaml to align
with the keycloak-init chart by making del_realms a flat list (remove the
intermediate realms_to_delete key) so the chart sees del_realms as the list of
realm names (e.g., include "preregistration" directly under del_realms) or, if
the chart actually expects the nested form, update upgrade-init-values.yaml
instead—ensure both files use the same structure for the del_realms key.
- Around line 518-572: The clientSecrets block is out of sync with the clients
list: add entries for mosip_partner_client_secret and
mosip_partnermanager_client_secret (so their secrets are present), and reconcile
mpartner_default_template_secret (either remove it from clientSecrets or ensure
a corresponding client exists) so every secret name (e.g.,
mosip_partner_client_secret, mosip_partnermanager_client_secret,
mpartner_default_template_secret) matches a defined client; update the
clientSecrets array accordingly to mirror the clients list exactly.
In `@deploy/import-init.sh`:
- Line 18: The kubectl invocation that sets IAM_HOST uses an unquoted jsonpath
expression which can be misinterpreted by the shell; update the assignment that
sets IAM_HOST (the line containing IAM_HOST and the kubectl get cm global -o
jsonpath=...) to pass the jsonpath argument in quotes (e.g., wrap the
{.data.mosip-iam-external-host} expression in single or double quotes) so the
braces are not interpreted by the shell.
In `@deploy/upgrade-init-values.yaml`:
- Around line 2-4: Both files incorrectly structure the Realm deletion settings:
replace the current del_realms list/nested mapping with a boolean flag and move
the realm names into a sibling list named realms_to_delete; specifically, set
del_realms to true and create a sibling key realms_to_delete containing the
existing realm entries (e.g., preregistration), and remove any nesting of
realms_to_delete under del_realms so del_realms and realms_to_delete are peers;
reference keys: del_realms and realms_to_delete.
In `@deploy/upgrade-init.sh`:
- Line 18: The shell assignment to IAM_HOST uses an unquoted JSONPath expression
which triggers SC1083; update the kubectl invocation that sets IAM_HOST (the
line using kubectl get cm global -o jsonpath=...) to quote the JSONPath
expression (e.g., wrap {.data.mosip-iam-external-host} in single or double
quotes) so the shell parses it correctly and avoids word-splitting or brace
interpretation errors.
- Around line 27-31: The kubectl wait command currently swallows failures via
"|| true", letting the script proceed to "helm -n $NS uninstall
keycloak-init-upgrade" and subsequent import even if the upgrade job failed;
remove the "|| true" and instead detect failure of the kubectl wait (the kubectl
wait invocation that targets the job label
app.kubernetes.io/instance=keycloak-init-upgrade) and exit non-zero (or log an
explicit error and prompt for confirmation) so the script fails fast and does
not run helm uninstall or the import when the upgrade didn't complete; ensure
the handling references the same NS variable and provides a clear log message on
failure.
---
Outside diff comments:
In `@deploy/import-init-values.yaml`:
- Around line 214-223: Remove the duplicate client definition for
"mosip-creser-idpass-client" in deploy/import-init-values.yaml by keeping a
single entry with its mappers and saroles and deleting the redundant block;
ensure the remaining block preserves the exact mappers: [] and saroles list
(REGISTRATION_PROCESSOR, DATA_READ, DOCUMENT_READ, BIOMETRIC_READ,
METADATA_READ, CREATE_SHARE, CREDENTIAL_REQUEST) and scan the file for any other
identical duplicates (including the upgrade-related section) so only one
canonical definition remains.
In `@deploy/upgrade-init.sh`:
- Around line 2-4: The usage comment at the top of upgrade-init.sh is stale and
still references import-init.sh; update the header comment to reflect the
correct script name and usage (e.g., change "# ./import-init.sh
[kube_config_file]" to "# ./upgrade-init.sh [kube_config_file]" or a generic
"Usage: ./upgrade-init.sh [kube_config_file]") so the file-level comment in
upgrade-init.sh accurately describes how to invoke this script.
---
Nitpick comments:
In `@deploy/import-init.sh`:
- Around line 21-25: The helm invocation uses a plain "helm install
keycloak-import ..." which will fail if the release already exists; change the
command to use "helm upgrade --install keycloak-import ..." (keep the same
release name "keycloak-import", chart "mosip/keycloak-init" and all existing
--set flags and -f import-init-values.yaml --version $CHART_VERSION) so the
script becomes idempotent and can be re-run safely.
In `@deploy/upgrade-init.sh`:
- Around line 21-25: Extract the repeated helm upgrade/install invocation into a
single helper function (e.g., run_keycloak_helm) that accepts no args and uses
the existing variables (NS, IAM_HOST, KEYCLOAK_SERVICE_NAME, CHART_VERSION) to
run: helm -n $NS upgrade --install keycloak-init-upgrade mosip/keycloak-init
with --set keycloakExternalHost="$IAM_HOST", --set
keycloakInternalHost="$KEYCLOAK_SERVICE_NAME.$NS", --set
keycloak.realms.mosip.realm_config.attributes.frontendUrl="https://$IAM_HOST/auth",
-f upgrade-init-values.yaml, --version $CHART_VERSION and --wait; then replace
the duplicated helm blocks in this file (and the copy in import-init.sh) with a
single call to run_keycloak_helm, ensuring environment variables used by the
original command remain unchanged.
| del_realms: | ||
| - preregistration | ||
| mosip: # realm | ||
| realms_to_delete: | ||
| - preregistration |
There was a problem hiding this comment.
Suspicious del_realms → realms_to_delete nesting.
The structure here is:
del_realms:
realms_to_delete:
- preregistrationThis creates a mapping del_realms containing a sub-key realms_to_delete, which differs from upgrade-init-values.yaml where del_realms is a flat list. Please verify the schema the keycloak-init chart expects and align both files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/import-init-values.yaml` around lines 3 - 5, The del_realms value is
nested under realms_to_delete which conflicts with the expected schema used
elsewhere (see upgrade-init-values.yaml); update the
deploy/import-init-values.yaml to align with the keycloak-init chart by making
del_realms a flat list (remove the intermediate realms_to_delete key) so the
chart sees del_realms as the list of realm names (e.g., include
"preregistration" directly under del_realms) or, if the chart actually expects
the nested form, update upgrade-init-values.yaml instead—ensure both files use
the same structure for the del_realms key.
| clientSecrets: | ||
| - name: mosip_abis_client_secret | ||
| secret: "" | ||
| - name: mosip_admin_client_secret | ||
| secret: "" | ||
| - name: mosip_admin_services_client_secret | ||
| secret: "" | ||
| - name: mosip_auth_client_secret | ||
| secret: "" | ||
| - name: mosip_crereq_client_secret | ||
| secret: "" | ||
| - name: mosip_creser_client_secret | ||
| secret: "" | ||
| - name: mosip_datsha_client_secret | ||
| secret: "" | ||
| - name: mosip_ida_client_secret | ||
| secret: "" | ||
| - name: mosip_misp_client_secret | ||
| secret: "" | ||
| - name: mosip_pms_client_secret | ||
| secret: "" | ||
| - name: mosip_policymanager_client_secret | ||
| secret: "" | ||
| - name: mosip_reg_client_secret | ||
| secret: "" | ||
| - name: mosip_regproc_client_secret | ||
| secret: "" | ||
| - name: mosip_resident_client_secret | ||
| secret: "" | ||
| - name: mosip_prereg_client_secret | ||
| secret: "" | ||
| - name: mosip_creser_idpass_client_secret | ||
| secret: "" | ||
| - name: mosip_syncdata_client_secret | ||
| secret: "" | ||
| - name: mosip_deployment_client_secret | ||
| secret: "" | ||
| - name: mpartner_default_auth_secret | ||
| secret: "" | ||
| - name: mosip_idrepo_client_secret | ||
| secret: "" | ||
| - name: mpartner_default_print_secret | ||
| secret: "" | ||
| - name: mosip_hotlist_client_secret | ||
| secret: "" | ||
| - name: mpartner_default_mobile_secret | ||
| secret: "" | ||
| - name: mosip_digitalcard_client_secret | ||
| secret: "" | ||
| - name: mpartner_default_digitalcard_secret | ||
| secret: "" | ||
| - name: mosip_testrig_client_secret | ||
| secret: "" | ||
| - name: mpartner_default_template_secret | ||
| secret: "" No newline at end of file |
There was a problem hiding this comment.
Same clientSecrets gaps as upgrade-init-values.yaml.
mosip_partner_client_secret and mosip_partnermanager_client_secret are missing for their respective clients (lines 245, 274). mpartner_default_template_secret (line 571) has no corresponding client definition. Please sync these with the clients list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/import-init-values.yaml` around lines 518 - 572, The clientSecrets
block is out of sync with the clients list: add entries for
mosip_partner_client_secret and mosip_partnermanager_client_secret (so their
secrets are present), and reconcile mpartner_default_template_secret (either
remove it from clientSecrets or ensure a corresponding client exists) so every
secret name (e.g., mosip_partner_client_secret,
mosip_partnermanager_client_secret, mpartner_default_template_secret) matches a
defined client; update the clientSecrets array accordingly to mirror the clients
list exactly.
| helm repo add mosip https://mosip.github.io/mosip-helm | ||
| helm repo update | ||
|
|
||
| IAM_HOST=$(kubectl get cm global -o jsonpath={.data.mosip-iam-external-host}) |
There was a problem hiding this comment.
Quote the JSONPath expression to avoid shell interpretation of braces.
ShellCheck SC1083: the { and } in the jsonpath template are unquoted and may be interpreted literally or cause unexpected behavior depending on the shell. Wrap the argument in quotes.
Proposed fix
- IAM_HOST=$(kubectl get cm global -o jsonpath={.data.mosip-iam-external-host})
+ IAM_HOST=$(kubectl get cm global -o jsonpath='{.data.mosip-iam-external-host}')📝 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.
| IAM_HOST=$(kubectl get cm global -o jsonpath={.data.mosip-iam-external-host}) | |
| IAM_HOST=$(kubectl get cm global -o jsonpath='{.data.mosip-iam-external-host}') |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 18-18: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 18-18: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/import-init.sh` at line 18, The kubectl invocation that sets IAM_HOST
uses an unquoted jsonpath expression which can be misinterpreted by the shell;
update the assignment that sets IAM_HOST (the line containing IAM_HOST and the
kubectl get cm global -o jsonpath=...) to pass the jsonpath argument in quotes
(e.g., wrap the {.data.mosip-iam-external-host} expression in single or double
quotes) so the braces are not interpreted by the shell.
| realms: | ||
| del_realms: | ||
| - preregistration |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine both files
echo "=== upgrade-init-values.yaml ===" && \
cat -n deploy/upgrade-init-values.yaml 2>/dev/null || echo "File not found"
echo -e "\n=== import-init-values.yaml ===" && \
cat -n deploy/import-init-values.yaml 2>/dev/null || echo "File not found"
echo -e "\n=== Searching for del_realms/realms_to_delete usage ===" && \
rg -rn 'del_realms|realms_to_delete' deploy/ --type yamlRepository: mosip/keycloak
Length of output: 1727
🌐 Web query:
keycloak-init helm chart del_realms realms_to_delete MOSIP
💡 Result:
In MOSIP’s keycloak-init tooling, the Helm values:
del_realms: a boolean flag that tells thekeycloak_init.pylogic to run the delete-realms step.realms_to_delete: the list of Keycloak realm names that should be deleted whendel_realms: true.
This capability exists because MOSIP added methods in keycloak_init.py to delete realms (along with related cleanup actions) as part of its keycloak-init enhancements. (issues.ecosyste.ms)
A typical values snippet looks like:
del_realms: true
realms_to_delete:
- realm1
- realm2Caution: deleting realms is destructive (removes realm config/users/clients under that realm). Also, MOSIP notes that keycloak-init generally adds/updates and that “cleanup” may otherwise require manual action/resetting persistence, so use realm deletion intentionally. (docs.mosip.io)
Citations:
- 1: https://issues.ecosyste.ms/hosts/GitHub/repositories/mosip%2Fkeycloak/issues?utm_source=openai
- 2: https://docs.mosip.io/platform/build-and-deploy/sandbox-installer?utm_source=openai
Both files have incorrect del_realms structure.
According to MOSIP keycloak-init specification, del_realms should be a boolean flag and realms_to_delete should be a sibling list:
del_realms: true
realms_to_delete:
- preregistration- upgrade-init-values.yaml (lines 3–4) incorrectly treats
del_realmsas a list. - import-init-values.yaml (lines 3–5) incorrectly nests
realms_to_deleteunderdel_realms.
Update both files to match the specification above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/upgrade-init-values.yaml` around lines 2 - 4, Both files incorrectly
structure the Realm deletion settings: replace the current del_realms
list/nested mapping with a boolean flag and move the realm names into a sibling
list named realms_to_delete; specifically, set del_realms to true and create a
sibling key realms_to_delete containing the existing realm entries (e.g.,
preregistration), and remove any nesting of realms_to_delete under del_realms so
del_realms and realms_to_delete are peers; reference keys: del_realms and
realms_to_delete.
| helm repo add mosip https://mosip.github.io/mosip-helm | ||
| helm repo update | ||
|
|
||
| IAM_HOST=$(kubectl get cm global -o jsonpath={.data.mosip-iam-external-host}) |
There was a problem hiding this comment.
Quote the JSONPath expression (same SC1083 issue as import-init.sh).
Proposed fix
- IAM_HOST=$(kubectl get cm global -o jsonpath={.data.mosip-iam-external-host})
+ IAM_HOST=$(kubectl get cm global -o jsonpath='{.data.mosip-iam-external-host}')📝 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.
| IAM_HOST=$(kubectl get cm global -o jsonpath={.data.mosip-iam-external-host}) | |
| IAM_HOST=$(kubectl get cm global -o jsonpath='{.data.mosip-iam-external-host}') |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 18-18: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 18-18: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/upgrade-init.sh` at line 18, The shell assignment to IAM_HOST uses an
unquoted JSONPath expression which triggers SC1083; update the kubectl
invocation that sets IAM_HOST (the line using kubectl get cm global -o
jsonpath=...) to quote the JSONPath expression (e.g., wrap
{.data.mosip-iam-external-host} in single or double quotes) so the shell parses
it correctly and avoids word-splitting or brace interpretation errors.
| echo Waiting for upgrade job to complete... | ||
| kubectl wait --for=condition=complete --timeout=600s -n $NS job -l app.kubernetes.io/instance=keycloak-init-upgrade || true | ||
|
|
||
| echo Cleaning up upgrade release | ||
| helm -n $NS uninstall keycloak-init-upgrade |
There was a problem hiding this comment.
|| true silently ignores upgrade job failure — the script proceeds to uninstall and import even if the upgrade didn't complete.
If the upgrade job times out or fails, || true swallows the error and the script continues to uninstall the release (line 31) and run the import (line 34). This can leave Keycloak in a partially upgraded state that the import then builds on, potentially causing inconsistencies. Consider failing fast or at least logging a warning and prompting before continuing.
Proposed fix — fail if upgrade job doesn't complete
echo Waiting for upgrade job to complete...
- kubectl wait --for=condition=complete --timeout=600s -n $NS job -l app.kubernetes.io/instance=keycloak-init-upgrade || true
+ if ! kubectl wait --for=condition=complete --timeout=600s -n $NS job -l app.kubernetes.io/instance=keycloak-init-upgrade; then
+ echo "ERROR: Upgrade job did not complete successfully. Aborting."
+ exit 1
+ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/upgrade-init.sh` around lines 27 - 31, The kubectl wait command
currently swallows failures via "|| true", letting the script proceed to "helm
-n $NS uninstall keycloak-init-upgrade" and subsequent import even if the
upgrade job failed; remove the "|| true" and instead detect failure of the
kubectl wait (the kubectl wait invocation that targets the job label
app.kubernetes.io/instance=keycloak-init-upgrade) and exit non-zero (or log an
explicit error and prompt for confirmation) so the script fails fast and does
not run helm uninstall or the import when the upgrade didn't complete; ensure
the handling references the same NS variable and provides a clear log message on
failure.
1 participant
…, keycloakInternalHost, and frontendUrl configurations. Fixed YAML indentation and removed non-existent sa_client_roles. Added helpful comments for client secrets management.
Summary by CodeRabbit
Release Notes
New Features
Chores