Fixes: mosip/esignet#1878 Add company-internal CA extension point to eSignet Helm chart#1990
Draft
Ivanmeneges wants to merge 1 commit into
Draft
Fixes: mosip/esignet#1878 Add company-internal CA extension point to eSignet Helm chart#1990Ivanmeneges wants to merge 1 commit into
Ivanmeneges wants to merge 1 commit into
Conversation
WalkthroughAdds an interactive TLS trust helper and integrates it into installer scripts; extends Helm chart values and templates to support an optional company-internal CA bundle (Secret/ConfigMap), an init container that builds a Java truststore from PEMs, and deployment wiring to mount the truststore and CA bundle. ChangesCustom Company CA Trust Support
Sequence Diagram(s)sequenceDiagram
participant User
participant InstallerScript as Installer Script
participant TLSHelper as configure_tls_trust
participant Kubectl
participant HelmChart as Helm Chart
participant JavaService as Java Service
User->>InstallerScript: Run install.sh
InstallerScript->>TLSHelper: source and call configure_tls_trust($NS)
User->>TLSHelper: Select TLS mode
alt Custom CA Mode
TLSHelper->>Kubectl: Check Secret exists
alt Secret missing
User->>TLSHelper: Provide PEM CA bundle path
TLSHelper->>Kubectl: Create CA Secret in namespace
end
TLSHelper-->>InstallerScript: TRUST_HELM_ARGS with customCA
else Insecure Mode
TLSHelper-->>InstallerScript: TRUST_HELM_ARGS with enable_insecure=true
else Default Mode
TLSHelper-->>InstallerScript: TRUST_HELM_ARGS (empty)
end
InstallerScript->>HelmChart: helm install/upgrade with $TRUST_HELM_ARGS
HelmChart->>JavaService: render/init customCA init container
JavaService->>JavaService: init container builds truststore and imports certs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@deploy/configure_tls_trust.sh`:
- Around line 27-39: The script enables customCA by setting TRUST_HELM_ARGS with
customCA.enabled and customCA.secretName but doesn't validate or propagate
customCA.secretKey; update deploy/configure_tls_trust.sh to check the existing
Secret (variable ca_secret in the script) for the expected key (default "ca.crt"
or a user-provided key), fail with an informative message if the key is missing,
and append --set customCA.secretKey=<key> to TRUST_HELM_ARGS when setting
customCA.enabled=true so the Helm chart's init container (which reads
customCA.secretKey) can find the PEM bundle.
In `@helm/esignet/templates/_helpers.tpl`:
- Around line 73-82: In the template defined by
"esignet.validateValues.customCA" add a validation branch that detects when
.Values.customCA.enabled is true AND both .Values.customCA.secretName and
.Values.customCA.configMapName are non-empty, and emit an error message
indicating that both cannot be set simultaneously (explain that Secret takes
precedence and one must be removed); update the conditional logic alongside the
existing checks for missing names and enable_insecure to ensure this new
mutually-exclusive check runs when customCA.enabled is true.
- Around line 52-54: esignet.validateValues must actively block invalid flag
combinations and be invoked during chart rendering: update the validate function
(esignet.validateValues) to call {{ failf }} when .Values.enable_insecure and
.Values.customCA.enabled are both true (or otherwise enforce a single source of
truth), then include a call to that helper from top-level templates (e.g.,
deployment.yaml and other rendered templates) so validation runs; additionally,
change the init container rendering logic so the enable_insecure init container
(the initContainers block that copies /cacerts) is only rendered when
.Values.enable_insecure is true AND .Values.customCA.enabled is false, or alter
its copy behavior to avoid overwriting the cacerts volume when
customCAInitContainer is enabled, referencing the customCAInitContainer, the
initContainers block, and the cacerts volume to locate the code to change.
In `@helm/esignet/values.yaml`:
- Around line 527-549: The values.yaml default for customCA.javaHome may not
match the Java installation in the main app image, causing the init container to
mount/update the wrong truststore; update either by adding a clear comment near
customCA.javaHome (and mention mountPath and that it must match the main
container JAVA_HOME) or add a runtime validation step in the init container
entrypoint (the init container that uses initContainerImage and writes to {{
.Values.customCA.javaHome }}/lib/security/cacerts) to check that
$JAVA_HOME/lib/security/cacerts exists at the target mountPath and exit with a
clear error if not found so the mismatch is detected early.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 80f08999-cf89-4492-a069-9b0aae7e3b6a
📒 Files selected for processing (10)
deploy/configure_tls_trust.shdeploy/esignet-with-plugins/install.shdeploy/esignet-with-plugins/values.yamldeploy/esignet/install.shdeploy/esignet/values.yamlhelm/esignet/README.mdhelm/esignet/templates/_helpers.tplhelm/esignet/templates/custom-ca-initcontainer.yamlhelm/esignet/templates/deployment.yamlhelm/esignet/values.yaml
cursor
Bot
force-pushed
the
cursor/internal-ca-helm-extension-403f
branch
from
31fd675 to
8a8e13a
Compare
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 `@helm/esignet/templates/custom-ca-initcontainer.yaml`:
- Line 18: The cp command copying "${JAVA_HOME}/lib/security/cacerts" to
"${KEYSTORE}" can fail with an opaque "No such file or directory" when
customCA.javaHome is wrong; update the initContainer command that contains cp
"${JAVA_HOME}/lib/security/cacerts" "${KEYSTORE}" to first test for the source
file and exit with a descriptive error that includes the resolved
JAVA_HOME/customCA.javaHome (e.g., if [ -f "${JAVA_HOME}/lib/security/cacerts"
]; then cp ...; else echo "ERROR: Java cacerts not found at
${JAVA_HOME}/lib/security/cacerts — ensure customCA.javaHome matches the Java
installation in the image" >&2; exit 1; fi), so operators see the bad javaHome
value and can fix values.yaml.
In `@helm/esignet/templates/deployment.yaml`:
- Around line 75-80: Call the existing validation template and add a runtime
guard so both init containers cannot be rendered together: invoke the
esignet.validateValues template near the top of deployment.yaml (before any
conditional init-container logic) and modify the enable_insecure init-container
block (the include of "common.tplvalues.render" for .Values.initContainers) to
only render when .Values.enable_insecure is true AND .Values.customCA.enabled is
false; also update the esignet.validateValues helper in _helpers.tpl to
return/throw a failure (not just print) when customCA.enabled and
enable_insecure are both true so Helm install/upgrade fails early.
In `@helm/esignet/values.yaml`:
- Around line 371-373: The default value for extraInitContainers is currently an
empty map ({}) but must be an array; update the values.yaml entry named
extraInitContainers to be an empty list ([]) so templates that call
common.tplvalues.render on extraInitContainers produce a YAML list of init
containers instead of a dict, avoiding invalid Kubernetes manifests when
rendering initContainers.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 3ccde635-a505-4632-b21d-183ba2977da5
📒 Files selected for processing (10)
deploy/configure_tls_trust.shdeploy/esignet-with-plugins/install.shdeploy/esignet-with-plugins/values.yamldeploy/esignet/install.shdeploy/esignet/values.yamlhelm/esignet/README.mdhelm/esignet/templates/_helpers.tplhelm/esignet/templates/custom-ca-initcontainer.yamlhelm/esignet/templates/deployment.yamlhelm/esignet/values.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helm/esignet/templates/_helpers.tpl (1)
52-54:⚠️ Potential issue | 🔴 Critical
esignet.truststoreRequiredreturns a truthy string even when both conditions are false.The
includefunction in Helm templates always returns a string. When the helper evaluatesor .Values.enable_insecure .Values.customCA.enabledwith both valuesfalse, the result is stringified to"false". In Helm'sifcontrol structure, any non-empty string—including"false"—evaluates as true. This causesif include "esignet.truststoreRequired" .in deployment.yaml to incorrectly render the cacerts volume mount even when neither insecure mode nor custom CA is enabled.Suggested fix
{{- define "esignet.truststoreRequired" -}} -{{- or .Values.enable_insecure .Values.customCA.enabled -}} +{{- if or .Values.enable_insecure .Values.customCA.enabled -}}true{{- end -}} {{- end -}}🤖 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 `@helm/esignet/templates/_helpers.tpl` around lines 52 - 54, The esignet.truststoreRequired helper template returns a stringified boolean which causes incorrect evaluation in Helm if conditions. When both enable_insecure and customCA.enabled are false, the result is the non-empty string "false" which evaluates as true in Helm's if block. Modify the esignet.truststoreRequired template to return a non-empty string (like "true") when the or condition is true, and return an empty string when the condition is false by wrapping the or expression in an if block that outputs a value only when the condition evaluates to true.
🤖 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 `@deploy/configure_tls_trust.sh`:
- Around line 29-35: The jsonpath syntax {.data.${ca_secret_key}} in the kubectl
commands at lines 29 and 34 fails for Secret keys containing dots (such as the
default ca.crt) because kubectl treats dots as object navigation operators
rather than literal key characters. Replace both jsonpath expressions with
bracket notation {.data['${ca_secret_key}']} which safely accesses map keys with
special characters including dots, ensuring the Secret key lookups work
correctly regardless of whether the key contains dots.
---
Outside diff comments:
In `@helm/esignet/templates/_helpers.tpl`:
- Around line 52-54: The esignet.truststoreRequired helper template returns a
stringified boolean which causes incorrect evaluation in Helm if conditions.
When both enable_insecure and customCA.enabled are false, the result is the
non-empty string "false" which evaluates as true in Helm's if block. Modify the
esignet.truststoreRequired template to return a non-empty string (like "true")
when the or condition is true, and return an empty string when the condition is
false by wrapping the or expression in an if block that outputs a value only
when the condition evaluates to true.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a4b1fd17-80c9-488f-ba95-e93e818534a4
📒 Files selected for processing (5)
deploy/configure_tls_trust.shhelm/esignet/templates/_helpers.tplhelm/esignet/templates/custom-ca-initcontainer.yamlhelm/esignet/templates/deployment.yamlhelm/esignet/values.yaml
Introduce customCA values and init container to import PEM-encoded corporate CA bundles into the Java truststore. Wire extraVolumes and extraVolumeMounts in the deployment template, and update install scripts with a dedicated TLS trust configuration option. Includes Helm validation for customCA configuration, mutual exclusivity with enable_insecure, and install-script support for non-default Secret keys. Signed-off-by: Ivanmeneges <Ivanmeneges@users.noreply.github.com> Co-authored-by: Ivanmeneges <Ivanmeneges@users.noreply.github.com>
cursor
Bot
force-pushed
the
cursor/internal-ca-helm-extension-403f
branch
from
a9735c8 to
db4484f
Compare
Ivanmeneges
changed the title
Ivanmeneges
changed the title
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.
Summary
Adds first-class Helm support for trusting a company-internal CA in eSignet deployments, addressing the gap where only the development-oriented
enable_insecureworkaround existed.Changes
Helm chart (
helm/esignet)customCAvalues block to mount a Secret/ConfigMap with PEM-encoded root/intermediate CA certificate(s)cacertstruststore before the app startsextraVolumes/extraVolumeMountsextension points in the deployment templateextraInitContainersfor additional custom init hookscustomCAandenable_insecuretogetherDeploy scripts
deploy/configure_tls_trust.shhelper used by eSignet install scriptsenable_insecure)customCA)Documentation
helm/esignet/README.mdwith usage examplescustomCAsamples in deploy values filesUsage
Notes
enable_insecureremains available for dev/self-signed environments but is mutually exclusive withcustomCASummary by CodeRabbit