*: Correct gRFC A1 implementation #7881

eshitachandwani · 2024-11-29T16:55:03Z

Fixes: #7556

Remove environment variable detection during transport creation. Introduce a per-address attribute to specify the proxy CONNECT string. This change aligns with the xDS design and provides more control over proxy behavior.
Create a new resolver that handles proxy configuration and delegates to child resolvers for target and proxy address resolution.
Introduce a new DialOption to preserve the current behavior of grpc.Dial, where the resolved target address is used in the proxy CONNECT request. This option ensures backward compatibility for users relying on the existing behavior.

RELEASE NOTES:

Target resolution is no longer performed on the client when using default resolvers with grpc.NewClient in conjunction with proxy environment variables.
New WithTargetResolutionEnabled() dial option added to explicitly force target URI resolution on the client.
When non-default resolvers are used alongside proxy environment variables, target resolution is now performed on the client.

codecov · 2024-11-29T16:59:56Z

Codecov Report

Attention: Patch coverage is 77.77778% with 30 lines in your changes missing coverage. Please review.

Project coverage is 82.11%. Comparing base (724f450) to head (ef927ce).

Files with missing lines	Patch %	Lines
internal/transport/proxy_utils.go	67.39%	23 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7881      +/-   ##
==========================================
+ Coverage   82.05%   82.11%   +0.06%     
==========================================
  Files         381      382       +1     
  Lines       38539    38628      +89     
==========================================
+ Hits        31622    31720      +98     
+ Misses       5602     5591      -11     
- Partials     1315     1317       +2

Files with missing lines	Coverage Δ
clientconn.go	`92.45% <100.00%> (-0.33%)`	⬇️
dialoptions.go	`90.80% <100.00%> (+1.82%)`	⬆️
.../resolver/delegatingresolver/delegatingresolver.go	`74.85% <100.00%> (+5.98%)`	⬆️
internal/transport/http2_client.go	`91.40% <100.00%> (+<0.01%)`	⬆️
internal/transport/proxy.go	`59.61% <100.00%> (-8.39%)`	⬇️
internal/transport/transport.go	`91.56% <ø> (+6.82%)`	⬆️
resolver_wrapper.go	`84.16% <100.00%> (+1.28%)`	⬆️
internal/transport/proxy_utils.go	`67.39% <67.39%> (ø)`

... and 24 files with indirect coverage changes

purnesh42H · 2024-12-03T04:42:29Z

internal/testutils/proxy.go

+}
+
+// NewProxyServer create and starts a proxy server.
+func NewProxyServer(lis net.Listener, requestCheck func(*http.Request) error, errCh chan error, doneCh chan struct{}, backendAddr string, resolutionOnClient bool, proxyServerStarted func()) *ProxyServer {


may be we can do something like this fake server in its own package

grpc-go/internal/testutils/xds/fakeserver/server.go

Line 19 in 17d08f7

// Package fakeserver provides a fake implementation of the management server.

? wdyt?

New should only create the object and StartServer/Run should start the go routine to accept requests

purnesh42H · 2024-12-03T04:43:58Z

internal/transport/http2_client.go

 	address := addr.Addr
+
+	//if the ProxyConnectAddr is set in the aattribute, do a proxy dial.


typo: attribute

purnesh42H · 2024-12-03T04:45:21Z

internal/transport/proxy.go

-		p, _ := t.Password()
+	if user := attributes.User(address); user != nil {
+		u := user.Username()
+		p, _ := user.Password()


hmmm...we should not ignore the bool. What happens if password is not set?

It will append an empty string with the username. I have referenced it from the earlier code itself.

then user.Password() should just return empty string? it looks weird to ignore or atleast we should log a warning. So, if password is empty will the connection still succeed or we should do early failure?

From what I understand, it will still succeed.

purnesh42H · 2024-12-03T04:46:30Z

internal/transport/proxy.go

-func proxyDial(ctx context.Context, addr string, grpcUA string) (net.Conn, error) {
-	newAddr := addr
-	proxyURL, err := mapAddress(addr)
+// proxyDial dials, connecting to a proxy first if necessary. Dials, does the


we can remove the necessary part now? It will always dial proxy if called?

purnesh42H · 2024-12-03T05:42:04Z

test/proxy_test.go

+}
+
+// TestGrpcDialWithProxy tests grpc.Dial using a proxy and default
+// resolver in the target URI.and verifies that it connects to the proxy server


typo space and remove period after target URI

test/proxy_test.go

purnesh42H · 2024-12-05T09:06:07Z

internal/proxyattributes/proxyattributes_test.go

+	addr := resolver.Address{
+		Addr: "test-address",
+	}
+


nix new line

purnesh42H · 2024-12-05T09:06:13Z

internal/proxyattributes/proxyattributes_test.go

+		Addr:       "test-address",
+		Attributes: attributes.New(userAndConnectAddrKey, attr{user: user, addr: ""}),
+	}
+


nix new line

purnesh42H · 2024-12-05T09:06:26Z

internal/proxyattributes/proxyattributes_test.go

+		Addr:       "test-address",
+		Attributes: attributes.New(userAndConnectAddrKey, attr{user: nil, addr: "proxy-address"}),
+	}
+


nix new line

purnesh42H · 2024-12-05T09:23:27Z

test/proxy_test.go

+// verifies that the client connects to the proxy server, includes the resolved
+// target URI in the HTTP CONNECT request, and successfully establishes a
+// connection to the backend server.
+func (s) TestGrpcNewClientWithProxyAndCustomResolver(t *testing.T) {


nit: TestGRPC

purnesh42H · 2024-12-05T09:23:42Z

test/proxy_test.go

+// Tests grpc.NewClient with default i.e DNS resolver for targetURI and a proxy
+// and verifies that it connects to proxy server and sends unresolved target URI
+// in the HTTP CONNECT req and connects to backend.
+func (s) TestGrpcNewClientWithProxy(t *testing.T) {


nit: TestGRPC

purnesh42H · 2024-12-05T09:54:59Z

test/proxy_test.go

+
+// Tests grpc.NewClient with the default "dns" resolver and dial option
+// enabling target resolution on the client and verifies that the resolution
+// happens on client.


nit: mention the dial option WithTargetResolutionEnabled()

purnesh42H · 2024-12-05T09:56:06Z

test/proxy_test.go

+	case <-resolutionCh:
+		t.Logf("target resolution happened on client")
+	default:
+		t.Fatalf("Client-side resolution should be called but wasn't")


"target resolution did not happen on client" ?

purnesh42H · 2024-12-05T09:59:14Z

test/proxy_test.go

+
+// Tests grpc.NewClient with grpc.WithNoProxy() set and verifies that it does
+// not dail to proxy, but directly to backend.
+func (s) TestGrpcNewClientWithNoProxy(t *testing.T) {


for the NoProxy and CustomDialer case, should we still override overrideHTTPSProxyFromEnvironment? That will make sure that even though proxy env is set we should skip it because of NoProxy and CustomDialer or is that not intended?

If WithNoProxy or WithContextDialer are set, the HTTPSProxyFromEnviornment function will never be called, but we can still override and add error if it is being called. Is it a good practice to do so? @purnesh42H

yeah so right now if HTTPSProxyFromEnviornment is not present, it doesn't matter if WithNoProxy or WithContextDialer is set? The resolution will happen at client anyways? So having it present and not being called in these two cases is what we want to verify.

purnesh42H · 2024-12-05T10:00:10Z

test/proxy_test.go

+		}
+		wantProxyAuthStr := "Basic " + base64.StdEncoding.EncodeToString([]byte(user+":"+password))
+		if got := req.Header.Get("Proxy-Authorization"); got != wantProxyAuthStr {
+			gotDecoded, _ := base64.StdEncoding.DecodeString(got)


we should not ignore decoding errors. it should return error if decoding fails

purnesh42H · 2024-12-05T10:02:46Z

internal/transport/proxy.go

-		p, _ := t.Password()
+	if user := attributes.User(address); user != nil {
+		u := user.Username()
+		p, _ := user.Password()


then user.Password() should just return empty string? it looks weird to ignore or atleast we should log a warning. So, if password is empty will the connection still succeed or we should do early failure?

arjan-bal

I haven't reviewed the tests yet, will take a look at them next.

arjan-bal · 2024-12-23T12:16:00Z

clientconn.go

+	//
+	// WithTargetResolutionEnabled in `grpc.Dial` ensures that it preserves
+	// behavior: when default scheme passthrough is used, skip hostname
+	// resolution, when any other scheme like "dns" is used for resolution,


any other scheme like "dns"

From my understanding, WithTargetResolutionEnabled ONLY effects the dns resolver's behaviour.

https://github.com/grpc/grpc-go/blob/063d352de07403a582ef33f8f5f8149e3b57c47e/internal/resolver/delegatingresolver/delegatingresolver.go#L117C36-L117C59

If this is the case, we should mention this in the godoc of WithTargetResolutionEnabled and change this comment to say when "dns" is used. Comments should be precise and concise.

arjan-bal · 2024-12-23T12:17:14Z

clientconn.go

+	// WithTargetResolutionEnabled in `grpc.Dial` ensures that it preserves
+	// behavior: when default scheme passthrough is used, skip hostname
+	// resolution, when any other scheme like "dns" is used for resolution,
+	// perform resolution on the client as expected.


nit: Remove the the ending as expected, it doesn't seem to add any extra information.

clientconn.go

dialoptions.go

internal/transport/http2_client.go

arjan-bal · 2024-12-23T15:36:37Z

internal/transport/proxy_test.go

Is there a reason for moving proxy tests out of internal/transport? It's preferable to keep tests in separate packages as it allows tests to run in parallel.

It looks like these tests are deleted, not moved. We need these tests.

Whatever was being tested in the tests is being tested in the E2E tests, basic auth and connect requests and connecting to the proxy server. The mapAddress function was moved to delegating resolver and that is being tested in delegating resolver. Do we need to test is again in unit tests?

TestHTTPConnectWithServerHello is added to verify that the optimization added in #7424 doesn't effect correctness. It is difficult to test it in en e2e fashion because it needs the server to send the first message after creation of the TCP connection and the proxy to buffer this message along with the CONNECT response. I think the other tests can be replaced.

internal/testutils/proxy.go

arjan-bal · 2024-12-24T11:10:20Z

dialoptions.go

@@ -384,7 +384,8 @@ func WithNoProxy() DialOption {
 }

 // WithTargetResolutionEnabled returns a DialOption which enables target
-// resolution on client. This is ignored if WithNoProxy is used.
+// resolution on client even when "dns" scheme is used. This is ignored if


I think we should mention when a proxy is used along with the the "dns" scheme.

arjan-bal · 2024-12-24T11:18:09Z

internal/transport/proxy_test.go

TestHTTPConnectWithServerHello is added to verify that the optimization added in #7424 doesn't effect correctness. It is difficult to test it in en e2e fashion because it needs the server to send the first message after creation of the TCP connection and the proxy to buffer this message along with the CONNECT response. I think the other tests can be replaced.

arjan-bal · 2024-12-24T11:23:48Z

internal/transport/proxy.go

+		u := user.Username()
+		p, pSet := user.Password()
+		if !pSet && logger.V(2) {
+			logger.Warningf("password not set for basic authentication for proxy dialing")


Does this warrant a log? I believe the password is allowed to be empty in HTTP Basic auth.

arjan-bal · 2024-12-24T11:32:11Z

test/proxy_test.go

+}
+
+// Creates and starts a proxy server.
+func newProxyServer(lis net.Listener, reqCheck func(*http.Request) error, errCh chan error, doneCh chan struct{}, backendAddr string, resOnClient bool, proxyStarted func()) *proxyServer {


Can you move these tests back to the transport directory? It will be easier to review the delta in the proxy server code instead of the whole code.

arjan-bal · 2024-12-24T11:34:18Z

test/proxy_test.go

+		if resOnClient {
+			out, err = net.Dial("tcp", req.URL.Host)
+		} else {
+			out, err = net.Dial("tcp", backendAddr)
+		}


The proxy server should always connect to the address received in the request. The address in the request can be either an IP or a hostname depending on how the client is configured.

arjan-bal · 2024-12-24T11:48:32Z

test/proxy_test.go

+		// and server to client.
+		go io.Copy(p.in, p.out)
+		go io.Copy(p.out, p.in)
+		close(doneCh)


What is the intended use of doneCh here? The go routines for the proxy are running in the background when doneCh is closed. If the intention is to track the completion of the CONNECT request, we should rename the variable to something more appropriate like connectDone.

arjan-bal · 2024-12-24T11:54:25Z

test/proxy_test.go

+	}
+	t.Logf("Started TestService backend at: %q", backend.Address)
+	t.Cleanup(backend.Stop)
+	return backend.Address


This is always an ip:port. Instead, you could use testutils.ParsePort to get the port and return localhost:port from here. This would allow you to test if the proxy receives an IP address or host based on the test case.

arjan-bal · 2024-12-24T11:55:36Z

test/proxy_test.go

+		t.Errorf("EmptyCall failed: %v", err)
+	}
+
+	select {


Please add another branch to check for timeouts and fail the test in finite time.

arjan-bal · 2024-12-24T11:59:20Z

test/proxy_test.go

+		if err := p.requestCheck(req); err != nil {
+			resp := http.Response{StatusCode: http.StatusMethodNotAllowed}
+			resp.Write(p.in)
+			p.in.Close()


I think we should log an error here instead of using the error channel, similar to the existing implementation. The write to the error channel can block indefinitely if no one reads and the test may pass if it doesn't read the errors in the channel, hiding real failures.

arjan-bal · 2024-12-24T12:02:14Z

test/proxy_test.go

+	}()
+
+	// Configure manual resolvers for both proxy and target backends
+	targetResolver := setupDNS(t)


We can use the real DNS resolver to resolve "localhost". Why use a fake hostname and manual resolver?

I cannot use a local host because the http.FromProxyEnvirnoment as well as httpproxy.FromEnviornment returns a nil proxy URL if the target URI is local host. The documentation says :

http.FromProxyEnvirnoment
As a special case, if req.URL.Host is "localhost" (with or without a port number), then a nil URL and nil error will be returned.

http.FromProxyEnvirnoment
As a special case, if req.URL.Host is "localhost" or a loopback address (with or without a port number), then a nil URL and nil error will be returned.

arjan-bal · 2024-12-26T06:15:08Z

test/proxy_test.go

+	os.Setenv("HTTPS_PROXY", pLis.Addr().String())
+	defer func() { os.Setenv("HTTPS_PROXY", proxyEnv) }()


Can use t.SetEnv that automatically resets the value as part of test cleanup.

eshitachandwani added the Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. label Nov 29, 2024

eshitachandwani added this to the 1.69 Release milestone Nov 29, 2024

eshitachandwani added the Type: Bug label Nov 29, 2024

eshitachandwani requested a review from purnesh42H November 30, 2024 10:44

eshitachandwani assigned purnesh42H Nov 30, 2024

Change proxy behaviour

619dcd4

eshitachandwani force-pushed the proxy_pr2 branch from 7fa029c to 619dcd4 Compare December 3, 2024 04:20

purnesh42H reviewed Dec 3, 2024

View reviewed changes

eshitachandwani added 3 commits December 5, 2024 00:07

change delegating resolver

65b33bd

Improving code

94364e2

correct import

9f6a067

eshitachandwani requested a review from purnesh42H December 5, 2024 06:15

improve tests

5ce86d0

arjan-bal self-requested a review December 5, 2024 07:31

arjan-bal self-assigned this Dec 5, 2024

purnesh42H modified the milestones: 1.69 Release, 1.70 Release Dec 5, 2024

purnesh42H reviewed Dec 5, 2024

View reviewed changes

purnesh42H assigned eshitachandwani and unassigned purnesh42H Dec 5, 2024

eshitachandwani added 6 commits December 5, 2024 17:05

address comments

fbef3a4

add warning and httpfunc test

342c332

add warning and httpfunc test

8726188

from delegating_pr

1cfb089

improve code

32d9de5

improve code

34b902f

eshitachandwani requested a review from purnesh42H December 8, 2024 11:55

eshitachandwani assigned purnesh42H Dec 8, 2024

eshitachandwani added 2 commits December 20, 2024 03:20

test

b515565

correct test environment

7ae2797

eshitachandwani requested a review from arjan-bal December 20, 2024 10:47

eshitachandwani and others added 4 commits December 23, 2024 12:42

Correct pr

5ce4658

Merge branch 'master' into proxy_pr2

a292b63

rebase

4483e95

comment

d4f6215

arjan-bal reviewed Dec 23, 2024

View reviewed changes

arjan-bal removed their assignment Dec 23, 2024

eshitachandwani added 3 commits December 23, 2024 23:11

proxy testutils refactor

333a68c

correct vet.sh

cacf058

correct vet

7c5b1b3

eshitachandwani requested a review from arjan-bal December 23, 2024 18:27

eshitachandwani assigned arjan-bal and unassigned eshitachandwani Dec 23, 2024

arjan-bal reviewed Dec 24, 2024

View reviewed changes

arjan-bal assigned eshitachandwani and unassigned arjan-bal Dec 24, 2024

arjan-bal removed the Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. label Dec 24, 2024

arjan-bal reviewed Dec 26, 2024

View reviewed changes

eshitachandwani and others added 5 commits December 27, 2024 10:12

Merge branch 'grpc:master' into proxy_pr2

cb6b09c

something

ff05ee3

working tests manual resolver

104cd18

working tests manual resolver

fb23cca

test

ef927ce

eshitachandwani requested a review from arjan-bal December 27, 2024 09:51

eshitachandwani assigned arjan-bal and unassigned eshitachandwani Dec 27, 2024

		address := addr.Addr

		//if the ProxyConnectAddr is set in the aattribute, do a proxy dial.

		os.Setenv("HTTPS_PROXY", pLis.Addr().String())
		defer func() { os.Setenv("HTTPS_PROXY", proxyEnv) }()

*: Correct gRFC A1 implementation #7881

Are you sure you want to change the base?

*: Correct gRFC A1 implementation #7881

Conversation

eshitachandwani commented Nov 29, 2024 • edited by arjan-bal Loading

codecov bot commented Nov 29, 2024 • edited Loading

Codecov Report

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H Dec 5, 2024 • edited Loading

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

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

arjan-bal 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjan-bal Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjan-bal Dec 24, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

eshitachandwani commented Nov 29, 2024 •

edited by arjan-bal

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

purnesh42H Dec 5, 2024 •

edited

Loading

arjan-bal Dec 24, 2024 •

edited

Loading

arjan-bal Dec 24, 2024 •

edited

Loading