Remove deprecated Query Server flags

CHANGELOG.md

@@ -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`
@@ -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
 

cmd/query/app/flags.go

@@ -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"
@@ -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")
@@ -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)

cmd/query/app/flags_test.go

@@ -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",
@@ -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"},
@@ -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",
 		},
 	}
 
@@ -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)
-			}
 
 		})
 	}

cmd/query/app/server.go

@@ -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
 	}
@@ -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)
@@ -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
 
@@ -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

cmd/query/app/server_test.go

@@ -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())
@@ -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, "")
@@ -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())
@@ -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() {
@@ -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()