Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA new RFC document is introduced proposing replacement of OLMv0 automatic dependency installation with an OLMv1 umbrella operator approach using ClusterExtension to install and manage the full Kuadrant component set as a unified deployment. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
doc/proposals/olm-dependency-model-transition.md (4)
98-103: Add language identifier to code block.The fenced code block showing version pinning examples should specify a language identifier for better rendering and syntax highlighting (e.g.,
bash,sh, orenv).📝 Suggested fix
-``` +```bash KUADRANT_OPERATOR_IMAGE=quay.io/kuadrant/kuadrant-operator:v1.2.0 AUTHORINO_OPERATOR_IMAGE=quay.io/kuadrant/authorino-operator:v0.14.0 LIMITADOR_OPERATOR_IMAGE=quay.io/kuadrant/limitador-operator:v0.11.0 DNS_OPERATOR_IMAGE=quay.io/kuadrant/dns-operator:v0.8.0</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@doc/proposals/olm-dependency-model-transition.mdaround lines 98 - 103, The
fenced code block containing the environment variable examples
(KUADRANT_OPERATOR_IMAGE, AUTHORINO_OPERATOR_IMAGE, LIMITADOR_OPERATOR_IMAGE,
DNS_OPERATOR_IMAGE) needs a language identifier for proper syntax highlighting;
update the opening fence fromtobash (orenv/sh) so the block
reads as a bash/env snippet and retains the same variable lines unchanged.</details> --- `76-82`: **Expand tradeoff analysis for manifest management approaches.** The two approaches are described briefly, but the proposal would benefit from more detailed tradeoff analysis: - **Build pipeline**: How does each approach affect CI/CD complexity? - **Update workflow**: How do manifest-only changes propagate in each model? - **Testing**: Which approach simplifies integration testing? - **Reference implementation**: Which approach does cluster-olm-operator use, and why? This information would help stakeholders evaluate which approach to adopt and is closely tied to the unresolved question on line 177. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@doc/proposals/olm-dependency-model-transition.md` around lines 76 - 82, Expand the tradeoff analysis under "Component Manifest Management" by adding comparative details for the two approaches: describe CI/CD/build pipeline impacts for "Embedded manifests" and "Init container extraction", explain how manifest-only updates propagate in each model (rebuild image vs. independent component image rollout or volume updates), outline testing implications (which simplifies integration/unit/e2e testing and mock strategies), and state which pattern "cluster-olm-operator" uses and why (reference the "cluster-olm-operator pattern" and justify with operational/practical reasons); ensure you mention these four headings (Build pipeline, Update workflow, Testing, Reference implementation) so reviewers can directly map the tradeoffs to the unresolved question on line 177. ``` </details> --- `159-161`: **Consider basic component enable/disable flags in initial implementation.** Whilst selective deployment is appropriately scoped as future work, consider whether the initial implementation should include basic enable/disable flags for each component, even if all are enabled by default. This would: - Validate that the architecture truly supports selective deployment - Reduce migration burden when implementing the full `KuadrantInstall` CR API - Provide an escape hatch for users who want to manage specific components independently However, this may add complexity that's not warranted if the selective deployment timeline is uncertain. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@doc/proposals/olm-dependency-model-transition.md` around lines 159 - 161, Add basic per-component enable/disable flags to the initial implementation so operators can opt out of individual components; modify the deployment configuration and operator bootstrap code to read new boolean flags (e.g., componentEnablements map or individual flags like enableGateway, enableOIDCPolicy, enableTelemetry) and short-circuit creation/reconciliation of corresponding components when false, ensure safety checks in the umbrella operator reconcile loop to prevent removal if active policy CRs exist (reference KuadrantInstall as the eventual CR API to mirror), and document defaults as enabled so behavior remains unchanged unless flags are set. ``` </details> --- `128-128`: **Expand CRD conflict handling guidance.** The CRD conflict scenario mentions error reporting but doesn't specify the recovery path. Consider adding: - Whether users should manually remove conflicting CRDs - If the umbrella operator could support CRD adoption from existing installations - Whether a pre-flight check could detect conflicts before deployment This relates to the unresolved question on line 179 about migration from standalone installations and should be addressed together. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@doc/proposals/olm-dependency-model-transition.md` at line 128, The doc's CRD conflict note ("**CRD conflict** - if a CRD already exists on the cluster...") is too brief and lacks a recovery path; expand the section to enumerate clear remediation and automation options: state whether operators should manually remove conflicting CRDs, describe pros/cons and steps for manual removal, document a possible "CRD adoption" approach the umbrella operator could implement (including prerequisites and safety checks), and recommend adding a pre-flight conflict detection step that fails fast with instructions; also cross-reference and reconcile this guidance with the unresolved migration question about standalone installations (the note around line 179) so both places present a consistent migration strategy. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@doc/proposals/olm-dependency-model-transition.md:
- Around line 116-122: Update the "Upgrade Flow" section to add a new subsection
that defines explicit backwards-compatibility criteria and an acceptance-testing
plan: (1) for each dependency named in the flow (Authorino, Limitador, DNS)
state what API/behavioral contracts must be preserved (CRD shapes, REST/gRPC
endpoints, config options, semantic versioning rules) to be considered
backwards-compatible; (2) list concrete test scenarios and suites the release
must run (smoke rollbacks, mixed-version integration tests with old
kuadrant-operator + new dependencies, stateful migration tests, API contract
tests, e2e traffic validation) and where they run (CI vs release gating); (3)
describe failure detection and remediation steps (automated rollback, alerting,
blocked promotion) and who owns the decision; and (4) require maintaining a
version compatibility matrix mapping kuadrant-operator versions to supported
dependency versions. Reference the "Upgrade Flow" section, the umbrella
operator, kuadrant-operator, and the dependency names (Authorino, Limitador,
DNS) when inserting this subsection.- Around line 105-114: Expand the RBAC section under "RBAC" to include a
concrete permissions matrix: list exact API groups, resources and verbs (e.g.,
apiregistration.k8s.io/v1, apiextensions.k8s.io/v1 CRDs:
get,list,watch,create,update,patch,delete; apps/v1 Deployments:
get,list,watch,create,update,patch,delete; rbac.authorization.k8s.io
ClusterRole/ClusterRoleBinding: create,bind; core/v1
Namespaces/ServiceAccounts/Services: create,delete,get,list,watch), clearly mark
which permissions are cluster-scoped vs namespace-scoped, provide an example
ClusterRole manifest for the umbrella operator's ServiceAccount (referencing
"ClusterExtension's ServiceAccount" and "umbrella operator") and an example Role
for a component operator, and add a short comparison paragraph showing
differences vs the current kuadrant-operator RBAC scope so reviewers can see
added/removed privileges.- Around line 5-6: Update the two placeholder links in the document where the
strings "RFC PR:
Kuadrant/architecture#0000"
and "Issue tracking:
Kuadrant/architecture#0000"
appear: replace the#0000placeholders in both the pull request and issue
URLs/text with the actual PR number and actual issue number respectively so the
markdown links point to the real RFC PR and tracking issue before merging.
Nitpick comments:
In@doc/proposals/olm-dependency-model-transition.md:
- Around line 98-103: The fenced code block containing the environment variable
examples (KUADRANT_OPERATOR_IMAGE, AUTHORINO_OPERATOR_IMAGE,
LIMITADOR_OPERATOR_IMAGE, DNS_OPERATOR_IMAGE) needs a language identifier for
proper syntax highlighting; update the opening fence fromtobash (or
env/sh) so the block reads as a bash/env snippet and retains the same
variable lines unchanged.- Around line 76-82: Expand the tradeoff analysis under "Component Manifest
Management" by adding comparative details for the two approaches: describe
CI/CD/build pipeline impacts for "Embedded manifests" and "Init container
extraction", explain how manifest-only updates propagate in each model (rebuild
image vs. independent component image rollout or volume updates), outline
testing implications (which simplifies integration/unit/e2e testing and mock
strategies), and state which pattern "cluster-olm-operator" uses and why
(reference the "cluster-olm-operator pattern" and justify with
operational/practical reasons); ensure you mention these four headings (Build
pipeline, Update workflow, Testing, Reference implementation) so reviewers can
directly map the tradeoffs to the unresolved question on line 177.- Around line 159-161: Add basic per-component enable/disable flags to the
initial implementation so operators can opt out of individual components; modify
the deployment configuration and operator bootstrap code to read new boolean
flags (e.g., componentEnablements map or individual flags like enableGateway,
enableOIDCPolicy, enableTelemetry) and short-circuit creation/reconciliation of
corresponding components when false, ensure safety checks in the umbrella
operator reconcile loop to prevent removal if active policy CRs exist (reference
KuadrantInstall as the eventual CR API to mirror), and document defaults as
enabled so behavior remains unchanged unless flags are set.- Line 128: The doc's CRD conflict note ("CRD conflict - if a CRD already
exists on the cluster...") is too brief and lacks a recovery path; expand the
section to enumerate clear remediation and automation options: state whether
operators should manually remove conflicting CRDs, describe pros/cons and steps
for manual removal, document a possible "CRD adoption" approach the umbrella
operator could implement (including prerequisites and safety checks), and
recommend adding a pre-flight conflict detection step that fails fast with
instructions; also cross-reference and reconcile this guidance with the
unresolved migration question about standalone installations (the note around
line 179) so both places present a consistent migration strategy.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `da05c1dc-7456-4835-89bf-e3b5c7002143` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 725014c1ddb9ec567f4b51c46d884884ecd59cac and 0a1a7bc3a6c29afe3095e9f67628ea350bd8d274. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `doc/proposals/olm-dependency-model-transition.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1935 +/- ##
==========================================
- Coverage 75.15% 75.06% -0.10%
==========================================
Files 120 123 +3
Lines 10487 11594 +1107
==========================================
+ Hits 7882 8703 +821
- Misses 2223 2447 +224
- Partials 382 444 +62
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@jasonmadigan actually created in the kuadrant operator repo as that was useful for context but prob should move to arch repo tbh |
eguzki
left a comment
eguzki
left a comment
There was a problem hiding this comment.
This looks promising. I like the umbrella approach idea. A couple of unanswered questions after reading the proposal:
-
What will the new umbrella operator be named? I think kuadrant operator is the best candidate, with a kuadrant-controller component managing all policy reconciliation work.
-
What's being replaced? Will the current kuadrant operator's new OLM release become the OLMv1-compatible umbrella operator, or are we looking at a brand new operator release? Will the current operators continue to be released to the OLM public catalog?
+1
Still to be decided. In an ideal world we would replace and everything would "seamlessly" migrate to v1 compatible but I am not sure right now how feasible that is. |
eguzki
left a comment
eguzki
left a comment
There was a problem hiding this comment.
Leaving some further thoughts about it
| Each umbrella operator release embeds component image references as environment variables or build-time constants: | ||
|
|
||
| ``` | ||
| KUADRANT_OPERATOR_IMAGE=quay.io/kuadrant/kuadrant-operator:v1.2.0 |
There was a problem hiding this comment.
That would be the bundle image actually, right? quay.io/kuadrant/kuadrant-operator-bundle ?
|
|
||
| ### Upgrade Flow | ||
|
|
||
| 1. OLMv1 deploys the new umbrella operator image containing updated component image references. |
There was a problem hiding this comment.
This would be my only concern here. The umbrella operator would be doing OLM operator's job. We need to implement it and maintain it.
There was a problem hiding this comment.
Yes but we have to take on that role in some form given OLM dependencies are going away. We either build a big CSV with all the dependencies in it or we do something more controllable such as the umbrella operator.
Signed-off-by: craig <cbrookes@redhat.com>
…ers helm templates Signed-off-by: craig <cbrookes@redhat.com>
|
@eguzki @jasonmadigan @didierofrivia I have made some updates to the proposal. Mainly it focuses on leveraging Helm at the core of the new operator. @didierofrivia Interested in any thoughts you may have on this proposal @Boomatang you may be interested in this also |
|
Will move this to the arch repo instead of keeping it here actually |
|
Closing in favour of Kuadrant/architecture#179 |
4 participants
@jasonmadigan This covers the problem and a proposed approach that was recommended by the OLM team. This is a very early very generated doc at this point and needs digging into further and the details refining
Summary by CodeRabbit
Documentation