Check if Plugin is bundled before installing #12575

RobbieMcKinstry · 2023-03-31T18:40:24Z

Description

This PR adds a check to pulumi plugin install to confirm that the plugin is not bundled with Pulumi before performing the download. Without this check, the CLI will produce a 404 error because the language plugins aren't distributable independently of the CLI executable.

Fixes #11703

Checklist

~~I have added tests that prove my fix is effective or that my feature works~~ I have manually checked the relevant CLI commands.

I have run make changelog and committed the changelog/pending/<file> documenting my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

pulumi-bot · 2023-03-31T18:41:16Z

Changelog

[uncommitted] (2023-08-07)

Bug Fixes

[cli/plugin] Improve error message during pulumi plugin install if the plugin is bundled with Pulumi
#12575

dixler · 2023-03-31T18:47:33Z

pkg/cmd/pulumi/plugin_install.go

+				// bundled plugins are not installable with this command.
+				// They are expected to be distributed with Pulumi itself.
+				if workspace.IsBundledPlugin(pluginSpec.Kind, pluginSpec.Name) {
+					return fmt.Errorf(
+						"the %v %v plugin is bundled with Pulumi, and cannot be directly installed"+
+							" with this command. If you need to reinstall this plugin, uninstall"+
+							" Pulumi via your package manager and re-install.",
+						pluginSpec.Name,
+						pluginSpec.Kind,
+					)
+				}
+


Can you add a small test on this to ensure that this check is run?

You'll want to follow @abhinav's style of splitting command code out of the cobra callback for that.

How would you want the control flow for the test?
Because this is in the top-level of the cobra cmd, we'd have to mock out the function, add a faker interface, and check the fake. Currently, this function has no tests at all.

@Frassle link?

Frassle · 2023-03-31T19:09:47Z

pkg/cmd/pulumi/plugin_install.go

@@ -103,6 +103,18 @@ func newPluginInstallCmd() *cobra.Command {
 					Checksums:         checksums,
 				}

+				// bundled plugins are not installable with this command.


Annoyingly I'm not sure we should use "bundled" to decided this.
java, dotnet, and yaml are all bundled (that is we expect to see them side-by-side with the CLI binary) but they are installable via pulumi plugin install and we probably are going to make use of that testing the install these things as we move to making them true plugins.
But maybe for decent UX just disable this check if PULUMI_DEV is set, I don't think any users will be depending on those installs yet but DEV will mean we can still quickly hook into it for testing.

Frassle · 2023-03-31T19:10:33Z

pkg/cmd/pulumi/plugin_install.go

+				// bundled plugins are not installable with this command.
+				// They are expected to be distributed with Pulumi itself.
+				if workspace.IsBundledPlugin(pluginSpec.Kind, pluginSpec.Name) {
+					return fmt.Errorf(
+						"the %v %v plugin is bundled with Pulumi, and cannot be directly installed"+
+							" with this command. If you need to reinstall this plugin, uninstall"+
+							" Pulumi via your package manager and re-install.",
+						pluginSpec.Name,
+						pluginSpec.Kind,
+					)
+				}
+


You'll want to follow @abhinav's style of splitting command code out of the cobra callback for that.

Frassle · 2023-03-31T19:10:54Z

sdk/go/common/workspace/plugins.go

@@ -1513,6 +1513,20 @@ func IsPluginKind(k string) bool {
 	}
 }

+// IsPreinstalledLanguagePlugin returns true if the spec describes


Suggested change

// IsPreinstalledLanguagePlugin returns true if the spec describes

// IsBundledPlugin returns true if the spec describes

Frassle · 2023-03-31T19:11:09Z

sdk/go/common/workspace/plugins.go

@@ -1513,6 +1513,20 @@ func IsPluginKind(k string) bool {
 	}
 }

+// IsPreinstalledLanguagePlugin returns true if the spec describes
+// a language plugin that comes preinstalled with the CLI.


Suggested change

// a language plugin that comes preinstalled with the CLI.

// a plugin that comes preinstalled with the CLI.

Not just language plugins

The whole comment is wrong lol

Frassle · 2023-03-31T19:11:36Z

sdk/go/common/workspace/plugins.go

+	// We currently bundle some plugins with "pulumi" and thus expect them to be next to the pulumi binary. We
+	// also always allow these plugins to be picked up from PATH even if PULUMI_IGNORE_AMBIENT_PLUGINS is set.
+	// Eventually we want to fix this so new plugins are true plugins in the plugin cache.


This comment is more appropriate in "getPluginInfoAndPath" rather than here.

Frassle · 2023-07-28T10:12:52Z

pkg/cmd/pulumi/plugin_install.go

-						runtime.GOOS + "-" + runtime.GOARCH: checksumBytes,
-					}
-				}
+		// Bundled plugins are generally not installable with this command. They are expected to be


This is the only real change here, the rest is just indent change from putting this in the Run method.

Frassle · 2023-07-28T10:15:42Z

pkg/cmd/pulumi/plugin_install_test.go

+		),
+	}
+
+	// TODO: This test actually hits GitHub to fail, it would be better if plugin installation could be mocked.


cc @abhinav your generally good at thinking up clean designs for this sort of thing, any recommendation here.
I've been using functions for this in some places (e.g. in pkg/codegen/convert/plugin_mapper.go), maybe not overkill to do that here?

I think a function reference to workspace.DownloadToFile (or whatever is responsible for downloading) is fine here.
Longer term, it makes sense for these top-level functions with so much complex behavior to be turned into objects with clearer contracts that we inject in here, e.g. a *PluginDownloader struct and an interface for that in the command implementation. Then we can mock it out and return an error on a download attempt, or start up a test HTTP server and make the downloader implementation hit that.

Edit: looks like PluginSpec.GetLatestVersion is the one that makes the request here. Again, short term we can do a reference. ((PluginSpec).GetLatestVersion is an unbound method reference.)
Long-term, we should separate data-only types and behavior types. PluginSpec appears to be intended as a data type. We should look towards dropping complex behavior from its methods, moving it into a Plugin[Something] abstraction responsible for plugin-related work that has side effects.

Frassle · 2023-07-28T13:24:08Z

pkg/cmd/pulumi/plugin_install.go

+		// distributed with Pulumi itself. But we turn this check off if PULUMI_DEV is set so we can
+		// test installing plugins that are being moved to their own distribution (such as when we move
+		// pulumi-nodejs).
+		isBundled := (pluginSpec.Kind == workspace.LanguagePlugin && pluginSpec.Name == "nodejs") ||


I'm very undecided about what's the right logic here.

Should it be we give this warning for all language plugins + the python/node policy/resource, on the basis that we don't really support language as plugins yet, they are tightly coupled to the same CLI version (we do make grpc breaking changes to the language interface).

Should it error for all our plugins, but if someone tries to install a community rust (for example) language plugin we just allow that (even if DEV=false)

Should it error for just the plugins which we know can't install via plugin install, i.e. nodejs, go, and python language plugins + the nodejs/python policy/resource. So if someone wants to install dotnet like this they can, even though it's not really supported.

I'm currently leaning towards 1 or 2.

(2) sounds more reasonable to me.
We should only consider what we know is bundled with pulumi as bundled for the purposes of this command. This avoids that assumption about what's supported by the wider tooling.
So if/when we do support other language plugins, nobody should have to come back here and fix this.

OK I've reworked this to explicitly list our plugins and only our plugins as "bundled". Any other language plugin will not look next to the pulumi binary, and will be allowed to install via "plugin install"

abhinav · 2023-07-28T16:34:45Z

@Frassle I pushed a commit with the test modified to not make that request. Feel free to squash it into the behavioral change, although I would recommend keeping the refactor commit separate.

abhinav · 2023-08-01T17:09:17Z

sdk/go/common/workspace/plugins.go

+	return (kind == LanguagePlugin && name == "nodejs") ||
+		(kind == LanguagePlugin && name == "go") ||
+		(kind == LanguagePlugin && name == "python") ||
+		(kind == LanguagePlugin && name == "dotnet") ||
+		(kind == LanguagePlugin && name == "yaml") ||
+		(kind == LanguagePlugin && name == "java") ||
+		(kind == ResourcePlugin && name == "pulumi-nodejs") ||
+		(kind == ResourcePlugin && name == "pulumi-python") ||
+		(kind == AnalyzerPlugin && name == "policy") ||
+		(kind == AnalyzerPlugin && name == "policy-python")


It's too bad we don't have pattern matching.

This is completely fine as-is, but if you'd prefer for this to not be a series of &&/||s, you can use a map/set with a composite key.

type bundledPlugin struct{ kind PluginKind; name string }

You can either declare a map, e.g.,

var bundledPlugins = map[bundledPlugin]struct{}{ {LanguagePlugin, "nodejs"}: {}, {LanguagePlugin, "go"}: {}, // .. } func IsPluginBundled(kind PluginKind, name string) bool { _, ok := bundledPlugins[bundledPlugin{kind, name}] return ok }

Or if you'd prefer to not add the : {}, you can declare a slice and compile the map at init.

var bundledPlugins = []bundledPlugin{ {LanguagePlugin, "nodejs"}, {LanguagePlugin, "go"}, // ... } // TODO: Do we have a shared set type we can use here // and do something like `set.FromSlice[bundledPlugin](bundledPlugins)`? var bundledPluginSet = make(map[bundledPlugin]struct{}) func init() { for _, p := range bundledPlugins { bundledPluginSet[p] = struct{}{} } }

It's simple to read, it's only in one place, we'll shrink this over time hopefully, probably fine as is.

Extracts the implementation of the `pulumi plugin install` command into a separate pluginInstallCmd. This mirrors the layout we use for some of the other commands, and will allow injecting global dependencies from tests. This change contains no behavioral changes.

This commit adds a check to `pulumi plugin install` to confirm that the plugin is not bundled with Pulumi before performing the download. Without this check, the CLI will produce a 404 error because the language plugins aren't distributable independently of the CLI executable. If PULUMI_DEV is set we'll skip the error and try to download anyway.

Frassle · 2023-08-07T14:47:24Z

bors merge

12575: Check if Plugin is bundled before installing r=Frassle a=RobbieMcKinstry  # Description This PR adds a check to `pulumi plugin install` to confirm that the plugin is not bundled with Pulumi before performing the download. Without this check, the CLI will produce a 404 error because the language plugins aren't distributable independently of the CLI executable.  Fixes #11703 ## Checklist  - [x] ~I have added tests that prove my fix is effective or that my feature works~ I have manually checked the relevant CLI commands.  - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change  - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version  Co-authored-by: Abhinav Gupta <abhinav@pulumi.com> Co-authored-by: Robbie McKinstry <robbie@pulumi.com>

bors · 2023-08-07T16:20:16Z

Build failed:

bors-ok

Frassle · 2023-08-08T09:38:43Z

bors retry

12575: Check if Plugin is bundled before installing r=Frassle a=RobbieMcKinstry  # Description This PR adds a check to `pulumi plugin install` to confirm that the plugin is not bundled with Pulumi before performing the download. Without this check, the CLI will produce a 404 error because the language plugins aren't distributable independently of the CLI executable.  Fixes #11703 ## Checklist  - [x] ~I have added tests that prove my fix is effective or that my feature works~ I have manually checked the relevant CLI commands.  - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change  - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version  Co-authored-by: Abhinav Gupta <abhinav@pulumi.com> Co-authored-by: Robbie McKinstry <robbie@pulumi.com>

bors · 2023-08-08T10:51:14Z

Build failed:

bors-ok

Frassle · 2023-08-08T11:02:33Z

Setup actions failed

bors retry

bors · 2023-08-08T11:40:02Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

bors-ok

RobbieMcKinstry requested review from Zaid-Ajaj and dixler March 31, 2023 18:40

dixler reviewed Mar 31, 2023

View reviewed changes

Frassle reviewed Mar 31, 2023

View reviewed changes

RobbieMcKinstry mentioned this pull request Jul 27, 2023

404 getting pulumi-language-nodejs #11703

Closed

Frassle self-assigned this Jul 27, 2023

Frassle force-pushed the mckinstry/#11703 branch from de2d8ad to c41ca1d Compare July 28, 2023 10:11

Frassle reviewed Jul 28, 2023

View reviewed changes

abhinav force-pushed the mckinstry/#11703 branch from c41ca1d to 338d79d Compare July 28, 2023 16:03

Frassle force-pushed the mckinstry/#11703 branch 2 times, most recently from 6d8e293 to 69bf8b8 Compare August 1, 2023 08:16

abhinav approved these changes Aug 1, 2023

View reviewed changes

abhinav and others added 2 commits August 7, 2023 15:32

Frassle force-pushed the mckinstry/#11703 branch from 69bf8b8 to c8d331c Compare August 7, 2023 14:33

bors bot merged commit 64aed77 into master Aug 8, 2023
52 checks passed

bors bot deleted the mckinstry/#11703 branch August 8, 2023 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if Plugin is bundled before installing #12575

Check if Plugin is bundled before installing #12575

RobbieMcKinstry commented Mar 31, 2023

pulumi-bot commented Mar 31, 2023 •

edited

dixler Mar 31, 2023

Frassle Mar 31, 2023

RobbieMcKinstry Mar 31, 2023

RobbieMcKinstry Mar 31, 2023

Frassle Mar 31, 2023

Frassle Mar 31, 2023

Frassle Mar 31, 2023

Frassle Mar 31, 2023

Frassle Mar 31, 2023

RobbieMcKinstry Mar 31, 2023

Frassle Mar 31, 2023

Frassle Jul 28, 2023

Frassle Jul 28, 2023

abhinav Jul 28, 2023 •

edited

Frassle Jul 28, 2023

abhinav Jul 28, 2023

Frassle Jul 31, 2023

abhinav commented Jul 28, 2023

abhinav Aug 1, 2023

Frassle Aug 1, 2023

Frassle commented Aug 7, 2023

bors bot commented Aug 7, 2023

Frassle commented Aug 8, 2023

bors bot commented Aug 8, 2023

Frassle commented Aug 8, 2023

bors bot commented Aug 8, 2023

	// IsPreinstalledLanguagePlugin returns true if the spec describes
	// IsBundledPlugin returns true if the spec describes

	// a language plugin that comes preinstalled with the CLI.
	// a plugin that comes preinstalled with the CLI.

Check if Plugin is bundled before installing #12575

Check if Plugin is bundled before installing #12575

Conversation

RobbieMcKinstry commented Mar 31, 2023

Description

Checklist

pulumi-bot commented Mar 31, 2023 • edited

Changelog

[uncommitted] (2023-08-07)

Bug Fixes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinav Jul 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinav commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frassle commented Aug 7, 2023

bors bot commented Aug 7, 2023

Frassle commented Aug 8, 2023

bors bot commented Aug 8, 2023

Frassle commented Aug 8, 2023

bors bot commented Aug 8, 2023

pulumi-bot commented Mar 31, 2023 •

edited

abhinav Jul 28, 2023 •

edited