rpc: disable use of http proxies by default#55502
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Oct 13, 2020
Merged
rpc: disable use of http proxies by default#55502craig[bot] merged 1 commit intocockroachdb:masterfrom
craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
Member
|
This change is |
bdarnell
approved these changes
Oct 13, 2020
Contributor
bdarnell
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @irfansharif)
pkg/server/init.go, line 487 at r1 (raw file):
// successful, an initState is returned. func (s *initServer) attemptJoinTo(ctx context.Context, addr string) (*initState, error) { // XXX: Ben, is this what you mean? What should be the added comment/commit
I believe this part of the change is unnecessary - s.config.dialOpts is initialized from Context.grpcDialOptions.
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 (bug fix): 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.
deb9242 to
b25e3f7
Compare
irfansharif
commented
Oct 13, 2020
Contributor
Author
irfansharif
left a comment
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/server/init.go, line 487 at r1 (raw file):
Previously, bdarnell (Ben Darnell) wrote…
I believe this part of the change is unnecessary -
s.config.dialOptsis initialized fromContext.grpcDialOptions.
Woops, didn't mean to include that diff.
bdarnell
approved these changes
Oct 13, 2020
Contributor
|
Build succeeded: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GRPC uses the HTTPS_PROXY environment variable by default1. 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 #55289, and was a recent regression
in 20.2 after having introduced the join rpc in #52526 (it's the rpc
responsible for adding new nodes to the cluster #52526). Our existing
RPC connections use the
WithContextDialeroption already, which has theside 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 #55289. This will need to get backported to release-20.2.
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.