fix: write index.yaml file atomically (#7954)

* 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.
Signed-off-by: rabadin <rvbadin@gmail.com>

* Review fix: use RenameWithFallback instead of os.Rename

Signed-off-by: rabadin <rvbadin@gmail.com>

Co-authored-by: rabadin <rvbadin@gmail.com>

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)
+}

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())
+	}
+}

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
 		}
 

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.

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")
+	}
+}