OSAC-1315: fix Helm deploy deadlock in setup.sh - #240
openshift-merge-bot[bot] merged 1 commit into
Conversation
WalkthroughThe setup script reorders initialization so namespace creation, CA bundle validation, and Keycloak-derived controller credentials are prepared before deployment. Helm mode additionally provisions external PostgreSQL, cert-manager Certificate, and TLS-enabled DB secrets prior to installing the osac chart. ChangesDeployment Initialization Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Risk Assessment: Medium-high severity. Reordering initialization affects bootstrap sequencing and secret lifecycles; failures in Keycloak secret extraction or cert-manager readiness can block deployments and may expose misconfiguration of TLS/DB connectivity. Verify secret existence, cert issuance, and namespace targeting before running in production. Suggested labels: Suggested reviewers:
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ 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: 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 `@scripts/setup.sh`:
- Around line 297-311: The Certificate resource named
postgres-client-cert-service is missing spec.usages so cert-manager may issue a
cert without the client auth EKU; add spec.usages: ["client auth"] to the
postgres-client-cert-service Certificate manifest (the resource with
metadata.name: postgres-client-cert-service and spec.privateKey.rotationPolicy)
so the issued certificate includes the client auth Extended Key Usage required
by PostgreSQL mTLS authentication.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 8a0a28f7-eefa-4fc1-96d8-6f9c94ac9c1b
📒 Files selected for processing (1)
scripts/setup.sh
1a77a60 to
284b6e0
Compare
omer-vishlitzky
changed the title
|
@omer-vishlitzky: This pull request references OSAC-1315 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 bug 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. |
| FC_CLIENT_SECRET=$(jq -er '.clients[] | select(.clientId == "osac-controller") | .secret // empty' prerequisites/keycloak/service/files/realm.json) | ||
| [[ -n "${FC_CLIENT_SECRET}" ]] || { echo "ERROR: Could not resolve secret for osac-controller in realm.json" >&2; exit 1; } | ||
| oc create secret generic fulfillment-controller-credentials \ | ||
| --from-literal=client-id=osac-controller \ | ||
| --from-literal=client-secret="${FC_CLIENT_SECRET}" \ | ||
| -n ${INSTALLER_NAMESPACE} \ | ||
| --dry-run=client -o yaml | oc apply -f - | ||
| fi |
There was a problem hiding this comment.
Ppassing secrets via --from-literal means they can show up in ps aux. Probably low risk in CI, but we could pipe them via stdin if we want to be totally secure. Not a blocker though.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
284b6e0 to
a86a7e7
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz, omer-vishlitzky 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 |
|
/retest |
1 similar comment
|
/retest |
3 participants
Summary
Fixes OSAC-1315 — the
e2e-vmaas-full-setup-helmperiodic job fails 100% becausesetup.shdeadlocks during Helm install.Root cause:
helm upgrade --install --waitblocks waiting for pods, but the pods need resources (ca-bundleConfigMap,fulfillment-controller-credentialssecret,fulfillment-dbsecret) that are either created after Helm returns or never created in Helm mode.Changes
Move
ensure-ca-bundle.shandfulfillment-controller-credentialscreation before the Helm install — these ran after theif/elseblock, buthelm --waitblocks indefinitely because pods mount them as required volumes. Now runs before for both modes (idempotent, no kustomize impact).Deploy the fulfillment database before OSAC Helm install (Helm mode only) — uses the existing
it/charts/postgreschart. The fulfillment-service chart expects an external PostgreSQL but nothing in the Helm path provisioned one. Also creates the client certificate andfulfillment-dbconnection secret.Create namespace early — needed so trust-manager can target it with the
ca-bundleBundle CR before Helm runs. Removes--create-namespacefrom the OSAC Helm install since it already exists.Evidence from the failing job
4 pods stuck in
ContainerCreatingfor 40 minutes until Helm times out:Equivalent kustomize job (build
2063773341717630976) passes.Test plan
e2e-vmaas-full-setup-helmperiodic job passesSummary by CodeRabbit