Skip to content

[HTTP Proxy] Support CIDR blocks in no_proxy config#31119

Merged
yashykt merged 5 commits intogrpc:masterfrom
stanhu:sh-support-cidr-no-proxy
Mar 31, 2023
Merged

[HTTP Proxy] Support CIDR blocks in no_proxy config#31119
yashykt merged 5 commits intogrpc:masterfrom
stanhu:sh-support-cidr-no-proxy

Conversation

@stanhu
Copy link
Copy Markdown
Contributor

@stanhu stanhu commented Sep 24, 2022 •
edited
Loading

This commit adds support for using CIDR blocks defined in the no_proxy environment variable. For example:

http_proxy=http://localhost:8080 no_proxy=10.10.0.0/24

The example above would bypass the proxy if the server IP matched 10.10.0.0 - 10.10.0.255.

Closes #22681

@stanhu stanhu requested a review from markdroth as a code owner September 24, 2022 05:52
@stanhu stanhu mentioned this pull request Sep 25, 2022
Closed
@stanhu stanhu force-pushed the sh-support-cidr-no-proxy branch from 1151d0e to 80cd1a3 Compare September 26, 2022 17:33
stanhu
stanhu commented Sep 26, 2022
View reviewed changes
src/core/ext/filters/client_channel/http_proxy.cc Outdated
return false;
}

auto proxy_address = StringToSockaddr(cidr[0], 0);
Copy link
Copy Markdown
Contributor Author

@stanhu stanhu Sep 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to parse port numbers for exact IP matches, but this should be done outside of the CIDR comparison. Golang does this: https://github.com/golang/go/blob/f771edd7f92a47c276d65fbd9619e16a786c6746/src/vendor/golang.org/x/net/http/httpproxy/proxy.go#L38-L50

@stanhu stanhu force-pushed the sh-support-cidr-no-proxy branch 3 times, most recently from 4e45501 to 07d303b Compare September 26, 2022 18:38
@stanhu stanhu force-pushed the sh-support-cidr-no-proxy branch from 07d303b to bb1b3a4 Compare December 30, 2022 06:25
@stanhu
Copy link
Copy Markdown
Contributor Author

stanhu commented Dec 30, 2022

@yashykt Would you be able to review?

@yashykt
Copy link
Copy Markdown
Member

yashykt commented Feb 14, 2023

Sorry for the delay. I'll take a look at this this week.

yashykt
yashykt requested changes Mar 7, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@yashykt yashykt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay! 🙏

@stanhu stanhu force-pushed the sh-support-cidr-no-proxy branch 2 times, most recently from f0b631b to 708a168 Compare March 7, 2023 21:12
@stanhu
Support CIDR blocks in no_proxy config
051e7d6 
This commit adds support for using CIDR blocks defined in the
`no_proxy` environment variable. For example:

```
http_proxy=http://localhost:8080 no_proxy=10.10.0.0/24
```

The example above would bypass the proxy if the server
IP matched 10.10.0.0 - 10.10.0.255.

Closes grpc#22681
yashykt
yashykt reviewed Mar 7, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@yashykt yashykt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, could I request that you do not force-push/delete older commits that were reviewed. It would help maintain review history. The commits would be squashed at merge time anyway.

@stanhu stanhu force-pushed the sh-support-cidr-no-proxy branch from 708a168 to 051e7d6 Compare March 7, 2023 21:13
@stanhu
Copy link
Copy Markdown
Contributor Author

stanhu commented Mar 8, 2023

Also, could I request that you do not force-push/delete older commits that were reviewed. It would help maintain review history. The commits would be squashed at merge time anyway.

Sure, thing. I'm curious, though, how it maintains review history. The pull request still has the comments?

@yashykt
Copy link
Copy Markdown
Member

yashykt commented Mar 8, 2023 •
edited
Loading

Sure, thing. I'm curious, though, how it maintains review history. The pull request still has the comments?

If the previous commits remain, I can do a diff and see what changed from the previous commit, as opposed to having to look at the entire PR again.

@stanhu
Copy link
Copy Markdown
Contributor Author

stanhu commented Mar 8, 2023

Trying to reproduce this build failure. Am I just missing this?

diff --cc BUILD
index 4ec32ff966,4ec32ff966..9159917f7c
--- a/BUILD
+++ b/BUILD
@@@ -2756,10 -2756,10 +2756,12 @@@ grpc_cc_library
          "//src/core:ext/filters/client_channel/lb_policy/child_policy_handler.cc",
          "//src/core:ext/filters/client_channel/lb_policy/oob_backend_metric.cc",
          "//src/core:ext/filters/client_channel/local_subchannel_pool.cc",
++        "//src/core:ext/filters/client_channel/parse_address.cc",
          "//src/core:ext/filters/client_channel/retry_filter.cc",
          "//src/core:ext/filters/client_channel/retry_service_config.cc",
          "//src/core:ext/filters/client_channel/retry_throttle.cc",
          "//src/core:ext/filters/client_channel/service_config_channel_arg_filter.cc",
++        "//src/core:ext/filters/client_channel/sockaddr_utils.cc",
          "//src/core:ext/filters/client_channel/subchannel.cc",
          "//src/core:ext/filters/client_channel/subchannel_pool_interface.cc",
          "//src/core:ext/filters/client_channel/subchannel_stream_client.cc",
@@@ -2782,9 -2782,9 +2784,11 @@@
          "//src/core:ext/filters/client_channel/lb_policy/oob_backend_metric.h",
          "//src/core:ext/filters/client_channel/lb_policy/oob_backend_metric_internal.h",
          "//src/core:ext/filters/client_channel/local_subchannel_pool.h",
++        "//src/core:ext/filters/client_channel/parse_address.h",
          "//src/core:ext/filters/client_channel/retry_filter.h",
          "//src/core:ext/filters/client_channel/retry_service_config.h",
          "//src/core:ext/filters/client_channel/retry_throttle.h",
++        "//src/core:ext/filters/client_channel/sockaddr_utils.h",
          "//src/core:ext/filters/client_channel/subchannel.h",
          "//src/core:ext/filters/client_channel/subchannel_interface_internal.h",
          "//src/core:ext/filters/client_channel/subchannel_pool_interface.h",

@yashykt yashykt force-pushed the sh-support-cidr-no-proxy branch from 01f193d to 6aeb5b1 Compare March 8, 2023 22:56
@yashykt yashykt added the release notes: yes Indicates if PR needs to be in release notes label Mar 8, 2023
@yashykt
Copy link
Copy Markdown
Member

yashykt commented Mar 8, 2023

oops, looks like I overwrote your commit. My bad! But, I think that should fix the build issues.

@stanhu
Copy link
Copy Markdown
Contributor Author

stanhu commented Mar 8, 2023

Thanks! I wasn't able to reproduce the error locally with bazel build :all.

markdroth
markdroth reviewed Mar 22, 2023
View reviewed changes
markdroth
markdroth reviewed Mar 22, 2023
View reviewed changes
markdroth
markdroth approved these changes Mar 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've re-triggered the tests. @yashykt, I'll let you get this merged once everything is passing.

@yashykt
Copy link
Copy Markdown
Member

yashykt commented Mar 22, 2023 •
edited
Loading

@stanhu can you please run -

tools/distrib/iwyu.sh
tools/distrib/fix_build_deps.py
tools/distrib/clang_format_code.sh

@stanhu
Copy link
Copy Markdown
Contributor Author

stanhu commented Mar 23, 2023

@yashykt I think I did it; I found it odd that it appeared that the diffs changed between each script.

@yashykt yashykt changed the title Support CIDR blocks in no_proxy config [HTTP Proxy] Support CIDR blocks in no_proxy config Mar 31, 2023
@yashykt yashykt merged commit 4110dea into grpc:master Mar 31, 2023
@yashykt
Copy link
Copy Markdown
Member

yashykt commented Mar 31, 2023

Thanks for the contribution!

@yashykt yashykt mentioned this pull request Mar 31, 2023
Closed
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Mar 31, 2023
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
@stanhu @yashykt @XuanWang-Amos
[HTTP Proxy] Support CIDR blocks in no_proxy config (grpc#31119)
609570c 
This commit adds support for using CIDR blocks defined in the `no_proxy`
environment variable. For example:

```
http_proxy=http://localhost:8080 no_proxy=10.10.0.0/24
```

The example above would bypass the proxy if the server IP matched
10.10.0.0 - 10.10.0.255.

Closes grpc#22681

---------

Co-authored-by: Yash Tibrewal <yashkt@google.com>
wanlin31 pushed a commit that referenced this pull request May 18, 2023
@stanhu @yashykt @wanlin31
[HTTP Proxy] Support CIDR blocks in no_proxy config (#31119)
9e66e64 
This commit adds support for using CIDR blocks defined in the `no_proxy`
environment variable. For example:

```
http_proxy=http://localhost:8080 no_proxy=10.10.0.0/24
```

The example above would bypass the proxy if the server IP matched
10.10.0.0 - 10.10.0.255.

Closes #22681

---------

Co-authored-by: Yash Tibrewal <yashkt@google.com>
@melkouri melkouri mentioned this pull request Jan 7, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yashykt yashykt yashykt approved these changes

@markdroth markdroth markdroth approved these changes

Assignees

@yashykt yashykt

Labels

bloat/low imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes title needs formatting

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

NO_PROXY handling doesn't support CIDR addresses

4 participants

@stanhu @yashykt @markdroth @grpc-kokoro