add v prefix (#2890)

* add v prefix Signed-off-by: Ludvig Liljenberg <lliljenberg@microsoft.com> --------- Signed-off-by: Ludvig Liljenberg <lliljenberg@microsoft.com>
getporter · Aug 31, 2023 · a8fa983 · a8fa983
1 parent 8fbff53
commit a8fa983
Show file tree

Hide file tree

Showing 2 changed files with 179 additions and 0 deletions.
diff --git a/pkg/porter/lifecycle.go b/pkg/porter/lifecycle.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	"get.porter.sh/porter/pkg/cache"
 	"get.porter.sh/porter/pkg/cnab"
@@ -196,6 +197,12 @@ func (p *Porter) resolveBundleReference(ctx context.Context, opts *BundleReferen
 	useReference := func(ref cnab.OCIReference) error {
 		pullOpts := *opts // make a copy just to do the pull
 		pullOpts.Reference = ref.String()
+
+		err := ensureVPrefix(&pullOpts)
+		if err != nil {
+			return err
+		}
+
 		cachedBundle, err := p.prepullBundleByReference(ctx, &pullOpts)
 		if err != nil {
 			return err
@@ -298,6 +305,40 @@ func (p *Porter) BuildActionArgs(ctx context.Context, installation storage.Insta
 	return args, nil
 }
 
+// ensureVPrefix adds a "v" prefix to the version tag if it's not already there.
+// Version tag should always be prefixed with a "v", see https://github.com/getporter/porter/issues/2886.
+// This is safe because "porter publish" adds a "v", see
+// https://github.com/getporter/porter/blob/17bd7816ef6bde856793f6122e32274aa9d01d1b/pkg/storage/installation.go#L350
+func ensureVPrefix(opts *BundleReferenceOptions) error {
+	var ociRef *cnab.OCIReference
+	if opts._ref != nil {
+		ociRef = opts._ref
+	} else {
+		ref, err := cnab.ParseOCIReference(opts.Reference)
+		if err != nil {
+			return fmt.Errorf("unable to parse OCI reference from '%s': %w", opts.Reference, err)
+		}
+		ociRef = &ref
+	}
+
+	if strings.HasPrefix(ociRef.Tag(), "v") {
+		// don't do anything if "v" is already there
+		return nil
+	}
+
+	vRef, err := ociRef.WithTag("v" + ociRef.Tag())
+	if err != nil {
+		return fmt.Errorf("unable to prefix reference tag '%s' with 'v': %w", ociRef.Tag(), err)
+	}
+
+	// always update the .Reference string, but don't add the _ref field unless it was already there (non-nil)
+	opts.Reference = vRef.String()
+	if opts._ref != nil {
+		opts._ref = &vRef
+	}
+	return nil
+}
+
 // prepullBundleByReference handles calling the bundle pull operation and updating
 // the shared options like name and bundle file path. This is used by install, upgrade
 // and uninstall

diff --git a/pkg/porter/lifecycle_test.go b/pkg/porter/lifecycle_test.go
@@ -2,6 +2,8 @@ package porter
 
 import (
 	"context"
+	"fmt"
+	"strings"
 	"testing"
 
 	"get.porter.sh/porter/pkg"
@@ -530,3 +532,139 @@ func TestPorter_applyActionOptionsToInstallation_PreservesExistingParams(t *test
 		"my-second-param": "2", // Should have kept the existing value from the last run
 	}, params, "Incorrect parameter values were persisted on the installationß")
 }
+
+// make sure ensureVPrefix correctly adds a 'v' to the version tag of a reference
+func Test_ensureVPrefix(t *testing.T) {
+	type args struct {
+		opts *BundleReferenceOptions
+	}
+
+	ref, err := cnab.ParseOCIReference("registry/bundle:1.2.3")
+	assert.NoError(t, err)
+
+	testCases := []struct {
+		name    string
+		args    args
+		wantErr assert.ErrorAssertionFunc
+	}{
+		{
+			name: "add v to Reference (nil _ref)",
+			args: struct{ opts *BundleReferenceOptions }{opts: &BundleReferenceOptions{
+				installationOptions: installationOptions{},
+				BundlePullOptions: BundlePullOptions{
+					Reference:        "registry/bundle:1.2.3",
+					_ref:             nil,
+					InsecureRegistry: false,
+					Force:            false,
+				},
+				bundleRef: nil,
+			},
+			}, wantErr: assert.NoError,
+		},
+		{
+			name: "add v to Reference (non-nil _ref)",
+			args: struct{ opts *BundleReferenceOptions }{opts: &BundleReferenceOptions{
+				installationOptions: installationOptions{},
+				BundlePullOptions: BundlePullOptions{
+					Reference:        "registry/bundle:1.2.3",
+					_ref:             &ref,
+					InsecureRegistry: false,
+					Force:            false,
+				},
+				bundleRef: nil,
+			},
+			}, wantErr: assert.NoError,
+		},
+	}
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			// before
+			idx := strings.LastIndex(tt.args.opts.Reference, ":")
+			assert.False(t, tt.args.opts.Reference[idx+1] == 'v')
+			if tt.args.opts._ref != nil {
+				assert.False(t, tt.args.opts._ref.Tag()[0] == 'v')
+			}
+
+			err := ensureVPrefix(tt.args.opts)
+
+			// after
+			tt.wantErr(t, err, fmt.Sprintf("ensureVPrefix(%v)", tt.args.opts))
+
+			idx = strings.LastIndex(tt.args.opts.Reference, ":")
+			assert.True(t, tt.args.opts.Reference[idx+1] == 'v')
+			if tt.args.opts._ref != nil {
+				assert.True(t, tt.args.opts._ref.Tag()[0] == 'v')
+			}
+		})
+	}
+}
+
+// make sure ensureVPrefix doesn't add an extra 'v' to the version tag of a reference if it's already there
+func Test_ensureVPrefix_idempotent(t *testing.T) {
+	type args struct {
+		opts *BundleReferenceOptions
+	}
+
+	vRef, err := cnab.ParseOCIReference("registry/bundle:v1.2.3")
+	assert.NoError(t, err)
+
+	testcasesIdempotent := []struct {
+		name    string
+		args    args
+		wantErr assert.ErrorAssertionFunc
+	}{
+		{
+			name: "don't add v to Reference with existing v (nil _ref)",
+			args: struct{ opts *BundleReferenceOptions }{opts: &BundleReferenceOptions{
+				installationOptions: installationOptions{},
+				BundlePullOptions: BundlePullOptions{
+					Reference:        "registry/bundle:v1.2.3",
+					_ref:             nil,
+					InsecureRegistry: false,
+					Force:            false,
+				},
+				bundleRef: nil,
+			},
+			}, wantErr: assert.NoError,
+		},
+		{
+			name: "don't add v to Reference with existing v (non-nil _ref)",
+			args: struct{ opts *BundleReferenceOptions }{opts: &BundleReferenceOptions{
+				installationOptions: installationOptions{},
+				BundlePullOptions: BundlePullOptions{
+					Reference:        "registry/bundle:v1.2.3",
+					_ref:             &vRef,
+					InsecureRegistry: false,
+					Force:            false,
+				},
+				bundleRef: nil,
+			},
+			}, wantErr: assert.NoError,
+		},
+	}
+	for _, tt := range testcasesIdempotent {
+		t.Run(tt.name, func(t *testing.T) {
+			// before
+			idx := strings.LastIndex(tt.args.opts.Reference, ":")
+			assert.True(t, tt.args.opts.Reference[idx+1] == 'v')
+			assert.True(t, tt.args.opts.Reference[idx+2] != 'v')
+			if tt.args.opts._ref != nil {
+				assert.True(t, tt.args.opts._ref.Tag()[0] == 'v')
+				assert.True(t, tt.args.opts._ref.Tag()[1] != 'v')
+			}
+
+			err := ensureVPrefix(tt.args.opts)
+
+			// after
+			tt.wantErr(t, err, fmt.Sprintf("ensureVPrefix(%v)", tt.args.opts))
+
+			idx = strings.LastIndex(tt.args.opts.Reference, ":")
+			assert.True(t, tt.args.opts.Reference[idx+1] == 'v')
+			assert.True(t, tt.args.opts.Reference[idx+2] != 'v')
+			if tt.args.opts._ref != nil {
+				assert.True(t, tt.args.opts._ref.Tag()[0] == 'v')
+				assert.True(t, tt.args.opts._ref.Tag()[1] != 'v')
+			}
+		})
+	}
+}