Jaeger: Separate GRPC and HTTP ports for Query

[rjs211]

Requirement - what kind of business use case are you trying to solve?

Required for enabling TLS support for query server in the future. #2249

Problem - what in Jaeger blocks you from solving the requirement?

There is a need for adding separate ports for grRPC and http component of query server so as to eliminate the use of cmux. Cmux elimination is required for adding TLS support in the future and maintain performance doing so. Presence of cmux forces the server to use a single certificate to be used for both gRPC and http requests, which isn't always favourable, as pointed out by @tcolgate https://github.com/jaegertracing/jaeger/pull/2337#discussion_r462166145

Proposal - what do you suggest to solve the problem or improve the existing situation?

Proposed by @jpkrohling in https://github.com/jaegertracing/jaeger/pull/2337#pullrequestreview-458476438 and reviewed by @yurishkuro
copying from above:

1. add a new flag, --query.grpc-server.host-port (like the collector)
2. replace --query.host-port with --query.http-server.host-port (again, like the collector)
3. when no specific gRPC or http host-port is set, the current behavior is kept
   This way, we keep backwards compatibility with current users, while we have a chance to get out of the cmux

Any open questions to address

declaring and defining of default values for flag: 'query.http-server.host-port' as ports.queryHTTP and flag: 'query.grpc-server.host-port' as ports.queryGRPC

[rjs211]

Is the following logic acceptable?
Nontls Logic for 2 ports:

if query.http-server.host-port != ":" and query.grpc-server.host-port != ":" {  // both are set
    run grpc on query.grpc-server.host-port
    run http on query.http-server.host-port    
}
elif query.grpc-server.host-port != ":" { // separate grpc port  is set
    run grpc on query.grpc-server.host-port
    run http on ports.queryHTTP     // defined in "github.com/jaegertracing/jaeger/ports"
}
elif query.http-server.host-port != ":" {  // http separete port is set
    run http on query.http-server.host-port
    run grpc on ports.queryGRPC    // To be defined in "github.com/jaegertracing/jaeger/ports"
}
else  {  // both not set  query.grpc-server.host-port == ":" and query.http-server.host-port == ":" 
    use cmux on query.host-port
}

[yurishkuro]

Why are you comparing with ":"? That's an odd default.

[rjs211]

Sorry, should have made myself clear.
Assume notEmpty(flag) is set true if the flag was not set. the default un-processed value of the flag (of string type) is "".
Nontls Logic for 2 ports:

This is the reference behavior. The actual conditionals and comparisons will be such that, at the end, this behavior is ensured.

if notEmpty(query.http-server.host-port)  and notEmpty(query.grpc-server.host-port)  {  // both are set
    run grpc on query.grpc-server.host-port
    run http on query.http-server.host-port    
}
elif notEmpty(query.grpc-server.host-port)  { // separate grpc port  is set
    run grpc on query.grpc-server.host-port
    run http on ports.queryHTTP     // defined in "github.com/jaegertracing/jaeger/ports"
}
elif notEmpty(query.http-server.host-port) {  // http separete port is set
    run http on query.http-server.host-port
    run grpc on ports.queryGRPC    // To be defined in "github.com/jaegertracing/jaeger/ports"
}
else  {  // both not set i.e.  empty(query.grpc-server.host-port)  and empty(query.http-server.host-port)  
    use cmux on query.host-port
}

p.s. previously i wrote the logic before writing the code. The default for processing string port flags was portflag_value = ports.GetAddressFromCLIOptions(0, v.GetString(port_flag)). so i had used the value of ":" which would be retirned when the value of portflag was "", Now that i wrote the code, I realized that that wasnt neccessary.

[yurishkuro]

I would prefer to simplify and also de-couple this a bit.

in config parser:

• if the legacy --query.host-port is defined, write its value into --query.http-server.host-port and --query.grpc-server.host-port fields, log a deprecation warning

in the service:

• if the two new options have different values, then start two servers on different ports, otherwise start cmux

[rjs211]

But current users, in the current behavior, need not speciifcally define --query.host-port , in which case, the default value of ports.PortToHostPort(ports.QueryHTTP) is used (viper default for query.host-port). The proposed behavior will force the current users to define the --query.host-port to maintain their current behavior. Is that preferable??

[jpkrohling]

For completeness, @yurishkuro's logic is missing the case where no flags are defined at all. In that case, a default value is set directly to the new flags, which is the same default that the old flag has. This way, the old flag can be changed to have no defaults, yielding an empty value when queried via viper. Note that Viper also has a IsSet method, which could be used to see if the value for the flag was explicitly set by users.