Plugin discovery fixes #12888
Plugin discovery fixes #12888
Conversation
lbajolet-hashicorp
force-pushed
the
plugin_discovery_fixes
branch
2 times, most recently
from
da72caf to
4d4d207
Compare
lbajolet-hashicorp
force-pushed
the
plugin_discovery_fixes
branch
from
4d4d207 to
3c9809b
Compare
When discovering the installed plugins locally, we perform a couple of checks on the version, namely that it is valid, if it is a prerelease, it needs to be a dev, and that the self-reported version matches the one hinted at through the name of the binary. This was done through regexes, but those were a wee bit simple when dealing with versions that have metadata. Those binaries would be completely ignored by Packer, and never loaded, although they are a valid use case. The version library we already used supports those however, and comparisons are more reliable with them. So, in order to simplify our code, and make it more reliable, we're exclusively using this library to perform parsing and comparisons of versions during the discovery phase.
When Packer discovers binary a bunch of checks are performed, which ultimately end with a checksum match check. This however should be the very first thing we do, even before attempting to run `describe' on the plugin binary we're discovering. So this commit moves this checksum match to the top of the discovery process for binaries.
When installing a plugin from a local binary, Packer builds the name of the plugin from the results of the `describe' command. Depending on how the plugin is built, the version reported may or may not contain a leading `v', which was not taken into account beforehand and the leading `v' was always injected in the path. This caused plugins that report a leading `v' in their version to be installed with two v's in their path, making them impossible to load. Therefore to fix this issue, we count on the version library to print out a version without the leading v, and we inject that in the resulting path.
When installing plugins with the `packer plugins install --path' command, the metadata is now scrubbed from the file installed locally. This is as a protection against collisions in the versions, as metadata is meaningless for version comparison, so if two versions of the same plugin are installed, the precedence order between them is undefined. Therefore to avoid such collisions, we remove the metadata from the file name, that way if two successive versions of a plugin include metadata in the version, they won't coexist, and the last installed will be the only installed version locally.
As with the version of the plugin, the API version should also match between the path and the self-reported API version from the describe command. This was not checked before, so users could masquerade a plugin's use of an API version that may be incompatible with Packer. To avoid this problem, we make sure both versions are the same, so that they work as expected.
lbajolet-hashicorp
force-pushed
the
plugin_discovery_fixes
branch
from
8fb1e76 to
0e9e523
Compare
| @@ -300,10 +300,16 @@ func (c *PluginsInstallCommand) InstallFromBinary(opts plugingetter.ListInstalla | |||
| }}) | |||
| } | |||
|
|
|||
| // Remove metadata from plugin path | |||
| noMetaVersion := semver.Core().String() | |||
There was a problem hiding this comment.
Is it possible to add a test for this logic in the existing TestPluginsInstallCommand_Run?
There was a problem hiding this comment.
Regarding testing, unit tests for commands are notoriously awful as they involve a lot of moving parts, which if we start changing behaviour starts breaking in completely unrelated places in the code (that's the only reason why the "mock plugins" initialisation method changes in this PR).
I have started looking into this after our (out of band) discussion, but frankly this is quite a disruptive change, and I don't want to delay the release of 1.11.0-alpha2.
I'll propose the following: let's try to merge this tomorrow, and release the alpha, this will let users experiment with a more up-to-date Packer, which ultimately will lead to us figuring out what else needs to be addressed in the code before the final release.
In parallel, I'll work on proper blackbox testing for commands, I had started working on that kind of testing platform in a separate repo, but I do believe we can make things work in a semi self-contained way. I'll prioritise this effort since it'll increase confidence in those changes, and will give us a base to improve testing all around later.
Does that sound good to you? If so could you approve this PR tomorrow so we can merge and release; otherwise please let me know what would be acceptable for you, and we can discuss the next steps towards merging this PR.
There was a problem hiding this comment.
especially related to the addition of the InstallList.ApiVersion
OK nevermind I'm an idiot, is this related to the sorting tests? That can be unit tested for sure, sorry I misunderstood your point! I'll reroll that change with those changes in.
There was a problem hiding this comment.
And good call, semver doesn't work with API versions, that's broken.
I'll fix that and add the tests right now, thanks for the persistence and the callout @sylviamoss
There was a problem hiding this comment.
Should be fixed now.
There was a problem hiding this comment.
I think you still talked through a point that I was also suggesting to be tested here, so your first message still makes sense, and for that I am with you on your decision if that's the best way to go.
For the InstallList.ApiVersion I did notice unit tests that could have been updated to cover that code and I am glad you caught a bug because of them 😅 thanks for working on them!
lbajolet-hashicorp
force-pushed
the
plugin_discovery_fixes
branch
2 times, most recently
from
b0cdceb to
c624c3a
Compare
packer/plugin-getter/plugins.go
Outdated
| if err != nil { | ||
| return err | ||
| panic(fmt.Sprintf("malformed API version in Packer. This is a programming error, please open an error to report it.")) |
There was a problem hiding this comment.
Why do we panic here instead of returning an error?
There was a problem hiding this comment.
Ah good question; this parsing is done on static data that is normally always valid: the API versions embedded in Packer (i.e. those from the SDK).
If it goes wrong, we are in an invalid state, and we definitely don't want to continue operating, since we'll never be able to check compatibility with plugins, protocol-wise at least.
Come to think of it, this may become an init time check, we probably want to crash ASAP if that somehow breaks someday.
I'll make the change before merging this PR
lbajolet-hashicorp
force-pushed
the
plugin_discovery_fixes
branch
from
c624c3a to
640d22b
Compare
As with versions, API versions are useful possibly for ordering plugin installations in order for Packer to choose which plugin to load. This could be unnecessary as API versions are stable, and only in dev plugins this could be a problem normally (there shouldn't be two same releases of a plugin), but this cements API version in ordering plugins so we avoid surprises later down the line.
The former way mock plugin sets were created meant that no API version was set, and since it's private in the SDK, it cannot be set outside of the package itself. Fortunately, there is a NewSet function we can call, which initialises a set properly so we can fill-in the information later. However, because all the set maps were created in a `var` section, we cannot create the set with `NewSet`, and then fill the information in (outside of if there was a fluent interface, but this isn't the case here). We therefore opted to keep the variables defined and accessible globally, but gone through a `init` function to initialise their values for tests.
If a plugin is installed manually, its version number could be valid but non-canonical (ex: 1.2.3 vs 01.002.0003). Since these two versions refer to the same version, but the looks are different, this may become ambiguous which version should be loaded. To avoid such a situation, we reject explicitely non-canonical version numbers in plugins, but only in path, we're aware that because of metadata, the version from `describe' may already differ from the file name.
When introduced back in January, the version variable extracted from the binary name had a typo in its name, and was named `protocolVerionStr' instead of `protocolVersionStr'. This commit was already merged into main, so it's too late to fix at introduction site, but we can fix it today as a separate commit ontop the stack.
lbajolet-hashicorp
force-pushed
the
plugin_discovery_fixes
branch
from
640d22b to
648aff4
Compare
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Multiple changes here: