CONSOLE-5298: Auto-generate throwaway TLS cert for HTTP/2 support#1170
CONSOLE-5298: Auto-generate throwaway TLS cert for HTTP/2 support#1170logonoff wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request references CONSOLE-5298 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
WalkthroughAdds HTTP/2 TLS certificate generation, validation, secret reconciliation, optional ingress-CA signing, informer/RBAC wiring, controller cleanup, unit tests, and e2e tests for the OpenShift console route. ChangesHTTP/2 Certificate Management for Console Route
Sequence DiagramsequenceDiagram
participant RouteController as RouteSyncController
participant LoadCA as loadIngressCA
participant Ensure as EnsureHTTP2Cert
participant SecretClient as Secrets API
participant Validator as validHTTP2Cert
participant Generator as GenerateHTTP2Cert
RouteController->>LoadCA: loadIngressCA()
LoadCA-->>RouteController: *libcrypto.CA or nil
RouteController->>Ensure: EnsureHTTP2Cert(hostname, ca)
Ensure->>SecretClient: Get existing TLS Secret
alt secret exists
Ensure->>Validator: validHTTP2Cert(cert,key,ca)
Validator-->>Ensure: valid / invalid
alt valid
Ensure-->>RouteController: return existing cert/key
else invalid
Ensure->>Generator: GenerateHTTP2Cert(hostname, ca)
Generator-->>Ensure: new cert/key
Ensure->>SecretClient: Update secret
Ensure-->>RouteController: return new cert/key
end
else secret missing
Ensure->>Generator: GenerateHTTP2Cert(hostname, ca)
Generator-->>Ensure: cert/key
Ensure->>SecretClient: Create TLS secret
Ensure-->>RouteController: return cert/key
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🤖 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 150-152: The call to removeHTTP2CertSecret currently only logs
failures so sync can succeed while the console-http2-cert remains; change
removeHTTP2CertSecret to return an error (instead of only logging) and update
both call sites in controller.go (the blocks checking c.routeName ==
api.OpenShiftConsoleRouteName) to capture that error and return it up the sync
path (i.e., if err := c.removeHTTP2CertSecret(ctx); err != nil { return err }).
Ensure removeHTTP2CertSecret surfaces delete errors from the Kubernetes client
rather than swallowing them.
In `@pkg/console/starter/starter.go`:
- Around line 154-158: The ingress-namespace informer factory
kubeInformersIngressOperatorNamespaced must include a List/Watch name-filter so
the Secret named api.IngressCASecretName is actually returned (RBAC uses
resourceNames), not just filtered at event time by util.IncludeNamesFilter;
update the call to informers.NewSharedInformerFactoryWithOptions that creates
kubeInformersIngressOperatorNamespaced to add informers.WithTweakListOptions (or
equivalent) that sets the ListOptions FieldSelector to
"metadata.name=<api.IngressCASecretName>" (e.g., via metav1.ListOptions or
fields.OneTermEqualSelector) so loadIngressCA() can find the router-ca Secret
during list/watch.
In `@pkg/console/subresource/route/http2_test.go`:
- Around line 339-344: The helper newFakeSecretLister currently ignores the
error returned by indexer.Add which can hide malformed test fixtures; change
newFakeSecretLister to check the error from indexer.Add for each secret and fail
fast on error (e.g., return an error from newFakeSecretLister or panic/fatal
with a clear message) so malformed secrets don't silently get dropped; update
all call sites/tests to handle the new error return if you choose to return an
error instead of panicking.
🪄 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: dd3e3297-5108-4e98-8975-c569d5690a6c
📒 Files selected for processing (8)
manifests/03-rbac-role-ns-openshift-ingress-operator.yamlmanifests/04-rbac-rolebinding.yamlpkg/api/api.gopkg/console/controllers/route/controller.gopkg/console/operator/sync_v400.gopkg/console/starter/starter.gopkg/console/subresource/route/http2.gopkg/console/subresource/route/http2_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (13)
**/*.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 contextOpenShift Tests Extension (OTE) binaries communicate with openshift-tests via JSON on stdout. Flag non-JSON stdout from the main binary process in process-level code: main(), init(), TestMain(), BeforeSuite(), AfterSuite(), SynchronizedBeforeSuite(), RunSpecs() setup, or top-level var/const initializers. Common violations include klog writing to stdout (must redirect to stderr), fmt.Print/Println/Printf in main or suite setup, and Ginkgo v2 suite configuration emitting warnings to stdout
Files:
pkg/api/api.gopkg/console/subresource/route/http2.gopkg/console/operator/sync_v400.gopkg/console/starter/starter.gopkg/console/subresource/route/http2_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:
pkg/api/api.gopkg/console/subresource/route/http2.gopkg/console/operator/sync_v400.gopkg/console/starter/starter.gopkg/console/subresource/route/http2_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/api/api.gopkg/console/subresource/route/http2.gopkg/console/operator/sync_v400.gopkg/console/starter/starter.gopkg/console/subresource/route/http2_test.gopkg/console/controllers/route/controller.go
**/*.{go,java,py,js,ts,jsx,tsx,cs,cpp,c,rb,php}
📄 CodeRabbit inference engine (Custom checks)
**/*.{go,java,py,js,ts,jsx,tsx,cs,cpp,c,rb,php}: Flag logging that may expose passwords, tokens, API keys, PII (email, SSN, credit card), session IDs, internal hostnames, or customer data
Flag MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB mode usage. Flag custom crypto implementations. Flag non-constant-time comparison of secrets or tokens
Files:
pkg/api/api.gopkg/console/subresource/route/http2.gopkg/console/operator/sync_v400.gopkg/console/starter/starter.gopkg/console/subresource/route/http2_test.gopkg/console/controllers/route/controller.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/api/api.gopkg/console/subresource/route/http2.gopkg/console/operator/sync_v400.gopkg/console/starter/starter.gopkg/console/subresource/route/http2_test.gopkg/console/controllers/route/controller.go
**/*.{yaml,yml}
📄 CodeRabbit inference engine (Custom checks)
Flag privileged: true, hostPID, hostNetwork, hostIPC, SYS_ADMIN capability, running as root without justification, and allowPrivilegeEscalation: true in container/K8s manifests
Files:
manifests/03-rbac-role-ns-openshift-ingress-operator.yamlmanifests/04-rbac-rolebinding.yaml
⚙️ CodeRabbit configuration file
**/*.{yaml,yml}: If this is a Kubernetes/OpenShift manifest or Helm template:
- securityContext: runAsNonRoot, readOnlyRootFilesystem,
allowPrivilegeEscalation: false- Drop ALL capabilities, add only what is required
- Resource limits (cpu, memory) on every container
- No hostPID, hostNetwork, hostIPC, privileged: true
- NetworkPolicy defined for the namespace
- OpenShift: SCC must be restricted or custom-scoped
- Liveness + readiness probes defined
- automountServiceAccountToken: false unless needed
- RBAC: least privilege; no cluster-admin for workloads
- Helm: no .Values interpolation in shell commands
Files:
manifests/03-rbac-role-ns-openshift-ingress-operator.yamlmanifests/04-rbac-rolebinding.yaml
**/*.yaml
⚙️ CodeRabbit configuration file
**/*.yaml: Review YAML manifests based on content and kind.Refer to /manifest-review when YAML contains:
kind: Roleorkind: ClusterRole(RBAC review)kind: RoleBindingorkind: ClusterRoleBindingannotations:section (check for cluster profiles)verbs: ["*"]or wildcard permissionsapiGroups: ["*"]or overly broad permissions- ServiceAccount references in subjects
Check for required annotations in manifests/:
include.release.openshift.io/hypershiftinclude.release.openshift.io/ibm-cloud-managedinclude.release.openshift.io/self-managed-high-availabilityinclude.release.openshift.io/single-node-developercapability.openshift.io/name: ConsoleFor quickstarts/, additionally check:
- QuickStart spec structure
- Task descriptions and prerequisites
- See quickstarts/README.md for guidelines
Files:
manifests/03-rbac-role-ns-openshift-ingress-operator.yamlmanifests/04-rbac-rolebinding.yaml
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/route/http2.gopkg/console/subresource/route/http2_test.go
**/*sync*.go
📄 CodeRabbit inference engine (CONVENTIONS.md)
Implement sync loops (
sync_v400) incrementally: start from zero, create/update missing requirements, and return to continue on next loop
Files:
pkg/console/operator/sync_v400.go
**/{deploy,config,operator,controllers}/**/*.{yaml,yml,go}
📄 CodeRabbit inference engine (Custom checks)
**/{deploy,config,operator,controllers}/**/*.{yaml,yml,go}: When deployment manifests, operator code, or controllers are added or modified, check for scheduling constraints that assume a standard HA topology without topology-awareness. Flag required pod anti-affinity (requiredDuringSchedulingIgnoredDuringExecution with topologyKey: kubernetes.io/hostname) that breaks on SNO, TNF with replicas > 2, and TNA
When deployment manifests, operator code, or controllers are added or modified, check for pod topology spread constraints with whenUnsatisfiable: DoNotSchedule and hostname topology key that breaks on SNO and is problematic when maxSkew exceeds the number of schedulable nodes on TNF and TNA
When deployment manifests, operator code, or controllers are added or modified, check for replica count derived from node count without considering SNO, TNF, and TNA topology variations where fewer than 3 control-plane nodes exist, and TNA's arbiter node should not run general workloads
When deployment manifests, operator code, or controllers are added or modified, check for nodeSelector or node affinity targeting control-plane nodes (node-role.kubernetes.io/master or node-role.kubernetes.io/control-plane) that will fail on HyperShift where no nodes carry these labels
When deployment manifests, operator code, or controllers are added or modified, check for scheduling workloads to all control-plane nodes equally without excluding arbiter nodes. Flag broad or wildcard tolerations that inadvertently schedule to resource-constrained arbiter nodes on TNA clusters
When deployment manifests, operator code, or controllers are added or modified, check for assumptions that dedicated worker nodes exist. Code targeting only worker nodes via node-role.kubernetes.io/worker nodeSelector may need to also consider nodes that carry both control-plane and worker roles on SNO and TNF
When deployment manifests, operator code, or controllers are added or modified, check PodDisruptionBudgets designe...
Files:
pkg/console/operator/sync_v400.gopkg/console/controllers/route/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
**/*_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 paths
Files:
pkg/console/subresource/route/http2_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/route/http2_test.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
Files:
pkg/console/controllers/route/controller.go
🪛 golangci-lint (2.12.2)
pkg/console/subresource/route/http2_test.go
[error] 342-342: Error return value of indexer.Add is not checked
(errcheck)
🔇 Additional comments (3)
pkg/console/subresource/route/http2_test.go (3)
20-70: LGTM!Also applies to: 72-163
165-316: LGTM!
318-337: LGTM!Also applies to: 347-358
…ole route HTTP/2 between browser and HAProxy requires a unique cert on the route (distinct from the shared wildcard cert) so the router's cert_config.map adds per-cert ALPN. Without this, removing GraphQL causes a 67% API discovery regression under HTTP/1.1; with HTTP/2 it is a 21x improvement. The route controller now auto-generates a throwaway TLS certificate for the console route hostname when no admin-provided cert is configured. The cert is signed by the ingress controller's CA (router-ca) when available, falling back to self-signed. It is persisted in a Secret in openshift-console so it survives operator restarts without triggering unnecessary route updates. Admin-provided certs always take precedence. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/framework/framework.go (1)
146-164:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
deleteResourceis missing a"Secret"case, breakingDeleteAll/cleanup.
getTestingResources()now includes theSecret(Line 53) andGetResourcehandles it (Lines 95-96), butdeleteResourcehas no"Secret"branch. InDeleteAll/LoopResources, once the throwawayconsole-http2-certsecret exists,DeleteCompletelyfetches it successfully, thendeleteResourcefalls into thedefaultcase and returns"resource Secret not identified", causingt.Fatalf. This will fail cleanup for any managed-state e2e run where the secret is present.🐛 Add the missing Secret delete case
case "ConfigMap": err = client.Core.ConfigMaps(resource.namespace).Delete(context.TODO(), resource.name, metav1.DeleteOptions{}) + case "Secret": + err = client.Core.Secrets(resource.namespace).Delete(context.TODO(), resource.name, metav1.DeleteOptions{}) case "Service": err = client.Core.Services(resource.namespace).Delete(context.TODO(), resource.name, metav1.DeleteOptions{})🤖 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 `@test/e2e/framework/framework.go` around lines 146 - 164, The deleteResource function is missing a case for resource.kind == "Secret", causing cleanup failures; add a "Secret" branch in deleteResource that calls the core client to delete the secret (use client.Core.Secrets(resource.namespace).Delete(context.TODO(), resource.name, metav1.DeleteOptions{})), placed alongside the other kind cases before the default, so secrets returned by GetResource/DeleteAll are handled correctly.
🤖 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 `@test/e2e/http2_cert_test.go`:
- Around line 138-139: The test currently deletes the TLS test secret with a
direct call to client.Core.Secrets(...).Delete which is skipped on early
failures; instead, register the cleanup immediately after creating the secret by
deferring a call that deletes tlsSecretName from api.OpenShiftConfigNamespace
(use the same client.Core.Secrets(...).Delete call inside the deferred
function). Ensure the deferred cleanup is placed right after the secret creation
and coexists with cleanupCustomURLTestCase so the tlsSecretName is always
removed even if t.Fatalf occurs.
- Around line 125-132: The poll handler currently treats any error from
client.Core.Secrets(...).Get as a successful fallback; change the logic in the
check where adminSecret, secretErr :=
client.Core.Secrets(api.OpenShiftConfigNamespace).Get(...) is called so that
only a metav1.IsNotFound(secretErr) is interpreted as "admin cert gone" (return
true, nil), while any other non-nil error should be returned as an error (return
false, secretErr) so the poll will surface or retry the transient failure; keep
the existing comparison of route.Spec.TLS.Certificate vs
string(adminSecret.Data["tls.crt"]) unchanged.
---
Outside diff comments:
In `@test/e2e/framework/framework.go`:
- Around line 146-164: The deleteResource function is missing a case for
resource.kind == "Secret", causing cleanup failures; add a "Secret" branch in
deleteResource that calls the core client to delete the secret (use
client.Core.Secrets(resource.namespace).Delete(context.TODO(), resource.name,
metav1.DeleteOptions{})), placed alongside the other kind cases before the
default, so secrets returned by GetResource/DeleteAll are handled correctly.
🪄 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: 8527e95f-fba4-4185-9080-f7f9b98e3ceb
📒 Files selected for processing (5)
pkg/console/controllers/route/controller_test.gopkg/console/subresource/route/http2_test.gotest/e2e/custom_url_test.gotest/e2e/framework/framework.gotest/e2e/http2_cert_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (11)
**/*.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 contextOpenShift Tests Extension (OTE) binaries communicate with openshift-tests via JSON on stdout. Flag non-JSON stdout from the main binary process in process-level code: main(), init(), TestMain(), BeforeSuite(), AfterSuite(), SynchronizedBeforeSuite(), RunSpecs() setup, or top-level var/const initializers. Common violations include klog writing to stdout (must redirect to stderr), fmt.Print/Println/Printf in main or suite setup, and Ginkgo v2 suite configuration emitting warnings to stdout
Files:
test/e2e/framework/framework.gopkg/console/controllers/route/controller_test.gotest/e2e/http2_cert_test.gotest/e2e/custom_url_test.gopkg/console/subresource/route/http2_test.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/framework/framework.gopkg/console/controllers/route/controller_test.gotest/e2e/http2_cert_test.gotest/e2e/custom_url_test.gopkg/console/subresource/route/http2_test.go
**/*.{go,java,py,js,ts,jsx,tsx,cs,cpp,c,rb,php}
📄 CodeRabbit inference engine (Custom checks)
**/*.{go,java,py,js,ts,jsx,tsx,cs,cpp,c,rb,php}: Flag logging that may expose passwords, tokens, API keys, PII (email, SSN, credit card), session IDs, internal hostnames, or customer data
Flag MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB mode usage. Flag custom crypto implementations. Flag non-constant-time comparison of secrets or tokens
Files:
test/e2e/framework/framework.gopkg/console/controllers/route/controller_test.gotest/e2e/http2_cert_test.gotest/e2e/custom_url_test.gopkg/console/subresource/route/http2_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/framework/framework.gopkg/console/controllers/route/controller_test.gotest/e2e/http2_cert_test.gotest/e2e/custom_url_test.gopkg/console/subresource/route/http2_test.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 paths
Files:
pkg/console/controllers/route/controller_test.gotest/e2e/http2_cert_test.gotest/e2e/custom_url_test.gopkg/console/subresource/route/http2_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/controllers/route/controller_test.gotest/e2e/http2_cert_test.gotest/e2e/custom_url_test.gopkg/console/subresource/route/http2_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/controllers/route/controller_test.gopkg/console/subresource/route/http2_test.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_test.go
**/*controller*.go
📄 CodeRabbit inference engine (CONVENTIONS.md)
Use the OpenShift library-go factory pattern for controllers
Files:
pkg/console/controllers/route/controller_test.go
**/{deploy,config,operator,controllers}/**/*.{yaml,yml,go}
📄 CodeRabbit inference engine (Custom checks)
**/{deploy,config,operator,controllers}/**/*.{yaml,yml,go}: When deployment manifests, operator code, or controllers are added or modified, check for scheduling constraints that assume a standard HA topology without topology-awareness. Flag required pod anti-affinity (requiredDuringSchedulingIgnoredDuringExecution with topologyKey: kubernetes.io/hostname) that breaks on SNO, TNF with replicas > 2, and TNA
When deployment manifests, operator code, or controllers are added or modified, check for pod topology spread constraints with whenUnsatisfiable: DoNotSchedule and hostname topology key that breaks on SNO and is problematic when maxSkew exceeds the number of schedulable nodes on TNF and TNA
When deployment manifests, operator code, or controllers are added or modified, check for replica count derived from node count without considering SNO, TNF, and TNA topology variations where fewer than 3 control-plane nodes exist, and TNA's arbiter node should not run general workloads
When deployment manifests, operator code, or controllers are added or modified, check for nodeSelector or node affinity targeting control-plane nodes (node-role.kubernetes.io/master or node-role.kubernetes.io/control-plane) that will fail on HyperShift where no nodes carry these labels
When deployment manifests, operator code, or controllers are added or modified, check for scheduling workloads to all control-plane nodes equally without excluding arbiter nodes. Flag broad or wildcard tolerations that inadvertently schedule to resource-constrained arbiter nodes on TNA clusters
When deployment manifests, operator code, or controllers are added or modified, check for assumptions that dedicated worker nodes exist. Code targeting only worker nodes via node-role.kubernetes.io/worker nodeSelector may need to also consider nodes that carry both control-plane and worker roles on SNO and TNF
When deployment manifests, operator code, or controllers are added or modified, check PodDisruptionBudgets designe...
Files:
pkg/console/controllers/route/controller_test.go
**/{test,tests,e2e,integration}/**/*_test.go
📄 CodeRabbit inference engine (Custom checks)
**/{test,tests,e2e,integration}/**/*_test.go: When new Ginkgo e2e tests are added, check for IPv4 assumptions including hardcoded IPv4 addresses (e.g., '10.0.0.1', '192.168.1.1', '172.16.0.0/12'), hardcoded IPv4 localhost ('127.0.0.1'), IPv4-only IP parsing, hardcoded IPv4 CIDRs in Service/Endpoint objects, IPv4-only net.ParseIP/ParseCIDR usage, assumptions about pod/node IPv4 addresses, and IPv4-only network policies
When new Ginkgo e2e tests are added, check for IPv4-only network assumptions such as building URLs by interpolating host/IP without brackets for IPv6 (e.g., fmt.Sprintf('http://%s:%d/path', host, port)). Use net.JoinHostPort(host, port) instead, which adds brackets automatically for IPv6
When new Ginkgo e2e tests are added, check for external connectivity requirements including connections to public internet hosts (e.g., google.com, github.com, quay.io, registry.redhat.io), pulling images from public registries, downloading content from external URLs, DNS resolution of public hostnames, and connections to external APIs or services outside the cluster
When new Ginkgo e2e tests are added, check whether they use any APIs or features that are NOT available on MicroShift. Flag tests that reference Project/ProjectRequest, Build/BuildConfig, DeploymentConfig, ClusterOperator, ClusterVersion, Etcd operator, CSV/OLM resources, MachineSet/Machine/MachineHealthCheck, ClusterAutoscaler/MachineAutoscaler, Console, Monitoring stack components, ImageRegistry operator, Samples operator, OperatorHub/CatalogSource/PackageManifest, CloudCredential/CredentialsRequest, Storage operator, Network operator CRDs, or any OpenShift API groups besides Route and SecurityContextConstraints
When new Ginkgo e2e tests are added, check whether they reference namespaces that do not exist on MicroShift: openshift-kube-apiserver, openshift-kube-controller-manager, openshift-kube-scheduler
When new Ginkgo e2e tests are added, check whether they make unsupported MicroShift assumptions inc...
Files:
test/e2e/http2_cert_test.gotest/e2e/custom_url_test.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:
test/e2e/http2_cert_test.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/route/http2_test.go
🪛 golangci-lint (2.12.2)
test/e2e/http2_cert_test.go
[error] 127-127: error is not nil (line 125) but it returns nil
(nilerr)
🔇 Additional comments (4)
pkg/console/subresource/route/http2_test.go (1)
319-442: LGTM!pkg/console/controllers/route/controller_test.go (1)
212-333: LGTM!test/e2e/custom_url_test.go (1)
418-449: LGTM!test/e2e/framework/framework.go (1)
210-222: Clarifyconsole-http2-certSecret expectations in e2e Managed/Unmanaged/Removed checks
test/e2e/framework/framework.go:getTestingResources()always includes{"Secret", consoleapi.ConsoleHTTP2CertSecretName, consoleapi.OpenShiftConsoleNamespace};ConsoleResourcesAvailable/ConsoleResourcesUnavailabledo not gate this on any admin-vs-throwaway HTTP/2 cert configuration.test/e2e/http2_cert_test.goasserts the operator creates this Secret in Managed mode (non-emptytls.crt/tls.key) and regenerates it after deletion, so the Managed expectation matches current test intent; ensure Unmanaged/Removed cleanup deletes it and does not recreate it, since the Unmanaged/Removed assertions require the Secret to becomeNotFound.
|
/label px-approved |
openshift-ci
Bot
added
the
px-approved
jseseCCS
left a comment
jseseCCS
left a comment
There was a problem hiding this comment.
reviewed for user-facing strings. a few suggestions around passive voice, word order, informal terminology ("throwaway")... please consider/apply as appropriate. Approving in good faith. 😺
|
as usual, approving in good faith that you'll consider/apply my comments as appropriate. |
|
/label docs-approved |
openshift-ci
Bot
added
the
docs-approved
Unit tests: - TestValidHTTP2Cert_ExpiredCert: cert within renewal buffer is rejected - TestEnsureHTTP2Cert_CARotation: cert regenerated when CA changes - TestEnsureHTTP2Cert_HostnameChange: cert regenerated when hostname changes - TestRemoveHTTP2CertSecret: secret cleanup (exists, not-found, nil client) - TestLoadIngressCA: CA loading (nil lister, not-found, invalid PEM, valid) E2e tests: - TestHTTP2CertAutoGeneration: secret created, route has TLS - TestHTTP2CertRegeneration: deleted secret is recreated - TestHTTP2CertAdminOverride: admin cert wins, falls back to throwaway - TestHTTP2CertNotOnDownloadsRoute: downloads route unaffected - Updated checkCustomTLSWasUnset to handle throwaway cert - Added console-http2-cert to getTestingResources for Removed test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@test/e2e/framework/framework.go`:
- Line 53: getTestingResources now includes {"Secret",
consoleapi.ConsoleHTTP2CertSecretName, consoleapi.OpenShiftConsoleNamespace} but
deleteResource lacks a case for "Secret", causing DeleteAll to error; update the
deleteResource function to handle the "Secret" kind by implementing a case
"Secret" branch that uses the corev1 Secrets client (or the same client pattern
used for other core resources) to delete the named secret in the given
namespace, mirror the existing error handling and logging behavior used by other
cases, and ensure DeleteAll will succeed when the HTTP/2 cert secret exists.
🪄 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: e7e9034b-8a0a-4313-99a2-24c4aababc45
📒 Files selected for processing (5)
pkg/console/controllers/route/controller_test.gopkg/console/subresource/route/http2_test.gotest/e2e/custom_url_test.gotest/e2e/framework/framework.gotest/e2e/http2_cert_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/custom_url_test.go
- pkg/console/controllers/route/controller_test.go
- test/e2e/http2_cert_test.go
- pkg/console/subresource/route/http2_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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 contextOpenShift Tests Extension (OTE) binaries communicate with openshift-tests via JSON on stdout. Flag non-JSON stdout from the main binary process in process-level code: main(), init(), TestMain(), BeforeSuite(), AfterSuite(), SynchronizedBeforeSuite(), RunSpecs() setup, or top-level var/const initializers. Common violations include klog writing to stdout (must redirect to stderr), fmt.Print/Println/Printf in main or suite setup, and Ginkgo v2 suite configuration emitting warnings to stdout
Files:
test/e2e/framework/framework.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/framework/framework.go
**/*.{go,java,py,js,ts,jsx,tsx,cs,cpp,c,rb,php}
📄 CodeRabbit inference engine (Custom checks)
**/*.{go,java,py,js,ts,jsx,tsx,cs,cpp,c,rb,php}: Flag logging that may expose passwords, tokens, API keys, PII (email, SSN, credit card), session IDs, internal hostnames, or customer data
Flag MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB mode usage. Flag custom crypto implementations. Flag non-constant-time comparison of secrets or tokens
Files:
test/e2e/framework/framework.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/framework/framework.go
🔇 Additional comments (1)
test/e2e/framework/framework.go (1)
95-96: LGTM!
| {"Deployment", consoleapi.OpenShiftConsoleDeploymentName, consoleapi.OpenShiftConsoleNamespace}, | ||
| {"Deployment", consoleapi.OpenShiftConsoleDownloadsDeploymentName, consoleapi.OpenShiftConsoleNamespace}, | ||
| {"Route", consoleapi.OpenShiftConsoleRouteName, consoleapi.OpenShiftConsoleNamespace}, | ||
| {"Secret", consoleapi.ConsoleHTTP2CertSecretName, consoleapi.OpenShiftConsoleNamespace}, |
There was a problem hiding this comment.
Add Secret deletion support before registering it as a tracked testing resource.
Line 53 adds "Secret" to getTestingResources, but deleteResource has no case "Secret". If the HTTP/2 cert secret exists, DeleteAll will fail with resource Secret not identified.
Proposed fix
func deleteResource(client *ClientSet, resource TestingResource) error {
var err error
switch resource.kind {
case "ConfigMap":
err = client.Core.ConfigMaps(resource.namespace).Delete(context.TODO(), resource.name, metav1.DeleteOptions{})
+ case "Secret":
+ err = client.Core.Secrets(resource.namespace).Delete(context.TODO(), resource.name, metav1.DeleteOptions{})
case "Service":
err = client.Core.Services(resource.namespace).Delete(context.TODO(), resource.name, metav1.DeleteOptions{})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {"Secret", consoleapi.ConsoleHTTP2CertSecretName, consoleapi.OpenShiftConsoleNamespace}, | |
| func deleteResource(client *ClientSet, resource TestingResource) error { | |
| var err error | |
| switch resource.kind { | |
| case "ConfigMap": | |
| err = client.Core.ConfigMaps(resource.namespace).Delete(context.TODO(), resource.name, metav1.DeleteOptions{}) | |
| case "Secret": | |
| err = client.Core.Secrets(resource.namespace).Delete(context.TODO(), resource.name, metav1.DeleteOptions{}) | |
| case "Service": | |
| err = client.Core.Services(resource.namespace).Delete(context.TODO(), resource.name, metav1.DeleteOptions{}) | |
| // ... remaining cases | |
| } | |
| // ... error handling and return | |
| } |
🤖 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 `@test/e2e/framework/framework.go` at line 53, getTestingResources now includes
{"Secret", consoleapi.ConsoleHTTP2CertSecretName,
consoleapi.OpenShiftConsoleNamespace} but deleteResource lacks a case for
"Secret", causing DeleteAll to error; update the deleteResource function to
handle the "Secret" kind by implementing a case "Secret" branch that uses the
corev1 Secrets client (or the same client pattern used for other core resources)
to delete the named secret in the given namespace, mirror the existing error
handling and logging behavior used by other cases, and ensure DeleteAll will
succeed when the HTTP/2 cert secret exists.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jseseCCS, logonoff, TheRealJon 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
|
test cases were run by claude. I also tested this on a cluster and verified console will serve assets using http2 when enabled via annotation /verified by Claude |
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 references CONSOLE-5298 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
Analysis / Root cause:
HTTP/2 between the browser and HAProxy requires a unique TLS certificate on the route (distinct from the shared wildcard cert). Without it, the router's
cert_config.mapdoes not add[alpn h2,http/1.1]for the route, and the browser cannot negotiate HTTP/2.The console route uses the shared ingress wildcard cert by default, so HTTP/2 is never negotiated. Admins can manually configure a custom cert, but this requires explicit action on every cluster.
Solution description:
The route controller now auto-generates a throwaway TLS certificate for the console route hostname when no admin-provided cert is configured:
pkg/console/subresource/route/http2.go): Uses library-go'scrypto.MakeServerCertForDurationto generate a serving cert. When the ingress controller's CA (router-cainopenshift-ingress-operator) is available, the cert is signed by it so the trust chain matches the wildcard cert. Falls back to a self-signed CA otherwise.console-http2-certSecret inopenshift-consoleso it survives operator restarts without triggering unnecessary route updates.AuthorityKeyId/SubjectKeyIdcomparison), and key integrity. Only regenerated when invalid.ingress.config.openshift.io/clusterspec.componentRoutesor the operator configspec.route.secret, the admin-provided cert takes precedence and the throwaway cert is not used.console-http2-certSecret is deleted whenManagementState=Removed.router-cainopenshift-ingress-operator(scoped byresourceNames).sync_v400.gowhere thecmErrclosure variable shadowed the outercmErrinRetryOnConflict.Note: HTTP/2 also requires
ROUTER_DISABLE_HTTP2=falseon the IngressController. This is a separate effort being discussed with the NI&D team (enabling HTTP/2 by default for new clusters).Test setup:
ROUTER_DISABLE_HTTP2=false:Test cases:
oc get secret console-http2-cert -n openshift-console)curl -kvso /dev/null --http2 https://<console-route> 2>&1 | grep "ALPN: server accepted h2"curl -kvso /dev/null --http2 https://<oauth-route> 2>&1 | grep "ALPN: server accepted http/1.1"console-http2-certSecret triggers regeneration on next syncManagementState=Removeddeletes theconsole-http2-certSecretROUTER_DISABLE_HTTP2=true(cert is generated but ALPN is not advertised by router)Browser conformance:
Additional info:
cert_config.mapso per-cert ALPN negotiation kicks in. It is not intended for production trust — admins should replace it with a real cert.Summary by CodeRabbit
New Features
Security
Tests