NO-JIRA: Revert "Merge pull request #1170 from logonoff/http2"#1194
NO-JIRA: Revert "Merge pull request #1170 from logonoff/http2"#1194logonoff wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request explicitly references no 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. |
WalkthroughThe change removes HTTP/2 certificate generation and ingress CA handling, simplifies route controller and informer wiring, updates one RBAC namespace, and removes related unit and end-to-end coverage. TLS unset validation and the e2e resource framework are adjusted accordingly. ChangesRoute TLS removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/label px-approved |
openshift-ci
Bot
added
px-approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/route/controller.go`:
- Around line 327-331: Update the Godoc comment for the exported custom
certificate validation function to begin exactly with ValidateCustomCertSecret,
while preserving the existing explanation of the required tls.crt and tls.key
data and validation behavior.
🪄 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: 99306135-eb03-4474-96f9-98b3976cf126
📒 Files selected for processing (11)
manifests/03-rbac-role-ns-openshift-ingress-operator.yamlmanifests/04-rbac-rolebinding.yamlpkg/api/api.gopkg/console/controllers/route/controller.gopkg/console/controllers/route/controller_test.gopkg/console/starter/starter.gopkg/console/subresource/route/http2.gopkg/console/subresource/route/http2_test.gotest/e2e/custom_url_test.gotest/e2e/framework/framework.gotest/e2e/http2_cert_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/console(manual)
💤 Files with no reviewable changes (9)
- manifests/03-rbac-role-ns-openshift-ingress-operator.yaml
- pkg/api/api.go
- test/e2e/http2_cert_test.go
- pkg/console/subresource/route/http2.go
- pkg/console/subresource/route/http2_test.go
- pkg/console/controllers/route/controller_test.go
- test/e2e/framework/framework.go
- pkg/console/starter/starter.go
- manifests/04-rbac-rolebinding.yaml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
**/*.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:
test/e2e/custom_url_test.gopkg/console/controllers/route/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 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:
test/e2e/custom_url_test.gopkg/console/controllers/route/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
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:
test/e2e/custom_url_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:
test/e2e/custom_url_test.go
**/test/**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/test/**/*.go: When new Ginkgo e2e tests are added (It(), Describe(), Context(), When(), etc.), check whether they make assumptions about IPv4 networking or require connectivity to external/public internet services. Flag if IPv6-only CI jobs would fail due to hardcoded IPv4 addresses, IPv4 localhost assumptions, IPv4-only IP parsing, hardcoded IPv4 CIDRs/addresses in Service/Endpoint objects, IPv4 values in net.ParseIP()/net.ParseCIDR(), assumptions that pod/node IPs are IPv4, hardcoded IPv4-only network policies, or URLs built without brackets for IPv6 (should use net.JoinHostPort instead). Also flag if external connectivity is required to public registries, external URLs, or external APIs.
When new Ginkgo e2e tests are added, verify they do not use unavailable MicroShift APIs (Project/ProjectRequest, Build/BuildConfig, DeploymentConfig, ClusterOperator, ClusterVersion, Etcd operator, CSV/OLM resources, MachineSet/Machine/MachineHealthCheck, ClusterAutoscaler, Console, Monitoring components, ImageRegistry operator, Samples operator, OperatorHub/CatalogSource, CloudCredential, Storage/Network operators) or reference missing namespaces (openshift-kube-apiserver, openshift-kube-controller-manager, openshift-kube-scheduler). Also flag tests making multi-node/HA assumptions, using FeatureGate resources, or assuming node scaling. Do not flag if test has [Skipped:MicroShift], [apigroup:...] tags, exutil.IsMicroShiftCluster() check, or is in protected Describe/Context.
OpenShift Tests Extension (OTE) binaries must not output non-JSON data to stdout from process-level code (main(), init(), TestMain(), BeforeSuite(), AfterSuite(), SynchronizedBeforeSuite(), RunSpecs() setup, or top-level var/const initializers), as this corrupts test listing in CI. Flag klog writing to stdout (must redirect to stderr), fmt.Print/Println/Printf in main/suite setup, Ginkgo v2 suite configuration emitting warnings to stdout, and log.SetOutput not set to stderr before logging. Do not flag wr...
Files:
test/e2e/custom_url_test.go
test/e2e/*.go
📄 CodeRabbit inference engine (.claude/skills/e2e-test-review.md)
test/e2e/*.go: Useframework.StandardSetup(t)for e2e test setup instead of manual client construction orframework.MustNewClientset(t, nil)when initializing test state.
Usedefer framework.StandardCleanup(t, client)to restore pristine state after each e2e test.
Create contexts with a timeout usingcontext.WithTimeout(..., framework.AsyncOperationTimeout)instead ofcontext.Background()for e2e operations.
Useretry.RetryOnConflictwhen updating console/operator configuration objects to handle concurrent update conflicts.
Usewait.Pollfor eventual consistency checks instead oftime.Sleep, and prefer a 5-second polling interval for most checks unless the resource being waited on justifies a different interval.
Use framework resource helpers such asGetConsoleDeployment,GetConsoleConfigMap,GetConsoleService,GetConsoleRoute, and similar helpers instead of manually fetching the same resources when a helper exists.
Write cleanup code that checksapierrors.IsNotFound(err)and only fails on non-NotFound errors when deleting test-created resources.
Provide helpful, contextual failure messages in assertions and fatal errors instead of vague messages liket.Fatal("failed")ort.Fatal("not ready").
Avoid ignoring returned errors with_; tests should assert on errors and verify behavior explicitly.
Use table-driven tests when a test covers multiple similar cases.
Use framework constants such asframework.AsyncOperationTimeoutinstead of hardcoded timeout durations when waiting for asynchronous operations.
Prefer the older acceptable framework client pattern (framework.MustNewClientset(t, nil)plus operator config retrieval) only whenframework.StandardSetup(t)is not used, and avoid manualclientcmd.BuildConfigFromFlags/kubernetes.NewForConfigsetup.
Files:
test/e2e/custom_url_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:
test/e2e/custom_url_test.gopkg/console/controllers/route/controller.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/controllers/route/controller.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.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.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.go
🔇 Additional comments (2)
pkg/console/controllers/route/controller.go (1)
49-52: LGTM!Also applies to: 65-69, 82-89, 102-102, 119-124, 208-219
test/e2e/custom_url_test.go (1)
438-441: 🩺 Stability & AvailabilityRemove the nil-guard concern. The route manifest already includes a TLS block, so
route.Spec.TLSis not expected to be nil on this path.> Likely an incorrect or invalid review comment.
| // Validate secret that holds custom TLS certificate and key. | ||
| // Secret has to contain `tls.crt` and `tls.key` data keys | ||
| // where the certificate and key are stored and both need | ||
| // to be in valid format. | ||
| // Return the custom TLS certificate and key |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/console/controllers/route/controller.go"
echo "--- line count ---"
wc -l "$FILE"
echo "--- relevant slice ---"
sed -n '315,345p' "$FILE" | cat -nRepository: openshift/console-operator
Length of output: 1921
Start the Godoc comment with ValidateCustomCertSecret. The exported function’s doc comment should begin with its identifier so GoDoc picks it up.
🤖 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/route/controller.go` around lines 327 - 331, Update
the Godoc comment for the exported custom certificate validation function to
begin exactly with ValidateCustomCertSecret, while preserving the existing
explanation of the required tls.crt and tls.key data and validation behavior.
Source: Coding guidelines
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, 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
|
/verified by ci |
openshift-ci-robot
added
the
verified
|
@logonoff: 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. |
|
@logonoff: This pull request explicitly references no 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. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@logonoff: The following test 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
This reverts commit 658550c, reversing changes made to 814688e.
The code worked but the outcome of having a self signed cert even if a cert was provided to the ingresscontroller not desirable. Combined with http2 not being enabled by default in this release and graphql removal not being a performance regression in http1, the feature doesn't serve much purpose...
Summary by CodeRabbit