Skip to content

Update Port from 8302 to 8502#5610

Closed
schristoff wants to merge 3 commits intomasterfrom
update_grpc_port
Closed

Update Port from 8302 to 8502#5610
schristoff wants to merge 3 commits intomasterfrom
update_grpc_port

Conversation

@schristoff
Copy link
Contributor

No description provided.

@schristoff
Copy link
Contributor Author

See #5609 for more

@schristoff schristoff requested review from a team and pearkes April 5, 2019 21:47
* DNS Interface (Default 8600). Used to resolve DNS queries. TCP and UDP.

* gRPC API (Default 8302). Currently gRPC is only used to expose Envoy xDS API to Envoy proxies.
* gRPC API (Default 8502). Currently gRPC is only used to expose Envoy xDS API to Envoy proxies.
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to say that it is disabled by default, but is recommended to be run on 8502, similar to what is described here:

https://www.consul.io/docs/agent/options.html#grpc_port

It's kind of a tricky port as we don't listen on it by default. Now that I'm looking at this again I wonder if we should also add https here as well: https://www.consul.io/docs/agent/options.html#https_port.

I think the intention of the "ports used" section is to call out network requirements specifically so I think it makes sense to cover all of the ports there (required or optional).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sidecar Service Ports is odd, please let me know if you'd like me to reword it to flow more naturally.

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion from other PR (post mortem):

  • gRPC API (Optional, Defaults to 8502 in -dev mode). Currently gRPC is only used to expose the xDS API to Envoy proxies. It is off by default but port 8502 is a convention used by various tools as the default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a similar clarification of the https port could be added but other than that I think this is fine. HTTPs is not auto-enabled in dev mode so it's not totally the same but the last sentence of the description would be appropriate there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm with @banks on the suggestions there about the off by default stuff, do you want to make that change @s-christoff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like we are maybe being too verbose or saying default too much, this could use some minor tweaks on wording. Let me know what you guys think about the update.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

This information was captured in #5693.

@schristoff schristoff requested a review from pearkes April 6, 2019 02:24
@freddygv
Copy link
Contributor

Closing, since changes were captured in #5693

@freddygv freddygv closed this May 21, 2019
@freddygv freddygv deleted the update_grpc_port branch June 14, 2019 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants