refactor(new): remove Pool Manager settings and add JD signing options#1188
refactor(new): remove Pool Manager settings and add JD signing options#1188guimoreirar merged 1 commit intomainfrom
Conversation
…ing options - Removed Pool Manager configuration options from values-template.yaml, values.yaml, and configmap.yaml. - Added JD_SIGNING_MODE and JD_VALIDATE_EXTERNAL_SIGNATURE options to values.yaml and configmap.yaml for enhanced signing capabilities.
WalkthroughRemoved Pool Manager (SaaS multi-tenant mode) configuration from the Helm chart and added JD signature validation settings. Changes include eliminating Pool Manager environment variables and the DEFAULT_MIDAZ_ORGANIZATION_ID, while introducing JD_SIGNING_MODE and JD_VALIDATE_EXTERNAL_SIGNATURE configuration options with an optional public key secret. Changes
Comment |
gandalf-at-lerian
left a comment
There was a problem hiding this comment.
Clean removal of Pool Manager config and solid additions for JD signing.
A few observations:
-
DEFAULT_MIDAZ_ORGANIZATION_IDsilently dropped from configmap — anyone relying on this env var in their deployment will lose it on upgrade. Intentional? If so, worth a note in release notes. -
JD_SIGNING_MODEdefaults and security posture look correct —local_pemas default withJD_VALIDATE_EXTERNAL_SIGNATURE: trueis the right call. External sig validation on by default = good. -
JD_PUBLIC_KEY_PEMconditional in secrets.yaml — clean. Only injected when set, avoids empty secret entries. -
Pool Manager removal — no leftover references in the templates. Clean cut.
LGTM 👍
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/plugin-br-bank-transfer/templates/configmap.yaml`:
- Around line 190-192: Validate JD_SIGNING_MODE and require JD_PUBLIC_KEY_PEM
when external validation is enabled: in the configmap template check that
.Values.bankTransfer.configmap.JD_SIGNING_MODE is one of the allowed values
(e.g., "local_pem" or "external_provided") and if not, fail fast using Helm's
required() with a helpful message; additionally, when
.Values.bankTransfer.configmap.JD_SIGNING_MODE == "external_provided" and
(.Values.bankTransfer.configmap.JD_VALIDATE_EXTERNAL_SIGNATURE | default "true")
== "true", call required() on .Values.bankTransfer.configmap.JD_PUBLIC_KEY_PEM
to force template rendering to fail with a clear error if the public key is
missing. Ensure defaults remain (default "local_pem") but add these conditional
checks around JD_SIGNING_MODE, JD_VALIDATE_EXTERNAL_SIGNATURE and
JD_PUBLIC_KEY_PEM to prevent invalid runtime configurations.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30f91946-1902-4531-bcf7-f32181f98dfe
📒 Files selected for processing (4)
charts/plugin-br-bank-transfer/templates/configmap.yamlcharts/plugin-br-bank-transfer/templates/secrets.yamlcharts/plugin-br-bank-transfer/values-template.yamlcharts/plugin-br-bank-transfer/values.yaml
| JD_SIGNING_MODE: {{ .Values.bankTransfer.configmap.JD_SIGNING_MODE | default "local_pem" | quote }} | ||
| JD_SIGNATURE_REQUIRED: {{ .Values.bankTransfer.configmap.JD_SIGNATURE_REQUIRED | default "true" | quote }} | ||
| JD_VALIDATE_EXTERNAL_SIGNATURE: {{ .Values.bankTransfer.configmap.JD_VALIDATE_EXTERNAL_SIGNATURE | default "true" | quote }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect JD signing-related wiring:"
rg -n 'JD_SIGNING_MODE|JD_VALIDATE_EXTERNAL_SIGNATURE|JD_PUBLIC_KEY_PEM' \
charts/plugin-br-bank-transfer/templates/configmap.yaml \
charts/plugin-br-bank-transfer/templates/secrets.yaml \
charts/plugin-br-bank-transfer/values.yaml \
charts/plugin-br-bank-transfer/values-template.yaml
echo
echo "Inspect current validation guards (expected to include enum and dependency checks after fix):"
rg -n 'fail|required.*JD_PUBLIC_KEY_PEM|JD_SIGNING_MODE must be one of' \
charts/plugin-br-bank-transfer/templates/configmap.yaml \
charts/plugin-br-bank-transfer/templates/secrets.yamlRepository: LerianStudio/helm
Length of output: 1529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Context around lines 190-192 in configmap.yaml ==="
sed -n '180,200p' charts/plugin-br-bank-transfer/templates/configmap.yaml
echo
echo "=== Check for any existing fail() or validation patterns in template files ==="
rg -n 'fail\s*\(' charts/plugin-br-bank-transfer/templates/ || echo "No fail() calls found"
echo
echo "=== Check structure of JD config block ==="
sed -n '185,210p' charts/plugin-br-bank-transfer/templates/configmap.yaml
echo
echo "=== Verify secrets.yaml conditional rendering for JD_PUBLIC_KEY_PEM ==="
sed -n '80,90p' charts/plugin-br-bank-transfer/templates/secrets.yamlRepository: LerianStudio/helm
Length of output: 5067
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify Helm template syntax: Check for examples of 'has' function or list handling ==="
rg -n 'has\s*\(' charts/plugin-br-bank-transfer/templates/ || echo "No 'has()' function found in templates"
echo
echo "=== Check for other validation patterns (fail, assert, etc.) in any chart templates ==="
find charts -name "*.yaml" -type f -exec grep -l 'fail\s*(' {} \; 2>/dev/null || echo "No fail() patterns found"
echo
echo "=== Verify Helm version or requirements to confirm fail() availability ==="
if [ -f "charts/plugin-br-bank-transfer/Chart.yaml" ]; then
grep -A 5 -B 5 "apiVersion\|appVersion" charts/plugin-br-bank-transfer/Chart.yaml | head -20
fiRepository: LerianStudio/helm
Length of output: 707
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify Sprig template functions available (checking for alternative to 'has' for list membership) ==="
echo "Looking for any list-based conditions in templates:"
rg -n 'in\s*\(|contains\s*\(|list\s*\(' charts/plugin-br-bank-transfer/templates/ || echo "No list functions found"
echo
echo "=== Verify actual issue: Can invalid JD_SIGNING_MODE values be deployed without failing template rendering? ==="
echo "Checking if there's any validation before using JD_SIGNING_MODE:"
sed -n '183,200p' charts/plugin-br-bank-transfer/templates/configmap.yaml
echo
echo "=== Check values files for documented valid modes ==="
grep -A 2 -B 2 'JD_SIGNING_MODE' charts/plugin-br-bank-transfer/values.yaml
echo
echo "=== Verify the dependency concern: check if JD_PUBLIC_KEY_PEM is truly optional in secrets ==="
sed -n '78,95p' charts/plugin-br-bank-transfer/templates/secrets.yamlRepository: LerianStudio/helm
Length of output: 3503
Fix invalid JD_SIGNING_MODE values and missing JD_PUBLIC_KEY_PEM validation.
Lines 190 and 192 accept invalid or incomplete configurations (e.g., JD_SIGNING_MODE=external_provided with JD_VALIDATE_EXTERNAL_SIGNATURE=true but no JD_PUBLIC_KEY_PEM), which render successfully but fail at runtime.
🔧 Proposed fix
# JD SPB Adapter (only when NOT in sandbox mode)
{{- if ne (toString .Values.bankTransfer.configmap.JD_SANDBOX_MODE) "true" }}
+ {{- $jdSigningMode := .Values.bankTransfer.configmap.JD_SIGNING_MODE | default "local_pem" }}
+ {{- $jdValidateExternal := toString (.Values.bankTransfer.configmap.JD_VALIDATE_EXTERNAL_SIGNATURE | default "true") }}
+ {{- if and (ne $jdSigningMode "local_pem") (ne $jdSigningMode "external_provided") (ne $jdSigningMode "disabled") }}
+ {{- fail "bankTransfer.configmap.JD_SIGNING_MODE must be one of: local_pem, external_provided, disabled" }}
+ {{- end }}
+ {{- if and (eq $jdSigningMode "external_provided") (eq $jdValidateExternal "true") (not .Values.bankTransfer.secrets.JD_PUBLIC_KEY_PEM) }}
+ {{- fail "bankTransfer.secrets.JD_PUBLIC_KEY_PEM is required when JD_SIGNING_MODE=external_provided and JD_VALIDATE_EXTERNAL_SIGNATURE=true" }}
+ {{- end }}
JD_BASE_URL: {{ required "bankTransfer.configmap.JD_BASE_URL is required when JD_SANDBOX_MODE is not true" .Values.bankTransfer.configmap.JD_BASE_URL | quote }}
JD_SOAP_PATH: {{ .Values.bankTransfer.configmap.JD_SOAP_PATH | default "/soap" | quote }}
JD_ORIGIN_ISPB: {{ required "bankTransfer.configmap.JD_ORIGIN_ISPB is required when JD_SANDBOX_MODE is not true" .Values.bankTransfer.configmap.JD_ORIGIN_ISPB | quote }}
@@
- JD_SIGNING_MODE: {{ .Values.bankTransfer.configmap.JD_SIGNING_MODE | default "local_pem" | quote }}
+ JD_SIGNING_MODE: {{ $jdSigningMode | quote }}
JD_SIGNATURE_REQUIRED: {{ .Values.bankTransfer.configmap.JD_SIGNATURE_REQUIRED | default "true" | quote }}
- JD_VALIDATE_EXTERNAL_SIGNATURE: {{ .Values.bankTransfer.configmap.JD_VALIDATE_EXTERNAL_SIGNATURE | default "true" | quote }}
+ JD_VALIDATE_EXTERNAL_SIGNATURE: {{ $jdValidateExternal | quote }}
JD_TIMEOUT_MS: {{ .Values.bankTransfer.configmap.JD_TIMEOUT_MS | default "8000" | quote }}
JD_MAX_RETRIES: {{ .Values.bankTransfer.configmap.JD_MAX_RETRIES | default "3" | quote }}
{{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/plugin-br-bank-transfer/templates/configmap.yaml` around lines 190 -
192, Validate JD_SIGNING_MODE and require JD_PUBLIC_KEY_PEM when external
validation is enabled: in the configmap template check that
.Values.bankTransfer.configmap.JD_SIGNING_MODE is one of the allowed values
(e.g., "local_pem" or "external_provided") and if not, fail fast using Helm's
required() with a helpful message; additionally, when
.Values.bankTransfer.configmap.JD_SIGNING_MODE == "external_provided" and
(.Values.bankTransfer.configmap.JD_VALIDATE_EXTERNAL_SIGNATURE | default "true")
== "true", call required() on .Values.bankTransfer.configmap.JD_PUBLIC_KEY_PEM
to force template rendering to fail with a clear error if the public key is
missing. Ensure defaults remain (default "local_pem") but add these conditional
checks around JD_SIGNING_MODE, JD_VALIDATE_EXTERNAL_SIGNATURE and
JD_PUBLIC_KEY_PEM to prevent invalid runtime configurations.
Midaz Pull Request Checklist
Pull Request Type
Checklist
Please check each item after it's completed.
Additional Notes
Obs: Please, always remember to target your PR to develop branch instead of main.