Skip to content

Commit

Permalink
Remove deprecated Query Server flags (#2772)
Browse files Browse the repository at this point in the history 
* Removing depricated query server flags

Signed-off-by: rjs211 <srivatsa211@gmail.com>

* modify comments to make things more clear

Signed-off-by: rjs211 <srivatsa211@gmail.com>

* adding changelog, correcting comments

Signed-off-by: rjs211 <srivatsa211@gmail.com>

* changing default behaviour of host-port flags

Signed-off-by: rjs211 <srivatsa211@gmail.com>

* pruning test cases, renaming test method

Signed-off-by: rjs211 <srivatsa211@gmail.com>

* changing comments for host-port default

Signed-off-by: rjs211 <srivatsa211@gmail.com>

* changelog for default query server behaviour change

Signed-off-by: rjs211 <srivatsa211@gmail.com>
  • Loading branch information
@rjs211
rjs211 authored Feb 4, 2021
1 parent 054b6b4 commit 1e1d244
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 133 deletions.
11 changes: 6 additions & 5 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ Changes by Version

#### Breaking Changes

* Remove deprecated flags of Query Server `--query.port` and `--query.host-port`, please use dedicated HTTP `--query.http-server.host-port` (defaults to `:16686`) and gRPC `--query.grpc-server.host-port` (defaults to `:16685`) host-ports flags instead ([#2772](https://github.com/jaegertracing/jaeger/pull/2772), [@rjs211](https://github.com/rjs211))
* By default, if no flags are set, the query server starts on the dedicated ports. To use common port for gRPC and HTTP endpoints, the host-port flags have to be explicitly set

* Remove deprecated CLI flags ([#2751](https://github.com/jaegertracing/jaeger/issues/2751), [@LostLaser](https://github.com/LostLaser)):
* `--collector.http-port` is replaced by `--collector.http-server.host-port`
* `--collector.grpc-port` is replaced by `--collector.grpc-server.host-port`
Expand All @@ -21,12 +24,10 @@ Changes by Version

#### New Features

* Add TLS Support for GRPC and HTTP endpoints of the Query server ([#2337](https://github.com/jaegertracing/jaeger/pull/2337), [@rjs211](https://github.com/rjs211))

Enabling TLS in either or both of GRPC or HTTP endpoints forces the server to use dedicated host-port for the two endpoints. The dedicated host-ports are set using the flags `--query.http-server.host-port` (defaults to `:16686`) and `--query.grpc-server.host-port` (defaults to `:16685`) for the HTTP and GRPC endpoints respectively. If either of both of the dedicated host-ports' flags are not explicitly set, the default values are used.

* Add TLS Support for gRPC and HTTP endpoints of the Query server ([#2337](https://github.com/jaegertracing/jaeger/pull/2337), [#2772](https://github.com/jaegertracing/jaeger/pull/2772), [@rjs211](https://github.com/rjs211))

If TLS is disabled in both endpoints (default), dedicated host-ports are used if either or both of `--query.http-server.host-port` or `--query.grpc-server.host-port` is explicitly set. If only one of the dedicated host-port flags is set, the other flag uses its corresponding default value. If neither of the dedicated host-port flags are explicitly set, the common host-port infered from `--query.host-port` (defaults to `:16686`) is used for both the GRPC and HTTP endpoints.
* If TLS in enabled on either or both of gRPC or HTTP endpoints, the gRPC host-port and the HTTP hostport have to be different
* If TLS is disabled on both endpoints, common HTTP and gRPC host-port can be explicitly set using the `--query.http-server.host-port` and `--query.grpc-server.host-port` host-port flags

### UI Changes

Expand Down
35 changes: 2 additions & 33 deletions cmd/query/app/flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,6 @@ import (
)

const (
queryHostPort = "query.host-port"
queryPort = "query.port"
queryPortWarning = "(deprecated, will be removed after 2020-08-31 or in release v1.20.0, whichever is later)"
queryHOSTPortWarning = "(deprecated, will be removed after 2020-12-31 or in release v1.21.0, whichever is later)"
queryHTTPHostPort = "query.http-server.host-port"
queryGRPCHostPort = "query.grpc-server.host-port"
queryBasePath = "query.base-path"
Expand Down Expand Up @@ -92,10 +88,8 @@ type QueryOptions struct {
// AddFlags adds flags for QueryOptions
func AddFlags(flagSet *flag.FlagSet) {
flagSet.Var(&config.StringSlice{}, queryAdditionalHeaders, `Additional HTTP response headers. Can be specified multiple times. Format: "Key: Value"`)
flagSet.String(queryHostPort, ports.PortToHostPort(ports.QueryHTTP), queryHOSTPortWarning+" see --"+queryHTTPHostPort+" and --"+queryGRPCHostPort)
flagSet.String(queryHTTPHostPort, ports.PortToHostPort(ports.QueryHTTP), "The host:port (e.g. 127.0.0.1:14268 or :14268) of the query's HTTP server")
flagSet.String(queryGRPCHostPort, ports.PortToHostPort(ports.QueryGRPC), "The host:port (e.g. 127.0.0.1:14250 or :14250) of the query's gRPC server")
flagSet.Int(queryPort, 0, queryPortWarning+" see --"+queryHostPort)
flagSet.String(queryBasePath, "/", "The base path for all HTTP routes, e.g. /jaeger; useful when running behind a reverse proxy")
flagSet.String(queryStaticFiles, "", "The directory path override for the static assets for the UI")
flagSet.String(queryUIConfig, "", "The path to the UI configuration file in JSON format")
Expand All @@ -105,37 +99,12 @@ func AddFlags(flagSet *flag.FlagSet) {
tlsHTTPFlagsConfig.AddFlags(flagSet)
}

// InitPortsConfigFromViper initializes the port numbers and TLS configuration of ports
func (qOpts *QueryOptions) InitPortsConfigFromViper(v *viper.Viper, logger *zap.Logger) *QueryOptions {
// InitFromViper initializes QueryOptions with properties from viper
func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) *QueryOptions {
qOpts.HTTPHostPort = v.GetString(queryHTTPHostPort)
qOpts.GRPCHostPort = v.GetString(queryGRPCHostPort)
qOpts.HostPort = ports.GetAddressFromCLIOptions(v.GetInt(queryPort), v.GetString(queryHostPort))

qOpts.TLSGRPC = tlsGRPCFlagsConfig.InitFromViper(v)
qOpts.TLSHTTP = tlsHTTPFlagsConfig.InitFromViper(v)

// If either GRPC or HTTP servers use TLS, use dedicated ports.
if qOpts.TLSGRPC.Enabled || qOpts.TLSHTTP.Enabled {
return qOpts
}

// --query.host-port flag is not set and either or both of --query.grpc-server.host-port or --query.http-server.host-port is set by the user with command line flags.
// i.e. user intends to use separate GRPC and HTTP host:port flags
if !(v.IsSet(queryHostPort) || v.IsSet(queryPort)) && (v.IsSet(queryHTTPHostPort) || v.IsSet(queryGRPCHostPort)) {
return qOpts
}
if v.IsSet(queryHostPort) || v.IsSet(queryPort) {
logger.Warn(fmt.Sprintf("Use of %s and %s is deprecated. Use %s and %s instead", queryPort, queryHostPort, queryHTTPHostPort, queryGRPCHostPort))
}
qOpts.HTTPHostPort = qOpts.HostPort
qOpts.GRPCHostPort = qOpts.HostPort
return qOpts

}

// InitFromViper initializes QueryOptions with properties from viper
func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) *QueryOptions {
qOpts = qOpts.InitPortsConfigFromViper(v, logger)
qOpts.BasePath = v.GetString(queryBasePath)
qOpts.StaticAssets = v.GetString(queryStaticFiles)
qOpts.UIConfig = v.GetString(queryUIConfig)
Expand Down
102 changes: 17 additions & 85 deletions cmd/query/app/flags_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,22 +29,14 @@ import (
spanstore_mocks "github.com/jaegertracing/jaeger/storage/spanstore/mocks"
)

func TestQueryBuilderFlagsDeprecation(t *testing.T) {
v, command := config.Viperize(AddFlags)
command.ParseFlags([]string{
"--query.port=80",
})
qOpts := new(QueryOptions).InitFromViper(v, zap.NewNop())
assert.Equal(t, ":80", qOpts.HostPort)
}

func TestQueryBuilderFlags(t *testing.T) {
v, command := config.Viperize(AddFlags)
command.ParseFlags([]string{
"--query.static-files=/dev/null",
"--query.ui-config=some.json",
"--query.base-path=/jaeger",
"--query.host-port=127.0.0.1:8080",
"--query.http-server.host-port=127.0.0.1:8080",
"--query.grpc-server.host-port=127.0.0.1:8081",
"--query.additional-headers=access-control-allow-origin:blerg",
"--query.additional-headers=whatever:thing",
"--query.max-clock-skew-adjustment=10s",
Expand All @@ -53,7 +45,8 @@ func TestQueryBuilderFlags(t *testing.T) {
assert.Equal(t, "/dev/null", qOpts.StaticAssets)
assert.Equal(t, "some.json", qOpts.UIConfig)
assert.Equal(t, "/jaeger", qOpts.BasePath)
assert.Equal(t, "127.0.0.1:8080", qOpts.HostPort)
assert.Equal(t, "127.0.0.1:8080", qOpts.HTTPHostPort)
assert.Equal(t, "127.0.0.1:8081", qOpts.GRPCHostPort)
assert.Equal(t, http.Header{
"Access-Control-Allow-Origin": []string{"blerg"},
"Whatever": []string{"thing"},
Expand Down Expand Up @@ -136,90 +129,32 @@ func TestQueryOptionsPortAllocationFromFlags(t *testing.T) {
expectedHostPort string
}{
{
// Since TLS is enabled in atleast one server, the dedicated host-ports obtained from viper are used, even if common host-port is specified
name: "Atleast one dedicated host-port and common host-port is specified, atleast one of GRPC, HTTP TLS enabled",
flagsArray: []string{
"--query.grpc.tls.enabled=true",
"--query.http-server.host-port=127.0.0.1:8081",
"--query.host-port=127.0.0.1:8080",
},
expectedHTTPHostPort: "127.0.0.1:8081",
// Default behavior. Dedicated host-port is used for both HTTP and GRPC endpoints
name: "No host-port flags specified, both GRPC and HTTP TLS disabled",
flagsArray: []string{},
expectedHTTPHostPort: ports.PortToHostPort(ports.QueryHTTP), // fallback in viper
expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper
verifyCommonPort: false,
},
{
// TLS is disabled in both servers, since common host-port is specified, common host-port is used
name: "Atleast one dedicated host-port is specified, common host-port is specified, both GRPC and HTTP TLS disabled",
flagsArray: []string{
"--query.http-server.host-port=127.0.0.1:8081",
"--query.host-port=127.0.0.1:8080",
},
expectedHTTPHostPort: "127.0.0.1:8080",
expectedGRPCHostPort: "127.0.0.1:8080",
verifyCommonPort: true,
expectedHostPort: "127.0.0.1:8080",
},
{
// Since TLS is enabled in atleast one server, the dedicated host-ports obtained from viper are used
name: "Atleast one dedicated host-port is specified, common host-port is not specified, atleast one of GRPC, HTTP TLS enabled",
flagsArray: []string{
"--query.grpc.tls.enabled=true",
"--query.http-server.host-port=127.0.0.1:8081",
},
expectedHTTPHostPort: "127.0.0.1:8081",
expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper
verifyCommonPort: false,
},
{
// TLS is disabled in both servers, since common host-port is not specified but atleast one dedicated port is specified, the dedicated host-ports obtained from viper are used
name: "Atleast one dedicated port, common port defined, both GRPC and HTTP TLS disabled",
// If any one host-port is specified, and TLS is diabled, fallback to ports defined in viper
name: "Atleast one dedicated host-port is specified, both GRPC and HTTP TLS disabled",
flagsArray: []string{
"--query.http-server.host-port=127.0.0.1:8081",
},
expectedHTTPHostPort: "127.0.0.1:8081",
expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper
verifyCommonPort: false,
},
{
// Since TLS is enabled in atleast one server, the dedicated host-ports obtained from viper are used, even if common host-port is specified and the dedicated host-port are not specified
name: "No dedicated host-port is specified, common host-port is specified, atleast one of GRPC, HTTP TLS enabled",
flagsArray: []string{
"--query.grpc.tls.enabled=true",
"--query.host-port=127.0.0.1:8080",
},
expectedHTTPHostPort: ports.PortToHostPort(ports.QueryHTTP), // fallback in viper
expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper
verifyCommonPort: false,
},
{
// TLS is disabled in both servers, since only common host-port is specified, common host-port is used
name: "No dedicated host-port is specified, common host-port is specified, both GRPC and HTTP TLS disabled",
flagsArray: []string{
"--query.host-port=127.0.0.1:8080",
},
expectedHTTPHostPort: "127.0.0.1:8080",
expectedGRPCHostPort: "127.0.0.1:8080",
verifyCommonPort: true,
expectedHostPort: "127.0.0.1:8080",
},
{
// Since TLS is enabled in atleast one server, the dedicated host-ports obtained from viper are used
name: "No dedicated host-port is specified, common host-port is not specified, atleast one of GRPC, HTTP TLS enabled",
// Allows usage of common host-ports. Flags allow this irrespective of TLS status
// The server with TLS enabled with equal HTTP & GRPC host-ports, is still an acceptable flag configuration, but will throw an error during initialisation
name: "Common equal host-port specified, TLS enabled in atleast one server",
flagsArray: []string{
"--query.grpc.tls.enabled=true",
"--query.http-server.host-port=127.0.0.1:8081",
"--query.grpc-server.host-port=127.0.0.1:8081",
},
expectedHTTPHostPort: ports.PortToHostPort(ports.QueryHTTP), // fallback in viper
expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper
verifyCommonPort: false,
},
{
// TLS is disabled in both servers, since common host-port is not specified and neither dedicated ports are specified, common host-port from viper is used
name: "No dedicated host-port is specified, common host-port is not specified, both GRPC and HTTP TLS disabled",
flagsArray: []string{},
expectedHTTPHostPort: ports.PortToHostPort(ports.QueryHTTP),
expectedGRPCHostPort: ports.PortToHostPort(ports.QueryHTTP),
verifyCommonPort: true,
expectedHostPort: ports.PortToHostPort(ports.QueryHTTP), // fallback in viper
expectedHTTPHostPort: "127.0.0.1:8081",
expectedGRPCHostPort: "127.0.0.1:8081",
},
}

Expand All @@ -231,9 +166,6 @@ func TestQueryOptionsPortAllocationFromFlags(t *testing.T) {

assert.Equal(t, test.expectedHTTPHostPort, qOpts.HTTPHostPort)
assert.Equal(t, test.expectedGRPCHostPort, qOpts.GRPCHostPort)
if test.verifyCommonPort {
assert.Equal(t, test.expectedHostPort, qOpts.HostPort)
}

})
}
Expand Down
8 changes: 3 additions & 5 deletions cmd/query/app/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ func (s *Server) initListener() (cmux.CMux, error) {
}

// old behavior using cmux
conn, err := net.Listen("tcp", s.queryOptions.HostPort)
conn, err := net.Listen("tcp", s.queryOptions.HTTPHostPort)
if err != nil {
return nil, err
}
Expand All @@ -187,7 +187,7 @@ func (s *Server) initListener() (cmux.CMux, error) {
s.logger.Info(
"Query server started",
zap.Int("port", tcpPort),
zap.String("addr", s.queryOptions.HostPort))
zap.String("addr", s.queryOptions.HTTPHostPort))

// cmux server acts as a reverse-proxy between HTTP and GRPC backends.
cmuxServer := cmux.New(s.conn)
Expand All @@ -197,8 +197,6 @@ func (s *Server) initListener() (cmux.CMux, error) {
cmux.HTTP2MatchHeaderFieldSendSettings("content-type", "application/grpc+proto"),
)
s.httpConn = cmuxServer.Match(cmux.Any())
s.queryOptions.HTTPHostPort = s.queryOptions.HostPort
s.queryOptions.GRPCHostPort = s.queryOptions.HostPort

return cmuxServer, nil

Expand Down Expand Up @@ -259,7 +257,7 @@ func (s *Server) Start() error {
// Start cmux server concurrently.
if !s.separatePorts {
go func() {
s.logger.Info("Starting CMUX server", zap.Int("port", tcpPort), zap.String("addr", s.queryOptions.HostPort))
s.logger.Info("Starting CMUX server", zap.Int("port", tcpPort), zap.String("addr", s.queryOptions.HTTPHostPort))

err := cmuxServer.Serve()
// TODO: Remove string comparison when https://github.com/soheilhy/cmux/pull/69 is merged
Expand Down
10 changes: 5 additions & 5 deletions cmd/query/app/server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ var testCertKeyLocation = "../../../pkg/config/tlscfg/testdata"
func TestServerError(t *testing.T) {
srv := &Server{
queryOptions: &QueryOptions{
HostPort: ":-1",
HTTPHostPort: ":-1",
},
}
assert.Error(t, srv.Start())
Expand Down Expand Up @@ -600,7 +600,7 @@ func TestServerInUseHostPort(t *testing.T) {
}
}

func TestServer(t *testing.T) {
func TestServerSinglePort(t *testing.T) {
flagsSvc := flags.NewService(ports.QueryAdminHTTP)
flagsSvc.Logger = zap.NewNop()
hostPort := ports.GetAddressFromCLIOptions(ports.QueryHTTP, "")
Expand All @@ -612,7 +612,7 @@ func TestServer(t *testing.T) {
querySvc := querysvc.NewQueryService(spanReader, dependencyReader, querysvc.QueryServiceOptions{})

server, err := NewServer(flagsSvc.Logger, querySvc,
&QueryOptions{HostPort: hostPort, GRPCHostPort: hostPort, HTTPHostPort: hostPort, BearerTokenPropagation: true},
&QueryOptions{GRPCHostPort: hostPort, HTTPHostPort: hostPort, BearerTokenPropagation: true},
opentracing.NoopTracer{})
assert.Nil(t, err)
assert.NoError(t, server.Start())
Expand Down Expand Up @@ -661,7 +661,7 @@ func TestServerGracefulExit(t *testing.T) {

querySvc := &querysvc.QueryService{}
tracer := opentracing.NoopTracer{}
server, err := NewServer(flagsSvc.Logger, querySvc, &QueryOptions{HostPort: hostPort, GRPCHostPort: hostPort, HTTPHostPort: hostPort}, tracer)
server, err := NewServer(flagsSvc.Logger, querySvc, &QueryOptions{GRPCHostPort: hostPort, HTTPHostPort: hostPort}, tracer)
assert.Nil(t, err)
assert.NoError(t, server.Start())
go func() {
Expand All @@ -688,7 +688,7 @@ func TestServerHandlesPortZero(t *testing.T) {

querySvc := &querysvc.QueryService{}
tracer := opentracing.NoopTracer{}
server, err := NewServer(flagsSvc.Logger, querySvc, &QueryOptions{HostPort: ":0", GRPCHostPort: ":0", HTTPHostPort: ":0"}, tracer)
server, err := NewServer(flagsSvc.Logger, querySvc, &QueryOptions{GRPCHostPort: ":0", HTTPHostPort: ":0"}, tracer)
assert.Nil(t, err)
assert.NoError(t, server.Start())
server.Close()
Expand Down

0 comments on commit 1e1d244

Please sign in to comment.