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

Proposal: Allow inlining of golang plugins #928

Closed
rhuss opened this issue Jul 13, 2020 · 9 comments · Fixed by #902
Closed

Proposal: Allow inlining of golang plugins #928

rhuss opened this issue Jul 13, 2020 · 9 comments · Fixed by #902
Labels
kind/proposal Issues or PRs related to proposals.
Milestone

Comments

@rhuss
Copy link
Contributor

rhuss commented Jul 13, 2020

kn provides a plugin architecture that allows calling to external commands via a naming convention (much like kubectl). This proposal suggests a small modification in kns plugin handling algorithm to allow also to include golang-based plugins internally plugins so that are compiled together with kn and that implements a Plugin golang interface. The required changes are implemented in PR #902.

Use Cases

Allowing to package plugins together with kn during build-time allows for an easy distribution mechanism with a single binary, so a plugin package management (like krew for kubectl) is not needed. It also allows third-parties to provide value-add by maintaining a curated set of plugins and align them dependency-wise so that they can be compiled together.

kn itself is agnostic of the underlying platform and only uses the endpoints that the Knative API defines. However, a vast majority of Knative installations run on Kubernetes, so having Kubernetes specific functionality implemented as kn internal plugins that integrate seamlessly into the given command structure is valuable for Knative Kubernetes users.

Examples of such Kubernetes specific plugins:

  • kn admin for managing the Knative configuration
  • kn service log for showing the Knative logs

Additionally, inlining plugins for managing Event Sources also helps in creating a better user and installation experience:

  • kn source kafka for managing a Kafka source
  • kn source github likewise, for a GitHub source

Scope

This proposal suggests only to offer the infrastructure for allowing such tight integration. Creating custom distributions of kn is out of scope for this proposal and left to third-parties. The integration must be minimally intrusive and not conflict with externally managed plugins.

From UX perspective, inlined plugins (as well as externally managed plugins) should integrate seamlessly into the command structure of the builtin commands so that from a user's POV there is no notable difference between built-in commands and plugins (i.e. the should appear on the same help pages as the commands and also follow the same CLI conventions for subcommand and option names).

Code & Examples

  • PR 902 which implemented the lookup and management of internal plugins via a global slice to which plugins can add to via their init() method.
  • kn service log plugin that uses this implementation
  • Demo [5:34] which shows the kn service log plugin used as an external and internal plugin.
@rhuss rhuss added the kind/proposal Issues or PRs related to proposals. label Jul 13, 2020
@rhuss
Copy link
Contributor Author

rhuss commented Jul 21, 2020

Here's an example how a plugin can leverage this. The sample in https://github.com/rhuss/kn-service-log shows the registration process:

package plugin

import (
	"os"

	"knative.dev/client/pkg/kn/plugin"

	"github.com/rhuss/kn-service-log/pkg"
)

func init() {
	plugin.InternalPlugins = append(plugin.InternalPlugins, &logPlugin{})
}

type logPlugin struct {}

func (l *logPlugin) Name() string {
	return "kn-service-log"
}

func (l *logPlugin) Execute(args []string) error {
	cmd := pkg.NewLogCommand()
	oldArgs := os.Args
	defer (func() {
		os.Args = oldArgs
	})()
	os.Args = append([]string { "kn-service-log" }, args...)
	return cmd.Execute()
}

func (l *logPlugin) Description() (string, error) {
	return "Print logs of pods belonging to a service", nil
}

func (l *logPlugin) CommandParts() []string {
	return []string{ "service", "log" }
}

// Path is empty because its an internal plugins
func (l *logPlugin) Path() string {
	return ""
}
  • The important part is the init() method. This will append an instance of the plugin to a global slice plugin.InternalPlugins(). This slice is managed by kn itself.
func init() {
	plugin.InternalPlugins = append(plugin.InternalPlugins, &logPlugin{})
}
  • Otherwise that package is not used directly by the plugin (which still can be used as a regular plugin)

  • In kn you need to create a package which references this plugin package from the plugin. Create a file 'plugin_register' within e.g. the "root" package and the content:

package root

import (
        _ "github.com/rhuss/kn-service-log/plugin"
)

// RegisterInlinePlugins is an empty function which however forces the
// compiler to run all init() methods of the registered imports
func RegisterInlinePlugins() {}

Note the import which references the plugin mentioned above and also triggers the init() function to be called.

If you now compile such a modified kn you automatically have the plugin inlined.

@daisy-ycguo
Copy link
Member

In kn you need to create a package which references this plugin package from the plugin. Create a file 'plugin_register' within e.g. the "root" package and the content:

A question to this item, if I want to register an inline plugin, do I have to change the source code of kn client ? or in another word, do I have to submit a PR to kn client repo ?

@rhuss
Copy link
Contributor Author

rhuss commented Jul 21, 2020

You would have to create an own variation of the kn build. The idea is that no plugin is added to kn but instead e.g. we could have a kn-builder CLI (in a separate sandbox project), that would do:

  • Evaluate a configuration file that has references to one or more plugin sources and other information (like the name of the binary to create or the version of kn which should be used for building)
  • Checkouts the kn source code
  • Adds a plugins.go file with imports to all referenced plugins (as in the example above)
  • Starts the build via hack/build.sh of the checked-out "kn"
  • Does anything else needed for a distribution.

With this, it's very easy to create an own distribution. The tricky part will be to align the dependencies of plugins and the kn used, but that would be the value-add that somebody creating such a distribution could add.

@maximilien
Copy link
Contributor

maximilien commented Jul 28, 2020

I like this. Overall my main concern would be to make sure (in time) that the kn release uses it. Perhaps by extracting some command groups as plugins OR better embed some client-contrib plugins into kn.

That would have two side effects:

  1. Test this feature for each release since the release would use it;
  2. Have a path for plugins to be embedded (in time) into kn yet be developed and maintained as a plugin

Not urgent but would be nice to have.

@rhuss
Copy link
Contributor Author

rhuss commented Aug 24, 2020

Adressed all comment, ptal

@github-actions
Copy link

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 23, 2020
@navidshaikh navidshaikh added this to the v0.17.0 milestone Nov 23, 2020
@navidshaikh navidshaikh linked a pull request Nov 23, 2020 that will close this issue
@navidshaikh
Copy link
Collaborator

/close

fixed by #902

@knative-prow-robot
Copy link
Contributor

@navidshaikh: Closing this issue.

In response to this:

/close

fixed by #902

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.

@navidshaikh
Copy link
Collaborator

/remove-lifecycle stale

@knative-prow-robot knative-prow-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/proposal Issues or PRs related to proposals.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants