OCPBUGS-58422: Handle disabled Ingress capability in HyperShift#1182
OCPBUGS-58422: Handle disabled Ingress capability in HyperShift#1182stefanonardo wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@stefanonardo: This pull request references Jira Issue OCPBUGS-58422, 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stefanonardo 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughThis PR updates ingress capability annotations, adds ingress-disabled early returns in controller sync paths, and changes operator startup wiring to skip route-related controllers when ingress is disabled. ChangesIngress-disabled controller gating
Estimated code review effort: 4 (Complex) | ~50 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/cherry-pick release-4.22 |
|
@stefanonardo: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
openshift-ci-robot
added
jira/valid-bug
|
@stefanonardo: This pull request references Jira Issue OCPBUGS-58422, 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 (yapei@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. |
Remove the validation that requires Console to be disabled when Ingress is disabled. The console-operator now handles ingress-disabled gracefully (openshift/console-operator#1182). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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 (2)
pkg/console/controllers/clidownloads/controller.go (1)
128-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent use of the resource-name constant.
Line 128 uses
api.ConfigResourceNamefor the Infrastructure Get, but Line 132 hardcodes"version"for the ClusterVersion Get right next to it. The same literal is repeated inoauthclients.go,sync_v400.go, andstarter.go. A shared named constant (e.g. alongsideapi.ConfigResourceName) would improve consistency and avoid a typo causing a silent lookup failure.🤖 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/console/controllers/clidownloads/controller.go` around lines 128 - 135, The ClusterVersion lookup in the controller is hardcoding "version" instead of using a shared resource-name constant, which makes the code inconsistent and typo-prone. Update `clidownloads/controller.go` so `c.clusterVersionLister.Get(...)` uses a named constant like the existing `api.ConfigResourceName`, and extract that resource name into a shared constant that can also be reused by `oauthclients.go`, `sync_v400.go`, and `starter.go` to keep all lookups consistent.pkg/console/controllers/oauthclients/oauthclients.go (1)
146-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "fetch infra+clusterVersion, check ingress-disabled" boilerplate across controllers.
This exact pattern (Get
InfrastructureConfig, GetClusterVersionby the hardcoded"version"name, callutil.IsExternalControlPlaneWithIngressDisabled, early-return) is repeated near-verbatim inpkg/console/operator/sync_v400.go(lines 64-70) andpkg/console/controllers/route/controller.go(lines 174-189). Consider extracting a small shared helper inpkg/console/controllers/util(e.g.IsIngressDisabled(infraLister, cvLister) (bool, error)) to avoid drift between the three call sites, and to centralize the magic"version"resource name (also hardcoded inpkg/console/starter/starter.goline 253) behind a named constant.🤖 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/console/controllers/oauthclients/oauthclients.go` around lines 146 - 159, The ingress-disabled check is duplicated across controllers, including the OAuth client sync path and other call sites, which risks drift and repeated hardcoded resource lookup details. Extract the shared “fetch InfrastructureConfig and ClusterVersion, then call util.IsExternalControlPlaneWithIngressDisabled” flow into a helper under pkg/console/controllers/util, and have oauthclients.go and the other controllers call it instead of inlining the lookup/early-return logic. While doing that, centralize the hardcoded "version" ClusterVersion name behind a named constant so all callers use the same symbol.
🤖 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/console/controllers/clidownloads/controller.go`:
- Around line 128-139: In clidownloads controller logic, the early return in the
`Get`/`IsExternalControlPlaneWithIngressDisabled` branch can leave
`OCDownloadsSyncDegraded` latched from a prior run. Update the condition state
in this path before returning by clearing the `OCDownloadsSyncDegraded`
zero-value condition in the same controller flow that handles
`infrastructureConfig`, `clusterVersionConfig`, and
`statusHandler.FlushAndReturn`, so the ClusterOperator does not remain Degraded
when ingress is disabled.
In `@pkg/console/controllers/oauthclients/oauthclients.go`:
- Around line 60-61: The OAuthClients controller is using Infrastructure and
ClusterVersion listers in sync() without registering their informers with the
factory. Update the controller setup to pass infrastructureConfigInformer and
clusterVersionInformer into WithInformers(...) alongside the existing informers
so cache synchronization waits on both resources and the controller resyncs when
either changes.
In `@pkg/console/starter/starter.go`:
- Around line 249-261: The ingressDisabled check in starter.go is acting as a
one-time startup gate, so if Infrastructure or ClusterVersions are not ready yet
the route and health-check controllers may never start for this pod. Update the
startup flow around util.IsExternalControlPlaneWithIngressDisabled and the
controller setup in the starter logic so this condition is re-evaluated when the
relevant config becomes available, or make it explicit that a restart is
required for capability changes to take effect. Preserve the klog.Info path for
the disabled case, but do not let a transient startup state permanently block
controller startup.
---
Nitpick comments:
In `@pkg/console/controllers/clidownloads/controller.go`:
- Around line 128-135: The ClusterVersion lookup in the controller is hardcoding
"version" instead of using a shared resource-name constant, which makes the code
inconsistent and typo-prone. Update `clidownloads/controller.go` so
`c.clusterVersionLister.Get(...)` uses a named constant like the existing
`api.ConfigResourceName`, and extract that resource name into a shared constant
that can also be reused by `oauthclients.go`, `sync_v400.go`, and `starter.go`
to keep all lookups consistent.
In `@pkg/console/controllers/oauthclients/oauthclients.go`:
- Around line 146-159: The ingress-disabled check is duplicated across
controllers, including the OAuth client sync path and other call sites, which
risks drift and repeated hardcoded resource lookup details. Extract the shared
“fetch InfrastructureConfig and ClusterVersion, then call
util.IsExternalControlPlaneWithIngressDisabled” flow into a helper under
pkg/console/controllers/util, and have oauthclients.go and the other controllers
call it instead of inlining the lookup/early-return logic. While doing that,
centralize the hardcoded "version" ClusterVersion name behind a named constant
so all callers use the same symbol.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bf8e3405-e144-4ab0-877e-4e5a944d5f36
📒 Files selected for processing (7)
manifests/03-rbac-role-ns-openshift-ingress-operator.yamlmanifests/04-rbac-rolebinding.yamlpkg/console/controllers/clidownloads/controller.gopkg/console/controllers/oauthclients/oauthclients.gopkg/console/controllers/route/controller.gopkg/console/operator/sync_v400.gopkg/console/starter/starter.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/console(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (15)
**/*.{yaml,yml,json}
📄 CodeRabbit inference engine (Custom checks)
**/*.{yaml,yml,json}: Flag privileged: true, hostPID, hostNetwork, hostIPC, SYS_ADMIN capability, running as root without justification, and allowPrivilegeEscalation: true in container/Kubernetes manifests
When deployment manifests, operator code, or controllers are added/modified, ensure they do not introduce scheduling constraints assuming standard HA topology (3+ control-plane nodes, dedicated workers). Flag: required pod anti-affinity with maxUnavailable: 0 (deadlocks on SNO/TNF/TNA), pod topology spread with DoNotSchedule and hostname key (breaks on SNO), replica counts derived from node count without topology awareness, nodeSelector/node affinity targeting control-plane nodes (fails on HyperShift), scheduling to all control-plane nodes equally without excluding arbiter nodes (TNA), assuming dedicated worker nodes exist (SNO/TNF), or PodDisruptionBudgets designed for 3+ nodes (TNF/TNA). Do not flag if change checks ControlPlaneTopology, node counts, or topology labels before applying constraints.
Files:
manifests/03-rbac-role-ns-openshift-ingress-operator.yamlmanifests/04-rbac-rolebinding.yaml
{manifests,bindata/assets,quickstarts,examples,profile-patches}/**/*.{yaml,yml}
📄 CodeRabbit inference engine (.claude/skills/manifest-review.md)
{manifests,bindata/assets,quickstarts,examples,profile-patches}/**/*.{yaml,yml}: Kubernetes manifests undermanifests/,bindata/assets/,quickstarts/,examples/, andprofile-patches/must include the appropriate cluster profile annotations when they are CVO-deployed resources (for exampleinclude.release.openshift.io/hypershift,include.release.openshift.io/ibm-cloud-managed,include.release.openshift.io/self-managed-high-availability, andinclude.release.openshift.io/single-node-developer).
Console resources in the manifest trees must include the capability annotationcapability.openshift.io/name: Console.
RBAC manifests must follow least-privilege design: grant only the permissions needed, avoid wildcards unless truly justified, use verbs that match the actual operation set, and set correctapiGroups(""for core APIs and specific groups for CRDs).
Resources must use the correct namespace for their role:openshift-consolefor console workload resources andopenshift-console-operatorfor operator resources; any cross-namespace reference must be explicit and intentional.
YAML manifests should use 2-space indentation, keep fields in a consistent order, and include---separators between multiple resources in the same file.
When binding roles to service accounts, useconsole-operatorin theopenshift-console-operatornamespace for operator cross-namespace access, and useconsolein theopenshift-consolenamespace for console workload-scoped access.
Files:
manifests/03-rbac-role-ns-openshift-ingress-operator.yamlmanifests/04-rbac-rolebinding.yaml
**
⚙️ CodeRabbit configuration file
**: # OpenShift Console Operator - AI Context HubThis file serves as the central AI documentation hub for the OpenShift Console Operator project. AI assistants (Claude, Cursor, Copilot, CodeRabbit, etc.) use this and the linked documents to understand project context.
Go Version and Dependencies
Go Version and Dependencies
- Go version: 1.24.0 (toolchain: go1.24.4)
- Dependency management: Uses
go.modwith vendoring- Build flags: Use
GOFLAGS="-mod=vendor"for builds and tests to ensure vendored dependencies are used- Key dependencies: openshift/api, openshift/library-go, k8s.io client libraries
- Go version: 1.24.0 (toolchain: go1.24.4)
- Dependency management: Uses
go.modwith vendoring- Build flags: Use
GOFLAGS="-mod=vendor"for builds and tests to ensure vendored dependencies are used- Key dependencies: openshift/api, openshift/library-go, k8s.io client libraries
Quick Reference
This Repository
Document Purpose ARCHITECTURE.md System architecture, components, repository structure CONVENTIONS.md Go coding standards, patterns, import organization TESTING.md Testing patterns, commands, debugging README.md Project README with setup instructions Console Repository (openshift/console)
For frontend-related guidelines, see the openshift/console repository:
Document Purpose STYLEGUIDE.md Frontend code style guidelines INTERNATIONALIZATION.md i18n patterns and translation guidelines CONTRIBUTING.md Contribution guidelines for the console project Project Summary
The **console-operator...
Files:
manifests/03-rbac-role-ns-openshift-ingress-operator.yamlmanifests/04-rbac-rolebinding.yamlpkg/console/operator/sync_v400.gopkg/console/controllers/route/controller.gopkg/console/controllers/oauthclients/oauthclients.gopkg/console/controllers/clidownloads/controller.gopkg/console/starter/starter.go
**/*.yaml
⚙️ CodeRabbit configuration file
**/*.yaml: Review YAML manifests based on content and kind.Refer to /manifest-review when YAML contains:
kind: Roleorkind: ClusterRole(RBAC review)kind: RoleBindingorkind: ClusterRoleBindingannotations:section (check for cluster profiles)verbs: ["*"]or wildcard permissionsapiGroups: ["*"]or overly broad permissions- ServiceAccount references in subjects
Check for required annotations in manifests/:
include.release.openshift.io/hypershiftinclude.release.openshift.io/ibm-cloud-managedinclude.release.openshift.io/self-managed-high-availabilityinclude.release.openshift.io/single-node-developercapability.openshift.io/name: ConsoleFor quickstarts/, additionally check:
- QuickStart spec structure
- Task descriptions and prerequisites
- See quickstarts/README.md for guidelines
Files:
manifests/03-rbac-role-ns-openshift-ingress-operator.yamlmanifests/04-rbac-rolebinding.yaml
**/*.{yaml,yml}
⚙️ CodeRabbit configuration file
**/*.{yaml,yml}: If this is a Kubernetes/OpenShift manifest or Helm template:
- securityContext: runAsNonRoot, readOnlyRootFilesystem,
allowPrivilegeEscalation: false- Drop ALL capabilities, add only what is required
- Resource limits (cpu, memory) on every container
- No hostPID, hostNetwork, hostIPC, privileged: true
- NetworkPolicy defined for the namespace
- OpenShift: SCC must be restricted or custom-scoped
- Liveness + readiness probes defined
- automountServiceAccountToken: false unless needed
- RBAC: least privilege; no cluster-admin for workloads
- Helm: no .Values interpolation in shell commands
Files:
manifests/03-rbac-role-ns-openshift-ingress-operator.yamlmanifests/04-rbac-rolebinding.yaml
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Follow Go coding standards and patterns documented in CONVENTIONS.md
Organize imports according to conventions documented in CONVENTIONS.md
Usegofmtto format Go code with standard formatting
Rungo vetchecks on all Go packagesFollow Go coding standards and patterns as documented in CONVENTIONS.md, including proper import organization
Organize Go code following the repository structure: main entry point in
cmd/console/main.go, API constants inpkg/api/, operator command setup inpkg/cmd/operator/, and version command inpkg/cmd/version/
**/*.go: Usegofmtfor formatting Go code
Follow standard Go naming conventions
Group imports in order: standard lib, 3rd party, kube/openshift, internal (marked with comments)
Use meaningful error messages with context in Go code
Set status conditions usingstatus.Handle*functions with type prefixes (*Degraded, *Progressing, *Available, *Upgradeable)
Use typed errors and wrap errors to preserve stack contextFlag MD5, SHA1, DES, RC4, 3DES, Blowfish, and ECB mode cryptographic usage. Also flag custom crypto implementations and non-constant-time comparison of secrets or tokens.
**/*.go: Do not use deprecated Go APIs such asioutil.ReadFile,ioutil.WriteFile,ioutil.ReadAll, ornet.DialinDialcallbacks; useos.ReadFile,os.WriteFile,io.ReadAll, andDialContextinstead.
When returning errors in Go, wrap them with%wand include meaningful context instead of returning the raw error or using%v.
Use specific error checks such asapierrors.IsNotFound(err)instead of matching error strings withstrings.Contains(err.Error(), ...).
Propagate the caller’scontext.Contextthrough operations and avoid replacing it withcontext.Background()inside request/controller code.
Usedeferto release acquired resources so cleanup happens on all return paths.
Avoid god functions: keep Go functions to roughly under 100 lines and split code with too many responsibilities into smaller...
Files:
pkg/console/operator/sync_v400.gopkg/console/controllers/route/controller.gopkg/console/controllers/oauthclients/oauthclients.gopkg/console/controllers/clidownloads/controller.gopkg/console/starter/starter.go
⚙️ CodeRabbit configuration file
**/*.go: Review Go code following OpenShift operator patterns.
See CONVENTIONS.md for coding standards and patterns.Refer to the following skills based on CODE PATTERNS, not just file paths:
Refer to /controller-review when code contains:
- Controller struct types (e.g.,
type *Controller struct)func New*Controller(factory functionsfactory.New().WithFilteredEventsInformers(pattern.ToController(method callsSync(ctx context.Context, controllerContext factory.SyncContext)methodsoperatorConfig.Spec.ManagementStatechecksstatus.NewStatusHandlerorstatus.Handle*functionsRefer to /sync-handler-review when code contains:
- Main operator sync functions (e.g.,
sync_v400.gocontent)- Sequential resource syncing with early returns
- Incremental reconciliation loops
- Multiple
resourceapply.Apply*()calls in sequence- Dependency ordering of ConfigMaps → Secrets → Service Accounts → RBAC → Services → Deployments → Routes
- Feature gate conditional logic
Refer to /go-quality-review for all Go code to check:
- Deprecated imports:
ioutil.ReadFile,ioutil.WriteFile,ioutil.ReadAll- Deprecated patterns:
DialwithoutDialContext- Error handling: missing
%win fmt.Errorf- Code smells: deep nesting (4+ levels), functions >100 lines
- Magic values: unexplained numbers/strings
- Context propagation:
context.Background()instead of passed ctx- Missing godoc on exported functions
Files:
pkg/console/operator/sync_v400.gopkg/console/controllers/route/controller.gopkg/console/controllers/oauthclients/oauthclients.gopkg/console/controllers/clidownloads/controller.gopkg/console/starter/starter.go
{pkg,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use gofmt for code formatting on pkg and cmd directories
{pkg,cmd}/**/*.go: Format code usinggofmt -w ./pkg ./cmd
Rungo vetchecks on all Go packages in ./pkg and ./cmd
Files:
pkg/console/operator/sync_v400.gopkg/console/controllers/route/controller.gopkg/console/controllers/oauthclients/oauthclients.gopkg/console/controllers/clidownloads/controller.gopkg/console/starter/starter.go
**/*sync*.go
📄 CodeRabbit inference engine (CONVENTIONS.md)
Implement sync loops (
sync_v400) incrementally: start from zero, create/update missing requirements, and return to continue on next loop
Files:
pkg/console/operator/sync_v400.go
**/operator/**/*.go
📄 CodeRabbit inference engine (Custom checks)
When deployment manifests, operator code, or controllers are added/modified, ensure they do not introduce scheduling constraints assuming standard HA topology. Check ControlPlaneTopology for SingleReplica/DualReplica/HighlyAvailableArbiter/External modes before applying constraints. Use required anti-affinity with maxUnavailable >= 1 (not maxUnavailable: 0). Cap replica counts to schedulable nodes. Exclude arbiter nodes on TNA. Avoid master nodeSelectors on HyperShift. Use library-go DeploymentController hooks (WithTopologyAwareReplicasHook, WithTopologyAwareSchedulingHook, WithControlPlaneNodeSelectorHook).
Files:
pkg/console/operator/sync_v400.go
**/sync_v400.go
📄 CodeRabbit inference engine (.claude/skills/sync-handler-review.md)
Incremental sync pattern: each sync loop should stop on the first error and resume from the next step on the next reconciliation instead of collecting and joining all errors.
Files:
pkg/console/operator/sync_v400.go
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}
⚙️ CodeRabbit configuration file
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}: Injection prevention (prodsec-skills):
- SQL: parameterized queries only; no string concatenation
- Command: no shell=True, os.system, or backtick exec with user input
- LDAP/XPath: escape special characters in filters
- Path traversal: canonicalize paths, reject ../
- Deserialization: no pickle/yaml.load()/eval on untrusted data
- Prototype pollution: no recursive merge of untrusted objects
- Validate at trust boundaries with allow-lists, not deny-lists
- Normalize Unicode and anchor regexes (^$); watch for ReDoS
Files:
pkg/console/operator/sync_v400.gopkg/console/controllers/route/controller.gopkg/console/controllers/oauthclients/oauthclients.gopkg/console/controllers/clidownloads/controller.gopkg/console/starter/starter.go
pkg/console/controllers/**/*.go
📄 CodeRabbit inference engine (ARCHITECTURE.md)
Place all controller implementations in
pkg/console/controllers/subdirectory, with each controller in its own package (e.g.,clidownloads/,oauthclients/,route/,service/)
Files:
pkg/console/controllers/route/controller.gopkg/console/controllers/oauthclients/oauthclients.gopkg/console/controllers/clidownloads/controller.go
**/*controller*.go
📄 CodeRabbit inference engine (CONVENTIONS.md)
Use the OpenShift library-go factory pattern for controllers
**/*controller*.go: Use the controller factory pattern withfactory.New().WithFilteredEventsInformers(),util.IncludeNamesFilter()for informer filtering, and a descriptiveToController()call with a recorder.
In controller sync logic, handle alloperatorConfig.Spec.ManagementStatevalues:Managed,Unmanaged,Removed, and return an error for unknown states.
Create and usestatus.NewStatusHandler(c.operatorClient), set the appropriate status condition helpers (HandleProgressingOrDegraded(),HandleDegraded(),HandleProgressing(),HandleAvailable()), and always callFlushAndReturn()at the end of sync.
Group imports with commented sections for standard library, third-party, kube, openshift, and operator/internal packages.
Wrap errors with context usingfmt.Errorf("failed to X: %w", err), handleapierrors.IsNotFound()appropriately for delete operations, optional resources, and get-before-create flows, and return meaningful error messages.
Useresourceapply.Apply*()functions from library-go, pass the controller recorder for events, and handle returned errors properly.
Set owner references withutil.OwnerRefFrom(cr)frompkg/console/subresource/util, ensure only one owner reference hascontroller=true, and clean up owner references when replacing existing resources.
Files:
pkg/console/controllers/route/controller.gopkg/console/controllers/clidownloads/controller.go
**/*controller.go
📄 CodeRabbit inference engine (.claude/skills/sync-handler-review.md)
**/*controller.go: ControllerSyncmethods must reconcile dependent resources in dependency order: ConfigMaps and Secrets first, then Service Accounts, RBAC (Roles and RoleBindings), Services, Deployments, and finally Routes.
Status updates must track reconciliation progress accurately by adding progressing/degraded conditions on failures and marking the resource available only after reconciliation completes successfully.
Return immediately on reconciliation errors instead of logging and continuing, to preserve incremental reconciliation behavior.
Useresourceapply.Apply*()helpers for resource creation and updates instead of writing separate create/update logic.
When a resource is removed from the desired config, the controller must delete the corresponding cluster object and treatNotFoundas a successful no-op.
Check feature gates before syncing gated resources, and only reconcile the gated resources when the feature is enabled.
RespectManagementState, including missing cleanup logic forRemovedstate.
Do not mutate live Kubernetes objects directly; build desired state and apply it through reconciliation helpers.
Always update status conditions during reconciliation; missing status condition updates is an anti-pattern.
Files:
pkg/console/controllers/route/controller.gopkg/console/controllers/clidownloads/controller.go
pkg/console/starter/**/*.go
📄 CodeRabbit inference engine (ARCHITECTURE.md)
Access feature gates via
featuregates.FeatureGateAccessinstarter.gofor features likeExternalOIDCandConsolePluginContentSecurityPolicy
Files:
pkg/console/starter/starter.go
🔇 Additional comments (7)
manifests/03-rbac-role-ns-openshift-ingress-operator.yaml (1)
11-11: LGTM!manifests/04-rbac-rolebinding.yaml (1)
90-90: LGTM!pkg/console/controllers/clidownloads/controller.go (2)
50-56: LGTM!Also applies to: 76-82
87-98: 🩺 Stability & AvailabilityConfirm the controller start order against the shared config informer factory — if that factory is already running before this controller starts, the missing informer may be a watch gap rather than a startup hazard.
pkg/console/controllers/route/controller.go (1)
174-195: LGTM!pkg/console/starter/starter.go (1)
305-317: LGTM!Also applies to: 442-504, 697-730
pkg/console/operator/sync_v400.go (1)
63-71: 🎯 Functional CorrectnessClusterVersion informer is already registered with the config informers, so this race concern doesn't apply.
> Likely an incorrect or invalid review comment.
stefanonardo
force-pushed
the
OCPBUGS-58422
branch
from
942cc71 to
5fd368d
Compare
Remove the validation that requires Console to be disabled when Ingress is disabled. The console-operator now handles ingress-disabled gracefully (openshift/console-operator#1182). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/retest |
Skip route and health check controllers when Ingress is disabled. Add ingress-disabled checks to OAuth, CLI downloads, and main sync. Gate ingress-operator RBAC manifests with Console+Ingress capability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
stefanonardo
force-pushed
the
OCPBUGS-58422
branch
from
5fd368d to
302c519
Compare
|
/retest |
|
@stefanonardo: The following tests failed, say
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. |
3 participants
Summary
When Ingress capability is disabled in HyperShift, the console operator crashes repeatedly because it expects IngressController CRDs and Routes that don't exist. This PR makes the operator handle the ingress-disabled case gracefully.
Changes
IsExternalControlPlaneWithIngressDisabled()before fetching IngressController (defensive).openshift-ingress-operatornamespace Role and RoleBinding withConsole+Ingresscapability annotation so CVO skips them when the Ingress capability is disabled (the namespace does not exist in that case).Related
Test plan
make test-unitpasses--disable-cluster-capabilities Ingress(using fix: OCPBUGS-58422: Allow disabling Ingress without Console in HyperShift hypershift#8933 to remove the Ingress/Console coupling validation):route "console" not found,IngressController not found, andFailedRouteGeterrors; ClusterOperator reportsDegraded=TrueIngress capability is disabled in external control plane topology, skipping route and health check controllers, zero route/IngressController errors in the logs, andOAuthClientsControllerDegradedcondition is cleared from the ClusterOperator statusSummary by CodeRabbit