Implement default server authenticators #4558

jpkrohling · 2021-12-15T15:56:40Z

Allow the interfaces to be extended without affecting implementations by allowing authenticators to provide which functions to override.

This is a non-breaking change, and current implementations might be changed in the future to use this.

Fixes #4556

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

codecov · 2021-12-15T15:59:44Z

Codecov Report

Merging #4558 (7999592) into main (3db1d11) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4558      +/-   ##
==========================================
+ Coverage   90.25%   90.26%   +0.01%     
==========================================
  Files         181      181              
  Lines       10595    10613      +18     
==========================================
+ Hits         9562     9580      +18     
  Misses        805      805              
  Partials      228      228

Impacted Files	Coverage Δ
config/configauth/default_serverauthenticator.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 3db1d11...7999592. Read the comment docs.

jpkrohling · 2021-12-15T15:58:16Z

config/configauth/default_serverauthenticator.go

+	componenthelper.ShutdownFunc
+}
+
+// WithAuthenticate


If the general direction looks good, I'll work on the godoc.

jpkrohling · 2021-12-15T15:59:40Z

config/confighttp/confighttp_test.go

 	host := &mockHost{
 		ext: map[config.ComponentID]component.Extension{
-			config.NewComponentID("mock"): &configauth.MockServerAuthenticator{


This is a good example of how consumers would be able to use this. I like the new approach, especially as it allows consumers to omit specifying the default implementations to use, like demonstrated by the removal of line 785.

jpkrohling · 2021-12-15T16:11:13Z

Interesting, the changelog check is failing even though there's a changelog entry...

jpkrohling · 2021-12-16T18:39:31Z

Rebased.

jpkrohling · 2021-12-17T08:28:19Z

@bogdandrutu, can you take a quick look and see if this is roughly the direction you wanted it to go? If so, I'll add godoc and ask @codeboten to review this.

config/configauth/default_serverauthenticator.go

jpkrohling · 2021-12-20T09:50:03Z

I'm converting this to draft, as we would probably re-evaluate this after #4583.

jpkrohling · 2021-12-21T09:54:10Z

@bogdandrutu, @pavankrish123, do you still see the need for this PR? The interface now has only one function on its own (Authenticate), plus the ones from the component interface.

In any case, I updated this PR to remove the interceptor functions as a follow-up from #4582.

jpkrohling · 2021-12-21T15:41:06Z

While the contrib test failures are related to the same area, I don't think they are caused by this PR. Once open-telemetry/opentelemetry-collector-contrib#6922 is merged, I'lll trigger the build again and see how it goes.

bogdandrutu · 2021-12-21T16:41:19Z

@jpkrohling I like it, please move forward with this. Also consider to enforce everyone using the framework by adding a private func, this way we can extend the interface in the future.

Allow the interfaces to be extended without affecting implementations by allowing authenticators to provide which functions to override. This is a non-breaking change, and current implementations might be changed in the future to use this. Fixes open-telemetry#4556 Signed-off-by: Juraci Paixão Kröhling <[email protected]>

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

jpkrohling · 2022-01-03T14:26:41Z

Also consider to enforce everyone using the framework by adding a private func

Sorry, could you expand on this?

I just updated this PR with the missing godoc and I think this is now ready to be merged.

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]>

* Implement default client authenticators 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]>

* 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]>

jpkrohling requested review from a team and bogdandrutu December 15, 2021 15:56

jpkrohling mentioned this pull request Dec 15, 2021

Allow "configauth" interfaces to be extended #4534

Closed

2 tasks

jpkrohling commented Dec 15, 2021

View reviewed changes

jpkrohling force-pushed the jpkrohling/issue4556 branch from 3256045 to 684e4e6 Compare December 15, 2021 16:14

jpkrohling mentioned this pull request Dec 15, 2021

Changelog check is failing #4559

Closed

jpkrohling force-pushed the jpkrohling/issue4556 branch from 684e4e6 to 715dd36 Compare December 16, 2021 18:38

bogdandrutu reviewed Dec 17, 2021

View reviewed changes

config/configauth/default_serverauthenticator.go Outdated Show resolved Hide resolved

jpkrohling marked this pull request as draft December 20, 2021 09:49

codeboten mentioned this pull request Dec 21, 2021

make update-otel open-telemetry/opentelemetry-collector-contrib#6922

Merged

jpkrohling force-pushed the jpkrohling/issue4556 branch from 715dd36 to b6f0c01 Compare December 21, 2021 09:52

jpkrohling marked this pull request as ready for review December 21, 2021 09:54

jpkrohling requested a review from bogdandrutu December 21, 2021 09:54

jpkrohling added 3 commits January 3, 2022 15:01

fixuop

475da63

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

Add missing godoc

7999592

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

jpkrohling force-pushed the jpkrohling/issue4556 branch from b6f0c01 to 7999592 Compare January 3, 2022 14:18

bogdandrutu approved these changes Jan 3, 2022

View reviewed changes

bogdandrutu merged commit 51fcb65 into open-telemetry:main Jan 3, 2022

jpkrohling mentioned this pull request Jan 4, 2022

Implement default client authenticators, where functions can be overridden #4557

Closed

jpkrohling mentioned this pull request Feb 10, 2022

Implement default client authenticators #4837

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement default server authenticators #4558

Implement default server authenticators #4558

jpkrohling commented Dec 15, 2021

codecov bot commented Dec 15, 2021 •

edited

Loading

jpkrohling Dec 15, 2021

jpkrohling Dec 15, 2021

jpkrohling commented Dec 15, 2021

jpkrohling commented Dec 16, 2021

jpkrohling commented Dec 17, 2021

jpkrohling commented Dec 20, 2021

jpkrohling commented Dec 21, 2021

jpkrohling commented Dec 21, 2021

bogdandrutu commented Dec 21, 2021

jpkrohling commented Jan 3, 2022

Implement default server authenticators #4558

Implement default server authenticators #4558

Conversation

jpkrohling commented Dec 15, 2021

codecov bot commented Dec 15, 2021 • edited Loading

Codecov Report

jpkrohling Dec 15, 2021

Choose a reason for hiding this comment

jpkrohling Dec 15, 2021

Choose a reason for hiding this comment

jpkrohling commented Dec 15, 2021

jpkrohling commented Dec 16, 2021

jpkrohling commented Dec 17, 2021

jpkrohling commented Dec 20, 2021

jpkrohling commented Dec 21, 2021

jpkrohling commented Dec 21, 2021

bogdandrutu commented Dec 21, 2021

jpkrohling commented Jan 3, 2022

codecov bot commented Dec 15, 2021 •

edited

Loading