Skip to content
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

Merged
merged 2 commits into from
May 23, 2018

Conversation

akonradi
Copy link
Contributor

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.

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>
@htuch htuch self-assigned this May 22, 2018
set_cookies.insert(value);
EXPECT_THAT(value, MatchesRegex("^foo=.*$"));
});
EXPECT_EQ(set_cookies.size(), 1);
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

htuch
htuch previously approved these changes May 22, 2018
Copy link
Member

@htuch htuch left a 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?

mattklein123
mattklein123 previously approved these changes May 22, 2018
Copy link
Member

@mattklein123 mattklein123 left a 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.

ttl = std::chrono::seconds(hash_policy.cookie().ttl().seconds());
}
hash_impls_.emplace_back(new CookieHashMethod(hash_policy.cookie().name(), ttl));
} break;
Copy link
Member

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>
@akonradi akonradi dismissed stale reviews from mattklein123 and htuch via 5ff8c3c May 23, 2018 00:14
@htuch htuch merged commit 396f52d into envoyproxy:master May 23, 2018
alyssawilk added a commit that referenced this pull request Jun 5, 2018
Risk Level: Low
Testing: new unit tests
Docs Changes: config documented inline.
Release Notes: added in #3462
Fixed #3532

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@akonradi akonradi deleted the generate-session-cookie branch September 6, 2018 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants