[Proxy] Support for setting proxy for addresses #34617

eugeneo · 2023-10-06T22:26:30Z

Fixes #33518.

src/core/ext/filters/client_channel/http_proxy_mapper.cc

eugeneo · 2023-10-11T21:45:57Z

Ready for review. Does not have a second variable (one for ranges of included addresses)

Automated fix for refs/heads/tasks/outgoing-proxy

markdroth

This looks like the right general approach.

Please let me know if you have any questions. Thanks!

src/core/ext/filters/client_channel/http_proxy_mapper.cc

src/core/ext/filters/client_channel/http_proxy_mapper.h

include/grpc/impl/channel_arg_names.h

src/core/ext/filters/client_channel/http_proxy_mapper.cc

test/core/client_channel/http_proxy_mapper_test.cc

include/grpc/impl/channel_arg_names.h

test/core/client_channel/http_proxy_mapper_test.cc

eugeneo

Thank you for the review. I addressed the comments.

src/core/ext/filters/client_channel/http_proxy_mapper.cc

include/grpc/impl/channel_arg_names.h

src/core/ext/filters/client_channel/http_proxy_mapper.cc

src/core/ext/filters/client_channel/http_proxy_mapper.h

test/core/client_channel/http_proxy_mapper_test.cc

include/grpc/impl/channel_arg_names.h

…include-addresses

eugeneo

Thank you for the review. I implemented environment variable and channel argument for including addresses.

src/core/ext/filters/client_channel/http_proxy_mapper.cc

include/grpc/impl/channel_arg_names.h

markdroth

This looks good overall! Comments are all minor.

Please let me know if you have any questions. Thanks!

src/core/ext/filters/client_channel/http_proxy_mapper.cc

markdroth · 2023-10-18T16:21:04Z

test/core/client_channel/http_proxy_mapper_test.cc

@@ -32,12 +36,55 @@
 #include "test/core/util/scoped_env_var.h"
 #include "test/core/util/test_config.h"

+// Need to be in the same namespace as absl::optional...
+namespace absl {


It's not okay to put code into the absl namespace, since we don't own that namespace.

Instead, I suggest you change the matcher to explictly print whatever you want. For example, you can replace line 85 with the following:

if (*address_string != address) { *result_listener << "Actual address: " << *address_string; return false; } return true;

Alternatively, you can explicitly print things in the individual tests via something like this:

EXPECT_THAT(address, AddressEq("1.2.3.4")) << grpc_sockaddr_to_string(address, true).value_or("unknown");

I removed these function, but I would like to suggest considering building a library of functions as it would be a significant boost to productivity. E.g. we could consider moving AddressEq matcher to a shared header.

I'd be fine with having a common library of matchers. We already have a similar one here:

grpc/test/core/resolver/endpoint_addresses_test.cc

Line 48 in a0c1027

MATCHER_P(EqualsAddress, address_str, "") {

Ack. Will do some code search in our code base at a later point and see if we can consolidate/reuse matchers.

test/core/client_channel/http_proxy_mapper_test.cc

eugeneo

Thank you for the review. Please take another look.

test/core/client_channel/http_proxy_mapper_test.cc

eugeneo · 2023-10-18T17:18:48Z

test/core/client_channel/http_proxy_mapper_test.cc

@@ -32,12 +36,55 @@
 #include "test/core/util/scoped_env_var.h"
 #include "test/core/util/test_config.h"

+// Need to be in the same namespace as absl::optional...
+namespace absl {


I removed these function, but I would like to suggest considering building a library of functions as it would be a significant boost to productivity. E.g. we could consider moving AddressEq matcher to a shared header.

src/core/ext/filters/client_channel/http_proxy_mapper.cc

eugeneo · 2023-10-18T17:35:40Z

src/core/ext/filters/client_channel/http_proxy_mapper.cc

                                      mask_bits);
  }
  return false;
 }

+bool AddressIncluded(
+    const absl::StatusOr<grpc_resolved_address>& target_address,


Does not work, absl::nullopt_t can not be converted to grpc_resolved_address:

external/com_google_absl/absl/status/statusor.h:779:10: error: no viable conversion from returned value of type 'const absl::nullopt_t' to function return type 'grpc_resolved_address'

I think there's no harm in leaving StatusOr.

src/core/ext/filters/client_channel/http_proxy_mapper.cc

Automated fix for refs/heads/tasks/outgoing-proxy

markdroth

Only minor comments remaining, feel free to merge after addressing. Thanks!

markdroth · 2023-10-18T22:04:50Z

src/core/ext/filters/client_channel/http_proxy_mapper.cc

+    absl::string_view host_name, absl::string_view addresses_and_subnets) {
+  for (absl::string_view entry :
+       absl::StrSplit(addresses_and_subnets, ',', absl::SkipEmpty())) {
+    if (ExactMatchOrSubdomain(host_name, entry) ||


I think we should call absl::StripAsciiWhitespace() just once here, rather than doing it separately in each of ExactMatchOrSubdomain() and ServerInCIDRRange().

markdroth · 2023-10-18T22:10:55Z

src/core/ext/filters/client_channel/http_proxy_mapper.cc

                                      mask_bits);
  }
  return false;
 }

+bool AddressIncluded(
+    const absl::StatusOr<grpc_resolved_address>& target_address,


I think using StatusOr<> seems like a very strange API for this function, since the function doesn't need the status; it just needs to know if there's an address that it should check for a CIDR range.

Same thing for ServerInCIDRRange() -- I think that function should go back to taking const grpc_resolved_address&, and the caller should not call it in the first place if there was no address to check.

Even if .value_or() doesn't work in this case, it's not hard for the caller to deal with this:

absl::StatusOr<grpc_resolved_address> address = StringToSockaddr(server_host, 0); absl::optional<grpc_resolved_address> addr; if (address.has_value()) addr = *address;

markdroth · 2023-10-18T22:14:21Z

test/core/client_channel/http_proxy_mapper_test.cc

@@ -32,12 +36,55 @@
 #include "test/core/util/scoped_env_var.h"
 #include "test/core/util/test_config.h"

+// Need to be in the same namespace as absl::optional...
+namespace absl {


I'd be fine with having a common library of matchers. We already have a similar one here:

grpc/test/core/resolver/endpoint_addresses_test.cc

Line 48 in a0c1027

MATCHER_P(EqualsAddress, address_str, "") {

eugeneo

Thank you. Updated.

eugeneo · 2023-10-18T22:58:40Z

src/core/ext/filters/client_channel/http_proxy_mapper.cc

+    absl::string_view host_name, absl::string_view addresses_and_subnets) {
+  for (absl::string_view entry :
+       absl::StrSplit(addresses_and_subnets, ',', absl::SkipEmpty())) {
+    if (ExactMatchOrSubdomain(host_name, entry) ||


eugeneo · 2023-10-18T23:08:16Z

src/core/ext/filters/client_channel/http_proxy_mapper.cc

                                      mask_bits);
  }
  return false;
 }

+bool AddressIncluded(
+    const absl::StatusOr<grpc_resolved_address>& target_address,


eugeneo · 2023-10-18T23:09:31Z

test/core/client_channel/http_proxy_mapper_test.cc

@@ -32,12 +36,55 @@
 #include "test/core/util/scoped_env_var.h"
 #include "test/core/util/test_config.h"

+// Need to be in the same namespace as absl::optional...
+namespace absl {


Ack. Will do some code search in our code base at a later point and see if we can consolidate/reuse matchers.

eugeneo added the release notes: yes Indicates if PR needs to be in release notes label Oct 6, 2023

eugeneo assigned markdroth Oct 6, 2023

github-actions bot added the lang/core label Oct 6, 2023

grpc-checks bot added per-call-memory/neutral per-channel-memory/neutral bloat/none labels Oct 6, 2023

eugeneo added the kokoro:force-run label Oct 9, 2023

grpc-kokoro removed the kokoro:force-run label Oct 9, 2023

eugeneo commented Oct 10, 2023

View reviewed changes

src/core/ext/filters/client_channel/http_proxy_mapper.cc Outdated Show resolved Hide resolved

[proxy] Allow proxying service addresses

b788d69

eugeneo force-pushed the tasks/outgoing-proxy branch from 31130de to b788d69 Compare October 11, 2023 21:42

grpc-checks bot added bloat/low and removed bloat/none labels Oct 11, 2023

eugeneo and others added 3 commits October 11, 2023 22:39

Automated change: Fix sanity tests

f81b024

fixup: progect gen

5e0ed3e

Merge pull request #64 from eugeneo/create-pull-request/patch-b788d69

a1f2c1f

Automated fix for refs/heads/tasks/outgoing-proxy

eugeneo marked this pull request as ready for review October 12, 2023 16:44

eugeneo requested a review from markdroth as a code owner October 12, 2023 16:44

markdroth reviewed Oct 12, 2023

View reviewed changes

fixup: code review

d49f816

eugeneo commented Oct 12, 2023

View reviewed changes

grpc-checks bot added bloat/none and removed bloat/low labels Oct 12, 2023

eugeneo added 6 commits October 12, 2023 16:19

fixup: iwyu

e5a096e

fixup: clang tidy

14a899c

Merge remote-tracking branch 'grpc/master' into tasks/outgoing-proxy-…

bb66dd3

…include-addresses

[proxy] Require explicit enable for address proxy.

c86e58d

fixup: add env var and channel arg for proxied addresses

a48c132

fixup: code review comments

55068b4

eugeneo commented Oct 17, 2023

View reviewed changes

src/core/ext/filters/client_channel/http_proxy_mapper.cc Outdated Show resolved Hide resolved

include/grpc/impl/channel_arg_names.h Outdated Show resolved Hide resolved

fixup: sanity

ec90cdd

grpc-checks bot added bloat/low and removed bloat/none labels Oct 17, 2023

eugeneo added 2 commits October 17, 2023 16:38

fixup: header

986a2a3

fixup: IWYU

3c28e57

markdroth reviewed Oct 18, 2023

View reviewed changes

fixup: code review comments

72d9dc6

eugeneo commented Oct 18, 2023

View reviewed changes

grpc-checks bot added bloat/none and removed bloat/low labels Oct 18, 2023

eugeneo and others added 2 commits October 18, 2023 19:16

Automated change: Fix sanity tests

6e6df3d

Merge pull request #67 from eugeneo/create-pull-request/patch-72d9dc6

8abdf31

Automated fix for refs/heads/tasks/outgoing-proxy

markdroth approved these changes Oct 18, 2023

View reviewed changes

Fixup: code review comments

556262a

eugeneo commented Oct 18, 2023

View reviewed changes

eugeneo enabled auto-merge (squash) October 18, 2023 23:11

eugeneo merged commit 1a76e7c into grpc:master Oct 18, 2023
53 of 70 checks passed

eugeneo deleted the tasks/outgoing-proxy branch October 19, 2023 16:32

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Oct 19, 2023

markdroth mentioned this pull request Oct 19, 2023

Add support for HTTP CONNECT for L7 load balancing #33518

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Proxy] Support for setting proxy for addresses #34617

[Proxy] Support for setting proxy for addresses #34617

eugeneo commented Oct 6, 2023 •

edited by markdroth

eugeneo commented Oct 11, 2023

markdroth left a comment

eugeneo left a comment

eugeneo left a comment

markdroth left a comment

markdroth Oct 18, 2023

eugeneo Oct 18, 2023

markdroth Oct 18, 2023

eugeneo Oct 18, 2023

eugeneo left a comment

eugeneo Oct 18, 2023

eugeneo Oct 18, 2023

markdroth left a comment

markdroth Oct 18, 2023

eugeneo Oct 18, 2023

markdroth Oct 18, 2023

markdroth Oct 18, 2023

eugeneo left a comment

eugeneo Oct 18, 2023

eugeneo Oct 18, 2023

eugeneo Oct 18, 2023

[Proxy] Support for setting proxy for addresses #34617

[Proxy] Support for setting proxy for addresses #34617

Conversation

eugeneo commented Oct 6, 2023 • edited by markdroth

eugeneo commented Oct 11, 2023

markdroth left a comment

Choose a reason for hiding this comment

eugeneo left a comment

Choose a reason for hiding this comment

eugeneo left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Oct 6, 2023 •

edited by markdroth