private/version: use minimum key in new sem version

Much evident on the storagenode dashboard, the minimum
version shown is a very old version and that is taken
from the deprecated part of version info from the
version control server, and we no longer update the
deprecated part on the server.

This change forces it to use the new sem version, and
checks for the old version if the server is probably
running an old version of the version control system.

Also fixes a bug where the suggested version returned
for all processes is taken from the storagenode part.

Issue: https://github.com/storj/storj/issues/5673

Change-Id: I57b7358c2826a6e25f441dfa9579a1aef50a7e89

private/version/checker/client_test.go

@@ -101,7 +101,14 @@ func newTestPeer(t *testing.T, ctx *testcontext.Context) *versioncontrol.Peer {
 		},
 		Binary: testVersions,
 	}
-	peer, err := versioncontrol.New(zaptest.NewLogger(t), serverConfig)
+
+	return newTestPeerWithConfig(t, ctx, serverConfig)
+}
+
+func newTestPeerWithConfig(t *testing.T, ctx *testcontext.Context, config *versioncontrol.Config) *versioncontrol.Peer {
+	t.Helper()
+
+	peer, err := versioncontrol.New(zaptest.NewLogger(t), config)
 	require.NoError(t, err)
 
 	ctx.Go(func() error {

private/version/checker/service.go

@@ -98,11 +98,13 @@ func (service *Service) checkVersion(ctx context.Context) (_ version.SemVer, all
 		service.checked.Release()
 	}()
 
-	allowedVersions, err := service.client.All(ctx)
+	process, err := service.client.Process(ctx, service.service)
 	if err != nil {
+		service.log.Error("failed to get process version info", zap.Error(err))
 		return service.acceptedVersion, true
 	}
-	suggestedVersion, err := allowedVersions.Processes.Storagenode.Suggested.SemVer()
+
+	suggestedVersion, err := process.Suggested.SemVer()
 	if err != nil {
 		return service.acceptedVersion, true
 	}
@@ -121,28 +123,40 @@ func (service *Service) checkVersion(ctx context.Context) (_ version.SemVer, all
 		return suggestedVersion, true
 	}
 
-	minimumOld, err := service.client.OldMinimum(ctx, service.service)
+	minimum, err = process.Minimum.SemVer()
 	if err != nil {
-		// Log about the error, but dont crash the Service and allow further operation
-		service.log.Error("Failed to do periodic version check.", zap.Error(err))
 		return suggestedVersion, true
 	}
 
-	minimum, err = version.NewSemVer(minimumOld.String())
+	if minimum.IsZero() {
+		// if the minimum version is not set, we check if the old minimum version is set
+		// TODO: I'm not sure if we should remove this check and stop supporting the old format,
+		//  but it seems like it's no longer needed, assuming there are no known community
+		//  satellites (or SNOs personally) running an old version control server, which (I think)
+		//  is very obviously 100% true currently.
+		minimumOld, err := service.client.OldMinimum(ctx, service.service)
 		if err != nil {
-		service.log.Error("Failed to convert old sem version to sem version.")
 			return suggestedVersion, true
 		}
 
+		minOld, err := version.NewSemVer(minimumOld.String())
+		if err != nil {
+			service.log.Error("failed to convert old sem version to new sem version", zap.Error(err))
+			return suggestedVersion, true
+		}
+
+		minimum = minOld
+	}
+
 	service.log.Debug("Allowed minimum version from control server.", zap.Stringer("Minimum Version", minimum.Version))
 
-	if isAcceptedVersion(service.Info.Version, minimumOld) {
+	if service.Info.Version.Compare(minimum) >= 0 {
 		service.log.Debug("Running on allowed version.", zap.Stringer("Version", service.Info.Version.Version))
 		return suggestedVersion, true
 	}
 	service.log.Warn("version not allowed/outdated",
 		zap.Stringer("current version", service.Info.Version.Version),
-		zap.Stringer("minimum allowed version", minimumOld),
+		zap.String("minimum allowed version", minimum.String()),
 	)
 	return suggestedVersion, false
 }
@@ -168,8 +182,3 @@ func (service *Service) SetAcceptedVersion(version version.SemVer) {
 func (service *Service) Checked() bool {
 	return service.checked.Released()
 }
-
-// isAcceptedVersion compares and checks if the passed version is greater/equal than the minimum required version.
-func isAcceptedVersion(test version.SemVer, target version.OldSemVer) bool {
-	return test.Major > uint64(target.Major) || (test.Major == uint64(target.Major) && (test.Minor > uint64(target.Minor) || (test.Minor == uint64(target.Minor) && test.Patch >= uint64(target.Patch))))
-}

private/version/checker/service_test.go

@@ -0,0 +1,111 @@
+// Copyright (C) 2023 Storj Labs, Inc.
+// See LICENSE for copying information.
+
+package checker_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap/zaptest"
+
+	"storj.io/common/testcontext"
+	"storj.io/private/version"
+	"storj.io/storj/private/version/checker"
+	"storj.io/storj/versioncontrol"
+)
+
+func TestVersion(t *testing.T) {
+	ctx := testcontext.New(t)
+	defer ctx.Cleanup()
+
+	minimum := "v1.89.5"
+	suggested := "v1.90.2"
+
+	testVersions := newTestVersions(t)
+	testVersions.Storagenode.Minimum.Version = minimum
+	testVersions.Storagenode.Suggested.Version = suggested
+
+	serverConfig := &versioncontrol.Config{
+		Address: "127.0.0.1:0",
+		Versions: versioncontrol.OldVersionConfig{
+			Satellite:   "v0.0.1",
+			Storagenode: "v0.0.1",
+			Uplink:      "v0.0.1",
+			Gateway:     "v0.0.1",
+			Identity:    "v0.0.1",
+		},
+		Binary: testVersions,
+	}
+
+	peer := newTestPeerWithConfig(t, ctx, serverConfig)
+	defer ctx.Check(peer.Close)
+
+	clientConfig := checker.ClientConfig{
+		ServerAddress:  "http://" + peer.Addr(),
+		RequestTimeout: 0,
+	}
+	config := checker.Config{
+		ClientConfig: clientConfig,
+	}
+
+	t.Run("CheckVersion", func(t *testing.T) {
+		type args struct {
+			name              string
+			version           string
+			errorMsg          string
+			isAcceptedVersion bool
+		}
+
+		tests := []args{
+			{
+				name:              "runs outdated version",
+				version:           "1.80.0",
+				errorMsg:          "outdated software version (v1.80.0), please update",
+				isAcceptedVersion: false,
+			},
+			{
+				name:              "runs minimum version",
+				version:           minimum,
+				isAcceptedVersion: true,
+			},
+			{
+				name:              "runs suggested version",
+				version:           suggested,
+				isAcceptedVersion: true,
+			},
+			{
+				name:              "runs version newer than minimum",
+				version:           "v1.90.2",
+				isAcceptedVersion: true,
+			},
+		}
+
+		for _, test := range tests {
+			t.Run(test.name, func(t *testing.T) {
+				ver, err := version.NewSemVer(test.version)
+				require.NoError(t, err)
+
+				versionInfo := version.Info{
+					Version: ver,
+					Release: true,
+				}
+
+				service := checker.NewService(zaptest.NewLogger(t), config, versionInfo, "storagenode")
+				latest, err := service.CheckVersion(ctx)
+				if test.errorMsg != "" {
+					require.Error(t, err)
+					require.Contains(t, err.Error(), test.errorMsg)
+				} else {
+					require.NoError(t, err)
+				}
+
+				require.Equal(t, suggested, latest.String())
+
+				minVersion, isAllowed := service.IsAllowed(ctx)
+				require.Equal(t, isAllowed, test.isAcceptedVersion)
+				require.Equal(t, minimum, minVersion.String())
+			})
+		}
+	})
+}