WIP OCPBUGS-71237: Generate session-secret for all auth types - #1204
WIP OCPBUGS-71237: Generate session-secret for all auth types#1204jhadvig wants to merge 1 commit into
Conversation
Extend the session-secret Secret (previously OIDC-only) to all authentication types including OpenShift/IntegratedOAuth. This provides shared encryption keys across console pods so cookies can be decrypted after pod restarts, enabling persistent sessions. Changes: - syncSessionSecret() now runs for all auth types, not just OIDC - Config builder sets session key file paths for OpenShift auth - Extract session key path constants to avoid duplication The deployment volume mount is already conditional on sessionSecret being non-nil, so no deployment changes are needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
openshift-ci-robot
added
jira/valid-reference
|
@jhadvig: This pull request references Jira Issue OCPBUGS-71237, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughThe console configuration builder now emits session authentication and encryption key paths for enabled authentication modes. Disabled authentication returns an empty session configuration. Session Secret synchronization now applies to all authentication types. Tests update structured and YAML expectations. ChangesSession configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig 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
![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/operator/sync_v400.go`:
- Around line 124-126: Update the error return immediately after
syncSessionSecret in the operator synchronization flow to wrap the error with
meaningful session Secret synchronization context while preserving the original
cause via %w. Keep the existing statusHandler.FlushAndReturn handling unchanged.
🪄 Autofix
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: 7a53bbeb-7bc9-492b-82cf-aa10b9880c50
📒 Files selected for processing (5)
pkg/console/operator/sync_v400.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/consoleserver/config_builder_test.gopkg/console/subresource/consoleserver/config_merger_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/console(manual) → reviewed against open PR#16911OCPBUGS-71237instead of the default branch
📜 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:
pkg/console/operator/sync_v400.gopkg/console/subresource/consoleserver/config_merger_test.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder_test.gopkg/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/operator/sync_v400.gopkg/console/subresource/consoleserver/config_merger_test.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder_test.gopkg/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/operator/sync_v400.gopkg/console/subresource/consoleserver/config_merger_test.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder_test.gopkg/console/subresource/consoleserver/config_builder.go
**/*sync*.go
📄 CodeRabbit inference engine (CONVENTIONS.md)
Implement sync loops (
sync_v400) incrementally: start from zero, create/update missing requirements, and return to continue on next loop
Files:
pkg/console/operator/sync_v400.go
**/operator/**/*.go
📄 CodeRabbit inference engine (Custom checks)
When deployment manifests, operator code, or controllers are added/modified, ensure they do not introduce scheduling constraints assuming standard HA topology. Check ControlPlaneTopology for SingleReplica/DualReplica/HighlyAvailableArbiter/External modes before applying constraints. Use required anti-affinity with maxUnavailable >= 1 (not maxUnavailable: 0). Cap replica counts to schedulable nodes. Exclude arbiter nodes on TNA. Avoid master nodeSelectors on HyperShift. Use library-go DeploymentController hooks (WithTopologyAwareReplicasHook, WithTopologyAwareSchedulingHook, WithControlPlaneNodeSelectorHook).
Files:
pkg/console/operator/sync_v400.go
**/sync_v400.go
📄 CodeRabbit inference engine (.claude/skills/sync-handler-review.md)
Incremental sync pattern: each sync loop should stop on the first error and resume from the next step on the next reconciliation instead of collecting and joining all errors.
Files:
pkg/console/operator/sync_v400.go
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}
⚙️ CodeRabbit configuration file
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}: Injection prevention (prodsec-skills):
- SQL: parameterized queries only; no string concatenation
- Command: no shell=True, os.system, or backtick exec with user input
- LDAP/XPath: escape special characters in filters
- Path traversal: canonicalize paths, reject ../
- Deserialization: no pickle/yaml.load()/eval on untrusted data
- Prototype pollution: no recursive merge of untrusted objects
- Validate at trust boundaries with allow-lists, not deny-lists
- Normalize Unicode and anchor regexes (^$); watch for ReDoS
Files:
pkg/console/operator/sync_v400.gopkg/console/subresource/consoleserver/config_merger_test.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder_test.gopkg/console/subresource/consoleserver/config_builder.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/consoleserver/config_merger_test.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder_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/consoleserver/config_merger_test.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder_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/consoleserver/config_merger_test.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder_test.gopkg/console/subresource/consoleserver/config_builder.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/consoleserver/config_merger_test.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/consoleserver/config_builder_test.go
🪛 ast-grep (0.45.0)
pkg/console/subresource/consoleserver/config_builder.go
[warning] 25-25: A credential is hard-coded as a string literal. Secrets stored in source code, such as passwords, API keys, and tokens, can be leaked through version control or binaries and used by internal or external malicious actors. Rotate the exposed secret and load it at runtime from a secure secret vault, a Hardware Security Module (HSM), or an environment variable if permitted by your company policy (e.g. password := os.Getenv("APP_PASSWORD")).
Context: sessionAuthKeyFilePath = "/var/session-secret/sessionAuthenticationKey"
Note: [CWE-798] Use of Hard-coded Credentials.
(hardcoded-credentials-string-literal-go)
🔇 Additional comments (4)
pkg/console/subresource/consoleserver/config_builder.go (1)
24-27: LGTM!Also applies to: 203-204, 224-225, 459-472
pkg/console/subresource/consoleserver/config_builder_test.go (1)
74-77: LGTM!Also applies to: 110-113, 157-160, 211-214, 365-368, 404-407, 443-446, 506-509, 569-572, 612-615, 655-658, 697-700, 753-756, 821-824, 888-891, 948-951, 1005-1008, 1049-1052, 1094-1097, 1151-1153, 1236-1238, 1263-1265, 1292-1294, 1322-1324, 1376-1378, 1422-1424, 1459-1461, 1497-1499, 1584-1586, 1632-1634, 1704-1706, 1780-1782, 1836-1838, 1874-1876
pkg/console/subresource/configmap/configmap_test.go (1)
126-128: LGTM!Also applies to: 194-196, 222-224, 280-282, 310-312, 365-367, 402-404, 453-455, 496-498, 547-549, 653-655, 704-706, 776-778, 827-829, 902-904, 972-974, 1045-1047, 1159-1161, 1231-1233, 1307-1309, 1546-1548, 1559-1561
pkg/console/subresource/consoleserver/config_merger_test.go (1)
64-66: LGTM!
| sessionSecret, err = co.syncSessionSecret(ctx, updatedOperatorConfig, controllerContext.Recorder()) | ||
| if err != nil { | ||
| return statusHandler.FlushAndReturn(err) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap the session Secret synchronization error with context.
Line 126 returns the raw error. Add the failed operation name and preserve the cause with %w. This improves diagnosis because this operation now runs for every authentication type.
Proposed fix
- return statusHandler.FlushAndReturn(err)
+ return statusHandler.FlushAndReturn(fmt.Errorf("sync session Secret: %w", err))As per coding guidelines, “When returning errors in Go, wrap them with %w and include meaningful context instead of returning the raw error or using %v.”
📝 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.
| sessionSecret, err = co.syncSessionSecret(ctx, updatedOperatorConfig, controllerContext.Recorder()) | |
| if err != nil { | |
| return statusHandler.FlushAndReturn(err) | |
| sessionSecret, err = co.syncSessionSecret(ctx, updatedOperatorConfig, controllerContext.Recorder()) | |
| if err != nil { | |
| return statusHandler.FlushAndReturn(fmt.Errorf("sync session Secret: %w", err)) |
🤖 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/operator/sync_v400.go` around lines 124 - 126, Update the error
return immediately after syncSessionSecret in the operator synchronization flow
to wrap the error with meaningful session Secret synchronization context while
preserving the original cause via %w. Keep the existing
statusHandler.FlushAndReturn handling unchanged.
Source: Coding guidelines
2 participants
Analysis / Root cause:
Console sessions are lost on pod restart because encryption keys are generated randomly per process, making cookies non-portable across pods. The
session-secretSecret infrastructure already exists for OIDC auth but was not enabled for OpenShift/IntegratedOAuth auth.Jira: https://redhat.atlassian.net/browse/OCPBUGS-71237
Companion PR: openshift/console#16911
Solution description:
Extend the existing
session-secretSecret management to all authentication types:syncSessionSecret()now runs for all auth types, not just OIDC. This creates the Secret with shared encryption keys (64-byte HMAC + 32-byte AES) on first reconciliation.sessionAuthKeyFilePathandsessionEncKeyFilePathconstants.sessionSecret != nil, which is now always true for non-disabled auth.Upgrade safety: On upgrade,
syncSessionSecret()creates the Secret before the ConfigMap and Deployment are synced. Kubelet won't start new pods until volumes are satisfiable.Rollback safety: If downgraded, the orphaned Secret is harmless (~96 bytes). The downgraded operator won't set session key paths, so console reverts to per-pod random keys. Users are logged out (expected and unavoidable).
Test cases:
Additional info:
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests