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

router: on shadowing, add "-shadow" to host, not after port #2600

Merged
merged 9 commits into from Feb 20, 2018

Conversation

Projects
None yet
4 participants
@dio
Member

dio commented Feb 14, 2018

router: on shadowing, add "-shadow" to host, not after port

Description:
This patch makes sure to add -shadow postfix to host when having host:port as Host.

Risk Level: Low

Testing:
unit test

Docs Changes:
N/A

Release Notes:
N/A

Fixes #2595

dio added some commits Feb 13, 2018

Split port from host then append -shadow
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Use std::forward_as_tuple
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Do splitToken only when it has :
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Move comment
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Add test for host with port
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@@ -59,5 +59,48 @@ TEST(ShadowWriterImplTest, All) {
callback->onFailure(Http::AsyncClient::FailureReason::Reset);
}
TEST(ShadowWriterImplTest, AllWithHostAndPort) {

This comment has been minimized.

@ccaraman

ccaraman Feb 15, 2018

Contributor

Since most of this is a duplicate of the first test, can you refactor it to have the host values be passed in as a parameter?

This comment has been minimized.

@dio

dio Feb 16, 2018

Member

Yes, thanks for suggesting this.

ASSERT(parts.size() == 2);
request->headers().Host()->value(absl::StrJoin(parts, "-shadow:"));
} else {
request->headers().Host()->value(absl::StrJoin(

This comment has been minimized.

@ccaraman

ccaraman Feb 15, 2018

Contributor

absl::StrCat() would work here instead of requiring forward_as_tuple()

This comment has been minimized.

@dio

dio Feb 16, 2018

Member

Have it fixed in 7d7e613. While I'm still not sure about the assertions.

ASSERT(!host.empty());
host += "-shadow";
request->headers().Host()->value(host);
if (absl::StrContains(request->headers().Host()->value().c_str(), ":")) {

This comment has been minimized.

@ccaraman

ccaraman Feb 15, 2018

Contributor

nit: a away to rework this is to call split token then have an if on size == 2 do the StrJoin() as it is in line 23 and the else statement will the have the assert for size ==1 and use StrCat()

This comment has been minimized.

@dio

dio Feb 16, 2018

Member

Yes, this is actually my first approach on this. But had second thought hehe. Please check 7d7e613 if that's OK.

@ccaraman

Thank you for the fix! A few comments.

namespace Envoy {
namespace Router {
void ShadowWriterImpl::shadow(const std::string& cluster, Http::MessagePtr&& request,
std::chrono::milliseconds timeout) {
ASSERT(request->headers().Host()->value().c_str() != "");

This comment has been minimized.

@ccaraman

ccaraman Feb 15, 2018

Contributor

Checking for empty works too

dio added some commits Feb 15, 2018

Merge remote-tracking branch 'upstream/master' into split-port
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Refactor test
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Refactor shadow writer implementation
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
request->headers().Host()->value(host);
ASSERT(!request->headers().Host()->value().empty());
// Switch authority to add a shadow postfix. This allows upstream logging to make a more

This comment has been minimized.

@ccaraman

ccaraman Feb 16, 2018

Contributor

nit: Do you mind fixing the second sentence to not have 'a' in 'to make a more'.

This comment has been minimized.

@dio

dio Feb 16, 2018

Member

👍

Remove 'a'
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@alyssawilk

Excellent change - thanks for the fix!

@alyssawilk alyssawilk merged commit e28e443 into envoyproxy:master Feb 20, 2018

10 checks passed

DCO All commits have a DCO sign-off from the author
ci/circleci: asan Your tests passed on CircleCI!
Details
ci/circleci: build_image Your tests passed on CircleCI!
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: format Your tests passed on CircleCI!
Details
ci/circleci: ipv6_tests Your tests passed on CircleCI!
Details
ci/circleci: mac Your tests passed on CircleCI!
Details
ci/circleci: release Your tests passed on CircleCI!
Details
ci/circleci: tsan Your tests passed on CircleCI!
Details

alyssawilk added a commit that referenced this pull request Feb 27, 2018

docs: clarifying the merge process (#2674)
Documenting that the merge should retain DCO, as discussed on #2600

Risk Level: n/a
Testing: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment