[MOSIP-43143]Updated esignet DSF and docs#148
[MOSIP-43143]Updated esignet DSF and docs#148bhumi46 wants to merge 88 commits intomosip:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds eSignet deployment automation: new GitHub Actions workflows (deploy, cleanup, destroy enhancements), a Helmsman DSF manifest for eSignet, many pre/post‑install and utility hook scripts, helper utilities and Helm values, partner-data cleanup tooling, and extensive docs and diagrams for deployment, teardown, and verification. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant GHA as GitHub Actions
participant Helm as Helmsman
participant Hooks as Hooks
participant K8s as Kubernetes
participant DB as Postgres/Redis/SoftHSM
User->>GHA: trigger `helmsman_esignet` (workflow_dispatch)
activate GHA
GHA->>GHA: validate inputs & required secrets
GHA->>Helm: run Helmsman apply (esignet-dsf.yaml)
activate Helm
Helm->>Hooks: run pre-install hooks
activate Hooks
Hooks->>K8s: create namespaces, copy secrets/configmaps, remove immutable Jobs
Hooks-->>Helm: preinstall complete
deactivate Hooks
Helm->>K8s: install Helm releases in priority order
K8s->>DB: provision Postgres/Redis/SoftHSM
DB-->>K8s: services ready
K8s->>K8s: deploy Keycloak, eSignet, mock services
Helm->>Hooks: run post-install hooks
activate Hooks
Hooks->>K8s: poll jobs, extract keys/IDs, create/sync secrets, patch env, restart deployments
Hooks-->>Helm: postinstall complete
deactivate Hooks
Helm-->>GHA: publish deployment summary
deactivate Helm
GHA-->>User: success/failure report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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 |
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <111699703+bhumi46@users.noreply.github.com> Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
…specific syntax
Scripts using 'function' keyword require bash, not POSIX sh.
This fixes the 'Syntax error: "(" unexpected' error in CI.
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Added '|| true' to helm delete and '--ignore-not-found=true' to kubectl delete to prevent script failure when resources don't exist. Signed-off-by: bhumi46 <thisisbn46@gmail.com>
1. Strip Helm metadata annotations when copying secrets to prevent ownership conflicts between postgres-init-* releases 2. Add protected: true to keycloak app to prevent deletion 3. Add existence checks for secrets before copying Signed-off-by: bhumi46 <thisisbn46@gmail.com>
1. Change postgres-init-mockidentitysystem priority to -15 (runs after esignet at -16) 2. Add new mockidentitysystem-init-db.sh hook that doesn't copy secrets 3. This prevents both releases from trying to manage the same db-common-secrets Signed-off-by: bhumi46 <thisisbn46@gmail.com>
1. Change postgres-init-mockidentitysystem priority to -15 (runs after esignet at -16) 2. Add new mockidentitysystem-init-db.sh hook that doesn't copy secrets 3. This prevents both releases from trying to manage the same db-common-secrets Signed-off-by: bhumi46 <thisisbn46@gmail.com>
The postgres-init Helm chart creates db-common-secrets itself. Copying it in the hook causes ownership conflict. Only copy postgres-postgresql (connection credentials) in the hook. Signed-off-by: bhumi46 <thisisbn46@gmail.com>
…em into one release Merged both postgres-init releases into a single 'postgres-init' release that initializes both mosip_esignet and mosip_mockidentitysystem databases. This eliminates the Helm secret ownership conflict for db-common-secrets. Signed-off-by: bhumi46 <thisisbn46@gmail.com>
…ents Helmsman hooks are called without arguments. Changed to read ESIGNET_CAPTCHA_SITE_KEY and ESIGNET_CAPTCHA_SECRET_KEY from environment variables which are set by GitHub Actions from secrets. Signed-off-by: bhumi46 <thisisbn46@gmail.com>
…ents Helmsman hooks are called without arguments. Changed to read ESIGNET_CAPTCHA_SITE_KEY and ESIGNET_CAPTCHA_SECRET_KEY from environment variables which are set by GitHub Actions from secrets. Signed-off-by: bhumi46 <thisisbn46@gmail.com>
After copying db-common-secrets from postgres namespace, add the required Helm labels and annotations so the postgres-init chart can adopt it: - app.kubernetes.io/managed-by=Helm - meta.helm.sh/release-name=postgres-init - meta.helm.sh/release-namespace=esignet Signed-off-by: bhumi46 <thisisbn46@gmail.com>
…ents Helmsman hooks are called without arguments. Changed to read ESIGNET_CAPTCHA_SITE_KEY and ESIGNET_CAPTCHA_SECRET_KEY from environment variables which are set by GitHub Actions from secrets. Signed-off-by: bhumi46 <thisisbn46@gmail.com>
- Add step to fetch db-common-secrets from postgres namespace - Mask password in GitHub Actions logs - Pass password to Helmsman via environment variable - Handles first deployment case when secret doesn't exist Signed-off-by: bhumi46 <thisisbn46@gmail.com>
- Add step to fetch db-common-secrets from postgres namespace - Mask password in GitHub Actions logs - Pass password to Helmsman via environment variable - Handles first deployment case when secret doesn't exist Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: Bhuminathan M <bn46@Bhuminathans-MacBook-Air.local> Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Helmsman/utils/copy-cm-and-secrets/copy_cm_func.sh (1)
9-9:⚠️ Potential issue | 🟡 MinorUnquoted
$1in test expression will cause a syntax error if no arguments are passed.-if [ $1 = "configmap" ] +if [ "$1" = "configmap" ]Same applies to all other positional parameter references (
$2,$3,$4,$5) on lines 34-38..github/workflows/helmsman_external_destroy_prereq.yml (1)
154-159:⚠️ Potential issue | 🟠 Major
cattle-monitoring-systemadded to verification (line 206) but not to the destruction list (line 158).The "all namespaces" destroy path on line 158 still only includes
cattle-logging-system istio-system istio-operator httpbin. The verification step will check whethercattle-monitoring-systemwas removed, but it was never targeted for deletion. Either add it to the destroy list or remove it from the verification list.Proposed fix
- NAMESPACES=(cattle-logging-system istio-system istio-operator httpbin) + NAMESPACES=(cattle-logging-system istio-system istio-operator httpbin cattle-monitoring-system)Also applies to: 205-207
🤖 Fix all issues with AI agents
In @.github/workflows/cleanup-partner-data.yml:
- Around line 59-65: The "Validate confirmation" step currently interpolates the
workflow input directly into the shell which allows command injection via
github.event.inputs.confirm_deletion; change this by first assigning the input
to a safe environment variable (e.g., via env: CONFIRM: ${{
github.event.inputs.confirm_deletion }} or a preceding shell assignment like
CONFIRM="${{ github.event.inputs.confirm_deletion }}" ) and then reference only
$CONFIRM inside the run block; apply the same pattern for the other inputs
(db_host, db_port, db_user) used in run blocks so they are passed as environment
variables (e.g., env: DB_HOST: ${{ github.event.inputs.db_host }}) and
referenced as $DB_HOST to eliminate direct ${ { github.event.inputs.* } }
interpolation in shell commands.
In @.github/workflows/helmsman_esignet.yml:
- Around line 24-26: The workflow currently triggers on push for the file path
and the "Set Default Mode" step uses github.event.inputs.mode which is empty on
push and falls back to "apply", causing accidental auto-deploys; update the
workflow .github/workflows/helmsman_esignet.yml by either removing the push:
paths: - Helmsman/dsf/esignet-dsf.yaml block entirely (rely only on
workflow_dispatch) or change the "Set Default Mode" logic to detect
github.event_name == 'push' and default github.event.inputs.mode to "dry-run"
for push events (keep the existing behavior for workflow_dispatch), ensuring you
reference the existing step that computes mode (the "Set Default Mode" step and
usage of github.event.inputs.mode) when making the change.
In `@docs/CLEANUP_PARTNER_DATA_GUIDE.md`:
- Line 97: The markdown references a missing image using the path
'../_images/cleanup-partner-data.png' via the image tag ``; either add the image file named
cleanup-partner-data.png into the _images directory (so the relative path
resolves) or remove/replace that markdown image tag from
CLEANUP_PARTNER_DATA_GUIDE.md with an existing image or alternative text/link.
In `@docs/esignet_README.md`:
- Line 35: The KUBECONFIG documentation is inconsistent: docs refers to
KUBECONFIG as "Base64 encoded" while the main README shows the raw YAML (e.g.,
the example starting with "apiVersion: v1..."); update the docs to state
explicitly which format is required, make both docs consistent, and provide
clear instructions for each option (e.g., "Provide raw kubeconfig YAML" or
"Provide base64-encoded kubeconfig using cat ~/.kube/config | base64 -w 0") so
consumers know to either paste the plain YAML or the base64 string for the
KUBECONFIG value; ensure the KUBECONFIG label and the example block match
exactly across the READMEs.
In `@Helmsman/hooks/cleanup-partner-data.sh`:
- Around line 79-156: Wrap each group of DELETEs for a single database in an
explicit transaction: for the set of execute_sql calls targeting "mosip_pms"
start a transaction (BEGIN), run all related DELETE statements (the blocks
deleting from auth_policy, policy_group, partner_policy_request, partner,
misp_license, oidc_client), and COMMIT at the end; if any execute_sql fails,
ROLLBACK and exit non‑zero. Do the same for the "mosip_esignet" block around the
client_detail deletes. Use the existing execute_sql invocation to run
BEGIN/COMMIT/ROLLBACK or perform them as first/last statements and ensure you
check the return status of execute_sql after each statement so you can ROLLBACK
on failure.
In `@Helmsman/hooks/esignet-demo-oidc-partner-onboarder-postinstall.sh`:
- Line 134: The ENCODED_KEY assignment uses plain base64 which may insert
newlines and break the JSON patch; change the encoding command that sets
ENCODED_KEY (which reads from PRIVATE_PUBLIC_KEY_PAIR) to produce a single-line
base64 string (e.g., use base64 with no wrapping like --wrap=0 or -w0, or an
equivalent option such as openssl base64 -A) so the resulting value can be
safely embedded in the kubectl patch JSON without embedded newlines.
In `@Helmsman/hooks/esignet-demo-oidc-partner-onboarder-preinstall.sh`:
- Around line 58-60: The sed replacement in the pipeline that runs on the
ConfigMap retrieved as "onboarder-namespace" is too broad (the pattern
s/name:.*/name: esignet-onboarder-namespace/g) and will replace every "name:"
entry in the YAML; update the pipeline so only metadata.name is changed: use a
JSON/YAML-aware tool (jq/yq) to set .metadata.name to
"esignet-onboarder-namespace" when operating on the ConfigMap (keeping the
kubectl get ... and kubectl apply -f - steps and the NS variable the same) or,
if you must use sed, restrict the match to the metadata.name field only (anchor
to the metadata/name path) so labels/annotations/data keys are not modified.
In `@Helmsman/hooks/esignet-partner-onboarder-preinstall.sh`:
- Line 55: The script currently prints the S3 secret value via the echo call
echo "s3-user-key found: $S3_USER_KEY", which leaks credentials to CI logs;
change that line to avoid expanding or printing $S3_USER_KEY and instead emit a
non-sensitive message (e.g., "s3-user-key found") matching the behavior in
esignet-demo-oidc-partner-onboarder-preinstall.sh, ensuring no secret values are
logged.
In `@Helmsman/hooks/esignet-postinstall-keycloak-init.sh`:
- Around line 39-47: The config-server secret sync can write empty values if the
source secrets in the $NS namespace are missing; after populating
ESIGNET_PMS_SECRET and ESIGNET_MPARTNER_SECRET (the variables derived from
PMS_CLIENT_SECRET_KEY and MPARTNER_DEFAULT_AUTH_SECRET_KEY), add a guard to
verify they are non-empty before piping into kubectl apply — if either
ESIGNET_PMS_SECRET or ESIGNET_MPARTNER_SECRET is empty, skip the jq/apply step
(or exit with an error) and log a clear message; update the conditional logic
around the ESIGNET_* assignments and the kubectl|jq|kubectl pipeline to prevent
applying blank values to the secret in config-server.
- Around line 21-29: The script currently reads ESIGNET_PMS_SECRET and
ESIGNET_MPARTNER_SECRET and unconditionally injects them into the keycloak
secret, which will overwrite target keys with empty values if the source is
missing; update the logic in the block that sets
ESIGNET_PMS_SECRET/ESIGNET_MPARTNER_SECRET to validate each fetched value (check
that ESIGNET_PMS_SECRET and ESIGNET_MPARTNER_SECRET are non-empty) and only
include non-empty keys in the kubectl + jq pipeline (or abort with an error/exit
non-zero) so that KEYCLOAK_PMS_SECRET and KEYCLOAK_MPARTNER_SECRET are never
replaced by empty strings; reference the ESIGNET_* variables and the kubectl ...
jq ... kubectl apply pipeline when implementing the conditional update.
- Line 1: Add strict error handling to the esignet-postinstall-keycloak-init.sh
script by enabling shell errexit/nounset/pipefail (e.g., set -e; set -o errexit;
set -o nounset; set -o pipefail) at the top and add explicit checks after
critical commands (kubectl and jq pipelines) so failures don't get silently
ignored; ensure the kubectl get | jq | kubectl apply pipeline checks exit codes
or captures errors and exits with a descriptive message, and validate existence
of source secrets before attempting the sync.
In `@Helmsman/hooks/esignet-preinstall-keycloak-init.sh`:
- Around line 19-22: The current lines export PMS_CLIENT_SECRET_VALUE and
MPARTNER_DEFAULT_AUTH_SECRET_VALUE using command substitutions which hide
non-zero failures from kubectl/base64; split each into a separate assignment and
export so failures surface: set PMS_CLIENT_SECRET_KEY and
PMS_CLIENT_SECRET_VALUE with PMS_CLIENT_SECRET_VALUE="$(kubectl -n keycloak get
secrets keycloak-client-secrets -o jsonpath="{.data.${PMS_CLIENT_SECRET_KEY}}" |
base64 -d")" then export PMS_CLIENT_SECRET_KEY and PMS_CLIENT_SECRET_VALUE (and
do the same split for MPARTNER_DEFAULT_AUTH_SECRET_KEY /
MPARTNER_DEFAULT_AUTH_SECRET_VALUE) so that kubectl/base64 errors cause the
script to fail under set -e; reference symbols: PMS_CLIENT_SECRET_KEY,
PMS_CLIENT_SECRET_VALUE, MPARTNER_DEFAULT_AUTH_SECRET_KEY,
MPARTNER_DEFAULT_AUTH_SECRET_VALUE in esignet-preinstall-keycloak-init.sh.
In `@Helmsman/hooks/softhsm-esignet-postinstall.sh`:
- Around line 25-32: The jq idempotency checks are comparing against the wrong
env var names so the script always re-applies the env; update the selectors used
when setting ESIGNET_HOST_ENV and the softhsm security-pin check to match the
actual env var names produced by `kubectl set env` (which preserve the original
key casing/hyphens after the SPRING_CLOUD_CONFIG_SERVER_OVERRIDES_ prefix).
Specifically, change the jq select strings currently looking for
"SPRING_CLOUD_CONFIG_SERVER_OVERRIDES_MOSIP_ESIGNET_HOST" and
"SPRING_CLOUD_CONFIG_SERVER_OVERRIDES_SOFTHSM_ESIGNET_SECURITY_PIN" to the
actual names created by the set env command (e.g.
"SPRING_CLOUD_CONFIG_SERVER_OVERRIDES_mosip-esignet-host" and
"SPRING_CLOUD_CONFIG_SERVER_OVERRIDES_SOFTHSM_ESIGNET_security-pin") so the
ESIGNET_HOST_ENV and the softhsm pin env existence checks correctly detect and
skip already-present vars.
In `@Helmsman/hooks/softhsm-esignet-setup.sh`:
- Around line 11-12: The script defines COPY_UTIL and claims to "syncs secrets
to config-server" but never uses it; update setup_softhsm_esignet to either call
the copy utility or remove the stale variable/comment. Specifically, within the
setup_softhsm_esignet function (and referencing SOFTHSM_NS), invoke COPY_UTIL
(the copy_cm_func.sh) to sync the relevant secrets/configmaps to the
config-server, adding checks for the file's existence, making it executable, and
handling errors (log/exit) if the copy fails; alternatively, if syncing is out
of scope, remove COPY_UTIL and update the header comment to reflect that the
function only labels/creates the namespace. Ensure references use the COPY_UTIL,
setup_softhsm_esignet, and SOFTHSM_NS symbols so reviewers can find the change.
In `@Helmsman/hooks/softhsm-mock-identity-system-preinstall.sh`:
- Line 18: The script labels the namespace using the SOFTHSM_NS variable but
never ensures the namespace exists first; update the preinstall script to create
or ensure the namespace identified by SOFTHSM_NS in an idempotent way (e.g., use
kubectl create namespace with a dry-run/apply or a safe existence check) before
calling kubectl label ns $SOFTHSM_NS istio-injection=enabled --overwrite so the
labeling cannot fail if the namespace is absent.
In `@Helmsman/utils/copy-cm-and-secrets/copy_cm_func.sh`:
- Around line 20-30: The strip_metadata function currently uses broad grep -v
patterns (e.g., grep -v "uid:") that will remove any line containing those
substrings and can drop legitimate data; update strip_metadata to match only
metadata keys (anchored/field-prefixed patterns) or, preferably, parse the YAML
and remove metadata fields with a YAML-aware tool (e.g., yq/jq) so only metadata
keys like metadata.uid, metadata.creationTimestamp, metadata.resourceVersion,
metadata.annotations["meta.helm.sh/release-name"], etc. are removed; locate the
strip_metadata function and replace the brittle grep -v lines with precise
anchored regexes (e.g., match lines starting with " uid:" or "^uid:") or a
yq-based transformation to surgically delete metadata keys and perform the
namespace substitution safely.
🟡 Minor comments (17)
Helmsman/hooks/redis-setup.sh-1-3 (1)
1-3:⚠️ Potential issue | 🟡 MinorMissing kubeconfig argument handling, inconsistent with sibling scripts.
Same issue as
oidc-ui-preinstall.sh— other hooks accept an optional[kubeconfig]argument.Helmsman/hooks/oidc-ui-preinstall.sh-1-3 (1)
1-3:⚠️ Potential issue | 🟡 MinorMissing kubeconfig argument handling, inconsistent with sibling scripts.
All other hook scripts accept an optional
[kubeconfig]argument, but this one does not. If the workflow passes a kubeconfig path, this script will silently ignore it.Proposed fix
#!/bin/bash +# Pre-install hook for oidc-ui +# This script copies configmaps to the esignet namespace +## Usage: ./oidc-ui-preinstall.sh [kubeconfig] + +if [ $# -ge 1 ] ; then + export KUBECONFIG="$1" +fi DST_NS=esignetHelmsman/hooks/softhsm-mock-identity-system-postinstall.sh-30-35 (1)
30-35:⚠️ Potential issue | 🟡 MinorConfig-server is restarted unconditionally, even when no changes were made.
This contradicts the idempotency claim in the header. If the security-pin already exists (line 27 branch), the restart on line 32 is unnecessary and causes a service disruption. Move the restart inside the
ifblock.Proposed fix
if [ -z "$SOFTHSM_MOCK_PIN_ENV" ]; then echo "Adding softhsm-mock-identity-system security-pin to config-server" kubectl -n config-server set env --keys=security-pin --from secret/softhsm-mock-identity-system deployment/config-server --prefix=SPRING_CLOUD_CONFIG_SERVER_OVERRIDES_SOFTHSM_MOCK_IDENTITY_SYSTEM_ + + # Restart config-server deployment to pick up new softhsm configuration + echo "Restarting config-server to pick up softhsm configuration" + kubectl -n config-server rollout restart deploy/config-server + echo "Config-server restart initiated, continuing deployment..." else echo "softhsm-mock-identity-system security-pin already exists in config-server, skipping" fi - # Restart config-server deployment to pick up new softhsm configuration - echo "Restarting config-server to pick up softhsm configuration" - kubectl -n config-server rollout restart deploy/config-server - - # Config-server restart initiated - continuing with deployment - echo "Config-server restart initiated, continuing deployment..."docs/_images/eSignet_DSF_Architecture_Flow.drawio-166-168 (1)
166-168:⚠️ Potential issue | 🟡 MinorInfo box contains incomplete text: "pkcs12 as optional" with trailing empty
<div>.The info box value appears to have draft/leftover content. The phrase "pkcs12 as optional" lacks context, and there's an empty
<div>element after it. Clean this up or expand it into a meaningful description.Helmsman/hooks/softhsm-esignet-postinstall.sh-43-51 (1)
43-51:⚠️ Potential issue | 🟡 MinorMissing
kubectl rollout status— script claims to wait but doesn't.Lines 47-48 comment and echo "Waiting for config-server to be ready" but no actual wait is performed. If downstream hooks depend on config-server being ready, this could cause race conditions.
Proposed fix
# Restart config-server deployment to pick up new softhsm configuration echo "Restarting config-server to pick up softhsm configuration" kubectl -n config-server rollout restart deploy/config-server # Wait for config-server rollout echo "Waiting for config-server to be ready" - - # Config-server restart initiated - continuing with deployment - echo "Config-server configured, continuing deployment..." + kubectl -n config-server rollout status deploy/config-server --timeout=300s + echo "Config-server is ready"Helmsman/hooks/mock-relying-party-service-preinstall.sh-22-22 (1)
22-22:⚠️ Potential issue | 🟡 Minor
WORKDIRmust be set externally; no guard or default is provided.If
WORKDIRis unset,COPY_UTILresolves to a broken path. Since this line executes beforeset -o nounset(line 92), it won't error on the unset variable — it will silently produce an incorrect path. Consider adding a guard:: "${WORKDIR:?WORKDIR must be set}"Helmsman/utils/keycloak-init-values.yaml-41-48 (1)
41-48:⚠️ Potential issue | 🟡 MinorCopy-paste descriptions:
get_certificateandsending_binding_otpreuse theadd_oidc_clientdescription.Lines 42 and 58 both say "Scope required to create OIDC client", which doesn't describe
get_certificateorsending_binding_otp. These appear to be copy-paste oversights..github/workflows/README.md-549-555 (1)
549-555:⚠️ Potential issue | 🟡 MinorInaccurate claim: "Private keys passed as environment variables (never written to disk)".
Line 552 states keys are never written to disk, but
mock-relying-party-service-preinstall.shwrites decoded keys to/tmp/client-private-keyand/tmp/jwe-userinfo-private-keybefore creating the Kubernetes secrets. Update the documentation to reflect this, or update the script to avoid writing to disk (e.g., pipe directly tokubectl).Helmsman/hooks/mock-relying-party-service-preinstall.sh-50-53 (1)
50-53:⚠️ Potential issue | 🟡 MinorPrivate key material written to predictable
/tmppath.
/tmp/client-private-keyand/tmp/jwe-userinfo-private-key(line 72) are world-readable predictable paths. In a shared runner environment, another process could read or symlink-attack these files between write and delete. Consider usingmktempfor a unique, safely-created temporary file.Proposed fix (apply similarly to line 72-75)
- echo "$MOCK_RELYING_PARTY_CLIENT_PRIVATE_KEY" | base64 -d | sed "s/'//g" | sed -z 's/\n/\\n/g' > /tmp/client-private-key + TMPFILE=$(mktemp) + echo "$MOCK_RELYING_PARTY_CLIENT_PRIVATE_KEY" | base64 -d | sed "s/'//g" | sed -z 's/\n/\\n/g' > "$TMPFILE" kubectl -n $NS delete secret mock-relying-party-service-secrets --ignore-not-found=true - kubectl -n $NS create secret generic mock-relying-party-service-secrets --from-file=client-private-key=/tmp/client-private-key - rm -f /tmp/client-private-key + kubectl -n $NS create secret generic mock-relying-party-service-secrets --from-file=client-private-key="$TMPFILE" + rm -f "$TMPFILE"Helmsman/hooks/esignet-preinstall-keycloak-init.sh-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorUsage comment mentions
[kubeconfig]parameter but it's never handled.The postinstall counterpart (
esignet-postinstall-keycloak-init.sh) handles$1as a kubeconfig path (lines 8-10). This script's usage comment documents the same interface but the code never reads$1. Either add the handling or remove the usage comment to avoid confusion..github/workflows/helmsman_mosip_destroy.yml-53-53 (1)
53-53:⚠️ Potential issue | 🟡 MinorTypo in namespace list:
mastedata-loader→masterdata-loader.Proposed fix
- all_namespaces: "conf-secrets admin apitestrig artifactory config-server dslrig esignet idrepo kernel masterdata packetmanager pms prereg print regproc resident mosip-file-server mock-smtp keymanager ida digitalcard datashare biosdk abis regclient websub mastedata-loader onboarder redis" + all_namespaces: "conf-secrets admin apitestrig artifactory config-server dslrig esignet idrepo kernel masterdata packetmanager pms prereg print regproc resident mosip-file-server mock-smtp keymanager ida digitalcard datashare biosdk abis regclient websub masterdata-loader onboarder redis"docs/esignet_README.md-70-70 (1)
70-70:⚠️ Potential issue | 🟡 MinorBroken relative link to reCAPTCHA setup guide.
From
docs/esignet_README.md, the path../../../docs/RECAPTCHA_SETUP_GUIDE.mdnavigates three levels up fromdocs/, which is incorrect. It should be a sibling path.Proposed fix
-> **Note:** For detailed instructions on creating Google reCAPTCHA keys with screenshots, see [RECAPTCHA Setup Guide](../../../docs/RECAPTCHA_SETUP_GUIDE.md). +> **Note:** For detailed instructions on creating Google reCAPTCHA keys with screenshots, see [RECAPTCHA Setup Guide](RECAPTCHA_SETUP_GUIDE.md).README.md-1250-1295 (1)
1250-1295:⚠️ Potential issue | 🟡 MinorDuplicate step numbering: both eSignet and Test Rigs are numbered "5."
Line 1250 introduces "5. Deploy eSignet" and line 1295 has "5. Deploy Test Rigs". The test rigs step should be renumbered to "6."
Proposed fix
-5. **Deploy Test Rigs (Manual):** +6. **Deploy Test Rigs (Manual):**.github/workflows/helmsman_esignet.yml-120-125 (1)
120-125:⚠️ Potential issue | 🟡 MinorHelm install script fetched from unpinned
masterbranch.
https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3can change at any time. A breaking change or compromise in the upstream script would silently affect your CI. Pin to a specific commit or tag.Proposed fix: pin to a known Helm release tag
- curl -fsSL -o get_helm.sh https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3 + curl -fsSL -o get_helm.sh https://raw.githubusercontent.com/helm/helm/v3.17.1/scripts/get-helm-3Replace
v3.17.1with whatever Helm version you want to target..github/workflows/helmsman_esignet.yml-52-60 (1)
52-60:⚠️ Potential issue | 🟡 MinorMasking empty secrets will mask empty strings in all log output.
If any of these secrets are not configured,
${{ secrets.* }}resolves to an empty string. Calling::add-mask::with an empty string causes GitHub Actions to attempt masking empty strings throughout all subsequent log lines, which can produce garbled output. Guard each mask call:Proposed fix (example for one secret, apply to all)
- echo "::add-mask::${{ secrets.MOCK_RELYING_PARTY_CLIENT_PRIVATE_KEY }}" + if [ -n "${{ secrets.MOCK_RELYING_PARTY_CLIENT_PRIVATE_KEY }}" ]; then + echo "::add-mask::${{ secrets.MOCK_RELYING_PARTY_CLIENT_PRIVATE_KEY }}" + fiApply the same pattern to all six
add-masklines.Helmsman/hooks/esignet-partner-onboarder-postinstall.sh-179-196 (1)
179-196:⚠️ Potential issue | 🟡 MinorHardcoded
sleep 210+sleep 120(5.5 min total) is fragile; log message is placed after the first sleep.Three issues:
sleep 210(line 184) comes before the echo on line 186, so there's 3.5 minutes of silence in logs with no indication of what's happening. Move the echo before the sleep.Both sleeps should be replaced with
kubectl rollout statusfor deterministic waits.Config-server is unconditionally restarted even when nothing changed (lines 179-183), adding unnecessary downtime. Consider skipping the restart if no secrets were actually updated.
Proposed fix: use rollout status and fix log ordering
# If config was not changed, still restart config-server to pick up any secret changes if [ "$CONFIG_CHANGED" = "false" ]; then echo "No new env vars added, but restarting config-server to pick up any secret changes..." kubectl -n config-server rollout restart deploy/config-server fi - sleep 210 - # Config-server restart initiated - continuing with deployment echo "Config-server restart initiated, waiting for rollout to complete..." + kubectl -n config-server rollout status deployment/config-server --timeout=300s # Restart esignet deployment to pick up new secrets echo "Restarting esignet deployment" kubectl rollout restart deployment -n $NS esignet 2>/dev/null || echo "esignet deployment not found, skipping restart" # Restart resident deployment to pick up new secrets (if exists) echo "Restarting resident deployment (if exists)" kubectl rollout restart deployment -n resident resident 2>/dev/null || echo "resident deployment not found, skipping restart" - sleep 120 + kubectl -n $NS rollout status deployment/esignet --timeout=180s 2>/dev/null || true + kubectl -n resident rollout status deployment/resident --timeout=180s 2>/dev/null || trueHelmsman/hooks/esignet-preinstall.sh-105-146 (1)
105-146:⚠️ Potential issue | 🟡 MinorUnused
CURRENT_GENERATIONvariable and brittlesleep 240instead of rollout status check.Two issues here:
CURRENT_GENERATION(Line 106) is assigned but never referenced — it appears to be leftover from an incomplete implementation that would compare generation before/after env changes.
sleep 240(Line 142) is a fixed 4-minute wait that is fragile: it can be too short if the rollout is slow, or wastefully long if the rollout completes quickly. Usekubectl rollout statusfor a deterministic wait.Proposed fix: replace sleep with rollout status
- # Get current generation before making any changes - CURRENT_GENERATION=$(kubectl get deployment config-server -n config-server -o jsonpath='{.metadata.generation}' 2>/dev/null || echo "0") ENV_CHANGES_MADE=falseif [ "$ENV_CHANGES_MADE" = true ]; then echo "Environment variables added, waiting for config-server to restart..." - sleep 240 # Wait 4 minutes for config-server rollout to complete - echo "Config-server should be ready now" + kubectl -n config-server rollout status deployment/config-server --timeout=300s + echo "Config-server rollout completed" else echo "No environment variable changes needed" fi
🧹 Nitpick comments (19)
Helmsman/hooks/pre-helmsman-cleanup.sh (2)
13-15: Quote variables to prevent word splitting and globbing.
$NSand$1should be quoted throughout the script. Whileesignetis safe today, unquoted variables are a shell anti-pattern that can break if values ever change or contain spaces.Proposed fix (representative — apply same pattern to all `$NS` usages)
-if [ $# -ge 1 ] ; then - export KUBECONFIG=$1 +if [ $# -ge 1 ] ; then + export KUBECONFIG="$1" fi
35-37: Fixedsleep 10is fragile for waiting on pod termination.Consider using
kubectl wait --for=deletewith a timeout instead of a fixed sleep, which may be too short under load or unnecessarily slow otherwise.Suggested improvement
-# Wait for pods to terminate -echo "Waiting for job pods to terminate..." -sleep 10 +# Wait for job pods to terminate +echo "Waiting for job pods to terminate..." +kubectl -n "$NS" wait --for=delete job -l "app.kubernetes.io/name=partner-onboarder" --timeout=60s 2>/dev/null || trueHelmsman/hooks/softhsm-esignet-setup.sh (1)
26-27:set -eandset -o errexitare redundant.
set -eis equivalent toset -o errexit. This pattern is repeated across all hook scripts in this PR. Consider keeping just one for clarity.Helmsman/hooks/redis-setup.sh (1)
7-23: Script is not idempotent —kubectl set envandrollout restartrun unconditionally.Unlike
softhsm-mock-identity-system-postinstall.shwhich checks for existing env vars before modifying the deployment, this script always sets the env var and restarts config-server. Consider adding an existence check (as done in the softhsm postinstall script) to avoid unnecessary deployment restarts.Helmsman/hooks/esignet-demo-oidc-partner-onboarder-preinstall.sh (1)
14-81: Significant duplication withesignet-partner-onboarder-preinstall.sh.This script shares ~80% of its logic (namespace creation, Istio labeling, job cleanup, configmap/secret copying, s3-user-key handling) with
esignet-partner-onboarder-preinstall.sh. Consider extracting the shared steps into a common helper function to reduce maintenance burden.Helmsman/hooks/cleanup-partner-data.sh (1)
42-58: Address SC2155: Declare and assigncountseparately to avoid masking return values.Per shellcheck SC2155, combining
localwith command substitution masks the exit code.Proposed fix
local count_sql="${sql/DELETE/SELECT COUNT(*)}" - local count=$(PGPASSWORD="$DB_PASSWORD" psql -h "$DB_HOST" -p "$DB_PORT" -U "$DB_USER" -d "$database" -t -c "$count_sql" 2>/dev/null | xargs || echo "0") + local count + count=$(PGPASSWORD="$DB_PASSWORD" psql -h "$DB_HOST" -p "$DB_PORT" -U "$DB_USER" -d "$database" -t -c "$count_sql" 2>/dev/null | xargs || echo "0").github/workflows/cleanup-partner-data.yml (1)
104-118: Static analysis: use grouped redirection for summary output.Per shellcheck SC2129, use a single grouped redirection instead of repeated
>> $GITHUB_STEP_SUMMARY:Proposed fix
- name: Summary if: success() + env: + DB_HOST: ${{ github.event.inputs.db_host }} + DB_PORT: ${{ github.event.inputs.db_port }} + DB_USER: ${{ github.event.inputs.db_user }} + DRY_RUN: ${{ github.event.inputs.dry_run }} run: | - echo "## Cleanup Summary" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "- **Database Host:** ${{ github.event.inputs.db_host }}" >> $GITHUB_STEP_SUMMARY - echo "- **Database Port:** ${{ github.event.inputs.db_port }}" >> $GITHUB_STEP_SUMMARY - echo "- **Database User:** ${{ github.event.inputs.db_user }}" >> $GITHUB_STEP_SUMMARY - echo "- **Dry Run:** ${{ github.event.inputs.dry_run }}" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - if [ "${{ github.event.inputs.dry_run }}" == "true" ]; then - echo "✅ Dry run completed - no data was deleted" >> $GITHUB_STEP_SUMMARY + { + echo "## Cleanup Summary" + echo "" + echo "- **Database Host:** $DB_HOST" + echo "- **Database Port:** $DB_PORT" + echo "- **Database User:** $DB_USER" + echo "- **Dry Run:** $DRY_RUN" + echo "" + if [ "$DRY_RUN" == "true" ]; then + echo "✅ Dry run completed - no data was deleted" - else - echo "⚠️ Deletion executed - partner data has been removed" >> $GITHUB_STEP_SUMMARY - fi + else + echo "⚠️ Deletion executed - partner data has been removed" + fi + } >> "$GITHUB_STEP_SUMMARY"This also addresses the script injection concern by using env vars instead of direct interpolation.
Helmsman/hooks/esignet-demo-oidc-partner-onboarder-postinstall.sh (3)
175-181:setoptions are placed after the function definitions but before invocation — the top-level lines 8–10 run without strict mode.This is a minor concern since
$#is always defined, but for consistency and to protect any future top-level code, consider moving thesetblock to the top of the script (right after the shebang).
29-31: ShellCheck SC2155: Declare and assign separately to avoid masking return values.Under
set -e, combininglocalwith command substitution masks a non-zero exit (the|| echo "0"fallback mitigates this here, but the pattern is repeated across many lines). Consider splitting declaration and assignment for robustness, e.g.:local active active=$(kubectl ... || echo "0")This applies to lines 29–31, 35–36, 73, 76, 87–89 as well.
15-107: Thetimeoutparameter (line 18) is accepted but the "freshness" check on line 39 uses a hardcoded 300-second threshold.If a caller passes a different timeout, the freshness window won't change accordingly. This is unlikely to cause issues with the current single call site (
300), but worth noting if reuse is intended.Helmsman/hooks/mock-relying-party-service-preinstall.sh (1)
89-95: Same pattern as the postinstall script:setoptions after top-level variable assignments.Lines 21-22 are evaluated before strict mode. Moving
setoptions to the top of the script (after the shebang) would catch issues like an unsetWORKDIRearly.Helmsman/utils/softhsm-mock-identity-system-values.yaml (1)
1-27: This file is identical tosofthsm-esignet-values.yaml.Both files share the exact same configuration. If they are expected to diverge, this is fine. If not, consider extracting a shared base values file to avoid drift.
docs/esignet_README.md (1)
402-409: Warning: deleting shared namespaces (keycloak,softhsm) may break other MOSIP services.The cleanup section suggests deleting the
keycloakandsofthsmnamespaces. If these are shared with the main MOSIP deployment, deleting them would break other services. Consider adding a warning note.Proposed addition
### Delete Namespaces +> **⚠️ WARNING**: Only delete shared namespaces (`keycloak`, `softhsm`) if you are fully removing MOSIP. These may be used by other MOSIP services. + ```bash kubectl delete ns esignet kubectl delete ns softhsm kubectl delete ns keycloak kubectl delete ns redis</details> </blockquote></details> <details> <summary>Helmsman/hooks/esignet-preinstall.sh (1)</summary><blockquote> `171-177`: **`set -e` / `set -o errexit` are redundant — they are the same option.** Line 172 (`set -e`) and Line 173 (`set -o errexit`) are identical. Remove one to avoid confusion. <details> <summary>Proposed cleanup</summary> ```diff # set commands for error handling. set -e -set -o errexit ## set -e : exit the script if any statement returns a non-true return value set -o nounset ## set -u : exit the script if you try to use an uninitialised variable set -o errtrace # trace ERR through 'time command' and other functions set -o pipefail # trace ERR through pipesHelmsman/hooks/esignet-partner-onboarder-postinstall.sh (2)
14-107:local var=$(cmd)masks return values (SC2155), andstart_timevariable is shadowed.Multiple
local var=$(...)declarations (lines 29-31, 35-36, 73, 76, 87-89) mask the exit code of the command. While the|| echo ...fallbacks mitigate this, the idiomatic fix is to declare and assign separately.Also,
start_timeon line 73 shadows the identically-named variable on line 31. Consider renaming line 73 tomonitor_startfor clarity.Example fix pattern (apply to all similar lines)
- local start_time=$(date +%s) + local monitor_start + monitor_start=$(date +%s)- local succeeded=$(kubectl -n $namespace get job/$JOB_NAME -o jsonpath='{.status.succeeded}' 2>/dev/null || echo "0") + local succeeded + succeeded=$(kubectl -n $namespace get job/$JOB_NAME -o jsonpath='{.status.succeeded}' 2>/dev/null || echo "0")
205-211: Redundantset -eandset -o errexit(same as in the preinstall script).Lines 206-207 are duplicates —
set -eis equivalent toset -o errexit.Remove the duplicate
# set commands for error handling. set -e -set -o errexit ## set -e : exit the script if any statement returns a non-true return value set -o nounset ## set -u : exit the script if you try to use an uninitialised variable set -o errtrace # trace ERR through 'time command' and other functions set -o pipefail # trace ERR through pipesHelmsman/dsf/esignet-dsf.yaml (3)
40-40: Hardcodedqajava11.mosip.nethostnames across the file should be parameterized.The DSF file already uses environment variable substitution for secrets (
$DB_USER_PASSWORD,$PMS_CLIENT_SECRET_KEY, etc.) and paths ($WORKDIR), but all hostnames are hardcoded toqajava11.mosip.net. This makes the config non-portable across environments and error-prone when deploying to a different domain.Consider introducing an environment variable (e.g.,
$MOSIP_DOMAIN) and using it consistently:Example refactor for a few representative lines
- postgresHost: "postgres.qajava11.mosip.net" + postgresHost: "postgres.$MOSIP_DOMAIN"- databases.mosip_esignet.host: "postgres.qajava11.mosip.net" + databases.mosip_esignet.host: "postgres.$MOSIP_DOMAIN"- keycloakExternalHost: "iam.qajava11.mosip.net" + keycloakExternalHost: "iam.$MOSIP_DOMAIN"- istio.hosts[0]: "esignet.qajava11.mosip.net" + istio.hosts[0]: "esignet.$MOSIP_DOMAIN"Apply the same pattern to all
*.qajava11.mosip.netreferences. SetMOSIP_DOMAINas an environment secret/variable in the GitHub Actions workflow.Also applies to: 74-75, 158-159, 174-175, 227-227, 277-279, 294-300
1-4:tillerNamespaceis irrelevant whentillerless: true.With
tillerless: true(line 3), thetillerNamespace: kube-system(line 2) is unused. This is not a bug, just dead configuration inherited from Helm 2 patterns.Clean up
helmDefaults: - tillerNamespace: kube-system tillerless: true install: true
196-199: Inlinekubectlcommand as a hook is inconsistent with the script-based hooks used elsewhere.Line 199 uses an inline
kubectl labelcommand as apreInstallhook, while all other releases use dedicated shell scripts. This isn't broken, but for consistency and to allow for future expansion (e.g., adding namespace creation), consider extracting this to a small script as well.
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Fix all issues with AI agents
In @.github/workflows/cleanup-partner-data.yml:
- Around line 104-118: The "Summary" step currently interpolates
github.event.inputs.* directly and writes unquoted $GITHUB_STEP_SUMMARY
(triggering SC2086); change the step to export the inputs via env: (e.g.,
DB_HOST, DB_PORT, DB_USER, DRY_RUN) and reference those env vars inside the run
block, and ensure you quote $GITHUB_STEP_SUMMARY when redirecting (e.g., ">>
\"$GITHUB_STEP_SUMMARY\""); also compare the dry-run value against the env var
(if [ "$DRY_RUN" = "true" ] ) and update the echo lines to use the env vars
instead of github.event.inputs.*.
In `@docs/CLEANUP_PARTNER_DATA_GUIDE.md`:
- Around line 75-84: The backup instructions currently only dump mosip_pms; add
a second pg_dump command to also back up the mosip_esignet database before
running the cleanup script: include a line analogous to the existing mosip_pms
dump but targeting -d mosip_esignet (e.g., pg_dump -h localhost -p 5433 -U
postgres -d mosip_esignet > esignet_backup_$(date +%Y%m%d_%H%M%S).sql) and
update the verification step (ls -lh) to show both pms and esignet backups so
CLEANUP_PARTNER_DATA_GUIDE.md documents backing up both mosip_pms and
mosip_esignet.
In `@docs/esignet_README.md`:
- Line 70: Update the broken relative link in esignet_README.md: replace the
incorrect path "../../../docs/RECAPTCHA_SETUP_GUIDE.md" with the correct local
docs path "./RECAPTCHA_SETUP_GUIDE.md" so the link on the line containing
"RECAPTCHA Setup Guide" points to the file in the same docs directory.
In `@Helmsman/dsf/esignet-dsf.yaml`:
- Around line 186-199: The preInstall hook for the release named
artifactory-1202 is labeling the wrong namespace; update the hooks.preInstall
value to target the release namespace (artifactory-1202) instead of artifactory
so Istio injection is applied to the correct namespace—locate the
artifactory-1202 release block and modify the preInstall command string
accordingly (hooks.preInstall).
In `@Helmsman/hooks/cleanup-partner-data.sh`:
- Around line 42-58: In execute_sql, avoid using SC2155 by declaring local count
before assigning it and stop suppressing stderr so connection errors surface
during dry-run; compute count_sql as before, run the psql call without
2>/dev/null, capture its stdout into count (e.g. declare local count;
count=$(PGPASSWORD="$DB_PASSWORD" psql -h "$DB_HOST" -p "$DB_PORT" -U "$DB_USER"
-d "$database" -t -c "$count_sql" | xargs) ) and on failure fall back to "0" or
print the psql stderr to alert the user; update the DRY_RUN branch in
execute_sql to use that pattern so errors are visible and SC2155 is avoided.
- Around line 79-156: The DELETE statements use unqualified table names and must
be schema-qualified to run against the correct schema; update every SQL passed
to execute_sql that targets the PMS database (calls with first arg "mosip_pms")
to prefix tables with pms. Specifically, change references for auth_policy,
policy_group, partner_policy_request, partner, misp_license, oidc_client (and
any other unqualified uses in the same file) to pms.auth_policy,
pms.policy_group, pms.partner_policy_request, pms.partner, pms.misp_license,
pms.oidc_client respectively so execute_sql calls execute against the intended
schema; leave or adjust schema qualification for mosip_esignet calls if that DB
uses a different schema.
In `@Helmsman/hooks/esignet-partner-onboarder-postinstall.sh`:
- Around line 158-177: The idempotency checks fail because the jq selectors look
for uppercased underscore names but kubectl set env --prefix creates env vars
preserving the key (e.g.,
SPRING_CLOUD_CONFIG_SERVER_OVERRIDES_mosip-esignet-misp-key), so MISP_KEY_ENV
and RESIDENT_OIDC_ENV never match; update the jq selectors to look for the
actual created names (e.g., select(.name ==
"SPRING_CLOUD_CONFIG_SERVER_OVERRIDES_mosip-esignet-misp-key") and select(.name
== "SPRING_CLOUD_CONFIG_SERVER_OVERRIDES_resident-oidc-clientid") respectively)
or normalize the names in both places (e.g., convert env names to
lowercase/replace underscores with hyphens before comparing) so the checks in
variables MISP_KEY_ENV and RESIDENT_OIDC_ENV correctly detect existing env vars
and preserve idempotency.
In `@Helmsman/hooks/esignet-preinstall.sh`:
- Around line 139-146: The fixed 4-minute sleep is fragile; replace the sleep
call in the ENV_CHANGES_MADE branch with a kubectl rollout status check for the
config-server deployment so we wait until the deployment is actually ready.
Specifically, call kubectl rollout status against the config-server deployment
(include the appropriate namespace variable if one exists) with a sensible
timeout (e.g., 5m) and handle non-zero exit codes by logging an error and
exiting non-zero; keep the existing echo messages before/after the wait. Update
references around ENV_CHANGES_MADE and the config-server readiness logic
accordingly.
- Around line 1-5: The usage line in esignet-preinstall.sh advertises a
[kubeconfig] argument but the script does not read it; either add the same
KUBECONFIG handling as the sibling script or remove the misleading usage text.
Fix by updating esignet-preinstall.sh to check arguments (e.g., if [ $# -ge 1 ];
then export KUBECONFIG=$1; fi) near the top of the script (before any kubectl
commands), or alternatively remove the "[kubeconfig]" from the usage comment if
you don’t want to accept an explicit kubeconfig parameter; reference the script
name esignet-preinstall.sh and the kubeconfig/env handling to locate the change.
- Around line 105-106: CURRENT_GENERATION is captured but never used; either
remove that assignment or use it to assert the deployment generation changed
after your installation. To fix, keep CURRENT_GENERATION=$(kubectl get
deployment config-server -n config-server -o jsonpath='{.metadata.generation}'
2>/dev/null || echo "0") and after applying changes poll the same kubectl
jsonpath to get NEW_GENERATION, loop with a sleep and timeout until
NEW_GENERATION != CURRENT_GENERATION (or exit non‑zero on timeout) to verify the
rollout progressed; alternatively remove the CURRENT_GENERATION line entirely
and rely on kubectl rollout status for readiness.
In `@Helmsman/hooks/mock-relying-party-service-preinstall.sh`:
- Around line 49-53: The script writes decoded sensitive keys from
MOCK_RELYING_PARTY_CLIENT_PRIVATE_KEY to a world-readable /tmp file; change it
to create a secure temporary file (use mktemp), set restrictive permissions
(umask/chmod) and/or use a secure file descriptor, write the base64-decoded
content into that secure temp file, create the Kubernetes secret from that
secure temp file (same approach for the JWE key handling around lines 71-75),
ensure you register a trap to securely remove the temp file on exit, and update
references from /tmp/client-private-key to the secure temp filename (referencing
the variable holding the temp path in the script).
In `@Helmsman/hooks/redis-setup.sh`:
- Around line 1-5: The script documents an optional [kubeconfig] argument but
never reads or exports it; update the top of Helmsman/hooks/redis-setup.sh to
accept an optional first positional parameter (e.g., KUBECONFIG="$1"), set a
sensible default when empty (e.g., leave system KUBECONFIG or ~/.kube/config),
and export KUBECONFIG before any kubectl/helm invocations; reference the script
header and the NS=redis variable to locate the file and ensure the exported
KUBECONFIG is available for the rest of the hook.
In `@Helmsman/hooks/softhsm-esignet-postinstall.sh`:
- Around line 43-54: The "Waiting for config-server" logs are misleading because
there is no actual wait after the rollout restart; after the kubectl -n
config-server rollout restart deploy/config-server call (in this script/hook),
add a real readiness check using kubectl rollout status for deploy/config-server
in the config-server namespace with a sensible timeout, run that check before
printing "Config-server configured, continuing deployment...", and ensure the
script exits non-zero on failure so the hook fails if the rollout does not
complete.
In `@Helmsman/utils/keycloak-init-values.yaml`:
- Around line 41-64: The YAML uses a copy-pasted description for unrelated
scopes; update the description fields for the get_certificate and
sending_binding_otp scope entries (the blocks starting with name:
get_certificate and name: sending_binding_otp) so they either contain
meaningful, specific descriptions relevant to their purpose or are set to an
empty string like upload_certificate; ensure you only change the description
value for those two scope entries and keep other attributes (protocol, Include
In Token Scope, attributes) unchanged.
In `@README.md`:
- Around line 1250-1295: The document has duplicated step numbering: the "Deploy
eSignet" section is numbered 5 and the "Deploy Test Rigs (Manual)" heading is
also numbered 5; update the "Deploy Test Rigs (Manual)" heading to use 6 and
increment the subsequent "Verify Deployment" heading to 7 (or renumber following
headings accordingly) so numbering is sequential; search for the headings
"Deploy eSignet", "Deploy Test Rigs (Manual)" and "Verify Deployment" in
README.md and adjust their numeric prefixes.
🧹 Nitpick comments (8)
.github/workflows/README.md (1)
464-582: Well-structured eSignet workflow documentation.The documentation is comprehensive, covering triggers, inputs, secrets, deployment modes, components, security features, verification, and troubleshooting. One minor nit from static analysis:
Line 520: The fenced code block should specify a language (e.g.,
```text) to satisfy markdownlint MD040.Proposed fix
-``` +```text MOSIP DSF → eSignet DSF -``` +```Helmsman/hooks/esignet-partner-onboarder-postinstall.sh (1)
184-196: Hard-codedsleep 210andsleep 120add 5.5 minutes of unconditional waiting.The
sleep 210on line 184 always executes regardless of whether config changed, and there's no comment explaining why 3.5 minutes is needed. Thesleep 120on line 196 adds another 2 minutes. Consider replacing these withkubectl rollout statuswith a--timeoutflag, which will proceed as soon as the rollout completes instead of always waiting the maximum duration.Suggested approach
- sleep 210 - # Config-server restart initiated - continuing with deployment - echo "Config-server restart initiated, continuing with deployment..." + echo "Waiting for config-server rollout to complete..." + kubectl -n config-server rollout status deploy/config-server --timeout=210s- sleep 120 + echo "Waiting for esignet rollout to complete..." + kubectl -n $NS rollout status deployment/esignet --timeout=120s 2>/dev/null || echo "esignet rollout status check skipped"Helmsman/hooks/esignet-demo-oidc-partner-onboarder-postinstall.sh (1)
15-107: Near-duplicate ofwait_for_job_completion()inesignet-partner-onboarder-postinstall.sh.
wait_for_job_status()here andwait_for_job_completion()in the sibling script share ~90% identical logic (job freshness detection, active/succeeded polling, timeout loop, diagnostic output). Consider extracting the shared function into a common utility (e.g.,Helmsman/hooks/lib/wait_for_job.sh) and sourcing it from both scripts, to reduce maintenance burden and divergence risk.Helmsman/hooks/pre-helmsman-cleanup.sh (1)
1-48: Missing strict error-handling flags — inconsistent with all other hooks.Every other hook in this PR enables
set -e,set -o nounset,set -o pipefail, etc. This cleanup script omits them entirely. While the--ignore-not-foundflag handles missing resources gracefully, a transient kubectl connection failure would be silently swallowed, and the script would report "cleanup complete" even though nothing was actually cleaned up. Consider adding at leastset -eandset -o pipefailto catch infrastructure-level failures.Proposed fix
if [ $# -ge 1 ] ; then export KUBECONFIG=$1 fi +set -e +set -o pipefail + NS=esignetHelmsman/dsf/esignet-dsf.yaml (1)
40-40: Hardcoded environment-specific domainqajava11.mosip.netthroughout the DSF.Multiple lines reference
qajava11.mosip.net(e.g.,postgres.qajava11.mosip.net,iam.qajava11.mosip.net,esignet.qajava11.mosip.net,healthservices.qajava11.mosip.net). If this DSF is intended to be reusable across environments, consider parameterizing the domain via an environment variable (e.g.,$CLUSTER_DOMAIN).Also applies to: 74-74, 158-158, 174-174, 227-227, 279-279, 294-300
docs/esignet_README.md (1)
103-107: Add language specifiers to fenced code blocks.These code blocks lack a language identifier, which triggers MD040 warnings and loses syntax highlighting. Use
textorplaintextfor ASCII diagrams and path listings.Proposed fix
Line 103:
-``` +```text ┌─────────────┐ ┌─────────────┐Line 146:
-``` +```text Helmsman/dsf/esignet-dsf.yamlLine 202:
-``` +```text Helmsman/hooks/Also applies to: 146-148, 202-204
.github/workflows/cleanup-partner-data.yml (1)
67-70: Duplicateapt-get updatewhen WireGuard is enabled.The WireGuard step (line 52) and the PostgreSQL client step (line 69) both run
apt-get update. Consider combining or conditioning to avoid the redundant call.Also applies to: 49-57
Helmsman/hooks/cleanup-partner-data.sh (1)
6-6:DB_PASSWORDis validated after printing connection details — consider validating earlier.The password check on line 69 runs after echoing host/port/user to stdout (lines 63-66). This is a minor ordering issue, but moving validation before any output would be cleaner, especially if this script is ever called in a context where partial output is parsed.
Also applies to: 69-73
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/esignet_README.md`:
- Around line 436-437: Update the two markdown links pointing to other docs so
they use paths relative to the current docs folder: replace
"../../docs/SECRET_GENERATION_GUIDE.md" with "./SECRET_GENERATION_GUIDE.md" and
"../../docs/ONBOARDING_GUIDE.md" with "./ONBOARDING_GUIDE.md" (the link texts
are "Secret Generation Guide" and "Onboarding Guide").
🧹 Nitpick comments (3)
docs/esignet_README.md (2)
165-167: Add language identifier to fenced code block.The fenced code block should specify a language identifier for proper syntax highlighting and accessibility.
📝 Proposed fix
-``` +```text Helmsman/dsf/esignet-dsf.yaml</details> --- `221-223`: **Add language identifier to fenced code block.** The fenced code block should specify a language identifier for proper syntax highlighting and accessibility. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text Helmsman/hooks/</details> </blockquote></details> <details> <summary>README.md (1)</summary><blockquote> `1272-1272`: **Consider adding hyphen for compound modifier.** The phrase "OpenID Connect (OIDC) based identity verification" could use a hyphen: "OIDC-based identity verification" for standard English compound modifier style. <details> <summary>✏️ Proposed fix</summary> ```diff -> **eSignet** is MOSIP's authentication and authorization service that provides OpenID Connect (OIDC) based identity verification. +> **eSignet** is MOSIP's authentication and authorization service that provides OpenID Connect (OIDC)-based identity verification.
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
1 participant
Summary by CodeRabbit
New Features
Documentation