Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: Correct gRFC A1 implementation #7881

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
619dcd4
Change proxy behaviour
eshitachandwani Dec 3, 2024
65b33bd
change delegating resolver
eshitachandwani Dec 4, 2024
94364e2
Improving code
eshitachandwani Dec 5, 2024
9f6a067
correct import
eshitachandwani Dec 5, 2024
5ce86d0
improve tests
eshitachandwani Dec 5, 2024
fbef3a4
address comments
eshitachandwani Dec 5, 2024
342c332
add warning and httpfunc test
eshitachandwani Dec 5, 2024
8726188
add warning and httpfunc test
eshitachandwani Dec 5, 2024
1cfb089
from delegating_pr
eshitachandwani Dec 7, 2024
32d9de5
improve code
eshitachandwani Dec 8, 2024
34b902f
improve code
eshitachandwani Dec 8, 2024
11c8fb1
improve
eshitachandwani Dec 9, 2024
b4c9980
correct tests
eshitachandwani Dec 18, 2024
93fc3e1
trying tests
eshitachandwani Dec 18, 2024
e22ad1d
tests
eshitachandwani Dec 19, 2024
b515565
test
eshitachandwani Dec 19, 2024
7ae2797
correct test environment
eshitachandwani Dec 20, 2024
5ce4658
Correct pr
eshitachandwani Dec 23, 2024
a292b63
Merge branch 'master' into proxy_pr2
eshitachandwani Dec 23, 2024
4483e95
rebase
eshitachandwani Dec 23, 2024
d4f6215
comment
eshitachandwani Dec 23, 2024
333a68c
proxy testutils refactor
eshitachandwani Dec 23, 2024
cacf058
correct vet.sh
eshitachandwani Dec 23, 2024
7c5b1b3
correct vet
eshitachandwani Dec 23, 2024
cb6b09c
Merge branch 'grpc:master' into proxy_pr2
eshitachandwani Dec 27, 2024
ff05ee3
something
eshitachandwani Dec 27, 2024
104cd18
working tests manual resolver
eshitachandwani Dec 27, 2024
fb23cca
working tests manual resolver
eshitachandwani Dec 27, 2024
ef927ce
test
eshitachandwani Dec 27, 2024
bc5efc3
trying test without manual resolver
eshitachandwani Jan 2, 2025
876f09e
e2e tests
eshitachandwani Jan 3, 2025
3e20010
correct vet
eshitachandwani Jan 3, 2025
6827a62
correct e2e tests
eshitachandwani Jan 3, 2025
9acd8de
correct vet
eshitachandwani Jan 3, 2025
8f5055e
vet
eshitachandwani Jan 3, 2025
bbd7f02
vet
eshitachandwani Jan 3, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions clientconn.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,10 +228,8 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *
//
// 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.
opts = append([]DialOption{withDefaultScheme("passthrough"), WithTargetResolutionEnabled()}, opts...)

// resolution, when "dns" is used for resolution,
// perform resolution on the client.
opts = append([]DialOption{withDefaultScheme("passthrough"), WithTargetResolutionEnabled()}, opts...)
arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
cc, err := NewClient(target, opts...)
if err != nil {
Expand Down
7 changes: 4 additions & 3 deletions dialoptions.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ type dialOptions struct {
idleTimeout time.Duration
defaultScheme string
maxCallAttempts int
targetResolutionEnabled bool // Specifies if client should perform target resolution when proxy is enabled.
useProxy bool // Specifies if a proxy should be used.
targetResolutionEnabled bool // Specifies if target hostnames should be resolved when proxying is enabled.
useProxy bool // Specifies if a server should be connected via proxy.
}

// DialOption configures how we set up the connection.
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

// WithNoProxy is used.
//
// # Experimental
//
Expand Down
111 changes: 0 additions & 111 deletions internal/testutils/proxy.go
arjan-bal marked this conversation as resolved.
Outdated
Show resolved Hide resolved

This file was deleted.

4 changes: 2 additions & 2 deletions internal/transport/http2_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,8 @@ type http2Client struct {
func dial(ctx context.Context, fn func(context.Context, string) (net.Conn, error), addr resolver.Address, grpcUA string) (net.Conn, error) {
address := addr.Addr

if _, present := proxyattributes.Get(addr); present {
return proxyDial(ctx, addr, grpcUA)
if opts, present := proxyattributes.Get(addr); present {
return proxyDial(ctx, addr, grpcUA, opts)
}
networkType, ok := networktype.Get(addr)
if fn != nil {
Expand Down
7 changes: 3 additions & 4 deletions internal/transport/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,13 @@
return base64.StdEncoding.EncodeToString([]byte(auth))
}

func doHTTPConnectHandshake(ctx context.Context, conn net.Conn, addr resolver.Address, grpcUA string) (_ net.Conn, err error) {
func doHTTPConnectHandshake(ctx context.Context, conn net.Conn, grpcUA string, opts proxyattributes.Options) (_ net.Conn, err error) {
defer func() {
if err != nil {
conn.Close()
}
}()

opts, _ := proxyattributes.Get(addr)
req := &http.Request{
Method: http.MethodConnect,
URL: &url.URL{Host: opts.ConnectAddr},
Expand All @@ -72,8 +71,8 @@
u := user.Username()
p, pSet := user.Password()
if !pSet && logger.V(2) {
logger.Warningf("password not set for basic authentication for proxy dialing")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

Check warning on line 75 in internal/transport/proxy.go

View check run for this annotation

Codecov / codecov/patch

internal/transport/proxy.go#L74-L75

Added lines #L74 - L75 were not covered by tests
req.Header.Add(proxyAuthHeaderKey, "Basic "+basicAuth(u, p))
}
if err := sendHTTPRequest(ctx, req, conn); err != nil {
Expand Down Expand Up @@ -104,12 +103,12 @@
}

// proxyDial establishes a TCP connection to the specified address and performs an HTTP CONNECT handshake.
func proxyDial(ctx context.Context, addr resolver.Address, grpcUA string) (net.Conn, error) {
func proxyDial(ctx context.Context, addr resolver.Address, grpcUA string, opts proxyattributes.Options) (net.Conn, error) {
conn, err := internal.NetDialerWithTCPKeepalive().DialContext(ctx, "tcp", addr.Addr)
if err != nil {
return nil, err
}
return doHTTPConnectHandshake(ctx, conn, addr, grpcUA)
return doHTTPConnectHandshake(ctx, conn, grpcUA, opts)
}

func sendHTTPRequest(ctx context.Context, req *http.Request, conn net.Conn) error {
Expand Down
89 changes: 86 additions & 3 deletions test/proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,24 @@
package test

import (
"bufio"
"bytes"
"context"
"encoding/base64"
"fmt"
"io"
"net"
"net/http"
"net/url"
"os"
"testing"
"time"

"golang.org/x/net/http/httpproxy"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/internal/resolver/delegatingresolver"
"google.golang.org/grpc/internal/stubserver"
"google.golang.org/grpc/internal/testutils"
testgrpc "google.golang.org/grpc/interop/grpc_testing"
"google.golang.org/grpc/resolver"
"google.golang.org/grpc/resolver/manual"
Expand Down Expand Up @@ -70,6 +73,86 @@ func setupDNS(t *testing.T) *manual.Resolver {
return mr
}

// proxyServer represents a test proxy server.
type proxyServer struct {
lis net.Listener
in net.Conn // Connection from the client to the proxy.
out net.Conn // Connection from the proxy to the backend.
requestCheck func(*http.Request) error // Function to check the request sent to proxy.
}

// Stop closes the ProxyServer and its connections to client and server.
func (p *proxyServer) stop() {
p.lis.Close()
if p.in != nil {
p.in.Close()
}
if p.out != nil {
p.out.Close()
}
}

// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

p := &proxyServer{
lis: lis,
requestCheck: reqCheck,
}

// Start the proxy server.
go func() {
in, err := p.lis.Accept()
if err != nil {
return
}
p.in = in
// This will be used in tests to check if the proxy server is started.
if proxyStarted != nil {
proxyStarted()
}
req, err := http.ReadRequest(bufio.NewReader(in))
if err != nil {
errCh <- fmt.Errorf("failed to read CONNECT req: %v", err)
return
}
if err := p.requestCheck(req); err != nil {
resp := http.Response{StatusCode: http.StatusMethodNotAllowed}
resp.Write(p.in)
p.in.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

errCh <- fmt.Errorf("get wrong CONNECT req: %+v, error: %v", req, err)
return
}
var out net.Conn
// If resolution is done on client,connect to address received in
// CONNECT request or else connect to backend address directly. This is
// to mimick the name resolution on proxy server.
if resOnClient {
out, err = net.Dial("tcp", req.URL.Host)
} else {
out, err = net.Dial("tcp", backendAddr)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if err != nil {
errCh <- fmt.Errorf("failed to dial to server: %v", err)
return
}
out.SetDeadline(time.Now().Add(defaultTestTimeout))

// Response OK to client
resp := http.Response{StatusCode: http.StatusOK, Proto: "HTTP/1.0"}
var buf bytes.Buffer
resp.Write(&buf)
p.in.Write(buf.Bytes())
p.out = out

// Perform the proxy function, i.e pass the data from client to server
// and server to client.
go io.Copy(p.in, p.out)
go io.Copy(p.out, p.in)
close(doneCh)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}()
return p
}

// setupProxy initializes and starts a proxy server, registers a cleanup to
// stop it, and returns the proxy's listener and helper channels.
func setupProxy(t *testing.T, backendAddr string, resolutionOnClient bool, reqCheck func(*http.Request) error) (net.Listener, chan error, chan struct{}, chan struct{}) {
Expand All @@ -86,8 +169,8 @@ func setupProxy(t *testing.T, backendAddr string, resolutionOnClient bool, reqCh
close(errCh)
})

proxyServer := testutils.NewProxyServer(pLis, reqCheck, errCh, doneCh, backendAddr, resolutionOnClient, func() { close(proxyStartedCh) })
t.Cleanup(proxyServer.Stop)
proxyServer := newProxyServer(pLis, reqCheck, errCh, doneCh, backendAddr, resolutionOnClient, func() { close(proxyStartedCh) })
t.Cleanup(proxyServer.stop)

return pLis, errCh, doneCh, proxyStartedCh
}
Expand Down
Loading