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.

Sign up for GitHub

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

Implement default client authenticators #4837

Merged
merged 5 commits into from
Mar 11, 2022
Merged

Implement default client authenticators #4837

merged 5 commits into from
Mar 11, 2022

Conversation

jpkrohling
Copy link
Member

Similar to #4558, this allows client authenticators to be created without having to implement all the functions from the ClientAuthenticator interface.

This is non-breaking change.

Fixes #4557

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

@jpkrohling jpkrohling requested review from a team and codeboten February 10, 2022 15:04
@jpkrohling jpkrohling mentioned this pull request Feb 10, 2022
Merged
@jpkrohling
Copy link
Member Author

See open-telemetry/opentelemetry-collector-contrib#7787 for more information on how client authenticators can use this new API.

@codecov
Copy link

codecov bot commented Feb 10, 2022 •
edited
Loading

Codecov Report

Merging #4837 (7080280) into main (16657c8) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4837      +/-   ##
==========================================
+ Coverage   90.88%   90.95%   +0.06%     
==========================================
  Files         181      182       +1     
  Lines       10633    10667      +34     
==========================================
+ Hits         9664     9702      +38     
+ Misses        751      748       -3     
+ Partials      218      217       -1
Impacted Files Coverage Δ
config/configauth/default_clientauthenticator.go 100.00% <100.00%> (ø)
model/internal/pdata/common.go 95.33% <0.00%> (+0.88%) ⬆️

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 16657c8...7080280. Read the comment docs.

@jpkrohling
Copy link
Member Author

cc @pavankrish123

@pavankrish123
Copy link
Contributor

LGTM!

@jpkrohling jpkrohling mentioned this pull request Feb 16, 2022
Closed
@jpkrohling
Copy link
Member Author

@codeboten, could you please review this one?

@pavankrish123
Copy link
Contributor

pavankrish123 commented Feb 17, 2022 •
edited
Loading

LGTM! Thanks @jpkrohling

bogdandrutu
bogdandrutu reviewed Feb 18, 2022
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

I've always been confused about the package choice for this extension. It is very strange to have an extension defined in the config sub-package instead of a sub-package in extension. I think we should fix this.

@bogdandrutu
Copy link
Member

@pavankrish123 don't hesitate to press the "Approve" button, even though it does not make things "green", but it is easier to find who approved/lgtm than looking in the history of comments :)

@jpkrohling
Copy link
Member Author

I think we should fix this.

@bogdandrutu, do you mean for this PR? We have the server counterpart to this merged already. I'd then suggest closing this PR, applying the changes to the server authenticators, and opening a new PR for the client authenticators (replacing this PR here).

bogdandrutu
bogdandrutu approved these changes Mar 2, 2022
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

I am ok merging this, then fix the package .

config/configauth/default_clientauthenticator.go Outdated Show resolved Hide resolved
codeboten
codeboten reviewed Mar 8, 2022
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Code looks good. @jpkrohling please take a look at @bogdandrutu's last comment

config/configauth/default_clientauthenticator.go Show resolved Hide resolved
config/configauth/default_clientauthenticator.go Show resolved Hide resolved
@jpkrohling
Implement default client authenticators
5f33f0f 
Similar to open-telemetry#4558, this allows client authenticators to be created without having to implement all the functions from the ClientAuthenticator interface.

This is non-breaking change.

Fixes open-telemetry#4557

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling
Changelog
982c75e 
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling
lint
7d674cd 
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling
Test default per-rpc-credentials implementation
5a02401 
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling
Add missing godoc
7080280 
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling jpkrohling force-pushed the jpkrohling/issue4557 branch from 51128dc to 7080280 Compare March 10, 2022 14:03
@jpkrohling
Copy link
Member Author

Comments addressed. Thanks for the review!

@jpkrohling jpkrohling mentioned this pull request Mar 11, 2022
Merged
@codeboten codeboten merged commit 5ee716f into open-telemetry:main Mar 11, 2022
@jpkrohling jpkrohling mentioned this pull request Mar 14, 2022
Closed
Nicholaswang pushed a commit to Nicholaswang/opentelemetry-collector that referenced this pull request Jun 7, 2022
@jpkrohling @Nicholaswang
Implement default client authenticators (open-telemetry#4837)
61df08d 
* Implement default client authenticators

Similar to open-telemetry#4558, this allows client authenticators to be created without having to implement all the functions from the ClientAuthenticator interface.

This is non-breaking change.

Fixes open-telemetry#4557

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MovieStoreGuy MovieStoreGuy MovieStoreGuy approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@codeboten codeboten codeboten approved these changes

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

Successfully merging this pull request may close these issues.

Implement default client authenticators, where functions can be overridden
5 participants
@jpkrohling @pavankrish123 @bogdandrutu @codeboten @MovieStoreGuy