Skip to content

Commit

Permalink
rpc: disable use of http proxies by default
Browse files Browse the repository at this point in the history
GRPC uses the HTTPS_PROXY environment variable by default[1]. This is
surprising, and likely undesirable for CRDB because it turns the proxy
into an availability risk and a throughput bottleneck. We disable the
use of proxies by default when retrieving grpc dial options.

This diff came up in the context of cockroachdb#55289, and was a recent regression
in 20.2 after having introduced the join rpc in cockroachdb#52526 (it's the rpc
responsible for adding new nodes to the cluster cockroachdb#52526). Our existing
RPC connections use the `WithContextDialer` option already, which has the
side effect of disabling proxy use. The join RPC didn't, so an existing
cluster upgrade to 20.2 wouldn't use the proxy until a node was added.

Fixes cockroachdb#55289. This will need to get backported to release-20.2.

[1]: https://github.com/grpc/grpc-go/blob/c0736608/Documentation/proxy.md

Release note: Previously we used the HTTPS_PROXY variable for the "join
RPC" when adding a node to the cluster (the RPC prevents new clusters
from starting or adding nodes to an existing cluster). The proxy needed
to configured to transparently pass HTTP/2+GRPC inter-node traffic. This
was an unintentional addition, and this patch ignores the proxies for
all intra-node traffic. They were already ignored in releases prior to
20.2.
  • Loading branch information
irfansharif committed Oct 13, 2020
1 parent 07039fe commit deb9242
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 1 deletion.
8 changes: 8 additions & 0 deletions pkg/rpc/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,14 @@ func (ctx *Context) grpcDialOptions(
dialOpts = append(dialOpts, grpc.WithDefaultCallOptions(grpc.UseCompressor((snappyCompressor{}).Name())))
}

// GRPC uses the HTTPS_PROXY environment variable by default[1]. This is
// surprising, and likely undesirable for CRDB because it turns the proxy
// into an availability risk and a throughput bottleneck. We disable the use
// of proxies by default.
//
// [1]: https://github.com/grpc/grpc-go/blob/c0736608/Documentation/proxy.md
dialOpts = append(dialOpts, grpc.WithNoProxy())

var unaryInterceptors []grpc.UnaryClientInterceptor
var streamInterceptors []grpc.StreamClientInterceptor

Expand Down
6 changes: 5 additions & 1 deletion pkg/server/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,11 @@ func (s *initServer) startJoinLoop(ctx context.Context, stopper *stop.Stopper) (
// attemptJoinTo attempts to join to the node running at the given address. If
// successful, an initState is returned.
func (s *initServer) attemptJoinTo(ctx context.Context, addr string) (*initState, error) {
conn, err := grpc.DialContext(ctx, addr, s.config.dialOpts...)
// XXX: Ben, is this what you mean? What should be the added comment/commit
// message here?
dialOpts := s.config.dialOpts
dialOpts = append(dialOpts, grpc.WithNoProxy())
conn, err := grpc.DialContext(ctx, addr, dialOpts...)
if err != nil {
return nil, err
}
Expand Down

0 comments on commit deb9242

Please sign in to comment.