Print security notice when index is added #616
Conversation
Only print security notice on install/upgrade when plugin comes from krew-index
k8s-ci-robot
added
area/multi-index
cncf-cla: yes
k8s-ci-robot
added
the
size/S
|
I think I'm good with this in principle. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, chriskim06 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
| @@ -104,7 +105,9 @@ kubectl krew upgrade foo bar"`, | |||
| return errors.Wrapf(err, "failed to upgrade plugin %q", pluginDisplayName) | |||
| } | |||
| fmt.Fprintf(os.Stderr, "Upgraded plugin: %s\n", pluginDisplayName) | |||
| internal.PrintSecurityNotice(plugin.Name) | |||
| if indexName == constants.DefaultIndexName { | |||
There was a problem hiding this comment.
TestKrewInstallDoesntShowSecurityWarningForCustomIndex
There was a problem hiding this comment.
+1
Even if it looks redundant, it is an important documentation of our intended logic.
cmd/krew/cmd/index.go
Outdated
| @@ -75,7 +75,13 @@ var indexAddCmd = &cobra.Command{ | |||
| if !indexoperations.IsValidIndexName(name) { | |||
| return errInvalidIndexName | |||
| } | |||
| return indexoperations.AddIndex(paths, name, args[1]) | |||
| err := indexoperations.AddIndex(paths, name, args[1]) | |||
| if err == nil { | |||
There was a problem hiding this comment.
I think if err != nil { return err} then printing this outside the if-block would be better.
also worth adding tests for (just to check WARNING: exists)
k8s-ci-robot
added
size/M
cmd/krew/cmd/index.go
Outdated
| } | ||
| internal.PrintWarning(os.Stderr, `You have added a new index from %q | ||
| The plugins in this index are not audited for security by the Krew maintainers. | ||
| Run them at your own risk.`+"\n", args[1]) |
There was a problem hiding this comment.
It might be better to say Install them at your own risk. for this particular msg.
cmd/krew/cmd/index.go
Outdated
| } | ||
| internal.PrintWarning(os.Stderr, `You have added a new index from %q | ||
| The plugins in this index are not audited for security by the Krew maintainers. | ||
| Install them at your own risk.`+"\n", args[1]) |
There was a problem hiding this comment.
nit:
| Install them at your own risk.`+"\n", args[1]) | |
| Install them at your own risk. | |
| `, args[1]) |
We are already using multiline string litrals with odd indentation. Then let's not bother about prettifying the trailing newline.
integration_test/index_test.go
Outdated
|
|
||
| test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithDefaultIndex() | ||
| out := string(test.Krew("index", "add", "foo", test.TempDir().Path("index/"+constants.DefaultIndexName)).RunOrFailOutput()) | ||
| if !strings.Contains(out, "WARNING") { |
There was a problem hiding this comment.
Can you also check for a more specific message? Just in case when add prints two warnings, we want to make sure the test asserts on the correct one.
| @@ -104,7 +105,9 @@ kubectl krew upgrade foo bar"`, | |||
| return errors.Wrapf(err, "failed to upgrade plugin %q", pluginDisplayName) | |||
| } | |||
| fmt.Fprintf(os.Stderr, "Upgraded plugin: %s\n", pluginDisplayName) | |||
| internal.PrintSecurityNotice(plugin.Name) | |||
| if indexName == constants.DefaultIndexName { | |||
There was a problem hiding this comment.
+1
Even if it looks redundant, it is an important documentation of our intended logic.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Right now I have the warning when the index is added as:
let me know if you guys think I should change this.
Fixes #615
Related issue: #576
/area multi-index