Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove deprecated Query Server flags #2772

Merged
merged 9 commits into from
Feb 4, 2021
Merged

Remove deprecated Query Server flags #2772

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8e1ad3f
Removing depricated query server flags
rjs211 Feb 3, 2021
64c0e25
modify comments to make things more clear
rjs211 Feb 3, 2021
9f24b16
adding changelog, correcting comments
rjs211 Feb 3, 2021
03ed617
Merge branch 'master' of https://github.com/jaegertracing/jaeger into…
rjs211 Feb 3, 2021
d58c20c
changing default behaviour of host-port flags
rjs211 Feb 4, 2021
0633da0
pruning test cases, renaming test method
rjs211 Feb 4, 2021
9ff874f
changing comments for host-port default
rjs211 Feb 4, 2021
0c058fa
changelog for default query server behaviour change
rjs211 Feb 4, 2021
5421036
Merge branch 'master' of https://github.com/jaegertracing/jaeger into…
rjs211 Feb 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 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 Query Server flags `--query.port` and `--query.host-port`, please use dedicated `--query.http-server.host-port` and `--query.grpc-server.host-port` flags ([#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`
Expand All @@ -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`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit concerned that this will be confusing. Both new flags have default values (different ones), so the traditional behavior of the program when a flag is not specified explicitly is to use the default. However, here we're saying that even though the help command will show :16685 as default, the actual server is going to run on the shared :16686 port.

Perhaps we could just move this into breaking changes section and always treat the ports as separate, unless the same value is given in the config (which should only work for non-TLS).

@jpkrohling thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could just move this into breaking changes section and always treat the ports as separate, unless the same value is given in the config (which should only work for non-TLS).

If we are breaking, then I'd force people to always use a different port. It makes the code easier to maintain, and I believe this is a good practice anyway...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be too "breaking". It's one thing to to change the flags, it's another to completely remove the ability to run on the same port. The same-port CMUX approach seems to work fine for non-TLS, I would rather keep it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.


### UI Changes

Expand Down
27 changes: 4 additions & 23 deletions cmd/query/app/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
Expand All @@ -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

}
Expand Down
98 changes: 28 additions & 70 deletions cmd/query/app/flags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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"},
Expand Down Expand Up @@ -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 {
Expand Down
8 changes: 3 additions & 5 deletions cmd/query/app/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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)
Expand All @@ -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

Expand Down Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions cmd/query/app/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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() {
Expand All @@ -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()
Expand Down
3 changes: 2 additions & 1 deletion ports/ports.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 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
Expand Down