From 8e1ad3f3a4a598dbeb395576a7d6a7a812fc5fc4 Mon Sep 17 00:00:00 2001 From: rjs211 Date: Wed, 3 Feb 2021 08:25:03 +0530 Subject: [PATCH 1/7] Removing depricated query server flags Signed-off-by: rjs211 --- cmd/query/app/flags.go | 27 ++-------- cmd/query/app/flags_test.go | 98 +++++++++++------------------------- cmd/query/app/server.go | 8 ++- cmd/query/app/server_test.go | 8 +-- 4 files changed, 39 insertions(+), 102 deletions(-) diff --git a/cmd/query/app/flags.go b/cmd/query/app/flags.go index 9adbf70a72f..c809a586c55 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(queryGRPCHostPort, ports.PortToHostPort(ports.QueryHTTP), "The host:port (e.g. 127.0.0.1:14250 or :14250) of the query's gRPC server") 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") @@ -109,26 +103,13 @@ func AddFlags(flagSet *flag.FlagSet) { func (qOpts *QueryOptions) InitPortsConfigFromViper(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)) + // if TLS is enabled, use different default host-port for GRPC endpoint + if (qOpts.TLSGRPC.Enabled || qOpts.TLSHTTP.Enabled) && !v.IsSet(queryGRPCHostPort) { + qOpts.GRPCHostPort = ports.PortToHostPort(ports.QueryGRPC) } - qOpts.HTTPHostPort = qOpts.HostPort - qOpts.GRPCHostPort = qOpts.HostPort return qOpts } diff --git a/cmd/query/app/flags_test.go b/cmd/query/app/flags_test.go index 5ed081cc9e1..e5e074ca52a 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,91 +129,56 @@ 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", - expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper - verifyCommonPort: false, + // Backward compatible default behavior. Since TLS is disabled, Common host-port is used for both HTTP and GRPC endpoints + name: "No host-port flags specified, both GRPC and HTTP disabled", + flagsArray: []string{}, + expectedHTTPHostPort: ports.PortToHostPort(ports.QueryHTTP), // fallback in viper + expectedGRPCHostPort: ports.PortToHostPort(ports.QueryHTTP), // fallback in viper + verifyCommonPort: true, }, { - // 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", + // If any one host-port is specified, and TLS is diabled, fallback to ports.QueryHTTP + 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", - "--query.host-port=127.0.0.1:8080", }, - expectedHTTPHostPort: "127.0.0.1:8080", - expectedGRPCHostPort: "127.0.0.1:8080", + expectedHTTPHostPort: "127.0.0.1:8081", + expectedGRPCHostPort: ports.PortToHostPort(ports.QueryHTTP), // fallback in viper 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", + // Since TLS is enabled in atleast one server, and no host-port flags are explicitly set, + // dedicated host-port are assigned during initilaizatiopn + name: "No dedicated 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", }, - expectedHTTPHostPort: "127.0.0.1:8081", - expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper + expectedHTTPHostPort: ports.PortToHostPort(ports.QueryHTTP), // fallback in viper + expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // initialization logic 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", + // Since TLS is enabled in atleast one server, and HTTP host-port is specified, the dedicated GRPC host-port is set during initialization + name: "HTTP 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", }, expectedHTTPHostPort: "127.0.0.1:8081", - expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper + expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // initialization logic 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", + // Since TLS is enabled in atleast one server, and HTTP host-port is specified, the dedicated HTTP host-port is obtained from viper + name: "GRPC host-port is specified, atleast one of GRPC, HTTP TLS enabled", flagsArray: []string{ "--query.grpc.tls.enabled=true", + "--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 + expectedHTTPHostPort: ports.PortToHostPort(ports.QueryHTTP), // fallback in viper + expectedGRPCHostPort: "127.0.0.1:8081", 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 - }, } for _, test := range flagPortCases { 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..93c3cb50a8a 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()) @@ -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() From 64c0e25db92e322c39e776d30ff099e3365daf01 Mon Sep 17 00:00:00 2001 From: rjs211 Date: Wed, 3 Feb 2021 08:31:17 +0530 Subject: [PATCH 2/7] modify comments to make things more clear Signed-off-by: rjs211 --- ports/ports.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ports/ports.go b/ports/ports.go index 6ab2e997f81..10abb75b592 100644 --- a/ports/ports.go +++ b/ports/ports.go @@ -38,7 +38,8 @@ const ( // CollectorAdminHTTP is the default admin HTTP port (health check, metrics, etc.) CollectorAdminHTTP = 14269 - // QueryGRPC is the default port of GRPC requests for Query trace retrieval + // QueryGRPC is the default port of GRPC requests for Query trace retrieval when TLS is enabled. + // When TLS is disabled, QueryHTTP is used for both GRPC and HTTP endpoints QueryGRPC = 16685 // QueryHTTP is the default port for UI and Query API (e.g. /api/* endpoints) QueryHTTP = 16686 From 9f24b16fbae8aaa9ba1ebf83f4bfb03733600dd2 Mon Sep 17 00:00:00 2001 From: rjs211 Date: Wed, 3 Feb 2021 09:18:58 +0530 Subject: [PATCH 3/7] adding changelog, correcting comments Signed-off-by: rjs211 --- CHANGELOG.md | 10 +++++----- ports/ports.go | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7dafd1629e9..0fd2d21ac54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ 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` and GRPC `--query.grpc-server.host-port` host-ports flags instead ([#2772](https://github.com/jaegertracing/jaeger/pull/2772), [@rjs211](https://github.com/rjs211)) + * 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 +23,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 in either or both of GRPC or HTTP endpoints, `--query.http-server.host-port`defaults to `:16686` and `--query.grpc-server.host-port` defaults to `:16685` + * If TLS is disabled in both endpoints (default), both `--query.http-server.host-port` and `--query.grpc-server.host-port` default to `:16686` ### UI Changes diff --git a/ports/ports.go b/ports/ports.go index 10abb75b592..68d8412d328 100644 --- a/ports/ports.go +++ b/ports/ports.go @@ -38,8 +38,8 @@ const ( // CollectorAdminHTTP is the default admin HTTP port (health check, metrics, etc.) CollectorAdminHTTP = 14269 - // QueryGRPC is the default port of GRPC requests for Query trace retrieval when TLS is enabled. - // When TLS is disabled, QueryHTTP is used for both GRPC and HTTP endpoints + // QueryGRPC is the default port of GRPC requests for Query trace retrieval when TLS is enabled + // When TLS is disabled, QueryHTTP is used as default fallback for both GRPC and HTTP endpoints QueryGRPC = 16685 // QueryHTTP is the default port for UI and Query API (e.g. /api/* endpoints) QueryHTTP = 16686 From d58c20c6e0f35cffe29cb8028e2d694768f4d2b8 Mon Sep 17 00:00:00 2001 From: rjs211 Date: Thu, 4 Feb 2021 06:46:19 +0530 Subject: [PATCH 4/7] changing default behaviour of host-port flags Signed-off-by: rjs211 --- cmd/query/app/flags.go | 18 +++------------ cmd/query/app/flags_test.go | 44 ++++++++----------------------------- 2 files changed, 12 insertions(+), 50 deletions(-) diff --git a/cmd/query/app/flags.go b/cmd/query/app/flags.go index c809a586c55..fb9c29c80df 100644 --- a/cmd/query/app/flags.go +++ b/cmd/query/app/flags.go @@ -89,7 +89,7 @@ type QueryOptions struct { func AddFlags(flagSet *flag.FlagSet) { flagSet.Var(&config.StringSlice{}, queryAdditionalHeaders, `Additional HTTP response headers. Can be specified multiple times. Format: "Key: Value"`) 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.QueryHTTP), "The host:port (e.g. 127.0.0.1:14250 or :14250) of the query's gRPC 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.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") @@ -99,24 +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.TLSGRPC = tlsGRPCFlagsConfig.InitFromViper(v) qOpts.TLSHTTP = tlsHTTPFlagsConfig.InitFromViper(v) - - // if TLS is enabled, use different default host-port for GRPC endpoint - if (qOpts.TLSGRPC.Enabled || qOpts.TLSHTTP.Enabled) && !v.IsSet(queryGRPCHostPort) { - qOpts.GRPCHostPort = ports.PortToHostPort(ports.QueryGRPC) - } - 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 e5e074ca52a..30c717c7712 100644 --- a/cmd/query/app/flags_test.go +++ b/cmd/query/app/flags_test.go @@ -129,55 +129,32 @@ func TestQueryOptionsPortAllocationFromFlags(t *testing.T) { expectedHostPort string }{ { - // Backward compatible default behavior. Since TLS is disabled, Common host-port is used for both HTTP and GRPC endpoints - name: "No host-port flags specified, both GRPC and HTTP disabled", + // 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.QueryHTTP), // fallback in viper - verifyCommonPort: true, + expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper }, { - // If any one host-port is specified, and TLS is diabled, fallback to ports.QueryHTTP + // 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.QueryHTTP), // fallback in viper - verifyCommonPort: true, + expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // fallback in viper }, { - // Since TLS is enabled in atleast one server, and no host-port flags are explicitly set, - // dedicated host-port are assigned during initilaizatiopn - name: "No dedicated host-port is specified, atleast one of GRPC, HTTP TLS enabled", - flagsArray: []string{ - "--query.grpc.tls.enabled=true", - }, - expectedHTTPHostPort: ports.PortToHostPort(ports.QueryHTTP), // fallback in viper - expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // initialization logic - verifyCommonPort: false, - }, - { - // Since TLS is enabled in atleast one server, and HTTP host-port is specified, the dedicated GRPC host-port is set during initialization - name: "HTTP host-port is 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", - }, - expectedHTTPHostPort: "127.0.0.1:8081", - expectedGRPCHostPort: ports.PortToHostPort(ports.QueryGRPC), // initialization logic - verifyCommonPort: false, - }, - { - // Since TLS is enabled in atleast one server, and HTTP host-port is specified, the dedicated HTTP host-port is obtained from viper - name: "GRPC host-port is specified, atleast one of GRPC, HTTP TLS enabled", - flagsArray: []string{ - "--query.grpc.tls.enabled=true", "--query.grpc-server.host-port=127.0.0.1:8081", }, - expectedHTTPHostPort: ports.PortToHostPort(ports.QueryHTTP), // fallback in viper + expectedHTTPHostPort: "127.0.0.1:8081", expectedGRPCHostPort: "127.0.0.1:8081", - verifyCommonPort: false, }, } @@ -189,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) - } }) } From 0633da08bbd2d6e0ec73270c355d9ea47cb3d72b Mon Sep 17 00:00:00 2001 From: rjs211 Date: Thu, 4 Feb 2021 06:50:02 +0530 Subject: [PATCH 5/7] pruning test cases, renaming test method Signed-off-by: rjs211 --- cmd/query/app/server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 93c3cb50a8a..c0c1694e10a 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -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, "") From 9ff874fb7378b867d09865a5d6f811e5a035aaeb Mon Sep 17 00:00:00 2001 From: rjs211 Date: Thu, 4 Feb 2021 06:50:57 +0530 Subject: [PATCH 6/7] changing comments for host-port default Signed-off-by: rjs211 --- ports/ports.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ports/ports.go b/ports/ports.go index 68d8412d328..6ab2e997f81 100644 --- a/ports/ports.go +++ b/ports/ports.go @@ -38,8 +38,7 @@ const ( // CollectorAdminHTTP is the default admin HTTP port (health check, metrics, etc.) CollectorAdminHTTP = 14269 - // QueryGRPC is the default port of GRPC requests for Query trace retrieval when TLS is enabled - // When TLS is disabled, QueryHTTP is used as default fallback for both GRPC and HTTP endpoints + // QueryGRPC is the default port of GRPC requests for Query trace retrieval QueryGRPC = 16685 // QueryHTTP is the default port for UI and Query API (e.g. /api/* endpoints) QueryHTTP = 16686 From 0c058fa697a8d5acb30f9fa60bb0bc6013b507bf Mon Sep 17 00:00:00 2001 From: rjs211 Date: Thu, 4 Feb 2021 06:52:23 +0530 Subject: [PATCH 7/7] changelog for default query server behaviour change Signed-off-by: rjs211 --- CHANGELOG.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fd2d21ac54..92c02730cff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,8 @@ 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` and GRPC `--query.grpc-server.host-port` host-ports flags instead ([#2772](https://github.com/jaegertracing/jaeger/pull/2772), [@rjs211](https://github.com/rjs211)) +* 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` @@ -23,10 +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), [#2772](https://github.com/jaegertracing/jaeger/pull/2772), [@rjs211](https://github.com/rjs211)) +* 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 in enabled in either or both of GRPC or HTTP endpoints, `--query.http-server.host-port`defaults to `:16686` and `--query.grpc-server.host-port` defaults to `:16685` - * If TLS is disabled in both endpoints (default), both `--query.http-server.host-port` and `--query.grpc-server.host-port` default to `:16686` + * 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