feat(manifests): enable RBAC authorization in SaaS template#1667
Conversation
Flip --enable-authz from false to true now that the RBAC enforcement middleware has landed (PR #1660). Already live-patched on hcmais. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 11 minutes and 22 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds README prerequisites for Keycloak client and JWKS/configmap, replaces secret token envs with OIDC envs sourced from ChangesManifests OIDC wiring & pod hardening
Possibly Related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/manifests/templates/template-services.yaml (1)
191-195:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
readOnlyRootFilesystemto the API server pod containers.Both the
migrationinitContainer and theapi-servercontainer still missreadOnlyRootFilesystem, so this manifest does not meet the repo's restricted container profile.Suggested hardening
securityContext: + readOnlyRootFilesystem: true allowPrivilegeEscalation: false capabilities: drop: - ALL … securityContext: + readOnlyRootFilesystem: true allowPrivilegeEscalation: false capabilities: drop: - ALLAs per coding guidelines, "All containers must have restricted SecurityContext:
runAsNonRoot, dropALLcapabilities,readOnlyRootFilesystem".Also applies to: 297-301
🤖 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 `@components/manifests/templates/template-services.yaml` around lines 191 - 195, The manifest's containers are missing readOnlyRootFilesystem in their securityContext; update the securityContext for the migration initContainer and the api-server container to include readOnlyRootFilesystem: true (in addition to the existing allowPrivilegeEscalation: false, capabilities.drop: [ALL]) and ensure runAsNonRoot is set as well so both containers meet the repo's restricted profile; apply the same change to the other container block referenced (the second api-server/migration occurrence).Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@components/manifests/templates/template-services.yaml`:
- Around line 191-195: The manifest's containers are missing
readOnlyRootFilesystem in their securityContext; update the securityContext for
the migration initContainer and the api-server container to include
readOnlyRootFilesystem: true (in addition to the existing
allowPrivilegeEscalation: false, capabilities.drop: [ALL]) and ensure
runAsNonRoot is set as well so both containers meet the repo's restricted
profile; apply the same change to the other container block referenced (the
second api-server/migration occurrence).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 033b8ddc-764c-4bec-b81b-0b42cb9c444a
📒 Files selected for processing (1)
components/manifests/templates/template-services.yaml
- Add --jwk-cert-url CLI flag pointing to Keycloak JWKS endpoint; without it the framework defaults to sso.redhat.com and rejects tokens signed by our Keycloak instance - Remove unused JWK_CERT_URL env var (framework reads the CLI flag) - Remove CREDENTIAL_ENCRYPTION_ALLOW_PLAINTEXT=true - Add readOnlyRootFilesystem: true to all container securityContexts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
…client credentials Replace AMBIENT_API_TOKEN (static bearer token) with OIDC client credentials (client_credentials grant) for control-plane → api-server authentication. The control-plane exchanges a Keycloak client-id and client-secret for short-lived JWTs, validated by the same JWKS path the api-server already uses for user tokens. - Update SaaS template: replace AMBIENT_API_TOKEN with OIDC_TOKEN_URL, OIDC_CLIENT_ID, OIDC_CLIENT_SECRET from ambient-control-plane-oidc secret - Remove AMBIENT_API_TOKEN from api-server env (no longer needed) - Update hcmais overlay with concrete Keycloak token URL - Add deployment prerequisites section to manifests README Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The gRPC interceptor needs to know the control-plane's OIDC client ID to tag it as a service caller. Without this, gRPC watch streams authenticate the JWT but don't grant service-caller privileges, so session/project events are silently filtered out. Read from the same ambient-control-plane-oidc secret to stay in sync. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The refactor from raw SQL to g2.Create(msg) in PR #1660 caused Gorm to include seq=0 in INSERT statements, colliding with the unique constraint when a seq=0 row already existed. The -> tag tells Gorm to omit seq from writes and let the Postgres DEFAULT nextval() fire, while still reading the value back via the RETURNING clause. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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)
components/manifests/templates/template-services.yaml (1)
203-221:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProduction startup can crash if
credential-encryption-keyis missingWith
AMBIENT_ENV=production(Line 203) and optional keyring/version refs (Line 215, Line 221), the api-server can start without those envs set, then exit at runtime when encryption keyring is absent. This turns into CrashLoopBackOff on fresh deployments unless that secret is guaranteed.Recommend making these secret refs required and documenting
credential-encryption-keyas a deployment prerequisite alongside the new OIDC prereqs.🤖 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 `@components/manifests/templates/template-services.yaml` around lines 203 - 221, AMBIENT_ENV is set to production but the CREDENTIAL_ENCRYPTION_KEYRING and CREDENTIAL_ENCRYPTION_KEY_VERSION envs reference secretKeyRef entries that are marked optional: true, which can allow the pod to start and then fail at runtime; remove the optional: true on the secretKeyRef blocks for CREDENTIAL_ENCRYPTION_KEYRING and CREDENTIAL_ENCRYPTION_KEY_VERSION (keep the same secret name credential-encryption-key and keys keyring/version) so the manifest requires the secret at creation time, and add a short note in deployment docs/prereqs documenting that the credential-encryption-key secret must exist alongside the OIDC prereqs (and reference the AMBIENT_ENV/GRPC_SERVICE_ACCOUNT env names for clarity).
🤖 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 `@components/manifests/README.md`:
- Around line 149-150: The ACL ConfigMap command currently uses echo with a
literal '\n' which produces a single-line string; update the command that
generates acl.yml (the --from-file=acl.yml part) to emit a real multi-line YAML
string (for example use printf, echo -e, or a shell $'...' quoted string) so the
resulting acl.yml contains actual newlines and valid YAML mapping for the claim
and pattern; ensure the command still runs in a POSIX shell and produces content
matching "- claim: email\n pattern: ^.*$" across two lines so ACL loading works
correctly.
---
Outside diff comments:
In `@components/manifests/templates/template-services.yaml`:
- Around line 203-221: AMBIENT_ENV is set to production but the
CREDENTIAL_ENCRYPTION_KEYRING and CREDENTIAL_ENCRYPTION_KEY_VERSION envs
reference secretKeyRef entries that are marked optional: true, which can allow
the pod to start and then fail at runtime; remove the optional: true on the
secretKeyRef blocks for CREDENTIAL_ENCRYPTION_KEYRING and
CREDENTIAL_ENCRYPTION_KEY_VERSION (keep the same secret name
credential-encryption-key and keys keyring/version) so the manifest requires the
secret at creation time, and add a short note in deployment docs/prereqs
documenting that the credential-encryption-key secret must exist alongside the
OIDC prereqs (and reference the AMBIENT_ENV/GRPC_SERVICE_ACCOUNT env names for
clarity).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d5faeb87-c976-4b35-9ce3-3cabcfee410a
📒 Files selected for processing (3)
components/manifests/README.mdcomponents/manifests/overlays/hcmais/control-plane-env-patch.yamlcomponents/manifests/templates/template-services.yaml
| --from-file=jwks.json=<(curl -s <KEYCLOAK_REALM_URL>/protocol/openid-connect/certs) \ | ||
| --from-file=acl.yml=<(echo '- claim: email\n pattern: ^.*$') |
There was a problem hiding this comment.
Fix ACL ConfigMap command: current echo likely writes literal \n
Line 150 uses echo with \n inside single quotes, which commonly produces a single-line literal string instead of valid YAML. That can break ACL loading for authz.
Suggested fix
- --from-file=acl.yml=<(echo '- claim: email\n pattern: ^.*$')
+ --from-file=acl.yml=<(printf '%s\n' '- claim: email' ' pattern: ^.*$')🤖 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 `@components/manifests/README.md` around lines 149 - 150, The ACL ConfigMap
command currently uses echo with a literal '\n' which produces a single-line
string; update the command that generates acl.yml (the --from-file=acl.yml part)
to emit a real multi-line YAML string (for example use printf, echo -e, or a
shell $'...' quoted string) so the resulting acl.yml contains actual newlines
and valid YAML mapping for the claim and pattern; ensure the command still runs
in a POSIX shell and produces content matching "- claim: email\n pattern: ^.*$"
across two lines so ACL loading works correctly.
Merge Queue Status
This pull request spent 19 seconds in the queue, including 4 seconds running CI. Required conditions to merge |
1 participant
Summary
--enable-authzfromfalsetotruein the SaaS template now that RBAC enforcement middleware has landed (feat(api-server): RBAC enforcement with scope-aware authorization #1660)Test plan
--enable-authz=true🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Security
Hardening
Documentation