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

Added PerRPCCredentials for gRPC settings #1250

Merged
merged 4 commits into from
Jul 15, 2020

Conversation

jpkrohling
Copy link
Member

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

Description:
This PR adds support for including a per-RPC authentication to gRPC settings. Initially, only "bearer" token has been added, but can be easily extended in the future based on real-world needs. The token can be read directly from the configuration file or from an external token file, such as the ones injected by Kubernetes into pods.

Link to tracking Issue: n/a

Testing: this was successfully tested with a custom processor, reading the bearer token from the context via metadata.FromIncomingContext(ctx). Additionally, unit tests were added to this PR.

Documentation: the config.yaml reference file has been updated to include this new option.

@jpkrohling
Copy link
Member Author

For a bit more context: I'm building a PoC for a multi-tenant solution, where the agent reads the token from the file system and adds it as part of the RPC payload to the remote collector. The collector then extracts the tenant information based on the JWT and enhances the spans with a tenant resource value.

The feature in this PR is quite generic and would be useful to quite a good number of use cases, not only in the scenario I described above.

The processor I mentioned before still needs some work, but will be contributed to the opentelemetry-collector-contrib in the next few days, once this PR here gets accepted (as this is a requirement for the processor).

@codecov
Copy link

codecov bot commented Jul 2, 2020

Codecov Report

Merging #1250 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1250   +/-   ##
=======================================
  Coverage   89.73%   89.74%           
=======================================
  Files         215      216    +1     
  Lines       15136    15150   +14     
=======================================
+ Hits        13583    13597   +14     
  Misses       1134     1134           
  Partials      419      419           
Impacted Files Coverage Δ
config/configgrpc/bearer_token.go 100.00% <100.00%> (ø)
config/configgrpc/configgrpc.go 100.00% <100.00%> (ø)
translator/internaldata/resource_to_oc.go 83.72% <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 cd2586c...6c780d1. Read the comment docs.

@jpkrohling jpkrohling force-pushed the AddPerRPCCredentials branch 3 times, most recently from c0809ff to 08440a8 Compare July 9, 2020 11:59
Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

@jpkrohling could you please add code (and test) to handle the case when the auth type is unknown other than that LGTM.

config/configgrpc/configgrpc.go Show resolved Hide resolved
config/configgrpc/bearer_token.go Outdated Show resolved Hide resolved
@jpkrohling
Copy link
Member Author

jpkrohling commented Jul 10, 2020

PR updated. The only point to clarify seems to be regarding the ability to read tokens from local files.

(PS: for some reason, I can't seem to be able to re-request a review from @odeke-em)

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

Please, fix the error msg in case of an unknown auth type.

config/configgrpc/configgrpc.go Outdated Show resolved Hide resolved
config/configgrpc/bearer_token.go Outdated Show resolved Hide resolved
@jpkrohling jpkrohling requested a review from pjanotti July 13, 2020 11:32
jpkrohling and others added 3 commits July 13, 2020 13:33
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Co-authored-by: Paulo Janotti <pjanotti@splunk.com>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@pjanotti pjanotti merged commit 71392e5 into open-telemetry:master Jul 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.

4 participants