Skip to content

CNTRLPLANE-3423: Revert "TRT-2858: Revert "Merge pull request #1196 from ingvagabund/tls-injection-to-console"" - #1205

Open
wking wants to merge 1 commit into
openshift:mainfrom
wking:revert-1200-revert-1196-tls-injection
Open

CNTRLPLANE-3423: Revert "TRT-2858: Revert "Merge pull request #1196 from ingvagabund/tls-injection-to-console""#1205
wking wants to merge 1 commit into
openshift:mainfrom
wking:revert-1200-revert-1196-tls-injection

Conversation

@wking

@wking wking commented Aug 5, 2026

Copy link
Copy Markdown
Member

Reverts #1200

TRT-2858 reported console Pods in 5.0.0-0.nightly-2026-07-28-081944 CI crash-looping with:

Failed to load config: servingInfo.minTLSVersion is not supported

The nightly no longer exists, with the linked page returning:

unable to find release tag 5.0.0-0.nightly-2026-07-28-081944, it may have been deleted

And while the console pull merged on the 24th and the console-operator pull merged on the 27th, I still suspect the issue might have been the console-operator change arriving in nightlies before the console change arrived in those nightlies. But checking a recent nightly, we definitely have the console change in now:

$ oc adm release info --commits registry.ci.openshift.org/ocp/release-5:5.0.0-0.nightly-2026-07-30-185227 | grep 'console '
  console                                        https://github.com/openshift/console                                        6d0dcf1d76337e46d95ea1b546d5ffe692d7918d
$ oc adm release info --commits registry.ci.openshift.org/ocp/release-5:5.0.0-0.nightly-2026-08-03-043516 | grep 'console '
  console                                        https://github.com/openshift/console                                        d934fc26da755bd6e1494349d32c85fb9a111dc6
$ git log --first-parent --date=short --format='%ad %h %s' 6d0dcf1d76337e46d95ea1b546d5ffe692d7918d..d934fc26da755bd6e1494349d32c85fb9a111dc6 | grep -n .
1:2026-08-01 d934fc26da Merge pull request #16880 from fao89/Review-CONSOLE-5118
...
33:2026-07-24 f9298136ac Merge pull request #16804 from ingvagabund/tls-injection-to-console
...
43:2026-07-21 ee4383e546 Merge pull request #16761 from logonoff/rspack

So... yeah, that's a big jump with a nightly from the 30th lacking changes that had merged on the 21st! But by the 3rd nightlies have the console change in place, so we should be safe just trying again now. CC @ingvagabund

Summary by CodeRabbit

  • New Features

    • Added support for configuring the console’s minimum TLS version and cipher suites from cluster security settings.
    • The console now observes API server security configuration and applies compatible TLS settings automatically.
    • Improved permissions allow the console operator to read required API server configuration.
  • Bug Fixes

    • Invalid TLS configuration is reported clearly, while missing optional settings remain non-blocking.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 5, 2026
@openshift-ci-robot

openshift-ci-robot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@wking: This pull request references CNTRLPLANE-3423 which is a valid jira issue.

Details

In response to this:

Reverts #1200

TRT-2858 reported console Pods in 5.0.0-0.nightly-2026-07-28-081944 CI crash-looping with:

Failed to load config: servingInfo.minTLSVersion is not supported

The nightly no longer exists, with the linked page returning:

unable to find release tag 5.0.0-0.nightly-2026-07-28-081944, it may have been deleted

And while the console pull merged on the 24th and the console-operator pull merged on the 27th, I still suspect the issue might have been the console-operator change arriving in nightlies before the console change arrived in those nightlies. But checking a recent nightly, we definitely have the console change in now:

$ oc adm release info --commits registry.ci.openshift.org/ocp/release-5:5.0.0-0.nightly-2026-07-30-185227 | grep 'console '
 console                                        https://github.com/openshift/console                                        6d0dcf1d76337e46d95ea1b546d5ffe692d7918d
$ oc adm release info --commits registry.ci.openshift.org/ocp/release-5:5.0.0-0.nightly-2026-08-03-043516 | grep 'console '
 console                                        https://github.com/openshift/console                                        d934fc26da755bd6e1494349d32c85fb9a111dc6
$ git log --first-parent --date=short --format='%ad %h %s' 6d0dcf1d76337e46d95ea1b546d5ffe692d7918d..d934fc26da755bd6e1494349d32c85fb9a111dc6 | grep -n .
1:2026-08-01 d934fc26da Merge pull request #16880 from fao89/Review-CONSOLE-5118
...
33:2026-07-24 f9298136ac Merge pull request #16804 from ingvagabund/tls-injection-to-console
...
43:2026-07-21 ee4383e546 Merge pull request #16761 from logonoff/rspack

So... yeah, that's a big jump with a nightly from the 30th lacking changes that had merged on the 21st! But by the 3rd nightlies have the console change in place, so we should be safe just trying again now. CC @ingvagabund

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.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Walkthrough

Changes

TLS configuration propagation

Layer / File(s) Summary
APIServer TLS observation wiring
pkg/console/configobservation/..., pkg/console/starter/starter.go, manifests/03-rbac-role-cluster.yaml
Adds the ConfigObserver, its listers, startup registration, and permission to read APIServer resources.
Observed TLS configuration parsing
pkg/console/operator/sync_v400.go, pkg/console/operator/sync_v400_test.go
Reads TLS settings from observed configuration, reports parsing errors, and adds extraction tests.
Console configuration TLS output
pkg/console/subresource/consoleserver/*, pkg/console/subresource/configmap/*
Passes TLS settings through ConfigMap generation and emits them in serving information. Tests cover configured and empty values.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: redhat-chai-bot, jhadvig

Sequence Diagram(s)

sequenceDiagram
  participant APIServer
  participant ConfigObserver
  participant ObservedConfig
  participant SyncConfigMap
  participant DefaultConfigMap
  participant ConsoleServerCLIConfigBuilder
  APIServer->>ConfigObserver: provide TLS security profile
  ConfigObserver->>ObservedConfig: write TLS settings
  SyncConfigMap->>ObservedConfig: read TLS settings
  SyncConfigMap->>DefaultConfigMap: pass TLS version and cipher suites
  DefaultConfigMap->>ConsoleServerCLIConfigBuilder: apply TLSConfig
  ConsoleServerCLIConfigBuilder-->>DefaultConfigMap: generate serving information
Loading

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name Status Explanation Resolution
No-Weak-Crypto ❌ Error The new observer forwards APIServer TLS profiles to Console; the Old profile includes DES-CBC3-SHA and SHA-1 suites, which the mapper emits as Go 3DES/SHA-1 cipher suites. Filter DES/3DES and SHA-1 cipher suites before writing ServingInfo, or restrict Console to Intermediate/Modern profiles.
Description check ⚠️ Warning The description explains the failure and retry rationale but omits explicit template sections for testing, browser conformance, additional information, and reviewers. Add the required template sections, especially test setup and test cases, and state browser conformance, additional information, and reviewers or explain why they do not apply.
✅ Passed checks (13 passed)
Check name Status Explanation
Title check ✅ Passed The title identifies the Jira issue and clearly describes reapplying the reverted TLS injection change.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The patch adds only static table-test names; both t.Run(tt.name) uses draw from string literals, and no Ginkgo titles or dynamic test-name construction were added.
Test Structure And Quality ✅ Passed The PR adds only standard-library table-driven tests using testing.T; no Ginkgo It blocks, cluster resources, or Eventually/Consistently waits require review under this check.
Microshift Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; changed tests use Go's testing package and Test* functions, with no It, Describe, Context, or When declarations.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The PR adds only standard Go unit tests (Test... with t.Run); it adds no Ginkgo e2e tests or multi-node assumptions requiring SNO protection.
Topology-Aware Scheduling Compatibility ✅ Passed The commit adds RBAC and TLS/config-observer logic only; diff inspection found no affinity, topology spread, replica, PDB, node selector, taint, or toleration constraints.
Ote Binary Stdout Contract ✅ Passed The PR builds console-operator, not an OTE test binary; changed process-level code adds no stdout writes, and vendored klog defaults to stderr.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed Changed tests use standard Go testing.T only; searches found no new Ginkgo constructs, IPv4-only literals, or external connectivity requirements.
Container-Privileges ✅ Passed The patch adds only ClusterRole read access to apiservers. Added-line and changed-file scans found no privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, root, or allowPrivilegeEscalation settings.
No-Sensitive-Data-In-Logs ✅ Passed The active observer logs only TLS versions and cipher-suite names; no passwords, tokens, PII, or customer data are logged, and the added CORS observer is not wired into production.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@openshift-ci
openshift-ci Bot requested review from jhadvig and spadgett August 5, 2026 20:25

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
pkg/console/configobservation/listers.go (1)

16-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add Godoc comments for new exported methods.

  • pkg/console/configobservation/listers.go#L16-L25: Document APIServerLister, ResourceSyncer, and PreRunHasSynced.
  • pkg/console/subresource/consoleserver/config_builder.go#L328-L332: Document TLSConfig and state that it configures generated ServingInfo TLS values.

As per coding guidelines, “Document exported Go functions with a Godoc comment that explains what the function does.”

🤖 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/configobservation/listers.go` around lines 16 - 25, Add GoDoc
comments immediately before the exported methods APIServerLister,
ResourceSyncer, and PreRunHasSynced in pkg/console/configobservation/listers.go,
describing each method’s returned value or purpose. Also document the exported
TLSConfig method in pkg/console/subresource/consoleserver/config_builder.go,
explicitly stating that it configures generated ServingInfo TLS values.

Sources: Coding guidelines, Path instructions

🤖 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/console/configobservation/listers.go`:
- Around line 16-25: Add GoDoc comments immediately before the exported methods
APIServerLister, ResourceSyncer, and PreRunHasSynced in
pkg/console/configobservation/listers.go, describing each method’s returned
value or purpose. Also document the exported TLSConfig method in
pkg/console/subresource/consoleserver/config_builder.go, explicitly stating that
it configures generated ServingInfo TLS values.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 49a6bc92-cbf4-4379-8331-723849632d43

📥 Commits

Reviewing files that changed from the base of the PR and between 67370e1 and eb37b61.

⛔ Files ignored due to path filters (6)
  • vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/OWNERS is excluded by !vendor/**, !**/vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/listers.go is excluded by !vendor/**, !**/vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/observe_audit.go is excluded by !vendor/**, !**/vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/observe_cors.go is excluded by !vendor/**, !**/vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/observe_tlssecurityprofile.go is excluded by !vendor/**, !**/vendor/**
  • vendor/modules.txt is excluded by !vendor/**, !**/vendor/**
📒 Files selected for processing (12)
  • manifests/03-rbac-role-cluster.yaml
  • pkg/console/configobservation/configobservercontroller/observe_config_controller.go
  • pkg/console/configobservation/listers.go
  • pkg/console/operator/sync_v400.go
  • pkg/console/operator/sync_v400_test.go
  • pkg/console/starter/starter.go
  • pkg/console/subresource/configmap/configmap.go
  • pkg/console/subresource/configmap/configmap_test.go
  • pkg/console/subresource/configmap/tech_preview_test.go
  • pkg/console/subresource/configmap/tls_config_test.go
  • pkg/console/subresource/consoleserver/config_builder.go
  • pkg/console/subresource/consoleserver/types.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 (17)
**/*.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
Use gofmt to format Go code with standard formatting
Run go vet checks on all Go packages

Follow 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 in pkg/api/, operator command setup in pkg/cmd/operator/, and version command in pkg/cmd/version/

**/*.go: Use gofmt for 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 using status.Handle* functions with type prefixes (*Degraded, *Progressing, *Available, *Upgradeable)
Use typed errors and wrap errors to preserve stack context

Flag 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 as ioutil.ReadFile, ioutil.WriteFile, ioutil.ReadAll, or net.Dial in Dial callbacks; use os.ReadFile, os.WriteFile, io.ReadAll, and DialContext instead.
When returning errors in Go, wrap them with %w and include meaningful context instead of returning the raw error or using %v.
Use specific error checks such as apierrors.IsNotFound(err) instead of matching error strings with strings.Contains(err.Error(), ...).
Propagate the caller’s context.Context through operations and avoid replacing it with context.Background() inside request/controller code.
Use defer to 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/subresource/configmap/tech_preview_test.go
  • pkg/console/starter/starter.go
  • pkg/console/subresource/configmap/configmap_test.go
  • pkg/console/subresource/configmap/configmap.go
  • pkg/console/subresource/consoleserver/config_builder.go
  • pkg/console/subresource/consoleserver/types.go
  • pkg/console/operator/sync_v400_test.go
  • pkg/console/configobservation/listers.go
  • pkg/console/operator/sync_v400.go
  • pkg/console/subresource/configmap/tls_config_test.go
  • pkg/console/configobservation/configobservercontroller/observe_config_controller.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 functions
  • factory.New().WithFilteredEventsInformers( pattern
  • .ToController( method calls
  • Sync(ctx context.Context, controllerContext factory.SyncContext) methods
  • operatorConfig.Spec.ManagementState checks
  • status.NewStatusHandler or status.Handle* functions

Refer to /sync-handler-review when code contains:

  • Main operator sync functions (e.g., sync_v400.go content)
  • 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: Dial without DialContext
  • Error handling: missing %w in 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/subresource/configmap/tech_preview_test.go
  • pkg/console/starter/starter.go
  • pkg/console/subresource/configmap/configmap_test.go
  • pkg/console/subresource/configmap/configmap.go
  • pkg/console/subresource/consoleserver/config_builder.go
  • pkg/console/subresource/consoleserver/types.go
  • pkg/console/operator/sync_v400_test.go
  • pkg/console/configobservation/listers.go
  • pkg/console/operator/sync_v400.go
  • pkg/console/subresource/configmap/tls_config_test.go
  • pkg/console/configobservation/configobservercontroller/observe_config_controller.go
**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

Follow testing patterns and commands documented in TESTING.md

Follow testing patterns and commands as documented in TESTING.md, including running unit tests with 'make test-unit' and checks with 'make check'

**/*_test.go: Use table-driven tests for comprehensive coverage
Use httptest for HTTP handler testing in Go
Include proper cleanup functions in tests
Test both success and failure paths

In Go tests, do not ignore returned errors; check err and fail the test with t.Fatalf or t.Errorf as appropriate.

Files:

  • pkg/console/subresource/configmap/tech_preview_test.go
  • pkg/console/subresource/configmap/configmap_test.go
  • pkg/console/operator/sync_v400_test.go
  • pkg/console/subresource/configmap/tls_config_test.go

⚙️ CodeRabbit configuration file

**/*_test.go: Review test code for quality and patterns.

Refer to /unit-test-review when test is in pkg//*_test.go:**

  • Table-driven test structure with test cases
  • Use of go-test/deep for struct comparisons
  • Test naming conventions (TestFunctionName)
  • Error handling with wantErr pattern
  • Edge case coverage (nil, empty, boundary values)
  • Proper assertions with helpful error messages
  • Test isolation (no shared mutable state)

Refer to /e2e-test-review when test contains:

  • framework.MustNewClientset(t, nil) or similar e2e framework usage
  • wait.Poll or wait.PollImmediate patterns
  • retry.RetryOnConflict for updates
  • Cleanup via defer functions
  • Console/operator CR manipulations
  • Test assertions on cluster state

Suggest to use /e2e-test-review when:

  • PR adds new feature requiring e2e coverage
  • Test file is empty or skeleton
  • Comments indicate "TODO: add test"

Review for common issues:

  • Missing cleanup (defer statements)
  • Using time.Sleep instead of wait.Poll
  • Missing context timeouts
  • Vague error messages in assertions
  • Tests without table-driven structure when testing multiple cases
  • Ignoring errors with _
  • Tests without assertions

Files:

  • pkg/console/subresource/configmap/tech_preview_test.go
  • pkg/console/subresource/configmap/configmap_test.go
  • pkg/console/operator/sync_v400_test.go
  • pkg/console/subresource/configmap/tls_config_test.go
{pkg,cmd}/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

Use gofmt for code formatting on pkg and cmd directories

{pkg,cmd}/**/*.go: Format code using gofmt -w ./pkg ./cmd
Run go vet checks on all Go packages in ./pkg and ./cmd

Files:

  • pkg/console/subresource/configmap/tech_preview_test.go
  • pkg/console/starter/starter.go
  • pkg/console/subresource/configmap/configmap_test.go
  • pkg/console/subresource/configmap/configmap.go
  • pkg/console/subresource/consoleserver/config_builder.go
  • pkg/console/subresource/consoleserver/types.go
  • pkg/console/operator/sync_v400_test.go
  • pkg/console/configobservation/listers.go
  • pkg/console/operator/sync_v400.go
  • pkg/console/subresource/configmap/tls_config_test.go
  • pkg/console/configobservation/configobservercontroller/observe_config_controller.go
pkg/console/subresource/**/*.go

📄 CodeRabbit inference engine (ARCHITECTURE.md)

Use pkg/console/subresource/ packages for resource builders, with separate packages for each resource type (authentication, configmap, deployment, oauthclient, route, secret, etc.)

Files:

  • pkg/console/subresource/configmap/tech_preview_test.go
  • pkg/console/subresource/configmap/configmap_test.go
  • pkg/console/subresource/configmap/configmap.go
  • pkg/console/subresource/consoleserver/config_builder.go
  • pkg/console/subresource/consoleserver/types.go
  • pkg/console/subresource/configmap/tls_config_test.go
pkg/**/*_test.go

📄 CodeRabbit inference engine (.claude/skills/unit-test-review.md)

pkg/**/*_test.go: Most unit tests should use the table-driven test pattern, including a tests := []struct{...} table and t.Run(tt.name, ...) subtests for scenarios with multiple cases.
Test function names and subtest case names should be descriptive of the behavior or scenario being tested (for example, TestGetNodeComputeEnvironments or "Custom hostname and TLS secret set").
Use github.com/go-test/deep (deep.Equal) for struct comparisons instead of == or manual field-by-field checks.
Cover both success and failure paths in unit tests, including edge cases such as empty inputs, boundary values, missing fields, duplicates, and large inputs.
Structure tests using Arrange-Act-Assert so setup, execution, and verification are clearly separated.
When testing error-returning functions, assert error presence correctly and, when relevant, validate the error message substring instead of ignoring the error or discarding it with _.
Prefer dependency injection via interfaces for testability, and keep tests isolated so they do not depend on execution order or shared mutable state.
Extract repeated setup into helper functions when common test fixtures are reused across multiple tests.
Write specific, informative assertions that explain what failed instead of vague or silent failures.
Inline simple test data, but move complex fixtures to helper functions or testdata/ files.
Avoid tests that rely on execution order, share global mutable state, use hardcoded sleeps, omit assertions, or verify implementation details instead of behavior.

Files:

  • pkg/console/subresource/configmap/tech_preview_test.go
  • pkg/console/subresource/configmap/configmap_test.go
  • pkg/console/operator/sync_v400_test.go
  • pkg/console/subresource/configmap/tls_config_test.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/subresource/configmap/tech_preview_test.go
  • pkg/console/starter/starter.go
  • pkg/console/subresource/configmap/configmap_test.go
  • pkg/console/subresource/configmap/configmap.go
  • pkg/console/subresource/consoleserver/config_builder.go
  • pkg/console/subresource/consoleserver/types.go
  • pkg/console/operator/sync_v400_test.go
  • pkg/console/configobservation/listers.go
  • pkg/console/operator/sync_v400.go
  • pkg/console/subresource/configmap/tls_config_test.go
  • pkg/console/configobservation/configobservercontroller/observe_config_controller.go
**/*.{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-cluster.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 under manifests/, bindata/assets/, quickstarts/, examples/, and profile-patches/ must include the appropriate cluster profile annotations when they are CVO-deployed resources (for example include.release.openshift.io/hypershift, include.release.openshift.io/ibm-cloud-managed, include.release.openshift.io/self-managed-high-availability, and include.release.openshift.io/single-node-developer).
Console resources in the manifest trees must include the capability annotation capability.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 correct apiGroups ("" for core APIs and specific groups for CRDs).
Resources must use the correct namespace for their role: openshift-console for console workload resources and openshift-console-operator for 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, use console-operator in the openshift-console-operator namespace for operator cross-namespace access, and use console in the openshift-console namespace for console workload-scoped access.

Files:

  • manifests/03-rbac-role-cluster.yaml
**/*.yaml

⚙️ CodeRabbit configuration file

**/*.yaml: Review YAML manifests based on content and kind.

Refer to /manifest-review when YAML contains:

  • kind: Role or kind: ClusterRole (RBAC review)
  • kind: RoleBinding or kind: ClusterRoleBinding
  • annotations: section (check for cluster profiles)
  • verbs: ["*"] or wildcard permissions
  • apiGroups: ["*"] or overly broad permissions
  • ServiceAccount references in subjects

Check for required annotations in manifests/:

  • include.release.openshift.io/hypershift
  • include.release.openshift.io/ibm-cloud-managed
  • include.release.openshift.io/self-managed-high-availability
  • include.release.openshift.io/single-node-developer
  • capability.openshift.io/name: Console

For quickstarts/, additionally check:

  • QuickStart spec structure
  • Task descriptions and prerequisites
  • See quickstarts/README.md for guidelines

Files:

  • manifests/03-rbac-role-cluster.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-cluster.yaml
pkg/console/starter/**/*.go

📄 CodeRabbit inference engine (ARCHITECTURE.md)

Access feature gates via featuregates.FeatureGateAccess in starter.go for features like ExternalOIDC and ConsolePluginContentSecurityPolicy

Files:

  • pkg/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_test.go
  • 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_test.go
  • 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
**/*{crypt,cipher,sign,hash,tls,ssl,cert,key,token}*

⚙️ CodeRabbit configuration file

**/*{crypt,cipher,sign,hash,tls,ssl,cert,key,token}*: Cryptographic security (prodsec-skills):

  • Banned: MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB mode
  • Symmetric: AES-256-GCM or ChaCha20-Poly1305
  • Passwords: Argon2id (not bcrypt/scrypt for new code)
  • Signing: Ed25519 or ECDSA P-256+
  • Key exchange: X25519 or ECDH P-256+
  • Constant-time comparison for all secret/token data
  • Zeroize key material after use (no garbage-collector reliance)
  • No custom crypto; use vetted libraries only
  • Post-quantum: flag if protecting long-lived secrets

Files:

  • pkg/console/subresource/configmap/tls_config_test.go
**/*controller*.go

📄 CodeRabbit inference engine (CONVENTIONS.md)

Use the OpenShift library-go factory pattern for controllers

**/*controller*.go: Use the controller factory pattern with factory.New().WithFilteredEventsInformers(), util.IncludeNamesFilter() for informer filtering, and a descriptive ToController() call with a recorder.
In controller sync logic, handle all operatorConfig.Spec.ManagementState values: Managed, Unmanaged, Removed, and return an error for unknown states.
Create and use status.NewStatusHandler(c.operatorClient), set the appropriate status condition helpers (HandleProgressingOrDegraded(), HandleDegraded(), HandleProgressing(), HandleAvailable()), and always call FlushAndReturn() 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 using fmt.Errorf("failed to X: %w", err), handle apierrors.IsNotFound() appropriately for delete operations, optional resources, and get-before-create flows, and return meaningful error messages.
Use resourceapply.Apply*() functions from library-go, pass the controller recorder for events, and handle returned errors properly.
Set owner references with util.OwnerRefFrom(cr) from pkg/console/subresource/util, ensure only one owner reference has controller=true, and clean up owner references when replacing existing resources.

Files:

  • pkg/console/configobservation/configobservercontroller/observe_config_controller.go
**/*controller.go

📄 CodeRabbit inference engine (.claude/skills/sync-handler-review.md)

**/*controller.go: Controller Sync methods 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.
Use resourceapply.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 treat NotFound as a successful no-op.
Check feature gates before syncing gated resources, and only reconcile the gated resources when the feature is enabled.
Respect ManagementState, including missing cleanup logic for Removed state.
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/configobservation/configobservercontroller/observe_config_controller.go
🔇 Additional comments (10)
pkg/console/configobservation/configobservercontroller/observe_config_controller.go (1)

20-54: LGTM!

pkg/console/starter/starter.go (1)

30-30: LGTM!

Also applies to: 639-646, 676-676

manifests/03-rbac-role-cluster.yaml (1)

40-40: LGTM!

pkg/console/operator/sync_v400.go (1)

5-5: LGTM!

Also applies to: 448-472, 955-978

pkg/console/operator/sync_v400_test.go (1)

19-19: LGTM!

Also applies to: 734-750, 752-846

pkg/console/subresource/consoleserver/types.go (1)

51-62: LGTM!

pkg/console/subresource/configmap/configmap.go (1)

54-55: LGTM!

Also applies to: 117-117

pkg/console/subresource/configmap/tls_config_test.go (1)

13-95: LGTM!

pkg/console/subresource/configmap/configmap_test.go (1)

1307-1311: LGTM!

pkg/console/subresource/configmap/tech_preview_test.go (1)

61-64: LGTM!

Also applies to: 130-132

@wking

wking commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

build timeout and some console debug thing seem unrelated:

/retest-required

@TheRealJon

Copy link
Copy Markdown
Member

/lgtm
/approve

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2026
@openshift-ci

openshift-ci Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: TheRealJon, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 6, 2026
@openshift-ci

openshift-ci Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: TheRealJon, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@wking

wking commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

I'm going to recycle the precedent from the original pull to get the other labels:

/label docs-approved
/label px-approved
/verified by CI passing, and an inability to automatically test nightly timing pre-merge

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Aug 6, 2026
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@wking: This PR has been marked as verified by CI passing,and an inability to automatically test nightly timing pre-merge.

Details

In response to this:

I'm going to recycle the precedent from the original pull to get the other labels:

/label docs-approved
/label px-approved
/verified by CI passing, and an inability to automatically test nightly timing pre-merge

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.

@openshift-ci openshift-ci Bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Aug 6, 2026
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

/retest-required

Remaining retests: 0 against base HEAD 67370e1 and 2 for PR HEAD eb37b61 in total

@openshift-ci

openshift-ci Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@wking: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-console eb37b61 link true /test e2e-aws-console

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants