Conversation
Signed-off-by: craig <cbrookes@redhat.com>
Signed-off-by: craig <cbrookes@redhat.com>
|
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:
📝 WalkthroughWalkthroughThis pull request introduces RFC 0019 proposing Kuadrant deployment consolidation for OLMv1 support. The design proposes an umbrella operator that renders embedded Helm charts client-side and coordinates component deployment, whilst shifting kuadrant-operator responsibility to runtime policy reconciliation. It documents migration from OLMv0, upgrade sequencing, operational considerations, and design rationale. ChangesOLMv1 Operator Consolidation RFC
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
Collaborator
Author
|
@eguzki said: Kuadrant/kuadrant-operator#1935 (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, make the end user do it, or we do something more controllable such as the umbrella operator. |
Collaborator
Author
|
replaces: Kuadrant/kuadrant-operator#1935 |
![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
🤖 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 `@rfcs/0018-olmv1-support.md`:
- Line 97: The PR currently gates registering the dependency management
reconciler on startup CRD discovery (checking for
clusterextensions.olm.operatorframework.io), which creates an availability gap
if OLMv1 appears later; instead always register the dependency management
reconciler and move the CRD presence check into the reconciler (or a periodic
discovery routine) so each Reconcile (or a periodic check) uses discovery to
decide whether to act; update the startup logic in
internal/controller/state_of_the_world.go to stop skipping registration and add
per-reconcile gating (or a discovery watcher) that checks for the
clusterextensions.olm.operatorframework.io CRD before managing ClusterExtension
CRs.
- Line 11: The RFC currently asserts a "kuadrant-operator-managed
ClusterExtension" model; update the document to match the agreed design by
removing or replacing statements that say kuadrant-operator will create/manage
ClusterExtension CRs and instead describe the umbrella-operator + Helm-rendered
manifests approach (i.e., umbrella-operator is responsible for creating
operands/ClusterExtension resources or rendering Helm manifests that create
those operands), updating all occurrences referencing "kuadrant-operator",
"ClusterExtension", and "ClusterExtension CRs" (notably the sections around the
current lines mentioning those terms) so the architecture, diagrams and prose
consistently describe umbrella-operator behavior and Helm-rendered manifests
rather than kuadrant-managed OLMv1 CR creation.
- Line 118: The RFC currently uses fixed cluster-scoped resource names (e.g.,
"authorino-installer-binding" and the other fixed name referenced at line 131)
which will collide in multi-install scenarios; update the spec to either declare
a hard singleton contract (explicitly state "one Kuadrant instance per cluster"
and add wording about lifecycle/ownership implications) or define a
deterministic unique naming strategy (require resource names to include the
Kuadrant instance identity/ID/namespace prefix and show the templated naming
convention) and update all occurrences (including the
"authorino-installer-binding" reference) to use that strategy so ownership and
lifecycle are unambiguous in multi-install environments.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a52f1bbc-ae9c-416d-ab18-79bb195cf702
📒 Files selected for processing (1)
rfcs/0018-olmv1-support.md
Collaborator
Author
|
@jasonmadigan moved this to here. Updated with a focus on Helm being the main installation method and source of truth |
Signed-off-by: craig <cbrookes@redhat.com>
- Add extension operator concept for operators that complement kuadrant-operator - Include mcp-gateway-operator in umbrella operator deployment alongside dependency operators - Add OLM manifest removal section (optional per operator, standalone operators may keep bundles) - Update install, upgrade, and helm chart sections to reference mcp-gateway-operator Signed-off-by: craig <cbrookes@redhat.com>
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 `@rfcs/0018-olmv1-support.md`:
- Around line 139-143: Update the "Rollback" section to explicitly limit the
rollback guarantee: state that reverting a ClusterExtension version only
reconciles workload Deployments (image references) back to prior versions and
does NOT automatically downgrade CRDs or API schemas; add a clear CRD downgrade
policy (choose and document one of: disallow CRD downgrades and require
forward-only migration, require an explicit migration gate/manual operator
action to downgrade CRDs, or explicitly mark CRD downgrades as unsupported with
documented consequences) and reference ClusterExtension, CRD/schema, and
Deployment image rollback to make the scope unambiguous.
- Line 5: The RFC metadata contains a placeholder PR reference
("Kuadrant/architecture#0000") that breaks traceability; replace that
placeholder with the actual pull request reference for this RFC by updating the
"RFC PR:" line in rfcs/0018-olmv1-support.md to the correct GitHub PR number
(e.g., Kuadrant/architecture#1234) so the document points to the real review
thread.
- Around line 129-132: Update the "Version Pinning" section to declare a single
source-of-truth: state that umbrella operator release builds must consume pinned
component versions from the release-line contract (release.yaml) and that the
build pipeline injects those declared image tags into charts' values.yaml for
each umbrella operator; document the required release branch policy that any
change to component minors must update release.yaml first and then the
chart/value injection step (CI should gate builds to prevent divergence), and
reference the exact artifacts to change: release.yaml (authoritative mapping)
and each chart's values.yaml (injected pins).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e76e73f-65fc-470e-9ff2-063a9a9cb9cd
📒 Files selected for processing (1)
rfcs/0018-olmv1-support.md
mikenairn
reviewed
Jun 9, 2026
| With the umbrella operator owning deployment of all components via Helm charts, individual operators no longer need their own OLM packaging artifacts (CSV, bundle manifests, catalog entries, `bundle.Dockerfile`). These can optionally be removed from each operator repo, leaving only the Helm chart as the manifest source. | ||
|
|
||
| This is **optional per operator**. Operators such as authorino-operator and limitador-operator have standalone use cases outside of a Kuadrant installation and may choose to retain their own OLM bundles for independent installation via OLMv1. Operators that are only used as part of a Kuadrant deployment (e.g., mcp-gateway-operator, kuadrant-operator) are stronger candidates for removal. | ||
|
|
Member
There was a problem hiding this comment.
🎉 This IMO is significant and i would personally rather see this is a Must to remove all OLM complexity from individual component operator repos.
Boomatang
reviewed
Jun 9, 2026
Boomatang
left a comment
Boomatang
left a comment
Member
There was a problem hiding this comment.
I like how this will simplify the operators, and put helm to the for-forth. I have a concern that the umbrella operator will be an operator of operators which will have to deal with upgrade paths. In general I don't think we focus enough on the upgrade paths. There could easily be assumptions here that we are making without knowing.
| 4. **Separation of concerns** — the umbrella operator handles deployment; kuadrant-operator handles policy reconciliation and has no OLMv1 awareness. | ||
| 5. **Helm as single manifest source** — both installation paths (direct Helm (upstream plain kubernetes) and umbrella operator (OpenShift)) consume the same charts. | ||
| 6. **Umbrella operator owns operands** — installs each dependency operator and creates their operands (Authorino, Limitador instances). The kuadrant-operator no longer performs these tasks. | ||
| 7. **Extension operators managed centrally** — operators that depend on kuadrant-operator (e.g., mcp-gateway-operator) are deployed by the umbrella operator after kuadrant-operator, removing their need for OLM dependency declarations. |
Member
There was a problem hiding this comment.
and creates their operands (Authorino, Limitador instances)
I am assuming this is via their CRDs. The Limitador CR is one that I have question about in this instances. Who has ownership of the CR? The configuration for the limits are also in the limitador CR, so we will have both umbrella operator and kuadrant-operator trying to maintain the configuration.
Collaborator
Author
There was a problem hiding this comment.
Yes that mix of concerns is a frustration but is probably beyond this work to change (As in have separated limits). My initial thought here is that the Umbrella operator owns the resource but doesn't own the limits (if you get me). So wether the resource exits at all is up to the umbrella operator. What is in the limits is up to the kuadrant operator based on the policies. It is then an open question as to whether we want to eventually move these limit definitions out of the shared resource. WDYT
…on research and analysis Signed-off-by: craig <cbrookes@redhat.com>
Collaborator
Author
|
@Boomatang @mikenairn @eguzki @jasonmadigan I have updated this further now. This is a significant and large change if we go with it, so would appreciate some other opinions and thoughts on what I have outlined. If we generally agree on the approach I would like to prioritise it for the next release |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rfcs/0018-olmv1-support.md (2)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the RFC PR placeholder with the actual PR number.
The placeholder
Kuadrant/architecture#0000should be replaced withKuadrant/architecture#179to match this PR.📝 Proposed fix
-- RFC PR: [Kuadrant/architecture#0000](https://github.com/Kuadrant/architecture/pull/0000) +- RFC PR: [Kuadrant/architecture#179](https://github.com/Kuadrant/architecture/pull/179)🤖 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 `@rfcs/0018-olmv1-support.md` at line 5, Replace the RFC PR placeholder text `Kuadrant/architecture#0000` in the RFC header with the actual PR reference `Kuadrant/architecture#179`; locate the line containing `RFC PR: [Kuadrant/architecture#0000](https://github.com/Kuadrant/architecture/pull/0000)` and update both the displayed text and URL to use `179` so the link and label read `Kuadrant/architecture#179`.
319-320:⚠️ Potential issue | 🟠 MajorExpand CRD ownership conflict guidance (umbrella vs standalone) in OLMv1.
- The RFC states that the umbrella operator bundle includes all component CRDs, and that if a CRD already exists (e.g., standalone Authorino), OLM surfaces a CRD conflict via the ClusterExtension status—leaving no documented “what to do next”.
- Add resolution options that cover both migration and pre-existing/co-install scenarios (not only OLMv0→OLMv1):
- A shared/separate CRD bundle approach (if supported), or clearly document why it isn’t feasible.
- Document mutually exclusive installation paths (umbrella stack vs standalone components).
- Document a CRD ownership handoff/migration procedure (standalone→umbrella or umbrella→standalone).
🤖 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 `@rfcs/0018-olmv1-support.md` around lines 319 - 320, Update the "CRD ownership conflicts with standalone installations" paragraph to expand guidance: after describing the conflict between the umbrella operator bundle and an existing standalone Authorino (mentioning ClusterExtension status and OLMv1 single-ownership), add clear resolution options that cover migration and co-install cases — include (a) a shared vs separate CRD bundle approach with rationale and feasibility notes, (b) explicit mutually exclusive installation paths (umbrella stack vs standalone components) with recommended user actions, and (c) a CRD ownership handoff/migration procedure (standalone→umbrella and umbrella→standalone) describing the exact steps, risks, and status updates to perform the ownership transfer. Ensure you reference the umbrella operator bundle, standalone Authorino, and ClusterExtension status in the new guidance so readers can correlate the procedures with the existing text.
🧹 Nitpick comments (2)
rfcs/0018-olmv1-support.md (2)
60-60: ⚡ Quick winReplace vague parenthetical with explicit section reference.
The phrase "(owned by umbrella operator now see migration)" is unclear. Replace with a proper cross-reference to the section that explains the ownership change.
📝 Proposed fix
-5. Create the Kuadrant CR (owned by umbrella operator now see migration) (to configure) — kuadrant-operator begins reconciling policies. +5. Create the Kuadrant CR (reconciled by the umbrella operator; see [Kuadrant CR Ownership Change](`#kuadrant-cr-ownership-change`)) — kuadrant-operator begins reconciling policies.🤖 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 `@rfcs/0018-olmv1-support.md` at line 60, Replace the vague parenthetical "(owned by umbrella operator now see migration)" in the sentence "Create the Kuadrant CR (owned by umbrella operator now see migration) (to configure) — kuadrant-operator begins reconciling policies." with a clear cross-reference to the specific migration section or heading that explains ownership change (e.g., "see Migration and Ownership Changes, section 'Migration: Umbrella Operator Ownership'"). Locate the exact phrase in the rfcs/0018-olmv1-support.md content and update it to read something like: "Create the Kuadrant CR (owned by the umbrella operator — see Migration: Umbrella Operator Ownership) (to configure) — kuadrant-operator begins reconciling policies." Ensure the referenced section title matches the document's actual heading.
303-303: 💤 Low valueClarify or remove confusing alternative criticism.
The phrase "Doesn't put helm front and centre" appears to criticise the "single OLMv1 bundle" alternative, but is ambiguous. If the intent is to contrast it with the chosen design (which does make Helm the single source), rephrase to make this explicit: "Does not establish Helm as the single manifest source" or similar.
🤖 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 `@rfcs/0018-olmv1-support.md` at line 303, The phrase "Doesn't put helm front and centre" in the "Single OLMv1 bundle containing all operators" alternative is ambiguous; update that sentence to explicitly state the intended contrast with the chosen design (which uses Helm as the single source) by rephrasing it to something like "Does not establish Helm as the single manifest source" or remove the clause if unnecessary; locate the sentence in the "Single OLMv1 bundle containing all operators" paragraph and replace the ambiguous wording accordingly.
🤖 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 `@rfcs/0018-olmv1-support.md`:
- Line 281: Update the "Rollback" bullet text to clarify rollback scope: replace
the current line "- **Rollback** — (not currently an option with OLM and so out
of scope currently)" with wording that distinguishes workload rollback from
CRD/schema rollback, e.g. state that OLM/ClusterExtension supports reverting to
prior ClusterExtension versions (allowing workload image rollbacks via version
pinning) but CRD/schema downgrades are unsupported and unsafe—emphasize that
rolling back workloads does not revert CRDs and can cause compatibility issues;
ensure this change is applied to the "Rollback" heading/line in
rfcs/0018-olmv1-support.md.
- Line 30: The parenthetical "becomes kuadrant-controller" after
"kuadrant-operator" is ambiguous; either remove the parenthetical or add a
dedicated section describing the planned rename, rationale, timeline, and
migration steps. If renaming is intended, expand the RFC to include a "Rename
and Migration" section that references "kuadrant-operator" and
"kuadrant-controller", explains why the rename is needed, lists
compatibility/migration implications for users and operators, and any upgrade
steps; if there is no rename planned, simply delete the parenthetical to avoid
confusion.
- Around line 268-271: Update the "Version Pinning" section to declare a single
authoritative source for component version pins (e.g., a release.yaml maintained
in the umbrella operator repository) and state that the build pipeline is
responsible for reading that file and injecting the declared versions into each
chart's values.yaml at build time; mention the file name (release.yaml), the
target (charts' values.yaml), and the pipeline step (inject/publish) so
maintainers can trace the mapping and avoid drift.
- Line 256: Choose ConfigMap projection (not env vars) for runtime updates:
document a clear projection API contract such that the umbrella operator writes
a ConfigMap named "kuadrant-observability-<kuadrant-cr-name>" in the same
namespace as the Kuadrant CR containing a single key "dataPlane.yaml" (or
"dataPlane.json") with the serialized spec.observability.dataPlane payload;
kuadrant-operator must read this ConfigMap on startup and watch it for changes
(use the ConfigMap name pattern and key "dataPlane.yaml" as the canonical
identifiers in the spec.observability.dataPlane projection), and include this
contract text in the RFC so both the umbrella operator (projection producer) and
kuadrant-operator (consumer) implement the same naming/key conventions and
update semantics.
- Around line 52-53: Replace the current “undefined” assumption in the paragraph
titled “OLMv1 behaviour with existing dependency declarations is undefined” with
an explicit statement that operator-controller/OLMv1 does not support
olm.package.required, olm.gvk.required, or olm.constraint and will fail
install/upgrade attempts for CSVs that contain those fields; update the text to
require completing the OLMv0→umbrella migration so no Kuadrant CSVs carry those
dependency fields prior to switching to OLMv1 and add a concise
contingency/recovery checklist describing that affected operators will be left
unmanaged/unupdatable, must have dependency declarations removed or refactored,
and provide steps for recovery (identify impacted CSVs, revert to OLMv0 or
remove dependency fields, re-apply CSVs) to be followed if the migration was not
completed beforehand.
---
Outside diff comments:
In `@rfcs/0018-olmv1-support.md`:
- Line 5: Replace the RFC PR placeholder text `Kuadrant/architecture#0000` in
the RFC header with the actual PR reference `Kuadrant/architecture#179`; locate
the line containing `RFC PR:
[Kuadrant/architecture#0000](https://github.com/Kuadrant/architecture/pull/0000)`
and update both the displayed text and URL to use `179` so the link and label
read `Kuadrant/architecture#179`.
- Around line 319-320: Update the "CRD ownership conflicts with standalone
installations" paragraph to expand guidance: after describing the conflict
between the umbrella operator bundle and an existing standalone Authorino
(mentioning ClusterExtension status and OLMv1 single-ownership), add clear
resolution options that cover migration and co-install cases — include (a) a
shared vs separate CRD bundle approach with rationale and feasibility notes, (b)
explicit mutually exclusive installation paths (umbrella stack vs standalone
components) with recommended user actions, and (c) a CRD ownership
handoff/migration procedure (standalone→umbrella and umbrella→standalone)
describing the exact steps, risks, and status updates to perform the ownership
transfer. Ensure you reference the umbrella operator bundle, standalone
Authorino, and ClusterExtension status in the new guidance so readers can
correlate the procedures with the existing text.
---
Nitpick comments:
In `@rfcs/0018-olmv1-support.md`:
- Line 60: Replace the vague parenthetical "(owned by umbrella operator now see
migration)" in the sentence "Create the Kuadrant CR (owned by umbrella operator
now see migration) (to configure) — kuadrant-operator begins reconciling
policies." with a clear cross-reference to the specific migration section or
heading that explains ownership change (e.g., "see Migration and Ownership
Changes, section 'Migration: Umbrella Operator Ownership'"). Locate the exact
phrase in the rfcs/0018-olmv1-support.md content and update it to read something
like: "Create the Kuadrant CR (owned by the umbrella operator — see Migration:
Umbrella Operator Ownership) (to configure) — kuadrant-operator begins
reconciling policies." Ensure the referenced section title matches the
document's actual heading.
- Line 303: The phrase "Doesn't put helm front and centre" in the "Single OLMv1
bundle containing all operators" alternative is ambiguous; update that sentence
to explicitly state the intended contrast with the chosen design (which uses
Helm as the single source) by rephrasing it to something like "Does not
establish Helm as the single manifest source" or remove the clause if
unnecessary; locate the sentence in the "Single OLMv1 bundle containing all
operators" paragraph and replace the ambiguous wording accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ac19701-509c-44d1-ae80-41a6218aa9fd
📒 Files selected for processing (1)
rfcs/0018-olmv1-support.md
Single OLM bundle containing all operators as a lower-effort alternative. Trades selective deployment flexibility for significantly reduced engineering overhead. Signed-off-by: craig <cbrookes@redhat.com>
Collaborator
Author
|
@Boomatang @mikenairn @jasonmadigan @eguzki hold off on reviewing for now. |
… model Signed-off-by: craig <cbrookes@redhat.com>
Collaborator
Author
|
@Boomatang @mikenairn @jasonmadigan @eguzki @didierofrivia This is ready for further review now |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
rfcs/0019-olmv1-operator-consolidation.md (1)
24-24: 💤 Low valueFix grammar, punctuation, and markdown structure issues.
Static analysis flagged several grammar and markdown issues that impair clarity:
Grammar/punctuation (low-effort fixes):
- Line 24: "Currently on OpenShift OLMv1 and OLMv0 run alongside each other" — add comma: "Currently, on OpenShift, OLMv1 and OLMv0 run alongside each other" (or rewrite for flow).
- Line 30: "some that fall into Level 2 are not making" — verb agreement, change to "…some operators that fall into Level 2 are not making…".
- Line 45: "Fewer operators means a smaller security surface area" — consider "Fewer operators mean a smaller…" (plural subject).
- Lines 86, 153, 166, 253, 276: Missing commas in compound sentences; apply standard comma-before-conjunction rule for independent clauses.
- Line 294: "sub-components" — typically one word "subcomponents" (minor stylistic point).
Markdown structure:
- Heading levels (MD001): Lines 16, 54, 82, 260, 292 jump from h1 (#) or h2 (##) directly to h3 (###). RFC sections should follow h2 for subsection titles, not h3. Fix by changing
### Install on upstream Kubernetesto## Install on upstream Kubernetes, etc.- Unused anchor definitions (MD053): Lines 9, 14, 52, 80, 251, 258, 299, 308 define anchors (
[summary]:#summary``) that are not referenced in the document. These can be removed or kept for future linking; the build warning is harmless but indicates incomplete cleanup of the template.Also applies to: 30-30, 45-45, 86-86, 153-153, 166-166, 253-253, 276-276, 294-294
🤖 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 `@rfcs/0019-olmv1-operator-consolidation.md` at line 24, Fix the grammar, punctuation, and markdown structure issues in the RFC: add missing commas in the sentence “Currently on OpenShift OLMv1 and OLMv0 run alongside each other” (change to “Currently, on OpenShift, OLMv1 and OLMv0 run alongside each other”), correct subject/verb agreement in “some that fall into Level 2 are not making” (change to “some operators that fall into Level 2 are not making”), change “Fewer operators means” to “Fewer operators mean”, replace “sub-components” with “subcomponents”, and add commas before conjunctions in the compound sentences flagged (lines noted in the review). For markdown, change incorrectly leveled headings (e.g., “### Install on upstream Kubernetes” and other h3s that should be h2s) back to the proper h2 (##) and remove or consolidate unused anchor definitions like “[summary]: `#summary`” that are not referenced. Locate these edits by searching the document for the exact quoted phrases and the anchor tokens to apply the fixes.Source: Linters/SAST tools
🤖 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 `@rfcs/0019-olmv1-operator-consolidation.md`:
- Around line 72-77: Update the "Breaking changes" section to explicitly state
the migration and release plan for dropping the Authorino and Limitador CRDs:
indicate whether direct CR creation is unsupported or will be deprecated, add a
deprecation timeline and required release notes, commit to providing a migration
guide that maps Authorino/Limitador CR fields to Helm values and
kuadrant-operator behavior, and add instructions or tooling guidance for
detecting clusters that currently create these CRs (reference the Authorino and
Limitador CRDs and kuadrant-operator behavior where relevant).
- Around line 305-306: Update the "CRD ownership conflicts with standalone
installations" paragraph to explicitly acknowledge this as a hard blocker for
clusters with pre-existing standalone installations and add a short "Migration"
subsection that (a) documents a required pre-uninstall step to remove standalone
operators and their CRDs before installing the umbrella operator and (b) or
alternatively describes a detection-and-guidance pre-flight check that detects
existing Authorino/Limitador/dns-operator CRDs and surfaces a clear migration
message (e.g., "Detected standalone Authorino; uninstall authorino-operator and
remove the Authorino CRD, then retry"); pick and commit to either the
pre-uninstall or detection+guidance option in the RFC and briefly note CRD
version reconciliation as exploratory/unsupported.
- Line 149: Clarify and commit to concrete mechanisms for projecting
spec.observability.dataPlane and for Kuadrant CR ownership handoff: pick one
projection method (ConfigMap or Deployment env) and spell out sync semantics—if
ConfigMap, describe kuadrant-operator's watch/reload strategy (library used,
resync/poll interval, whether it supports in-memory reload without Pod restart)
and whether the umbrella operator patches the Deployment to trigger restarts; if
env vars, state that rolling restarts are expected and acceptable. For Kuadrant
CR handoff, document the lifecycle and ownership rules: where Kuadrant CRs are
initially created (user vs kuadrant-operator), whether the umbrella operator
will create missing CRs, the adoption flow (umbrella operator updates
ownerReferences on the Kuadrant CR and signals kuadrant-operator to stop
reconciling), and explicit handling of finalizers (detect existing finalizers,
remove or run finalizer logic safely before changing ownerReferences). Reference
spec.observability.dataPlane, kuadrant-operator Deployment/ConfigMap, Kuadrant
CR, ownerReferences, and finalizer in the RFC text so implementers can locate
and implement the exact behavior.
- Around line 182-188: The RFC's "control-plane gap" description is ambiguous
about what degrades and for how long; update the paragraph referencing the
control-plane gap to explicitly state that new policy CRs (e.g., AuthPolicy)
created while a component operator like authorino-operator is scaled to 0 will
not be reconciled and corresponding runtime CRs (e.g., AuthConfig) will not be
created until the replacement controller is active, quantify expected gap
duration per component (typical/median seconds or minutes for steps: scale-down
→ Helm apply → scale-up for Authorino/Limitador), and describe the idempotency
detection logic used by the umbrella operator (the exact state checks performed
before each step such as presence/absence of ownerReferences, resource
existence, observedGeneration/status conditions, and Helm release state) so
readers can verify the claim that steps are safe to resume after restarts.
- Around line 92-103: The RFC defers critical chart-design details needed by the
umbrella operator; expand the document to include a concise sketch of the Helm
chart layout and ordering (e.g., charts/kuadrant-operator split into
per-component templates: Deployment/Service/RBAC/ServiceAccount, CRDs placed in
crds/, and separate templates for Authorino and Limitador), a mapping showing
how Authorino/Limitador CR fields map to values.yaml keys, the render/apply
order the umbrella operator must follow (CRDs first, then
workloads/dependencies, then kuadrant-operator), and a brief namespace/RBAC
sharing strategy (which components share namespaces or ClusterRoles, and how to
toggle components via values). If this is out of scope for this RFC, move the
chart refactor design to a dedicated RFC/design doc and add a clear reference to
it here.
---
Nitpick comments:
In `@rfcs/0019-olmv1-operator-consolidation.md`:
- Line 24: Fix the grammar, punctuation, and markdown structure issues in the
RFC: add missing commas in the sentence “Currently on OpenShift OLMv1 and OLMv0
run alongside each other” (change to “Currently, on OpenShift, OLMv1 and OLMv0
run alongside each other”), correct subject/verb agreement in “some that fall
into Level 2 are not making” (change to “some operators that fall into Level 2
are not making”), change “Fewer operators means” to “Fewer operators mean”,
replace “sub-components” with “subcomponents”, and add commas before
conjunctions in the compound sentences flagged (lines noted in the review). For
markdown, change incorrectly leveled headings (e.g., “### Install on upstream
Kubernetes” and other h3s that should be h2s) back to the proper h2 (##) and
remove or consolidate unused anchor definitions like “[summary]: `#summary`” that
are not referenced. Locate these edits by searching the document for the exact
quoted phrases and the anchor tokens to apply the fixes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce4c671f-1d6f-4535-8f76-ea6bbadf8c1c
📒 Files selected for processing (1)
rfcs/0019-olmv1-operator-consolidation.md
Comment on lines
+92
to
+103
| ### Helm Chart Refactoring | ||
|
|
||
| The current kuadrant-operator chart (`charts/kuadrant-operator/`) is a monolithic kustomize-generated blob — a single `manifests.yaml` of ~14,500 lines produced by `kustomize build config/helm`. This must be refactored to support per-component rendering, ordered deployment, and direct workload deployment. | ||
|
|
||
| **Required changes:** | ||
|
|
||
| 1. **Replace kustomize generation with per-component Helm templates** — one template per component (Deployment, Service, RBAC, ServiceAccount). CRDs must be separated (via Helm's `crds/` directory or a dedicated template) since they must be established before resources that depend on them. | ||
| 2. **Configurable values.yaml** — image references, namespaces, replica counts, and resource limits exposed as values. The umbrella operator injects version-pinned values at render time; direct Helm users override via `--set` or values files. | ||
| 3. **Workload templates for Authorino and Limitador** — the chart deploys these workloads directly (Deployment, Service, RBAC) rather than creating operator CRs. Configuration previously exposed via the `Authorino` and `Limitador` CRs moves to Helm values. | ||
| 4. **Ordered rendering support** — the umbrella operator renders and applies charts sequentially: CRDs first, then workloads and dependencies, then kuadrant-operator. | ||
|
|
||
| > The exact layout and requirements for these charts is something to be investigated and beyond the details of this doc. |
There was a problem hiding this comment.
Defer chart refactoring design scope — RFC is incomplete without it.
Line 103 states: "The exact layout and requirements for these charts is something to be investigated and beyond the details of this doc." This defers a critical design dependency:
The umbrella operator's core responsibility is rendering and applying Helm charts in order. Without specifying:
- Per-component template isolation (how Authorino, Limitador, kuadrant-operator, dns-operator templates are separated).
- Dependency ordering (which components must be applied before others, e.g., CRDs before resources that reference them).
- Configuration parameterisation (how Authorino/Limitador CR fields map to
values.yamlkeys). - Namespace/RBAC sharing strategy (which components share namespaces, ClusterRoles, etc.).
…it is unclear whether the refactoring is feasible or what the umbrella operator's implementation complexity will be.
Recommendation: Provide at least a sketch of the chart structure in this RFC (e.g., directory layout, helm dependency model, or conditionals for component selection) or move the chart refactoring design to a separate RFC/design document and reference it here.
🤖 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 `@rfcs/0019-olmv1-operator-consolidation.md` around lines 92 - 103, The RFC
defers critical chart-design details needed by the umbrella operator; expand the
document to include a concise sketch of the Helm chart layout and ordering
(e.g., charts/kuadrant-operator split into per-component templates:
Deployment/Service/RBAC/ServiceAccount, CRDs placed in crds/, and separate
templates for Authorino and Limitador), a mapping showing how
Authorino/Limitador CR fields map to values.yaml keys, the render/apply order
the umbrella operator must follow (CRDs first, then workloads/dependencies, then
kuadrant-operator), and a brief namespace/RBAC sharing strategy (which
components share namespaces or ClusterRoles, and how to toggle components via
values). If this is out of scope for this RFC, move the chart refactor design to
a dedicated RFC/design doc and add a clear reference to it here.
Comment on lines
+182
to
+188
| #### Control plane gap | ||
|
|
||
| There is a brief window per component (between operator scale-down and Helm Deployment apply) where no controller is watching that component's configuration. During this window: | ||
|
|
||
| - Existing policies continue to be enforced by the running operands | ||
| - New policy changes or CR updates are not reconciled until the replacement is in place | ||
| - No user action is required — the umbrella operator handles the entire sequence |
There was a problem hiding this comment.
Clarify the control-plane gap: what functionality is actually degraded and for how long?
The RFC acknowledges a "control-plane gap" during migration where no controller watches the component, but claims "Existing policies continue to be enforced" with "No user action required". This needs preciseness:
-
Policy enforcement vs. policy creation: Authorino and Limitador continue serving traffic (correct), but what happens to new policy CRs created during the gap? If a user creates a new
AuthPolicywhile kuadrant-operator is scaled to 0 (step 6 of migration), the correspondingAuthConfigCR is not created. Authorino has nothing to enforce. This is not silent failure but degradation that should be disclosed. -
Gap duration per component: The migration steps are sequential (lines 157–168). The gap for Authorino is ~steps 1–3 (scale down authorino-operator → apply Helm Authorino). How long is this in practice? Seconds? Minutes? The RFC should state the expected window and whether users should pause traffic during migration.
-
Idempotency detection: Line 168 claims "Each step is idempotent — if the umbrella operator restarts mid-migration, it re-checks state and resumes." How is state checked? Is ownerReference presence sufficient? What if a resource is partially updated? The RFC should outline the state detection logic so readers can assess whether the claim is sound.
Recommendation: Rewrite lines 182–188 to:
- Explicitly state that new policy CRs created during the gap are not reconciled until the replacement controller is in place.
- Provide expected gap duration per component.
- Outline how idempotency is detected (state conditions checked before each step).
🤖 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 `@rfcs/0019-olmv1-operator-consolidation.md` around lines 182 - 188, The RFC's
"control-plane gap" description is ambiguous about what degrades and for how
long; update the paragraph referencing the control-plane gap to explicitly state
that new policy CRs (e.g., AuthPolicy) created while a component operator like
authorino-operator is scaled to 0 will not be reconciled and corresponding
runtime CRs (e.g., AuthConfig) will not be created until the replacement
controller is active, quantify expected gap duration per component
(typical/median seconds or minutes for steps: scale-down → Helm apply → scale-up
for Authorino/Limitador), and describe the idempotency detection logic used by
the umbrella operator (the exact state checks performed before each step such as
presence/absence of ownerReferences, resource existence,
observedGeneration/status conditions, and Helm release state) so readers can
verify the claim that steps are safe to resume after restarts.
Comment on lines
+305
to
+306
| - **CRD ownership conflicts with standalone installations** — if standalone Authorino is already installed on a cluster, the umbrella operator's bundle includes Authorino CRDs. Under OLMv1's single-ownership model, two bundles cannot own the same CRDs. | ||
|
|
There was a problem hiding this comment.
Acknowledge CRD ownership conflict blocker and propose mitigation path.
The RFC correctly identifies (as an unresolved question) that CRD ownership conflicts can occur if standalone Authorino/Limitador/dns-operator instances already exist on a cluster. Under OLMv1's single-ownership model, the umbrella operator's bundle cannot declare CRDs that another bundle already owns.
Scope: This affects only clusters with pre-existing standalone component installations (not greenfield or pure OLMv0-only clusters), but it is a hard blocker for those users.
Recommendation: Provide at least one mitigation path in the RFC:
- Pre-uninstall step: Document that users must uninstall standalone component operators before installing the umbrella operator. This is simple but imposes a manual step.
- Detection and guidance: The umbrella operator's pre-flight checks detect existing Authorino/Limitador CRDs and provide a migration guide (e.g., "Detected standalone Authorino; please uninstall authorino-operator and remove the Authorino CRD, then retry").
- CRD version reconciliation: Investigate whether OLMv1 allows upgrading a standalone bundle's CRDs under a new owning bundle (non-standard, unlikely to be supported).
Recommend committing to option 1 or 2 and documenting it in the RFC's "Migration" section for completeness.
🤖 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 `@rfcs/0019-olmv1-operator-consolidation.md` around lines 305 - 306, Update the
"CRD ownership conflicts with standalone installations" paragraph to explicitly
acknowledge this as a hard blocker for clusters with pre-existing standalone
installations and add a short "Migration" subsection that (a) documents a
required pre-uninstall step to remove standalone operators and their CRDs before
installing the umbrella operator and (b) or alternatively describes a
detection-and-guidance pre-flight check that detects existing
Authorino/Limitador/dns-operator CRDs and surfaces a clear migration message
(e.g., "Detected standalone Authorino; uninstall authorino-operator and remove
the Authorino CRD, then retry"); pick and commit to either the pre-uninstall or
detection+guidance option in the RFC and briefly note CRD version reconciliation
as exploratory/unsupported.
Boomatang
reviewed
Jun 15, 2026
Member
There was a problem hiding this comment.
Has there being scope creep in this? It now seems to be suggestion that we are dropping support for authorino-operator and the limitador-operator, with a rewrite of their logic into the kuadrant-operator. With the authorino-operator in particular, is there much of community usage of that operator? I assume we will be having to put a deprecation notice on the operators. Or is the idea to have both the logic in the kuadrant-operator, and have the standalone operators.
Collaborator
Author
There was a problem hiding this comment.
the scope creep here is dropping use of the operators in Kuadrant rather than deleting them entirely.
3 tasks
3 tasks
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
3 participants
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.
RFC for replacing OLMv0 dependency model with an umbrella operator.
Key decisions:
umbrella operator on OpenShift)
cluster-olm-operator
chart/umbrella operator
Closes #164
Summary by CodeRabbit