Skip to content
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

designs: extensible CLI and scaffolding plugins #1250

Merged
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
147 changes: 147 additions & 0 deletions designs/extensible-cli-and-scaffolding-plugins.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
# Extensible CLI and Scaffolding Plugins

## Overview
I would like for Kubebuilder to become more extensible, such that it could be imported and used as a library in other projects. Specifically, I'm looking for a way to use Kubebuilder's existing CLI and scaffolding for Go projects, but to also be able to augment the Kubebuilder project structure with other custom project types so that I can support the Kubebuilder workflow with non-Go operators (e.g. operator-sdk's Ansible and Helm-based operators).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use version for project versioning e.g. 1, 2. For this use-case, we can use the term types to refer different types of Operator projects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, I has to re-read from the start once I understood that version was not refering to a version but to a type

The idea is for Kubebuilder to define one or more plugin interfaces that can be used to drive what the `init`, `create api` and `create webhooks` subcommands do and to add a new `cli` package that other projects can use to integrate out-of-tree plugins with the Kubebuilder CLI in their own projects.

## Related issues and PRs

* [#1148](https://github.com/kubernetes-sigs/kubebuilder/pull/1148)
* [#1171](https://github.com/kubernetes-sigs/kubebuilder/pull/1171)
* Possibly [#1218](https://github.com/kubernetes-sigs/kubebuilder/issues/1218)

## Prototype implementation
https://github.com/joelanford/kubebuilder-exp

## Plugin interfaces

### Required
Each plugin would minimally be required to implement the `Plugin` interface.

```go
type Plugin interface {
// Version returns the project version that this plugin implements.
// For example, Kubebuilder's Go v2 plugin implementation would return 2.
Version() uint
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should keep this as Version() string as that will allow more flexible versioning formats as 2.1.0 or 2.3-alpha1 similar to api version names-

// Name returns a name defining the plugin type.
// For example, Kubebuilder's plugins would return "go".
Name() string
}
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better would be call this method Name instead of Version. Version indicates the version of the plugin itself that could v1, v2,....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comment at #1250 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also lean to both Name() string and Version() string


### Optional
Next, a plugin could optionally implement further interfaces to declare its support for specific Kubebuilder subcommands. For example:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be achievable without specific subcommand interfaces? Having specific interfaces for Init, CreateAPI, ... would limit the subcommands to these list and would require modifying the source when any other subcommand wants to be added. What about the following:

type Plugin interface {
    // Name is the name of this plugin.
    // For example, Kubebuilder's Go v2 plugin implementation would return "kubebuilder"
    Name() string
    // Version is the project version that this plugin implements.
    // For example, Kubebuilder's Go v2 plugin implementation would return "2"
    Version() string
    // Subcommands returns the different subcommands that this plugin implements.
    Subcommands() []Subcommand
}

type Subcommand struct {
    // Command returns the subcommand that this type is implementing.
    // For example, `kubebuilder create api` subcommand implementation would return `[]string{"create", "api"}`
    Command() []string
    // Description returns a description of what this subcommand does. It is used to display help.
    Description() string
    // Example returns one or more examples of the command-line usage
    // of this plugin's project subcommand support. It is used to display help.
    Example() string
    // BindFlags binds the plugin's flags to the CLI. This allows each plugin to define its own
    // command line flags for the kubebuilder subcommand.
    BindFlags(fs *pflag.FlagSet)
    // Run runs the subcommand.
    Run() error
}

Copy link
Contributor

@estroz estroz Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how plugin interfaces are lain out in the current POC, I'd say this is a good idea.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we went with the above approach, I would think that Subcommand may need optional Subcommands. I don't like the idea of having to understand "create", "API" -> create API. Reading this into a CLI parser would be difficult right?

I think my vote would be for having specific interfaces for specific commands.

  1. If we run into a case where it makes sense to pass more info around, we would need to make this generic to fit the above model. This could lead to lots of confusion in the future IMO.
  2. Being explicit about when I want my plugin to run if I am choosing an override means that if and when kubebuilder adds a new command, I am not accidentally overriding/layering it in my plugin, with a generic subcommand. I think that this is a good thing.
  3. Marking deprecation and changes to a plugin interface will be easier if we have concrete types rather than implementations of a generic interface IMO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we went with the above approach, I would think that Subcommand may need optional Subcommands. I don't like the idea of having to understand "create", "API" -> create API. Reading this into a CLI parser would be difficult right?

Subcommand.Command is a []string. kubebuilder create api would return []string{"create", "api"}. The recursive approach could also be done by adding a Subcommands() []Subcommand and changing Command string but I usually tend to avoid recursion.

  1. If we run into a case where it makes sense to pass more info around, we would need to make this generic to fit the above model. This could lead to lots of confusion in the future IMO.

The info is passed through the flags, the commands layer has no info to pass around. Could you think of any example?

  1. Being explicit about when I want my plugin to run if I am choosing an override means that if and when kubebuilder adds a new command, I am not accidentally overriding/layering it in my plugin, with a generic subcommand. I think that this is a good thing.

The counter part is that no extension could develop any command that kubebuilder doesn't offer as a Plugin interface, while the geenric approach would allow it.

  1. Marking deprecation and changes to a plugin interface will be easier if we have concrete types rather than implementations of a generic interface IMO.

The Deprecated interface could still be implemented and checked with generic interface, which difficulty are you seeing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The info is passed through the flags, the commands layer has no info to pass around. Could you think of any example?

There might be other metadata or state to inject. For example, configuration from the PROJECT file, plugin configuration, environment info, etc. I suppose that could also be defined similar to the Deprecated interface, where the CLI checks if the subcommand implements some injection function(s).

The counter part is that no extension could develop any command that kubebuilder doesn't offer as a Plugin interface, while the geenric approach would allow it.

The WithExtraCommands() functional option to cli.New would allow other binaries to implement their own subcommands, with the caveat that they wouldn't be allowed to override subcommands defined and used by Kubebuilder. So init and create would have well-defined subcommands and interfaces for customizing their logic, but they would not allow plugins to define other custom subcommands.

Using Operator SDK as an example, it would be possible to create a new subcommand that provides integrations with OLM and pass it into the CLI with cli.New(cli.WithExtraCommands(olmCmd))

IMO, preventing open-ended subcommand building in Kubebuilder's built-in subcommand trees would be a feature that ensure's that the Kubebuilder-defined operator development workflow is consistent across different extensions/binaries.

Another potential pain point with this approach is that it sort of locks Kubebuilder into this interface for all subcommands. I can't think of a great example at the moment, but I could envision a requirement where maybe the init subcommand interface needs to have an extra required method on its plugin interface.

Perhaps a middle ground would be something like this:

type Plugin interface { ... }

type InitPluginGetter interface {
    Plugin
    GetInitPlugin() InitPlugin
}

type InitPlugin interface {
    GenericSubcommand
}

type GenericSubcommand interface {
    // Description returns a description of what this subcommand does. It is used to display help.
    Description() string
    // Example returns one or more examples of the command-line usage
    // of this plugin's project subcommand support. It is used to display help.
    Example() string
    // BindFlags binds the plugin's flags to the CLI. This allows each plugin to define its own
    // command line flags for the kubebuilder subcommand.
    BindFlags(fs *pflag.FlagSet)
    // Run runs the subcommand.
    Run() error
}

In this case, we would still have explicit subcommands and type checking, most subcommand plugins could use the GenericSubcommand interface, but it would give us some flexibility in the future to allow changing individual plugin definitions, while still retaining compiler enforced type checking/safety on Kubebuilder's subcommands.

* `InitPlugin` - to initialize new projects
* `CreateAPIPlugin` - to create APIs (and possibly controllers) for existing projects
* `CreateWebhookPlugin` - to create webhooks for existing projects

Each of these interfaces would follow the same pattern (see the `InitPlugin` interface example below).

```go
type InitPlugin interface {
Plugin

// InitDescription returns a description of what this plugin initializes
// for a new project. It is used to display help.
InitDescription() string

// InitHelp returns one or more examples of the command-line usage
// of this plugin's project initialization support. It is used to display help.
InitExample() string

// BindInitFlags binds the plugin's init flags to the CLI. This allows each
// plugin to define its own command line flags for the `kubebuilder init`
// subcommand.
BindInitFlags(fs *pflag.FlagSet)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I kinda don't want to suggest this, but I figured I throw it out: all the CLI arg logic from CT is in its own package, and can be used with arbitrary CLI markers. It's used in a couple of the helper commands. but it completely ignores existing UX for CLI programs, so we probably don't want to use it

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the benefit of doing this vs. using pflag.FlagSet and the plugin are responsible for adding its own flags and the types that it uses internally for the flags.

I am having a tough time seeing the benefit of that approach vs this approach which feels pretty common to me?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess not exposing pflag?

Copy link
Contributor

@estroz estroz Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could wrap *pflag.FlagSet in a PluginFlagSet struct so we can change the underlying flagset implementation later while keeping the interface forward-compatible:

type PluginFlagSet struct {
	*pflag.FlagSet
}

type InitPlugin interface {
	...
	BindInitFlags(PluginFlagSet)
}

That way we can deprecate pflag by using a conversion function to transform *pflag.FlagSet to a new flagset type until we remove support entirely. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the result of all this back and forth might just be that avoiding pflag (and having cobra in the interface) is more work than it's worth ATM. Sorry for starting all that. Let's just go forward with pflag for now, and we can always re-adjust a bit later if we really need to.


// Init initializes a project.
Init() error
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One high level pattern is -- There will be metadata associated with the project (ex. domain, groups...) and type (helm, golang...), we need a way to persist that data in probably PROJECT file and each subcommand invocation can be provided with that metadata and can be written back at the end during the invocation or at the end.
If we can come up with a generic metadata interface (Generic Key-Value may be ?) on PROJECT file, then we can decouple commandline args from the plugins.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this sounds like a necessity. I like @DirectXMan12's suggestion from above. Is this along the lines of what you're looking for?

```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I have a plugin that implements InitPlugin interface, does it override the default init behavior or does it augment the default behavior ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to #1250 (comment) it seems you queue Init commands using kubebuilder Init as the root.


#### Deprecated Plugins

To generically support deprecated project versions, we could also add a `Deprecated` interface that the CLI could use to decide when to print deprecation warnings:

```go
// Deprecated is an interface that, if implemented, informs the CLI
// that the plugin is deprecated. The CLI uses this to print deprecation
// warnings when the plugin is in use.
type Deprecated interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 :-)

// DeprecationWarning returns a deprecation message that callers
// can use to warn users of deprecations
DeprecationWarning() string
}
```

## CLI

To make the above plugin system extensible and usable by other projects, we could add a new CLI package that Kubebuilder (and other projects) could use as their entrypoint.

Example Kubebuilder main.go:

```go
func main() {
c, err := cli.New(
cli.WithPlugins(
&golangv1.Plugin{},
&golangv2.Plugin{},
),
)
if err != nil {
log.Fatal(err)
}
if err := c.Run(); err != nil {
log.Fatal(err)
}
}
```

Example Operator SDK main.go:

```go
func main() {
c, err := cli.New(
cli.WithCommandName("operator-sdk"),
cli.WithDefaultProjectVersion("2"),
cli.WithExtraCommands(newCustomCobraCmd()),
cli.WithPlugins(
&golangv1.Plugin{},
&golangv2.Plugin{},
&helmv1.Plugin{},
&ansiblev1.Plugin{},
),
)
if err != nil {
log.Fatal(err)
}
if err := c.Run(); err != nil {
log.Fatal(err)
}
}
```

## Comments & Questions

### Cobra Commands
As discussed earlier as part of [#1148](https://github.com/kubernetes-sigs/kubebuilder/pull/1148), one goal is to eliminate the use of `cobra.Command` in the exported API of Kubebuilder since that is considered an internal implementation detail.

However, at some point, projects that make use of this extensibility will likely want to integrate their own subcommands. In this proposal, `cli.WithExtraCommands()` _DOES_ expose `cobra.Command` to allow callers to pass their own subcommands to the CLI.

In [#1148](https://github.com/kubernetes-sigs/kubebuilder/pull/1148), callers would use Kubebuilder's cobra commands to build their CLI. Here, control of the CLI is retained by Kubebuilder, and callers pass their subcommands to Kubebuilder. This has several benefits:
1. Kubebuilder's CLI subcommands are never exposed except via the explicit plugin interface. This allows the Kubebuilder project to re-implement its subcommand internals without worrying about backwards compatibility of consumers of Kubebuilder's CLI.
2. If desired, Kubebuilder could ensure that extra subcommands do not overwrite/reuse the existing Kubebuilder subcommand names. For example, only Kubebuilder gets to define the `init` subcommand
3. The overall binary's help handling is self-contained in Kubebuilder's CLI. Callers don't have to figure out how to have a cohesive help output between the Kubebuilder CLI and their own custom subcommands.

With all of that said, even this exposure of `cobra.Command` could be problematic. If Kubebuilder decides in the future to transition to a different CLI framework (or to roll its own) it has to either continue maintaining support for these extra cobra commands passed into it, or it was to break the CLI API.

Are there other ideas for how to handle the following requirements?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as I don't want to expose cobra, I kinda feel like if we try we might end up re-implementing it poorly. As a thought exercise:

What do we need/want?

  1. Cohesive --help with additional subcommands listed based on the --scaffold flag and the persistent plugins
  2. Additional subcommands that basically have all args and unknown flags passed to them
  3. (nice to have from a UX perspective) common flags may be specified anywhere on the command line, not just before the subcommand

anything I'm missing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's going to be hard to avoid exposing cobra.

* Eliminate use of cobra in CLI interface
* Allow other projects to have custom subcommands
* Support cohesive help output

### Other
1. Should the `InitPlugin` interface methods be required of all plugins?
2. Any other approaches or ideas?
3. Anything I didn't cover that could use more explanation?