Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First install of krew 0.4.0 sets it to the detached index #630

Closed
chriskim06 opened this issue Aug 5, 2020 · 10 comments · Fixed by #635
Closed

First install of krew 0.4.0 sets it to the detached index #630

chriskim06 opened this issue Aug 5, 2020 · 10 comments · Fixed by #635

Comments

@chriskim06
Copy link
Member

Now that the multi-index changes are in, when a user follows these instructions and runs:

(
  set -x; cd "$(mktemp -d)" &&
  curl -fsSLO "https://github.com/kubernetes-sigs/krew/releases/latest/download/krew.{tar.gz,yaml}" &&
  tar zxvf krew.tar.gz &&
  KREW=./krew-"$(uname | tr '[:upper:]' '[:lower:]')_amd64" &&
  "$KREW" install --manifest=krew.yaml --archive=krew.tar.gz &&
  "$KREW" update
)

the installation of krew will have the "detached" index in its receipt because it is being installed via manifest. This will prevent them from receiving krew upgrades in the future.

/area multi-index

@chriskim06 chriskim06 changed the title First install of krew sets it to the detached index First install of krew 0.4.0 sets it to the detached index Aug 5, 2020
@chriskim06
Copy link
Member Author

Here are a couple things I could think of:

  • change the installation instructions to not install with a manifest
(
  set -x; cd "$(mktemp -d)" &&
  curl -fsSLO "https://github.com/kubernetes-sigs/krew/releases/latest/download/krew.{tar.gz,yaml}" &&
  tar zxvf krew.tar.gz &&
  KREW=./krew-"$(uname | tr '[:upper:]' '[:lower:]')_amd64" &&
  "$KREW" install krew
)
  • add a new install flag that only works with --manifest/--manifest-url to set the index (defaults to "detached") and modify installation instructions with the new flag
  • add special handling in install.go for if name == constants.KrewPluginName after reading the manifest to set the index

@ahmetb @corneliusweig do you guys have any suggestions?

@ahmetb
Copy link
Member

ahmetb commented Aug 12, 2020

What if we got rid of the notion that there's a detached, and attached these plugins to default? Is this a bad idea?

Our pre-multiindex behavior was also that "upgrade" doesn't succeed if there are such plugins, and developers must remove them to get rid of those errors.

@chriskim06
Copy link
Member Author

Ah ok I thought that upgrade would work for manifest plugins pre-multiindex. How did krew used to detect that a plugin was installed with a manifest before? I thought that was new with the modification to receipts.

If the behavior before was that upgrading manifest plugins failed then I think its fine to remove detached. One of the main reasons I thought we added that was to avoid people testing plugins for a custom index, then trying to upgrade it later on when its in the custom index and having that fail saying that "plugin foo wasn't found in index/default/..."

@ahmetb
Copy link
Member

ahmetb commented Aug 12, 2020

How did krew used to detect that a plugin was installed with a manifest before?

It didn't. :) Try out for yourself if you want.

detached, if worked without hurdles, would be ideal, but it doesn't. I can't find a clean solution to this besides if pluginName == "krew" which is not good.

@chriskim06
Copy link
Member Author

Hmm maybe I'm misunderstanding. I was thinking the workflow is:

  • test installing plugin locally with a manifest/archive and leave it installed
  • plugin author submits plugin to krew-index
  • later on submits a new version
  • then plugin author runs krew upgrade

I tried simulating these steps by picking a random krew-index plugin and installing an older version (I installed score 1.8.0 and its at 1.8.1 in krew-index now):

$ kc krew version
OPTION            VALUE
GitTag            v0.3.4
GitCommit         324f5ed
IndexURI          https://github.com/kubernetes-sigs/krew-index.git
BasePath          /home/kimc/.krew
IndexPath         /home/kimc/.krew/index
InstallPath       /home/kimc/.krew/store
BinPath           /home/kimc/.krew/bin
DetectedPlatform  linux/amd64

$ kc krew list
PLUGIN         VERSION
access-matrix  v0.4.4
ctx            v0.9.1
fleet          v0.1.10
get-all        v1.3.2
krew           v0.3.4
neat           v2.0.1
ns             v0.9.1
tree           v0.4.0
view-secret    v0.6.0
whoami         v0.0.35

$ wget https://raw.githubusercontent.com/kubernetes-sigs/krew-index/1635a3ecabd07fa68df507e6bb131231813aaf1e/plugins/score.yaml
...

$ wget https://github.com/zegl/kube-score/releases/download/v1.8.0/kube-score_1.8.0_linux_amd64.tar.gz
...

$ kc krew install --manifest=score.yaml --archive=kube-score_1.8.0_linux_amd64.tar.gz
Installing plugin: score
Installed plugin: score
\
 | Use this plugin:
 |      kubectl score
 | Documentation:
 |      https://github.com/zegl/kube-score
/
WARNING: You installed plugin "score" from the krew-index plugin repository.
   These plugins are not audited for security by the Krew maintainers.
   Run them at your own risk.

$ kc krew list
PLUGIN         VERSION
access-matrix  v0.4.4
ctx            v0.9.1
fleet          v0.1.10
get-all        v1.3.2
krew           v0.3.4
neat           v2.0.1
ns             v0.9.1
score          v1.8.0
tree           v0.4.0
view-secret    v0.6.0
whoami         v0.0.35

$ kc krew upgrade
Updated the local copy of plugin index.
Upgrading plugin: whoami
Skipping plugin whoami, it is already on the newest version
Upgrading plugin: access-matrix
Skipping plugin access-matrix, it is already on the newest version
Upgrading plugin: ctx
Skipping plugin ctx, it is already on the newest version
Upgrading plugin: fleet
Skipping plugin fleet, it is already on the newest version
Upgrading plugin: krew
Skipping plugin krew, it is already on the newest version
Upgrading plugin: neat
Skipping plugin neat, it is already on the newest version
Upgrading plugin: ns
Skipping plugin ns, it is already on the newest version
Upgrading plugin: get-all
Skipping plugin get-all, it is already on the newest version
Upgrading plugin: score
Upgraded plugin: score
WARNING: You installed plugin "score" from the krew-index plugin repository.
   These plugins are not audited for security by the Krew maintainers.
   Run them at your own risk.
Upgrading plugin: tree
Skipping plugin tree, it is already on the newest version
Upgrading plugin: view-secret
Skipping plugin view-secret, it is already on the newest version

@corneliusweig
Copy link
Contributor

First of all, I find the detached index an important improvement over the current behavior. We should try to preserve that.

Of the suggested options, I think changing the installation instructions gives us the least headache. It works for the old and the new krew versions and does just the right thing. After the last index migration, all index plugins install their receipts, including krew. Thus, migrated KREW_ROOTs will also associate krew with the default index.

The only downside I see is that users will have to download the krew tarball twice. To reduce that pain, we can instead seek to reduce the tarball size. For example, produce separate archives for each platform. However, download size should not matter too much anyway, because it's a one-time thing.


This should work (tested only in bash/zsh):

diff --git site/content/docs/user-guide/setup/install.md site/content/docs/user-guide/setup/install.md
index d51e962..0217dcc 100644
--- site/content/docs/user-guide/setup/install.md
+++ site/content/docs/user-guide/setup/install.md
@@ -22,11 +22,10 @@ Krew self-hosts).
     ```sh
     (
       set -x; cd "$(mktemp -d)" &&
-      curl -fsSLO "https://github.com/kubernetes-sigs/krew/releases/latest/download/krew.{tar.gz,yaml}" &&
+      curl -fsSLO "https://github.com/kubernetes-sigs/krew/releases/latest/download/krew.tar.gz
       tar zxvf krew.tar.gz &&
       KREW=./krew-"$(uname | tr '[:upper:]' '[:lower:]')_amd64" &&
-      "$KREW" install --manifest=krew.yaml --archive=krew.tar.gz &&
-      "$KREW" update
+      "$KREW" install krew
     )
     ```
 
@@ -49,11 +48,10 @@ Krew self-hosts).
     ```fish
     begin
       set -x; set temp_dir (mktemp -d); cd "$temp_dir" &&
-      curl -fsSLO "https://github.com/kubernetes-sigs/krew/releases/latest/download/krew.{tar.gz,yaml}" &&
+      curl -fsSLO "https://github.com/kubernetes-sigs/krew/releases/latest/download/krew.tar.gz &&
       tar zxvf krew.tar.gz &&
       set KREWNAME krew-(uname | tr '[:upper:]' '[:lower:]')_amd64 &&
-      ./$KREWNAME install \
-        --manifest=krew.yaml --archive=krew.tar.gz &&
+      ./$KREWNAME install krew  &&
       set -e KREWNAME; set -e temp_dir
     end
     ```
@@ -72,14 +70,12 @@ Krew self-hosts).
 ## Windows {#windows}
 
 1. Make sure `git` is installed on your system.
-1. Download `krew.exe` and `krew.yaml` from the [Releases][releases] page to
-   a directory.
+1. Download `krew.exe` from the [Releases][releases] page to a directory.
 1. Launch a command-line window (`cmd.exe`) and navigate to that directory.
-1. Run the following command to install krew (pass the correct
-   paths to `krew.yaml` and `krew.zip` below):
+1. Run the following command to install krew:
 
     ```sh
-    krew install --manifest=krew.yaml
+    krew install krew
     ```
 
 1. Add `%USERPROFILE%\.krew\bin` directory to your `PATH` environment variable

@ahmetb
Copy link
Member

ahmetb commented Aug 17, 2020

Yeah, I believe the only reason we used krew install --manifest=krew.yaml --archive=krew.tar.gz ever since the initial version was to prevent re-downloading.

My initial feeling is: let's apply that patch as it solves this problem

However, I feel like we're making a potentially offline installation process not-so-offline anymore. For example, you could previously download+install krew itself from a tarball, but now you have to have the default index to install krew.

Similarly, what if an operator installs krew with this approach (default index), deletes the default index (or replaces it with another git-remote) in their corporate network? We tell users in the docs that they can delete the default index.

@chriskim06
Copy link
Member Author

Similarly, what if an operator installs krew with this approach (default index), deletes the default index (or replaces it with another git-remote) in their corporate network? We tell users in the docs that they can delete the default index.

This should still be ok though right?

Also, the docs should probably mention something about krew not being able to upgrade itself when the default index is changed though.

@corneliusweig
Copy link
Contributor

corneliusweig commented Aug 22, 2020

Similarly, what if an operator installs krew with this approach (default index), deletes the default index (or replaces it with another git-remote) in their corporate network? We tell users in the docs that they can delete the default index.

I think warning about a detached krew that will not receive updates in the docs should be enough. Then folks who want to pull up their own index are aware of this pitfall and can prepare for it.

However, I feel like we're making a potentially offline installation process not-so-offline anymore. For example, you could previously download+install krew itself from a tarball, but now you have to have the default index to install krew.

Well it's still possible to install krew via the tarball (and again, we should document this in our custom index docs). Only this installation will not receive any upgrades. However, what is indeed troublesome is that operators have no way to install a krew version from their index. I think what we should do is to offer overriding the DefaultIndexURI via an env variable. Then operators can start using their own index (containing some krew plugin) from scratch. This will then also set up the default index and point to this custom index location.


Once we have that and update the docs, we can start another attempt to release v0.4.0, right?

@ahmetb
Copy link
Member

ahmetb commented Aug 23, 2020

I don't think we need to update the docs for this particular caveat of "detached" krew. We already changed Install docs to say "krew install krew", so I think we can go tag a release again right away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants