-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
routing: allow generating session cookies #3462
Conversation
This enables configuring Envoy to generate cookies that expire at the end of a session instead of requiring them to have an explicit max-age. Signed-off-by: Alex Konradi <akonradi@google.com>
48a711c
to
6d0ec4c
Compare
set_cookies.insert(value); | ||
EXPECT_THAT(value, MatchesRegex("^foo=.*$")); | ||
}); | ||
EXPECT_EQ(set_cookies.size(), 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want this to be a vector (or multiset) so you can validate the cardinality matches the number of requests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could change it to a std::map<string, size_t>
and increment the count for the key, but the sendMultipleRequests
helper already EXPECTs that each request is completed and calls this callback for each one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I think this comment can be ignored. Ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @envoyproxy/maintainers anyone else with thoughts on this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks. Small nit if you feel like it.
source/common/router/config_impl.cc
Outdated
ttl = std::chrono::seconds(hash_policy.cookie().ttl().seconds()); | ||
} | ||
hash_impls_.emplace_back(new CookieHashMethod(hash_policy.cookie().name(), ttl)); | ||
} break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: Can you put the break inside the '}' as is done elsewhere. I wish clang-format fixed this.
Signed-off-by: Alex Konradi <akonradi@google.com>
This enables configuring Envoy to generate cookies that expire at the
end of a session instead of requiring them to have an explicit max-age.
Risk Level: Low
Testing: added unit tests and an integration test
Docs Changes: documented new behavior in API and release docs
Release Notes: router: allow cookie routing to generate session cookies.