Support reading manifest url from files (kubernetes-sigs#429)

* Support reading manifest url from files

Addressing some technical debt:

- Introduce indexscanner.ReadPlugin(io.ReadCloser)
- Make ReadPluginFromFile use that to simplify
- Move validation.ValidatePlugin from several places to ReadPlugin() method
- Add missing "preserves ENOENT" error for ReadPluginFromFile
- Fix verify-gofmt.sh reference (now gone) in run-tests.sh

Adding new stuff:

- introduce --manifest-url that is mut.ex. with --manifest and positional args
- add unit tests for url fetching
- add integration tests for --manifest-url argument behavior

Signed-off-by: Ahmet Alp Balkan <[email protected]>

* fix TestKrewInstall_ManifestArgsAreMutuallyExclusive

Signed-off-by: Ahmet Alp Balkan <[email protected]>

* close() -> shutdown()

Signed-off-by: Ahmet Alp Balkan <[email protected]>

cmd/krew/cmd/install.go

@@ -17,6 +17,7 @@ package cmd
 import (
 	"bufio"
 	"fmt"
+	"net/http"
 	"os"
 
 	"github.com/pkg/errors"
@@ -25,15 +26,14 @@ import (
 
 	"sigs.k8s.io/krew/cmd/krew/cmd/internal"
 	"sigs.k8s.io/krew/internal/index/indexscanner"
-	"sigs.k8s.io/krew/internal/index/validation"
 	"sigs.k8s.io/krew/internal/installation"
 	"sigs.k8s.io/krew/pkg/index"
 )
 
 func init() {
 	var (
-		manifest, archiveFileOverride *string
-		noUpdateIndex                 *bool
+		manifest, manifestURL, archiveFileOverride *string
+		noUpdateIndex                              *bool
 	)
 
 	// installCmd represents the install command
@@ -49,9 +49,9 @@ Examples:
   To install plugins from a newline-delimited file, run:
     kubectl krew install < file.txt
 
-  (For developers) To provide a custom plugin manifest, use the --manifest
-  argument. Similarly, instead of downloading files from a URL, you can specify a
-  local --archive file:
+  (For developers) To provide a custom plugin manifest, use the --manifest or
+  --manifest-url arguments. Similarly, instead of downloading files from a URL,
+  you can specify a local --archive file:
 	kubectl krew install --manifest=FILE [--archive=FILE]
 
 Remarks:
@@ -77,17 +77,21 @@ Remarks:
 				}
 			}
 
-			if len(pluginNames) != 0 && *manifest != "" {
-				return errors.New("must specify either specify either plugin names (via positional arguments or STDIN), or --manifest; not both")
+			if *manifest != "" && *manifestURL != "" {
+				return errors.New("cannot specify --manifest and --manifest-url at the same time")
 			}
 
-			if *archiveFileOverride != "" && *manifest == "" {
-				return errors.New("--archive can be specified only with --manifest")
+			if len(pluginNames) != 0 && (*manifest != "" || *manifestURL != "") {
+				return errors.New("must specify either specify either plugin names (via positional arguments or STDIN), or --manifest/--manifest-url; not both")
+			}
+
+			if *archiveFileOverride != "" && *manifest == "" && *manifestURL == "" {
+				return errors.New("--archive can be specified only with --manifest or --manifest-url")
 			}
 
 			var install []index.Plugin
 			for _, name := range pluginNames {
-				plugin, err := indexscanner.LoadPluginFileFromFS(paths.IndexPluginsPath(), name)
+				plugin, err := indexscanner.LoadPluginByName(paths.IndexPluginsPath(), name)
 				if err != nil {
 					if os.IsNotExist(err) {
 						return errors.Errorf("plugin %q does not exist in the plugin index", name)
@@ -98,12 +102,15 @@ Remarks:
 			}
 
 			if *manifest != "" {
-				plugin, err := indexscanner.ReadPluginFile(*manifest)
+				plugin, err := indexscanner.ReadPluginFromFile(*manifest)
 				if err != nil {
-					return errors.Wrap(err, "failed to load custom manifest file")
+					return errors.Wrap(err, "failed to load plugin manifest from file")
 				}
-				if err := validation.ValidatePlugin(plugin.Name, plugin); err != nil {
-					return errors.Wrap(err, "plugin manifest validation error")
+				install = append(install, plugin)
+			} else if *manifestURL != "" {
+				plugin, err := readPluginFromURL(*manifestURL)
+				if err != nil {
+					return errors.Wrap(err, "failed to read plugin manifest file from url")
 				}
 				install = append(install, plugin)
 			}
@@ -164,9 +171,23 @@ Remarks:
 		},
 	}
 
-	manifest = installCmd.Flags().String("manifest", "", "(Development-only) specify plugin manifest directly.")
+	manifest = installCmd.Flags().String("manifest", "", "(Development-only) specify local plugin manifest file")
+	manifestURL = installCmd.Flags().String("manifest-url", "", "(Development-only) specify plugin manifest file from url")
 	archiveFileOverride = installCmd.Flags().String("archive", "", "(Development-only) force all downloads to use the specified file")
 	noUpdateIndex = installCmd.Flags().Bool("no-update-index", false, "(Experimental) do not update local copy of plugin index before installing")
 
 	rootCmd.AddCommand(installCmd)
 }
+
+func readPluginFromURL(url string) (index.Plugin, error) {
+	klog.V(4).Infof("downloading manifest from url %s", url)
+	resp, err := http.Get(url)
+	if err != nil {
+		return index.Plugin{}, errors.Wrapf(err, "request to url failed (%s)", url)
+	}
+	klog.V(4).Infof("manifest downloaded from url, status=%v headers=%v", resp.Status, resp.Header)
+	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
+		return index.Plugin{}, errors.Errorf("unexpected status code (http %d) from url", resp.StatusCode)
+	}
+	return indexscanner.ReadPlugin(resp.Body)
+}

cmd/krew/cmd/install_test.go

@@ -0,0 +1,62 @@
+// Copyright 2019 The Kubernetes Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package cmd
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"path/filepath"
+	"testing"
+)
+
+func Test_readPluginFromURL(t *testing.T) {
+	server := httptest.NewServer(http.FileServer(http.Dir(filepath.Join("../../../integration_test/testdata"))))
+	defer server.Close()
+
+	tests := []struct {
+		name     string
+		url      string
+		wantName string
+		wantErr  bool
+	}{
+		{
+			name:     "successful request",
+			url:      server.URL + "/konfig.yaml",
+			wantName: "konfig",
+		},
+		{
+			name:    "invalid server",
+			url:     "http://example.invalid:80/foo.yaml",
+			wantErr: true,
+		},
+		{
+			name:    "bad response",
+			url:     server.URL + "/404.yaml",
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := readPluginFromURL(tt.url)
+			if (err != nil) != tt.wantErr {
+				t.Fatalf("readPluginFromURL() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if got.Name != tt.wantName {
+				t.Fatalf("readPluginFromURL() returned name=%v; want=%v", got.Name, tt.wantName)
+			}
+		})
+	}
+}

cmd/krew/cmd/upgrade.go

@@ -62,7 +62,7 @@ kubectl krew upgrade foo bar"`,
 
 			var nErrors int
 			for _, name := range pluginNames {
-				plugin, err := indexscanner.LoadPluginFileFromFS(paths.IndexPluginsPath(), name)
+				plugin, err := indexscanner.LoadPluginByName(paths.IndexPluginsPath(), name)
 				if err != nil {
 					if os.IsNotExist(err) {
 						return errors.Errorf("plugin %q does not exist in the plugin index", name)

cmd/validate-krew-manifest/main.go

@@ -64,7 +64,7 @@ func main() {
 
 func validateManifestFile(path string) error {
 	klog.V(4).Infof("reading file %s", path)
-	p, err := indexscanner.ReadPluginFile(path)
+	p, err := indexscanner.ReadPluginFromFile(path)
 	if err != nil {
 		return errors.Wrap(err, "failed to read plugin file")
 	}

integration_test/install_test.go

@@ -15,6 +15,8 @@
 package integrationtest
 
 import (
+	"net/http"
+	"net/http/httptest"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -105,6 +107,20 @@ func TestKrewInstall_Manifest(t *testing.T) {
 	test.AssertExecutableInPATH("kubectl-" + validPlugin)
 }
 
+func TestKrewInstall_ManifestURL(t *testing.T) {
+	skipShort(t)
+
+	test, cleanup := NewTest(t)
+	defer cleanup()
+	srv, shutdown := localTestServer()
+	defer shutdown()
+
+	test.Krew("install",
+		"--manifest-url", srv+"/"+validPlugin+constants.ManifestExtension).
+		RunOrFail()
+	test.AssertExecutableInPATH("kubectl-" + validPlugin)
+}
+
 func TestKrewInstall_ManifestAndArchive(t *testing.T) {
 	skipShort(t)
 
@@ -132,7 +148,23 @@ func TestKrewInstall_OnlyArchive(t *testing.T) {
 	}
 }
 
-func TestKrewInstall_PositionalArgumentsAndManifest(t *testing.T) {
+func TestKrewInstall_ManifestArgsAreMutuallyExclusive(t *testing.T) {
+	skipShort(t)
+
+	test, cleanup := NewTest(t)
+	defer cleanup()
+	srv, shutdown := localTestServer()
+	defer shutdown()
+
+	if err := test.Krew("install",
+		"--manifest", filepath.Join("testdata", fooPlugin+constants.ManifestExtension),
+		"--manifest-url", srv+"/"+validPlugin+constants.ManifestExtension).
+		Run(); err == nil {
+		t.Fatal("expected mutually exclusive arguments to cause failure")
+	}
+}
+
+func TestKrewInstall_NoManifestArgsWhenPositionalArgsSpecified(t *testing.T) {
 	skipShort(t)
 
 	test, cleanup := NewTest(t)
@@ -143,6 +175,17 @@ func TestKrewInstall_PositionalArgumentsAndManifest(t *testing.T) {
 		"--archive", filepath.Join("testdata", fooPlugin+".tar.gz")).
 		Run()
 	if err == nil {
-		t.Fatal("expected failure")
+		t.Fatal("expected failure when positional args and --manifest specified")
+	}
+
+	err = test.Krew("install", validPlugin,
+		"--manifest-url", filepath.Join("testdata", fooPlugin+constants.ManifestExtension)).Run()
+	if err == nil {
+		t.Fatal("expected failure when positional args and --manifest-url specified")
 	}
 }
+
+func localTestServer() (string, func()) {
+	s := httptest.NewServer(http.FileServer(http.Dir("testdata")))
+	return s.URL, s.Close
+}

internal/index/indexscanner/scanner.go

@@ -60,7 +60,7 @@ func LoadPluginListFromFS(indexDir string) ([]index.Plugin, error) {
 	list := make([]index.Plugin, 0, len(files))
 	for _, file := range files {
 		pluginName := strings.TrimSuffix(file, filepath.Ext(file))
-		p, err := LoadPluginFileFromFS(indexDir, pluginName)
+		p, err := LoadPluginByName(indexDir, pluginName)
 		if err != nil {
 			// Index loading shouldn't fail because of one plugin.
 			// Show error instead.
@@ -72,38 +72,37 @@ func LoadPluginListFromFS(indexDir string) ([]index.Plugin, error) {
 	return list, nil
 }
 
-// LoadPluginFileFromFS loads a plugins index file by its name. When plugin
+// LoadPluginByName loads a plugins index file by its name. When plugin
 // file not found, it returns an error that can be checked with os.IsNotExist.
-func LoadPluginFileFromFS(pluginsDir, pluginName string) (index.Plugin, error) {
+func LoadPluginByName(pluginsDir, pluginName string) (index.Plugin, error) {
 	if !validation.IsSafePluginName(pluginName) {
 		return index.Plugin{}, errors.Errorf("plugin name %q not allowed", pluginName)
 	}
 
 	klog.V(4).Infof("Reading plugin %q", pluginName)
-	pluginsDir, err := filepath.EvalSymlinks(pluginsDir)
-	if err != nil {
-		return index.Plugin{}, err
-	}
-	p, err := ReadPluginFile(filepath.Join(pluginsDir, pluginName+constants.ManifestExtension))
-	if os.IsNotExist(err) {
-		return index.Plugin{}, err
-	} else if err != nil {
-		return index.Plugin{}, errors.Wrap(err, "failed to read the plugin manifest")
-	}
-	return p, validation.ValidatePlugin(pluginName, p)
+	return ReadPluginFromFile(filepath.Join(pluginsDir, pluginName+constants.ManifestExtension))
 }
 
-// ReadPluginFile loads a file from the FS. When plugin file not found, it
+// ReadPluginFromFile loads a file from the FS. When plugin file not found, it
 // returns an error that can be checked with os.IsNotExist.
-func ReadPluginFile(indexFilePath string) (index.Plugin, error) {
-	f, err := os.Open(indexFilePath)
+func ReadPluginFromFile(path string) (index.Plugin, error) {
+	f, err := os.Open(path)
 	if os.IsNotExist(err) {
+		// TODO(ahmetb): we should use go1.13+ errors.Is construct at call sites to evaluate if an error is os.IsNotExist
 		return index.Plugin{}, err
 	} else if err != nil {
 		return index.Plugin{}, errors.Wrap(err, "failed to open index file")
 	}
+	return ReadPlugin(f)
+}
+
+func ReadPlugin(f io.ReadCloser) (index.Plugin, error) {
 	defer f.Close()
-	return DecodePluginFile(f)
+	p, err := DecodePluginFile(f)
+	if err != nil {
+		return p, errors.Wrap(err, "failed to decode plugin manifest")
+	}
+	return p, errors.Wrap(validation.ValidatePlugin(p.Name, p), "plugin manifest validation error")
 }
 
 // DecodePluginFile tries to decodes a plugin manifest from r.

internal/index/indexscanner/scanner_test.go

@@ -23,7 +23,7 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 )
 
-func Test_readIndexFile(t *testing.T) {
+func TestReadPluginFile(t *testing.T) {
 	type args struct {
 		indexFilePath string
 	}
@@ -64,32 +64,42 @@ func Test_readIndexFile(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := ReadPluginFile(tt.args.indexFilePath)
+			got, err := ReadPluginFromFile(tt.args.indexFilePath)
 			if (err != nil) != tt.wantErr {
-				t.Errorf("readIndexFile() error = %v, wantErr %v", err, tt.wantErr)
+				t.Errorf("ReadPluginFromFile() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
 			if err != nil {
 				return
 			}
 			if got.Name != "foo" && got.Kind != "Plugin" {
-				t.Errorf("readIndexFile() has not parsed the metainformations %v", got)
+				t.Errorf("ReadPluginFromFile() has not parsed the metainformations %v", got)
 				return
 			}
 
 			sel, err := metav1.LabelSelectorAsSelector(got.Spec.Platforms[0].Selector)
 			if err != nil {
-				t.Errorf("readIndexFile() error parsing label err: %v", err)
+				t.Errorf("ReadPluginFromFile() error parsing label err: %v", err)
 				return
 			}
 			if !sel.Matches(tt.matchFirst) || sel.Matches(neverMatch) {
-				t.Errorf("readIndexFile() didn't parse label selector properly: %##v", sel)
+				t.Errorf("ReadPluginFromFile() didn't parse label selector properly: %##v", sel)
 				return
 			}
 		})
 	}
 }
 
+func TestReadPluginFile_preservesNotFoundErr(t *testing.T) {
+	_, err := ReadPluginFromFile(filepath.Join(testdataPath(t), "does-not-exist.yaml"))
+	if err == nil {
+		t.Fatal("expected error")
+	}
+	if !os.IsNotExist(err) {
+		t.Fatalf("returned error is not IsNotExist type: %v", err)
+	}
+}
+
 func TestLoadIndexListFromFS(t *testing.T) {
 	type args struct {
 		indexDir string
@@ -161,7 +171,7 @@ func TestLoadIndexFileFromFS(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := LoadPluginFileFromFS(tt.args.indexDir, tt.args.pluginName)
+			got, err := LoadPluginByName(tt.args.indexDir, tt.args.pluginName)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("LoadIndexFileFromFS() got = %##v,error = %v, wantErr %v", got, err, tt.wantErr)
 				return