-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
config: rename ports.grpc
to ports.xds
#10588
Conversation
🤔 This PR has changes in the |
Test failures is a known flake and unrelated to this change. |
@@ -242,8 +242,8 @@ The options below are all specified on the command-line. | |||
If it is provided after Consul has been initialized with an encryption key, then | |||
the provided key is ignored and a warning will be displayed. | |||
|
|||
- `-grpc-port` ((#\_grpc_port)) - the gRPC API port to listen on. Default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since our docs aren't versioned yet, I think we need to keep the two old grpc lines in here, but mark them as deprecated and link them to the new names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunate, but makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually when we deprecate stuff like this we also indicate which version of consul it was deprecated in so that folks using older versions can still figure out what's going on for their version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry I missed this comment. I wasn't sure which version this would land in yet. I guess it will be 1.11, so I can add that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending the one minor suggestion about retaining the old config keys on the website.
The DebugConfig in the self endpoint can change at any time. It's not a stable API. With the previous change to rename GRPCPort to XDSPort this command would have broken. This commit adds the XDSPort to a stable part of the XDS api, and changes the envoy command to read this new field. It includes support for the old API as well, in case a newer CLI is used with an older API, and adds a test for both cases.
Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com>
The compatv2 integration tests were failing because they use an older CLI version with a newer HTTP API. This commit restores the GRPCPort field to the DebugConfig output to allow older CIs to continue to fetch the port.
aa35514
to
b5cd205
Compare
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/408416. |
Now that we have gRPC endpoints (streaming subscribe, and a new one coming in enterprise) that use the regular RPC port it seems likely that we will continue to use the RPC port for any "regular" RPC endpoints.
However, our config has a
ports.grpc
andaddresses.grpc
that apply only to the xDS interface. Since the consumer of the xDS interface is quite different from the RPC interface (expected to be localhost, and a Envoy proxy), it seems likely that this port will only ever be used for xDS, and all other things will continue to use the RPC port.To make our config and documentation less confusing I've renamed this config setting to be
xds
instead ofgrpc
. This change should be entirely backwards compatible. The old config setting and command line flag both continue to work, and theconsul connect envoy
command is still able to query for this port with the new name.TODO: