Enable custom authentication logic #2229
Conversation
Co-authored-by: Chris Smith <[email protected]>
Co-authored-by: Chris Smith <[email protected]>
refactor configauth to use auth extensions. movie oidc to extension
Codecov Report
@@ Coverage Diff @@
## main #2229 +/- ##
==========================================
- Coverage 91.98% 91.87% -0.11%
==========================================
Files 271 271
Lines 15664 15687 +23
==========================================
+ Hits 14408 14412 +4
- Misses 854 870 +16
- Partials 402 405 +3
Continue to review full report at Codecov.
|
config/configauth/README.md
Outdated
|
|
||
| Examples: | ||
| Authenticators are set up as extensions and passed to the server settings for a receiver. When writing a custom authenticator extension, it must call `configauth.AddAuthenticatorToRegistry` from within its createExtension factory method. This allows the receiver to look up the instance of the authenticator that it is configured to use. See [the OIDC extension factory](../../extension/oidcextension/factory.go) as an example. |
There was a problem hiding this comment.
Global registries with self-registration are often sources of problems, make testing more difficult, etc. What if I want to have a test with 2 different instances of the extension? (Think of a test which creates 2 full instances of the Collector to test communication between 2 Collectors, which are perhaps configured with different sets of extensions).
A possible alternate is that the caller of createExtension probes the returned value to see if it implements the Authenticator interface and adds it to the service.Application's local registry of authenticators. We will need to find a way to make this local registry available at the point where the authenticators are created (in GRPCServerSettings.ToServerOption function), not sure what exactly is the best way to do it (possibly add to the ReceiverCreateParams).
There was a problem hiding this comment.
Think of a test which creates 2 full instances of the Collector to test communication between 2 Collectors, which are perhaps configured with different sets of extensions
To make sure I understood this case: this would be with a custom main, where I would create two Application instances as part of the same binary, right? If so, I have to admit that I haven't thought about this case.
I think @bogdandrutu mentioned that he wanted to work on a view registry for the metrics views as well. For Jaeger, we've been thinking about using something similar to this PR here to allow people to extend Jaeger with their own storage mechanisms, where the storage would be a custom extension that people can build and register with an extension/component of ours.
There's definitely a need for registries :-)
There was a problem hiding this comment.
To make sure I understood this case: this would be with a custom main, where I would create two Application instances as part of the same binary, right? If so, I have to admit that I haven't thought about this case.
Yes.
There's definitely a need for registries :-)
Yes, and registries in global variables are undesirable. See for example how we avoided global factory registry although there can be only one per Application. So whatever data we believe is a single instance per Collector needs to be in the Application, not a global variable.
There was a problem hiding this comment.
I'll try to scratch something as part of another PR, unless someone wants to work on it first.
There was a problem hiding this comment.
I can take a look at it as a part of this PR.
I think I see two code paths forward right now, that I want to run past y'all since I haven't spent too much time inside the service code.
- Have the extensions builder would also have to return this registry of Authenticators back up to the service.Application, which can then be passed into the pipeline setup code for the receivers to have access to.
- Instead of an explicit registry for Authenticators, we can just pass the service's builtExtensions map to the receivers that can expect to find the authenticator as an extension
Let me know if either of those sound right, or if I'm completely off the mark here.
There was a problem hiding this comment.
I can take a look at it as a part of this PR.
I'd really rather have it as part of another effort, as we need a proper solution for registries in general.
There was a problem hiding this comment.
@jpkrohling ok this sounds like it might be a bit more sweeping of a change then what I suggested above
There was a problem hiding this comment.
Can this PR move forward without this wider change to registries or does it need to wait until then?
|
To be clear: I believe custom authentication logic is a good idea, but I don't think the implementation in this PR is acceptable. |
|
@tigrannajaryan I guess what I meant was, can I make a change to this PR that enables custom auth without exposing a global registry that may not fix the wider registry problem, but does move this PR away from that implementation |
@christopher-taormina-zocdoc yes, if you update this PR such that there is no longer a global registry then it works for me. Perhaps you can show a draft implementation first if you prefer. |
component/receiver.go
Outdated
| @@ -63,6 +63,9 @@ type ReceiverCreateParams struct { | |||
|
|
|||
| // ApplicationStartInfo can be used by components for informational purposes | |||
| ApplicationStartInfo ApplicationStartInfo | |||
|
|
|||
There was a problem hiding this comment.
@jpkrohling @tigrannajaryan This commit removes the global registry and passes the built extensions into the ReceiverCreateParams to be used eventually by the configgrpc code. I can clean this up more, but wanted you to take a look before I keep going.
config/configgrpc/configgrpc.go
Outdated
| @@ -236,7 +237,7 @@ func (gss *GRPCServerSettings) ToListener() (net.Listener, error) { | |||
| } | |||
|
|
|||
| // ToServerOption maps configgrpc.GRPCServerSettings to a slice of server options for gRPC | |||
| func (gss *GRPCServerSettings) ToServerOption() ([]grpc.ServerOption, error) { | |||
| func (gss *GRPCServerSettings) ToServerOption(extensions map[string]component.ServiceExtension) ([]grpc.ServerOption, error) { | |||
There was a problem hiding this comment.
Can you try something different?
- Create an interface somewhere named, say, "WantsServiceExtensions", defining a
SetServiceExtensions(map[string]component.ServiceExtensionfunction - In the bootstrap code for the
application, where it builds theconfigobjects, check whether the config that was just built is aWantsServiceExtensions, and if so, callSetServiceExtensionswith the map of extensions.
This way, we have a dependency injection-like mechanism, abstracting this requirement away from the callers, making it easy to use this config with all receivers without them having to manually wire up the map.
There was a problem hiding this comment.
Oh interesting. I might be looking at the wrong place in code, but won't that be earlier than the extensions are built?
There was a problem hiding this comment.
I don't remember how the code looks like, but if they are part of the same func/chain, you can iterate over the config objects after you build the extensions.
There was a problem hiding this comment.
Here
opentelemetry-collector/service/service.go
Line 361 in 7eb2628
There was a problem hiding this comment.
Pushed a new commit using this approach, let me know what you think.
| @@ -230,3 +230,7 @@ func (r *otlpReceiver) registerLogsConsumer(ctx context.Context, tc consumer.Log | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func (r *otlpReceiver) SetServiceExtensions(extensions map[string]component.ServiceExtension) { | |||
| r.cfg.GRPC.SetServiceExtensions(extensions) | |||
There was a problem hiding this comment.
Can't we auto-inject directly into the configgrpc? The goal is to remove this knowledge from individual components.
There was a problem hiding this comment.
The issue I ran into with trying to inject into the configgrpc, is that there's no guaranteed place for it on an individual component's config. For example, the otlp receiver has the GRPC server settings object inside a Protocols struct on the config, while the opencensus receiver just has the GRPC settings as top level attributes on its config struct. This led me to writing a recursive reflection function to iterate over the entire config struct which to me feels a bit messy and overcomplicated.
There was a problem hiding this comment.
Thinking one abstraction further, perhaps the unmarshaller could check whether the struct it just created implements the new interface?
There was a problem hiding this comment.
The config yaml is unmarshalled before any extensions are built, which makes sense because the extensions need their own config to be built
There was a problem hiding this comment.
You can keep a record of the objects that were built and desire extensions. You can iterate this list later and inject the desired requests.
There was a problem hiding this comment.
You are probably not missing, I haven't looked at this code for a while. I'll take a look in early Jan to see if I have any new ideas.
There was a problem hiding this comment.
@jpkrohling I think the draft you posted currently leads to this same issue, where the built exporter would still need to be responsible for passing the authenticator to the grpc config settings, instead of being able to push it straight into the grpc config.
There was a problem hiding this comment.
True, but the config model is the key there. I think it's possible to inject the authenticator to the config. I'll take a look tomorrow.
There was a problem hiding this comment.
The code editor was still open and the idea fresh in my head, so, I went ahead and pushed another commit, injecting the authenticator in the exporter's config. But you are right, something (exporter? its config?) would still need to make the bridge between the gRPC settings and the authenticator. I never thought I would say this, but I miss Java right now, especially CDI :-)
There was a problem hiding this comment.
In time: we just missed the train, but I think this would be a good topic for the SIG meeting, to see if other folks have other ideas. https://docs.google.com/document/d/1r2JC5MB7GupCE7N32EwGEXs9V_YIsPgoFiLP4VWVMkE
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
@tigrannajaryan last I spoke to @jpkrohling he would be around after January 10th or so |
@christopher-taormina-zocdoc He is the expert on auth stuff, so let's wait for him to review. |
|
I'll take a look in the next couple of days. |
|
I haven't forgotten about this one, it was just very hard to look into this with the care that it deserves. |
|
Sorry, it's been really hard to get some time to work on this, but here's a very raw version of what I had in mind: https://github.com/jpkrohling/opentelemetry-collector/tree/jpkrohling/2229-inject-authenticator |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
Sorry I've been busy as of late @jpkrohling, was this discussed in a SIG meeting since you last mentioned it? |
|
No, sorry, but I do have in my queue an item to experiment with adding registries to the collector, perhaps as part of the |
|
As a status update: we talked about this during the SIG meeting last week. While the other usage of registries that I had in mind was sorted out, this one hasn't yet. @tigrannajaryan, @bogdandrutu: I would need your help here. How can we unblock this? People need a way to extend the authenticators with their own logic and looks like we can't get the list of extensions when building configuration objects. |
|
Bogdan and I briefly discussed the auth in Collector and believe that there need to be changes in how it is handled generally. I am not able to review this PR right now, but we will try to settle the auth before the GA. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
* add integration test for envvar config source * use CollectorContainer EffectiveConfig method * include initial config in env config source test Co-authored-by: Ryan Fitzpatrick <[email protected]>
Description:
This PR aims to make creating authentication more flexible. Rather than having to add every possible authenticator to
configauth, this PR pulls the actual authenticators into extensions that are registered withconfigauthand can looked up by the receiver when being set up.Link to tracking Issue: #2101
Testing: Tests for the authenticator have not changed, but just moved. Additional tests were added for the new factory class and the configuration.
Documentation: There is a new README for the OIDC extension, as well as some expanded information in the
configauthreadme to illustrate how to create an authenticator and register it.cc @chris-smith-zocdoc @jpkrohling