Helm 3: fix "latest" tag bug (#5279)

* add extra ref parsing, validation

Signed-off-by: Josh Dolitsky <jdolitsky@gmail.com>

* add fix for missing locator

Signed-off-by: Josh Dolitsky <jdolitsky@gmail.com>

* add repo and tag fields for clarity

Signed-off-by: Josh Dolitsky <jdolitsky@gmail.com>

* small refector

Signed-off-by: Josh Dolitsky <jdolitsky@gmail.com>

pkg/registry/cache.go

@@ -137,7 +137,7 @@ func (cache *filesystemCache) ChartToLayers(ch *chart.Chart) ([]ocispec.Descript
 }
 
 func (cache *filesystemCache) LoadReference(ref *Reference) ([]ocispec.Descriptor, error) {
-	tagDir := filepath.Join(cache.rootDir, "refs", escape(ref.Locator), "tags", tagOrDefault(ref.Object))
+	tagDir := filepath.Join(cache.rootDir, "refs", escape(ref.Repo), "tags", tagOrDefault(ref.Tag))
 
 	// add meta layer
 	metaJSONRaw, err := getSymlinkDestContent(filepath.Join(tagDir, "meta"))
@@ -165,8 +165,8 @@ func (cache *filesystemCache) LoadReference(ref *Reference) ([]ocispec.Descripto
 }
 
 func (cache *filesystemCache) StoreReference(ref *Reference, layers []ocispec.Descriptor) (bool, error) {
-	tag := tagOrDefault(ref.Object)
+	tag := tagOrDefault(ref.Tag)
-	tagDir := mkdir(filepath.Join(cache.rootDir, "refs", escape(ref.Locator), "tags", tag))
+	tagDir := mkdir(filepath.Join(cache.rootDir, "refs", escape(ref.Repo), "tags", tag))
 
 	// Retrieve just the meta and content layers
 	metaLayer, contentLayer, err := extractLayers(layers)
@@ -239,7 +239,7 @@ func (cache *filesystemCache) StoreReference(ref *Reference, layers []ocispec.De
 }
 
 func (cache *filesystemCache) DeleteReference(ref *Reference) error {
-	tagDir := filepath.Join(cache.rootDir, "refs", escape(ref.Locator), "tags", tagOrDefault(ref.Object))
+	tagDir := filepath.Join(cache.rootDir, "refs", escape(ref.Repo), "tags", tagOrDefault(ref.Tag))
 	if _, err := os.Stat(tagDir); os.IsNotExist(err) {
 		return errors.New("ref not found")
 	}
@@ -427,10 +427,10 @@ func getRefsSorted(refsRootDir string) ([][]interface{}, error) {
 				tagDir := filepath.Dir(path)
 
 				// Determine the ref
-				locator := unescape(strings.TrimLeft(
+				repo := unescape(strings.TrimLeft(
 					strings.TrimPrefix(filepath.Dir(filepath.Dir(tagDir)), refsRootDir), "/\\"))
-				object := filepath.Base(tagDir)
+				tag := filepath.Base(tagDir)
-				ref := fmt.Sprintf("%s:%s", locator, object)
+				ref := fmt.Sprintf("%s:%s", repo, tag)
 
 				// Init hashmap entry if does not exist
 				if _, ok := refsMap[ref]; !ok {

pkg/registry/client.go

@@ -60,7 +60,7 @@ func NewClient(options *ClientOptions) *Client {
 // PushChart uploads a chart to a registry
 func (c *Client) PushChart(ref *Reference) error {
 	c.setDefaultTag(ref)
-	fmt.Fprintf(c.out, "The push refers to repository [%s]\n", ref.Locator)
+	fmt.Fprintf(c.out, "The push refers to repository [%s]\n", ref.Repo)
 	layers, err := c.cache.LoadReference(ref)
 	if err != nil {
 		return err
@@ -74,14 +74,14 @@ func (c *Client) PushChart(ref *Reference) error {
 		totalSize += layer.Size
 	}
 	fmt.Fprintf(c.out,
-		"%s: pushed to remote (%d layers, %s total)\n", ref.Object, len(layers), byteCountBinary(totalSize))
+		"%s: pushed to remote (%d layers, %s total)\n", ref.Tag, len(layers), byteCountBinary(totalSize))
 	return nil
 }
 
 // PullChart downloads a chart from a registry
 func (c *Client) PullChart(ref *Reference) error {
 	c.setDefaultTag(ref)
-	fmt.Fprintf(c.out, "%s: Pulling from %s\n", ref.Object, ref.Locator)
+	fmt.Fprintf(c.out, "%s: Pulling from %s\n", ref.Tag, ref.Repo)
 	layers, err := oras.Pull(context.Background(), c.resolver, ref.String(), c.cache.store, KnownMediaTypes()...)
 	if err != nil {
 		return err
@@ -91,9 +91,9 @@ func (c *Client) PullChart(ref *Reference) error {
 		return err
 	}
 	if !exists {
-		fmt.Fprintf(c.out, "Status: Downloaded newer chart for %s:%s\n", ref.Locator, ref.Object)
+		fmt.Fprintf(c.out, "Status: Downloaded newer chart for %s:%s\n", ref.Repo, ref.Tag)
 	} else {
-		fmt.Fprintf(c.out, "Status: Chart is up to date for %s:%s\n", ref.Locator, ref.Object)
+		fmt.Fprintf(c.out, "Status: Chart is up to date for %s:%s\n", ref.Repo, ref.Tag)
 	}
 	return nil
 }
@@ -109,7 +109,7 @@ func (c *Client) SaveChart(ch *chart.Chart, ref *Reference) error {
 	if err != nil {
 		return err
 	}
-	fmt.Fprintf(c.out, "%s: saved\n", ref.Object)
+	fmt.Fprintf(c.out, "%s: saved\n", ref.Tag)
 	return nil
 }
 
@@ -131,7 +131,7 @@ func (c *Client) RemoveChart(ref *Reference) error {
 	if err != nil {
 		return err
 	}
-	fmt.Fprintf(c.out, "%s: removed\n", ref.Object)
+	fmt.Fprintf(c.out, "%s: removed\n", ref.Tag)
 	return err
 }
 
@@ -152,8 +152,8 @@ func (c *Client) PrintChartTable() error {
 }
 
 func (c *Client) setDefaultTag(ref *Reference) {
-	if ref.Object == "" {
+	if ref.Tag == "" {
-		ref.Object = HelmChartDefaultTag
+		ref.Tag = HelmChartDefaultTag
 		fmt.Fprintf(c.out, "Using default tag: %s\n", HelmChartDefaultTag)
 	}
 }

pkg/registry/reference.go

@@ -17,15 +17,25 @@ limitations under the License.
 package registry // import "k8s.io/helm/pkg/registry"
 
 import (
+	"errors"
+	"regexp"
 	"strings"
 
 	"github.com/containerd/containerd/reference"
 )
 
+var (
+	validPortRegEx     = regexp.MustCompile("^([1-9]\\d{0,3}|0|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])$") // adapted from https://stackoverflow.com/a/12968117
+	emptyRepoError     = errors.New("parsed repo was empty")
+	tooManyColonsError = errors.New("ref may only contain a single colon character (:) unless specifying a port number")
+)
+
 type (
 	// Reference defines the main components of a reference specification
 	Reference struct {
 		*reference.Spec
+		Tag  string
+		Repo string
 	}
 )
 
@@ -35,11 +45,90 @@ func ParseReference(s string) (*Reference, error) {
 	if err != nil {
 		return nil, err
 	}
-	ref := Reference{&spec}
+
+	// convert to our custom type and make necessary mods
+	ref := Reference{Spec: &spec}
+	ref.setExtraFields()
+
+	// ensure the reference is valid
+	err = ref.validate()
+	if err != nil {
+		return nil, err
+	}
+
 	return &ref, nil
 }
 
-// Repo returns a reference's repo minus the hostname
+// setExtraFields adds the Repo and Tag fields to a Reference
-func (ref *Reference) Repo() string {
+func (ref *Reference) setExtraFields() {
-	return strings.TrimPrefix(strings.TrimPrefix(ref.Locator, ref.Hostname()), "/")
+	ref.Tag = ref.Object
+	ref.Repo = ref.Locator
+	ref.fixNoTag()
+	ref.fixNoRepo()
+}
+
+// fixNoTag is a fix for ref strings such as "mychart:1.0.0", which result in missing tag
+func (ref *Reference) fixNoTag() {
+	if ref.Tag == "" {
+		parts := strings.Split(ref.Repo, ":")
+		numParts := len(parts)
+		if 0 < numParts {
+			lastIndex := numParts - 1
+			lastPart := parts[lastIndex]
+			if !strings.Contains(lastPart, "/") {
+				ref.Repo = strings.Join(parts[:lastIndex], ":")
+				ref.Tag = lastPart
+			}
+		}
+	}
+}
+
+// fixNoRepo is a fix for ref strings such as "mychart", which have the repo swapped with tag
+func (ref *Reference) fixNoRepo() {
+	if ref.Repo == "" {
+		ref.Repo = ref.Tag
+		ref.Tag = ""
+	}
+}
+
+// validate makes sure the ref meets our criteria
+func (ref *Reference) validate() error {
+	err := ref.validateRepo()
+	if err != nil {
+		return err
+	}
+	return ref.validateNumColons()
+}
+
+// validateRepo checks that the Repo field is non-empty
+func (ref *Reference) validateRepo() error {
+	if ref.Repo == "" {
+		return emptyRepoError
+	}
+	return nil
+}
+
+// validateNumColon ensures the ref only contains a single colon character (:)
+// (or potentially two, there might be a port number specified i.e. :5000)
+func (ref *Reference) validateNumColons() error {
+	if strings.Contains(ref.Tag, ":") {
+		return tooManyColonsError
+	}
+	parts := strings.Split(ref.Repo, ":")
+	lastIndex := len(parts) - 1
+	if 1 < lastIndex {
+		return tooManyColonsError
+	}
+	if 0 < lastIndex {
+		port := strings.Split(parts[lastIndex], "/")[0]
+		if !isValidPort(port) {
+			return tooManyColonsError
+		}
+	}
+	return nil
+}
+
+// isValidPort returns whether or not a string looks like a valid port
+func isValidPort(s string) bool {
+	return validPortRegEx.MatchString(s)
 }

pkg/registry/reference_test.go

@@ -25,25 +25,65 @@ import (
 func TestReference(t *testing.T) {
 	is := assert.New(t)
 
-	// bad ref
+	// bad refs
 	s := ""
 	_, err := ParseReference(s)
-	is.Error(err)
+	is.Error(err, "empty ref")
+
+	s = "my:bad:ref"
+	_, err = ParseReference(s)
+	is.Error(err, "ref contains too many colons (2)")
+
+	s = "my:really:bad:ref"
+	_, err = ParseReference(s)
+	is.Error(err, "ref contains too many colons (3)")
 
 	// good refs
-	s = "localhost:5000/mychart:latest"
+	s = "mychart"
 	ref, err := ParseReference(s)
 	is.NoError(err)
-	is.Equal("localhost:5000", ref.Hostname())
+	is.Equal("mychart", ref.Repo)
-	is.Equal("mychart", ref.Repo())
+	is.Equal("", ref.Tag)
-	is.Equal("localhost:5000/mychart", ref.Locator)
+
-	is.Equal("latest", ref.Object)
+	s = "mychart:1.5.0"
+	ref, err = ParseReference(s)
+	is.NoError(err)
+	is.Equal("mychart", ref.Repo)
+	is.Equal("1.5.0", ref.Tag)
+
+	s = "myrepo/mychart"
+	ref, err = ParseReference(s)
+	is.NoError(err)
+	is.Equal("myrepo/mychart", ref.Repo)
+	is.Equal("", ref.Tag)
+
+	s = "myrepo/mychart:1.5.0"
+	ref, err = ParseReference(s)
+	is.NoError(err)
+	is.Equal("myrepo/mychart", ref.Repo)
+	is.Equal("1.5.0", ref.Tag)
+
+	s = "mychart:5001:1.5.0"
+	ref, err = ParseReference(s)
+	is.NoError(err)
+	is.Equal("mychart:5001", ref.Repo)
+	is.Equal("1.5.0", ref.Tag)
+
+	s = "myrepo:5001/mychart:1.5.0"
+	ref, err = ParseReference(s)
+	is.NoError(err)
+	is.Equal("myrepo:5001/mychart", ref.Repo)
+	is.Equal("1.5.0", ref.Tag)
+
+	s = "localhost:5000/mychart:latest"
+	ref, err = ParseReference(s)
+	is.NoError(err)
+	is.Equal("localhost:5000/mychart", ref.Repo)
+	is.Equal("latest", ref.Tag)
 
 	s = "my.host.com/my/nested/repo:1.2.3"
 	ref, err = ParseReference(s)
 	is.NoError(err)
-	is.Equal("my.host.com", ref.Hostname())
+	is.Equal("my.host.com/my/nested/repo", ref.Repo)
-	is.Equal("my/nested/repo", ref.Repo())
+	is.Equal("1.2.3", ref.Tag)
-	is.Equal("my.host.com/my/nested/repo", ref.Locator)
-	is.Equal("1.2.3", ref.Object)
 }