Adds --manifest-url flag to allow plugin installation from remote manifests #350

rewanthtammana · 2019-10-03T14:03:53Z

This feature addition lets the user/developer to install a krew plugin directly from the source URL instead of manually downloading the plugin file.

Problem statement

Install kubectl plugin from config file

Current approach

wget https://raw.githubusercontent.com/rewanth1997/kubectl-fields/master/deploy/krew/plugin.yaml
kubectl krew install --manifest=./plugin.yaml

Proposed approach

kubectl krew install --manifest-url="https://github.com/rewanth1997/kubectl-fields/raw/master/deploy/krew/plugin.yaml"

This new feature addition will download the plugin from the URL to a temporary location and then installs the plugin from that temporary fie using the regular manifest method.

Please review the feature request. I'm happy to make the necessary changes if any.

Regards,
Rewanth

rewanthtammana · 2019-10-03T18:20:46Z

/assign @soltysh

ahmetb · 2019-10-03T20:36:21Z

This proposal was previously discussed & denied, I believe.

rewanthtammana · 2019-10-04T03:58:38Z

Oh, I couldn't find any such discussion on Github.

This PR downloads the yaml file from the URL and stores it in a temporary location. After download, it transfers the control to --manifest flag. After installation, this PR cleans up the file from the system.

I couldn't think of any potential drawbacks. In fact, I see this as an advantage for developers/users. May I know the reason behind denying this idea?

ahmetb · 2019-10-04T10:16:31Z

I also tried to find but couldn't. Now I'm thinking if it was a private discussion with @corneliusweig.

The main reasoning was a about a few points:

You can just do --manifest <(curl url) and compose tools/pipes/sub-shells as one should do with UNIX command-line tools.
--manifest-url is yet another thing to maintain, and makes the user/test surface larger. We thought about detecting url in --manifest but that is also "magic", which is not a good thing to do in tools.

Since this has came up multiple times by now, we might be open to it.

/unassign @soltysh
/assign
/assign @corneliusweig

corneliusweig · 2019-10-07T20:05:17Z

Hi @rewanth1997, the issue @ahmetb mentioned is #193.

I think the main reason against the --manifest-url option is that this is only intended for plugin devs and not for krew users. Adding this --manifest-url option would also encourage end-users to install dangling plugins via this option. However, we don't want this, because krew should also take care of updates and this doesn't work when installing from URL.

However, even plugin devs do stumble over this --manifest <(curl URL) thing. How about this: if a user tries to install with --manifest URL, we could detect if the argument is in fact a URL and print an error suggesting to try --manifest <(curl URL) instead?

rewanthtammana · 2019-10-09T09:25:45Z

I wasn't aware of the update issue. Thanks for mentioning that.

I just checked the update module implementation in krew, https://github.com/kubernetes-sigs/krew/blob/master/pkg/gitutil/git.go#L59. With this implementation, the plugins installed via kubectl install --manifest <(curl ...) won't be updated with krew update command.

The idea of throwing a custom error to use curl seems good but building a generic regex expression for URL is very tough. Limiting this just to http/https protocol check isn't a good idea either.

corneliusweig · 2019-10-09T20:46:07Z

@rewanth1997 Implementing the URL parser is not necessary. Go already has that built in. See https://github.com/kubernetes-sigs/krew/pull/302/files

if _, err := url.ParseRequestURI(word); err == nil {
   return true
}

rewanthtammana · 2019-10-13T18:54:05Z

@corneliusweig I don't think this will work for our use case.

url.ParseRequestURI considers anything starting with "/" as URI. It might be an absolute path to the plugin file in the host machine.

"/home/rewanth/kubectl-fields/deploy/krew/plugin.yaml" gets detected as URI but we are looking for URL.

package main

import (
    "fmt"
    "net/url"
)

func main() {
    fmt.Println(isUrl("https://github.com/kubernetes-sigs/krew/pull/350"))
    fmt.Println(isUrl("/home/rewanth/kubectl-fields/deploy/krew/plugin.yaml"))
    fmt.Println(isUrl("plugin.yaml"))
}

func isUrl(toTest string) bool {
    _, err := url.ParseRequestURI(toTest)
    if err != nil {
        return false
    } else {
        return true
    }
}

Output

true
true
false

k8s-ci-robot · 2019-10-14T06:04:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rewanth1997
To complete the pull request process, please assign ahmetb
You can assign the PR to them by writing /assign @ahmetb in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

rewanthtammana · 2019-10-14T06:12:02Z

rewanth@ubuntu:/tmp/krew/out/bin$ ./krew-linux_amd64 install --manifest=https://raw.githubusercontent.com/rewanth1997/kubectl-fields/master/deploy/krew/plugin.yaml
F1013 22:59:14.313896  107113 root.go:58] Cannot install plugin directly from URL
To install from URL, use --manifest <(curl https://raw.githubusercontent.com/rewanth1997/kubectl-fields/master/deploy/krew/plugin.yaml)

@corneliusweig I added an additional check to check for Schema and Host to validate the URL. Please review the PR.

corneliusweig · 2019-10-14T06:40:24Z

Good find! I'll consider adding the same logic in the other PR.

Can you also add some tests for this behavior? Otherwise this looks good. Thanks!

rewanthtammana · 2019-10-14T06:58:00Z

I validated the module independently with different protocols like SMB, FTP, HTTP, etc. I believe these checks are good enough for integration. Please let me know if something else needs to be done.

package main

import (
    "fmt"
    "net/url"
)

func main() {
    fmt.Println(isUrl("http://github.com/kubernetes-sigs/krew/pull/350"))
    fmt.Println(isUrl("https://github.com/kubernetes-sigs/krew/pull/350"))
    fmt.Println(isUrl("ftp://mydomain/krew/plugin.yaml"))
    fmt.Println(isUrl("smb://mywindowsdomain/krew/plugin.yaml"))

    fmt.Println(isUrl("/home/rewanth/kubectl-fields/deploy/krew/plugin.yaml"))
    fmt.Println(isUrl("http//plugin.yaml"))
    fmt.Println(isUrl("plugin.yaml"))
}

func isUrl(toTest string) bool {
    u, err := url.ParseRequestURI(toTest)
    if err == nil && u.Scheme != "" && u.Host != "" {
        return true
    } else {
        return false
    }
}

Output

true
true
true
true
false
false
false

Goplayground link to run tests: https://play.golang.org/p/xUN0SHp1c9t

ahmetb · 2019-10-14T17:49:56Z

@corneliusweig have we settled on --manifest vs --manifest-url?
It seems like latter is more verbose (as opposed to magic) and can be easily implemented, albeit with extra testing requirements.

cmd/krew/cmd/install.go

cmd/krew/cmd/internal/downloadFile.go

cmd/krew/cmd/install.go

Uses random port instead of hard-coded port. Closes http response object

rewanthtammana · 2019-12-08T20:14:01Z

@ahmetb @corneliusweig

All the tests are successful on my local system. Just to confirm, I forked my current working repository (https://github.com/rewanth1997/krew/tree/manifest-url) into my test account (https://github.com/testinguser883/krew/tree/manifest-url) and the build is successful there also (https://travis-ci.org/testinguser883/krew/builds/622366453). I'm not sure why travis is failing here.

I made all the suggested changes expect --archive and --manifest-url conflict.

I added the abstraction layer, removed hard-coded port number, closed response object after use and fixed a few linter bugs as well. Please review the latest code.

corneliusweig · 2019-12-08T20:51:42Z

@rewanth1997 I think you need to rebase your current branch on top of origin/master. Here is what you need to do:

git remote add krew-origin [email protected]:kubernetes-sigs/krew.git
git fetch krew-origin
git rebase krew-origin/master

I guess you will have to resolve a few merge conflicts.

corneliusweig

Ok, this looks much better now. Left a few more comments, but that should be easy to do.

cmd/krew/cmd/internal/read_plugin.go

cmd/krew/cmd/install.go

rewanthtammana · 2019-12-08T22:08:36Z

@corneliusweig All recommended changes updated.

rewanthtammana · 2019-12-08T22:46:43Z

@corneliusweig Even the mutually exclusive condition is fixed in the latest commit. All tests are passed locally. Please let me know if any further changes are required.

ahmetb · 2019-12-09T02:07:04Z

The tests are failing with a very unusual error.
I'll make another review or push commits directly to your branch, thanks for staying on top of this @rewanth1997.

ahmetb · 2019-12-09T02:13:06Z

integration_test/install_test.go

+	t.Helper()
+
+	server := httptest.NewUnstartedServer(http.FileServer(http.Dir("testdata")))
+
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("trouble starting local server")
+	}
+	server.Listener = listener
+	server.Start()
+	return server, server.URL


You can probably just do this:

httptest.NewServer(..) return server.URL, server.Close

it does everything you want, and then you can embed it to the test because it's 2 lines.

ahmetb · 2019-12-09T02:14:06Z

integration_test/install_test.go

+func TestKrewInstall_EmptyManifestURL(t *testing.T) {
+	skipShort(t)
+
+	server, baseURL := startLocalServer(t)
+	defer server.Close()
+
+	test, cleanup := NewTest(t)
+	defer cleanup()
+
+	err := test.Krew("install",
+		"--manifest-url", baseURL+emtpyManifestURI).Run()
+	if err == nil {
+		t.Errorf("expected install to fail but was successful")
+	}
+}


what does this really test?
our validation method? aren't those already covered with unit tests?

ahmetb · 2019-12-09T02:14:16Z

integration_test/install_test.go

+func TestKrewInstall_InvalidManifestURL(t *testing.T) {
+	skipShort(t)
+
+	server, baseURL := startLocalServer(t)
+	defer server.Close()
+
+	test, cleanup := NewTest(t)
+	defer cleanup()
+
+	err := test.Krew("install",
+		"--manifest-url", baseURL+invalidManifestURI).Run()
+	if err == nil {
+		t.Errorf("expected install to fail but was successful")
+	}
+}


what does this really test?
our validation method? aren't those already covered with unit tests?

ahmetb · 2019-12-09T02:16:56Z

cmd/krew/cmd/internal/read_plugin.go

+// GetPlugin is an abstraction layer for manifest and manifest-url handlers and returns plugin object
+func GetPlugin(manifest string, manifestURL string) (index.Plugin, error) {
+	if manifest != "" {
+		return indexscanner.ReadPluginFile(manifest)
+	}
+
+	return readRemotePluginManifest(manifestURL)
+}


This isn't really a good way to do things.
We could do this at the call site, now you just embedded the decisionmaking here, and it doesn't have unit tests.

ahmetb · 2019-12-09T02:17:42Z

cmd/krew/cmd/internal/read_plugin.go

+func readRemotePluginManifest(url string) (index.Plugin, error) {
+	resp, err := http.Get(url)
+	if err != nil {
+		return index.Plugin{}, errors.Wrapf(err, "Error downloading manifest from %q", url)
+	}
+	defer resp.Body.Close()
+
+	plugin, err := indexscanner.DecodePluginFile(resp.Body)
+	if err != nil {
+		return index.Plugin{}, errors.Wrapf(err, "Error decoding manifest from %q", url)
+	}
+
+	return plugin, nil
+}


this doesn't have unit tests :)
would be good to unit-test this, rather than trying to test /empty.yaml /invalid.yaml etc.

ahmetb · 2019-12-09T02:21:08Z

cmd/krew/cmd/internal/read_plugin.go

@@ -0,0 +1,49 @@
+// Copyright 2019 The Kubernetes Authors.


this cmd/ package does nearly nothing noteworthy that requires creating a new package for. especially because cmd/ directory directly has "tool" names (krew, generate-plugin-list, ...) this doesn't fit here.

ahmetb · 2019-12-09T02:22:55Z

I think this PR has showed us fundamentally how we should approach accepting patches to krew codebase.

To me, minimalism is key, and ideally we should not require ~200 lines of new code to add one flag like this.

I'll be preparing a PR that does the same thing. I think some issues in the PR stems from the fact that our indexscanner API isn't the best it could be. Let's see how much new code it'll end up in.

ahmetb · 2019-12-09T02:26:45Z

cmd/krew/cmd/install.go

@@ -81,8 +85,8 @@ Remarks:
 				return errors.New("must specify either specify either plugin names (via positional arguments or STDIN), or --manifest; not both")


you missed this part. this ought to also factor in --manifest-url.

ahmetb · 2019-12-09T03:19:19Z

/hold
In about an hour I prepared a patch that does this: #429
It only results in ~120 lines of net added code while addressing the underlying technical debt and adding some missing tests.

rewanthtammana · 2019-12-09T05:24:36Z

The tests are failing with a very unusual error.

Yes, even I'm not sure why? This branch is building successfully without any errors
https://travis-ci.org/testinguser883/krew/builds/622405722?utm_source=github_status&utm_medium=notification

It would be great if you can point out why the build is failing here (maybe it's because this branch is not synced with the current master repo?)

ahmetb · 2019-12-09T06:36:49Z

Yeah, most likely you’ll need to rebase.
On the other hand, we can continue on my PR as we’re getting to ~100 comments on this and we still have some way to go. I think my patch addresses this more cleanly, but thank you for doing the work on this.

rewanthtammana · 2019-12-09T07:16:29Z

Yeah, sure. We will work on your PR. I will try to give inputs to the best of my knowledge.

@corneliusweig @ahmetb Thanks for considering my idea, being so patient and reviewing my PR.

corneliusweig · 2019-12-09T22:50:56Z

Hey @rewanth1997, sorry that your PR doesn't make it into krew's codebase. Don't be upset though. The final draft was already much more mature than the initial one, so you probably could learn a lot by doing it. For your future work, you should focus on testing your code. You can learn a lot from looking at @ahmetb's version :)

/close

k8s-ci-robot · 2019-12-09T22:50:58Z

@corneliusweig: Closed this PR.

In response to this:

Hey @rewanth1997, sorry that your PR doesn't make it into krew's codebase. Don't be upset though. The final draft was already much more mature than the initial one, so you probably could learn a lot by doing it. For your future work, you should focus on testing your code. You can learn a lot from looking at @ahmetb's version :)

/close

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.

This comment has been minimized.

Sign in to view

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 3, 2019

k8s-ci-robot requested review from ahmetb and soltysh October 3, 2019 14:04

rewanthtammana force-pushed the manifest-url branch 2 times, most recently from 51dadcc to 3da3c54 Compare October 3, 2019 18:13

This comment has been minimized.

Sign in to view

k8s-ci-robot assigned soltysh Oct 3, 2019

rewanthtammana force-pushed the manifest-url branch from 86ca432 to c34fd2e Compare October 3, 2019 18:46

Adds --manifest-url option to install plugin from URL

f708231

rewanthtammana force-pushed the manifest-url branch from c34fd2e to f708231 Compare October 3, 2019 18:55

k8s-ci-robot assigned ahmetb and corneliusweig and unassigned soltysh Oct 4, 2019

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 14, 2019

corneliusweig suggested changes Dec 7, 2019

View reviewed changes

rewanthtammana added 2 commits December 8, 2019 11:34

Adds abstraction layer for reading plugin.

00becab

Uses random port instead of hard-coded port. Closes http response object

Fixes linter bugs

0609ed9

corneliusweig suggested changes Dec 8, 2019

View reviewed changes

rewanthtammana force-pushed the manifest-url branch from eec4072 to 0609ed9 Compare December 8, 2019 21:21

rewanthtammana added 2 commits December 8, 2019 13:34

Adds code to wrap errors

fb99bad

Adds additional unit test cases

3c15a96

Removes mutual exclusive condition b/w archive and manifest-url

c399cd4

ahmetb reviewed Dec 9, 2019

View reviewed changes

ahmetb mentioned this pull request Dec 9, 2019

Support reading manifest url from files #429

Merged

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2019

ahmetb mentioned this pull request Dec 9, 2019

Check for failures when plugins are removed #421

Closed

k8s-ci-robot closed this Dec 9, 2019

		@@ -81,8 +85,8 @@ Remarks:
		return errors.New("must specify either specify either plugin names (via positional arguments or STDIN), or --manifest; not both")

Adds --manifest-url flag to allow plugin installation from remote manifests #350

Adds --manifest-url flag to allow plugin installation from remote manifests #350

Conversation

rewanthtammana commented Oct 3, 2019 • edited Loading

Problem statement

Current approach

Proposed approach

This comment has been minimized.

This comment has been minimized.

rewanthtammana commented Oct 3, 2019

ahmetb commented Oct 3, 2019

rewanthtammana commented Oct 4, 2019 • edited Loading

ahmetb commented Oct 4, 2019

corneliusweig commented Oct 7, 2019

rewanthtammana commented Oct 9, 2019 • edited Loading

corneliusweig commented Oct 9, 2019

rewanthtammana commented Oct 13, 2019 • edited Loading

k8s-ci-robot commented Oct 14, 2019

rewanthtammana commented Oct 14, 2019

corneliusweig commented Oct 14, 2019

rewanthtammana commented Oct 14, 2019 • edited Loading

ahmetb commented Oct 14, 2019

rewanthtammana commented Dec 8, 2019

corneliusweig commented Dec 8, 2019

corneliusweig left a comment

Choose a reason for hiding this comment

rewanthtammana commented Dec 8, 2019

rewanthtammana commented Dec 8, 2019

ahmetb commented Dec 9, 2019

ahmetb Dec 9, 2019

Choose a reason for hiding this comment

ahmetb Dec 9, 2019

Choose a reason for hiding this comment

ahmetb Dec 9, 2019

Choose a reason for hiding this comment

ahmetb Dec 9, 2019

Choose a reason for hiding this comment

ahmetb Dec 9, 2019

Choose a reason for hiding this comment

ahmetb Dec 9, 2019

Choose a reason for hiding this comment

ahmetb commented Dec 9, 2019

ahmetb Dec 9, 2019

Choose a reason for hiding this comment

ahmetb commented Dec 9, 2019

rewanthtammana commented Dec 9, 2019

ahmetb commented Dec 9, 2019

rewanthtammana commented Dec 9, 2019

corneliusweig commented Dec 9, 2019

k8s-ci-robot commented Dec 9, 2019

rewanthtammana commented Oct 3, 2019 •

edited

Loading

rewanthtammana commented Oct 4, 2019 •

edited

Loading

rewanthtammana commented Oct 9, 2019 •

edited

Loading

rewanthtammana commented Oct 13, 2019 •

edited

Loading

rewanthtammana commented Oct 14, 2019 •

edited

Loading