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

Jump to bottom

TLS support for gRPC Query server #2297

Merged
merged 6 commits into from Jun 23, 2020
Merged

TLS support for gRPC Query server #2297

merged 6 commits into from Jun 23, 2020

Conversation

jan25
Copy link
Contributor

@jan25 jan25 commented Jun 21, 2020 •
edited

Which problem is this PR solving?

Short description of the changes

  • Similar to collector TLS implementation. Made use of pkg/config/tlscfg and added new query options

TODO

- Add docs for --query.grpc.tls.* options (will go in documentation repo)

@jan25
Add TLS options for query service
419f74b 
Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
@jan25
Fix tests
7781d1a 
Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
@jan25 jan25 requested a review from a team as a code owner June 21, 2020 17:17
@jan25 jan25 requested a review from vprithvi June 21, 2020 17:17
@jan25
Fix all-in-one
941a56b 
Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
@yurishkuro
Copy link
Member

Please see the build error:

vet: cmd/all-in-one/main.go:201:12: cannot initialize 1 variables with 2 values

yurishkuro
yurishkuro reviewed Jun 21, 2020
View reviewed changes
cmd/query/app/server.go Outdated Show resolved Hide resolved
yurishkuro
yurishkuro reviewed Jun 21, 2020
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

thanks, for the PR, lgtm aside from a couple minor issues

@jan25
Fix otel all-in-one use of query-svc
9ad2bc6 
Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
@jan25
Copy link
Contributor Author

jan25 commented Jun 22, 2020

Thanks for the review! I adjusted the code based on suggestions. For docs, i'll open a separate PR in documentation repo

yurishkuro
yurishkuro reviewed Jun 22, 2020
View reviewed changes
cmd/opentelemetry/cmd/all-in-one/main.go Outdated Show resolved Hide resolved
cmd/query/app/server.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 22, 2020 •
edited

Codecov Report

Merging #2297 into master will decrease coverage by 0.01%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2297      +/-   ##
==========================================
- Coverage   96.23%   96.21%   -0.02%     
==========================================
  Files         218      218              
  Lines       10692    10704      +12     
==========================================
+ Hits        10289    10299      +10     
- Misses        347      350       +3     
+ Partials       56       55       -1
Impacted Files Coverage Δ
cmd/query/app/server.go 93.18% <88.23%> (-1.63%) ⬇️
cmd/query/app/flags.go 100.00% <100.00%> (ø)
plugin/storage/badger/spanstore/reader.go 96.79% <0.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 126701a...d00015d. Read the comment docs.

@jan25
Remove redundant code
472213c 
Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
yurishkuro
yurishkuro approved these changes Jun 22, 2020
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

🎉

@yurishkuro
Copy link
Member

yurishkuro commented Jun 22, 2020 •
edited

@jan25 let me know if you'd like to increase unit test coverage in server.go

image

image

@jan25
Copy link
Contributor Author

jan25 commented Jun 22, 2020

Yes, let me see to improve the coverage

@jan25
Copy link
Contributor Author

jan25 commented Jun 23, 2020

I see two ways to improve coverage - One is to write TestCreateServer that just creates server instance with TLS enabled and tests for nil err. Other is to setup server with TLS, gRPC client and test a RPC call(which may need test certs and keys). Which one would you recommend?

@yurishkuro
Copy link
Member

It would be nice to have both, but looking at collector code we don't have tests for the client/server TLS. Let's at least add tests for the error cases (like invalid path to a cert file).

In the future we may consolidate starting the gRPC server into a shared package that can be tested for TLS.

@jan25
Test error on create tls query server
d00015d 
Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
@yurishkuro yurishkuro merged commit 89afdad into jaegertracing:master Jun 23, 2020
@yurishkuro
Copy link
Member

Thanks, @jan25 🎉

@jan25 jan25 deleted the issue-2248 branch June 24, 2020 05:24
@rjs211 rjs211 mentioned this pull request Jul 9, 2020
Merged
@jpkrohling jpkrohling mentioned this pull request Aug 26, 2020
Merged
yurishkuro added a commit to jaegertracing/documentation that referenced this pull request Jan 19, 2021
@yurishkuro
Indicate that TLS on query service API is supported for HTTP and gRPC
3e3f229 
Related to:
  * jaegertracing/jaeger#2337
  * jaegertracing/jaeger#2297

Signed-off-by: Yuri Shkuro <github@ysh.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@vprithvi vprithvi Awaiting requested review from vprithvi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support mTLS on query-service gRPC endpoints
2 participants
@jan25 @yurishkuro