Move the core plugin implementation into a versioned module.#3750
Conversation
Untangles the core packaging and plugin implementations. Signed-off-by: Michael Nelson <minelson@vmware.com>
antgamdia
left a comment
antgamdia
left a comment
There was a problem hiding this comment.
Great! Thanks for the changes! Happy to see how we are getting rid of (some) tech debt :)
I'd have just a question regarding multiple versions satisfying a given core API (below), but +1 anyway.
| packages.UnimplementedPackagesServiceServer | ||
|
|
||
| // plugins is a slice of all registered plugins which satisfy the core.packages.v1alpha1 | ||
| // pluginsWithServers is a slice of all registered pluginsWithServers which satisfy the core.packages.v1alpha1 |
There was a problem hiding this comment.
At first, I wondered why we are using this v1apha1 in a core API, but I guess this is what the follow-up PR (#3752) is for, no? Thanks for the change!
There was a problem hiding this comment.
The core packages proto has always been versioned as v1alpha1, so we've always been checking whether a plugin satisfies the core.packages.v1alpha1 interface. The followup PR just moves the implementation of that interface into a versioned module.
| if err != nil { | ||
| return fmt.Errorf("failed to create core.packages.v1alpha1 server: %w", err) | ||
| } | ||
| packages.RegisterPackagesServiceServer(grpcSrv, packagesServer) |
There was a problem hiding this comment.
No error is returned here?
There was a problem hiding this comment.
Nope, this GRPC generated function does not return anything in its signature.
| // The argument for the reflect.TypeOf is based on what grpc-go | ||
| // does itself at: | ||
| // https://github.com/grpc/grpc-go/blob/v1.38.0/server.go#L621 | ||
| packagingPlugins := pluginsServer.GetPluginsSatisfyingInterface(reflect.TypeOf((*packages.PackagesServiceServer)(nil)).Elem()) |
There was a problem hiding this comment.
Nice, thanks for the reference. However, I'm wondering: if a given pkg plugin has multiple versions that satisfy the core API (ex: plugin v1: core ops, plugin v2: core ops + customs ops)... would these two versions be added here?
If so, we would be aggregating repeated results, wouldn't we?
There was a problem hiding this comment.
Yes, if the api server is serving the core.packages.v1alpha1 api only, then you would want to enable only a single (for example) helm plugin that satisfies that API (so in your example, I think there would be no need for the v1 one if the v2 one was enabled as both implement the core ops). Or if that becomes an issue, we can enforce some restrictions on startup (such as aggregating only one / the latest / whatever, or just refusing to start if more than one per packaging system is found to implement it).
The purpose is that we later may serve both a core.packages.v1 API and a core.packages.v2 API with a separate helm plugin for each... which is easy as long as the v2 API doesn't satisfy the v1 API (which it should not, and we can enforce that, I think).
absoludity
left a comment
absoludity
left a comment
There was a problem hiding this comment.
Thanks Antonio.
| if err != nil { | ||
| return fmt.Errorf("failed to create core.packages.v1alpha1 server: %w", err) | ||
| } | ||
| packages.RegisterPackagesServiceServer(grpcSrv, packagesServer) |
There was a problem hiding this comment.
Nope, this GRPC generated function does not return anything in its signature.
| // The argument for the reflect.TypeOf is based on what grpc-go | ||
| // does itself at: | ||
| // https://github.com/grpc/grpc-go/blob/v1.38.0/server.go#L621 | ||
| packagingPlugins := pluginsServer.GetPluginsSatisfyingInterface(reflect.TypeOf((*packages.PackagesServiceServer)(nil)).Elem()) |
There was a problem hiding this comment.
Yes, if the api server is serving the core.packages.v1alpha1 api only, then you would want to enable only a single (for example) helm plugin that satisfies that API (so in your example, I think there would be no need for the v1 one if the v2 one was enabled as both implement the core ops). Or if that becomes an issue, we can enforce some restrictions on startup (such as aggregating only one / the latest / whatever, or just refusing to start if more than one per packaging system is found to implement it).
The purpose is that we later may serve both a core.packages.v1 API and a core.packages.v2 API with a separate helm plugin for each... which is easy as long as the v2 API doesn't satisfy the v1 API (which it should not, and we can enforce that, I think).
2 participants
Untangles the core packaging and plugin implementations.
Signed-off-by: Michael Nelson minelson@vmware.com
Description of the change
Untangles the core plugin implementation from the core packages. I needed to update the core plugin in small ways for the resources work, but would have been making the situation worse, so decided to move the core plugin implementation now rather than adding the the tech-debt.
The main change is that the core plugin implementation no longer knows anything about the core packages API. Where the core plugin used to check registered plugins to see if they implement the core packaging API, it now instead provides an exported function:
GetPluginsSatisfyingInterface, which is called by the main executable after registering the plugins to get a slice of plugins that satisfy the core packaging v1alpha1 interface. This is then passed to the core packaging plugin'sNewServer()to initialize.Otherwise it works in just the same way. Most of the other changes are just moving structs and function types that are shared between plugins and the core server to a core module where they can be imported by all.
Benefits
We don't add to the tech-debt as we continue to update the core plugin and packages implementations. I can continue on the resources work without adding to that tech debt :)
Possible drawbacks
Applicable issues
GetK8sResourcesForInstalledPackage#3403Additional information
I'll move the core packages implementation in a followup when taking a break from resources and UI integration next week.