Separate Ports for GRPC and HTTP requests in Query Server #2387

rjs211 · 2020-08-13T05:03:48Z

Which problem is this PR solving?

Resolves Separate GRPC and HTTP ports for Query #2377
Resolves gRPC with TLS doesn't work with cmux in mixed mode #2338

Short description of the changes

separate the HTTP port and gRPC port for query server
TODO: define default port for gRPC and http is set

Signed-off-by: rjs211 <[email protected]>

codecov · 2020-08-13T05:35:37Z

Codecov Report

Merging #2387 into master will decrease coverage by 0.03%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master    #2387      +/-   ##
==========================================
- Coverage   95.57%   95.54%   -0.04%     
==========================================
  Files         208      208              
  Lines       10690    10741      +51     
==========================================
+ Hits        10217    10262      +45     
- Misses        401      404       +3     
- Partials       72       75       +3

Impacted Files	Coverage Δ
ports/ports.go	`100.00% <ø> (ø)`
cmd/query/app/server.go	`89.14% <94.44%> (-4.19%)`	⬇️
cmd/query/app/flags.go	`100.00% <100.00%> (ø)`
...lugin/sampling/strategystore/adaptive/processor.go	`100.00% <0.00%> (+0.79%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5508165...dfe0c00. Read the comment docs.

rjs211 · 2020-08-13T06:12:16Z

Is there any convention for defining new ports and port Numbers? (Need to add a default port of gRPC requests) query.grpc-server.host-port

jpkrohling · 2020-08-13T12:26:51Z

For ports that are highly related, just use the next integer for the new port, if it's free. Like: 16686 for query-http and 16687 for query-grpc.

rjs211 · 2020-08-13T12:31:25Z

I am going with 16685 for query-grpc (new port) and 16686 for query-http (existing port )( 16687 is already in use by query-admin)

objectiser · 2020-08-13T15:03:41Z

@rjs211 @jpkrohling Wouldn't this be a breaking change then? Shouldn't the default value for both of the new flags be 16686, with the logic as suggested by Yuri - so that by default (with no flags) it will use cmux. So if users want them on separate ports, they will then decide which port numbers they want to use.

rjs211 · 2020-08-13T15:20:54Z

@objectiser

The actual assignment is as follows:

The values of {query.host-port , query.port, query.http-server.host-port, query.grpc-server.host-port } are initalized form the viper (including default values. At the end of this point, grpc and http flags have different default values )
if neither of {query.host-port , query.port} is explicitly set and atleast one of {query.http-server.host-port, query.grpc-server.host-port} is set explicitly, then the current option with different values of http and grpc ports is returned.
else (this includes no ports ~~defined~~ set case as well UPDATE: No ports "set" ), both query.http-server.host-port and query.grpc-server.host-port is assigned the value of {query.host-port , query.port} whichever is defined.

for now, default values of query.http-server.host-port or query.grpc-server.host-port are used in case of the second point (when one of them is defined).

This will be more significant when the usage of TLS is enabled for either or both of grpc and http (once http TLS is supported by #2337 ) and the use of two separate ports will be forced on the user (as explained by issue #2377 ).

objectiser · 2020-08-13T15:42:09Z

@rjs211 Understand the reasons for having separate ports when TLS is being configured, but from the sounds of (1):

The values of {query.host-port , query.port, query.http-server.host-port, query.grpc-server.host-port } are initalized form the viper (including default values. At the end of this point, grpc and http flags have different default values )

the default behaviour (i.e. where no flags are defined) will be that grpc and http query ports are different. Therefore this is a breaking change?

rjs211 · 2020-08-13T15:46:30Z

the case of No ports set or no flags set is (3). In that case, the value of query.host-port would be defined (it may not be explicitly set, in which case, it will hold default value). The conditional in (2) would fail if no ports / flags are set. Sorry, I got confused between set and defined.

jpkrohling · 2020-08-13T15:55:03Z

Looks like we are all on the same page, but just to be clear: we should not have breaking changes.

Signed-off-by: rjs211 <[email protected]>

cmd/query/app/flags_test.go

… dev-addPort

Signed-off-by: rjs211 <[email protected]>

jpkrohling

I'll do a couple of manual tests, and there are a couple of minor things to change, but in general, LGTM.

cmd/query/app/flags.go

cmd/query/app/flags_test.go

cmd/query/app/server_test.go

cmd/query/app/flags.go

jpkrohling · 2020-08-24T09:52:45Z

For the manual test, I build a simple Go application that lists the services from a target Query server, and I can confirm that the gRPC changes from this PR works:

package main

import (
	"context"
	"flag"
	"fmt"

	"github.com/jaegertracing/jaeger/proto-gen/api_v2"
	_ "github.com/jaegertracing/jaeger/proto-gen/api_v2"
	"go.uber.org/zap"
	"google.golang.org/grpc"
)

func main() {
	logger, err := zap.NewDevelopment()
	if err != nil {
		fmt.Println(err)
		return
	}

	hostPort := flag.String("host-port", "localhost:16686", "the host-port to use when connecting to the remote Jaeger Query server")
	flag.Parse()

	fmt.Printf("Using host-port: %s\n", *hostPort)

	conn, err := grpc.Dial(*hostPort, grpc.WithInsecure())
	if err != nil {
		logger.Error("failed to connect to the collector", zap.Error(err))
	}
	defer conn.Close()

	client := api_v2.NewQueryServiceClient(conn)

	req := api_v2.GetServicesRequest{}
	resp, err := client.GetServices(context.Background(), &req)
	if err != nil {
		logger.Error("failed to get list of services", zap.Error(err))
		return
	}

	if len(resp.Services) == 0 {
		fmt.Println("No services at the target server.")
		return
	}

	fmt.Println("Available services at the target server:")
	for _, svc := range resp.Services {
		fmt.Printf("- %s\n", svc)
	}
}

… dev-addPort

Signed-off-by: rjs211 <[email protected]>

… dev-addPort

Signed-off-by: rjs211 <[email protected]>

cmd/query/app/flags.go

ports/ports.go

Signed-off-by: rjs211 <[email protected]> 1. Added test cases for `HostPortToPort` method 2. Modified Depricated warning to reasonable date

jpkrohling

LGTM.

jpkrohling · 2020-09-09T12:25:58Z

Anyone else wants to re-review? @yurishkuro ?

albertteoh

Looks pretty good to me! Mostly minor comments.

cmd/query/app/flags.go

ports/ports.go

cmd/query/app/server.go

cmd/query/app/flags.go

ports/ports_test.go

cmd/query/app/flags.go

ports/ports.go

Signed-off-by: rjs211 <[email protected]>

albertteoh · 2020-09-10T12:19:12Z

@rjs211 please increase the patch code coverage.

Signed-off-by: rjs211 <[email protected]>

albertteoh

Thanks @rjs211! LGTM

jpkrohling · 2020-09-11T07:59:59Z

Wonderful work, @rjs211! Thank you for your contribution!

rjs211 · 2020-09-11T08:13:56Z

@jpkrohling @albertteoh @objectiser @yurishkuro Thank you very much for your guidance and patience. This is my first code contribution to opensource. Hope to contribute further.

pavolloffay · 2020-09-16T14:43:37Z

This PR introduced warning message in OTEL all-in-one:

2020-09-16T16:18:41.492+0200	WARN	app/flags.go:111	Use of query.port and query.host-port is deprecated.  Use query.http-server.host-port and query.grpc-server.host-port instead
github.com/jaegertracing/jaeger/cmd/query/app.(*QueryOptions).InitPortsConfigFromViper
	/home/ploffay/projects/jaegertracing/jaeger/cmd/query/app/flags.go:111
github.com/jaegertracing/jaeger/cmd/query/app.(*QueryOptions).InitFromViper
	/home/ploffay/projects/jaegertracing/jaeger/cmd/query/app/flags.go:120
main.startQuery
	/home/ploffay/projects/jaegertracing/jaeger/cmd/opentelemetry/cmd/all-in-one/main.go:194
main.main
	/home/ploffay/projects/jaegertracing/jaeger/cmd/opentelemetry/cmd/all-in-one/main.go:152
runtime.main
	/home/ploffay/bin/go/src/runtime/proc.go:203

rjs211 · 2020-09-16T15:47:28Z

I logged the warning intentionally as per my understanding of #2377 (comment) . Should have I not done that?

pavolloffay · 2020-09-16T15:54:12Z

I have submitted a PR to fix it #2479

rjs211 added 2 commits August 13, 2020 10:00

Adding methods to get port from address

5e8af60

Signed-off-by: rjs211 <[email protected]>

separating ports

4242790

Signed-off-by: rjs211 <[email protected]>

rjs211 requested a review from a team as a code owner August 13, 2020 05:03

rjs211 requested a review from objectiser August 13, 2020 05:03

Adding default GRPC port number

33bfabf

Signed-off-by: rjs211 <[email protected]>

rjs211 commented Aug 13, 2020

View reviewed changes

cmd/query/app/flags_test.go Show resolved Hide resolved

Merge branch 'master' of https://github.com/jaegertracing/jaeger into…

61563b2

… dev-addPort

objectiser mentioned this pull request Aug 17, 2020

Plan for 1.19 release #2398

Closed

return error in server close method

1dcd286

Signed-off-by: rjs211 <[email protected]>

jpkrohling reviewed Aug 24, 2020

View reviewed changes

cmd/query/app/flags.go Outdated Show resolved Hide resolved

rjs211 added 5 commits August 28, 2020 09:32

Merge branch 'master' of https://github.com/jaegertracing/jaeger into…

be498b8

… dev-addPort

remove redundant lines and code cleanup

50c362d

Signed-off-by: rjs211 <[email protected]>

Merge branch 'master' of https://github.com/jaegertracing/jaeger into…

20d92b1

… dev-addPort

Merge branch 'master' of https://github.com/jaegertracing/jaeger into…

e1f7f85

… dev-addPort

Removing possibly flaky test

68878bd

Signed-off-by: rjs211 <[email protected]>

jpkrohling reviewed Sep 9, 2020

View reviewed changes

cmd/query/app/flags.go Outdated Show resolved Hide resolved

ports/ports.go Outdated Show resolved Hide resolved

Adding minor changes for review

1236e5d

Signed-off-by: rjs211 <[email protected]> 1. Added test cases for `HostPortToPort` method 2. Modified Depricated warning to reasonable date

jpkrohling approved these changes Sep 9, 2020

View reviewed changes

albertteoh reviewed Sep 9, 2020

View reviewed changes

rjs211 mentioned this pull request Sep 10, 2020

gRPC with TLS doesn't work with cmux in mixed mode #2338

Closed

Using available methods for host-port split, correcting nit-bits

8bd7008

Signed-off-by: rjs211 <[email protected]>

jpkrohling requested a review from albertteoh September 10, 2020 11:46

added tests to increase coverage

dfe0c00

Signed-off-by: rjs211 <[email protected]>

albertteoh approved these changes Sep 11, 2020

View reviewed changes

jpkrohling merged commit 393e04d into jaegertracing:master Sep 11, 2020

rjs211 deleted the dev-addPort branch January 28, 2021 03:01

rjs211 mentioned this pull request Jan 28, 2021

Add changelog entry for Query server TLS support #2746

Merged

xeviknal mentioned this pull request Jul 26, 2021

ERR GetAppTraces, Jaeger GRPC client error: rpc error: code = Unavailable desc = connection closed kiali/kiali#4207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate Ports for GRPC and HTTP requests in Query Server #2387

Separate Ports for GRPC and HTTP requests in Query Server #2387

rjs211 commented Aug 13, 2020 •

edited by jpkrohling

Loading

codecov bot commented Aug 13, 2020 •

edited

Loading

rjs211 commented Aug 13, 2020

jpkrohling commented Aug 13, 2020

rjs211 commented Aug 13, 2020

objectiser commented Aug 13, 2020

rjs211 commented Aug 13, 2020 •

edited

Loading

objectiser commented Aug 13, 2020

rjs211 commented Aug 13, 2020 •

edited

Loading

jpkrohling commented Aug 13, 2020

jpkrohling left a comment

jpkrohling commented Aug 24, 2020

jpkrohling left a comment

jpkrohling commented Sep 9, 2020

albertteoh left a comment

albertteoh commented Sep 10, 2020

albertteoh left a comment •

edited

Loading

jpkrohling commented Sep 11, 2020

rjs211 commented Sep 11, 2020

pavolloffay commented Sep 16, 2020 •

edited by yurishkuro

Loading

rjs211 commented Sep 16, 2020

pavolloffay commented Sep 16, 2020

Separate Ports for GRPC and HTTP requests in Query Server #2387

Separate Ports for GRPC and HTTP requests in Query Server #2387

Conversation

rjs211 commented Aug 13, 2020 • edited by jpkrohling Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Aug 13, 2020 • edited Loading

Codecov Report

rjs211 commented Aug 13, 2020

jpkrohling commented Aug 13, 2020

rjs211 commented Aug 13, 2020

objectiser commented Aug 13, 2020

rjs211 commented Aug 13, 2020 • edited Loading

objectiser commented Aug 13, 2020

rjs211 commented Aug 13, 2020 • edited Loading

jpkrohling commented Aug 13, 2020

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling commented Aug 24, 2020

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling commented Sep 9, 2020

albertteoh left a comment

Choose a reason for hiding this comment

albertteoh commented Sep 10, 2020

albertteoh left a comment • edited Loading

Choose a reason for hiding this comment

jpkrohling commented Sep 11, 2020

rjs211 commented Sep 11, 2020

pavolloffay commented Sep 16, 2020 • edited by yurishkuro Loading

rjs211 commented Sep 16, 2020

pavolloffay commented Sep 16, 2020

rjs211 commented Aug 13, 2020 •

edited by jpkrohling

Loading

codecov bot commented Aug 13, 2020 •

edited

Loading

rjs211 commented Aug 13, 2020 •

edited

Loading

rjs211 commented Aug 13, 2020 •

edited

Loading

albertteoh left a comment •

edited

Loading

pavolloffay commented Sep 16, 2020 •

edited by yurishkuro

Loading