Repository: GoogleContainerTools/krew
Pull request #72 (2018-10-08 18:10:23)

pkg/index: add manifest apiVersion check
We currently do not check apiVersion in plugin manifests and fail other
validation steps like "bin field is missing". This is confusing to users.

Adding an apiVersion check. Right now it's an exact match, but it should be ok.
When we can figure out how to parse k8s apiVersion format and compare them,
we can refactor this into isMinAPIversion().

This will require updates to many testdata files next time apiVersion is bumped.

ahmetb: /cc @lbb 
codecov-io: # [Codecov](https://codecov.io/gh/GoogleContainerTools/krew/pull/72?src=pr&el=h1) Report
> Merging [#72](https://codecov.io/gh/GoogleContainerTools/krew/pull/72?src=pr&el=desc) into [master](https://codecov.io/gh/GoogleContainerTools/krew/commit/8a8e2b3f8b08a25b4311c82cbbae28193f84192d?src=pr&el=desc) will **increase** coverage by `0.37%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/GoogleContainerTools/krew/pull/72/graphs/tree.svg?width=650&token=J3nFtARI2m&height=150&src=pr)](https://codecov.io/gh/GoogleContainerTools/krew/pull/72?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   46.57%   46.94%   +0.37%     
==========================================
  Files          12       12              
  Lines         569      573       +4     
==========================================
+ Hits          265      269       +4     
  Misses        257      257              
  Partials       47       47
```


| [Impacted Files](https://codecov.io/gh/GoogleContainerTools/krew/pull/72?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/index/validate.go](https://codecov.io/gh/GoogleContainerTools/krew/pull/72/diff?src=pr&el=tree#diff-cGtnL2luZGV4L3ZhbGlkYXRlLmdv) | `94.11% <100%> (+0.78%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/GoogleContainerTools/krew/pull/72?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/GoogleContainerTools/krew/pull/72?src=pr&el=footer). Last update [8a8e2b3...2343618](https://codecov.io/gh/GoogleContainerTools/krew/pull/72?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/cmd/krew/cmd/install.go b/cmd/krew/cmd/install.go
index a2b9c67..daa5f11 100644
--- a/cmd/krew/cmd/install.go
+++ b/cmd/krew/cmd/install.go
@@ -67,7 +67,7 @@ All plugins will be downloaded and made available to: "kubectl plugin <name>"`,
 			for _, name := range pluginNames {
 				plugin, err := indexscanner.LoadPluginFileFromFS(paths.IndexPath(), name)
 				if err != nil {
-					return errors.Wrapf(err, "failed to load plugin %q from index", name)
+					return errors.Wrapf(err, "failed to load plugin %q from the index", name)
 				}
 				install = append(install, plugin)
 			}
diff --git a/pkg/index/validate.go b/pkg/index/validate.go
index 5d673ac..10d0dcd 100644
--- a/pkg/index/validate.go
+++ b/pkg/index/validate.go
@@ -21,10 +21,17 @@ import (
 	"github.com/pkg/errors"
 )
 
+const (
+	currentAPIVersion = "krew.googlecontainertools.github.com/v1alpha2"
+)
+
 var (
 	safePluginRegexp = regexp.MustCompile(`^[\w-]+$`)
+
 	// windowsForbidden is taken from  https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file
-	windowsForbidden = []string{"CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9"}
+	windowsForbidden = []string{"CON", "PRN", "AUX", "NUL", "COM1", "COM2",
+		"COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", "LPT1", "LPT2",
+		"LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9"}
 )
 
 // IsSafePluginName checks if the plugin Name is save to use.
@@ -40,8 +47,16 @@ func IsSafePluginName(name string) bool {
 	return true
 }
 
+func isSupportedAPIVersion(apiVersion string) bool {
+	return apiVersion == currentAPIVersion
+}
+
 // Validate TODO(lbb)
 func (p Plugin) Validate(name string) error {
+	if !isSupportedAPIVersion(p.APIVersion) {
+		return errors.Errorf("plugin manifest has apiVersion=%q, not supported in this version of krew (try updating plugin index or install a newer version of krew)", p.APIVersion)
+	}
+
 	if !IsSafePluginName(name) {
 		return errors.Errorf("the plugin name %q is not allowed, must match %q", name, safePluginRegexp.String())
 	}


Test patch:
diff --git a/pkg/index/indexscanner/testdata/testindex/dontscan.yaml b/pkg/index/indexscanner/testdata/testindex/dontscan.yaml
index ca49915..a963065 100644
--- a/pkg/index/indexscanner/testdata/testindex/dontscan.yaml
+++ b/pkg/index/indexscanner/testdata/testindex/dontscan.yaml
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-apiVersion: plugin.krew.app/v1alpha1
+apiVersion: krew.googlecontainertools.github.com/v1alpha2
 kind: Plugin
 metadata:
   name: dontscan
diff --git a/pkg/index/indexscanner/testdata/testindex/plugins/badplugin.yaml b/pkg/index/indexscanner/testdata/testindex/plugins/badplugin.yaml
index 2285923..eccc518 100644
--- a/pkg/index/indexscanner/testdata/testindex/plugins/badplugin.yaml
+++ b/pkg/index/indexscanner/testdata/testindex/plugins/badplugin.yaml
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-apiVersion: plugin.krew.app/v1alpha1
+apiVersion: krew.googlecontainertools.github.com/v1alpha2
 kind: Plugin
 metadata:
   name: badplugin
diff --git a/pkg/index/indexscanner/testdata/testindex/plugins/badplugin2.yaml b/pkg/index/indexscanner/testdata/testindex/plugins/badplugin2.yaml
index a17029d..2c47451 100644
--- a/pkg/index/indexscanner/testdata/testindex/plugins/badplugin2.yaml
+++ b/pkg/index/indexscanner/testdata/testindex/plugins/badplugin2.yaml
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-apiVersion: plugin.krew.app/v1alpha1
+apiVersion: krew.googlecontainertools.github.com/v1alpha2
 kind: Plugin
 metadata:
   name: badplugin
diff --git a/pkg/index/indexscanner/testdata/testindex/plugins/bar.yaml b/pkg/index/indexscanner/testdata/testindex/plugins/bar.yaml
index 9c67846..87b28ff 100644
--- a/pkg/index/indexscanner/testdata/testindex/plugins/bar.yaml
+++ b/pkg/index/indexscanner/testdata/testindex/plugins/bar.yaml
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-apiVersion: plugin.krew.app/v1alpha1
+apiVersion: krew.googlecontainertools.github.com/v1alpha2
 kind: Plugin
 metadata:
   name: bar
diff --git a/pkg/index/indexscanner/testdata/testindex/plugins/foo.yaml b/pkg/index/indexscanner/testdata/testindex/plugins/foo.yaml
index ec25b88..4f050cb 100644
--- a/pkg/index/indexscanner/testdata/testindex/plugins/foo.yaml
+++ b/pkg/index/indexscanner/testdata/testindex/plugins/foo.yaml
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-apiVersion: plugin.krew.app/v1alpha1
+apiVersion: krew.googlecontainertools.github.com/v1alpha2
 kind: Plugin
 metadata:
   name: foo
diff --git a/pkg/index/indexscanner/testdata/testindex/plugins/notyaml.txt b/pkg/index/indexscanner/testdata/testindex/plugins/notyaml.txt
index 253a6c5..d1bd3f0 100644
--- a/pkg/index/indexscanner/testdata/testindex/plugins/notyaml.txt
+++ b/pkg/index/indexscanner/testdata/testindex/plugins/notyaml.txt
@@ -1,4 +1,4 @@
-apiVersion: plugin.krew.app/v1alpha1
+apiVersion: krew.googlecontainertools.github.com/v1alpha2
 kind: Plugin
 metadata:
   name: notyaml-plugin
diff --git a/pkg/index/indexscanner/testdata/testindex/plugins/wrongname.yaml b/pkg/index/indexscanner/testdata/testindex/plugins/wrongname.yaml
index 68f8718..64dda4f 100644
--- a/pkg/index/indexscanner/testdata/testindex/plugins/wrongname.yaml
+++ b/pkg/index/indexscanner/testdata/testindex/plugins/wrongname.yaml
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-apiVersion: plugin.krew.app/v1alpha1
+apiVersion: krew.googlecontainertools.github.com/v1alpha2
 kind: Plugin
 metadata:
   name: mismatchedname
diff --git a/pkg/index/validate_test.go b/pkg/index/validate_test.go
index d991a55..60f4c8d 100644
--- a/pkg/index/validate_test.go
+++ b/pkg/index/validate_test.go
@@ -67,24 +67,45 @@ func Test_IsSafePluginName(t *testing.T) {
 	}
 }
 
+func Test_isSupportedAPIVersion(t *testing.T) {
+	tests := []struct {
+		name string
+		in   string
+		want bool
+	}{
+		{"wrong group", "networking.k8s.io/v1", false},
+		{"just api group", "krew.googlecontainertools.github.com", false},
+		{"old version", "krew.googlecontainertools.github.com/v1alpha1", false},
+		{"equal version", "krew.googlecontainertools.github.com/v1alpha2", true},
+		{"newer 1", "krew.googlecontainertools.github.com/v1alpha3", false},
+		{"newer 2", "krew.googlecontainertools.github.com/v1", false},
+		{"newer 2", "krew.googlecontainertools.github.com/v2alpha1", false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isSupportedAPIVersion(tt.in); got != tt.want {
+				t.Errorf("isSupportedAPIVersion(%v) = %v, want %v", tt.in, got, tt.want)
+			}
+		})
+	}
+}
+
 func TestPlugin_Validate(t *testing.T) {
 	type fields struct {
 		TypeMeta   metav1.TypeMeta
 		ObjectMeta metav1.ObjectMeta
 		Spec       PluginSpec
 	}
-	type args struct {
-		name string
-	}
 	tests := []struct {
-		name    string
-		fields  fields
-		args    args
-		wantErr bool
+		name       string
+		fields     fields
+		pluginName string
+		wantErr    bool
 	}{
 		{
 			name: "validate success",
 			fields: fields{
+				TypeMeta:   metav1.TypeMeta{APIVersion: currentAPIVersion},
 				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
 				Spec: PluginSpec{
 					Version:          "",
@@ -101,14 +122,36 @@ func TestPlugin_Validate(t *testing.T) {
 					}},
 				},
 			},
-			args: args{
-				name: "foo",
+			pluginName: "foo",
+			wantErr:    false,
+		},
+		{
+			name: "bad api version",
+			fields: fields{
+				TypeMeta:   metav1.TypeMeta{APIVersion: "core/v1"},
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: PluginSpec{
+					Version:          "",
+					ShortDescription: "short",
+					Description:      "",
+					Caveats:          "",
+					Platforms: []Platform{{
+						Head:     "http://example.com",
+						URI:      "",
+						Sha256:   "",
+						Selector: nil,
+						Files:    []FileOperation{{"", ""}},
+						Bin:      "foo",
+					}},
+				},
 			},
-			wantErr: false,
+			pluginName: "foo",
+			wantErr:    true,
 		},
 		{
 			name: "no short description",
 			fields: fields{
+				TypeMeta:   metav1.TypeMeta{APIVersion: currentAPIVersion},
 				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
 				Spec: PluginSpec{
 					Version:          "",
@@ -125,14 +168,13 @@ func TestPlugin_Validate(t *testing.T) {
 					}},
 				},
 			},
-			args: args{
-				name: "foo",
-			},
-			wantErr: true,
+			pluginName: "foo",
+			wantErr:    true,
 		},
 		{
 			name: "no file operations",
 			fields: fields{
+				TypeMeta:   metav1.TypeMeta{APIVersion: currentAPIVersion},
 				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
 				Spec: PluginSpec{
 					Version:          "",
@@ -149,14 +191,13 @@ func TestPlugin_Validate(t *testing.T) {
 					}},
 				},
 			},
-			args: args{
-				name: "foo",
-			},
-			wantErr: true,
+			pluginName: "foo",
+			wantErr:    true,
 		},
 		{
 			name: "wrong plugin name",
 			fields: fields{
+				TypeMeta:   metav1.TypeMeta{APIVersion: currentAPIVersion},
 				ObjectMeta: metav1.ObjectMeta{Name: "wrong-name"},
 				Spec: PluginSpec{
 					Version:          "",
@@ -173,14 +214,13 @@ func TestPlugin_Validate(t *testing.T) {
 					}},
 				},
 			},
-			args: args{
-				name: "foo",
-			},
-			wantErr: true,
+			pluginName: "foo",
+			wantErr:    true,
 		},
 		{
 			name: "unsafe plugin name",
 			fields: fields{
+				TypeMeta:   metav1.TypeMeta{APIVersion: currentAPIVersion},
 				ObjectMeta: metav1.ObjectMeta{Name: "../foo"},
 				Spec: PluginSpec{
 					Version:          "",
@@ -197,10 +237,8 @@ func TestPlugin_Validate(t *testing.T) {
 					}},
 				},
 			},
-			args: args{
-				name: "../foo",
-			},
-			wantErr: true,
+			pluginName: "../foo",
+			wantErr:    true,
 		},
 	}
 	for _, tt := range tests {
@@ -210,8 +248,8 @@ func TestPlugin_Validate(t *testing.T) {
 				ObjectMeta: tt.fields.ObjectMeta,
 				Spec:       tt.fields.Spec,
 			}
-			if err := p.Validate(tt.args.name); (err != nil) != tt.wantErr {
-				t.Errorf("Plugin.Validate() error = %v, wantErr %v", err, tt.wantErr)
+			if err := p.Validate(tt.pluginName); (err != nil) != tt.wantErr {
+				t.Errorf("Plugin.Validate(%s) error = %v, wantErr %v", tt.pluginName, err, tt.wantErr)
 			}
 		})
 	}