feat(manifests): rewrite SaaS templates to match hcmais deployment#1659
Open
jsell-rh wants to merge 3 commits into
Open
feat(manifests): rewrite SaaS templates to match hcmais deployment#1659jsell-rh wants to merge 3 commits into
jsell-rh wants to merge 3 commits into
Conversation
Replace legacy templates (backend-api, frontend, operator, public-api) with the actual stack running on hcmais (ambient-api namespace): - ambient-api-server: JWT+gRPC, init migration, credential encryption - ambient-api-server-db: RHEL PostgreSQL 16, emptyDir - ambient-control-plane: runner/sidecar image config, Vertex AI - ambient-ui: SSO credentials, readOnlyRootFilesystem All configuration parameterized — IMAGE_TAG, KEYCLOAK_REALM_URL, route hostnames, SSO callback, Vertex project. CRDs dropped from templates (already in kustomize base/crds/). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
📝 WalkthroughWalkthroughThe PR removes the operator template entirely and restructures the services template to consolidate database, API server, control plane, and UI deployments with updated security contexts, init containers, environment variable wiring for JWT/Keycloak/Vertex, routes, and template parameters. The validation script is updated to validate only the services template with expanded parameter arguments. ChangesKubernetes Manifest Consolidation
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 docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/templates/template-services.yaml`:
- Line 252: The template currently sets an extremely verbose log flag "-v=10"
which is too high for production; change it to a safer default (e.g., match the
base "-v=4") and make it configurable instead of hard-coded. Locate the "-v=10"
entry in template-services.yaml and replace it with a parameterized log level
(use a values/config variable such as logLevel or verbosity) and ensure the
template falls back to a sensible default (4) when the config value is not
provided.
- Around line 191-195: The migration init container ("migration") and the main
API container ("api-server") are missing readOnlyRootFilesystem in their
securityContext; add readOnlyRootFilesystem: true to both containers'
securityContext blocks, and if either binary needs writable paths, create
corresponding emptyDir volumes and mount them only at those specific writable
paths (mirror the UI deployment pattern for ephemeral writable mounts) so the
rest of the filesystem remains read-only.
- Around line 112-114: The manifest defines a volume named "data" using emptyDir
(volumes -> name: data -> emptyDir) which makes PostgreSQL data ephemeral;
replace or parameterize this with a persistentVolumeClaim
(persistentVolumeClaim: claimName: ambient-api-server-db-data) so the DB uses
the same PVC as the base manifest, or add a clear comment and a template/flag to
choose between emptyDir for previews and a PVC for production to avoid data loss
on pod restart.
🪄 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: 4c718f9f-c57c-40da-a76b-ab9ad0a3e55a
📒 Files selected for processing (3)
components/manifests/templates/template-operator.yamlcomponents/manifests/templates/template-services.yamlcomponents/manifests/templates/validate.sh
💤 Files with no reviewable changes (1)
- components/manifests/templates/template-operator.yaml
Comment on lines
+112
to
+114
| volumes: | ||
| - name: data | ||
| emptyDir: {} |
Contributor
There was a problem hiding this comment.
Database uses emptyDir—data lost on pod restart.
Per context snippet 1, the base manifest uses a PVC (persistentVolumeClaim: claimName: ambient-api-server-db-data). This template uses emptyDir, meaning all PostgreSQL data is ephemeral.
If this is intentional for the SaaS preview environment, consider adding a comment or providing a parameterized option for persistent storage in production scenarios.
🤖 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 112 -
114, The manifest defines a volume named "data" using emptyDir (volumes -> name:
data -> emptyDir) which makes PostgreSQL data ephemeral; replace or parameterize
this with a persistentVolumeClaim (persistentVolumeClaim: claimName:
ambient-api-server-db-data) so the DB uses the same PVC as the base manifest, or
add a clear comment and a template/flag to choose between emptyDir for previews
and a PVC for production to avoid data loss on pod restart.
Comment on lines
+191
to
+195
| securityContext: | ||
| allowPrivilegeEscalation: false | ||
| capabilities: | ||
| drop: | ||
| - ALL |
Contributor
There was a problem hiding this comment.
API server containers missing readOnlyRootFilesystem.
Per coding guidelines, all containers should have readOnlyRootFilesystem: true. Both the migration init container and the api-server container are missing this.
If the binary writes to the filesystem, add emptyDir volumes for writable paths (similar to the UI deployment pattern at lines 560-564).
🛡️ Suggested fix
securityContext:
+ readOnlyRootFilesystem: true
allowPrivilegeEscalation: false
capabilities:
drop:
- ALLApply to both the init container (line 191) and main container (line 297) security contexts.
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 migration init container ("migration") and the main API container
("api-server") are missing readOnlyRootFilesystem in their securityContext; add
readOnlyRootFilesystem: true to both containers' securityContext blocks, and if
either binary needs writable paths, create corresponding emptyDir volumes and
mount them only at those specific writable paths (mirror the UI deployment
pattern for ephemeral writable mounts) so the rest of the filesystem remains
read-only.
| - --cors-allowed-headers=X-Ambient-Project | ||
| - --grpc-server-bindaddress=:9000 | ||
| - --alsologtostderr | ||
| - -v=10 |
Contributor
There was a problem hiding this comment.
Verbose logging -v=10 in production template.
Log level 10 is very verbose. Context snippet 3 shows base uses -v=4. High verbosity can impact performance and storage costs. Consider parameterizing or defaulting to a lower level.
♻️ Suggested fix
- - -v=10
+ - -v=4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - -v=10 | |
| - -v=4 |
🤖 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` at line 252, The
template currently sets an extremely verbose log flag "-v=10" which is too high
for production; change it to a safer default (e.g., match the base "-v=4") and
make it configurable instead of hard-coded. Locate the "-v=10" entry in
template-services.yaml and replace it with a parameterized log level (use a
values/config variable such as logLevel or verbosity) and ensure the template
falls back to a sensible default (4) when the config value is not provided.
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
1 participant
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
Replaces legacy OpenShift SaaS templates (from PR #1642) with templates matching the actual hcmais deployment (
ambient-apinamespace).Removed: template-operator.yaml (backend-api, frontend, operator, public-api, CRDs, ConfigMaps — none deployed on hcmais)
Rewritten: template-services.yaml now contains only:
ambient-api-server— JWT+gRPC, init migration, credential encryption env varsambient-api-server-db— RHEL PostgreSQL 16, emptyDir, Recreate strategyambient-control-plane— runner/sidecar image config (all parameterized), Vertex AIambient-ui— SSO from sso-credentials Secret, readOnlyRootFilesystemAll values parameterized:
IMAGE_TAG,KEYCLOAK_REALM_URL, route hostnames, SSO callback, Vertex project ID. CRDs dropped (already in kustomizebase/crds/).Secrets (provisioned separately)
ambient-api-server-db,ambient-control-plane-token,ambient-cp-token-keypair,ambient-vertex,ambient-anthropic,credential-encryption-key(optional),sso-credentialsTest plan
bash components/manifests/templates/validate.sh— passesoc processwith test params produces valid manifests matching live hcmais state🤖 Generated with Claude Code
Summary by CodeRabbit