✨ external plugins(plugin phase 2): add support to pass CLI flags

[everettraven]

Description of Change

This change implements the BindFlags() method for the external plugin init, create api, create webhook, and edit subcommands.

It has 2 methods in which flags can be bound:

1. An external plugin defines a flags subcommand that accepts boolean flags to represent which subcommand to return flags to Kubebuilder for. (ex: flags --init)
2. All flags passed by a user are bound when an external plugin is used. This method also adds a default usage message to the flags stating that Kubebuilder was unable to verify the flags passed by a user and suggests that the user consult the external plugin documentation for more information.

The logic flow that is taken is:

flowchart TD
    A[BindFlags] --> B[Request Flags from External Plugin];
    B ----> C{Error?};
    C -- Yes --> D[Parse all flags and provide general message to consult documentation];
    C -- No --> E[Parse only flags specified by External Plugin response];
    D ----> F[Call External Plugin with flags];
    E ----> F;

Loading

Motivation for the change

With the current Phase 2 Plugins implementation, the user passed CLI flags are not properly bound. This results in an error stating that a flag passed by a user is an unknown flag even if the external plugin defines the flag.

For example:
If myexternalplugin/v1 defines the --captain flag and a user runs kubebuilder init --plugins myexternalplugin/v1 --captain kube the output would be:

INFO[0000] Adding external plugin: myexternalplugin     
Error: unknown flag: --captain
Usage:
  kubebuilder init [flags]

Examples:
  # Help for initializing a project with version "2"
  kubebuilder init --project-version="2" -h

  # Help for initializing a project with version "3"
  kubebuilder init --project-version="3" -h

Flags:
  -h, --help                     help for init
      --project-version string   project version (default "3")

Global Flags:
      --plugins strings   plugin keys to be used for this subcommand execution

2022/05/16 09:38:53 unknown flag: --captain

For details on the motivation see issue #2689

Additional Context

resolves #2689

[k8s-ci-robot]

Hi @everettraven. 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.

[camilamacedo86]

I would like to see this scenarios tested with e2e. Example:

By having the sample plugin here and calling them as we do in the others e2e tests.
So that we can ensure the expected results, which would make it simple, we review the changes.

could we try to work on that ? WDYT @rashmigottipati @everettraven ?

[rashmigottipati]

@camilamacedo86 I'll work on a separate PR to add e2e test with the python external plugin I've built. But I wouldn't make e2e tests as a part of this PR, as the goal of this PR is to enable CLI flags for external plugins.

[camilamacedo86]

If we have the e2e tests, we would add a new test/check to ensure that it works.

That would be adding a new call passing the flags and checking the result that value was used in the mock plugin. All prs must be covered with tests and any new feature that we add will need to be covered on the e2e afterwords. ( in this another PR ) So, the best would be if we have it now.

However, to move forward with this one ( without the e2e tests ) could we add in the description of this PR the tests done locally with the results showing that it works as well? Would that be possible? WDYT @rashmigottipati @everettraven ?

[everettraven]

I'd be happy to go through and add as a comment to this thread the reasoning for this change with proof when running it locally.

[everettraven]

When using the original method (adding a print statement to print the args) with the changes that pass the CLI flags to an external plugin, running kubebuilder init --plugins myexternalplugin/v1 --domain example.com results in this error output:

[init --plugins myexternalplugin/v1 --domain example.com]
INFO[0000] Adding external plugin: myexternalplugin     
usage: myexternalplugin.py [-h] [--domain DOMAIN] [--license LICENSE]
                           [--boolean | --no-boolean]
myexternalplugin.py: error: unrecognized arguments: init --plugins myexternalplugin/v1
Error: failed to initialize project: unable to scaffold with "myexternalplugin/v1": exit status 2
Usage:
  kubebuilder init [flags]

Examples:
  # Help for initializing a project with version "2"
  kubebuilder init --project-version="2" -h

  # Help for initializing a project with version "3"
  kubebuilder init --project-version="3" -h

Flags:
      --boolean                  flag to test boolean logic
      --domain string            Project domain (default "my.domain")
      --float float              flag to test float logic (default 123.45)
  -h, --help                     help for init
      --integer int              flag to test integer logic (default 123)
      --license string           Project license (default "apache2")
      --project-version string   project version (default "3")

Global Flags:
      --plugins strings   plugin keys to be used for this subcommand execution

2022/06/01 09:18:05 failed to initialize project: unable to scaffold with "myexternalplugin/v1": exit status 2

The important line to note in that error message is:

myexternalplugin.py: error: unrecognized arguments: init --plugins myexternalplugin/v1

This tells us that we are parsing all arguments after the program name itself, including subcommands and flags that are likely only to be used with Kubebuilder (--plugins flag specifically). We can see this more so from the print statement that I added for testing here:

[init --plugins myexternalplugin/v1 --domain example.com]

The modified function ensures that we only parse flags and the corresponding flag values. It also ensures that we do not parse the --plugins flag since it is likely only used with Kubebuilder.

With the new changes the output of the same command is:

[--domain example.com]
INFO[0000] Adding external plugin: myexternalplugin  

There is no error since the proper flags are passed to the external plugin.

@camilamacedo86 @rashmigottipati I hope this helps demonstrate the reason for the change!

[camilamacedo86]

Would possible here to extend the cobra dep Flag so that we would allow the same options?

[everettraven]

I took a look at doing this and it seems like the way to determine the type of the flag is an interface. I think that structure would make it difficult for external plugin authors to be able to specify the type of the flag, and the new Flag struct introduced below makes it a bit easier IMO.

[everettraven]

For more details on it, here is the godocs for the pflag library that Kubebuilder uses for flags: https://pkg.go.dev/github.com/spf13/pflag

[camilamacedo86]

/ok-to-test

[camilamacedo86]

I am OK with the changes but we need @rashmigottipati OK here since she is who wrote the plugin phase 2 and is responsible for this piece.

But we really need to add the samples/tests with the samples and doc for phase 2 otherwise it is like impossible to keep it maintained and let people know how to use it.

[rashmigottipati]

Nice work @everettraven!
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, everettraven, rashmigottipati

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [camilamacedo86]

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