OCPBUGS-65967: Clean up old session cookies to prevent accumulation #15837
Conversation
When users are load-balanced across multiple console pods, each pod creates a session cookie with a unique name based on POD_NAME: openshift-session-token-<POD_NAME>. With a 1-month cookie expiration, users accumulate cookies from different pods without old ones being removed, eventually causing the cookie header to exceed 4096 bytes. This fix cleans up session cookies from other pods when creating a new session, ensuring only one active session cookie exists at a time. Changes: - Modified AddSession() to expire old pod cookies before creating new session - Updated DeleteSession() to use modern cookie expiration pattern - Added test to verify old pod cookies are properly expired Fixes: OCPBUGS-65967 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-ci-robot
added
jira/severity-moderate
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-65967, 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. |
WalkthroughThe changes update session cookie handling: AddSession now expires session cookies from other pods (by cookie-name prefix) while keeping the current pod's cookie, and DeleteSession stops mutating cookie objects in-place and instead constructs and sets new http.Cookie instances with MaxAge=-1 to invalidate cookies. Test code uses type-inferred utilptr.To calls. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
openshift-ci
bot
added
component/backend
|
/hold Needs testing |
openshift-ci
bot
added
the
do-not-merge/hold
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/auth/sessions/combined_sessions.go(2 hunks)pkg/auth/sessions/combined_sessions_test.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/auth/sessions/combined_sessions_test.gopkg/auth/sessions/combined_sessions.go
🧬 Code graph analysis (2)
pkg/auth/sessions/combined_sessions_test.go (2)
pkg/auth/sessions/combined_sessions.go (2)
NewSessionStore(31-44)SessionCookieName(26-29)pkg/auth/sessions/server_session.go (1)
OpenshiftAccessTokenCookieName(17-17)
pkg/auth/sessions/combined_sessions.go (1)
pkg/auth/sessions/server_session.go (1)
OpenshiftAccessTokenCookieName(17-17)
🔇 Additional comments (2)
pkg/auth/sessions/combined_sessions_test.go (2)
55-55: LGTM: Type inference improves readability.The changes from
utilptr.To[string]toutilptr.Toleverage Go's type inference, making the code cleaner while maintaining equivalent functionality.Also applies to: 67-67, 96-96
699-745: Verify Path attribute in expired cookies.The test correctly verifies that
MaxAgeis set to-1for old pod cookies, but it doesn't check whether thePathattribute is set correctly. Cookies must have matchingName,Domain, andPathto be deleted by the browser. IfPathis not set correctly in the expired cookies, the browser won't delete them, which would defeat the purpose of this cleanup logic.Consider adding an assertion like:
if c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-1" || c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-2" { require.Equal(t, -1, c.MaxAge, "Old pod cookie %s should be expired", c.Name) + require.Equal(t, "/", c.Path, "Old pod cookie %s should have correct Path for deletion", c.Name) expiredCookies++ }
When deleting cookies via HTTP response headers, browsers need the deletion cookie to match the Name, Path, and Domain of the original cookie. The previous implementation only set MaxAge=-1 on the existing cookie object without explicitly setting the Path, which could prevent proper cookie deletion. This change creates a new cookie with the minimal required attributes (Name, Path, Value="", MaxAge=-1) using the path from the session store options, ensuring the browser properly recognizes and deletes the cookie. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Assign for my own review tracking purposes: |
|
/retest |
1 similar comment
|
/retest |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-65967, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@TheRealJon: 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. |
|
From the code's perspective, I think there is no problem and it will solve the problem. Will need the help from QE to verify it on the real cluster if it actually solves the problem. /lgtm QE verification: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Leo6Leo, 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 |
|
@Leo6Leo @TheRealJon here are my steps to verify the changes, I created a testing cluster with cluster-bot (
So it appears to me that the old session cookies are still not cleared as we expect? |
4 participants
Problem
When users are load-balanced across multiple console pods, each pod creates a session cookie with a unique name based on
POD_NAME:openshift-session-token-<POD_NAME>.With a 1-month cookie expiration, users accumulate cookies from different pods without old ones being removed. This is especially problematic with Azure AD SSO where new authentication tokens are issued on each login, causing OpenShift to create new session cookies instead of reusing old ones.
Over time, the cookie header can exceed 4096 bytes, causing errors.
Solution
This fix cleans up session cookies from other pods when creating a new session, ensuring only one active session cookie exists at a time.
Changes
AddSession()to expire old pod cookies before creating new session inpkg/auth/sessions/combined_sessions.go:46-73DeleteSession()to use modern cookie expiration pattern (removed outdated Go loop variable capture)TestCombinedSessionStore_AddSession_CleansUpOldPodCookiesto verify old pod cookies are properly expiredTesting
Impact
Fixes: OCPBUGS-65967
🤖 Generated with Claude Code