Fixing bug when looking up plugins on path with slash in argument #1415

psschwei · 2021-08-04T18:49:55Z

Description

Per discussion in PR 1412, found a bug where we fail to look up in path when there's slash in arguments. (This bug was also causing the service import test to fail).

Changes

fixing bug when looking up plugins on path with slash in argument

(changelog entry to follow)

/lint

knative-prow-robot

@psschwei: 0 warnings.

In response to this:

Description

Per discussion in PR 1412, found a bug where we fail to look up in path when there's slash in arguments. (This bug was also causing the service import test to fail).

Changes

fixing bug when looking up plugins on path with slash in argument

(changelog entry to follow)

/lint

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.

codecov · 2021-08-04T18:53:41Z

Codecov Report

Merging #1415 (5d1e08d) into main (9d1bfc6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1415   +/-   ##
=======================================
  Coverage   78.51%   78.51%           
=======================================
  Files         160      160           
  Lines        8283     8285    +2     
=======================================
+ Hits         6503     6505    +2     
  Misses       1097     1097           
  Partials      683      683

Impacted Files	Coverage Δ
pkg/kn/plugin/manager.go	`84.14% <100.00%> (+0.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d1bfc6...5d1e08d. Read the comment docs.

dsimansk · 2021-08-04T19:43:18Z

@psschwei thansk!
Could you please try to add unit test that would verify plugin with slash in arguments won't have that slash part in name maybe? If it's too cumbersome to reach that meaningful coverage, I'd happy to go with it.

@rhuss as a lesson here, maybe it isn't a good idea to have file to path in args. I recall some review feedback when working on kn service import. :)

psschwei · 2021-08-04T21:10:46Z

As I'm writing a test, I have a question about what the assumptions are for plugin arguments.

Take this example:

kn plugin test /with/slash argument

According to the logic in this PR, that would "find" a plugin called kn-plugin-test-argument (i.e. it would simply drop the argument containing a slash). Is that the behavior we'd expect? Or do we expect that kn would drop ALL terms after and including one with a slash (i.e. our plugin would be kn-plugin-test and /with/slash and argument are parameters for the plugin)?

If it's the latter, then I think we'd need to update the logic some to parse the initial parts slice before processing.

dsimansk · 2021-08-04T21:44:57Z

Or do we expect that kn would drop ALL terms after and including one with a slash (i.e. our plugin would be kn-plugin-test and /with/slash and argument are parameters for the plugin)?

Yes, I believe this is already true with current implementation.

I hope we may safely assume that there won't plugin sub-command with slashes in name.

Handling of plugin name and arguments is done by the outer for-loop that's moving through params array and reducing it.
Until the plugin can recognized/found, the plugin object is returned. In addition, original args are preserved for the execution time. Therefore dropping arg with slashes in look-up function shouldn't result in missing arg in execution.

I believe it should go like this.

kn plugin test arg0 /arg1/slash arg2

/arg1/slash => always dropped due to slashes breaking on path lookup
kn plugin test arg0 arg2 => plugin not found
kn plugin test arg0  => plugin not found
kn plugin test  => found 

//the rest of original array [arg0, /arg1/slash/, arg2] are arguments to the plugin cmd "kn plugin test"

psschwei · 2021-08-04T22:14:15Z

Got it -- I think I was confusing the plugin lookup and actually executing the plugin :)

dsimansk · 2021-08-05T07:27:01Z

Thanks for the fix & the test coverage @psschwei!

@rhuss @maximilien can you please take a look?

/lgtm

rhuss · 2021-08-05T08:00:51Z

pkg/kn/plugin/manager.go

@@ -401,6 +401,10 @@ func findMostSpecificPluginInPath(dir string, parts []string, lookupInPath bool)
 		var nameParts []string
 		var commandParts []string
 		for _, p := range parts[0:i] {
+			// skip arguments with slashes
+			if strings.Contains(p, "/") || strings.Contains(p, "\\") {


I don't think that you should just skip it, but completely stop the loop when the first part appears that can not be used as a plugin part, otherwise, you could call plugins accidentally, e.g.

kn myplugin not/possible bla

would also call an existing plugin kn-myplugin-bla, but this is not following the plugin contract.

Also, you should ensure that you only filter characters that are not possible on the current OS filesystem. E.g. on UNIX you can have a backslash as part of a file name, so a plugin like kn-myplugin-ext\ra is possible on UNIX (but not on Windows). So you should filter on the concrete file separator.

Also, you should ensure that you only filter characters that are not possible on the current's OS filesystem. E.g. on UNIX you can have a backslash as part of a file name, so a plugin like kn-myplugin-ext\ra is possible on UNIX (but not on Windows). So you should filter on the concrete file separator.

+1 that's a great point.

maximilien

/ok-to-test

rhuss · 2021-08-06T07:59:58Z

pkg/kn/plugin/manager_test.go

+	fmt.Println(tmpPathDir)
+
+	createTestPluginInDirectory(t, "kn-path-test", tmpPathDir)
+	pluginCommands := []string{"path", "test", "/withslash"}


The test should also use a os.PathSeparator otherwise it will fail on Windows.

Also I would suggest to use a loop and multiple tests, e.g.

path test /withslash path test /withslash extra arg path test ext\ra (using something like `os.PathSeparator == '/' ? "\\" : "/"`)

knative-metrics-robot · 2021-08-06T15:33:20Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/plugin/manager.go	89.8%	89.9%	0.1

rhuss

/approve
/lgtm

thanks !

knative-prow-robot · 2021-08-06T15:42:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: psschwei, rhuss

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~~ [rhuss]

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

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 4, 2021

knative-prow-robot reviewed Aug 4, 2021

View reviewed changes

knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 4, 2021

psschwei mentioned this pull request Aug 4, 2021

Look up plugins on $PATH by default #1412

Merged

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 4, 2021

knative-prow-robot assigned dsimansk Aug 5, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2021

rhuss reviewed Aug 5, 2021

View reviewed changes

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2021

maximilien reviewed Aug 5, 2021

View reviewed changes

knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Aug 5, 2021

rhuss reviewed Aug 6, 2021

View reviewed changes

psschwei added 4 commits August 6, 2021 11:30

fixing bug when looking up plugins on path with slash in argument

9fb4a39

test for dropping slash substrings

350f639

use path separator

46c7bc4

more tests, using pathseparator in tests

5d1e08d

psschwei force-pushed the fix-lookup-path-with-slash branch from bb380a8 to 5d1e08d Compare August 6, 2021 15:30

rhuss approved these changes Aug 6, 2021

View reviewed changes

knative-prow-robot assigned rhuss Aug 6, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 6, 2021

knative-prow-robot merged commit 60c740f into knative:main Aug 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing bug when looking up plugins on path with slash in argument #1415

Fixing bug when looking up plugins on path with slash in argument #1415

psschwei commented Aug 4, 2021

knative-prow-robot left a comment

codecov bot commented Aug 4, 2021 •

edited

Loading

dsimansk commented Aug 4, 2021

psschwei commented Aug 4, 2021

dsimansk commented Aug 4, 2021

psschwei commented Aug 4, 2021

dsimansk commented Aug 5, 2021

rhuss Aug 5, 2021 •

edited

Loading

dsimansk Aug 5, 2021

maximilien left a comment

rhuss Aug 6, 2021

knative-metrics-robot commented Aug 6, 2021

rhuss left a comment

knative-prow-robot commented Aug 6, 2021

Fixing bug when looking up plugins on path with slash in argument #1415

Fixing bug when looking up plugins on path with slash in argument #1415

Conversation

psschwei commented Aug 4, 2021

Description

Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

codecov bot commented Aug 4, 2021 • edited Loading

Codecov Report

dsimansk commented Aug 4, 2021

psschwei commented Aug 4, 2021

dsimansk commented Aug 4, 2021

psschwei commented Aug 4, 2021

dsimansk commented Aug 5, 2021

rhuss Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

dsimansk Aug 5, 2021

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

rhuss Aug 6, 2021

Choose a reason for hiding this comment

knative-metrics-robot commented Aug 6, 2021

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Aug 6, 2021

codecov bot commented Aug 4, 2021 •

edited

Loading

rhuss Aug 5, 2021 •

edited

Loading