diff --git a/CHANGELOG.md b/CHANGELOG.md index 7dafd1629e9..92c02730cff 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/cmd/query/app/flags.go b/cmd/query/app/flags.go index 9adbf70a72f..fb9c29c80df 100644 --- a/cmd/query/app/flags.go +++ b/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) diff --git a/cmd/query/app/flags_test.go b/cmd/query/app/flags_test.go index 5ed081cc9e1..30c717c7712 100644 --- a/cmd/query/app/flags_test.go +++ b/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) - } }) } diff --git a/cmd/query/app/server.go b/cmd/query/app/server.go index 37da8b9f59a..f9aa0e82162 100644 --- a/cmd/query/app/server.go +++ b/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 diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 18807fa38ad..c0c1694e10a 100644 --- a/cmd/query/app/server_test.go +++ b/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()