feat(ambient-ui): add deploy skill, remove from production overlay#1622
Conversation
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProduction overlay removes ambient-ui Deployment, ServiceAccount, and Service via a new delete patch and stripped kustomization references. Comprehensive deployment documentation for ambient-ui covers OIDC/PKCE auth flow, Docker build setup, environment variables, native SSO prerequisites, and overlay onboarding. Skills discovery guide updated to reference deployment documentation. ChangesAmbient UI Production Removal and Deployment Docs
Possibly related PRs
🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
jsell-rh
force-pushed
the
jsell/feat/ambient-ui-deploy-skill
branch
from
d0812b7 to
1d7e4f5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
skills/ambient-ui/deploy/SKILL.md (1)
165-168: ⚡ Quick winAvoid
--from-literalfor sensitive secret values in docs.Using
--from-literal=sso-client-secret=...can leak secrets via shell history/process visibility. Document safer input methods (stdin/file/env-from-secure-source) for production runbooks.🤖 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 `@skills/ambient-ui/deploy/SKILL.md` around lines 165 - 168, The docs currently show creating the ambient-ui-config secret with --from-literal for sso-client-secret and session-secret which can expose secrets; update SKILL.md to recommend non-literal methods instead: instruct users to create the secret using files or stdin (e.g., write sso-client-secret and session-secret to secure files or export them from a secrets manager and use --from-file or --from-env-file / apply a YAML with stringData read from protected input) and show the ambient-ui-config secret name and keys (sso-client-id, sso-client-secret, session-secret) as the targets to populate rather than using --from-literal in examples. Ensure the documentation also suggests generating session-secret into a file via a secure command and removing the file from disk after creating the secret.
🤖 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 `@skills/ambient-ui/deploy/SKILL.md`:
- Line 101: The README currently recommends setting
NODE_TLS_REJECT_UNAUTHORIZED=0 for production which disables TLS verification;
remove that recommendation and replace it with guidance to trust the
cluster/service CA bundle instead. Update SKILL.md so the table row for
NODE_TLS_REJECT_UNAUTHORIZED either omits a production value or sets it to "1
(default)" and add a short note referencing how to configure a trusted CA (e.g.,
supplying the service CA bundle to the runtime or configuring
NODE_EXTRA_CA_CERTS), and include the env var name NODE_TLS_REJECT_UNAUTHORIZED
in the guidance so reviewers can find and verify the change.
---
Nitpick comments:
In `@skills/ambient-ui/deploy/SKILL.md`:
- Around line 165-168: The docs currently show creating the ambient-ui-config
secret with --from-literal for sso-client-secret and session-secret which can
expose secrets; update SKILL.md to recommend non-literal methods instead:
instruct users to create the secret using files or stdin (e.g., write
sso-client-secret and session-secret to secure files or export them from a
secrets manager and use --from-file or --from-env-file / apply a YAML with
stringData read from protected input) and show the ambient-ui-config secret name
and keys (sso-client-id, sso-client-secret, session-secret) as the targets to
populate rather than using --from-literal in examples. Ensure the documentation
also suggests generating session-secret into a file via a secure command and
removing the file from disk after creating the secret.
🪄 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: 67a7890d-d2ae-48ef-b349-474d0e16c91c
📒 Files selected for processing (3)
components/manifests/overlays/production/ambient-ui-delete-patch.yamlcomponents/manifests/overlays/production/kustomization.yamlskills/ambient-ui/deploy/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- components/manifests/overlays/production/kustomization.yaml
jsell-rh
force-pushed
the
jsell/feat/ambient-ui-deploy-skill
branch
3 times, most recently
from
d814d38 to
3b859bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/overlays/production/ambient-ui-oauth-patch.yaml`:
- Around line 13-14: The NODE_EXTRA_CA_CERTS env in the ambient-ui deployment is
pointing to the serviceaccount CA but ambient-api-server certs are signed by the
OpenShift service-ca; update
components/manifests/overlays/production/ambient-ui-oauth-patch.yaml to mount
the OpenShift service-ca bundle (service-ca.crt) into the ambient-ui pod (e.g.,
from the service-ca/configmap used by ambient-api-server) and change the
NODE_EXTRA_CA_CERTS value to the mounted path (service-ca.crt) so Node will
trust https://ambient-api-server:8000; ensure the mount name/path matches the
service-ca ConfigMap used in ambient-api-server-service-ca-patch.yaml and that
the ambient-ui container references that same mount and env var.
🪄 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: 2b6f83b8-ef5b-43fb-861f-eeb959551ab0
📒 Files selected for processing (4)
components/manifests/overlays/production/ambient-ui-delete-patch.yamlcomponents/manifests/overlays/production/ambient-ui-oauth-patch.yamlcomponents/manifests/overlays/production/kustomization.yamlskills/deployments/ambient-ui/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- components/manifests/overlays/production/kustomization.yaml
| - name: NODE_EXTRA_CA_CERTS | ||
| value: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find ambient-api-server manifests and inspect TLS/cert annotations and volumes.
rg -n -C3 --iglob '*.{yaml,yml,json}' 'ambient-api-server|serving-cert|service-ca|ca\.crt|tls' components/manifests
# 2) Find where NODE_EXTRA_CA_CERTS is set and whether corresponding CA volume/configmap is mounted.
rg -n -C4 --iglob '*.{yaml,yml,json}' 'NODE_EXTRA_CA_CERTS|serviceaccount/ca\.crt|service-ca\.crt|configMap|volumeMounts' components/manifests
# 3) Check if a service-ca injected ConfigMap pattern exists for ambient-ui or ambient-api-server.
rg -n -C3 --iglob '*.{yaml,yml,json}' 'service\.beta\.openshift\.io/inject-cabundle|openshift.*service-ca|service-ca' components/manifestsRepository: ambient-code/platform
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 1) Show NODE_EXTRA_CA_CERTS context in ambient-ui-oauth-patch.yaml =="
nl -ba components/manifests/overlays/production/ambient-ui-oauth-patch.yaml | sed -n '1,120p'
echo "== 2) Find all uses of NODE_EXTRA_CA_CERTS =="
rg -n --iglob 'components/manifests/**/*.y*ml' 'NODE_EXTRA_CA_CERTS' components/manifests
echo "== 3) Look for OpenShift injected cabundle annotations =="
rg -n --iglob 'components/manifests/**/*.y*ml' 'service\.beta\.openshift\.io/(inject-cabundle|inject-ca-bundle|inject-cabundle)' components/manifests || true
echo "== 4) Search for mounted service-ca bundle files/paths =="
rg -n --iglob 'components/manifests/**/*.y*ml' 'service-ca\.crt|openshift.*service-ca|service-ca|cabundle|ca-bundle|ca-bundle\.crt' components/manifests || true
echo "== 5) Inspect ambient-api-server service-ca patch =="
nl -ba components/manifests/overlays/production/ambient-api-server-service-ca-patch.yaml | sed -n '1,120p'
echo "== 6) Inspect ambient-api-server JWT/TLS mount notes (trust vs serving cert) =="
nl -ba components/manifests/overlays/production/ambient-api-server-jwt-args-patch.yaml | sed -n '1,140p'Repository: ambient-code/platform
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 1) Show NODE_EXTRA_CA_CERTS context in ambient-ui-oauth-patch.yaml =="
awk '{printf "%5d: %s\n", NR, $0}' components/manifests/overlays/production/ambient-ui-oauth-patch.yaml | sed -n '1,120p'
echo "== 2) Find all uses of NODE_EXTRA_CA_CERTS =="
rg -n --iglob 'components/manifests/**/*.y*ml' 'NODE_EXTRA_CA_CERTS' components/manifests
echo "== 3) Look for OpenShift injected cabundle annotations =="
rg -n --iglob 'components/manifests/**/*.y*ml' 'service\.beta\.openshift\.io/(inject-cabundle|inject-ca-bundle|inject-cabundle|inject-ca-bundle)' components/manifests || true
echo "== 4) Search for mounted service-ca bundle files/paths =="
rg -n --iglob 'components/manifests/**/*.y*ml' 'service-ca\.crt|openshift.*service-ca|cabundle|ca-bundle|ca-bundle\.crt' components/manifests || true
echo "== 5) Inspect ambient-api-server service-ca patch =="
awk '{printf "%5d: %s\n", NR, $0}' components/manifests/overlays/production/ambient-api-server-service-ca-patch.yaml | sed -n '1,120p'
echo "== 6) Inspect ambient-api-server TLS mount notes (trust vs serving cert) =="
awk '{printf "%5d: %s\n", NR, $0}' components/manifests/overlays/production/ambient-api-server-jwt-args-patch.yaml | sed -n '1,160p'Repository: ambient-code/platform
Length of output: 7671
🌐 Web query:
OpenShift service.beta.openshift.io inject-cabundle annotation mount path NODE_EXTRA_CA_CERTS service-ca.crt
💡 Result:
In OpenShift, the service.beta.openshift.io/inject-cabundle=true annotation is used to automatically inject the OpenShift service CA certificate into a ConfigMap [1][2]. When this annotation is applied to a ConfigMap, the cluster populates a key named service-ca.crt with the PEM-encoded service CA bundle [3][4]. To make this certificate available to an application, you must mount the ConfigMap as a volume in your Pod's deployment specification [1][2]. Explicitly referencing the service-ca.crt key in a volume mount ensures that the Pod will not start until the CA bundle has been successfully injected by the cluster [3][4]. Regarding NODE_EXTRA_CA_CERTS, this is an environment variable used by Node.js applications to trust additional custom CA certificates [5][6]. It is not directly related to the OpenShift service-ca.crt injection mechanism, but if your application requires the OpenShift service CA to establish TLS connections to internal services, you can point the NODE_EXTRA_CA_CERTS environment variable to the file path where the service-ca.crt is mounted [5]. For example, if you mount the ConfigMap at /etc/pki/tls/certs, you would set the environment variable as follows [5]: export NODE_EXTRA_CA_CERTS=/etc/pki/tls/certs/service-ca.crt Important considerations include: - Data deletion: Applying the inject-cabundle annotation to a ConfigMap will delete any existing data in that ConfigMap [7][2]. It is best practice to use a dedicated ConfigMap for the service CA [7][2]. - Optional mounting: If you do not want your Pod to be blocked from starting until the injection is complete, you can set the volume's optional parameter to true [1][2]. - Alternative Injection: For custom CA certificates that are not the service CA, OpenShift provides a separate mechanism using the config.openshift.io/inject-trusted-cabundle=true label on an empty ConfigMap [8][9]. This injects a merged bundle of user-provided and system CA certificates [8][9].
Citations:
- 1: https://docs.okd.io/4.16/security/certificates/service-serving-certificate.html
- 2: https://docs.okd.io/latest/security/certificates/service-serving-certificate.html
- 3: https://github.com/openshift/service-ca-operator/blob/769deb779b1c1cde182446bd4aa8e5e411e0df1d/README.md
- 4: https://github.com/openshift/service-ca-operator/blob/main/README.md
- 5: https://docs.redhat.com/en/documentation/red_hat_openshift_dev_spaces/3.16/html/user_guide/using-credentials-and-configurations-in-workspaces
- 6: https://stackoverflow.com/questions/70198705/how-can-i-set-node-extra-ca-certs-on-node
- 7: https://docs.okd.io/4.6/security/certificates/service-serving-certificate.html
- 8: https://github.com/openshift/openshift-docs/blob/main/modules/certificate-injection-using-operators.adoc
- 9: https://docs.redhat.com/en/documentation/openshift_container_platform/4.9/html/networking/configuring-a-custom-pki
Fix NODE_EXTRA_CA_CERTS to trust the OpenShift service-ca signer (not the serviceaccount CA).
components/manifests/overlays/production/ambient-ui-oauth-patch.yaml sets ambient-ui NODE_EXTRA_CA_CERTS to /var/run/secrets/kubernetes.io/serviceaccount/ca.crt (lines 13-14), but the manifests provide no OpenShift service-ca injected bundle/configmap mount for service-ca.crt. Since ambient-api-server serving certs are provisioned via OpenShift service-ca (ambient-api-server-service-ca-patch.yaml), ambient-ui should mount the matching service-ca service-ca.crt and point NODE_EXTRA_CA_CERTS at that path so Node validates https://ambient-api-server:8000 correctly.
🤖 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/overlays/production/ambient-ui-oauth-patch.yaml` around
lines 13 - 14, The NODE_EXTRA_CA_CERTS env in the ambient-ui deployment is
pointing to the serviceaccount CA but ambient-api-server certs are signed by the
OpenShift service-ca; update
components/manifests/overlays/production/ambient-ui-oauth-patch.yaml to mount
the OpenShift service-ca bundle (service-ca.crt) into the ambient-ui pod (e.g.,
from the service-ca/configmap used by ambient-api-server) and change the
NODE_EXTRA_CA_CERTS value to the mounted path (service-ca.crt) so Node will
trust https://ambient-api-server:8000; ensure the mount name/path matches the
service-ca ConfigMap used in ambient-api-server-service-ca-patch.yaml and that
the ambient-ui container references that same mount and env var.
0c10c03 to
69ade2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@skills/deployments/ambient-ui/SKILL.md`:
- Line 89: The verification uses a distroless-incompatible command "podman run
--rm --entrypoint=\"\" ambient-ui-test ls /app/server.js" which will fail
because distroless images lack shell utilities; replace that line in SKILL.md
with a Node-based existence check invoked with the same podman run/--entrypoint
flow (use "node -e" and Node's fs API such as fs.existsSync or fs.statSync) so
the container itself runs Node to assert /app/server.js exists and exits with a
proper status.
🪄 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: 4a9e5acf-bc2a-4018-af4b-02ece224a36f
📒 Files selected for processing (5)
components/manifests/overlays/production/ambient-ui-delete-patch.yamlcomponents/manifests/overlays/production/ambient-ui-oauth-patch.yamlcomponents/manifests/overlays/production/kustomization.yamlskills/deployments/ambient-ui/SKILL.mdskills/discover/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
- components/manifests/overlays/production/ambient-ui-oauth-patch.yaml
- components/manifests/overlays/production/kustomization.yaml
jsell-rh
force-pushed
the
jsell/feat/ambient-ui-deploy-skill
branch
from
69ade2f to
4862f3b
Compare
Add skills/ambient-ui/deploy/SKILL.md — deployment instructions covering Docker build, manifests, native SSO auth architecture, prerequisites, troubleshooting, and verification. Remove ambient-ui from production overlay via $patch: delete. Blocked until OIDC client on sso.redhat.com is provisioned for native SSO. Kind overlays (local dev) are unaffected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jsell-rh
force-pushed
the
jsell/feat/ambient-ui-deploy-skill
branch
from
4862f3b to
5d2908b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
skills/deployments/ambient-ui/SKILL.md (1)
83-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix distroless verification command to override entrypoint explicitly.
Line 89 still risks failing on images with a fixed entrypoint (common for distroless). Use
--entrypoint=nodeso the file-check command is guaranteed to execute.Proposed doc fix
-podman run --rm ambient-ui-test node -e 'require("fs").statSync("/app/server.js"); console.log("server.js found")' +podman run --rm --entrypoint=node ambient-ui-test -e 'require("fs").statSync("/app/server.js"); console.log("server.js found")'🤖 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 `@skills/deployments/ambient-ui/SKILL.md` around lines 83 - 90, The podman run check in SKILL.md uses "podman run --rm ambient-ui-test node -e ..." which can fail on distroless images with a fixed entrypoint; update the command to explicitly override the container entrypoint (use the podman/docker flag to set the entrypoint to node, e.g., --entrypoint=node) so the inline JS ("require('fs').statSync(...)") will run reliably even if the image declares an entrypoint.
🤖 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.
Duplicate comments:
In `@skills/deployments/ambient-ui/SKILL.md`:
- Around line 83-90: The podman run check in SKILL.md uses "podman run --rm
ambient-ui-test node -e ..." which can fail on distroless images with a fixed
entrypoint; update the command to explicitly override the container entrypoint
(use the podman/docker flag to set the entrypoint to node, e.g.,
--entrypoint=node) so the inline JS ("require('fs').statSync(...)") will run
reliably even if the image declares an entrypoint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 79580578-1d24-4252-8b48-eba4d8b81eee
📒 Files selected for processing (4)
components/manifests/overlays/production/ambient-ui-delete-patch.yamlcomponents/manifests/overlays/production/kustomization.yamlskills/deployments/ambient-ui/SKILL.mdskills/discover/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- components/manifests/overlays/production/kustomization.yaml
Merge Queue Status
This pull request spent 1 minute 22 seconds in the queue, including 13 seconds running CI. Required conditions to merge |
1 participant
Summary
skills/ambient-ui/deploy/SKILL.md): Comprehensive deployment instructions covering Docker build gotchas, manifest file map, environment config, auth architecture, prerequisites (manual steps), adding to new overlays, troubleshooting, and verification checklist.origin-oauth-proxycannot produce JWTs required by theambient-api-server(--enable-jwt=trueagainst Red Hat SSO). Added$patch: deleteto exclude ambient-ui Deployment, ServiceAccount, and Service from the production kustomize output. Kind overlays (local dev) are unaffected.Why
The ambient-ui deployment to staging surfaced many issues: SDK dist not built in Docker, standalone output path mismatch, probe 401s behind oauth-proxy, cookie secret size, RBAC for token reviews, HTTP vs HTTPS to API server, and ultimately an incompatible auth token format. The deploy skill captures all of this so future agents don't repeat these mistakes. The production removal prevents broken deploys until an OIDC client on
sso.redhat.comis provisioned and the sidecar is swapped tooauth2-proxy.Test plan
kustomize build overlays/production— 0 ambient-ui referenceskustomize build overlays/kind— 10 ambient-ui references (still included)skills/ambient-ui/deploy/SKILL.md🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Documentation