Change DAPR_GRPC_ENDPOINT to infer TLS based on query parameter

[JoshVanL]

Required reading: https://github.com/grpc/grpc/blob/master/doc/naming.md

In the current proposal, SDKs are meant to enable TLS on gRPC endpoints based on the scheme being https. This is wrong- gRPC uses the scheme to determine which resolver to use. Not only is wrong w.r.t to the gRPC protocol, it also means users will not be able to use other resolvers (e.g. unix://) in conjunction with TLS.

This PR updates the proposal so that the DAPR_GRPC_ENDPOINT URL now includes an optional ?tls=<bool> parameter. This parameter signals to the SDK whether to enable TLS on this endpoint or not. The SDK will continue to infer whether to use TLS or not based on the endpoint port when this parameter is not present. DAPR_GRPC_ENDPOINT defaults to port 443 if not present.

Using ? query parameters is a natural choice here as they are 1. generally well understood by users and developers, 2. parsable by URL language libraries, and 3. used elsewhere in other systems like postgres to solve this problem.

[ItalyPaleAle]

Isn’t this a breaking change that will break existing apps that are running unless they update the SDK?

[elena-kolevska]

@ItalyPaleAle I worked on adding these env variables for Python and JS. The JS PR hasn't been merged yet, it's currently in review. The Python one was merged, but we haven't had a new release since. So, regarding these two SDKs - now is a good time to update them.

[artursouza]

In case of "https", there should be no way to connect to an HTTPS endpoint without TLS. If all we need is to enable TLS to other schemes that can operate with or without TLS, then it would not be a breaking change. It would simply allow other schemes to work with TLS when the default is to work without.

My suggestion is to make ?tls=<bool> as optional and ignore if when trying to disable TLS on a protocol that is over SSL by definition (like HTTPS).

[JoshVanL]

In case of "https", there should be no way to connect to an HTTPS endpoint without TLS. If all we need is to enable TLS to other schemes that can operate with or without TLS, then it would not be a breaking change. It would simply allow other schemes to work with TLS when the default is to work without.

https is not a valid hostname resolver which is the reason for its entire removal in this updated proposal. The proposal today is actually saying that the https resolver is using the dns resolver, but also to use TLS in the transport to the peer. This overloads what the name resolver (schema) is used for https://grpc.io/docs/guides/custom-name-resolution/

My suggestion is to make ?tls= as optional and ignore if when trying to disable TLS on a protocol that is over SSL by definition (like HTTPS).

Similarly to above, there is no name resolver which would require TLS in the transport.

[yaron2]

+1 binding

[XavierGeerinck]

+1 on this proposal. gRPC is non trivial and unclear at the moment and adding the https protocol makes it confusing. We should however try to leverage existing parsers in the SDKs where we can

[shubham1172]

+1 binding

[mukundansundar]

+1 binding

[artursouza]

I can vote on the condition we keep the backwards compatibility following the deprecation timeline of 2 releases for unsupported patterns in gRPC.

[berndverst]

I can vote on the condition we keep the backwards compatibility following the deprecation timeline of 2 releases for unsupported patterns in gRPC.

+1 from me (for Python SDK) also based on this condition.

As mentioned in another comment, Python SDK is already released. Backwards compatibility should be maintained if possible.

[elena-kolevska]

Here's a matrix of different possible endpoints and how they would be parsed based on this proposal. If it looks good, I can add it to the PR so it serves as a reference, and possibly as test cases for implementations.

URL	Endpoint string to pass to grpc client	Hostname	Port	Secure	Error
:5000	dns:localhost:5000	localhost	5000	FALSE
:5000?tls=false	dns:localhost:5000	localhost	5000	FALSE
:5000?tls=true	dns:localhost:5000	localhost	5000	TRUE
myhost	dns:myhost:443	myhost	443	FALSE
myhost?tls=false	dns:myhost:443	myhost	443	FALSE
myhost?tls=true	dns:myhost:443	myhost	443	TRUE
myhost:443	dns:myhost:443	myhost	443	FALSE
myhost:443?tls=false	dns:myhost:443	myhost	443	FALSE
myhost:443?tls=true	dns:myhost:443	myhost	443	TRUE
http://myhost	dns:myhost:443	myhost	443	FALSE
http://myhost?tls=false					Error: The tls query parameter is not supported for http(s) endpoints: 'tls=false'
http://myhost?tls=true					Error: The tls query parameter is not supported for http(s) endpoints: 'tls=true'
http://myhost:443	dns:myhost:443	myhost	443	FALSE
http://myhost:443?tls=false					Error: The tls query parameter is not supported for http(s) endpoints: 'tls=false'
http://myhost:443?tls=true					Error: The tls query parameter is not supported for http(s) endpoints: 'tls=true'
http://myhost:5000	dns:myhost:5000	myhost	5000	FALSE
http://myhost:5000?tls=false					Error: The tls query parameter is not supported for http(s) endpoints: 'tls=false'
http://myhost:5000?tls=true					Error: The tls query parameter is not supported for http(s) endpoints: 'tls=true'
https://myhost:443	dns:myhost:443	myhost	443	TRUE
https://myhost:443?tls=false					Error: The tls query parameter is not supported for http(s) endpoints: 'tls=false'
https://myhost:443?tls=true					Error: The tls query parameter is not supported for http(s) endpoints: 'tls=true'
dns:myhost	dns:myhost:443	myhost	443	FALSE
dns:myhost?tls=false	dns:myhost:443	myhost	443	FALSE
dns:myhost?tls=true	dns:myhost:443	myhost	443	TRUE
dns://myauthority:53/myhost	dns://myauthority:53/myhost:443	myhost	443	FALSE
dns://myauthority:53/myhost?tls=false	dns://myauthority:53/myhost:443	myhost	443	FALSE
dns://myauthority:53/myhost?tls=true	dns://myauthority:53/myhost:443	myhost	443	TRUE
dns://myhost					Error: Invalid dns authority 'myhost' in URL 'dns://myhost'
unix:my.sock	unix:my.sock	my.sock		FALSE
unix:my.sock?tls=true	unix:my.sock	my.sock		TRUE
unix://my.sock	unix://my.sock	my.sock		FALSE
unix://my.sock?tls=true	unix://my.sock	my.sock		TRUE
unix-abstract:my.sock	unix-abstract:my.sock	my.sock		FALSE
unix-abstract:my.sock?tls=true	unix-abstract:my.sock	my.sock		TRUE
vsock:mycid:5000	vsock:mycid:5000	mycid	5000	FALSE
vsock:mycid:5000?tls=true	vsock:mycid:5000	mycid	5000	TRUE
[2001:db8:1f70::999:de8:7648:6e8]	dns:[2001:db8:1f70::999:de8:7648:6e8]:443	[2001:db8:1f70::999:de8:7648:6e8]	443	FALSE
dns:[2001:db8:1f70::999:de8:7648:6e8]	dns:[2001:db8:1f70::999:de8:7648:6e8]:443	[2001:db8:1f70::999:de8:7648:6e8]	443	FALSE
https://[2001:db8:1f70::999:de8:7648:6e8]	dns:[2001:db8:1f70::999:de8:7648:6e8]:443	[2001:db8:1f70::999:de8:7648:6e8]	443	TRUE
host:5000/v1/dapr					Error: Paths are not supported for gRPC endpoints: '/v1/dapr'
host:5000/?a=1					Error: Paths are not supported for gRPC endpoints: '/'

[artursouza]

@elena-kolevska Can this table be PR-ed separately? I merged to make progress on this.