Support multiple JSESSIONID cookies for CHIPS migration#546
Support multiple JSESSIONID cookies for CHIPS migration#546hoffmaen merged 1 commit intocloudfoundry:developfrom
Conversation
7ce954a to
59cbdcf
Compare
| for _, sc := range sessionCookies { | ||
| vcapCookie := newVcapCookie() | ||
| vcapCookie.Secure = vcapCookie.Secure || sc.Secure | ||
| vcapCookie.SameSite = sc.SameSite | ||
| vcapCookie.Partitioned = sc.Partitioned | ||
| vcapCookie.MaxAge = sc.MaxAge | ||
| vcapCookie.Expires = sc.Expires | ||
| addCookieToResponse(response, vcapCookie, logger) | ||
| addCookieToResponse(response, createVcapMetaCookie(vcapCookie), logger) | ||
| } |
There was a problem hiding this comment.
Since you moved the addCookieToResponse call inside the loop, please get rid of the newVcapCookie function and just use struct literals to make this a bit more readable.
| for _, sc := range sessionCookies { | |
| vcapCookie := newVcapCookie() | |
| vcapCookie.Secure = vcapCookie.Secure || sc.Secure | |
| vcapCookie.SameSite = sc.SameSite | |
| vcapCookie.Partitioned = sc.Partitioned | |
| vcapCookie.MaxAge = sc.MaxAge | |
| vcapCookie.Expires = sc.Expires | |
| addCookieToResponse(response, vcapCookie, logger) | |
| addCookieToResponse(response, createVcapMetaCookie(vcapCookie), logger) | |
| } | |
| for _, sc := range sessionCookies { | |
| vcapCookie := &http.Cookie{ | |
| Secure: vcapCookie.Secure || sc.Secure | |
| // ... | |
| } | |
| addCookieToResponse(response, vcapCookie, logger) | |
| addCookieToResponse(response, createVcapMetaCookie(vcapCookie), logger) | |
| } |
There was a problem hiding this comment.
Hm, I don't think it becomes more readable when removing the function that creates the cookies with default values.
It'd become more repetitive however. In the staleSessionScenario, we'd first explicitly create the struct with default values, then conditionally set most of the parameters again.
| addCookieToResponse(response, createVcapMetaCookie(vcapCookie), logger) | ||
| } | ||
| } else if staleSessionScenario { | ||
| vcapCookie := newVcapCookie() |
| sessionCookies = append(sessionCookies, cookie) | ||
| } | ||
| } | ||
| return |
There was a problem hiding this comment.
It has been like this before, but please add the variables to the return statement.
c6c95d5 to
23d951f
Compare
23d951f to
e71b49b
Compare
a18e
left a comment
There was a problem hiding this comment.
LGTM after a round of internal review/adjustments
| for _, sc := range sessionCookies { | ||
| vcapCookie := newVcapCookie() | ||
| vcapCookie.Secure = vcapCookie.Secure || sc.Secure | ||
| vcapCookie.SameSite = sc.SameSite | ||
| vcapCookie.Partitioned = sc.Partitioned | ||
| vcapCookie.MaxAge = sc.MaxAge | ||
| vcapCookie.Expires = sc.Expires | ||
| addCookieToResponse(response, vcapCookie, logger) | ||
| addCookieToResponse(response, createVcapMetaCookie(vcapCookie), logger) | ||
| } |
Summary
Fixes Problem 2 from #529.
During a CHIPS migration, an application may set multiple session cookies with the same name in a single response — typically a new partitioned
JSESSIONIDalongside a non-partitioned delete (MaxAge=-1). Previously, gorouter only translated the first matching session cookie into a__VCAP_ID__+__VCAP_ID_META__pair, silently dropping the second. This broke the migration because the browser never received the corresponding__VCAP_ID__delete cookie for the non-partitioned variant (or vice-versa).This change makes
getSessionCookiescollect all matching session cookies from the response and emits a__VCAP_ID__+__VCAP_ID_META__pair for each one, preserving per-cookie attributes (Partitioned,SameSite,MaxAge,Expires,Secure).Backward Compatibility
Breaking Change? No
AI Usage
The work was done with the assistance of Claude (claude-opus-4-6, Anthropic) via claude code.