Skip to content

Support multiple JSESSIONID cookies for CHIPS migration#546

Merged
hoffmaen merged 1 commit intocloudfoundry:developfrom
sap-contributions:chips-migration
Mar 31, 2026
Merged

Support multiple JSESSIONID cookies for CHIPS migration#546
hoffmaen merged 1 commit intocloudfoundry:developfrom
sap-contributions:chips-migration

Conversation

@hoffmaen
Copy link
Copy Markdown
Contributor

@hoffmaen hoffmaen commented Mar 26, 2026

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 JSESSIONID alongside 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 getSessionCookies collect 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.

Comment on lines +561 to +570
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)
}
Copy link
Copy Markdown
Member

@maxmoehl maxmoehl Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with @hoffmaen on this one.

addCookieToResponse(response, createVcapMetaCookie(vcapCookie), logger)
}
} else if staleSessionScenario {
vcapCookie := newVcapCookie()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Comment thread src/code.cloudfoundry.org/gorouter/proxy/round_tripper/proxy_round_tripper.go Outdated
sessionCookies = append(sessionCookies, cookie)
}
}
return
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been like this before, but please add the variables to the return statement.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@hoffmaen hoffmaen force-pushed the chips-migration branch 2 times, most recently from c6c95d5 to 23d951f Compare March 31, 2026 11:56
Copy link
Copy Markdown
Contributor

@a18e a18e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after a round of internal review/adjustments

Comment on lines +561 to +570
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)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with @hoffmaen on this one.

@github-project-automation github-project-automation bot moved this from Inbox to Pending Merge | Prioritized in Application Runtime Platform Working Group Mar 31, 2026
@hoffmaen hoffmaen merged commit 82b744c into cloudfoundry:develop Mar 31, 2026
1 check passed
@github-project-automation github-project-automation bot moved this from Pending Merge | Prioritized to Done in Application Runtime Platform Working Group Mar 31, 2026
@hoffmaen hoffmaen deleted the chips-migration branch March 31, 2026 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants