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

Restructure kubectl plugin execution #2994

Merged
merged 4 commits into from
Apr 14, 2023
Merged

Conversation

twz123
Copy link
Member

@twz123 twz123 commented Apr 13, 2023

Description

This PR will enable better integration with kubectl plugins and will prepare k0s for KEP-3638. See #2879.

K0s used the NewDefaultKubectlCommandWithArgs function to obtain a kubelet command to be embedded into k0s. That function merely executes its plugin logic unconditionally, i.e. it won't wait until that command is being executed by Cobra. This lead to a situation in which kubectl plugins not only shadowed kubectl commands, but also k0s commands. An inttest was added to prove that, before this change, an executable named kubectl-airgap would effectively replace the builtin k0s airgap command.

Address this by using a two-pass approach when executing the embedded kubectl command. First, during the Cobra bootstrap phase, call NewKubectlCommand to create a Cobra command without triggering the plugin execution logic. Then patch the command's callbacks, so that when the command is actually invoked by Cobra, the NewDefaultKubectlCommandWithArgs function is called. That function will execute plugins and never return, if any. Otherwise it will just return a new Cobra command, which k0s will simply discard and use the previously created one.

Also don't print kubectl usage on unknown subcommands. The basic kubectl command will never accept any arguments. This will be handled more or less automatically by Cobra when used as a root command. When run as a subcommand, Cobra will instead pass all of the arguments to the command's run methods, which will trigger some unexpected output. Address this by specifying cobra.NoArgs for the kubectl command.

Reorder persistent flag registration for k0s kc, so that they are grouped together. Also add InitLogs/FlushLogs, which is done by k8s.io/component-base/cli, which k0s doesn't use.

Streamline kubectl integration test: The common test matrix wasn't checking for stderr contents so far. Address this by capturing both stdout and stderr when executing the test commands and forward those along with the execution error to the check function. Add asserts for stderr contents to all the tests in the matrix. Moreover, the integration test environment wasn't completely isolated. For example, the support-bundle plugin that's installed in the footloose image was leaking into the test setup. Address this by setting up a private PATH and use that when invoking the test commands. That way, the plugin-list and symlink-warning test cases can be integrated into the common test matrix as well.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Manual test
  • Auto test added

Checklist:

  • My code follows the style guidelines of this project
  • My commit messages are signed-off
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

twz123 added 4 commits April 13, 2023 17:12
The common test matrix wasn't checking for stderr contents so far.
Address this by capturing both stdout and stderr when executing the test
commands and forward those along with the execution error to the check
function. Add asserts for stderr contents to all the tests in the
matrix.

Moreover, the integration test environment wasn't completely isolated.
for example, the support-bundle plugin that's installed in the footloose
image was leaking into the test setup. Address this by setting up a
private PATH and use that when invoking the test commands. That way, the
plugin-list and symlink-warning test cases can be integrated into the
common test matrix as well.

Signed-off-by: Tom Wieczorek <[email protected]>
The basic kubectl command will never accept any arguments. This will be
handled more or less automatically by Cobra when used as a root command.
When run as a subcommand, Cobra will instead pass all of the arguments
to the command's run methods, which will trigger some unexpected output.
Address this by specifying NoArgs for the kubectl command.

Signed-off-by: Tom Wieczorek <[email protected]>
So that they are grouped together. Also add InitLogs/FlushLogs, which is
done by k8s.io/component-base/cli, which k0s doesn't use.

Signed-off-by: Tom Wieczorek <[email protected]>
K0s used the NewDefaultKubectlCommandWithArgs function to obtain a
kubelet command to be embedded into k0s. That function merely executes
its plugin logic unconditionally, i.e. it won't wait until that command
is being executed by Cobra. This lead to a situation in which kubectl
plugins not only shadowed kubectl commands, but also k0s commands. An
inntest was added to prove that, before this change, an executable named
kubectl-airgap would effectively replace the builtin k0s airgap command.

Address this by using a two-pass approach when executing the
embedded kubectl command. First, during the Cobra bootstrap phase,
call NewKubectlCommand to create a Cobra command without triggering
the plugin execution logic. Then patch the command's callbacks,
so that when the command is _actually_ invoked by Cobra, the
NewDefaultKubectlCommandWithArgs function is called. That function
will execute plugins and never return, if any. Otherwise it will just
return a new Cobra command, which k0s will simply discard and use the
previously created one.

Signed-off-by: Tom Wieczorek <[email protected]>
@twz123 twz123 added bug Something isn't working area/cli labels Apr 13, 2023
@twz123 twz123 mentioned this pull request Apr 13, 2023
11 tasks
@twz123 twz123 marked this pull request as ready for review April 14, 2023 06:21
@twz123 twz123 requested a review from a team as a code owner April 14, 2023 06:21
@twz123 twz123 requested review from ncopa and jnummelin April 14, 2023 06:21
@twz123 twz123 mentioned this pull request Apr 14, 2023
16 tasks
}

// In vanilla kubectl, log initialization and flushing is handled by
// k8s.io/component-base/cli. But k0s doesn't use it, so it needs to
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: I was totally sure we use component-base/cli, I wonder where does this false memory come from

Copy link
Member Author

Choose a reason for hiding this comment

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

I digged a bit in the GitHub history when I wrote that comment and found a superseeded PR of yours. That might be the reason:

@twz123 twz123 merged commit ea40a9f into k0sproject:main Apr 14, 2023
@twz123 twz123 deleted the k0s-kc-inttest branch April 14, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants