Repository: helm/helm (go)

Issue:
`index.yaml` are not written atomically
`index.yaml` files, as generated by `helm repo index`, are not written atomically. As a result files can be half-written when helm or another client reads them.

This is an example of a custom YAML scanner breaking because of a half-written file:
```
/<redacted>/path/to/scanner.py", line 319, in scan_index
"could not find expected ':'", self.get_mark())
yaml.scanner.ScannerError: while scanning a simple key
in "<unicode string>", line 3497, column 5:
appVersi
^
could not find expected ':'
in "<unicode string>", line 3497, column 13:
appVersi
^
```


Pull request:
fix: write index.yaml file atomically
This refactors the already-existing `AtomicWriteFile` utility
to a central location and uses it to write index files
atomically.
This is done to avoid having half-written index files break
client requests.

Drive-bys:
  - Add test for AtomicWriteFile.
  - Add test IndexFile.WriteFile.

Closes #7988 

Patch:
diff --git a/cmd/helm/helm.go b/cmd/helm/helm.go
index 7e1fcb6e1..fcc7315f5 100644
--- a/cmd/helm/helm.go
+++ b/cmd/helm/helm.go
@@ -43,9 +43,7 @@ import (
 // FeatureGateOCI is the feature gate for checking if `helm chart` and `helm registry` commands should work
 const FeatureGateOCI = gates.Gate("HELM_EXPERIMENTAL_OCI")
 
-var (
-	settings = cli.New()
-)
+var settings = cli.New()
 
 func init() {
 	log.SetFlags(log.Lshortfile)
@@ -72,13 +70,16 @@ func main() {
 	actionConfig := new(action.Configuration)
 	cmd := newRootCmd(actionConfig, os.Stdout, os.Args[1:])
 
-	helmDriver := os.Getenv("HELM_DRIVER")
-	if err := actionConfig.Init(settings.RESTClientGetter(), settings.Namespace(), helmDriver, debug); err != nil {
-		log.Fatal(err)
-	}
-	if helmDriver == "memory" {
-		loadReleasesInMemory(actionConfig)
-	}
+	// run when each command's execute method is called
+	cobra.OnInitialize(func() {
+		helmDriver := os.Getenv("HELM_DRIVER")
+		if err := actionConfig.Init(settings.RESTClientGetter(), settings.Namespace(), helmDriver, debug); err != nil {
+			log.Fatal(err)
+		}
+		if helmDriver == "memory" {
+			loadReleasesInMemory(actionConfig)
+		}
+	})
 
 	if err := cmd.Execute(); err != nil {
 		debug("%+v", err)
diff --git a/internal/fileutil/fileutil.go b/internal/fileutil/fileutil.go
new file mode 100644
index 000000000..739093f3b
--- /dev/null
+++ b/internal/fileutil/fileutil.go
@@ -0,0 +1,51 @@
+/*
+Copyright The Helm Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package fileutil
+
+import (
+	"io"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
+	"helm.sh/helm/v3/internal/third_party/dep/fs"
+)
+
+// AtomicWriteFile atomically (as atomic as os.Rename allows) writes a file to a
+// disk.
+func AtomicWriteFile(filename string, reader io.Reader, mode os.FileMode) error {
+	tempFile, err := ioutil.TempFile(filepath.Split(filename))
+	if err != nil {
+		return err
+	}
+	tempName := tempFile.Name()
+
+	if _, err := io.Copy(tempFile, reader); err != nil {
+		tempFile.Close() // return value is ignored as we are already on error path
+		return err
+	}
+
+	if err := tempFile.Close(); err != nil {
+		return err
+	}
+
+	if err := os.Chmod(tempName, mode); err != nil {
+		return err
+	}
+
+	return fs.RenameWithFallback(tempName, filename)
+}
diff --git a/pkg/cli/environment.go b/pkg/cli/environment.go
index e279331b0..2e64e0810 100644
--- a/pkg/cli/environment.go
+++ b/pkg/cli/environment.go
@@ -26,21 +26,17 @@ import (
 	"fmt"
 	"os"
 	"strconv"
-	"sync"
 
 	"github.com/spf13/pflag"
-
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 
 	"helm.sh/helm/v3/pkg/helmpath"
-	"helm.sh/helm/v3/pkg/kube"
 )
 
 // EnvSettings describes all of the environment settings.
 type EnvSettings struct {
-	namespace  string
-	config     genericclioptions.RESTClientGetter
-	configOnce sync.Once
+	namespace string
+	config    *genericclioptions.ConfigFlags
 
 	// KubeConfig is the path to the kubeconfig file
 	KubeConfig string
@@ -63,8 +59,7 @@ type EnvSettings struct {
 }
 
 func New() *EnvSettings {
-
-	env := EnvSettings{
+	env := &EnvSettings{
 		namespace:        os.Getenv("HELM_NAMESPACE"),
 		KubeContext:      os.Getenv("HELM_KUBECONTEXT"),
 		KubeToken:        os.Getenv("HELM_KUBETOKEN"),
@@ -75,7 +70,16 @@ func New() *EnvSettings {
 		RepositoryCache:  envOr("HELM_REPOSITORY_CACHE", helmpath.CachePath("repository")),
 	}
 	env.Debug, _ = strconv.ParseBool(os.Getenv("HELM_DEBUG"))
-	return &env
+
+	// bind to kubernetes config flags
+	env.config = &genericclioptions.ConfigFlags{
+		Namespace:   &env.namespace,
+		Context:     &env.KubeContext,
+		BearerToken: &env.KubeToken,
+		APIServer:   &env.KubeAPIServer,
+		KubeConfig:  &env.KubeConfig,
+	}
+	return env
 }
 
 // AddFlags binds flags to the given flagset.
@@ -107,42 +111,27 @@ func (s *EnvSettings) EnvVars() map[string]string {
 		"HELM_REPOSITORY_CACHE":  s.RepositoryCache,
 		"HELM_REPOSITORY_CONFIG": s.RepositoryConfig,
 		"HELM_NAMESPACE":         s.Namespace(),
-		"HELM_KUBECONTEXT":       s.KubeContext,
-		"HELM_KUBETOKEN":         s.KubeToken,
-		"HELM_KUBEAPISERVER":     s.KubeAPIServer,
-	}
 
+		// broken, these are populated from helm flags and not kubeconfig.
+		"HELM_KUBECONTEXT":   s.KubeContext,
+		"HELM_KUBETOKEN":     s.KubeToken,
+		"HELM_KUBEAPISERVER": s.KubeAPIServer,
+	}
 	if s.KubeConfig != "" {
 		envvars["KUBECONFIG"] = s.KubeConfig
 	}
-
 	return envvars
 }
 
-//Namespace gets the namespace from the configuration
+// Namespace gets the namespace from the configuration
 func (s *EnvSettings) Namespace() string {
-	if s.namespace != "" {
-		return s.namespace
-	}
-
-	if ns, _, err := s.RESTClientGetter().ToRawKubeConfigLoader().Namespace(); err == nil {
+	if ns, _, err := s.config.ToRawKubeConfigLoader().Namespace(); err == nil {
 		return ns
 	}
 	return "default"
 }
 
-//RESTClientGetter gets the kubeconfig from EnvSettings
+// RESTClientGetter gets the kubeconfig from EnvSettings
 func (s *EnvSettings) RESTClientGetter() genericclioptions.RESTClientGetter {
-	s.configOnce.Do(func() {
-		clientConfig := kube.GetConfig(s.KubeConfig, s.KubeContext, s.namespace)
-		if s.KubeToken != "" {
-			clientConfig.BearerToken = &s.KubeToken
-		}
-		if s.KubeAPIServer != "" {
-			clientConfig.APIServer = &s.KubeAPIServer
-		}
-
-		s.config = clientConfig
-	})
 	return s.config
 }
diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go
index 0013dbdf0..ef26f3348 100644
--- a/pkg/downloader/chart_downloader.go
+++ b/pkg/downloader/chart_downloader.go
@@ -18,7 +18,6 @@ package downloader
 import (
 	"fmt"
 	"io"
-	"io/ioutil"
 	"net/url"
 	"os"
 	"path/filepath"
@@ -26,6 +25,7 @@ import (
 
 	"github.com/pkg/errors"
 
+	"helm.sh/helm/v3/internal/fileutil"
 	"helm.sh/helm/v3/internal/urlutil"
 	"helm.sh/helm/v3/pkg/getter"
 	"helm.sh/helm/v3/pkg/helmpath"
@@ -72,31 +72,6 @@ type ChartDownloader struct {
 	RepositoryCache  string
 }
 
-// atomicWriteFile atomically (as atomic as os.Rename allows) writes a file to a
-// disk.
-func atomicWriteFile(filename string, body io.Reader, mode os.FileMode) error {
-	tempFile, err := ioutil.TempFile(filepath.Split(filename))
-	if err != nil {
-		return err
-	}
-	tempName := tempFile.Name()
-
-	if _, err := io.Copy(tempFile, body); err != nil {
-		tempFile.Close() // return value is ignored as we are already on error path
-		return err
-	}
-
-	if err := tempFile.Close(); err != nil {
-		return err
-	}
-
-	if err := os.Chmod(tempName, mode); err != nil {
-		return err
-	}
-
-	return os.Rename(tempName, filename)
-}
-
 // DownloadTo retrieves a chart. Depending on the settings, it may also download a provenance file.
 //
 // If Verify is set to VerifyNever, the verification will be nil.
@@ -126,7 +101,7 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven
 
 	name := filepath.Base(u.Path)
 	destfile := filepath.Join(dest, name)
-	if err := atomicWriteFile(destfile, data, 0644); err != nil {
+	if err := fileutil.AtomicWriteFile(destfile, data, 0644); err != nil {
 		return destfile, nil, err
 	}
 
@@ -142,7 +117,7 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven
 			return destfile, ver, nil
 		}
 		provfile := destfile + ".prov"
-		if err := atomicWriteFile(provfile, body, 0644); err != nil {
+		if err := fileutil.AtomicWriteFile(provfile, body, 0644); err != nil {
 			return destfile, nil, err
 		}
 
diff --git a/pkg/kube/config.go b/pkg/kube/config.go
index 624c4a1f7..e00c9acb1 100644
--- a/pkg/kube/config.go
+++ b/pkg/kube/config.go
@@ -19,6 +19,8 @@ package kube // import "helm.sh/helm/v3/pkg/kube"
 import "k8s.io/cli-runtime/pkg/genericclioptions"
 
 // GetConfig returns a Kubernetes client config.
+//
+// Deprecated
 func GetConfig(kubeconfig, context, namespace string) *genericclioptions.ConfigFlags {
 	cf := genericclioptions.NewConfigFlags(true)
 	cf.Namespace = &namespace
diff --git a/pkg/repo/index.go b/pkg/repo/index.go
index 36386665e..6ef2cf8b5 100644
--- a/pkg/repo/index.go
+++ b/pkg/repo/index.go
@@ -17,6 +17,7 @@ limitations under the License.
 package repo
 
 import (
+	"bytes"
 	"io/ioutil"
 	"os"
 	"path"
@@ -29,6 +30,7 @@ import (
 	"github.com/pkg/errors"
 	"sigs.k8s.io/yaml"
 
+	"helm.sh/helm/v3/internal/fileutil"
 	"helm.sh/helm/v3/internal/urlutil"
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chart/loader"
@@ -197,7 +199,7 @@ func (i IndexFile) WriteFile(dest string, mode os.FileMode) error {
 	if err != nil {
 		return err
 	}
-	return ioutil.WriteFile(dest, b, mode)
+	return fileutil.AtomicWriteFile(dest, bytes.NewReader(b), mode)
 }
 
 // Merge merges the given index file into this index.


Test patch:
diff --git a/internal/fileutil/fileutil_test.go b/internal/fileutil/fileutil_test.go
new file mode 100644
index 000000000..9a4bc32c9
--- /dev/null
+++ b/internal/fileutil/fileutil_test.go
@@ -0,0 +1,62 @@
+/*
+Copyright The Helm Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package fileutil
+
+import (
+	"bytes"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestAtomicWriteFile(t *testing.T) {
+	dir, err := ioutil.TempDir("", "helm-tmp")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(dir)
+
+	testpath := filepath.Join(dir, "test")
+	stringContent := "Test content"
+	reader := bytes.NewReader([]byte(stringContent))
+	mode := os.FileMode(0644)
+
+	err = AtomicWriteFile(testpath, reader, mode)
+	if err != nil {
+		t.Errorf("AtomicWriteFile error: %s", err)
+	}
+
+	got, err := ioutil.ReadFile(testpath)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if stringContent != string(got) {
+		t.Fatalf("expected: %s, got: %s", stringContent, string(got))
+	}
+
+	gotinfo, err := os.Stat(testpath)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if mode != gotinfo.Mode() {
+		t.Fatalf("expected %s: to be the same mode as %s",
+			mode, gotinfo.Mode())
+	}
+}
diff --git a/pkg/getter/getter_test.go b/pkg/getter/getter_test.go
index 60eb4738e..79a3338e9 100644
--- a/pkg/getter/getter_test.go
+++ b/pkg/getter/getter_test.go
@@ -53,9 +53,10 @@ func TestProviders(t *testing.T) {
 }
 
 func TestAll(t *testing.T) {
-	all := All(&cli.EnvSettings{
-		PluginsDirectory: pluginDir,
-	})
+	env := cli.New()
+	env.PluginsDirectory = pluginDir
+
+	all := All(env)
 	if len(all) != 3 {
 		t.Errorf("expected 3 providers (default plus two plugins), got %d", len(all))
 	}
@@ -66,9 +67,10 @@ func TestAll(t *testing.T) {
 }
 
 func TestByScheme(t *testing.T) {
-	g := All(&cli.EnvSettings{
-		PluginsDirectory: pluginDir,
-	})
+	env := cli.New()
+	env.PluginsDirectory = pluginDir
+
+	g := All(env)
 	if _, err := g.ByScheme("test"); err != nil {
 		t.Error(err)
 	}
diff --git a/pkg/getter/httpgetter_test.go b/pkg/getter/httpgetter_test.go
index a1288bf47..4d7ada852 100644
--- a/pkg/getter/httpgetter_test.go
+++ b/pkg/getter/httpgetter_test.go
@@ -122,7 +122,7 @@ func TestDownload(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	g, err := All(new(cli.EnvSettings)).ByScheme("http")
+	g, err := All(cli.New()).ByScheme("http")
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/pkg/getter/plugingetter_test.go b/pkg/getter/plugingetter_test.go
index 71563e169..a18fa302b 100644
--- a/pkg/getter/plugingetter_test.go
+++ b/pkg/getter/plugingetter_test.go
@@ -24,9 +24,9 @@ import (
 )
 
 func TestCollectPlugins(t *testing.T) {
-	env := &cli.EnvSettings{
-		PluginsDirectory: pluginDir,
-	}
+	env := cli.New()
+	env.PluginsDirectory = pluginDir
+
 	p, err := collectPlugins(env)
 	if err != nil {
 		t.Fatal(err)
@@ -54,9 +54,8 @@ func TestPluginGetter(t *testing.T) {
 		t.Skip("TODO: refactor this test to work on windows")
 	}
 
-	env := &cli.EnvSettings{
-		PluginsDirectory: pluginDir,
-	}
+	env := cli.New()
+	env.PluginsDirectory = pluginDir
 	pg := NewPluginGetter("echo", env, "test", ".")
 	g, err := pg()
 	if err != nil {
@@ -80,9 +79,9 @@ func TestPluginSubCommands(t *testing.T) {
 		t.Skip("TODO: refactor this test to work on windows")
 	}
 
-	env := &cli.EnvSettings{
-		PluginsDirectory: pluginDir,
-	}
+	env := cli.New()
+	env.PluginsDirectory = pluginDir
+
 	pg := NewPluginGetter("echo -n", env, "test", ".")
 	g, err := pg()
 	if err != nil {
diff --git a/pkg/plugin/plugin_test.go b/pkg/plugin/plugin_test.go
index a869255c4..af0b61846 100644
--- a/pkg/plugin/plugin_test.go
+++ b/pkg/plugin/plugin_test.go
@@ -305,9 +305,8 @@ func TestSetupEnv(t *testing.T) {
 	name := "pequod"
 	base := filepath.Join("testdata/helmhome/helm/plugins", name)
 
-	s := &cli.EnvSettings{
-		PluginsDirectory: "testdata/helmhome/helm/plugins",
-	}
+	s := cli.New()
+	s.PluginsDirectory = "testdata/helmhome/helm/plugins"
 
 	SetupPluginEnv(s, name, base)
 	for _, tt := range []struct {
diff --git a/pkg/repo/index_test.go b/pkg/repo/index_test.go
index 5dbd5e551..466a2c306 100644
--- a/pkg/repo/index_test.go
+++ b/pkg/repo/index_test.go
@@ -428,3 +428,23 @@ func TestIndexAdd(t *testing.T) {
 		t.Errorf("Expected http://example.com/charts/deis-0.1.0.tgz, got %s", i.Entries["deis"][0].URLs[0])
 	}
 }
+
+func TestIndexWrite(t *testing.T) {
+	i := NewIndexFile()
+	i.Add(&chart.Metadata{Name: "clipper", Version: "0.1.0"}, "clipper-0.1.0.tgz", "http://example.com/charts", "sha256:1234567890")
+	dir, err := ioutil.TempDir("", "helm-tmp")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(dir)
+	testpath := filepath.Join(dir, "test")
+	i.WriteFile(testpath, 0600)
+
+	got, err := ioutil.ReadFile(testpath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !strings.Contains(string(got), "clipper-0.1.0.tgz") {
+		t.Fatal("Index files doesn't contain expected content")
+	}
+}