Normalize CLI flags to use host:port addresses (#1827)

* Normalize CLI flags to use host:port addresses Signed-off-by: Annanay <[email protected]> * Addressed review comments Signed-off-by: Annanay <[email protected]> * Continue support for deprecated flags in all-in-one Signed-off-by: Annanay <[email protected]> * Nitpick, fix agent options in all-in-one Signed-off-by: Annanay <[email protected]> * Addressed comments, use HostPort in variable names Signed-off-by: Annanay <[email protected]> * Address comments Signed-off-by: Annanay <[email protected]> * Add Changelog entries Signed-off-by: Annanay <[email protected]> * Fix merge changes Signed-off-by: Annanay <[email protected]> * Checkpoint Signed-off-by: Annanay Agarwal <[email protected]> * Rebased and addressed comments Signed-off-by: Annanay <[email protected]> * Remove TChan ports, remove leftover files from merge Signed-off-by: Annanay <[email protected]> * Address comments Signed-off-by: Annanay <[email protected]> * Fix go.mod, edit util function name Signed-off-by: Annanay <[email protected]> * Fix test, print address in log Signed-off-by: Annanay <[email protected]> * Make fmt Signed-off-by: Annanay <[email protected]> * Add small test Signed-off-by: Yuri Shkuro <[email protected]> * Another silly test Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Annanay <[email protected]> Co-authored-by: Annanay Agarwal <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
jaegertracing · Mar 31, 2020 · 87b1910 · 87b1910
1 parent b1868a6
commit 87b1910
Show file tree

Hide file tree

Showing 14 changed files with 181 additions and 68 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -26,6 +26,16 @@ Changes by Version
     --collector.port
     ```
 
+* Normalize CLI flags to use host:port addresses ([#1827](https://github.com/jaegertracing/jaeger/pull/1827), [@annanay25](https://github.com/annanay25))
+
+  Flags previously accepting listen addresses in any other format have been deprecated:
+
+  * `collector.port` is superseded by `collector.tchan-server.host-port`
+  * `collector.http-port` is superseded by `collector.http-server.host-port`
+  * `collector.grpc-port` is superseded by `collector.grpc-server.host-port`
+  * `collector.zipkin.http-port` is superseded by `collector.zipkin.host-port`
+  * `admin-http-port` is superseded by `admin.http.host-port`
+
 #### New Features
 
 #### Bug fixes, Minor Improvements

diff --git a/cmd/all-in-one/main.go b/cmd/all-in-one/main.go
@@ -130,7 +130,7 @@ by default uses only in-memory database.`,
 			c.Start(cOpts)
 
 			// agent
-			grpcBuilder.CollectorHostPorts = append(grpcBuilder.CollectorHostPorts, fmt.Sprintf("127.0.0.1:%d", cOpts.CollectorGRPCPort))
+			grpcBuilder.CollectorHostPorts = append(grpcBuilder.CollectorHostPorts, cOpts.CollectorGRPCHostPort)
 			agentMetricsFactory := metricsFactory.Namespace(metrics.NSOptions{Name: "agent", Tags: nil})
 			builders := map[agentRep.Type]agentApp.CollectorProxyBuilder{
 				agentRep.GRPC: agentApp.GRPCCollectorProxyBuilder(grpcBuilder),

diff --git a/cmd/collector/app/builder_flags.go b/cmd/collector/app/builder_flags.go
@@ -17,6 +17,7 @@ package app
 
 import (
 	"flag"
+	"strings"
 
 	"github.com/spf13/viper"
 
@@ -31,10 +32,17 @@ const (
 	collectorNumWorkers           = "collector.num-workers"
 	collectorHTTPPort             = "collector.http-port"
 	collectorGRPCPort             = "collector.grpc-port"
+	collectorHTTPHostPort         = "collector.http-server.host-port"
+	collectorGRPCHostPort         = "collector.grpc-server.host-port"
+	collectorZipkinHTTPPort       = "collector.zipkin.http-port"
+	collectorZipkinHTTPHostPort   = "collector.zipkin.host-port"
 	collectorTags                 = "collector.tags"
-	collectorZipkinHTTPort        = "collector.zipkin.http-port"
 	collectorZipkinAllowedOrigins = "collector.zipkin.allowed-origins"
 	collectorZipkinAllowedHeaders = "collector.zipkin.allowed-headers"
+
+	collectorHTTPPortWarning       = "(deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later)"
+	collectorGRPCPortWarning       = "(deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later)"
+	collectorZipkinHTTPPortWarning = "(deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later)"
 )
 
 var tlsFlagsConfig = tlscfg.ServerFlagsConfig{
@@ -51,16 +59,16 @@ type CollectorOptions struct {
 	QueueSize int
 	// NumWorkers is the number of internal workers in a collector
 	NumWorkers int
-	// CollectorHTTPPort is the port that the collector service listens in on for http requests
-	CollectorHTTPPort int
-	// CollectorGRPCPort is the port that the collector service listens in on for gRPC requests
-	CollectorGRPCPort int
+	// CollectorHTTPHostPort is the host:port address that the collector service listens in on for http requests
+	CollectorHTTPHostPort string
+	// CollectorGRPCHostPort is the host:port address that the collector service listens in on for gRPC requests
+	CollectorGRPCHostPort string
 	// TLS configures secure transport
 	TLS tlscfg.Options
 	// CollectorTags is the string representing collector tags to append to each and every span
 	CollectorTags map[string]string
-	// CollectorZipkinHTTPPort is the port that the Zipkin collector service listens in on for http requests
-	CollectorZipkinHTTPPort int
+	// CollectorZipkinHTTPHostPort is the host:port address that the Zipkin collector service listens in on for http requests
+	CollectorZipkinHTTPHostPort string
 	// CollectorZipkinAllowedOrigins is a list of origins a cross-domain request to the Zipkin collector service can be executed from
 	CollectorZipkinAllowedOrigins string
 	// CollectorZipkinAllowedHeaders is a list of headers that the Zipkin collector service allowes the client to use with cross-domain requests
@@ -69,13 +77,16 @@ type CollectorOptions struct {
 
 // AddFlags adds flags for CollectorOptions
 func AddFlags(flags *flag.FlagSet) {
-	flags.Uint(collectorDynQueueSizeMemory, 0, "(experimental) The max memory size in MiB to use for the dynamic queue.")
 	flags.Int(collectorQueueSize, DefaultQueueSize, "The queue size of the collector")
 	flags.Int(collectorNumWorkers, DefaultNumWorkers, "The number of workers pulling items from the queue")
-	flags.Int(collectorHTTPPort, ports.CollectorHTTP, "The HTTP port for the collector service")
-	flags.Int(collectorGRPCPort, ports.CollectorGRPC, "The gRPC port for the collector service")
+	flags.Int(collectorHTTPPort, 0, collectorHTTPPortWarning+" see --"+collectorHTTPHostPort)
+	flags.Int(collectorGRPCPort, 0, collectorGRPCPortWarning+" see --"+collectorGRPCHostPort)
+	flags.Int(collectorZipkinHTTPPort, 0, collectorZipkinHTTPPortWarning+" see --"+collectorZipkinHTTPHostPort)
+	flags.String(collectorHTTPHostPort, ports.PortToHostPort(ports.CollectorHTTP), "The host:port (e.g. 127.0.0.1:5555 or :5555) of the collector's HTTP server")
+	flags.String(collectorGRPCHostPort, ports.PortToHostPort(ports.CollectorGRPC), "The host:port (e.g. 127.0.0.1:5555 or :5555) of the collector's GRPC server")
+	flags.String(collectorZipkinHTTPHostPort, ports.PortToHostPort(0), "The host:port (e.g. 127.0.0.1:5555 or :5555) of the collector's Zipkin server")
+	flags.Uint(collectorDynQueueSizeMemory, 0, "(experimental) The max memory size in MiB to use for the dynamic queue.")
 	flags.String(collectorTags, "", "One or more tags to be added to the Process tags of all spans passing through this collector. Ex: key1=value1,key2=${envVar:defaultValue}")
-	flags.Int(collectorZipkinHTTPort, 0, "The HTTP port for the Zipkin collector service e.g. 9411")
 	flags.String(collectorZipkinAllowedOrigins, "*", "Comma separated list of allowed origins for the Zipkin collector service, default accepts all")
 	flags.String(collectorZipkinAllowedHeaders, "content-type", "Comma separated list of allowed headers for the Zipkin collector service, default content-type")
 	tlsFlagsConfig.AddFlags(flags)
@@ -86,12 +97,25 @@ 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.CollectorHTTPPort = v.GetInt(collectorHTTPPort)
-	cOpts.CollectorGRPCPort = v.GetInt(collectorGRPCPort)
+	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.CollectorTags = flags.ParseJaegerTags(v.GetString(collectorTags))
-	cOpts.CollectorZipkinHTTPPort = v.GetInt(collectorZipkinHTTPort)
 	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
+}
diff --git a/cmd/collector/app/builder_flags_test.go b/cmd/collector/app/builder_flags_test.go
@@ -0,0 +1,57 @@
+// Copyright (c) 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.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package app
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/jaegertracing/jaeger/pkg/config"
+)
+
+func TestCollectorOptionsWithFlags_CheckHostPort(t *testing.T) {
+	c := &CollectorOptions{}
+	v, command := config.Viperize(AddFlags)
+	command.ParseFlags([]string{
+		"--collector.http-server.host-port=5678",
+		"--collector.grpc-server.host-port=1234",
+		"--collector.zipkin.host-port=3456",
+	})
+	c.InitFromViper(v)
+
+	assert.Equal(t, ":5678", c.CollectorHTTPHostPort)
+	assert.Equal(t, ":1234", c.CollectorGRPCHostPort)
+	assert.Equal(t, ":3456", c.CollectorZipkinHTTPHostPort)
+}
+
+func TestCollectorOptionsWithFlags_CheckFullHostPort(t *testing.T) {
+	c := &CollectorOptions{}
+	v, command := config.Viperize(AddFlags)
+	command.ParseFlags([]string{
+		"--collector.http-server.host-port=:5678",
+		"--collector.grpc-server.host-port=127.0.0.1:1234",
+		"--collector.zipkin.host-port=0.0.0.0:3456",
+	})
+	c.InitFromViper(v)
+
+	assert.Equal(t, ":5678", c.CollectorHTTPHostPort)
+	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, ""))
+}
diff --git a/cmd/collector/app/collector.go b/cmd/collector/app/collector.go
@@ -83,7 +83,7 @@ func (c *Collector) Start(builderOpts *CollectorOptions) error {
 	c.spanHandlers = handlerBuilder.BuildHandlers(c.spanProcessor)
 
 	if grpcServer, err := server.StartGRPCServer(&server.GRPCServerParams{
-		Port:          builderOpts.CollectorGRPCPort,
+		HostPort:      builderOpts.CollectorGRPCHostPort,
 		Handler:       c.spanHandlers.GRPCHandler,
 		TLSConfig:     builderOpts.TLS,
 		SamplingStore: c.strategyStore,
@@ -95,7 +95,7 @@ func (c *Collector) Start(builderOpts *CollectorOptions) error {
 	}
 
 	if httpServer, err := server.StartHTTPServer(&server.HTTPServerParams{
-		Port:           builderOpts.CollectorHTTPPort,
+		HostPort:       builderOpts.CollectorHTTPHostPort,
 		Handler:        c.spanHandlers.JaegerBatchesHandler,
 		HealthCheck:    c.hCheck,
 		MetricsFactory: c.metricsFactory,
@@ -108,7 +108,7 @@ func (c *Collector) Start(builderOpts *CollectorOptions) error {
 	}
 
 	if zkServer, err := server.StartZipkinServer(&server.ZipkinServerParams{
-		Port:           builderOpts.CollectorZipkinHTTPPort,
+		HostPort:       builderOpts.CollectorZipkinHTTPHostPort,
 		Handler:        c.spanHandlers.ZipkinSpansHandler,
 		AllowedHeaders: builderOpts.CollectorZipkinAllowedHeaders,
 		AllowedOrigins: builderOpts.CollectorZipkinAllowedOrigins,

diff --git a/cmd/collector/app/server/grpc.go b/cmd/collector/app/server/grpc.go
@@ -17,7 +17,6 @@ package server
 import (
 	"fmt"
 	"net"
-	"strconv"
 
 	"go.uber.org/zap"
 	"google.golang.org/grpc"
@@ -33,7 +32,7 @@ import (
 // GRPCServerParams to construct a new Jaeger Collector gRPC Server
 type GRPCServerParams struct {
 	TLSConfig     tlscfg.Options
-	Port          int
+	HostPort      string
 	Handler       *handler.GRPCHandler
 	SamplingStore strategystore.StrategyStore
 	Logger        *zap.Logger
@@ -58,8 +57,7 @@ func StartGRPCServer(params *GRPCServerParams) (*grpc.Server, error) {
 		server = grpc.NewServer()
 	}
 
-	grpcPortStr := ":" + strconv.Itoa(params.Port)
-	listener, err := net.Listen("tcp", grpcPortStr)
+	listener, err := net.Listen("tcp", params.HostPort)
 	if err != nil {
 		return nil, fmt.Errorf("failed to listen on gRPC port: %w", err)
 	}
@@ -75,7 +73,7 @@ func serveGRPC(server *grpc.Server, listener net.Listener, params *GRPCServerPar
 	api_v2.RegisterCollectorServiceServer(server, params.Handler)
 	api_v2.RegisterSamplingManagerServer(server, sampling.NewGRPCHandler(params.SamplingStore))
 
-	params.Logger.Info("Starting jaeger-collector gRPC server", zap.Int("grpc-port", params.Port))
+	params.Logger.Info("Starting jaeger-collector gRPC server", zap.String("grpc.host-port", params.HostPort))
 	go func() {
 		if err := server.Serve(listener); err != nil {
 			params.Logger.Error("Could not launch gRPC service", zap.Error(err))

diff --git a/cmd/collector/app/server/grpc_test.go b/cmd/collector/app/server/grpc_test.go
@@ -36,7 +36,7 @@ import (
 func TestFailToListen(t *testing.T) {
 	logger, _ := zap.NewDevelopment()
 	server, err := StartGRPCServer(&GRPCServerParams{
-		Port:          -1,
+		HostPort:      ":-1",
 		Handler:       handler.NewGRPCHandler(logger, &mockSpanProcessor{}),
 		SamplingStore: &mockSamplingStore{},
 		Logger:        logger,

diff --git a/cmd/collector/app/server/http.go b/cmd/collector/app/server/http.go
@@ -17,7 +17,6 @@ package server
 import (
 	"net"
 	"net/http"
-	"strconv"
 
 	"github.com/gorilla/mux"
 	"github.com/uber/jaeger-lib/metrics"
@@ -32,7 +31,7 @@ import (
 
 // HTTPServerParams to construct a new Jaeger Collector HTTP Server
 type HTTPServerParams struct {
-	Port           int
+	HostPort       string
 	Handler        handler.JaegerBatchesHandler
 	SamplingStore  strategystore.StrategyStore
 	MetricsFactory metrics.Factory
@@ -42,15 +41,14 @@ type HTTPServerParams struct {
 
 // StartHTTPServer based on the given parameters
 func StartHTTPServer(params *HTTPServerParams) (*http.Server, error) {
-	httpPortStr := ":" + strconv.Itoa(params.Port)
-	params.Logger.Info("Starting jaeger-collector HTTP server", zap.String("http-host-port", httpPortStr))
+	params.Logger.Info("Starting jaeger-collector HTTP server", zap.String("http host-port", params.HostPort))
 
-	listener, err := net.Listen("tcp", httpPortStr)
+	listener, err := net.Listen("tcp", params.HostPort)
 	if err != nil {
 		return nil, err
 	}
 
-	server := &http.Server{Addr: httpPortStr}
+	server := &http.Server{Addr: params.HostPort}
 	serveHTTP(server, listener, params)
 
 	return server, nil

diff --git a/cmd/collector/app/server/zipkin.go b/cmd/collector/app/server/zipkin.go
@@ -17,7 +17,6 @@ package server
 import (
 	"net"
 	"net/http"
-	"strconv"
 	"strings"
 
 	"github.com/gorilla/mux"
@@ -32,7 +31,7 @@ import (
 
 // ZipkinServerParams to construct a new Jaeger Collector Zipkin Server
 type ZipkinServerParams struct {
-	Port           int
+	HostPort       string
 	Handler        handler.ZipkinSpansHandler
 	AllowedOrigins string
 	AllowedHeaders string
@@ -42,19 +41,18 @@ type ZipkinServerParams struct {
 
 // StartZipkinServer based on the given parameters
 func StartZipkinServer(params *ZipkinServerParams) (*http.Server, error) {
-	if params.Port == 0 {
+	if params.HostPort == "" {
 		return nil, nil
 	}
 
-	httpPortStr := ":" + strconv.Itoa(params.Port)
-	params.Logger.Info("Listening for Zipkin HTTP traffic", zap.Int("zipkin.http-port", params.Port))
+	params.Logger.Info("Listening for Zipkin HTTP traffic", zap.String("zipkin host-port", params.HostPort))
 
-	listener, err := net.Listen("tcp", httpPortStr)
+	listener, err := net.Listen("tcp", params.HostPort)
 	if err != nil {
 		return nil, err
 	}
 
-	server := &http.Server{Addr: httpPortStr}
+	server := &http.Server{Addr: params.HostPort}
 	serveZipkin(server, listener, params)
 
 	return server, nil