Repository: helm/helm (go)

Issue:
Dependencies: Resolve version when only URL is known
Currently, when is resolving dependencies and a dependency is specified as a URL but it has not been added to Helm via `helm repo add` the dependency is locked to the exact value specified for the requested dependency. If the specified value is a range (e.g., `^1.2.3) the range is the version it is locked to.

The current situation creates a situation where locking isn't really locked to a version.

With more distributed repos that cross link each other this problem has the potential to get worse.

Instead of locking to the range, Helm should work out the version and lock to the version.

I would suggest the steps:

1. If not cached locally via `helm repo add`, create an automated and not-visible to the user cache.
2. Resolve the version from this invisible cache
3. Lock to the version
4. Clean up any invisible caches (periodically?)

Pull request:
Lockfiles without version ranges
There were two situations where a lockfile could end up with a version range as a version instead of a pinned version.

1. When `file://` was used. This would point to the file location on the filesystem but, instead of using the version in that chart at that file location the version range would be used.
2. If a repository was used that was not added with `helm repo add`. Instead of causing an error that the repository was not available the version range was used in the lockfile. This is not locking.

This change looks to correct these problems. The `file://` handling fix was fairly straight forward. In that case update gets the current version of the dependency on disk and locks to that.

Handling unknown repos went through 4 different major versions. This 4th one is the one that settled on because all the changes are in the download manager which handles network issues. Other cases had added network operations to the resolver which is meant to iterate over existing versions. I chose this as the network and downloading stay in the same struct and it's methods are largely reusable within the new logic.

Closes #8449 

**If applicable**:
- [ ] this PR contains documentation
- [x] this PR contains unit tests
- [ ] this PR has been tested for backwards compatibility


Patch:
diff --git a/internal/resolver/resolver.go b/internal/resolver/resolver.go
index cec29c947..c72a39e82 100644
--- a/internal/resolver/resolver.go
+++ b/internal/resolver/resolver.go
@@ -27,6 +27,7 @@ import (
 	"github.com/pkg/errors"
 
 	"helm.sh/helm/v3/pkg/chart"
+	"helm.sh/helm/v3/pkg/chart/loader"
 	"helm.sh/helm/v3/pkg/helmpath"
 	"helm.sh/helm/v3/pkg/provenance"
 	"helm.sh/helm/v3/pkg/repo"
@@ -68,14 +69,22 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string
 		}
 		if strings.HasPrefix(d.Repository, "file://") {
 
-			if _, err := GetLocalPath(d.Repository, r.chartpath); err != nil {
+			chartpath, err := GetLocalPath(d.Repository, r.chartpath)
+			if err != nil {
+				return nil, err
+			}
+
+			// The version of the chart locked will be the version of the chart
+			// currently listed in the file system within the chart.
+			ch, err := loader.LoadDir(chartpath)
+			if err != nil {
 				return nil, err
 			}
 
 			locked[i] = &chart.Dependency{
 				Name:       d.Name,
 				Repository: d.Repository,
-				Version:    d.Version,
+				Version:    ch.Metadata.Version,
 			}
 			continue
 		}
diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go
index 2d6918739..bcd5dcec4 100644
--- a/pkg/downloader/manager.go
+++ b/pkg/downloader/manager.go
@@ -16,6 +16,8 @@ limitations under the License.
 package downloader
 
 import (
+	"crypto"
+	"encoding/hex"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -158,14 +160,27 @@ func (m *Manager) Update() error {
 		return nil
 	}
 
-	// Check that all of the repos we're dependent on actually exist and
-	// the repo index names.
+	// Get the names of the repositories the dependencies need that Helm is
+	// configured to know about.
 	repoNames, err := m.resolveRepoNames(req)
 	if err != nil {
 		return err
 	}
 
-	// For each repo in the file, update the cached copy of that repo
+	// For the repositories Helm is not configured to know about, ensure Helm
+	// has some information about them and, when possible, the index files
+	// locally.
+	// TODO(mattfarina): Repositories should be explicitly added by end users
+	// rather than automattic. In Helm v4 require users to add repositories. They
+	// should have to add them in order to make sure they are aware of the
+	// respoitories and opt-in to any locations, for security.
+	repoNames, err = m.ensureMissingRepos(repoNames, req)
+	if err != nil {
+		return err
+	}
+
+	// For each of the repositories Helm is configured to know about, update
+	// the index information locally.
 	if !m.SkipUpdate {
 		if err := m.UpdateRepositories(); err != nil {
 			return err
@@ -427,6 +442,62 @@ Loop:
 	return nil
 }
 
+// ensureMissingRepos attempts to ensure the repository information for repos
+// not managed by Helm is present. This takes in the repoNames Helm is configured
+// to work with along with the chart dependencies. It will find the deps not
+// in a known repo and attempt to ensure the data is present for steps like
+// version resolution.
+func (m *Manager) ensureMissingRepos(repoNames map[string]string, deps []*chart.Dependency) (map[string]string, error) {
+
+	var ru []*repo.Entry
+
+	for _, dd := range deps {
+
+		// When the repoName for a dependency is known we can skip ensuring
+		if _, ok := repoNames[dd.Name]; ok {
+			continue
+		}
+
+		// The generated repository name, which will result in an index being
+		// locally cached, has a name pattern of "helm-manager-" followed by a
+		// sha256 of the repo name. This assumes end users will never create
+		// repositories with these names pointing to other repositories. Using
+		// this method of naming allows the existing repository pulling and
+		// resolution code to do most of the work.
+		rn, err := key(dd.Repository)
+		if err != nil {
+			return repoNames, err
+		}
+		rn = managerKeyPrefix + rn
+
+		repoNames[dd.Name] = rn
+
+		// Assuming the repository is generally available. For Helm managed
+		// access controls the repository needs to be added through the user
+		// managed system. This path will work for public charts, like those
+		// supplied by Bitnami, but not for protected charts, like corp ones
+		// behind a username and pass.
+		ri := &repo.Entry{
+			Name: rn,
+			URL:  dd.Repository,
+		}
+		ru = append(ru, ri)
+	}
+
+	// Calls to UpdateRepositories (a public function) will only update
+	// repositories configured by the user. Here we update repos found in
+	// the dependencies that are not known to the user if update skipping
+	// is not configured.
+	if !m.SkipUpdate && len(ru) > 0 {
+		fmt.Fprintln(m.Out, "Getting updates for unmanaged Helm repositories...")
+		if err := m.parallelRepoUpdate(ru); err != nil {
+			return repoNames, err
+		}
+	}
+
+	return repoNames, nil
+}
+
 // resolveRepoNames returns the repo names of the referenced deps which can be used to fetch the cached index file
 // and replaces aliased repository URLs into resolved URLs in dependencies.
 func (m *Manager) resolveRepoNames(deps []*chart.Dependency) (map[string]string, error) {
@@ -517,16 +588,18 @@ func (m *Manager) UpdateRepositories() error {
 	}
 	repos := rf.Repositories
 	if len(repos) > 0 {
+		fmt.Fprintln(m.Out, "Hang tight while we grab the latest from your chart repositories...")
 		// This prints warnings straight to out.
 		if err := m.parallelRepoUpdate(repos); err != nil {
 			return err
 		}
+		fmt.Fprintln(m.Out, "Update Complete. ⎈Happy Helming!⎈")
 	}
 	return nil
 }
 
 func (m *Manager) parallelRepoUpdate(repos []*repo.Entry) error {
-	fmt.Fprintln(m.Out, "Hang tight while we grab the latest from your chart repositories...")
+
 	var wg sync.WaitGroup
 	for _, c := range repos {
 		r, err := repo.NewChartRepository(c, m.Getters)
@@ -536,15 +609,27 @@ func (m *Manager) parallelRepoUpdate(repos []*repo.Entry) error {
 		wg.Add(1)
 		go func(r *repo.ChartRepository) {
 			if _, err := r.DownloadIndexFile(); err != nil {
-				fmt.Fprintf(m.Out, "...Unable to get an update from the %q chart repository (%s):\n\t%s\n", r.Config.Name, r.Config.URL, err)
+				// For those dependencies that are not known to helm and using a
+				// generated key name we display the repo url.
+				if strings.HasPrefix(r.Config.Name, managerKeyPrefix) {
+					fmt.Fprintf(m.Out, "...Unable to get an update from the %q chart repository:\n\t%s\n", r.Config.URL, err)
+				} else {
+					fmt.Fprintf(m.Out, "...Unable to get an update from the %q chart repository (%s):\n\t%s\n", r.Config.Name, r.Config.URL, err)
+				}
 			} else {
-				fmt.Fprintf(m.Out, "...Successfully got an update from the %q chart repository\n", r.Config.Name)
+				// For those dependencies that are not known to helm and using a
+				// generated key name we display the repo url.
+				if strings.HasPrefix(r.Config.Name, managerKeyPrefix) {
+					fmt.Fprintf(m.Out, "...Successfully got an update from the %q chart repository\n", r.Config.URL)
+				} else {
+					fmt.Fprintf(m.Out, "...Successfully got an update from the %q chart repository\n", r.Config.Name)
+				}
 			}
 			wg.Done()
 		}(r)
 	}
 	wg.Wait()
-	fmt.Fprintln(m.Out, "Update Complete. ⎈Happy Helming!⎈")
+
 	return nil
 }
 
@@ -739,3 +824,18 @@ func move(tmpPath, destPath string) error {
 	}
 	return nil
 }
+
+// The prefix to use for cache keys created by the manager for repo names
+const managerKeyPrefix = "helm-manager-"
+
+// key is used to turn a name, such as a repository url, into a filesystem
+// safe name that is unique for querying. To accomplish this a unique hash of
+// the string is used.
+func key(name string) (string, error) {
+	in := strings.NewReader(name)
+	hash := crypto.SHA256.New()
+	if _, err := io.Copy(hash, in); err != nil {
+		return "", nil
+	}
+	return hex.EncodeToString(hash.Sum(nil)), nil
+}


Test patch:
diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go
index 0b0c3a6f8..f59188508 100644
--- a/internal/resolver/resolver_test.go
+++ b/internal/resolver/resolver_test.go
@@ -82,6 +82,17 @@ func TestResolve(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "repo from valid local path with range resolution",
+			req: []*chart.Dependency{
+				{Name: "base", Repository: "file://base", Version: "^0.1.0"},
+			},
+			expect: &chart.Lock{
+				Dependencies: []*chart.Dependency{
+					{Name: "base", Repository: "file://base", Version: "0.1.0"},
+				},
+			},
+		},
 		{
 			name: "repo from invalid local path",
 			req: []*chart.Dependency{
diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go
index dd3cbbd7b..e60cf7624 100644
--- a/pkg/downloader/manager_test.go
+++ b/pkg/downloader/manager_test.go
@@ -389,3 +389,33 @@ func TestErrRepoNotFound_Error(t *testing.T) {
 		})
 	}
 }
+
+func TestKey(t *testing.T) {
+	tests := []struct {
+		name   string
+		expect string
+	}{
+		{
+			name:   "file:////tmp",
+			expect: "afeed3459e92a874f6373aca264ce1459bfa91f9c1d6612f10ae3dc2ee955df3",
+		},
+		{
+			name:   "https://example.com/charts",
+			expect: "7065c57c94b2411ad774638d76823c7ccb56415441f5ab2f5ece2f3845728e5d",
+		},
+		{
+			name:   "foo/bar/baz",
+			expect: "15c46a4f8a189ae22f36f201048881d6c090c93583bedcf71f5443fdef224c82",
+		},
+	}
+
+	for _, tt := range tests {
+		o, err := key(tt.name)
+		if err != nil {
+			t.Fatalf("unable to generate key for %q with error: %s", tt.name, err)
+		}
+		if o != tt.expect {
+			t.Errorf("wrong key name generated for %q, expected %q but got %q", tt.name, tt.expect, o)
+		}
+	}
+}