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

🌱 [WIP] Proposal for Phase 2 Plugins #1801

Conversation

rashmigottipati
Copy link
Contributor

Description of the change
This PR adds a design proposal for Kubebuilder Phase 2 Plugins

Motivation for the change
As the proposed scaffold plugin system is implemented, looking forward to achieving agreement on a way forward for Kubebuilder Phase 2 Plugins so that kubebuilder and operator sdk projects can align on requirements and progress on phase 2, which adds support for chaining plugins together

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 9, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @rashmigottipati. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 9, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rashmigottipati
To complete the pull request process, please assign directxman12 after the PR has been reviewed.
You can assign the PR to them by writing /assign @directxman12 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 9, 2020
@rashmigottipati rashmigottipati changed the title 🌱 : [WIP] Proposal for Phase 2 Plugins 🌱 [WIP] Proposal for Phase 2 Plugins Nov 9, 2020
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

It is nice. I added a few questions.


## Overview

Phase 2 of the scaffold plugin system adds support for chaining plugins together. The idea for plugins phase 2 is to have support for a plugin architecture that enables plugins to be separate binaries discoverable by the Kubebuilder CLI binary via user specified plugin file paths and that they are language indepedent. Some of the phase 2 high-level requirements summarized from the feature request and discussions are:
Copy link
Member

Choose a reason for hiding this comment

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

what do you mean with enables plugins to be separate binaries? We will build each plugin? Will we not so longer extending the code implementation?


##### Go built-in plugin package vs Hashicorp plugin library vs custom plugin library

Based on the evaluation of plugin libraries such as the built-in go-plugin library and Hashicorp’s plugin library, we have come to the conclusion that it is more suitable to write our own custom typed plugin library.
Copy link
Member

Choose a reason for hiding this comment

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

Could we add the link as a reference/link of the libraries that are cited here?

Config *config.Config `json:"config,omitempty"`

// Files contains the model of the files that are being scaffolded
Files map[string]File `json:"files,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

are we speaking here about an array with all boilerplates?

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 it is talking about already templated files. Is there anyway that allows us to pass it before templating and templating after all plugins have run? I think we may be able to abstract part of the templating and scaffolding code so that plugins are easier to develop.

The proposed plugin universe is:

```go
type File struct {
Copy link
Member

Choose a reason for hiding this comment

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

ìs the File our boilerplate? (template used to scaffold the files)

Contents string `json:"contents,omitempty"`

// Info is the os.FileInfo
Info os.FileInfo `json:"info,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

why we need to have an attribute such as Info os.FileInfo? Is to allow write a dir for example? Could you please add the use cases?

Comment on lines +67 to +69
To support phase 2 plugins, we propose that we modify the "layout" PROJECT file key to either of these options as order matters for certain plugins:
* Ordered list of plugin {name}/{version} or
* Ordered map of {name}/{version} to plugin-specific config
Copy link
Member

Choose a reason for hiding this comment

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

The PROJECT file represents the input/config of the project. so, are you saying here that we need to store the plugins used in the order that they were executed? Am I right?


```yaml
version: "3-alpha"
layout: ["go/v1.0.0", "go.kubebuilder.io/v3-alpha"]
Copy link
Member

Choose a reason for hiding this comment

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

the layout means the father of the plugins used to scaffold the project. So, what would mean "go/v1.0.0", "go.kubebuilder.io/v3-alpha"? Could we give an example of a scenario that we would like to address and then have the project file as it would be in this scenario for a better understand?

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 this example is confusing, as both plugins are the base go plugin (one with the short form and the other one the full name) which doesn't make sense. Additionally, we have v2 and v3 versions of that plugin, no v1.0.0 nor v3-alpha, to add up to the confussion.

version: v1
plugins:
- go.sdk.operatorframework.io/v2-alpha: {
type: JSON
Copy link
Member

Choose a reason for hiding this comment

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

Since the tool has access to the plugin implementation, why we need to add the type here? Could you please add the example scenario and then describe why/when the Type is used in the process to illustrate why it is required at all?

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 type is meant to determine how the universe is passed. JSON doesn't seem to be a good name election as it is just a encoding language. I would say in-tree or built-in or something similar for the ones that we already have and stdin/stdout or something similar for the ones that you are proposing in this document.

type: JSON
}
```

Copy link
Member

Choose a reason for hiding this comment

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

Could we add a real/valid scenario for we have a better understanding of the purposed idea? E.g Something such as: add the use case then address the scenario with the example.

I am as X, would like to N, so that I can Z.

  1. Run the command : otherbin init --plugins=[a,b]
  2. Then, update the path path of the plugin
  3. Do the scaffold
  4. The Project file would be like as:

* Either an ordered list of plugin {name}/{version}, or ordered map of {name}/{version} to plugin-specific config.
* Order matters for certain plugins, ex. a bazel plugin would need to run after all Go files are generated.

## Evaluation of Plugin libraries
Copy link
Member

Choose a reason for hiding this comment

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

Could we add here the use cases that we are trying to address with this proposal (I am as X, would like to N, so that I can Z.)? Also, could we add how we will test the plugins and ensure its quality?


## Comments & Questions

* How to handle help text? Are plugins allowed to define their own flags?
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add an example over who the consumer will consume and customize the plugins in phase 2? Or are we not covering it in this phase? For example, I am as a consumer(sdk) of kubebuilder plugins, would like to extend the init subcommand/plugin and define that the main.go will be a scaffold in a new path, so that for my tool can do the same scaffolds but in another path.

Also, would be nice we have here what are the goals and no goals of this EP. So, if it is not a goal of this proposal we would easily know.

Copy link
Contributor

@Adirio Adirio left a comment

Choose a reason for hiding this comment

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

Several questions, mostly to clarify myself:


## Overview

Phase 2 of the scaffold plugin system adds support for chaining plugins together. The idea for plugins phase 2 is to have support for a plugin architecture that enables plugins to be separate binaries discoverable by the Kubebuilder CLI binary via user specified plugin file paths and that they are language indepedent. Some of the phase 2 high-level requirements summarized from the feature request and discussions are:
Copy link
Contributor

Choose a reason for hiding this comment

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

Interpreted languages, i.e. Python, do not generate binaries. Are we leaving them apart intentionally? I think that interpreted languages are well suited for this kind of scripting purposes and considering them an option would be interesting.


* mdbook-like structure
* A Plugin receives a JSON blob plugin "universe" from upstream plugins containing file "nodes", each of which contain file metadata.
* The plugin can add or delete nodes (nodes are not mutable), then returns the modified universe so downstream plugins can do the same.
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 like to discuss why nodes need to be inmutable. There are probably good reasons for you to consider them like that, I just want to know them.


The built-in plugin library seems to be a non-starter as it is more suitable for in-tree plugins rather than out-of-tree and it doesn’t offer cross language support. Hashicorp’s go plugin system seems more suitable than built-in go-plugin library as it enables cross language/platform support. However, it is more suited for long running plugins as opposed to short lived plugins and it could be overkill with the usage of protobuf.

For the stated reasons, the proposal is to write our own plugin system that passes JSON blobs back and forth across stdin/stdout.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, probably good reasons to choose stdin/stdout. Could we addd some justification why we chose this approach isntead of, for example, sockets?

Config *config.Config `json:"config,omitempty"`

// Files contains the model of the files that are being scaffolded
Files map[string]File `json:"files,omitempty"`
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 it is talking about already templated files. Is there anyway that allows us to pass it before templating and templating after all plugins have run? I think we may be able to abstract part of the templating and scaffolding code so that plugins are easier to develop.


```yaml
version: "3-alpha"
layout: ["go/v1.0.0", "go.kubebuilder.io/v3-alpha"]
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 this example is confusing, as both plugins are the base go plugin (one with the short form and the other one the full name) which doesn't make sense. Additionally, we have v2 and v3 versions of that plugin, no v1.0.0 nor v3-alpha, to add up to the confussion.

version: v1
plugins:
- go.sdk.operatorframework.io/v2-alpha: {
type: JSON
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 type is meant to determine how the universe is passed. JSON doesn't seem to be a good name election as it is just a encoding language. I would say in-tree or built-in or something similar for the ones that we already have and stdin/stdout or something similar for the ones that you are proposing in this document.

kind: Captain
version: v1
plugins:
- go.sdk.operatorframework.io/v2-alpha: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't match any of the plugins in layout. How should we interpret this?


## Comments & Questions

* How to handle help text? Are plugins allowed to define their own flags?
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 say that a *.yaml file that defines the characteristics of the plugin could be used. For example it could define that we should call it with python file-of-the-plugin.py and that uses stdin and stdout to communicate with the main executable. When the main executable discovers the plugins it would read this file and register that plugin. In this file we could also add thing like allowed flags, commands, descriptions, etc.

## Comments & Questions

* How to handle help text? Are plugins allowed to define their own flags?
* Do we allow plugins to depend on environment variables?
Copy link
Contributor

Choose a reason for hiding this comment

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

Cons: the same plugin call would do different things depending on the environment
Difficulties: hard to enforce a plugin not to use environmental variables when it is an external executable for you.

@Adirio
Copy link
Contributor

Adirio commented Nov 15, 2020

Prow only detects WIP if they are the first word in the title, so I'm manually holding it as it won't get the hold:work-in-progress label automatically. Feel free to remove the hold once it is not a WIP.

/hold
/ok-to-test

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 15, 2020
@Adirio Adirio mentioned this pull request Nov 17, 2020
2 tasks
Copy link
Contributor

@Adirio Adirio left a comment

Choose a reason for hiding this comment

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

Some additional considerations


## Overview

Phase 2 of the scaffold plugin system adds support for chaining plugins together. The idea for plugins phase 2 is to have support for a plugin architecture that enables plugins to be separate binaries discoverable by the Kubebuilder CLI binary via user specified plugin file paths and that they are language indepedent. Some of the phase 2 high-level requirements summarized from the feature request and discussions are:
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like there are 2 steps here:

  1. Chain plugins
  2. Enable external/out-of-tree plugins
    I will refer to Phase 1.5 to achieve the first step and Phase 2 to achieve both.

Comment on lines +10 to +13
* Pre-generate a project file for handling complex plugin use-cases.
* Enhanced "layout" PROJECT file key.
* Either an ordered list of plugin {name}/{version}, or ordered map of {name}/{version} to plugin-specific config.
* Order matters for certain plugins, ex. a bazel plugin would need to run after all Go files are generated.
Copy link
Contributor

Choose a reason for hiding this comment

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

The current project file supports a comma-separated lists of plugins in the layout field and offers a unstructured field were plugins can store their config. While I do like the suggested approach of having an ordered map with plugin-keys to configs, that is a breaking change and would require project version 4. Project version 3 (current) is still able to support our needs so this can be implemented without having to wait for the fourth project version.

The built-in plugin library seems to be a non-starter as it is more suitable for in-tree plugins rather than out-of-tree and it doesn’t offer cross language support. Hashicorp’s go plugin system seems more suitable than built-in go-plugin library as it enables cross language/platform support. However, it is more suited for long running plugins as opposed to short lived plugins and it could be overkill with the usage of protobuf.

For the stated reasons, the proposal is to write our own plugin system that passes JSON blobs back and forth across stdin/stdout.
The plugin specification will include type metadata to allow the potential for using other plugin libraries in the future if the need arises. From an implementation standpoint, we could create a new implementation of our plugin interface based on the type specified in the project file. We can do a type switch on the type of plugin the data is being passed to. For ex: if it is native go, then we pass the universe directly & if it is a binary wrapper, then we pass the serialized stream of JSON bytes to it.
Copy link
Contributor

Choose a reason for hiding this comment

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

From the implementation viewpoint, instead of making the cli understand all different kind of plugins, we could code a plugin with the current interface that is in charge of doing this translation. For example, we have a JSON plugin that has the current plugin interface and creates the JSON blobs and pass them to the external plugin.

Now, imagine I'm a C++ plugin developer, I would copy this JSON plugin and develop my C++ plugin that interacts with the JSON-based exposed API.

By doing this, we would only need to support out-of-tree Go plugins, as the integration with other languages would be done by this JSON (and potentially others in a future) plugins. What do you hink about this approach? And would this approach bring the built-in plugin library back to the table?

By the way, I think that these plugins should work under demand more than sending all the data. The C++ plugin would request the admiral_types.go file representation and the plugin would sent it in a JSON blob. Obviously the plugin will send some general metadata first (for example, but not limitd to, the plugin config).

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Hi @rashmigottipati,

It is open for a while and without follow up to address the comments. Also, we have the 1.5 EP which was accepted and is implemented already and very close to be merged. See: #2060

This shows that the plugin phase 2.0 after that get merged and the issues in #2016 are addressed might only be more about check the possibility of the plugins be as separate binaries that can be discovered by the Kubebuilder CLI binary via user-specified plugin file paths.

So, could you please follow that and check if this one could be closed or updated to address what will be missing accordingly?

@rashmigottipati
Copy link
Contributor Author

Closing this as it makes more sense to create a new proposal that incorporates plugin phase 1.5 implementation and discusses design of plugins as separate binaries that can be discovered by the Kubebuilder CLI binary via user-specified plugin file paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants