-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Enabling OpAMP Supervisor Authentication with Extension #32762
Comments
Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
Hey @dmolenda-sumo, have you made any progress on this? I'd be happy to help with prototyping some different options if you'd like, I think last this was discussed we were considering what the best way to implement this was (e.g. if we had to separate out a different interface for these extensions or not). |
I'm still interested in helping with this issue! |
I'm going to assign this to @BinaryFissionGames since he's available to work on this now but please let us know @dmolenda-sumo if you were already in progress. |
Unfortunately I won't be able to work on this issue :(. It's great that @BinaryFissionGames will be able to work on it |
It would be great if general the concept of "extension" like in opentelemetry collector would be supported, especially the oauth2clientauthextension (https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/extension/oauth2clientauthextension)n would be useful |
I've taken some time to explore some possibilities here, and we have a few options. The focus of these options is on connecting with WebSockets, because normal one-shot http request authentication is already well supported by the auth interface. Option 1: Create some adapter between websocket.Dialer::DialContext and http.RoundTripperThis is the simplest approach, but it has some downsides. Essentially, the auth.Client interface works by allowing an extension to define a RoundTripper. If we can adapt RoundTripper to a Dialer-like interface, we can re-use the existing extensions with no modifications. For purely http authentication (not websockets), we'd be able to use the normal RoundTripper without any modifications. Pros:
Cons:
Option 2: Add a new method to the auth.Client interfaceThis approach is more work, but is much more clear on the limitations of authenticating websockets. In this approach, we'd add another method to the auth.Client interface, that allows the auth extension to return a Dialer-like interface (implements DialContext for websockets). This would be very similar to the RoundTripper method, where a base dialer is passed in and expected to have it's DialContext method called. Pros:
Cons:
Option 3: Don't re-use authentication extensionsIn this approach, we don't re-use authentication extensions, but instead write separate code to handle authentication in the supervisor. Pros:
Cons:
In my opinion, option 2 makes the most sense. It's the most work, but is also the most robust option that is also the most flexible. The interface for dialing a websocket connection just isn't fully compatible with the RoundTripper interface, and I think that could cause us issues down the road, so I don't feel option 1 is fully suitable. Option 3 doesn't make sense long-term, because we'd really like these auth methods to be usable in both the extension and supervisor if possible, and I think having a completely separate auth system just for OpAMP is confusing to a user. @evan-bradley @tigrannajaryan - would like to know if you have any input on this or any other ideas. |
@BinaryFissionGames Thank you for the detailed writeup. I can also look into this if necessary, but do you know what connection options are available in other WebSocket libraries? My main concern with option 2 is that we make an interface that is overly specific to the Gorilla WebSocket library. |
This is a good point. I looked into some other websocket libraries, it seems like these are the three biggest ones:
I actually wasn't aware of the third one here. What's interesting about that one is that it actually lets you specify the http client. If that's the case, we could actually get the RoundTripper to work. We'd need to use this library in the opamp-go library, but it seems doable, and probably simpler in the long run. I think I'll try that out first and see if we run into any issues. |
We discussed at the Agent WG, it was decided we do not want to change the websocket library as it would be a big change. I'll explore option 2 a bit more and see if I can make an interface that is library agnostic. |
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
Component(s)
cmd/opampsupervisor
Is your feature request related to a problem? Please describe.
Some vendors have specific authentication methods and the current authentication method between the OpAMP server and OpAMP supervisor is not sufficient. Currently, the only way to solve this problem is to create fork and add custom authentication methods there.
Describe the solution you'd like
It would be useful to reuse the authentication extensions that are already available. To do this, the supervisor needs to be modified to include the extension in its build. Furthermore, the current supervisor configuration must be extended to allow for the definition of the extension and enable its use in the server definition.
The first step will be to attempt to add extensions to the supervisor in their current form along with all the required dependencies. This will help us determine the number of dependencies needed and whether this number is acceptable for us.
Describe alternatives you've considered
An alternative would be to somehow separate the logic responsible for authentication from that required by the collector of each extension. This would reduce the number of dependencies needed for the supervisor. Unfortunately, this solution requires changes to the collector, which will make it much more time-consuming. For this reason, we decided to try the first approach for now.
No response
Additional context
No response
The text was updated successfully, but these errors were encountered: