Skip to content

FIX resolve #822 changing cm to secret#161

Closed
cooktheryan wants to merge 1 commit into
rossoctl:mainfrom
cooktheryan:cm-to-secret
Closed

FIX resolve #822 changing cm to secret#161
cooktheryan wants to merge 1 commit into
rossoctl:mainfrom
cooktheryan:cm-to-secret

Conversation

@cooktheryan

Copy link
Copy Markdown
Contributor

Summary

Rather than storing keycloak creds in CM it should use a secret

Related issue(s)

rossoctl/rossoctl#822

(Optional) Testing Instructions

Fixes #
rossoctl/rossoctl#822

Signed-off-by: Ryan Cook <rcook@redhat.com>

@Ladas Ladas left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

@pdettori question, should the keycloak admin secret be in the team namespaces? Seem like it should be rather in some kagenti namespace and team namespace should get only client secret?

@pdettori

pdettori commented Mar 4, 2026

Copy link
Copy Markdown
Member

@Ladas right, we should not have keycloak admin secret be in the teams* namespaces

@cooktheryan

Copy link
Copy Markdown
Contributor Author

@pdettori @Ladas do you all want me to take a swing at that or solve in a followup issue and PR?

@Ladas

Ladas commented Mar 5, 2026

Copy link
Copy Markdown

@cooktheryan I think follow up PR is good, because this is pre-existing issue?

@cooktheryan

Copy link
Copy Markdown
Contributor Author

@pdettori would you be ok with @Ladas perspective and merge this?

@pdettori

pdettori commented Mar 6, 2026

Copy link
Copy Markdown
Member

@cooktheryan yes, looks good to me.

@pdettori

pdettori commented Mar 6, 2026

Copy link
Copy Markdown
Member

But let's check also with the AuthBridge team as @mrsabath is doing also changes in this area.

@pdettori
pdettori requested a review from mrsabath March 6, 2026 13:53
@mrsabath

mrsabath commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Thank you @cooktheryan for these changes. They are good, but they will affect the current demo we are trying to build. I propose we close on the demo work first, since it turned out to be more work that we initially anticipated (see epic #173 ) and once that is finally working successfully, we should merge this PR on the top of it. Are you OK with this approach?

@mrsabath

mrsabath commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Testing Update

The epic #173 (end-to-end demo validation) is now complete and all related PRs are merged. I've tested this PR locally with the following results:

Conflict Resolution:

E2E Verification:

  • Built webhook with the rebased changes and deployed to Kind cluster
  • Verified the injected sidecar spec now uses secretKeyRef (pointing to keycloak-admin-secret) instead of configMapKeyRef for KEYCLOAK_ADMIN_USERNAME and KEYCLOAK_ADMIN_PASSWORD
  • Client-registration sidecar successfully read credentials from the Secret and completed registration
  • Full agent flow works end-to-end (inbound validation, outbound client_credentials, tool invocation)

Note: The PR has conflicts with current main. I'll open a companion PR with the rebased version and updated demo docs that include the keycloak-admin-secret creation step.

@cooktheryan LGTM from the AuthBridge side. The conflicts are due to recent demo/webhook changes that landed while the PR was waiting.

@cooktheryan

Copy link
Copy Markdown
Contributor Author

@mrsabath I'm good with moving forward to your rebase. Massive respect for the review, testing and rebase.

You rock!

pdettori added a commit that referenced this pull request Mar 9, 2026
fix: move Keycloak admin credentials from ConfigMap to Secret (rebased #161)
@mrsabath

mrsabath commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

Closing -- all changes from this PR are included in the merged #179 (rebased version).

Verified that every change from @cooktheryan's original commit is on main:

  • container_builder.go: configMapKeyRef to secretKeyRef for admin creds -- VERIFIED
  • container_builder_test.go: AdminCredentialsFromSecret + NonSensitiveKeysFromConfigMap tests -- VERIFIED (with improved found checks from review feedback)
  • example_deployment.yaml: Secret + secretKeyRef for admin creds -- VERIFIED
  • example_deployment_spiffe.yaml: Secret + secretKeyRef for admin creds -- VERIFIED
  • configmaps-webhook.yaml: Admin creds moved to keycloak-admin-secret Secret -- VERIFIED
  • github-issue/configmaps.yaml: Admin creds moved to Secret -- VERIFIED (file was slimmed in feat: migrate demo-ui.md from demo realm to kagenti realm #149; admin creds no longer in this file)
  • config/rbac/role.yaml: secrets RBAC rule added -- VERIFIED
  • charts/clusterrole.yaml: secrets RBAC rule added -- VERIFIED
  • CLAUDE.md: keycloak-admin-secret documented -- VERIFIED
  • kagenti-webhook/CLAUDE.md: Runtime deps updated -- VERIFIED

Thank you @cooktheryan for the original contribution!

@mrsabath mrsabath closed this Mar 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants