operator-framework / rukpak

RukPak runs in a Kubernetes cluster and defines APIs for installing cloud native content
Apache License 2.0
52 stars 50 forks source link

Support out-of-tree provisioner implementations #425

Open exdx opened 2 years ago

exdx commented 2 years ago

Rukpak currently supports multiple provisioner implementations that are all housed in-tree. The tooling and documentation are geared towards this design. Furthermore, all the packages involved in building a provisioner are behind the internal directory at the root level at the moment.

As the APIs stabilize, it becomes more desirable to support different provisioner implementations that don't necessarily live in the rukpak codebase. This can be thought of similar to the Rust crate system, where the standard library is small and provides a few necessary crates, and many other crates are then developed by the community outside of the core payload. For example, a helm provisioner would be useful, but does not necessarily have to be a part of core rukpak.

Support for out of tree provisioners can come in several stages.

Phase 1:

Phase 2:

Phase 3:

This issue can be in reference to Phase 1, and follow-up issues can arise to track the subsequent phases.

exdx commented 2 years ago

note: this may be more relevant in the near term based on how the deppy rukpak provisioner is implemented and packaged.

We can look to the Gateway API as an example of extensible kuberenetes APIs that can be used by several different providers.

akihikokuroda commented 2 years ago

For the phase 1, what should be moved to the pkg? Are they updater, storage, source and util?

akihikokuroda commented 2 years ago

I would like to refactor the helm provisioner to out-of-tree provisioner. So I want to start the phase 1. @operator-framework/rukpak-maintainers WDYT?

tylerslaton commented 2 years ago

I would like to refactor the helm provisioner to out-of-tree provisioner. So I want to start the phase 1. @operator-framework/rukpak-maintainers WDYT?

That sounds like a good call to me. I think we'll want to get some of the common packages (like what you listed above) into pkg first so out-of-tree repositories can utilize them. That'll feed into getting an out-of-tree provisioner for helm going without much blockage from our (current) internal packages.

joelanford commented 2 years ago

we'll want to get some of the common packages (like what you listed above) into pkg first so out-of-tree repositories can utilize them

My ask here is that we do this pretty selectively and with a critical eye on the package names and exported types/functions.

akihikokuroda commented 2 years ago

I think the minimal required API is only the api. The internal/source and internal/storage are useful and may help consistency among provisioners. The issue #478 need to be addressed, though. The internal/updater is also useful and can be moved into api.

joelanford commented 2 years ago

@akihikokuroda that sounds about right to me. I wonder if we need everything in internal/source and internal/storage or if there's a way to cut things up such that there's a smaller exported footprint.

I agree that the default unpacker stuff needs some work. We're kind of stuck there in that:

  1. If we could come up with an elegant, non-brittle solution for building the default/composite unpacker, it would be nice to export just that.
  2. If we can't do 1), it seems like the next best thing is to export each of the individual unpackers and let out-of-tree provisioners build their own composite unpacker. That's not super great because it means exporting all of the unpacker implementations directly.

internal/updater

I'm a bit torn on exactly where to put this. It's very API-centric, so there's an argument that it could be moved into the api tree. On the otherhand, it isn't the API (it's more like a set of helpers for the API), so I think there's an equally good argument for it being in a separate spot to keep the api tree "clean" in terms of what typical k8s api trees look like.

akihikokuroda commented 2 years ago

We can change NewDefaultUnpacker to take a map (map[string]interface{})for the configuration

const (
        // ConfigNamespace is key of the config for namespace string
        ConfigNamespace string = "namespace"

        // ConfigUnpackImage is key of the config for unpack image name string
        ConfigUnpackImage string = "unpackImage"

        // ConfigBaseUploadManagerURL is key of the config for base upload manager URL string
        ConfigBaseUploadManagerURL string = "baseUploadManagerURL"

        // ConfigRootCAs is key of the config for *x509.CertPool value for Root CAs
        ConfigRootCAs string = "rootCAs"
)

func NewDefaultUnpacker(config map[string]interface{}) (pkgsource.Unpacker, error)

with this, its usage is like:

        config := make(map[string]interface{})
        config[source.ConfigMgr] = mgr
        config[source.ConfigNamespace] = ns
        config[source.ConfigUnpackImage] = unpackImage
        config[source.ConfigBaseUploadManagerURL] = baseUploadManagerURL
        config[source.ConfigRootCAs] = rootCAs
        unpacker, err := pkgutil.NewDefaultUnpacker(config)

WDYT?

timflannagan commented 2 years ago

@akihikokuroda I tend to think that having an explicit config parameter for a constructor makes the API a bit more extensible, but for consumers that just want to use the default configuration for an unpacker, it's a bit awkward to use pkgutil.NewDefaultUnpacker(map[string]interface{}{})? This is probably where the functional options pattern that gets used extensively throughout go comes into play.

Also, I think it would be easier to use an explicit struct vs. relying on a map string -> interface k/v pairs.

akihikokuroda commented 2 years ago

@timflannagan When we add a new source that requires additional parameters, the API doesn't need to be changed and it can keep the backward compatibility with the map. I can change to an explicit struct.

timflannagan commented 2 years ago

@akihikokuroda Right, you'd have a function signature of func NewUnpacker(option ...UnpackerOption) that's variadic, so adding introducing new API capabilities doesn't require existing clients to update their usage of the function.

Dave does a much better job of explaining this than I do: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis.

timflannagan commented 2 years ago

What's out game plan for moving the helm provisioner out-of-tree in terms of repository location? I think we could continue to house it within the operator-framework organization.

akihikokuroda commented 2 years ago

These 3 PRs complete exporting packages for now. #497, #500, and #527.

akihikokuroda commented 2 years ago

Can we have another repository in the operator-framework organization so that we can duplicate the helm provisioner in it to find any issues for the out-of-tree provisioner?

tylerslaton commented 2 years ago

@akihikokuroda Yep, that sounds like a good plan. I'm working with the team to get that repo created so we can start iterating on it.

github-actions[bot] commented 1 year ago

This issue has become stale because it has been open 60 days with no activity. The maintainers of this repo will remove this label during issue triage or it will be removed automatically after an update. Adding the lifecycle/frozen label will cause this issue to ignore lifecycle events.