OCPBUGS-85579: Move Console resources from bundle to runtime with capability detection#455
OCPBUGS-85579: Move Console resources from bundle to runtime with capability detection#455sebrandon1 wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-reference
|
@sebrandon1: This pull request references Jira Issue OCPBUGS-85579, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
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:
WalkthroughThis PR adds embedded OpenShift Console samples and a QuickStart, reconciles them through a capability-gated controller, wires dynamic-client startup support, and updates RBAC for ClusterVersion and Console resources. ChangesConsole resources
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RunOperator
participant ControllerSet
participant ConsoleResourcesController
participant ClusterVersion
participant DynamicClient
RunOperator->>ControllerSet: pass dynamic client
ControllerSet->>ConsoleResourcesController: register controller
ConsoleResourcesController->>ClusterVersion: read Console capability
ClusterVersion-->>ConsoleResourcesController: return enabled capabilities
ConsoleResourcesController->>DynamicClient: apply embedded Console resources
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 11❌ Failed checks (1 warning, 10 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
bindata/console/cert-manager-example-quickstart.yaml (1)
5-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale/inconsistent
include.release.openshift.io/*annotations.These CVO release-payload inclusion annotations are inert for a resource applied via dynamic client at runtime, and this PR removed the same annotations from the sibling
ConsoleYAMLSamplemanifests (acme-issuer, certificate, issuer) and reportedly fromconfig/console/cert-manager-example-quickstart.yamlas well. Leaving them here looks like a leftover from before this PR's annotation cleanup rather than an intentional keep.🧹 Proposed cleanup
metadata: name: cert-manager-example - annotations: - include.release.openshift.io/ibm-cloud-managed: "true" - include.release.openshift.io/self-managed-high-availability: "true" - include.release.openshift.io/single-node-developer: "true" spec:🤖 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 `@bindata/console/cert-manager-example-quickstart.yaml` around lines 5 - 8, Remove the stale include.release.openshift.io/* annotations from the cert-manager example quickstart manifest so it stays consistent with the cleaned-up sibling ConsoleYAMLSample manifests. Update the cert-manager-example-quickstart YAML by deleting the annotations block under the sample metadata, keeping the resource aligned with the annotation cleanup already applied elsewhere.bundle/manifests/cert-manager-operator.clusterserviceversion.yaml (1)
413-431: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRBAC grants broader verbs than the controller actually uses.
consoleResourcesController(pkg/controller/certmanager/console_resources.go) only callsClusterVersions().Get(...)for a single named object andGet/Create/Updateonconsoleyamlsamples/consolequickstarts. The new rules additionally grantlist/watchonclusterversionsanddelete/patch/watchon the console resources, none of which are exercised by this controller.🔒 Proposed trim to match actual usage
- apiGroups: - config.openshift.io resources: - clusterversions - featuregates verbs: - get - - list - - watch - apiGroups: - console.openshift.io resources: - consolequickstarts - consoleyamlsamples verbs: - - create - - delete - - get - - list - - patch - - update - - watch + - create + - get + - updateAs per path instructions,
**/*.{yaml,yml}Kubernetes manifests should follow "RBAC: least privilege; no cluster-admin for workloads."🤖 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 `@bundle/manifests/cert-manager-operator.clusterserviceversion.yaml` around lines 413 - 431, Trim the RBAC in the CSV manifest to match consoleResourcesController’s actual usage: in pkg/controller/certmanager/console_resources.go it only needs ClusterVersions().Get for a single object and Get/Create/Update on consoleyamlsamples and consolequickstarts. Remove the extra list/watch verbs for clusterversions and the delete/patch/watch verbs on the console resources so the permissions follow least privilege.Source: Path instructions
pkg/controller/certmanager/console_resources.go (2)
80-147: 🎯 Functional Correctness | 🔵 TrivialNo test coverage for the new controller.
sync,hasConsoleCapability, andapplyConsoleResourceimplement the core capability-gating and apply logic for this PR but ship with no unit tests (e.g. using fake dynamic/config clientsets) to verify skip-on-disabled-capability, create-on-not-found, and update-on-existing paths.Do you want me to draft unit tests using
k8s.io/client-go/dynamic/fakeand the fake config clientset for these three code paths?🤖 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 `@pkg/controller/certmanager/console_resources.go` around lines 80 - 147, Add unit tests for consoleResourcesController covering the new sync, hasConsoleCapability, and applyConsoleResource paths using fake config/dynamic clients. Verify sync skips when Console capability is disabled, proceeds when enabled, and returns wrapped errors from capability lookup or apply failures. Also test applyConsoleResource for create-on-not-found and update-on-existing behavior, referencing consoleResourcesController, hasConsoleCapability, and applyConsoleResource so the tests stay resilient if lines move.
149-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSilent GVR fallback for unrecognized kinds.
gvrForKindmaps any kind other than"ConsoleQuickStart"toconsoleYAMLSampleGVR, including kinds that aren'tConsoleYAMLSample. If a future asset file has an unexpected/typo'dkind, this will silently route the apply to the wrong GVR instead of failing with a clear error at decode time.♻️ Proposed fix: fail explicitly on unknown kinds
-func gvrForKind(kind string) schema.GroupVersionResource { - switch kind { - case "ConsoleQuickStart": - return consoleQuickStartGVR - default: - return consoleYAMLSampleGVR - } -} +func gvrForKind(kind string) (schema.GroupVersionResource, error) { + switch kind { + case "ConsoleQuickStart": + return consoleQuickStartGVR, nil + case "ConsoleYAMLSample": + return consoleYAMLSampleGVR, nil + default: + return schema.GroupVersionResource{}, fmt.Errorf("unsupported console resource kind %q", kind) + } +}🤖 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 `@pkg/controller/certmanager/console_resources.go` around lines 149 - 156, The gvrForKind helper currently falls back to consoleYAMLSampleGVR for every unknown kind, which can silently apply the wrong resource. Update gvrForKind in console_resources.go to explicitly recognize only the supported kinds (such as ConsoleQuickStart and ConsoleYAMLSample) and return an error or fail fast for anything else, then propagate that failure through the caller that decodes the asset so unexpected or misspelled kind values are rejected instead of routed incorrectly.
🤖 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.
Nitpick comments:
In `@bindata/console/cert-manager-example-quickstart.yaml`:
- Around line 5-8: Remove the stale include.release.openshift.io/* annotations
from the cert-manager example quickstart manifest so it stays consistent with
the cleaned-up sibling ConsoleYAMLSample manifests. Update the
cert-manager-example-quickstart YAML by deleting the annotations block under the
sample metadata, keeping the resource aligned with the annotation cleanup
already applied elsewhere.
In `@bundle/manifests/cert-manager-operator.clusterserviceversion.yaml`:
- Around line 413-431: Trim the RBAC in the CSV manifest to match
consoleResourcesController’s actual usage: in
pkg/controller/certmanager/console_resources.go it only needs
ClusterVersions().Get for a single object and Get/Create/Update on
consoleyamlsamples and consolequickstarts. Remove the extra list/watch verbs for
clusterversions and the delete/patch/watch verbs on the console resources so the
permissions follow least privilege.
In `@pkg/controller/certmanager/console_resources.go`:
- Around line 80-147: Add unit tests for consoleResourcesController covering the
new sync, hasConsoleCapability, and applyConsoleResource paths using fake
config/dynamic clients. Verify sync skips when Console capability is disabled,
proceeds when enabled, and returns wrapped errors from capability lookup or
apply failures. Also test applyConsoleResource for create-on-not-found and
update-on-existing behavior, referencing consoleResourcesController,
hasConsoleCapability, and applyConsoleResource so the tests stay resilient if
lines move.
- Around line 149-156: The gvrForKind helper currently falls back to
consoleYAMLSampleGVR for every unknown kind, which can silently apply the wrong
resource. Update gvrForKind in console_resources.go to explicitly recognize only
the supported kinds (such as ConsoleQuickStart and ConsoleYAMLSample) and return
an error or fail fast for anything else, then propagate that failure through the
caller that decodes the asset so unexpected or misspelled kind values are
rejected instead of routed incorrectly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8f662201-1b0b-4379-96b5-0476cecc4279
📒 Files selected for processing (14)
bindata/console/cert-manager-acme-issuer-sample.yamlbindata/console/cert-manager-certificate-sample.yamlbindata/console/cert-manager-example-quickstart.yamlbindata/console/cert-manager-issuer-sample.yamlbundle/manifests/cert-manager-example_console.openshift.io_v1_consolequickstart.yamlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/console/cert-manager-acme-issuer-sample.yamlconfig/console/cert-manager-certificate-sample.yamlconfig/console/cert-manager-example-quickstart.yamlconfig/console/cert-manager-issuer-sample.yamlpkg/controller/certmanager/cert_manager_controller_set.gopkg/controller/certmanager/console_resources.gopkg/operator/assets/bindata.gopkg/operator/starter.go
💤 Files with no reviewable changes (5)
- config/console/cert-manager-example-quickstart.yaml
- config/console/cert-manager-acme-issuer-sample.yaml
- config/console/cert-manager-certificate-sample.yaml
- bundle/manifests/cert-manager-example_console.openshift.io_v1_consolequickstart.yaml
- config/console/cert-manager-issuer-sample.yaml
sebrandon1
force-pushed
the
fix-consoleless-runtime
branch
from
ff46829 to
ef15415
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/controller/certmanager/console_resources.go (2)
80-91: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePanic on embedded asset load/parse failure.
NewConsoleResourcesControllerpanics if any embedded console asset fails to load or has an unrecognized kind. Since these are compiled-in assets this is unlikely to fail at runtime, but a panic here crashes the whole operator process at startup rather than degrading just this controller.🤖 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 `@pkg/controller/certmanager/console_resources.go` around lines 80 - 91, NewConsoleResourcesController currently panics inside the consoleAssetFiles load loop when assets.Asset, resourceread.ReadUnstructuredOrDie, or gvrForKind fails, which can crash the operator at startup. Replace those panic paths with error propagation from NewConsoleResourcesController and have the caller handle the failure so only the console resources setup is affected; use the existing consoleAssetFiles iteration and consoleAsset initialization points to wire the error handling through cleanly.
50-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo cleanup path if Console capability is later disabled/unset.
RBAC includes
deleteforconsoleyamlsamples/consolequickstarts, butsync()only ever applies resources when enabled and does nothing (not even cleanup) when disabled. If this is intentional (capabilities are generally not revocable post-enable), consider dropping the unuseddeleteverb or adding a comment clarifying the decision.Also applies to: 110-118
🤖 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 `@pkg/controller/certmanager/console_resources.go` around lines 50 - 51, The RBAC for console resources grants delete access, but the controller path in sync() only creates/updates when the Console capability is enabled and never performs cleanup when it is disabled or unset. Either remove the unused delete verb from the kubebuilder RBAC markers on console_resources.go (including the related markers around the console YAML samples/quickstarts permissions) or add a clear comment in sync() explaining that capability disablement is intentionally not reconciled and no cleanup is expected.
🤖 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 `@pkg/controller/certmanager/console_resources.go`:
- Around line 93-97: The controller setup in consoleResourcesController only
watches operatorClient.Informer(), so changes in ClusterVersion and transient
GET failures in hasConsoleCapability won’t retrigger reconciliation. Update the
controller wiring to add either a periodic resync or a ClusterVersion
informer/watch alongside the existing operatorClient.Informer() in the
factory.New() chain. Keep the existing sync logic intact, and make sure
reconcile can be triggered when ClusterVersion changes even if operator config
does not.
---
Nitpick comments:
In `@pkg/controller/certmanager/console_resources.go`:
- Around line 80-91: NewConsoleResourcesController currently panics inside the
consoleAssetFiles load loop when assets.Asset,
resourceread.ReadUnstructuredOrDie, or gvrForKind fails, which can crash the
operator at startup. Replace those panic paths with error propagation from
NewConsoleResourcesController and have the caller handle the failure so only the
console resources setup is affected; use the existing consoleAssetFiles
iteration and consoleAsset initialization points to wire the error handling
through cleanly.
- Around line 50-51: The RBAC for console resources grants delete access, but
the controller path in sync() only creates/updates when the Console capability
is enabled and never performs cleanup when it is disabled or unset. Either
remove the unused delete verb from the kubebuilder RBAC markers on
console_resources.go (including the related markers around the console YAML
samples/quickstarts permissions) or add a clear comment in sync() explaining
that capability disablement is intentionally not reconciled and no cleanup is
expected.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6e8aa412-343b-4c09-a1a4-81abd03722f7
📒 Files selected for processing (16)
bindata/console/cert-manager-acme-issuer-sample.yamlbindata/console/cert-manager-certificate-sample.yamlbindata/console/cert-manager-example-quickstart.yamlbindata/console/cert-manager-issuer-sample.yamlbundle/manifests/cert-manager-example_console.openshift.io_v1_consolequickstart.yamlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/console/cert-manager-acme-issuer-sample.yamlconfig/console/cert-manager-certificate-sample.yamlconfig/console/cert-manager-example-quickstart.yamlconfig/console/cert-manager-issuer-sample.yamlconfig/manifests/kustomization.yamlconfig/rbac/role.yamlpkg/controller/certmanager/cert_manager_controller_set.gopkg/controller/certmanager/console_resources.gopkg/operator/assets/bindata.gopkg/operator/starter.go
💤 Files with no reviewable changes (6)
- bundle/manifests/cert-manager-example_console.openshift.io_v1_consolequickstart.yaml
- config/console/cert-manager-issuer-sample.yaml
- config/manifests/kustomization.yaml
- config/console/cert-manager-acme-issuer-sample.yaml
- config/console/cert-manager-certificate-sample.yaml
- config/console/cert-manager-example-quickstart.yaml
✅ Files skipped from review due to trivial changes (1)
- pkg/operator/assets/bindata.go
🚧 Files skipped from review as they are similar to previous changes (7)
- bindata/console/cert-manager-issuer-sample.yaml
- bundle/manifests/cert-manager-operator.clusterserviceversion.yaml
- bindata/console/cert-manager-certificate-sample.yaml
- bindata/console/cert-manager-example-quickstart.yaml
- config/rbac/role.yaml
- pkg/operator/starter.go
- pkg/controller/certmanager/cert_manager_controller_set.go
sebrandon1
force-pushed
the
fix-consoleless-runtime
branch
from
ef15415 to
cfbca6e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/certmanager/console_resources_test.go (1)
118-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate asset-loading/parsing logic.
The load-parse-resolve-GVR loop is repeated verbatim in
TestSyncConsoleEnabled(Lines 118-129) andTestConsoleAssetsPreParsed(Lines 184-199). Extracting a small helper (e.g.,loadConsoleAssets(t) []consoleAsset) would reduce duplication and keep both tests in sync if the asset format changes.Also applies to: 183-201
🤖 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 `@pkg/controller/certmanager/console_resources_test.go` around lines 118 - 129, The asset-loading/parsing loop in TestSyncConsoleEnabled and TestConsoleAssetsPreParsed is duplicated, so extract the shared load-parse-resolve-GVR logic into a small helper like loadConsoleAssets(t) []consoleAsset and have both tests call it. Keep the helper centered around assets.Asset, resourceread.ReadUnstructuredOrDie, and gvrForKind so any future asset-format change only needs one update.
🤖 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.
Nitpick comments:
In `@pkg/controller/certmanager/console_resources_test.go`:
- Around line 118-129: The asset-loading/parsing loop in TestSyncConsoleEnabled
and TestConsoleAssetsPreParsed is duplicated, so extract the shared
load-parse-resolve-GVR logic into a small helper like loadConsoleAssets(t)
[]consoleAsset and have both tests call it. Keep the helper centered around
assets.Asset, resourceread.ReadUnstructuredOrDie, and gvrForKind so any future
asset-format change only needs one update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5743956a-24cb-423e-a650-4a6545f60885
📒 Files selected for processing (17)
bindata/console/cert-manager-acme-issuer-sample.yamlbindata/console/cert-manager-certificate-sample.yamlbindata/console/cert-manager-example-quickstart.yamlbindata/console/cert-manager-issuer-sample.yamlbundle/manifests/cert-manager-example_console.openshift.io_v1_consolequickstart.yamlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/console/cert-manager-acme-issuer-sample.yamlconfig/console/cert-manager-certificate-sample.yamlconfig/console/cert-manager-example-quickstart.yamlconfig/console/cert-manager-issuer-sample.yamlconfig/manifests/kustomization.yamlconfig/rbac/role.yamlpkg/controller/certmanager/cert_manager_controller_set.gopkg/controller/certmanager/console_resources.gopkg/controller/certmanager/console_resources_test.gopkg/operator/assets/bindata.gopkg/operator/starter.go
💤 Files with no reviewable changes (6)
- config/console/cert-manager-acme-issuer-sample.yaml
- bundle/manifests/cert-manager-example_console.openshift.io_v1_consolequickstart.yaml
- config/console/cert-manager-certificate-sample.yaml
- config/console/cert-manager-example-quickstart.yaml
- config/console/cert-manager-issuer-sample.yaml
- config/manifests/kustomization.yaml
✅ Files skipped from review due to trivial changes (2)
- bindata/console/cert-manager-example-quickstart.yaml
- pkg/operator/assets/bindata.go
🚧 Files skipped from review as they are similar to previous changes (8)
- config/rbac/role.yaml
- bundle/manifests/cert-manager-operator.clusterserviceversion.yaml
- pkg/operator/starter.go
- pkg/controller/certmanager/cert_manager_controller_set.go
- bindata/console/cert-manager-certificate-sample.yaml
- bindata/console/cert-manager-issuer-sample.yaml
- bindata/console/cert-manager-acme-issuer-sample.yaml
- pkg/controller/certmanager/console_resources.go
sebrandon1
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
sebrandon1
force-pushed
the
fix-consoleless-runtime
branch
from
cfbca6e to
df7616b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/controller/certmanager/console_resources.go (1)
93-97: 🩺 Stability & Availability | 🟠 MajorController has no trigger for
ClusterVersioncapability changes.
WithInformers(operatorClient.Informer())is the only sync trigger, buthasConsoleCapabilitydoes a liveClusterVersionGET. Without a resync period or aClusterVersioninformer/watch, capability changes (e.g., Console capability enabled post-install) or transient GET failures won't retrigger reconciliation until an unrelated operator-config change occurs.🤖 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 `@pkg/controller/certmanager/console_resources.go` around lines 93 - 97, Update the controller construction in the console resources setup, near hasConsoleCapability and the existing operatorClient informer, to add a resync mechanism or ClusterVersion informer/watch as an additional sync trigger. Ensure changes to ClusterVersion capabilities and transient capability-check GET failures cause reconciliation without requiring an unrelated operator configuration change.
🤖 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 `@pkg/controller/certmanager/console_resources.go`:
- Around line 99-121: Update the asset-application loop in
consoleResourcesController.sync to attempt every entry in c.consoleAssets even
when an individual ApplyUnstructuredResourceImproved call fails. Collect each
asset-specific error, then return an aggregated error after the loop while
preserving the existing capability check and successful-resource behavior.
---
Duplicate comments:
In `@pkg/controller/certmanager/console_resources.go`:
- Around line 93-97: Update the controller construction in the console resources
setup, near hasConsoleCapability and the existing operatorClient informer, to
add a resync mechanism or ClusterVersion informer/watch as an additional sync
trigger. Ensure changes to ClusterVersion capabilities and transient
capability-check GET failures cause reconciliation without requiring an
unrelated operator configuration change.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 199e0302-8b1d-438f-82ff-569e589b6b80
📒 Files selected for processing (17)
bindata/console/cert-manager-acme-issuer-sample.yamlbindata/console/cert-manager-certificate-sample.yamlbindata/console/cert-manager-example-quickstart.yamlbindata/console/cert-manager-issuer-sample.yamlbundle/manifests/cert-manager-example_console.openshift.io_v1_consolequickstart.yamlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/console/cert-manager-acme-issuer-sample.yamlconfig/console/cert-manager-certificate-sample.yamlconfig/console/cert-manager-example-quickstart.yamlconfig/console/cert-manager-issuer-sample.yamlconfig/manifests/kustomization.yamlconfig/rbac/role.yamlpkg/controller/certmanager/cert_manager_controller_set.gopkg/controller/certmanager/console_resources.gopkg/controller/certmanager/console_resources_test.gopkg/operator/assets/bindata.gopkg/operator/starter.go
💤 Files with no reviewable changes (6)
- bundle/manifests/cert-manager-example_console.openshift.io_v1_consolequickstart.yaml
- config/console/cert-manager-issuer-sample.yaml
- config/manifests/kustomization.yaml
- config/console/cert-manager-acme-issuer-sample.yaml
- config/console/cert-manager-example-quickstart.yaml
- config/console/cert-manager-certificate-sample.yaml
🚧 Files skipped from review as they are similar to previous changes (10)
- config/rbac/role.yaml
- bindata/console/cert-manager-certificate-sample.yaml
- bundle/manifests/cert-manager-operator.clusterserviceversion.yaml
- bindata/console/cert-manager-acme-issuer-sample.yaml
- bindata/console/cert-manager-issuer-sample.yaml
- pkg/controller/certmanager/cert_manager_controller_set.go
- pkg/operator/starter.go
- bindata/console/cert-manager-example-quickstart.yaml
- pkg/controller/certmanager/console_resources_test.go
- pkg/operator/assets/bindata.go
sebrandon1
force-pushed
the
fix-consoleless-runtime
branch
2 times, most recently
from
40b5262 to
54a5449
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@sebrandon1: This pull request references Jira Issue OCPBUGS-85579, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (amendiol@redhat.com), skipping review request. 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. |
…ability detection Remove ConsoleYAMLSample and ConsoleQuickStart manifests from the OLM bundle and create them at runtime only when the Console capability is enabled on the cluster. On consoleless RAN/RDS clusters, the operator now skips Console resource creation instead of failing during InstallPlan execution. This follows the pattern established by cluster-monitoring-operator (PR #2011, OCPBUGS-14922) for handling optional Console resources in OLM-managed operators. Changes: - Remove 4 Console manifests from bundle/manifests/ - Remove config/console from kustomize manifests generation - Add console_resources.go controller that checks ClusterVersion capabilities before creating Console resources via dynamic client - Use library-go ApplyUnstructuredResourceImproved for idempotent create-or-update with change detection - Add Console YAML files to bindata/ for runtime embedding - Add RBAC for console.openshift.io and clusterversions via kubebuilder markers - Remove ineffective capability.openshift.io/name annotations
sebrandon1
force-pushed
the
fix-consoleless-runtime
branch
from
54a5449 to
12597e0
Compare
|
@sebrandon1: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Problem
cert-manager-operator fails on consoleless clusters (RAN/RDS/SNO) when OLM tries to install Console resources (ConsoleYAMLSample, ConsoleQuickStart):
Why PR #424's approach didn't work:
The
capability.openshift.io/name: Consoleannotation is only processed by CVO for core platform components — not by OLM for third-party operator bundles.Solution
Move Console resource creation from the OLM bundle to operator runtime, following the pattern from cluster-monitoring-operator #2011.
How it works:
ClusterVersion.status.capabilities.enabledCapabilitiesfor Console capabilityChanges
bundle/manifests/(OLM no longer creates them)pkg/controller/certmanager/console_resources.go— runtime Console capability detectionbindata/console/for runtime embeddingBehavior
Testing
Verified on OCP 4.22:
References
Other operators that solved this same problem
Runtime capability/API detection (the pattern this PR follows):
CVO annotation approach (works for CVO-managed components only, NOT for OLM operators):
Annotation removed / Console resources deleted entirely:
Test framework fixes:
Adding
capability.openshift.io/name: Consoleto existing manifests during an upgrade causes CVO to force-enable the Console capability on clusters that had it disabled (OCPBUGS-20331). Both CMO (#2118 revert, #2254 re-fix) and kube-apiserver-operator (#1565) had to rename their manifests to work around this. The runtime detection approach used in this PR avoids this pitfall entirely.Summary by CodeRabbit
New Features
IssuerandCertificate, verifying the generated TLS Secret, and using certificates with Routes and Ingresses.Improvements