Ensure SemanticVersion field is always non-nil to avoid panics, fix c…

…hangelog
hashicorp · Aug 25, 2022 · 7e0106e · 7e0106e
1 parent 360056d
commit 7e0106e
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 28 deletions.
diff --git a/changelog/16688.txt b/changelog/16688.txt
@@ -1,6 +1,6 @@
-```change
+```release-note:change
 plugins: `GET /sys/plugins/catalog` endpoint now returns an additional `detailed` field in the response with additional plugin metadata.
 ```
-```improvement
+```release-note:improvement
 plugins: Plugin catalog supports registering plugins with a semantic version
 ```
diff --git a/vault/logical_system_test.go b/vault/logical_system_test.go
@@ -4882,41 +4882,53 @@ func TestSystemBackend_LoggersByName(t *testing.T) {
 }
 
 func TestSortVersionedPlugins(t *testing.T) {
-	versionedPlugin := func(typ consts.PluginType, name string, version string) pluginutil.VersionedPlugin {
+	versionedPlugin := func(typ consts.PluginType, name string, version string, builtin bool) pluginutil.VersionedPlugin {
 		return pluginutil.VersionedPlugin{
-			Type:            typ.String(),
-			Name:            name,
-			Version:         version,
-			SHA256:          "",
-			Builtin:         false,
-			SemanticVersion: semver.Must(semver.NewVersion(version)),
+			Type:    typ.String(),
+			Name:    name,
+			Version: version,
+			SHA256:  "",
+			Builtin: builtin,
+			SemanticVersion: func() *semver.Version {
+				if version != "" {
+					return semver.Must(semver.NewVersion(version))
+				}
+
+				return semver.Must(semver.NewVersion("0.0.0"))
+			}(),
 		}
 	}
 
 	differingTypes := []pluginutil.VersionedPlugin{
-		versionedPlugin(consts.PluginTypeSecrets, "c", "1.0.0"),
-		versionedPlugin(consts.PluginTypeDatabase, "c", "1.0.0"),
-		versionedPlugin(consts.PluginTypeCredential, "c", "1.0.0"),
+		versionedPlugin(consts.PluginTypeSecrets, "c", "1.0.0", false),
+		versionedPlugin(consts.PluginTypeDatabase, "c", "1.0.0", false),
+		versionedPlugin(consts.PluginTypeCredential, "c", "1.0.0", false),
 	}
 	differingNames := []pluginutil.VersionedPlugin{
-		versionedPlugin(consts.PluginTypeCredential, "c", "1.0.0"),
-		versionedPlugin(consts.PluginTypeCredential, "b", "1.0.0"),
-		versionedPlugin(consts.PluginTypeCredential, "a", "1.0.0"),
+		versionedPlugin(consts.PluginTypeCredential, "c", "1.0.0", false),
+		versionedPlugin(consts.PluginTypeCredential, "b", "1.0.0", false),
+		versionedPlugin(consts.PluginTypeCredential, "a", "1.0.0", false),
 	}
 	differingVersions := []pluginutil.VersionedPlugin{
-		versionedPlugin(consts.PluginTypeCredential, "c", "10.0.0"),
-		versionedPlugin(consts.PluginTypeCredential, "c", "2.0.1"),
-		versionedPlugin(consts.PluginTypeCredential, "c", "2.1.0"),
+		versionedPlugin(consts.PluginTypeCredential, "c", "10.0.0", false),
+		versionedPlugin(consts.PluginTypeCredential, "c", "2.0.1", false),
+		versionedPlugin(consts.PluginTypeCredential, "c", "2.1.0", false),
+	}
+	versionedUnversionedAndBuiltin := []pluginutil.VersionedPlugin{
+		versionedPlugin(consts.PluginTypeCredential, "c", "1.0.0", false),
+		versionedPlugin(consts.PluginTypeCredential, "c", "", false),
+		versionedPlugin(consts.PluginTypeCredential, "c", "1.0.0", true),
 	}
 
 	for name, tc := range map[string][]pluginutil.VersionedPlugin{
 		"ascending types":    differingTypes,
 		"ascending names":    differingNames,
 		"ascending versions": differingVersions,
 		// Include differing versions twice so we can test out equality too.
-		"all types": append(differingTypes,
+		"differing types, names and versions": append(differingTypes,
 			append(differingNames,
 				append(differingVersions, differingVersions...)...)...),
+		"mix of unversioned, versioned, and builtin": versionedUnversionedAndBuiltin,
 	} {
 		t.Run(name, func(t *testing.T) {
 			sortVersionedPlugins(tc)

diff --git a/vault/plugin_catalog.go b/vault/plugin_catalog.go
@@ -699,9 +699,19 @@ func (c *PluginCatalog) listInternal(ctx context.Context, pluginType consts.Plug
 		switch len(parts) {
 		case 1: // Unversioned, no type (legacy)
 			normalizedName = parts[0]
+			// Use 0.0.0 to ensure unversioned is sorted as the oldest version.
+			semanticVersion, err = semver.NewVersion("0.0.0")
+			if err != nil {
+				return nil, err
+			}
 		case 2: // Unversioned
 			if isPluginType(parts[0]) {
 				normalizedName = parts[1]
+				// Use 0.0.0 to ensure unversioned is sorted as the oldest version.
+				semanticVersion, err = semver.NewVersion("0.0.0")
+				if err != nil {
+					return nil, err
+				}
 			} else {
 				return nil, fmt.Errorf("unknown plugin type in plugin catalog: %s", plugin)
 			}

diff --git a/vault/plugin_catalog_test.go b/vault/plugin_catalog_test.go
@@ -256,9 +256,7 @@ func TestPluginCatalog_ListVersionedPlugins(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unexpected error %v", err)
 	}
-	sort.SliceStable(plugins, func(i, j int) bool {
-		return plugins[i].Name < plugins[j].Name
-	})
+	sortVersionedPlugins(plugins)
 
 	if len(plugins) != len(builtinKeys) {
 		t.Fatalf("unexpected length of plugin list, expected %d, got %d", len(builtinKeys), len(plugins))
@@ -278,13 +276,31 @@ func TestPluginCatalog_ListVersionedPlugins(t *testing.T) {
 	defer file.Close()
 
 	command := filepath.Base(file.Name())
-	err = core.pluginCatalog.Set(context.Background(), "mysql-database-plugin", consts.PluginTypeDatabase, "", command, []string{"--test"}, []string{}, []byte{'1'})
+	err = core.pluginCatalog.Set(
+		context.Background(),
+		"mysql-database-plugin",
+		consts.PluginTypeDatabase,
+		"",
+		command,
+		[]string{"--test"},
+		[]string{},
+		[]byte{'1'},
+	)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	// Set another plugin
-	err = core.pluginCatalog.Set(context.Background(), "aaaaaaa", consts.PluginTypeDatabase, "", command, []string{"--test"}, []string{}, []byte{'1'})
+	// Set another plugin, with version information
+	err = core.pluginCatalog.Set(
+		context.Background(),
+		"aaaaaaa",
+		consts.PluginTypeDatabase,
+		"1.1.0",
+		command,
+		[]string{"--test"},
+		[]string{},
+		[]byte{'1'},
+	)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -294,9 +310,7 @@ func TestPluginCatalog_ListVersionedPlugins(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unexpected error %v", err)
 	}
-	sort.SliceStable(plugins, func(i, j int) bool {
-		return plugins[i].Name < plugins[j].Name
-	})
+	sortVersionedPlugins(plugins)
 
 	// plugins has a test-added plugin called "aaaaaaa" that is not built in
 	if len(plugins) != len(builtinKeys)+1 {
@@ -307,6 +321,9 @@ func TestPluginCatalog_ListVersionedPlugins(t *testing.T) {
 	if !reflect.DeepEqual(plugins[0].Name, "aaaaaaa") {
 		t.Fatalf("expected did not match actual, got %#v\n expected %#v\n", plugins[0], "aaaaaaa")
 	}
+	if plugins[0].SemanticVersion == nil {
+		t.Fatalf("expected non-nil semantic version for %v", plugins[0].Name)
+	}
 
 	// verify the builtin plugins are correct
 	for i, plugin := range plugins[1:] {
@@ -329,6 +346,10 @@ func TestPluginCatalog_ListVersionedPlugins(t *testing.T) {
 				t.Fatalf("expected +builtin metadata but got %s", plugin.Version)
 			}
 		}
+
+		if plugin.SemanticVersion == nil {
+			t.Fatalf("expected non-nil semantic version for %v", plugin)
+		}
 	}
 }