Harden remote plugin installs #12915
Conversation
lbajolet-hashicorp
added
enhancement
command/plugins
lbajolet-hashicorp
force-pushed
the
harden_remote_plugin_installs
branch
4 times, most recently
from
6f12837 to
d6662c9
Compare
lbajolet-hashicorp
force-pushed
the
harden_remote_plugin_installs
branch
from
d6662c9 to
7a60d5e
Compare
lbajolet-hashicorp
force-pushed
the
harden_remote_plugin_installs
branch
from
7a60d5e to
540c5b6
Compare
lbajolet-hashicorp
force-pushed
the
harden_remote_plugin_installs
branch
from
540c5b6 to
1729ed9
Compare
|
So this change started because we enforce the version match when installing via I say that because for a user wishing to test a new version of a binary, if there is a mismatch, I will have to open an issue on the parent repo and wait. Or I can build the binary with the correct version which then introduces a new step of where someone has to build a binary just to fix the version. Or I can rename the binary to match the version which may be off the next time I install. All options which will need to be documented. IMO any manual intervention will bring CI/CD to halt until addressed. By forcing the version mismatch at local binary install I can see folks acting to it sooner. If we default the version to a dev version I also see people continuing to use the binary but can then decide what to do. They may be okay with the binary being a dev version, especially since required_versions supports dev version matching. What do you think? |
Hmmmm, I like the idea honestly. This may indeed be a good bridging solution which may motivate users to report the problem upstream while not completely blocking them. I'll see what I can do to amend this PR to adopt this behaviour instead. |
packer/plugin-getter/plugins.go
Outdated
| errs = multierror.Append(errs, err) | ||
| continue | ||
| } | ||
| if descVersion.Prerelease() != "" { |
There was a problem hiding this comment.
This code is also adding hardening to the installation of prereleases, right? Should this check be after the check of versions?
There was a problem hiding this comment.
Pre-release remote installation is not supported at the moment, only local installation is through packer plugins install --path.
Though with Wilken's last remark, I guess this may change so that the version remotely installed could be a dev one, albeit with a warning that there's a mismatch.
But in any case, describing a constraint with -dev in the version, or installing pre-releases through packer plugins install without passing in a path is not a supported workflow for the moment.
lbajolet-hashicorp
force-pushed
the
harden_remote_plugin_installs
branch
from
1729ed9 to
3c043c1
Compare
|
I added this description to the CHANGELOG as a temporary entry. core: Remote plugins installed containing an internal version number that differs from the version
number within the binary name can lead to confusion when tracking Packer plugin version
information. To help track such deprecates in the plugin version, packer init and packer plugin install have been updated to install such plugins as dev versions (e.g. 1.0.0-dev).
Users are encouraged to notify plugin maintainers of any version mismatches. https://github.com/hashicorp/packer/pull/12915I think that for remote installation and local installation a version mismatch should not fail warn and treat the binary as a development binary. This will not break user flows who may have this issue now. In the future if we need to we can make it a hard error. Thoughts. |
|
hmmm. It looks like the following situation in Packer today errors silently. I tried renaming a binary to have a different version and Packer errors saying the the provision was not found. Looking at the logs I found the message indicating why it was not loaded. But this was not straight forward. |
Ah yes, this is in line with how listing installations worked previously. I'm a bit skeptical of this approach to an extent as it may report too many errors and drown the problem a bit, but this would help in situations like the one you describe. |
lbajolet-hashicorp
force-pushed
the
harden_remote_plugin_installs
branch
from
3c043c1 to
cd51a94
Compare
|
While I understand the rationale for wanting to include the check. It is adding an additional amount of work and a bit of unknowns that I fear will become a hurdle for users. Our goal for predictable loading was to consolidate plugin loading into a single binary name and directory. This additional change seems a bit premature at this time. Especially since less than a handful of plugins are running into this case. Once we add this behavior to an official release it would be hard to retract it if it is relied upon. Seeing as this has never been an issue, let's hold and revisit if and when it becomes a thing. The new HCP Packer version tracking will bring more visibility into the versions being used. I suggest we revert the change where we require the version in the binary name to match the version in the describe command as the next step. #12888 |
lbajolet-hashicorp
force-pushed
the
harden_remote_plugin_installs
branch
3 times, most recently
from
c356041 to
08e42c7
Compare
The zipfile containing the binaries we attempt to install from a remote source is placed in the temporary directory of the host. In general it is wiped automatically by the OS, but in some cases (windows typically), it isn't. To avoid cluttering the temporary directory, we clean-up after ourselves, and remove the temporary zip file that we create when attempting to install a plugin, regardless of it succeeding or not.
Given that calling the describe command on plugins and deserialising the output as a plugin description is something done in multiple places in the code, we factorise this operation so we don't need to copy/paste the code around.
When installing a remote plugin, and after we've either successfully installed a binary, or exhausted all the possible sources, we print a final error message if nothing was reported. However, given that errs is a pointer to a structure, and if no errors were produced, the the error list could be nil, leading to the call to `Len()' to crash Packer. This is exceedingly rare as in general the code attempts to read multiple sources from Github, and therefore we almost always get some error reported, but while changing the function's code, I managed to make it crash while removing/changing some error statements. Therefore to avoid future surprises, we first check that `errs' is not nil before invoking `Len()' on it, as no errors and no plugins installed mean that something went wrong all the same.
Since we named the version from the getter `version', this means we have a naming conflict inside the loop that attempts to install a versioned candidate for a plugin, making it impossible to invoke something from the go-version package. Since we'll introduce a change that needs the latter capability, we must either rename the local variable to something else than `version', or we need to alias the package locally. This commit implements the latter, opting to call the package goversion.
Whenever a Github release exposes an entry for another OS/arch combination, this gets registered as an error, which in the event no binary is compatible with the host's OS/arch, gets reported at the end of the getter process. While this is sound in theory, in practice we get the list of all the combinations that don't match the host's, which is not something a Packer user can act on, and might therefore be more confusing than helping to solve the issue. Therefore we opt in this commit to stop registering those cases as real errors, and only log them as an INFO statement.
When listing installed plugins, we check that the plugin's reported version through describe matches what's in the name of the file. Doing do, we were parsing the same version string twice without modifying it, which was not necessary, so this commit changes that.
Since we're hardening what Packer is able to load locally when it comes to plugins, we need also to harden the installation process a bit. While testing we noticed some remotes had published their plugins with version mismatches between the tag and the binary. This was not a problem in the past, as Packer did not care for this, only the binary name was important, and the plugin could be installed without problem. Nowadays however, since Packer enforces the plugin version reported in the name to be the same as the plugin self-reported version, this makes it impossible for the installed plugin to load anymore in such an instance. Therefore in order to limit confusion, and so users are able to understand the problem and report it to the plugins with that mismatch, we instead install the plugin as a dev version, and report it to the user so they can still use it, but are able to report it to the plugin developers.
When packer init is invoked with a --force argument, but no --update, we clamp the version to install based on the last one locally installed. Doing this may however cause the constraint to always be false if the latest available version of a plugin is a pre-release, as none of the upstream constraints will match that. Therefore this commit changes how the constraint is derived from the local list of installations, so that only the last installation that matches the original constraint will be used, and not a pre-release.
When installing a plugin from a remote source, we list the installed plugins that match the constraints specified, and if the constraint is already satisfied, we don't do anything. However, since remote installation is only relevant for releases of a plugin, we should only look at the installed releases of a plugin, and not consider pre-releases for that step. This wasn't the case before this commit, as if a prerelease version of a commit (ex: 10.8.1-dev), and we try to invoke `packer init` with a constraint on this version specifically, Packer would locate that pre-release and assume it was already installed, so would silently succeed the command and do nothing. This isn't the expected behaviour as we should install the final release of that plugin, regardless of any prerelease installation of the plugin. So this commit fixes that by only listing releases, so we don't report the plugin being already installed if a prerelease is what's installed.
When a checksum file for a release is downloaded and iterated upon to find the compatible binary for a release, we used to log each non-compatible entry in the logs. This is noisy as we know there's going to be multiple entries that don't match the host's os/arch, and there's no good reason to show those, so we silence them.
nywilken
force-pushed
the
harden_remote_plugin_installs
branch
from
1a37f84 to
709fdcd
Compare
This change is an attempt to remove the need for additional temporary files, along with calls to stat the temp files, to reduce the number of files being created, opened, and closed. In addition to this change, the logic for falling back to a previous version if the highest matched version is a pre-release has been removed. Instead we will assume that any prior versions will exhibit the same issue and return immediately. A user can install the version manually if they will or they can modify their version constraint to a properly released version.
``` ~> packer init mondoo_req.pkr.hcl Failed getting the "github.com/mondoohq/cnspec" plugin: error: Remote installation of the plugin version 10.8.1-dev is unsupported. This is likely an upstream issue with the 10.8.1 release, which should be reported. If you require this specific version of the plugin, download the binary and install it manually. packer plugins install --path '<plugin_binary>' github.com/mondoohq/cnspec ```
* Only create plugin directories if there is potential plugin install
… a prerelease * Refactor InstallError string messages
|
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. |
Since we're hardening what Packer is able to load locally when it comes to plugins, we need also to harden the installation process a bit.
While testing we noticed some remotes had published their plugins with version mismatches between the tag and the binary.
This was not a problem in the past, as Packer did not care for this, only the binary name was important, and the plugin could be installed without problem.
Nowadays however, since Packer enforces the plugin version reported in the name to be the same as the plugin self-reported version, this makes it impossible for the installed plugin to load anymore in such an instance.
Therefore in order to limit confusion, and so users are able to understand the problem and report it to the plugins with that mismatch, we reject the installations that expose this mismatch, and report it to the user if they cannot install anything else.