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

Support for Custom Exporter Authenticators as Extensions #3128

Merged
merged 65 commits into from
May 20, 2021
Merged

Support for Custom Exporter Authenticators as Extensions #3128

merged 65 commits into from
May 20, 2021

Conversation

pavankrish123
Copy link
Contributor

@pavankrish123 pavankrish123 commented May 7, 2021

Description:
Adding a feature - This PR adds support to add client side (exporter) authenticators for HTTP and gRPC clients through extension based authenticators. This is built of top of what was added for receiver (server) side authenticators via extensions in #2603

Link to tracking Issue: #3115

Pending
This is a draft PR to get an approval on the design and direction to start conversation. There are these pending items

Testing:

  • Did a manual testing for static bearer token.
  • Added unit tests

@pavankrish123
Copy link
Contributor Author

@jpkrohling please let me know what you think.

config/configauth/clientauth.go Outdated Show resolved Hide resolved
exporter/otlphttpexporter/otlp.go Outdated Show resolved Hide resolved
extension/oauth2clientextension/README.md Outdated Show resolved Hide resolved
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Once you move the auth configuration to the ToClient, do the same for the gRPC and provide an example of what it would take for a gRPC client to get authentication. Should be relatively simple.

We would probably also want to deprecate/remove the PerRPCCredentials we currently have, in favor of an extension with a static token. In fact, I think the current implementation could be the first implementation of the authenticator, as it's both existing and simple. The OAuth2 could be a second implementation, part of a follow-up PR.

exporter/otlphttpexporter/otlp.go Outdated Show resolved Hide resolved
extension/oauth2clientextension/extension.go Outdated Show resolved Hide resolved
extension/oauth2clientextension/facrory_test.go Outdated Show resolved Hide resolved
@pavankrish123 pavankrish123 marked this pull request as ready for review May 12, 2021 07:39
@pavankrish123 pavankrish123 requested a review from a team May 12, 2021 07:39
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Looks way better! But would it be possible to split this PR, removing the OAuth extension, in favor of a simple "static token"? This way, this PR will purely be a refactor of the current gRPC PerRPCCredentials, with a subsequent PR being a new feature (OAuth2 client auth for both HTTP and gRPC).

exporter/otlphttpexporter/otlp.go Outdated Show resolved Hide resolved
@pavankrish123 pavankrish123 changed the title [DRAFT]Support for Custom Exporter Authenticators as Extensions Support for Custom Exporter Authenticators as Extensions May 12, 2021
@pavankrish123 pavankrish123 marked this pull request as draft May 12, 2021 12:57
@gramidt
Copy link
Member

gramidt commented May 19, 2021

@pavankrish123 - Can you verify your rebase with the 'main' branch? The build-and-test CircleCI is still failing due to "unauthorized" which should have been resolved by #3423.

@tigrannajaryan
Copy link
Member

We need to wait for open-telemetry/opentelemetry-collector-contrib#3423 to be merged before we can merge this.

This is the core repo, not contrib, I was wrong. Contrib is now fixed, we need a similar fix for the core. @owais I believe you are already working on this.

@gramidt
Copy link
Member

gramidt commented May 19, 2021

Good catch, @tigrannajaryan! Ignore my comment, @pavankrish123.

@gramidt
Copy link
Member

gramidt commented May 20, 2021

@pavankrish123 - Once #3219 has been merged, we'll just need to rebase again to kickoff the blocked tests. Once this is done, let's ping Tigran to get this merged.

@tigrannajaryan
Copy link
Member

Please rebase.

@pavankrish123
Copy link
Contributor Author

I am on it.

@gramidt
Copy link
Member

gramidt commented May 20, 2021

Thank again, @pavankrish123! Looks like everything is ready to be merged 🚀

@pavankrish123
Copy link
Contributor Author

Thanks @gramidt @tigrannajaryan - let me know if you need anything else.

@tigrannajaryan
Copy link
Member

@pavankrish123 Great, I can merge this now. Are you prepared to quickly fix the contrib which will be failing because of this change as soon as we try to update it to the latest version of core?

@gramidt
Copy link
Member

gramidt commented May 20, 2021

@tigrannajaryan - I'm happy to help quickly fix contrib

@pavankrish123
Copy link
Contributor Author

pavankrish123 commented May 20, 2021

@tigrannajaryan @gramidt I am happy to jump on too. Just let me know as soon as the contrib is updated to the latest version of core

@gramidt
Copy link
Member

gramidt commented May 20, 2021

Thank you, @pavankrish123!

@pavankrish123
Copy link
Contributor Author

@tigrannajaryan @gramidt open-telemetry/opentelemetry-collector-contrib#3438 opened this to track the work. All in all it looks like there are 5 exporters that need to be touched.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

@pavankrish123 great job and thank you for your patience.

@tigrannajaryan tigrannajaryan merged commit af71182 into open-telemetry:main May 20, 2021
@tigrannajaryan
Copy link
Member

@pavankrish123 @gramidt I merged this PR, thank you all. Please decide who updates the contrib and create a PR that does it.

@pavankrish123 pavankrish123 deleted the client_auth_ext branch May 21, 2021 00:33
tigrannajaryan pushed a commit that referenced this pull request May 28, 2021
This PR renames the OIDC Auth Extension to oidcauthextension, following the naming pattern agreed as part of #3128

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
tigrannajaryan pushed a commit that referenced this pull request May 28, 2021
This PR ports the client authenticator interfaces from PR #3128  in a piece meal fashion. The interfaces are currently not used by any client configurations and are published for reviewing only. 

Plan is to modify ToClient and ToDialOptions apis once all the clients are prepared per #3287 (core and contrib side as well).

Link to tracking Issue:
#3287 #3282

Testing:
Unit tests, [manual test described (for only oidc)](#3128 (comment))
tigrannajaryan pushed a commit that referenced this pull request Jun 3, 2021
… extensions configuration map (#3340)

This PR is a port of the configfrpc's ToDialOptions() and confighttp ToClient() from PR #3128  in a piece meal fashion. 

The following are the changes
- Refactored configgrpc.PerRPCAuth as extension implementing configauth.GrpcClientAuthenticator
- Plugged in extensions configuration to all the grpc based clients in the core (OTLP, OpenCensus, Jaeger, JaegerReceiver)
- Plugged in extensions configuration to all the HTTP based clients in the core (Zipkin, OTLPHTTP)

Link to tracking Issue:
 #3282 #3276

Testing:
Unit tests, [manual test described (for only oidc)](#3128 (comment))
dashpole pushed a commit to dashpole/opentelemetry-collector that referenced this pull request Jun 14, 2021
…try#3128)

This PR adds support to add client side (exporter) authenticators for HTTP and gRPC clients through extension based authenticators. This is built of top of what was added  for receiver (server) side authenticators via extensions in open-telemetry#2603 

**Link to tracking Issue:** open-telemetry#3115

**Testing:** 
- Did a manual testing for static bearer token.
- Added unit tests
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.

5 participants