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. |
WalkthroughExpire old pod-specific session cookies during AddSession, GetSession, and UpdateTokens; DeleteSession now creates new expired cookies instead of mutating existing ones. Tests added to verify cleanup behavior and some test helpers switched to type-inferred Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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. |
|
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: |
|
@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? |
|
/jira refresh The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity. |
|
@openshift-bot: This pull request references Jira Issue OCPBUGS-65967, which is valid. 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. |
|
/hold |
|
To provide some updates after @yapei 's observation. I did reproduced the issues that @yapei reported on a freshly nightly build clusterbot cluster, and with the locally build console image that contain @TheRealJon 's changes in this PR. Claude & I discovered few things:
See the patch fix here: https://gist.github.com/Leo6Leo/620999e1febdd7d11adc8998f20c9f42 |
Extract cookie cleanup logic into expireOldPodCookies helper method and add proper cookie attributes (Secure, HttpOnly, SameSite) required for browsers to properly delete cookies. Expand cleanup to GetSession and UpdateTokens to handle all load balancing scenarios. Add comprehensive test coverage for all cleanup paths. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Leo Li <leoli@redhat.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@Leo6Leo I've reviewed and applied your patch. I'll spin up a cluster to check and make sure everything is working as expected. Thanks for the help while I was OOO! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/auth/sessions/combined_sessions.go`:
- Around line 54-62: The cookie-clearing code that calls http.SetCookie
currently omits the Domain attribute, which violates the "Must match all
attributes of the original cookie" rule; update both occurrences where
http.SetCookie is used (the block that sets Name: cookie.Name, Value: "",
MaxAge: -1) to include Domain: cs.clientStore.Options.Domain so the expired
cookie matches the original cookie attributes from cs.clientStore.Options and
will be properly deleted if Domain is later configured.
| http.SetCookie(w, &http.Cookie{ | ||
| Name: cookie.Name, | ||
| Value: "", | ||
| Path: cs.clientStore.Options.Path, | ||
| MaxAge: -1, | ||
| Secure: cs.clientStore.Options.Secure, | ||
| HttpOnly: cs.clientStore.Options.HttpOnly, | ||
| SameSite: cs.clientStore.Options.SameSite, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find Domain configuration in cookie options
rg -n "\.Options\.Domain" --type=goRepository: openshift/console
Length of output: 43
🏁 Script executed:
# Also check the Options struct definition
rg -n "type.*Options struct" --type=go -A 10Repository: openshift/console
Length of output: 50374
🏁 Script executed:
# Examine the specific file and lines mentioned
cat -n pkg/auth/sessions/combined_sessions.go | head -250 | tail -200Repository: openshift/console
Length of output: 9045
🏁 Script executed:
# Find where clientStore is defined and what type it is
rg -n "clientStore.*Options" pkg/auth/sessions/combined_sessions.go -B 5 -A 5Repository: openshift/console
Length of output: 1662
🏁 Script executed:
# Search for the Options struct definition related to cookies/sessions
rg -n "type.*Options struct" pkg/auth/sessions/ -A 15Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Look for imports to understand what cookie store is being used
head -30 pkg/auth/sessions/combined_sessions.goRepository: openshift/console
Length of output: 648
🏁 Script executed:
# Check the gorilla/sessions Options struct definition
rg -n "type Options struct" vendor/github.com/gorilla/sessions/ -A 20Repository: openshift/console
Length of output: 1079
🏁 Script executed:
# Also verify if Domain is ever set on clientStore anywhere in the codebase
rg -n "\.Options\.Domain\s*=" --type=goRepository: openshift/console
Length of output: 43
🏁 Script executed:
# Verify that Domain is never configured in clientStore.Options throughout the codebase
rg -n "clientStore\.Options" pkg/auth/sessions/ --type=goRepository: openshift/console
Length of output: 1108
🏁 Script executed:
# Also check if Domain might be set through some other mechanism
rg -n "Domain\s*:" pkg/auth/sessions/ --type=go -B 2 -A 2Repository: openshift/console
Length of output: 43
Include Domain in cookie expiration for consistency, even though it's not currently configured.
The comment on line 53 states "Must match all attributes of the original cookie," but the Domain attribute from cs.clientStore.Options is omitted. While Domain is not currently set in your cookie store initialization, including it makes the code more complete and defensive. If Domain configuration is ever added in the future, this ensures cookies will be properly deleted.
Proposed fix
http.SetCookie(w, &http.Cookie{
Name: cookie.Name,
Value: "",
Path: cs.clientStore.Options.Path,
+ Domain: cs.clientStore.Options.Domain,
MaxAge: -1,
Secure: cs.clientStore.Options.Secure,
HttpOnly: cs.clientStore.Options.HttpOnly,
SameSite: cs.clientStore.Options.SameSite,
})Also applies to: 231-239
🤖 Prompt for AI Agents
In `@pkg/auth/sessions/combined_sessions.go` around lines 54 - 62, The
cookie-clearing code that calls http.SetCookie currently omits the Domain
attribute, which violates the "Must match all attributes of the original cookie"
rule; update both occurrences where http.SetCookie is used (the block that sets
Name: cookie.Name, Value: "", MaxAge: -1) to include Domain:
cs.clientStore.Options.Domain so the expired cookie matches the original cookie
attributes from cs.clientStore.Options and will be properly deleted if Domain is
later configured.
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
I am not sure if I should be the one giving LGTM on this PR as it contains my patch. I will leave it to @jhadvig or @krishagarwal278 to give the final green light on this. |
/verified by @yapei |
openshift-ci-robot
added
the
verified
|
@yapei: 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. |
|
The fix looks good to me. @Leo6Leo your approach to identify these key issues, was significant 🙌🏻
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: krishagarwal278, 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 |
|
/retest-required |
|
@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. |
|
@TheRealJon: Jira Issue Verification Checks: Jira Issue OCPBUGS-65967 Jira Issue OCPBUGS-65967 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
/cherry-pick release-4.21 |
|
@Leo6Leo: new pull request created: #15985 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 kubernetes-sigs/prow repository. |
7 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