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.

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

Set the grpc port name to include http(s) prefix. #1122

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

jpkrohling
Copy link
Contributor

@jpkrohling jpkrohling commented Jul 7, 2020

Fixes #1117 by setting the gRPC port name to include the https- prefix in case TLS is enabled, and http- if TLS isn't enabled.

Signed-off-by: Juraci Paixão Kröhling [email protected]

@jpkrohling jpkrohling requested a review from objectiser July 7, 2020 13:13
@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #1122 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1122      +/-   ##
==========================================
+ Coverage   88.07%   88.09%   +0.02%     
==========================================
  Files          86       86              
  Lines        5283     5294      +11     
==========================================
+ Hits         4653     4664      +11     
  Misses        466      466              
  Partials      164      164              
Impacted Files Coverage Δ
pkg/service/collector.go 100.00% <100.00%> (ø)

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 55f0191...44f2c4c. Read the comment docs.

}

// if we don't have a jaeger provided, it's certainly not TLS...
if nil == jaeger {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this ever happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in the common path, not with the code we have today.

@jpkrohling
Copy link
Contributor Author

I manually tested this in an OpenShift cluster (crc) with Service Mesh installed and auto-injecting the Istio sidecar, and can confirm that this fixes the reported issue:

$ kubectl logs deployment/myapp -c jaeger-agent -f
2020/07/07 14:17:44 maxprocs: Leaving GOMAXPROCS=6: CPU quota undefined
{"level":"info","ts":1594131464.91614,"caller":"flags/service.go:116","msg":"Mounting metrics handler on admin server","route":"/metrics"}
{"level":"info","ts":1594131464.9164233,"caller":"flags/admin.go:120","msg":"Mounting health check on admin server","route":"/"}
{"level":"info","ts":1594131464.916458,"caller":"flags/admin.go:126","msg":"Starting admin HTTP server","http-addr":":14271"}
{"level":"info","ts":1594131464.9164734,"caller":"flags/admin.go:112","msg":"Admin server started","http.host-port":"[::]:14271","health-status":"unavailable"}
{"level":"warn","ts":1594131464.9164839,"caller":"reporter/flags.go:61","msg":"Using deprecated configuration","option":"jaeger.tags"}
{"level":"info","ts":1594131464.9167476,"caller":"grpc/builder.go:57","msg":"Agent requested secure grpc connection to collector(s)"}
{"level":"info","ts":1594131464.916988,"caller":"[email protected]/clientconn.go:106","msg":"parsed scheme: \"dns\"","system":"grpc","grpc_log":true}
{"level":"info","ts":1594131464.9179482,"caller":"command-line-arguments/main.go:78","msg":"Starting agent"}
{"level":"info","ts":1594131464.9179833,"caller":"healthcheck/handler.go:128","msg":"Health Check state change","status":"ready"}
{"level":"info","ts":1594131464.9180768,"caller":"app/agent.go:69","msg":"Starting jaeger-agent HTTP server","http-port":5778}
{"level":"info","ts":1594131464.9197319,"caller":"dns/dns_resolver.go:212","msg":"ccResolverWrapper: sending update to cc: {[{10.128.0.71:14250  <nil> 0 <nil>}] <nil> <nil>}","system":"grpc","grpc_log":true}
{"level":"info","ts":1594131464.919748,"caller":"[email protected]/clientconn.go:948","msg":"ClientConn switching balancer to \"round_robin\"","system":"grpc","grpc_log":true}
{"level":"warn","ts":1594131464.9200146,"caller":"[email protected]/clientconn.go:1223","msg":"grpc: addrConn.createTransport failed to connect to {10.128.0.71:14250  <nil> 0 <nil>}. Err :connection error: desc = \"transport: Error while dialing dial tcp 10.128.0.71:14250: connect: connection refused\". Reconnecting...","system":"grpc","grpc_log":true}
{"level":"info","ts":1594131465.921045,"caller":"base/balancer.go:196","msg":"roundrobinPicker: newPicker called with info: {map[]}","system":"grpc","grpc_log":true}
{"level":"warn","ts":1594131465.921427,"caller":"[email protected]/clientconn.go:1223","msg":"grpc: addrConn.createTransport failed to connect to {10.128.0.71:14250  <nil> 0 <nil>}. Err :connection error: desc = \"transport: Error while dialing dial tcp 10.128.0.71:14250: connect: connection refused\". Reconnecting...","system":"grpc","grpc_log":true}
{"level":"info","ts":1594131467.4787097,"caller":"base/balancer.go:196","msg":"roundrobinPicker: newPicker called with info: {map[]}","system":"grpc","grpc_log":true}
{"level":"info","ts":1594131467.5079808,"caller":"base/balancer.go:196","msg":"roundrobinPicker: newPicker called with info: {map[0xc0002055e0:{{10.128.0.71:14250  <nil> 0 <nil>}}]}","system":"grpc","grpc_log":true}
{"level":"info","ts":1594131494.9264517,"caller":"dns/dns_resolver.go:212","msg":"ccResolverWrapper: sending update to cc: {[{10.128.0.71:14250  <nil> 0 <nil>}] <nil> <nil>}","system":"grpc","grpc_log":true}

And I see traces arriving at the Jaeger backend. Here's a sample of a trace generated based on the business-application-injected-sidecar.yaml:

image

@jpkrohling jpkrohling merged commit 1dce126 into jaegertracing:master Jul 7, 2020
jpkrohling added a commit to jpkrohling/jaeger-operator that referenced this pull request Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port names should start with https/http
2 participants