forward proxy: ip host headers by mattklein123 · Pull Request #7565 · envoyproxy/envoy

mattklein123 · 2019-07-12T22:53:17Z

Support IP addresses in host headers.

This completes the MVP of dynamic forward proxy.

Risk Level: Low
Testing: Unit and integration tests
Docs Changes: N/A
Release Notes: N/A

Support IP addresses in host headers. This completes the MVP of dynamic forward proxy. Fixes #1606 Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2019-07-12T22:54:32Z

@lizan @PiotrSikora PTAL I'm sure I got something wrong here either on the TLS or spec side. cc @alyssawilk

alyssawilk

Looks good! (modulo defering TLS magic to TLS folks =P)

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

source/extensions/transport_sockets/tls/context_impl.cc

source/extensions/clusters/dynamic_forward_proxy/cluster.cc

Signed-off-by: Matt Klein <mklein@lyft.com>

source/extensions/transport_sockets/tls/context_impl.cc

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

source/extensions/clusters/dynamic_forward_proxy/cluster.cc

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2019-07-16T18:04:46Z

@alyssawilk @lizan @PiotrSikora updated per comments.

Signed-off-by: Matt Klein <mklein@lyft.com>

lizan

Can you merge master to pick #7608?

PiotrSikora

Hmm, I was sure that I saw it before, but I cannot find it now... Could you add a test that verifies that using IP address in "verify_subject_alt_name" works against peer with "upstreamlocalhostcert.pem"?

mattklein123 · 2019-07-18T17:03:10Z

Hmm, I was sure that I saw it before, but I cannot find it now... Could you add a test that verifies that using IP address in "verify_subject_alt_name" works against peer with "upstreamlocalhostcert.pem"?

Will do.

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2019-07-19T04:30:38Z

@PiotrSikora updated with the test you asked for.

test/extensions/transport_sockets/tls/ssl_socket_test.cc

source/extensions/transport_sockets/tls/context_impl.cc

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2019-07-20T21:02:46Z

/retest

repokitteh-read-only · 2019-07-20T21:02:50Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #7565 (comment) was created by @mattklein123.

see: more, trace.

PiotrSikora

LGTM, sans the explicit IPv4 and IPv6 tests.

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2019-07-21T18:12:06Z

@PiotrSikora @lizan I pushed an update per your comments, but I'm seeing what I think is a persistent flake in coverage which I don't think is due to my changes:

[ RUN      ] IpVersions/SslReadBufferLimitTest.SomeLimit/IPv4
test/extensions/transport_sockets/tls/ssl_socket_test.cc:3854: Failure
Expected: (expected_chunk_size) >= (data.length()), actual: 32768 vs 65440
test/extensions/transport_sockets/tls/ssl_socket_test.cc:3854: Failure
Expected: (expected_chunk_size) >= (data.length()), actual: 32768 vs 65440
test/extensions/transport_sockets/tls/ssl_socket_test.cc:3854: Failure
Expected: (expected_chunk_size) >= (data.length()), actual: 32768 vs 65440
test/extensions/transport_sockets/tls/ssl_socket_test.cc:3854: Failure
Expected: (expected_chunk_size) >= (data.length()), actual: 32768 vs 65440
[  FAILED  ] IpVersions/SslReadBufferLimitTest.SomeLimit/IPv4, where GetParam() = 4-byte object <00-00 00-00> (28 ms)

I will take a look tonight or tomorrow but LMK if you have any thoughts. I'm assuming it's somehow due to the sharding we now have.

test/extensions/transport_sockets/tls/ssl_socket_test.cc

Signed-off-by: Matt Klein <mklein@lyft.com>

PiotrSikora · 2019-07-21T18:26:42Z

I don't see anything in the code that should break that test.

forward proxy: ip host headers

56d6ee2

Support IP addresses in host headers. This completes the MVP of dynamic forward proxy. Fixes #1606 Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 requested a review from alyssawilk as a code owner July 12, 2019 22:53

mattklein123 assigned lizan, PiotrSikora and alyssawilk Jul 12, 2019

alyssawilk reviewed Jul 15, 2019

View reviewed changes

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc Show resolved Hide resolved

source/extensions/transport_sockets/tls/context_impl.cc Show resolved Hide resolved

source/extensions/clusters/dynamic_forward_proxy/cluster.cc Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into ip_host_headers

698a924

Signed-off-by: Matt Klein <mklein@lyft.com>

PiotrSikora reviewed Jul 15, 2019

View reviewed changes

mattklein123 added 5 commits July 15, 2019 16:35

Merge remote-tracking branch 'origin/master' into ip_host_headers

7da7546

Signed-off-by: Matt Klein <mklein@lyft.com>

Merge remote-tracking branch 'origin/master' into ip_host_headers

a2d50fc

Signed-off-by: Matt Klein <mklein@lyft.com>

comment

c581b8b

Signed-off-by: Matt Klein <mklein@lyft.com>

Merge remote-tracking branch 'origin/master' into ip_host_headers

8581f66

Signed-off-by: Matt Klein <mklein@lyft.com>

fix

f35e9aa

Signed-off-by: Matt Klein <mklein@lyft.com>

Merge remote-tracking branch 'origin/master' into ip_host_headers

143b7f9

Signed-off-by: Matt Klein <mklein@lyft.com>

alyssawilk previously approved these changes Jul 17, 2019

View reviewed changes

lizan previously approved these changes Jul 17, 2019

View reviewed changes

PiotrSikora reviewed Jul 17, 2019

View reviewed changes

mattklein123 added 2 commits July 18, 2019 20:39

Merge remote-tracking branch 'origin/master' into ip_host_headers

a4255d3

Signed-off-by: Matt Klein <mklein@lyft.com>

comment

da94cde

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 dismissed stale reviews from lizan and alyssawilk via da94cde July 19, 2019 04:10

Merge remote-tracking branch 'origin/master' into ip_host_headers

3e45358

Signed-off-by: Matt Klein <mklein@lyft.com>

lizan reviewed Jul 19, 2019

View reviewed changes

test/extensions/transport_sockets/tls/ssl_socket_test.cc Outdated Show resolved Hide resolved

lizan reviewed Jul 19, 2019

View reviewed changes

test/extensions/transport_sockets/tls/ssl_socket_test.cc Outdated Show resolved Hide resolved

lizan reviewed Jul 19, 2019

View reviewed changes

source/extensions/transport_sockets/tls/context_impl.cc Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into ip_host_headers

23e77da

Signed-off-by: Matt Klein <mklein@lyft.com>

comments

f8c9f31

Signed-off-by: Matt Klein <mklein@lyft.com>

PiotrSikora previously approved these changes Jul 21, 2019

View reviewed changes

comment

27dbe14

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 dismissed PiotrSikora’s stale review via 27dbe14 July 21, 2019 18:09

PiotrSikora previously approved these changes Jul 21, 2019

View reviewed changes

test/extensions/transport_sockets/tls/ssl_socket_test.cc Outdated Show resolved Hide resolved

test/extensions/transport_sockets/tls/ssl_socket_test.cc Outdated Show resolved Hide resolved

comment

c6b8704

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 dismissed PiotrSikora’s stale review via c6b8704 July 21, 2019 18:17

PiotrSikora approved these changes Jul 21, 2019

View reviewed changes

lizan approved these changes Jul 21, 2019

View reviewed changes

mattklein123 merged commit 81654d1 into master Jul 22, 2019

mattklein123 deleted the ip_host_headers branch July 22, 2019 02:13

Conversation

mattklein123 commented Jul 12, 2019

Uh oh!

mattklein123 commented Jul 12, 2019

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 commented Jul 16, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jul 18, 2019

Uh oh!

mattklein123 commented Jul 19, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 commented Jul 20, 2019

Uh oh!

repokitteh-read-only bot commented Jul 20, 2019

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jul 21, 2019

Uh oh!

Uh oh!

Uh oh!

PiotrSikora commented Jul 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants