Repository navigation
fix(platform): add cert-manager dependency to webhook-cert consumers - #2726
Conversation
github-actions
Bot
added
size/XS
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughSix package source manifests now declare ChangesCert-manager dependency additions
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request optimizes the platform deployment sequence by explicitly defining a dependency on cert-manager for several core components. By ensuring that cert-manager is ready before these packages attempt to reconcile their Certificate resources, the changes eliminate unnecessary retry loops during initial cluster provisioning, significantly improving deployment speed and reliability. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates several core platform component definitions to include cozystack.cert-manager in their dependsOn lists, ensuring proper initialization order. Review feedback suggests that the oidc variant of cozystack-engine should also include this dependency to maintain consistency across variants. Additionally, it was noted that modifications to the platform package require a migration update or an explicit note on backward compatibility according to the project's style guidelines.
| - name: default | ||
| dependsOn: | ||
| - cozystack.networking | ||
| - cozystack.cert-manager |
There was a problem hiding this comment.
The oidc variant of cozystack-engine (defined at line 58) also includes the lineage-controller-webhook and other components that likely depend on cert-manager being ready. It should probably also have cozystack.cert-manager added to its dependsOn list (around line 59) to ensure consistent behavior across variants and avoid reconcile delays.
There was a problem hiding this comment.
Good catch — the oidc variant installs the same components (cozystack-api and lineage-controller-webhook) that render cert-manager.io/v1 Certificate resources, so it needs the dependency too. Added cozystack.cert-manager to the oidc variant's dependsOn in 1ff3fc9.
| - name: default | ||
| dependsOn: | ||
| - cozystack.networking | ||
| - cozystack.cert-manager |
There was a problem hiding this comment.
Per the repository style guide (line 78), changes to the platform package should include a migration update or a note on backward compatibility. While adding a dependency is generally safe, please confirm if a migration version bump is needed for existing clusters to apply these new dependency constraints correctly.
References
- Flag any change to the platform package that lacks a corresponding migration update or an explicit note that backward compatibility is preserved. (link)
There was a problem hiding this comment.
No migration update is needed here. The change is purely a declarative addition to PackageSource.spec.variants[].dependsOn, which cozystack-controller reads on each reconcile and translates into HelmRelease.spec.dependsOn. New installs pick up the constraint at first reconcile; existing clusters get the updated ordering applied non-destructively, since cozystack.cert-manager was always installed before these consumers in practice — this PR just makes the ordering explicit. No data or schema migration is involved, so backward compatibility is preserved.
![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 `@packages/core/platform/sources/cozystack-engine.yaml`:
- Line 16: Update the PR summary to include
packages/core/platform/sources/cozystack-engine.yaml and add
cozystack.cert-manager to the oidc variant's dependsOn list to ensure webhook
cert consumers install after cert-manager; specifically, in
cozystack-engine.yaml add "- cozystack.cert-manager" under
variants.default.dependsOn (already present) and also under the oidc variant's
dependsOn so that system/cozystack-api and system/lineage-controller-webhook
have cert-manager available before they render cert-manager.io/v1 Certificate
resources; ensure the PR description lists cert-manager alongside
cert-manager-issuers, keycloak, kubevirt, kubevirt-cdi-operator, and
victoria-metrics-operator.
🪄 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: Pro
Run ID: 39bee7f8-2746-484e-a495-b6d141c48db3
📒 Files selected for processing (5)
packages/core/platform/sources/capi-operator.yamlpackages/core/platform/sources/cozystack-engine.yamlpackages/core/platform/sources/ingress-nginx.yamlpackages/core/platform/sources/kamaji.yamlpackages/core/platform/sources/rabbitmq-operator.yaml
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. Minimal correctness fix (+6 lines across 5 files), each of the 5 packages does render cert-manager.io/v1 Certificate CRs, and the oidc variant of cozystack-engine is correctly covered.
Verified each chart actually renders a cert-manager Certificate (so the dependency is justified, not cargo-cult):
system/capi-operator→charts/cluster-api-operator/templates/operator-components.yaml(cert-manager.io/v1 block in the vendored CAPI operator chart)system/cozystack-api→templates/certmanager.yamlsystem/lineage-controller-webhook→templates/certmanager.yamlsystem/ingress-nginx→charts/ingress-nginx/templates/admission-webhooks/cert-manager.yamlsystem/kamaji→charts/kamaji/templates/{certmanager_issuer,certmanager_certificate}.yamlsystem/rabbitmq-operator→templates/messaging-topology-operator.yml
oidc variant coverage: yq '.spec.variants[] | select(.name == "oidc") | .components[].path' on cozystack-engine.yaml gives cozystack-api and lineage-controller-webhook among its components — both render Certificates — so the oidc variant needs the same gate. Daniil's second commit lifts the dependency into oidc as well, matching what Gemini flagged.
Why this matters on cold installs (matches PR body): without dependsOn: cozystack.cert-manager, helm-controller renders the Certificate manifest, the cert-manager webhook isn't admitting requests yet (its own pod is still rolling out / its Service endpoints haven't reconciled), the apply gets a webhook-not-available rejection, and helm-controller retries on its interval cadence until the webhook comes up — typically 2-3 min per affected package, sometimes more under contention. With the dependency added, the dependent HR doesn't even reconcile until cert-manager's HR is Ready, which for cert-manager is gated on the webhook Deployment's readiness probe (which only flips Ready once the webhook can actually admit). Net: one of the more common "first install is slow" complaints on cold clusters is fixed deterministically.
Bot nudges:
- Gemini medium (oidc variant missing the dep) — addressed in the second commit.
- Gemini low ("per style guide, platform changes might need a migration update") — N/A here.
PackageSource.spec.variants[].dependsOnis helm-controller orchestration metadata, not a chart-rendered manifest. There's no runtime state to migrate — on next reconcile the platform controller picks up the new dependency declaration and gates accordingly. Nomigrations/script needed.
No CI failures on the path I can read (DCO + CodeRabbit pass; e2e in progress on this SHA but the pre-rebase ancestor's e2e was green and these 6 lines can't plausibly regress anything).
The Slack message that flagged this PR for review is one of the few cozystack-org PRs in #review-plz I hadn't picked up yet — happy to land it as soon as someone with merge rights wants to.
Five PackageSources whose charts apply cert-manager Certificate CRs and mount the resulting Secret on a webhook Deployment lacked an explicit dependsOn: cozystack.cert-manager. Under Flux v2.8 helm-controller v1.5.0 kstatus, the dependent Deployment trips progressDeadlineSeconds (10m default) when cert-manager controller isn't running yet, and the HR is failed early. Repeatedly observed on PR #2619 with capi-operator, kamaji, rabbitmq-operator, cozystack-api (cozystack-engine), and ingress-nginx all cascading to InstallFailed at ~12-15m into a fresh install. The mariadb-operator, kubeovn-webhook, snapshot-controller, etcd-operator, postgres-operator, mongodb-operator, victoria-metrics-operator, linstor, and linstor-scheduler PackageSources already had the dependency, which is why they weren't in the failure set. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]> (cherry picked from commit 0fc0796)
The oidc variant installs system/cozystack-api and system/lineage-controller-webhook, both of which render cert-manager.io/v1 Certificate resources. Add cozystack.cert-manager to the oidc variant's dependsOn so it matches the default variant and webhook-cert consumers do not race the cert-manager controller on cold installs. Address review feedback from gemini-code-assist and coderabbitai on packages/core/platform/sources/cozystack-engine.yaml. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
e7fdaf3 to
535f881
Compare
2 participants
What this PR does
Adds
cert-manageras adependsOnentry to the five PackageSources whose charts applycert-manager.io/v1CertificateCRs and need the webhook ready before their first reconcile. Without this, the helm install renders Certificate manifests, the cert-manager webhook isn't admitting requests yet (cold cluster), the apply fails, and helm-controller retries until the webhook eventually comes up — burning ~2-3 minutes per affected package on cold installs.The five PackageSources:
capi-operator,cozystack-engine,ingress-nginx,kamaji, andrabbitmq-operator(the platformcozy-systemset with webhook-cert consumers; not the apps layer).In
cozystack-engine, the dependency is added to both thedefaultandoidcvariants — theoidcvariant installs the samecozystack-apiandlineage-controller-webhookcomponents that render Certificate resources, so it had the same race.Origin
First commit lifted unchanged from #2619. Second commit (
oidcvariant) added in response to review feedback.Release note
Summary by CodeRabbit
Release Notes