Normalize query service CLI flags to use host:port addresses (#2212)

* Normalize CLI flags to use host:port addresses

Signed-off-by: ohdearaugustin <j-reindl@live.at>

* revert change to master

Signed-off-by: ohdearaugustin <j-reindl@live.at>

* refactor getAddressFromCLIOptions

Signed-off-by: ohdearaugustin <j-reindl@live.at>

* cleanup flags

Signed-off-by: ohdearaugustin <j-reindl@live.at>

* add GetAddressFromCLIOptionHostPort test

Signed-off-by: ohdearaugustin <j-reindl@live.at>

* add port to logging and fix tests

Signed-off-by: ohdearaugustin <j-reindl@live.at>

* cleanup tests

Signed-off-by: ohdearaugustin <j-reindl@live.at>

* remove comment

Signed-off-by: ohdearaugustin <j-reindl@live.at>

* add test for port only

Signed-off-by: ohdearaugustin <j-reindl@live.at>

* fix deprecation message

Signed-off-by: ohdearaugustin <j-reindl@live.at>

* remove port option

Signed-off-by: ohdearaugustin <j-reindl@live.at>

* Fix var name

Signed-off-by: Yuri Shkuro <ys@uber.com>

Co-authored-by: Yuri Shkuro <ys@uber.com>

cmd/collector/app/builder_flags.go

@@ -17,7 +17,6 @@ package app
 
 import (
 	"flag"
-	"strings"
 
 	"github.com/spf13/viper"
 
@@ -110,25 +109,12 @@ func (cOpts *CollectorOptions) InitFromViper(v *viper.Viper) *CollectorOptions {
 	cOpts.DynQueueSizeMemory = v.GetUint(collectorDynQueueSizeMemory) * 1024 * 1024 // we receive in MiB and store in bytes
 	cOpts.QueueSize = v.GetInt(collectorQueueSize)
 	cOpts.NumWorkers = v.GetInt(collectorNumWorkers)
-	cOpts.CollectorHTTPHostPort = getAddressFromCLIOptions(v.GetInt(collectorHTTPPort), v.GetString(CollectorHTTPHostPort))
-	cOpts.CollectorGRPCHostPort = getAddressFromCLIOptions(v.GetInt(collectorGRPCPort), v.GetString(CollectorGRPCHostPort))
-	cOpts.CollectorZipkinHTTPHostPort = getAddressFromCLIOptions(v.GetInt(collectorZipkinHTTPPort), v.GetString(CollectorZipkinHTTPHostPort))
+	cOpts.CollectorHTTPHostPort = ports.GetAddressFromCLIOptions(v.GetInt(collectorHTTPPort), v.GetString(CollectorHTTPHostPort))
+	cOpts.CollectorGRPCHostPort = ports.GetAddressFromCLIOptions(v.GetInt(collectorGRPCPort), v.GetString(CollectorGRPCHostPort))
+	cOpts.CollectorZipkinHTTPHostPort = ports.GetAddressFromCLIOptions(v.GetInt(collectorZipkinHTTPPort), v.GetString(CollectorZipkinHTTPHostPort))
 	cOpts.CollectorTags = flags.ParseJaegerTags(v.GetString(collectorTags))
 	cOpts.CollectorZipkinAllowedOrigins = v.GetString(collectorZipkinAllowedOrigins)
 	cOpts.CollectorZipkinAllowedHeaders = v.GetString(collectorZipkinAllowedHeaders)
 	cOpts.TLS = tlsFlagsConfig.InitFromViper(v)
 	return cOpts
 }
-
-// Utility function to get listening address based on port (deprecated flags) or host:port (new flags)
-func getAddressFromCLIOptions(port int, hostPort string) string {
-	if port != 0 {
-		return ports.PortToHostPort(port)
-	}
-
-	if strings.Contains(hostPort, ":") {
-		return hostPort
-	}
-
-	return ":" + hostPort
-}

cmd/collector/app/builder_flags_test.go

@@ -51,7 +51,3 @@ func TestCollectorOptionsWithFlags_CheckFullHostPort(t *testing.T) {
 	assert.Equal(t, "127.0.0.1:1234", c.CollectorGRPCHostPort)
 	assert.Equal(t, "0.0.0.0:3456", c.CollectorZipkinHTTPHostPort)
 }
-
-func Test_getAddressFromCLIOptions(t *testing.T) {
-	assert.Equal(t, ":123", getAddressFromCLIOptions(123, ""))
-}

cmd/query/app/flags.go

@@ -36,7 +36,9 @@ 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)"
 	queryBasePath           = "query.base-path"
 	queryStaticFiles        = "query.static-files"
 	queryUIConfig           = "query.ui-config"
@@ -47,8 +49,8 @@ const (
 
 // QueryOptions holds configuration for query service
 type QueryOptions struct {
-	// Port is the port that the query service listens in on
-	Port int
+	// HostPort is the host:port address that the query service listens o n
+	HostPort string
 	// BasePath is the prefix for all UI and API HTTP routes
 	BasePath string
 	// StaticAssets is the path for the static assets for the UI (https://github.com/uber/jaeger-ui)
@@ -66,7 +68,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.Int(queryPort, ports.QueryHTTP, "The port for the query service")
+	flagSet.String(queryHostPort, ports.PortToHostPort(ports.QueryHTTP), "The host:port (e.g. 127.0.0.1:5555 or :5555) of the query's HTTP 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")
@@ -76,7 +79,7 @@ func AddFlags(flagSet *flag.FlagSet) {
 
 // InitFromViper initializes QueryOptions with properties from viper
 func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) *QueryOptions {
-	qOpts.Port = v.GetInt(queryPort)
+	qOpts.HostPort = ports.GetAddressFromCLIOptions(v.GetInt(queryPort), v.GetString(queryHostPort))
 	qOpts.BasePath = v.GetString(queryBasePath)
 	qOpts.StaticAssets = v.GetString(queryStaticFiles)
 	qOpts.UIConfig = v.GetString(queryUIConfig)

cmd/query/app/flags_test.go

@@ -28,13 +28,22 @@ 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.port=80",
+		"--query.host-port=127.0.0.1:8080",
 		"--query.additional-headers=access-control-allow-origin:blerg",
 		"--query.additional-headers=whatever:thing",
 		"--query.max-clock-skew-adjustment=10s",
@@ -43,7 +52,7 @@ 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, 80, qOpts.Port)
+	assert.Equal(t, "127.0.0.1:8080", qOpts.HostPort)
 	assert.Equal(t, http.Header{
 		"Access-Control-Allow-Origin": []string{"blerg"},
 		"Whatever":                    []string{"thing"},

cmd/query/app/server.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2019 The Jaeger Authors.
+// Copyright (c) 2019,2020 The Jaeger Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -15,7 +15,6 @@
 package app
 
 import (
-	"fmt"
 	"net"
 	"net/http"
 	"strings"
@@ -95,20 +94,21 @@ func createHTTPServer(querySvc *querysvc.QueryService, queryOpts *QueryOptions,
 
 // Start http, GRPC and cmux servers concurrently
 func (s *Server) Start() error {
-	conn, err := net.Listen("tcp", fmt.Sprintf(":%d", s.queryOptions.Port))
+	conn, err := net.Listen("tcp", s.queryOptions.HostPort)
 	if err != nil {
 		return err
 	}
 	s.conn = conn
 
-	tcpPort := s.queryOptions.Port
+	var tcpPort int
 	if port, err := netutils.GetPort(s.conn.Addr()); err == nil {
 		tcpPort = port
 	}
 
 	s.svc.Logger.Info(
 		"Query server started",
-		zap.Int("port", tcpPort))
+		zap.Int("port", tcpPort),
+		zap.String("addr", s.queryOptions.HostPort))
 
 	// cmux server acts as a reverse-proxy between HTTP and GRPC backends.
 	cmuxServer := cmux.New(s.conn)
@@ -120,7 +120,7 @@ func (s *Server) Start() error {
 	httpListener := cmuxServer.Match(cmux.Any())
 
 	go func() {
-		s.svc.Logger.Info("Starting HTTP server", zap.Int("port", tcpPort))
+		s.svc.Logger.Info("Starting HTTP server", zap.Int("port", tcpPort), zap.String("addr", s.queryOptions.HostPort))
 
 		switch err := s.httpServer.Serve(httpListener); err {
 		case nil, http.ErrServerClosed, cmux.ErrListenerClosed:
@@ -133,7 +133,7 @@ func (s *Server) Start() error {
 
 	// Start GRPC server concurrently
 	go func() {
-		s.svc.Logger.Info("Starting GRPC server", zap.Int("port", tcpPort))
+		s.svc.Logger.Info("Starting GRPC server", zap.Int("port", tcpPort), zap.String("addr", s.queryOptions.HostPort))
 
 		if err := s.grpcServer.Serve(grpcListener); err != nil {
 			s.svc.Logger.Error("Could not start GRPC server", zap.Error(err))
@@ -143,7 +143,7 @@ func (s *Server) Start() error {
 
 	// Start cmux server concurrently.
 	go func() {
-		s.svc.Logger.Info("Starting CMUX server", zap.Int("port", tcpPort))
+		s.svc.Logger.Info("Starting CMUX server", zap.Int("port", tcpPort), zap.String("addr", s.queryOptions.HostPort))
 
 		err := cmuxServer.Serve()
 		// TODO: Remove string comparison when https://github.com/soheilhy/cmux/pull/69 is merged

cmd/query/app/server_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2019 The Jaeger Authors.
+// Copyright (c) 2019,2020 The Jaeger Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -16,7 +16,6 @@ package app
 
 import (
 	"context"
-	"fmt"
 	"testing"
 	"time"
 
@@ -38,7 +37,7 @@ import (
 func TestServerError(t *testing.T) {
 	srv := &Server{
 		queryOptions: &QueryOptions{
-			Port: -1,
+			HostPort: ":-1",
 		},
 	}
 	assert.Error(t, srv.Start())
@@ -47,6 +46,7 @@ func TestServerError(t *testing.T) {
 func TestServer(t *testing.T) {
 	flagsSvc := flags.NewService(ports.QueryAdminHTTP)
 	flagsSvc.Logger = zap.NewNop()
+	hostPort := ports.GetAddressFromCLIOptions(ports.QueryHTTP, "")
 
 	spanReader := &spanstoremocks.Reader{}
 	dependencyReader := &depsmocks.Reader{}
@@ -56,11 +56,11 @@ func TestServer(t *testing.T) {
 	querySvc := querysvc.NewQueryService(spanReader, dependencyReader, querysvc.QueryServiceOptions{})
 
 	server := NewServer(flagsSvc, querySvc,
-		&QueryOptions{Port: ports.QueryHTTP, BearerTokenPropagation: true},
+		&QueryOptions{HostPort: hostPort, BearerTokenPropagation: true},
 		opentracing.NoopTracer{})
 	assert.NoError(t, server.Start())
 
-	client := newGRPCClient(t, fmt.Sprintf(":%d", ports.QueryHTTP))
+	client := newGRPCClient(t, hostPort)
 	defer client.conn.Close()
 
 	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
@@ -90,7 +90,7 @@ func TestServerGracefulExit(t *testing.T) {
 
 	querySvc := &querysvc.QueryService{}
 	tracer := opentracing.NoopTracer{}
-	server := NewServer(flagsSvc, querySvc, &QueryOptions{Port: ports.QueryAdminHTTP}, tracer)
+	server := NewServer(flagsSvc, querySvc, &QueryOptions{HostPort: ports.PortToHostPort(ports.QueryAdminHTTP)}, tracer)
 	assert.NoError(t, server.Start())
 
 	// Wait for servers to come up before we can call .Close()
@@ -111,14 +111,14 @@ func TestServerHandlesPortZero(t *testing.T) {
 
 	querySvc := &querysvc.QueryService{}
 	tracer := opentracing.NoopTracer{}
-	server := NewServer(flagsSvc, querySvc, &QueryOptions{Port: 0}, tracer)
+	server := NewServer(flagsSvc, querySvc, &QueryOptions{HostPort: ":0"}, tracer)
 	assert.NoError(t, server.Start())
 	server.Close()
 
 	message := logs.FilterMessage("Query server started")
 	assert.Equal(t, 1, message.Len(), "Expected query started log message.")
 
 	onlyEntry := message.All()[0]
-	port := onlyEntry.ContextMap()["port"].(int64)
+	port := onlyEntry.ContextMap()["port"]
 	assert.Greater(t, port, int64(0))
 }

ports/ports.go

@@ -16,6 +16,7 @@ package ports
 
 import (
 	"strconv"
+	"strings"
 )
 
 const (
@@ -50,3 +51,16 @@ const (
 func PortToHostPort(port int) string {
 	return ":" + strconv.Itoa(port)
 }
+
+// GetAddressFromCLIOptions gets listening address based on port (deprecated flags) or host:port (new flags)
+func GetAddressFromCLIOptions(port int, hostPort string) string {
+	if port != 0 {
+		return PortToHostPort(port)
+	}
+
+	if strings.Contains(hostPort, ":") {
+		return hostPort
+	}
+
+	return ":" + hostPort
+}

ports/ports_test.go

@@ -23,3 +23,19 @@ import (
 func TestPortToHostPort(t *testing.T) {
 	assert.Equal(t, ":42", PortToHostPort(42))
 }
+
+func TestGetAddressFromCLIOptionsLegacy(t *testing.T) {
+	assert.Equal(t, ":123", GetAddressFromCLIOptions(123, ""))
+}
+
+func TestGetAddressFromCLIOptionHostPort(t *testing.T) {
+	assert.Equal(t, "127.0.0.1:123", GetAddressFromCLIOptions(0, "127.0.0.1:123"))
+}
+
+func TestGetAddressFromCLIOptionOnlyPort(t *testing.T) {
+	assert.Equal(t, ":123", GetAddressFromCLIOptions(0, ":123"))
+}
+
+func TestGetAddressFromCLIOptionOnlyPortWithoutColon(t *testing.T) {
+	assert.Equal(t, ":123", GetAddressFromCLIOptions(0, "123"))
+}