This repository was archived by the owner on Sep 9, 2026. It is now read-only.
MGMT-22988: Add Keycloak service accounts for admin and controller - #462
Merged
openshift-merge-bot[bot] merged 2 commits intoApr 28, 2026
Conversation
|
@jhernand: This pull request references MGMT-22988 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
jhernand
requested review from
CrystalChun
and removed request for
akshaynadkarni and
trewest
April 28, 2026 10:51
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
WalkthroughUpdates add configurable OAuth client-secret handling across integration tests, tooling, Helm charts, and manifests: integration tooling accepts and propagates a provided client secret and creates a Kubernetes secret; Keycloak Helm values now allow appending clients/users/groups from a base realm JSON and documentation for service-account clients and values is added; the default Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
charts/service/templates/grpc-server/authconfig.yaml (1)
83-86: Prefer values-driven admin service-account list in Helm template.These usernames are hardcoded while clients are now values-driven in this PR scope. Making this list configurable avoids auth drift if client IDs change.
♻️ Suggested Helm-safe refactor
+ {{- $adminServiceAccounts := .Values.auth.adminServiceAccounts | default (list "service-account-osac-admin" "service-account-osac-controller") }} # Admin service accounts are service accounts that are allowed to act as administrators. admin_service_accounts := { - "service-account-osac-admin", - "service-account-osac-controller", + {{- range $adminServiceAccounts }} + "{{ . }}", + {{- end }} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/service/templates/grpc-server/authconfig.yaml` around lines 83 - 86, The hardcoded admin_service_accounts set should be driven from Helm values to avoid drift; replace the literal assignment for admin_service_accounts with a Helm-templated construction that reads a values list (e.g. .Values.auth.adminServiceAccounts or .Values.clients.adminServiceAccounts) and renders each entry into the set (use a Helm range to emit items or toYaml/indent to format a list into the expected structure). Update the template where admin_service_accounts is defined so it iterates over the values list and falls back to an empty list or a sensible default if the values key is absent, ensuring the variable name admin_service_accounts remains the same.
🤖 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/keycloak/templates/realm.yaml`:
- Around line 23-25: The template currently concatenates .Values.clients into
$realm.clients and then blindly resets realm.roles.client[clientId], risking
duplicate client entries and wiping existing role mappings; update the logic
around $realm, .Values.clients, concat and realm.roles.client to detect
collisions: either validate and fail when any .Values.clients has a clientId
already present in $realm.clients or within .Values.clients itself, or build a
merge that skips/filters duplicate clientId values before calling set (so you
never overwrite existing realm.roles.client[clientId]); ensure checks reference
the clientId field and produce an explicit error/guard rather than
unconditionally appending and resetting.
In `@charts/service/templates/grpc-server/authconfig.yaml`:
- Around line 75-76: Fix the typo in the policy comment that currently reads
"situatioos" to "situations"; locate the comment block that starts with
"Emergency service accounts are Kubernetes service accounts that are allowed to
act as administrators in case" and correct the misspelled word "situatioos" to
"situations".
In `@it/it_suite_test.go`:
- Around line 56-58: The test suite currently logs the full config including
ClientSecret (struct field ClientSecret) via logger.Info("Configuration",
slog.Any("values", config)), so create a sanitized copy of the config
immediately before logging: clone the config used in it_suite_test.go, set
copy.ClientSecret to a redacted value (e.g. "" or "REDACTED"), and pass that
sanitized copy to logger.Info instead of the original config to prevent secrets
from being emitted.
In `@manifests/base/grpc-server/authconfig.yaml`:
- Around line 74-76: Fix the typo in the comment above the
emergency_service_accounts block: change "situatioos" to "situations" in the
comment that begins "Emergency service accounts are Kubernetes service accounts
that are allowed to act as administrators..." so the rendered and base manifests
are correct (refer to the emergency_service_accounts block in the
authconfig.yaml).
In `@README.md`:
- Around line 329-338: Add a short prerequisite step before the osac login
example that explains how to obtain or export the CA bundle named bundle.pem (so
the --ca-file argument is valid); e.g., instruct users to extract the cluster CA
into bundle.pem from the Kubernetes service/secret or kubeconfig (mentioning
bundle.pem by name) and where to place it locally, then reference using that
path in the osac login command shown; ensure this note appears immediately
before the osac login block so readers know to create or locate bundle.pem
first.
---
Nitpick comments:
In `@charts/service/templates/grpc-server/authconfig.yaml`:
- Around line 83-86: The hardcoded admin_service_accounts set should be driven
from Helm values to avoid drift; replace the literal assignment for
admin_service_accounts with a Helm-templated construction that reads a values
list (e.g. .Values.auth.adminServiceAccounts or
.Values.clients.adminServiceAccounts) and renders each entry into the set (use a
Helm range to emit items or toYaml/indent to format a list into the expected
structure). Update the template where admin_service_accounts is defined so it
iterates over the values list and falls back to an empty list or a sensible
default if the values key is absent, ensuring the variable name
admin_service_accounts remains the same.
🪄 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: CHILL
Plan: Pro
Run ID: e37c4fb8-6116-45a8-9f87-655e97faed00
📒 Files selected for processing (8)
README.mdcharts/keycloak/files/realm.jsoncharts/keycloak/templates/realm.yamlcharts/keycloak/values.yamlcharts/service/templates/grpc-server/authconfig.yamlit/it_suite_test.goit/it_tool.gomanifests/base/grpc-server/authconfig.yaml
💤 Files with no reviewable changes (1)
- charts/keycloak/files/realm.json
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 `@internal/cmd/service/start/controller/start_controller_cmd.go`:
- Around line 105-111: Add the missing CLI flag that sets the authClientSecret
field so the mutual-exclusion checks work: define a flags.StringVar binding
&runner.args.authClientSecret to the flag "auth-client-secret" (default ""),
with help text matching the existing "--auth-client-secret-file" message (e.g.
"OAuth client secret value. Mutually exclusive with
'--auth-client-secret-file'."). Place this alongside the existing
flags.StringVar for authClientSecretFile so the validation code that checks
runner.args.authClientSecret and runner.args.authClientSecretFile is reachable
and behaves as intended.
🪄 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: CHILL
Plan: Pro
Run ID: bc63e758-a519-4703-9423-130f60546599
📒 Files selected for processing (8)
charts/README.mdcharts/keycloak/README.mdcharts/service/templates/controller/deployment.yamlcharts/service/values.yamlinternal/cmd/service/start/controller/start_controller_cmd.goit/it_tool.gomanifests/README.mdmanifests/base/controller/deployment.yaml
✅ Files skipped from review due to trivial changes (2)
- charts/keycloak/README.md
- manifests/README.md
This is a preparation step for changing the controller to use a Keycloak service account instead of a Kubernetes service account for authentication. The `fulfillment-controller` client that was previously hardcoded in the base realm JSON has been removed. Instead, two new service account clients (`osac-admin` and `osac-controller`) are now created dynamically via the Helm chart values during integration test setup. The Keycloak realm Helm template has been updated to support additional clients passed through `.Values.clients`. It automatically appends them to the `clients` array and creates the corresponding empty `roles.client` entries that Keycloak requires. The OPA authorization policy in the authconfig has been restructured to distinguish between emergency Kubernetes service accounts (renamed to `emergency_service_accounts`) and regular admin service accounts (`admin_service_accounts`), which now includes the two new Keycloak service accounts. The integration test tool now supports the `IT_CLIENT_SECRET` environment variable, allowing developers to set a known client secret for the service accounts. When not set, a random secret is generated. This is useful for logging in with the CLI after preserving a test cluster. Related: https://redhat.atlassian.net/browse/MGMT-22988 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
jhernand
force-pushed
the
add_keycloak_service_accounts_for_controller
branch
from
April 28, 2026 14:18
c928376 to
c599c17
Compare
Replace the Kubernetes service account token authentication used by the controller with OAuth client credentials flow. The controller now authenticates to the fulfillment API using a client identifier and secret obtained from the same OAuth issuer used for user authentication. The `start controller` command gains `--auth-issuer-url`, `--auth-client-id`, and `--auth-client-secret` flags, each with a `...-file` variant that reads the value from a file (trimming whitespace). The direct and file-based variants are mutually exclusive. The Helm chart adds an `auth.controllerCredentials` value that follows the same projected volume pattern used by `database.connection`: users map keys from ConfigMaps or Secrets to `client-id` and `client-secret` parameters, giving flexibility over how credentials are stored. The issuer URL is taken from the existing `auth.issuerUrl` value. The old `--token-file` flag referencing the pod service account token is removed from the deployment template. The kustomize base controller deployment is updated to mount a well-known Secret (`fulfillment-controller-credentials`) with `client-id` and `client-secret` keys, and the issuer URL hardcoded to the same Keycloak address used by the other components in the base manifests. The integration tests create a Kubernetes Secret containing the `osac-controller` Keycloak service account credentials and pass it to the Helm chart via the new `auth.controllerCredentials` value. The documentation for the Keycloak chart, the service Helm chart, and the kustomize manifests is updated to explain the new prerequisites and configuration. Related: https://redhat.atlassian.net/browse/MGMT-22988 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
jhernand
force-pushed
the
add_keycloak_service_accounts_for_controller
branch
from
April 28, 2026 14:20
c599c17 to
e11b858
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This was referenced Apr 28, 2026
This was referenced May 6, 2026
This was referenced May 26, 2026
This was referenced Jun 3, 2026
1 task
5 tasks
2 tasks
5 tasks
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
fulfillment-controllerclient from the base Keycloak realm JSON andreplaces it with two dynamically created service account clients (
osac-adminandosac-controller) configured via Helm chart values during integration test setup..Values.clients,automatically appending them to the
clientsarray and creating the requiredroles.cliententries.
accounts (
emergency_service_accounts) and regular admin service accounts(
admin_service_accounts), granting both admin permissions.IT_CLIENT_SECRETenvironment variable support for setting a known client secret, useful forlogging in with the CLI after preserving a test cluster.
This is a preparation step for changing the controller to use a Keycloak service account instead of
a Kubernetes service account for authentication.
Test plan
osac-controllerandosac-adminclients and their service account users arecorrectly created in the Keycloak realm
Related: https://redhat.atlassian.net/browse/MGMT-22988
Summary by CodeRabbit
Documentation
New Features
Improvements
Removals