fix: support CIDR blocks in no_proxy env variable #2876
Conversation
packages/grpc-js/src/http_proxy.ts
Outdated
| for (const host of getNoProxyHostList()) { | ||
| const parsedCIDR = parseCIDR(host); | ||
| // host is a single IP address or a CIDR notation or a domain | ||
| if (parsedCIDR && isIpInCIDR(parsedCIDR, serverHost) || serverHost.endsWith(host)) { |
There was a problem hiding this comment.
The case where a non-IP serverHost string is passed to isIpInCIDR is not explicitly handled, making the result unclear. Either this line should avoid making that call, or isIpInCIDR should short-circuit return false if the serverHost is not an IP address, or ipToInt should return an error value if ip is not an IP address and isIpInCIDR should check for that value.
packages/grpc-js/src/http_proxy.ts
Outdated
| for (const host of getNoProxyHostList()) { | ||
| const parsedCIDR = parseCIDR(host); | ||
| // host is a single IP address or a CIDR notation or a domain | ||
| if (parsedCIDR && isIpInCIDR(parsedCIDR, serverHost) || serverHost.endsWith(host)) { |
There was a problem hiding this comment.
The original logic uses host === serverHost to check for matches. That has been replaced here with serverHost.endsWith(host). That change is not related to supporting CIDR ranges, so it should be reverted.
There was a problem hiding this comment.
The original logic where we handle single IPs is handled by isIpInCIDR(parsedCIDR, serverHost) as this latter transforms single IPs to CIDR notations (here).
This serverHost.endsWith(host) was to support the case where we include domain names/suffixes in the NO_PROXY (for example NO_PROXY=example.com,.local.,.local.).
There was a problem hiding this comment.
This case wasn't stated in the issue, I'll update it and make sure it's well handled in the code. Thank you for raising this 🙇
|
Thank you @murgatroid99 for your review, I made some changes accordingly to your comments. I hope it's clearer 🙂 |
packages/grpc-js/src/http_proxy.ts
Outdated
| const parsedCIDR = parseCIDR(host); | ||
| // host is a single IP or a domain name suffix | ||
| if (!parsedCIDR) { | ||
| if (host === serverHost || serverHost.includes(host)) { |
There was a problem hiding this comment.
I checked and I see that other implementations support suffix matching, so I am OK with keeping the endsWith check here, but includes is definitely not correct. The equality check is also redundant with the substring check, so we should not have both.
|
Thank you for the reviews! |
Issue created #2878
Issue:
The grpc-js lib doesn't support having CIDR blocks in the NO_PROXY variable (like 172.16.0.0/12
192.168.0.0/16). It only checks if the address we target is in the list of IPs in the NO_PROXY. Hence, it supposes that NO_PROXY is a list of single IPs.
For example if the NO_PROXY contains 192.168.0.0/16 and the IP you're trying to reach is 192.168.0.10. This latter is NOT considered in the range of the NO_PROXY because the lib just compares the IP to the elements of the NO_PROXY list.
We also want to support the case where we whitelist domain name suffixes in the NO_PROXY (like example.com, .internal., .local. ).
What is done in this PR: