versionsapi: canonicalize ref in version constructors (#3652)

* versionsapi: canonicalize ref in version constructors

Co-authored-by: 3u13r <lc@edgeless.systems>
Signed-off-by: Paul Meyer <katexochen0@gmail.com>

* hack/cli-k8s-compat: canonicalize ref

---------

Signed-off-by: Paul Meyer <katexochen0@gmail.com>
Co-authored-by: Paul Meyer <katexochen0@gmail.com>

hack/cli-k8s-compatibility/main.go

@@ -45,7 +45,7 @@ func main() {
 	}
 
 	cliInfo := versionsapi.CLIInfo{
-		Ref:        *refFlag,
+		Ref:        versionsapi.CanonicalizeRef(*refFlag),
 		Stream:     *streamFlag,
 		Version:    *versionFlag,
 		Kubernetes: []string{},

internal/api/versionsapi/cli/add.go

@@ -16,7 +16,6 @@ import (
 	"github.com/edgelesssys/constellation/v2/internal/api/versionsapi"
 	"github.com/edgelesssys/constellation/v2/internal/logger"
 	"github.com/spf13/cobra"
-	"golang.org/x/mod/semver"
 )
 
 func newAddCmd() *cobra.Command {
@@ -53,19 +52,8 @@ func runAdd(cmd *cobra.Command, _ []string) (retErr error) {
 		return err
 	}
 	log := logger.NewTextLogger(flags.logLevel)
-	log.Debug("Using flags", "dryRun", flags.dryRun, "kind", flags.kind, "latest", flags.latest, "ref", flags.ref,
+	log.Debug("Using flags", "dryRun", flags.dryRun, "kind", flags.version.Kind(), "latest", flags.latest, "ref", flags.version.Ref(),
-		"release", flags.release, "stream", flags.stream, "version", flags.version)
+		"stream", flags.version.Stream(), "version", flags.version.Version())
-
-	log.Debug("Validating flags")
-	if err := flags.validate(log); err != nil {
-		return err
-	}
-
-	log.Debug("Creating version struct")
-	ver, err := versionsapi.NewVersion(flags.ref, flags.stream, flags.version, flags.kind)
-	if err != nil {
-		return fmt.Errorf("creating version: %w", err)
-	}
 
 	log.Debug("Creating versions API client")
 	client, clientClose, err := versionsapi.NewClient(cmd.Context(), flags.region, flags.bucket, flags.distributionID, flags.dryRun, log)
@@ -80,27 +68,27 @@ func runAdd(cmd *cobra.Command, _ []string) (retErr error) {
 	}()
 
 	log.Info("Adding version")
-	if err := ensureVersion(cmd.Context(), client, flags.kind, ver, versionsapi.GranularityMajor, log); err != nil {
+	if err := ensureVersion(cmd.Context(), client, flags.version, versionsapi.GranularityMajor, log); err != nil {
 		return err
 	}
 
-	if err := ensureVersion(cmd.Context(), client, flags.kind, ver, versionsapi.GranularityMinor, log); err != nil {
+	if err := ensureVersion(cmd.Context(), client, flags.version, versionsapi.GranularityMinor, log); err != nil {
 		return err
 	}
 
 	if flags.latest {
-		if err := updateLatest(cmd.Context(), client, flags.kind, ver, log); err != nil {
+		if err := updateLatest(cmd.Context(), client, flags.version, log); err != nil {
 			return fmt.Errorf("setting latest version: %w", err)
 		}
 	}
 
-	log.Info(fmt.Sprintf("List major->minor URL: %s", ver.ListURL(versionsapi.GranularityMajor)))
+	log.Info(fmt.Sprintf("List major->minor URL: %s", flags.version.ListURL(versionsapi.GranularityMajor)))
-	log.Info(fmt.Sprintf("List minor->patch URL: %s", ver.ListURL(versionsapi.GranularityMinor)))
+	log.Info(fmt.Sprintf("List minor->patch URL: %s", flags.version.ListURL(versionsapi.GranularityMinor)))
 
 	return nil
 }
 
-func ensureVersion(ctx context.Context, client *versionsapi.Client, kind versionsapi.VersionKind, ver versionsapi.Version, gran versionsapi.Granularity,
+func ensureVersion(ctx context.Context, client *versionsapi.Client, ver versionsapi.Version, gran versionsapi.Granularity,
 	log *slog.Logger,
 ) error {
 	verListReq := versionsapi.List{
@@ -108,7 +96,7 @@ func ensureVersion(ctx context.Context, client *versionsapi.Client, kind version
 		Stream:      ver.Stream(),
 		Granularity: gran,
 		Base:        ver.WithGranularity(gran),
-		Kind:        kind,
+		Kind:        ver.Kind(),
 	}
 	verList, err := client.FetchVersionList(ctx, verListReq)
 	var notFoundErr *apiclient.NotFoundError
@@ -140,11 +128,11 @@ func ensureVersion(ctx context.Context, client *versionsapi.Client, kind version
 	return nil
 }
 
-func updateLatest(ctx context.Context, client *versionsapi.Client, kind versionsapi.VersionKind, ver versionsapi.Version, log *slog.Logger) error {
+func updateLatest(ctx context.Context, client *versionsapi.Client, ver versionsapi.Version, log *slog.Logger) error {
 	latest := versionsapi.Latest{
 		Ref:    ver.Ref(),
 		Stream: ver.Stream(),
-		Kind:   kind,
+		Kind:   ver.Kind(),
 	}
 	latest, err := client.FetchVersionLatest(ctx, latest)
 	var notFoundErr *apiclient.NotFoundError
@@ -164,7 +152,7 @@ func updateLatest(ctx context.Context, client *versionsapi.Client, kind versions
 		Ref:     ver.Ref(),
 		Stream:  ver.Stream(),
 		Version: ver.Version(),
-		Kind:    kind,
+		Kind:    ver.Kind(),
 	}
 	if err := client.UpdateVersionLatest(ctx, latest); err != nil {
 		return fmt.Errorf("updating latest version: %w", err)
@@ -174,60 +162,20 @@ func updateLatest(ctx context.Context, client *versionsapi.Client, kind versions
 }
 
 type addFlags struct {
-	version        string
+	version        versionsapi.Version
-	stream         string
-	ref            string
-	release        bool
 	latest         bool
 	dryRun         bool
 	region         string
 	bucket         string
 	distributionID string
-	kind           versionsapi.VersionKind
 	logLevel       slog.Level
 }
 
-func (f *addFlags) validate(log *slog.Logger) error {
-	if !semver.IsValid(f.version) {
-		return fmt.Errorf("version %q is not a valid semantic version", f.version)
-	}
-	if semver.Canonical(f.version) != f.version {
-		return fmt.Errorf("version %q is not a canonical semantic version", f.version)
-	}
-
-	if f.ref == "" && !f.release {
-		return fmt.Errorf("either --ref or --release must be set")
-	}
-
-	if f.kind == versionsapi.VersionKindUnknown {
-		return fmt.Errorf("unknown version kind %q", f.kind)
-	}
-
-	if f.release {
-		log.Debug(fmt.Sprintf("Setting ref to %q, as release flag is set", versionsapi.ReleaseRef))
-		f.ref = versionsapi.ReleaseRef
-	} else {
-		log.Debug("Setting latest to true, as release flag is not set")
-		f.latest = true // always set latest for non-release versions
-	}
-
-	if err := versionsapi.ValidateRef(f.ref); err != nil {
-		return fmt.Errorf("invalid ref %w", err)
-	}
-
-	if err := versionsapi.ValidateStream(f.ref, f.stream); err != nil {
-		return fmt.Errorf("invalid stream %w", err)
-	}
-
-	return nil
-}
-
 func parseAddFlags(cmd *cobra.Command) (addFlags, error) {
 	ref, err := cmd.Flags().GetString("ref")
 	if err != nil {
 		return addFlags{}, err
 	}
-	ref = versionsapi.CanonicalizeRef(ref)
 	stream, err := cmd.Flags().GetString("stream")
 	if err != nil {
 		return addFlags{}, err
@@ -274,17 +222,24 @@ func parseAddFlags(cmd *cobra.Command) (addFlags, error) {
 		return addFlags{}, err
 	}
 
+	if release {
+		ref = versionsapi.ReleaseRef
+	} else {
+		latest = true // always set latest for non-release versions
+	}
+
+	ver, err := versionsapi.NewVersion(ref, stream, version, kind)
+	if err != nil {
+		return addFlags{}, fmt.Errorf("creating version: %w", err)
+	}
+
 	return addFlags{
-		version:        version,
+		version:        ver,
-		stream:         stream,
-		ref:            versionsapi.CanonicalizeRef(ref),
-		release:        release,
 		latest:         latest,
 		dryRun:         dryRun,
 		region:         region,
 		bucket:         bucket,
 		distributionID: distributionID,
 		logLevel:       logLevel,
-		kind:           kind,
 	}, nil
 }

internal/api/versionsapi/version.go

@@ -41,7 +41,7 @@ type Version struct {
 // NewVersion creates a new Version object and validates it.
 func NewVersion(ref, stream, version string, kind VersionKind) (Version, error) {
 	ver := Version{
-		ref:     ref,
+		ref:     CanonicalizeRef(ref),
 		stream:  stream,
 		version: version,
 		kind:    kind,
@@ -62,7 +62,7 @@ func NewVersionFromShortPath(shortPath string, kind VersionKind) (Version, error
 	}
 
 	ver := Version{
-		ref:     ref,
+		ref:     ref, // Canonicalized by parseShortPath.
 		stream:  stream,
 		version: version,
 		kind:    kind,
@@ -331,7 +331,7 @@ func CanonicalizeRef(ref string) string {
 	canRef := notAZ09Regexp.ReplaceAllString(ref, "-")
 
 	if canRef == ReleaseRef {
-		return "" // No ref should be cannonicalized to the release ref.
+		return "" // No ref should be canonicalized to the release ref.
 	}
 
 	return canRef
@@ -401,7 +401,7 @@ func MeasurementURL(version Version) (measurementURL, signatureURL *url.URL, err
 }
 
 var (
-	shortPathRegex        = regexp.MustCompile(`^ref/([a-zA-Z0-9-]+)/stream/([a-zA-Z0-9-]+)/([a-zA-Z0-9.-]+)$`)
+	shortPathRegex        = regexp.MustCompile(`^ref/([^/]+)/stream/([a-zA-Z0-9-]+)/([a-zA-Z0-9.-]+)$`)
 	shortPathReleaseRegex = regexp.MustCompile(`^stream/([a-zA-Z0-9-]+)/([a-zA-Z0-9.-]+)$`)
 )
 
@@ -422,6 +422,7 @@ func parseShortPath(shortPath string) (ref, stream, version string, err error) {
 	if shortPathRegex.MatchString(shortPath) {
 		matches := shortPathRegex.FindStringSubmatch(shortPath)
 		ref := matches[1]
+		ref = CanonicalizeRef(ref)
 		if err := ValidateRef(ref); err != nil {
 			return "", "", "", err
 		}

internal/api/versionsapi/version_test.go

@@ -16,6 +16,111 @@ import (
 	"github.com/edgelesssys/constellation/v2/internal/constants"
 )
 
+func TestNewVersion(t *testing.T) {
+	testCases := map[string]struct {
+		ref     string
+		stream  string
+		version string
+		kind    VersionKind
+		wantVer Version
+		wantErr bool
+	}{
+		"stable release image": {
+			ref:     ReleaseRef,
+			stream:  "stable",
+			version: "v9.9.9",
+			kind:    VersionKindImage,
+			wantVer: Version{
+				ref:     ReleaseRef,
+				stream:  "stable",
+				version: "v9.9.9",
+				kind:    VersionKindImage,
+			},
+		},
+		"release debug image": {
+			ref:     ReleaseRef,
+			stream:  "debug",
+			version: "v9.9.9",
+			kind:    VersionKindImage,
+			wantVer: Version{
+				ref:     ReleaseRef,
+				stream:  "debug",
+				version: "v9.9.9",
+				kind:    VersionKindImage,
+			},
+		},
+		"stable release cli": {
+			ref:     ReleaseRef,
+			stream:  "stable",
+			version: "v9.9.9",
+			kind:    VersionKindCLI,
+			wantVer: Version{
+				ref:     ReleaseRef,
+				stream:  "stable",
+				version: "v9.9.9",
+				kind:    VersionKindCLI,
+			},
+		},
+		"release debug cli": {
+			ref:     ReleaseRef,
+			stream:  "debug",
+			version: "v9.9.9",
+			kind:    VersionKindCLI,
+			wantVer: Version{
+				ref:     ReleaseRef,
+				stream:  "debug",
+				version: "v9.9.9",
+				kind:    VersionKindCLI,
+			},
+		},
+		"unknown kind": {
+			ref:     ReleaseRef,
+			stream:  "debug",
+			version: "v9.9.9",
+			kind:    VersionKindUnknown,
+			wantErr: true,
+		},
+		"non-release ref as input": {
+			ref:     "working-branch",
+			stream:  "debug",
+			version: "v9.9.9",
+			kind:    VersionKindImage,
+			wantVer: Version{
+				ref:     "working-branch",
+				stream:  "debug",
+				version: "v9.9.9",
+				kind:    VersionKindImage,
+			},
+		},
+		"non-canonical ref as input": {
+			ref:     "testing-1.23",
+			stream:  "debug",
+			version: "v9.9.9",
+			kind:    VersionKindImage,
+			wantVer: Version{
+				ref:     "testing-1-23",
+				stream:  "debug",
+				version: "v9.9.9",
+				kind:    VersionKindImage,
+			},
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			assert := assert.New(t)
+
+			ver, err := NewVersion(tc.ref, tc.stream, tc.version, tc.kind)
+			if tc.wantErr {
+				assert.Error(err)
+				return
+			}
+			assert.NoError(err)
+			assert.Equal(tc.wantVer, ver)
+		})
+	}
+}
+
 func TestNewVersionFromShortPath(t *testing.T) {
 	testCases := map[string]struct {
 		path    string
@@ -78,6 +183,26 @@ func TestNewVersionFromShortPath(t *testing.T) {
 			kind:    VersionKindCLI,
 			wantErr: true,
 		},
+		"non-release ref as input": {
+			path: "ref/working-branch/stream/debug/v9.9.9",
+			kind: VersionKindImage,
+			wantVer: Version{
+				ref:     "working-branch",
+				stream:  "debug",
+				version: "v9.9.9",
+				kind:    VersionKindImage,
+			},
+		},
+		"non-canonical ref as input": {
+			path: "ref/testing-1.23/stream/debug/v9.9.9",
+			kind: VersionKindImage,
+			wantVer: Version{
+				ref:     "testing-1-23",
+				stream:  "debug",
+				version: "v9.9.9",
+				kind:    VersionKindImage,
+			},
+		},
 	}
 
 	for name, tc := range testCases {