From bb563f7cc4d919fe6bc2c6f978c9c5317264d497 Mon Sep 17 00:00:00 2001
From: Lucas Bajolet <lucas.bajolet@hashicorp.com>
Date: Thu, 1 Feb 2024 15:01:37 -0500
Subject: [PATCH] packer: fix plugin version sorting and pickup

When Packer orders the plugins by their version, we'd assumed it was
ordered from highest to lowest.

However, this was the case because our implementation of `Less' was
actually returning `>=', which is not what it was supposed to do.

This commit therefore fixes the implementation of `Less' to do what it
is documented to do, and ensures the behaviour is correct through
additional testing.
Changing this requires some changes to the loading process as well
because of the aforementioned assumption with regards to ordering.
---
 hcl2template/plugin.go               |   2 +-
 packer/plugin-getter/plugins.go      |   2 +-
 packer/plugin-getter/plugins_test.go | 120 +++++++++++++++++++++++++++
 packer/plugin.go                     |  10 ---
 4 files changed, 122 insertions(+), 12 deletions(-)

diff --git a/hcl2template/plugin.go b/hcl2template/plugin.go
index 279f534e0..15358573d 100644
--- a/hcl2template/plugin.go
+++ b/hcl2template/plugin.go
@@ -94,7 +94,7 @@ func (cfg *PackerConfig) DetectPluginBinaries() hcl.Diagnostics {
 			continue
 		}
 		log.Printf("[TRACE] Found the following %q installations: %v", pluginRequirement.Identifier, sortedInstalls)
-		install := sortedInstalls[0]
+		install := sortedInstalls[len(sortedInstalls)-1]
 		err = cfg.parser.PluginConfig.DiscoverMultiPlugin(pluginRequirement.Accessor, install.BinaryPath)
 		if err != nil {
 			diags = append(diags, &hcl.Diagnostic{
diff --git a/packer/plugin-getter/plugins.go b/packer/plugin-getter/plugins.go
index 6fec3dddb..11511ee38 100644
--- a/packer/plugin-getter/plugins.go
+++ b/packer/plugin-getter/plugins.go
@@ -267,7 +267,7 @@ func (l InstallList) Less(i, j int) bool {
 		return lowRawPluginName < hiRawPluginName
 	}
 
-	return semver.Compare(lowPluginPath.Version, hiPluginPath.Version) > 0
+	return semver.Compare(lowPluginPath.Version, hiPluginPath.Version) < 0
 }
 
 // Swap swaps the elements with indexes i and j.
diff --git a/packer/plugin-getter/plugins_test.go b/packer/plugin-getter/plugins_test.go
index 4cc78acc3..9fb248304 100644
--- a/packer/plugin-getter/plugins_test.go
+++ b/packer/plugin-getter/plugins_test.go
@@ -507,3 +507,123 @@ func zipFile(content map[string]string) io.ReadCloser {
 }
 
 var _ Getter = &mockPluginGetter{}
+
+func Test_LessInstallList(t *testing.T) {
+	tests := []struct {
+		name       string
+		installs   InstallList
+		expectLess bool
+	}{
+		{
+			"v1.2.1 < v1.2.2 => true",
+			InstallList{
+				&Installation{
+					BinaryPath: "host/org/plugin",
+					Version:    "v1.2.1",
+				},
+				&Installation{
+					BinaryPath: "github.com",
+					Version:    "v1.2.2",
+				},
+			},
+			true,
+		},
+		{
+			// Impractical with the changes to the loading model
+			"v1.2.1 = v1.2.1 => false",
+			InstallList{
+				&Installation{
+					BinaryPath: "host/org/plugin",
+					Version:    "v1.2.1",
+				},
+				&Installation{
+					BinaryPath: "github.com",
+					Version:    "v1.2.1",
+				},
+			},
+			false,
+		},
+		{
+			"v1.2.2 < v1.2.1 => false",
+			InstallList{
+				&Installation{
+					BinaryPath: "host/org/plugin",
+					Version:    "v1.2.2",
+				},
+				&Installation{
+					BinaryPath: "github.com",
+					Version:    "v1.2.1",
+				},
+			},
+			false,
+		},
+		{
+			"v1.2.2-dev < v1.2.2 => true",
+			InstallList{
+				&Installation{
+					BinaryPath: "host/org/plugin",
+					Version:    "v1.2.2-dev",
+				},
+				&Installation{
+					BinaryPath: "github.com",
+					Version:    "v1.2.2",
+				},
+			},
+			true,
+		},
+		{
+			"v1.2.2 < v1.2.2-dev => false",
+			InstallList{
+				&Installation{
+					BinaryPath: "host/org/plugin",
+					Version:    "v1.2.2",
+				},
+				&Installation{
+					BinaryPath: "github.com",
+					Version:    "v1.2.2-dev",
+				},
+			},
+			false,
+		},
+		{
+			"v1.2.1 < v1.2.2-dev => true",
+			InstallList{
+				&Installation{
+					BinaryPath: "host/org/plugin",
+					Version:    "v1.2.1",
+				},
+				&Installation{
+					BinaryPath: "github.com",
+					Version:    "v1.2.2-dev",
+				},
+			},
+			true,
+		},
+		{
+			"v1.2.3 < v1.2.2-dev => false",
+			InstallList{
+				&Installation{
+					BinaryPath: "host/org/plugin",
+					Version:    "v1.2.3",
+				},
+				&Installation{
+					BinaryPath: "github.com",
+					Version:    "v1.2.2-dev",
+				},
+			},
+			false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			isLess := tt.installs.Less(0, 1)
+			if isLess != tt.expectLess {
+				t.Errorf("Less mismatch for %s < %s, expected %t, got %t",
+					tt.installs[0].Version,
+					tt.installs[1].Version,
+					tt.expectLess, isLess)
+			}
+		})
+	}
+}
diff --git a/packer/plugin.go b/packer/plugin.go
index 5fcef8954..c6d4c1259 100644
--- a/packer/plugin.go
+++ b/packer/plugin.go
@@ -100,16 +100,6 @@ func (c *PluginConfig) Discover() error {
 		}
 
 		pluginName := matches[1]
-
-		// If the plugin is already registered in the plugin map, we
-		// can ignore the current executable, as they're sorted by
-		// version in descending order, so if it's already in the map,
-		// a more recent version was already discovered.
-		_, ok := pluginMap[pluginName]
-		if ok {
-			continue
-		}
-
 		pluginMap[pluginName] = install.BinaryPath
 	}