CNTRLPLANE-3423: feat: inject centralized TLS into console operand - #1196
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ingvagabund: This pull request references CNTRLPLANE-3423 which is a valid jira issue. 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. |
|
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 (12)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (10)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (8)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*_test.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
{pkg,cmd}/**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
pkg/console/subresource/**/*.go📄 CodeRabbit inference engine (ARCHITECTURE.md)
Files:
pkg/**/*_test.go📄 CodeRabbit inference engine (.claude/skills/unit-test-review.md)
Files:
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}⚙️ CodeRabbit configuration file
Files:
**/*sync*.go📄 CodeRabbit inference engine (CONVENTIONS.md)
Files:
**/operator/**/*.go📄 CodeRabbit inference engine (Custom checks)
Files:
🔀 Multi-repo context openshift/consoleLinked repositories findingsopenshift/console
🔇 Additional comments (2)
WalkthroughThe console operator now observes API server TLS settings, extracts them from ChangesTLS configuration flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Starter
participant ConfigObserver
participant ConfigAPIServer
participant SyncConfigMap
participant DefaultConfigMap
participant ConsoleConfigBuilder
Starter->>ConfigObserver: construct and start controller
ConfigObserver->>ConfigAPIServer: watch API server informer
ConfigAPIServer-->>ConfigObserver: provide API server TLS settings
SyncConfigMap->>SyncConfigMap: read observedConfig
SyncConfigMap->>DefaultConfigMap: pass TLS minimum version and cipher suites
DefaultConfigMap->>ConsoleConfigBuilder: call TLSConfig
ConsoleConfigBuilder-->>DefaultConfigMap: render ServingInfo TLS fields
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/console/configobservation/listers.go (2)
16-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd Godoc for the newly exported methods.
pkg/console/configobservation/listers.go#L16-L25: documentAPIServerLister,ResourceSyncer, andPreRunHasSynced.pkg/console/subresource/consoleserver/config_builder.go#L328-L332: documentTLSConfig.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 add a Godoc comment before the exported TLSConfig symbol in pkg/console/subresource/consoleserver/config_builder.go explaining what it provides.Sources: Coding guidelines, Path instructions
3-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the repository’s comment-separated import groups.
pkg/console/configobservation/listers.go#L3-L8: group Kubernetes/OpenShift imports according to the documented convention.pkg/console/configobservation/configobservercontroller/observe_config_controller.go#L3-L14: separate the internalconsole-operatorimport from external OpenShift/Kubernetes dependencies.As per coding guidelines, “Group imports in order: standard lib, 3rd party, kube/openshift, internal (marked with comments).”
🤖 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 3 - 8, Reorganize imports in pkg/console/configobservation/listers.go (lines 3-8) into the repository’s documented Kubernetes/OpenShift grouping, and update pkg/console/configobservation/configobservercontroller/observe_config_controller.go (lines 3-14) to place the internal console-operator import in its own commented group after external OpenShift/Kubernetes dependencies; preserve all imported symbols.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.
Inline comments:
In `@pkg/console/operator/sync_v400.go`:
- Around line 450-453: Update the TLS handling in the reconciliation flow around
getTLSConfigFromObservedConfig so a non-nil tlsErr is returned through the
existing sync/status error path instead of logging and continuing with empty TLS
values. Preserve successful configuration generation when no error occurs, and
use the established reconciliation error handling so the failure is surfaced
through status conditions.
In `@pkg/console/subresource/configmap/configmap_test.go`:
- Around line 1307-1311: Update the DefaultConfigMap invocation in this test to
capture the returned error instead of discarding it, then fail the test
immediately when err is non-nil before using cm. Preserve the existing TLS
arguments and assertions.
---
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 add a Godoc comment before the exported TLSConfig symbol
in pkg/console/subresource/consoleserver/config_builder.go explaining what it
provides.
- Around line 3-8: Reorganize imports in
pkg/console/configobservation/listers.go (lines 3-8) into the repository’s
documented Kubernetes/OpenShift grouping, and update
pkg/console/configobservation/configobservercontroller/observe_config_controller.go
(lines 3-14) to place the internal console-operator import in its own commented
group after external OpenShift/Kubernetes dependencies; preserve all imported
symbols.
🪄 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: 7649474e-8843-4515-a27c-7073e91ea32f
⛔ Files ignored due to path filters (5)
vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/listers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/observe_audit.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/observe_cors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/observe_tlssecurityprofile.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (10)
manifests/03-rbac-role-cluster.yamlpkg/console/configobservation/configobservercontroller/observe_config_controller.gopkg/console/configobservation/listers.gopkg/console/operator/sync_v400.gopkg/console/starter/starter.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/configmap/tls_config_test.gopkg/console/subresource/consoleserver/config_builder.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/console(manual) → reviewed against open PR#16804tls-injection-to-consoleinstead of the default branch
📜 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
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/subresource/configmap/tls_config_test.gopkg/console/configobservation/listers.gopkg/console/configobservation/configobservercontroller/observe_config_controller.gopkg/console/starter/starter.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/operator/sync_v400.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/subresource/configmap/tls_config_test.gopkg/console/configobservation/listers.gopkg/console/configobservation/configobservercontroller/observe_config_controller.gopkg/console/starter/starter.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/operator/sync_v400.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
Usehttptestfor HTTP handler testing in Go
Include proper cleanup functions in tests
Test both success and failure pathsIn Go tests, do not ignore returned errors; check
errand fail the test witht.Fatalfort.Errorfas appropriate.
Files:
pkg/console/subresource/configmap/tls_config_test.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/configmap/tech_preview_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/deepfor struct comparisons- Test naming conventions (TestFunctionName)
- Error handling with
wantErrpattern- 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 usagewait.Pollorwait.PollImmediatepatternsretry.RetryOnConflictfor updates- Cleanup via
deferfunctions- 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.Sleepinstead ofwait.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/tls_config_test.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/configmap/tech_preview_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 usinggofmt -w ./pkg ./cmd
Rungo vetchecks on all Go packages in ./pkg and ./cmd
Files:
pkg/console/subresource/configmap/tls_config_test.gopkg/console/configobservation/listers.gopkg/console/configobservation/configobservercontroller/observe_config_controller.gopkg/console/starter/starter.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/operator/sync_v400.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/tls_config_test.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/tech_preview_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 atests := []struct{...}table andt.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,TestGetNodeComputeEnvironmentsor"Custom hostname and TLS secret set").
Usegithub.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 ortestdata/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/tls_config_test.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/configmap/tech_preview_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/tls_config_test.gopkg/console/configobservation/listers.gopkg/console/configobservation/configobservercontroller/observe_config_controller.gopkg/console/starter/starter.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/tech_preview_test.gopkg/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 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/configobservation/configobservercontroller/observe_config_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/configobservation/configobservercontroller/observe_config_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
**/*.{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 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-cluster.yaml
**/*.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-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
**/*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
🔀 Multi-repo context openshift/console
Linked repositories findings
openshift/console
- On PR branch
refs/pull/16804/head, centralized TLS observation is wired throughstarter.gointo the newConfigObserver, and TLS values are consumed bysync_v400.gowhen generating the ConfigMap.[::openshift/console::] - The same branch adds the
config.openshift.io/apiserversRBAC permission required by the observer’s API-server informer.[::openshift/console::] DefaultConfigMapcallers and tests were updated for the new TLS parameters; no additional repository consumers were identified.[::openshift/console::]
| false, // techPreviewEnabled - default to false for tests | ||
| false, // olmLifecycleMetadataEnabled - default to false for tests | ||
| nil, // additionalHosts | ||
| "", // tlsMinVersion - empty for legacy tests | ||
| []string{}, // tlsCiphers - empty for legacy tests |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check the DefaultConfigMap error in this test.
Although TLS arguments are added here, the invocation starting at Line 1292 still assigns cm, _, _. Fail immediately on err; otherwise a generation failure can produce a nil dereference and obscure the cause.
🤖 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/subresource/configmap/configmap_test.go` around lines 1307 -
1311, Update the DefaultConfigMap invocation in this test to capture the
returned error instead of discarding it, then fail the test immediately when err
is non-nil before using cm. Preserve the existing TLS arguments and assertions.
Sources: Coding guidelines, Path instructions
7c9be39 to
a87821f
Compare
a87821f to
eecfe76
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/console/subresource/consoleserver/config_builder.go (1)
328-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Godoc comment for the exported method.
Document
TLSConfigimmediately above its declaration, with a comment beginning withTLSConfig.Proposed fix
+// TLSConfig configures the minimum TLS version and cipher suites. func (b *ConsoleServerCLIConfigBuilder) TLSConfig(minVersion configv1.TLSProtocolVersion, ciphers []string) *ConsoleServerCLIConfigBuilder {As per coding guidelines, exported Go functions must have a Godoc comment.
🤖 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/subresource/consoleserver/config_builder.go` around lines 328 - 333, Add a Godoc comment immediately above the exported ConsoleServerCLIConfigBuilder.TLSConfig method, beginning with “TLSConfig” and briefly describing its configuration behavior.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/subresource/consoleserver/config_builder.go`:
- Around line 328-333: Add a Godoc comment immediately above the exported
ConsoleServerCLIConfigBuilder.TLSConfig method, beginning with “TLSConfig” and
briefly describing its configuration behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4fef1c6c-45fd-417a-bca9-be809f3311af
⛔ Files ignored due to path filters (6)
vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/listers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/observe_audit.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/observe_cors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/operator/configobserver/apiserver/observe_tlssecurityprofile.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (10)
manifests/03-rbac-role-cluster.yamlpkg/console/configobservation/configobservercontroller/observe_config_controller.gopkg/console/configobservation/listers.gopkg/console/operator/sync_v400.gopkg/console/starter/starter.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/configmap/tls_config_test.gopkg/console/subresource/consoleserver/config_builder.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/console(manual) → reviewed against open PR#16804tls-injection-to-consoleinstead of the default branch
🚧 Files skipped from review as they are similar to previous changes (9)
- manifests/03-rbac-role-cluster.yaml
- pkg/console/configobservation/listers.go
- pkg/console/subresource/configmap/tls_config_test.go
- pkg/console/subresource/configmap/configmap.go
- pkg/console/starter/starter.go
- pkg/console/subresource/configmap/tech_preview_test.go
- pkg/console/configobservation/configobservercontroller/observe_config_controller.go
- pkg/console/subresource/configmap/configmap_test.go
- pkg/console/operator/sync_v400.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/subresource/consoleserver/config_builder.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/subresource/consoleserver/config_builder.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/subresource/consoleserver/config_builder.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/consoleserver/config_builder.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/consoleserver/config_builder.go
🔀 Multi-repo context
Linked repositories findings
openshift/console
Inspected PR branch refs/pull/16804/head:
pkg/console/starter/starter.gostarts the newConfigObserver, which observes centralized TLS configuration.pkg/console/operator/sync_v400.goconsumes observed TLS settings when generating the console ConfigMap.manifests/03-rbac-role-cluster.yamlgrantsconsole-operatorget/list/watchaccess toconfig.openshift.io/apiservers, required by the API-server informer.pkg/console/subresource/configmap/configmap.gochangesDefaultConfigMap’s signature; repository call sites and tests were updated accordingly. No additional consumers were found.
🔇 Additional comments (1)
pkg/console/subresource/consoleserver/config_builder.go (1)
88-89: LGTM!Also applies to: 375-382
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
eecfe76 to
10d84fd
Compare
jhadvig
left a comment
jhadvig
left a comment
There was a problem hiding this comment.
Looks good, one comment otherwise good to go 👍
To support cluster wide cryptographic policy aligning with a centralized TLS configuration.
10d84fd to
e59e3e4
Compare
|
/retest-required |
|
/test e2e-gcp-ovn |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ingvagabund, spadgett The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/label docs-approved |
openshift-ci
Bot
added
docs-approved
|
/retest-required |
|
/verified by @kaleemsiddiqu |
openshift-ci-robot
added
the
verified
|
@kaleemsiddiqu: This PR has been marked as verified by 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. |
|
@ingvagabund: 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. |
5 participants
Needs openshift/console#16804
Summary by CodeRabbit