fix: move Keycloak admin credentials from ConfigMap to Secret (rebased #161) - #179
Merged
Merged
Conversation
Signed-off-by: Ryan Cook <rcook@redhat.com>
Update demo-ui.md and demo-manual.md to document the new keycloak-admin-secret Secret that client-registration reads admin credentials from (instead of the environments ConfigMap). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
4 tasks
pdettori
approved these changes
Mar 9, 2026
pdettori
left a comment
pdettori
left a comment
Member
There was a problem hiding this comment.
Good security improvement moving Keycloak admin credentials from ConfigMap to a dedicated Secret. Code, RBAC, examples, docs, and tests are all updated consistently.
Areas reviewed: Go, Helm/RBAC, YAML, Docs
Commits: 2, all signed-off ✓
CI: passing ✓
One suggestion on configmaps-webhook.yaml (misplaced comment block) and two minor test robustness nits. Nothing blocking.
| name: keycloak-admin-secret | ||
| namespace: team1 | ||
| type: Opaque | ||
| stringData: |
Member
There was a problem hiding this comment.
suggestion: The --- + Secret header insertion causes the PLATFORM_CLIENT_IDS comment block (the lines after KEYCLOAK_ADMIN_PASSWORD) to land under the Secret's stringData: instead of the ConfigMap's data:. The comments are YAML comments so it's not functionally broken, but it's misleading — a reader would think PLATFORM_CLIENT_IDS belongs in the Secret. Consider moving the PLATFORM_CLIENT_IDS comment block back above the --- separator under the ConfigMap.
| @@ -134,3 +134,45 @@ func TestBuildClientRegistrationContainer_HasPlatformClientIDsEnv(t *testing.T) | |||
| t.Error("client-registration container missing PLATFORM_CLIENT_IDS env var") | |||
| } | |||
| } | |||
Member
There was a problem hiding this comment.
nit: This test silently passes if neither KEYCLOAK_ADMIN_USERNAME nor KEYCLOAK_ADMIN_PASSWORD is found in the env list at all (the inner loop just skips). Consider adding a found bool or t.Errorf if the key is absent, so the test catches accidental removal of these env vars.
| } | ||
| } | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
nit: Same as above — silently passes if KEYCLOAK_URL or KEYCLOAK_REALM are missing from the env list entirely.
| verbs: ["get", "list", "watch"] | ||
| - apiGroups: [""] | ||
| resources: ["secrets"] | ||
| verbs: ["get", "list", "watch"] |
Member
There was a problem hiding this comment.
👍 Good least-privilege: only get, list, watch on secrets — no write access needed.
- Move PLATFORM_CLIENT_IDS comment block from Secret stringData back to the environments ConfigMap data section where it belongs - Add 'found' checks to AdminCredentialsFromSecret test so it fails if KEYCLOAK_ADMIN_USERNAME/PASSWORD env vars are absent entirely - Add 'found' checks to NonSensitiveKeysFromConfigMap test so it fails if KEYCLOAK_URL/KEYCLOAK_REALM env vars are absent entirely Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
vz-ibm
added a commit
to vz-ibm/kagenti-extensions
that referenced
this pull request
Aug 10, 2026
Reviewer OsherElhadad flagged three blocking issues (plus CodeQL alert rossoctl#179): 1. _patch_watsonx_for_reasoning_models was unconditional — it applied to every WatsonX deployment including the default mistral-large-2512, which supports response_format natively. That silently regressed structured output for existing users. Gate the patch behind a new SPARC_SCHEMA_IN_PROMPT=true setting (Settings.schema_in_prompt); the default path is now unchanged. 2. All three _patch_* helpers rebound methods on the shared ALTK class without a re-entry guard. ReflectionEngine caches components per track and lazily builds them, so a track switch would wrap the already-wrapped method — retry counts multiplied and debug lines duplicated on every subsequent call. Add _sparc_patched_reasoning / _sparc_patched_retry / _sparc_patched_debug sentinel attributes; each _patch_* no-ops if its sentinel is already set. 3. engine.reflect logged raw tool arguments at INFO. Args are caller-controlled — embedded newlines let a caller forge log lines (CodeQL alert rossoctl#179) — and can carry payloads (PII, payment ids, etc.). The same hunk had also dropped session_id/track from INFO. Restore session_id and track at INFO alongside tool/decision/score, and log args only at DEBUG when SPARC_LOG_REQUESTS=true. Also addresses smaller items from the same review: - tests/test_haiku_empty_response.py: replaced production PII in the conversation fixture (user id, names, DOBs, payment id, reservation ids) with synthetic values; gated both live-LLM probes behind pytest.skipif so they only run when RUN_HAIKU_TESTS=1 plus OAIKEY/OAIBASE are set (matches the reviewer's suggested opt-in path). - tests/test_providers.py: added three deterministic unit tests — retry wrapper recovers after two ValueError empty-response failures, retry wrapper re-raises unrelated ValueErrors, and all three patches are idempotent under repeat application. No credentials or network required; fills in the coverage gap the reviewer noted for the retry and schema-injection wrappers. Signed-off-by: Vitaly Zabershinsky <VITALYZ@il.ibm.com>
oblinder
pushed a commit
to s-and-p-team/cortex
that referenced
this pull request
Aug 12, 2026
Reviewer OsherElhadad flagged three blocking issues (plus CodeQL alert rossoctl#179): 1. _patch_watsonx_for_reasoning_models was unconditional — it applied to every WatsonX deployment including the default mistral-large-2512, which supports response_format natively. That silently regressed structured output for existing users. Gate the patch behind a new SPARC_SCHEMA_IN_PROMPT=true setting (Settings.schema_in_prompt); the default path is now unchanged. 2. All three _patch_* helpers rebound methods on the shared ALTK class without a re-entry guard. ReflectionEngine caches components per track and lazily builds them, so a track switch would wrap the already-wrapped method — retry counts multiplied and debug lines duplicated on every subsequent call. Add _sparc_patched_reasoning / _sparc_patched_retry / _sparc_patched_debug sentinel attributes; each _patch_* no-ops if its sentinel is already set. 3. engine.reflect logged raw tool arguments at INFO. Args are caller-controlled — embedded newlines let a caller forge log lines (CodeQL alert rossoctl#179) — and can carry payloads (PII, payment ids, etc.). The same hunk had also dropped session_id/track from INFO. Restore session_id and track at INFO alongside tool/decision/score, and log args only at DEBUG when SPARC_LOG_REQUESTS=true. Also addresses smaller items from the same review: - tests/test_haiku_empty_response.py: replaced production PII in the conversation fixture (user id, names, DOBs, payment id, reservation ids) with synthetic values; gated both live-LLM probes behind pytest.skipif so they only run when RUN_HAIKU_TESTS=1 plus OAIKEY/OAIBASE are set (matches the reviewer's suggested opt-in path). - tests/test_providers.py: added three deterministic unit tests — retry wrapper recovers after two ValueError empty-response failures, retry wrapper re-raises unrelated ValueErrors, and all three patches are idempotent under repeat application. No credentials or network required; fills in the coverage gap the reviewer noted for the retry and schema-injection wrappers. Signed-off-by: Vitaly Zabershinsky <VITALYZ@il.ibm.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
oblinder
pushed a commit
to s-and-p-team/cortex
that referenced
this pull request
Aug 12, 2026
Reviewer OsherElhadad flagged three blocking issues (plus CodeQL alert rossoctl#179): 1. _patch_watsonx_for_reasoning_models was unconditional — it applied to every WatsonX deployment including the default mistral-large-2512, which supports response_format natively. That silently regressed structured output for existing users. Gate the patch behind a new SPARC_SCHEMA_IN_PROMPT=true setting (Settings.schema_in_prompt); the default path is now unchanged. 2. All three _patch_* helpers rebound methods on the shared ALTK class without a re-entry guard. ReflectionEngine caches components per track and lazily builds them, so a track switch would wrap the already-wrapped method — retry counts multiplied and debug lines duplicated on every subsequent call. Add _sparc_patched_reasoning / _sparc_patched_retry / _sparc_patched_debug sentinel attributes; each _patch_* no-ops if its sentinel is already set. 3. engine.reflect logged raw tool arguments at INFO. Args are caller-controlled — embedded newlines let a caller forge log lines (CodeQL alert rossoctl#179) — and can carry payloads (PII, payment ids, etc.). The same hunk had also dropped session_id/track from INFO. Restore session_id and track at INFO alongside tool/decision/score, and log args only at DEBUG when SPARC_LOG_REQUESTS=true. Also addresses smaller items from the same review: - tests/test_haiku_empty_response.py: replaced production PII in the conversation fixture (user id, names, DOBs, payment id, reservation ids) with synthetic values; gated both live-LLM probes behind pytest.skipif so they only run when RUN_HAIKU_TESTS=1 plus OAIKEY/OAIBASE are set (matches the reviewer's suggested opt-in path). - tests/test_providers.py: added three deterministic unit tests — retry wrapper recovers after two ValueError empty-response failures, retry wrapper re-raises unrelated ValueErrors, and all three patches are idempotent under repeat application. No credentials or network required; fills in the coverage gap the reviewer noted for the retry and schema-injection wrappers. Signed-off-by: Vitaly Zabershinsky <VITALYZ@il.ibm.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
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
3 participants
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
main(resolving conflicts with feat: migrate demo-ui.md from demo realm to kagenti realm #149, feat: add client_credentials fallback for outbound requests without Authorization header #171, feat: add agent audience scope to platform clients during registration #172)KEYCLOAK_ADMIN_USERNAMEandKEYCLOAK_ADMIN_PASSWORDfrom theenvironmentsConfigMap to a dedicatedkeycloak-admin-secretSecretdemo-ui.md,demo-manual.md) with thekeycloak-admin-secretcreation stepPLATFORM_CLIENT_IDSenv var from feat: add agent audience scope to platform clients during registration #172 (the original PR inadvertently removed it due to being based on an older commit)Changes from original #161
kubectl create secret generic keycloak-admin-secretBuildClientRegistrationContainerWithSpireOption(current API)Related
Test plan
secretKeyReffor admin credsSigned-off-by: Mariusz Sabath mrsabath@gmail.com