Repository: grafana/tanka
Pull request #1017 (2024-04-19 15:07:41)

Add repository config option for private repositories
Closes https://github.com/grafana/tanka/issues/922

## What we are trying to do
Usage of private Helm repositories right now requires the credentials to be stored in the `chartfile.yaml`. This leads to issues described in the linked issue where different users usually have different credentials, and those credentials you probably don't want to commit to version control. Helm has a `repositories.yaml` file that is updated when you run `helm repo add` where it stores your user-supplied credentials to any private repository. This file has a similar layout to the repositories section in the `chartfile.yaml`
```yaml
# repositories.yaml
apiVersion: ""
generated: "0001-01-01T00:00:00Z"
repositories:
- caFile: ""
  certFile: ""
  insecure_skip_tls_verify: false
  keyFile: ""
  name: simkev2
  pass_credentials_all: false
  password: <generated token>
  url: https://raw.githubusercontent.com/SimKev2/charts/main/stable/
  username: <generated token>
```

It would be good to provide users an option to use this existing helm config file with their tanka commands to manage helm charts.

## Testing
Currently you can add the chart but it has errors vendoring:
```bash
$ tk tool charts add simkev2/nginx-ingress@1.2.0:1.2.0
{"level":"info","time":"2024-04-19T10:01:20-05:00","message":"Adding 1 Charts ..."}
{"level":"info","time":"2024-04-19T10:01:20-05:00","message":"OK: simkev2/nginx-ingress@1.2.0:1.2.0"}
{"level":"info","time":"2024-04-19T10:01:20-05:00","message":"Added 1 Charts to helmfile.yaml. Vendoring ..."}
{"level":"info","time":"2024-04-19T10:01:20-05:00","message":"Vendoring..."}
{"level":"info","time":"2024-04-19T10:01:20-05:00","message":"Syncing Repositories ..."}
{"level":"info","time":"2024-04-19T10:01:20-05:00","message":"Pulling Charts ..."}
Error: repository "simkev2" not found for chart "simkev2/nginx-ingress"

$ tk tool charts vendor
{"level":"info","time":"2024-04-19T10:01:45-05:00","message":"Vendoring..."}
{"level":"info","time":"2024-04-19T10:01:45-05:00","message":"Syncing Repositories ..."}
{"level":"info","time":"2024-04-19T10:01:46-05:00","message":"Pulling Charts ..."}
Error: repository "simkev2" not found for chart "simkev2/nginx-ingress"
```

After these changes I can specify my helm config file and it works:
```bash
$ ../../../../../go/src/github.com/grafana/tanka/tk tool charts add --repository-config $HOME/Library/Preferences/helm/repositories.yaml simkev2/nginx-ingress@1.2.0:1.2.0
{"level":"info","time":"2024-04-19T10:02:51-05:00","message":"Adding 1 Charts ..."}
{"level":"info","time":"2024-04-19T10:02:51-05:00","message":"OK: simkev2/nginx-ingress@1.2.0:1.2.0"}
{"level":"info","time":"2024-04-19T10:02:51-05:00","message":"Added 1 Charts to helmfile.yaml. Vendoring ..."}
{"level":"info","time":"2024-04-19T10:02:51-05:00","message":"Vendoring..."}
{"level":"info","time":"2024-04-19T10:02:51-05:00","message":"Syncing Repositories ..."}
{"level":"info","time":"2024-04-19T10:02:51-05:00","message":"Pulling Charts ..."}
{"level":"info","time":"2024-04-19T10:02:51-05:00","message":"simkev2/nginx-ingress@1.2.0 downloaded"}

$ rm -rf charts/1.2.0/
$ ../../../../../go/src/github.com/grafana/tanka/tk tool charts vendor --repository-config $HOME/Library/Preferences/helm/repositories.yaml
{"level":"info","time":"2024-04-19T10:03:27-05:00","message":"Vendoring..."}
{"level":"info","time":"2024-04-19T10:03:27-05:00","message":"fluxcd-community/flux2@2.12.4 (dir: 2.12.4) exists"}
{"level":"info","time":"2024-04-19T10:03:27-05:00","message":"Syncing Repositories ..."}
{"level":"info","time":"2024-04-19T10:03:27-05:00","message":"Pulling Charts ..."}
{"level":"info","time":"2024-04-19T10:03:28-05:00","message":"simkev2/nginx-ingress@1.2.0 downloaded"}
```

## Questions for Discussion
2 questions I have for these changes

1. Is there a better way to share `repository-config` flag between the `Add` and `Vendor` commands? I see the log-level option uses the `addCommandsWithLogLevelOption` function, but I don't believe I can run the function through a helper function like that twice? And having a helper function that adds the repo config option and then calls that seemed weird to me.

2. Should we try and merge the `repositories.yaml` repositories with the `chartfile.yaml` repositories instead of overwriting them? I went for the simple option of just replacing the existing config with the helm config, but this does mean that if you had a mix of public and private repositories you would need to ensure that `helm repo add ...` was run on your local machine for each of the public repos. I don't think that is a huge ask for the simplicity it brings the code, but I am open to opinions if we should try to merge-with-overwrite the helm config file to our chartfile.yaml.

github-actions[bot]: [PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.4.7
:---:
:rocket: Deployed preview to https://grafana.github.io/tanka/pr-preview/pr-1017/
on branch [`gh-pages`](https://github.com/grafana/tanka/tree/gh-pages) at 2024-04-19 15:10 UTC
<!-- Sticky Pull Request Commentpr-preview -->
julienduchesne: 1. eh, let's adhere to the WET principle (write everything twice). We can always reformat later if this becomes an issue
2. I think the way you did it is good enough. It maintains the current functionality and provides enough to close the issue.

Patch:
diff --git a/cmd/tk/toolCharts.go b/cmd/tk/toolCharts.go
index 47b80933..84e0fd5d 100644
--- a/cmd/tk/toolCharts.go
+++ b/cmd/tk/toolCharts.go
@@ -35,6 +35,7 @@ func chartsVendorCmd() *cli.Command {
 		Short: "Download Charts to a local folder",
 	}
 	prune := cmd.Flags().Bool("prune", false, "also remove non-vendored files from the destination directory")
+	repoConfigPath := cmd.Flags().String("repository-config", "", "specify a local helm repository config file to use instead of the repositories in the chartfile.yaml. For use with private repositories")
 
 	cmd.Run = func(cmd *cli.Command, args []string) error {
 		c, err := loadChartfile()
@@ -42,7 +43,7 @@ func chartsVendorCmd() *cli.Command {
 			return err
 		}
 
-		return c.Vendor(*prune)
+		return c.Vendor(*prune, *repoConfigPath)
 	}
 
 	return cmd
@@ -53,6 +54,7 @@ func chartsAddCmd() *cli.Command {
 		Use:   "add [chart@version] [...]",
 		Short: "Adds Charts to the chartfile",
 	}
+	repoConfigPath := cmd.Flags().String("repository-config", "", "specify a local helm repository config file to use instead of the repositories in the chartfile.yaml. For use with private repositories")
 
 	cmd.Run = func(cmd *cli.Command, args []string) error {
 		c, err := loadChartfile()
@@ -60,7 +62,7 @@ func chartsAddCmd() *cli.Command {
 			return err
 		}
 
-		return c.Add(args)
+		return c.Add(args, *repoConfigPath)
 	}
 
 	return cmd
diff --git a/pkg/helm/charts.go b/pkg/helm/charts.go
index ee27907e..73b692e2 100644
--- a/pkg/helm/charts.go
+++ b/pkg/helm/charts.go
@@ -58,6 +58,29 @@ func LoadChartfile(projectRoot string) (*Charts, error) {
 	return charts, nil
 }
 
+// LoadHelmRepoConfig reads in a helm config file
+func LoadHelmRepoConfig(repoConfigPath string) (*ConfigFile, error) {
+	// make sure path is valid
+	repoPath, err := filepath.Abs(repoConfigPath)
+	if err != nil {
+		return nil, err
+	}
+
+	// open repo config file
+	data, err := os.ReadFile(repoPath)
+	if err != nil {
+		return nil, err
+	}
+
+	// parse the file non-strictly to account for any minor config changes
+	rc := &ConfigFile{}
+	if err := yaml.Unmarshal(data, rc); err != nil {
+		return nil, err
+	}
+
+	return rc, nil
+}
+
 // Charts exposes the central Chartfile management functions
 type Charts struct {
 	// Manifest are the chartfile.yaml contents. It holds data about the developers intentions
@@ -89,12 +112,20 @@ func (c Charts) ManifestFile() string {
 
 // Vendor pulls all Charts specified in the manifest into the local charts
 // directory. It fetches the repository index before doing so.
-func (c Charts) Vendor(prune bool) error {
+func (c Charts) Vendor(prune bool, repoConfigPath string) error {
 	dir := c.ChartDir()
 	if err := os.MkdirAll(dir, os.ModePerm); err != nil {
 		return err
 	}
 
+	if repoConfigPath != "" {
+		repoConfig, err := LoadHelmRepoConfig(repoConfigPath)
+		if err != nil {
+			return err
+		}
+		c.Manifest.Repositories = repoConfig.Repositories
+	}
+
 	// Check that there are no output conflicts before vendoring
 	if err := c.Manifest.Requires.Validate(); err != nil {
 		return err
@@ -199,7 +230,7 @@ func (c Charts) Vendor(prune bool) error {
 
 // Add adds every Chart in reqs to the Manifest after validation, and runs
 // Vendor afterwards
-func (c *Charts) Add(reqs []string) error {
+func (c *Charts) Add(reqs []string, repoConfigPath string) error {
 	log.Info().Msgf("Adding %v Charts ...", len(reqs))
 
 	// parse new charts, append in memory
@@ -238,7 +269,7 @@ func (c *Charts) Add(reqs []string) error {
 
 	// worked fine? vendor it
 	log.Info().Msgf("Added %v Charts to helmfile.yaml. Vendoring ...", added)
-	return c.Vendor(false)
+	return c.Vendor(false, repoConfigPath)
 }
 
 func (c *Charts) AddRepos(repos ...Repo) error {
diff --git a/pkg/helm/spec.go b/pkg/helm/spec.go
index edd449c1..c519ae86 100644
--- a/pkg/helm/spec.go
+++ b/pkg/helm/spec.go
@@ -33,6 +33,19 @@ type Chartfile struct {
 	Directory string `json:"directory,omitempty"`
 }
 
+// ConfigFile represents the default Helm config structure to be used in place of the chartfile
+// Repositories if supplied.
+type ConfigFile struct {
+	// Version of the Helm repo config schema
+	APIVersion string `json:"apiVersion"`
+
+	// The datetime of when this repo config was generated
+	Generated string `json:"generated"`
+
+	// Repositories to source from
+	Repositories Repos `json:"repositories"`
+}
+
 // Repo describes a single Helm repository
 type Repo struct {
 	Name     string `json:"name,omitempty"`


Test patch:
diff --git a/pkg/helm/charts_test.go b/pkg/helm/charts_test.go
index 834a45cf..44bf7662 100644
--- a/pkg/helm/charts_test.go
+++ b/pkg/helm/charts_test.go
@@ -88,28 +88,47 @@ func TestAdd(t *testing.T) {
 	c, err := InitChartfile(filepath.Join(tempDir, Filename))
 	require.NoError(t, err)
 
-	err = c.Add([]string{"stable/prometheus@11.12.1"})
+	err = c.Add([]string{"stable/prometheus@11.12.1"}, "")
 	assert.NoError(t, err)
 
 	// Adding again the same chart
-	err = c.Add([]string{"stable/prometheus@11.12.1"})
+	err = c.Add([]string{"stable/prometheus@11.12.1"}, "")
 	assert.EqualError(t, err, "1 Chart(s) were skipped. Please check above logs for details")
 
 	// Adding a chart with a different version to the same path, causes a conflict
-	err = c.Add([]string{"stable/prometheus@11.12.0"})
+	err = c.Add([]string{"stable/prometheus@11.12.0"}, "")
 	assert.EqualError(t, err, `Validation errors:
  - output directory "prometheus" is used twice, by charts "stable/prometheus@11.12.1" and "stable/prometheus@11.12.0"`)
 
 	// Add a chart with a specific extract directory
-	err = c.Add([]string{"stable/prometheus@11.12.0:prometheus-11.12.0"})
+	err = c.Add([]string{"stable/prometheus@11.12.0:prometheus-11.12.0"}, "")
+	assert.NoError(t, err)
+
+	// Add a chart while specifying a helm repo config file
+	require.NoError(t, os.WriteFile(filepath.Join(tempDir, "helmConfig.yaml"), []byte(`
+apiVersion: ""
+generated: "0001-01-01T00:00:00Z"
+repositories:
+- caFile: ""
+  certFile: ""
+  insecure_skip_tls_verify: false
+  keyFile: ""
+  name: private
+  pass_credentials_all: false
+  password: ""
+  url: https://charts.helm.sh/stable
+  username: ""
+`), 0644))
+	err = c.Add([]string{"private/prometheus@11.12.1:private-11.12.1"}, filepath.Join(tempDir, "helmConfig.yaml"))
 	assert.NoError(t, err)
 
 	// Check file contents
 	listResult, err := os.ReadDir(filepath.Join(tempDir, "charts"))
 	assert.NoError(t, err)
-	assert.Equal(t, 2, len(listResult))
-	assert.Equal(t, "prometheus", listResult[0].Name())
-	assert.Equal(t, "prometheus-11.12.0", listResult[1].Name())
+	assert.Equal(t, 3, len(listResult))
+	assert.Equal(t, "private-11.12.1", listResult[0].Name())
+	assert.Equal(t, "prometheus", listResult[1].Name())
+	assert.Equal(t, "prometheus-11.12.0", listResult[2].Name())
 
 	chartContent, err := os.ReadFile(filepath.Join(tempDir, "charts", "prometheus", "Chart.yaml"))
 	assert.NoError(t, err)
@@ -118,6 +137,10 @@ func TestAdd(t *testing.T) {
 	chartContent, err = os.ReadFile(filepath.Join(tempDir, "charts", "prometheus-11.12.0", "Chart.yaml"))
 	assert.NoError(t, err)
 	assert.Contains(t, string(chartContent), `version: 11.12.0`)
+
+	chartContent, err = os.ReadFile(filepath.Join(tempDir, "charts", "private-11.12.1", "Chart.yaml"))
+	assert.NoError(t, err)
+	assert.Contains(t, string(chartContent), `version: 11.12.1`)
 }
 
 func TestAddOCI(t *testing.T) {
@@ -128,7 +151,7 @@ func TestAddOCI(t *testing.T) {
 	err = c.AddRepos(Repo{Name: "karpenter", URL: "oci://public.ecr.aws/karpenter"})
 	assert.NoError(t, err)
 
-	err = c.Add([]string{"karpenter/karpenter@v0.27.1"})
+	err = c.Add([]string{"karpenter/karpenter@v0.27.1"}, "")
 	assert.NoError(t, err)
 
 	// Check file contents
@@ -143,7 +166,7 @@ func TestRevendorDeletedFiles(t *testing.T) {
 	c, err := InitChartfile(filepath.Join(tempDir, Filename))
 	require.NoError(t, err)
 
-	err = c.Add([]string{"stable/prometheus@11.12.1"})
+	err = c.Add([]string{"stable/prometheus@11.12.1"}, "")
 	assert.NoError(t, err)
 
 	// Check file contents
@@ -153,7 +176,7 @@ func TestRevendorDeletedFiles(t *testing.T) {
 
 	// Delete the whole dir and revendor
 	require.NoError(t, os.RemoveAll(filepath.Join(tempDir, "charts", "prometheus")))
-	assert.NoError(t, c.Vendor(true))
+	assert.NoError(t, c.Vendor(true, ""))
 
 	// Check file contents
 	chartContent, err = os.ReadFile(filepath.Join(tempDir, "charts", "prometheus", "Chart.yaml"))
@@ -162,7 +185,7 @@ func TestRevendorDeletedFiles(t *testing.T) {
 
 	// Delete just the Chart.yaml and revendor
 	require.NoError(t, os.Remove(filepath.Join(tempDir, "charts", "prometheus", "Chart.yaml")))
-	assert.NoError(t, c.Vendor(true))
+	assert.NoError(t, c.Vendor(true, ""))
 
 	// Check file contents
 	chartContent, err = os.ReadFile(filepath.Join(tempDir, "charts", "prometheus", "Chart.yaml"))
@@ -178,17 +201,17 @@ func TestPrune(t *testing.T) {
 			require.NoError(t, err)
 
 			// Add a chart
-			require.NoError(t, c.Add([]string{"stable/prometheus@11.12.1"}))
+			require.NoError(t, c.Add([]string{"stable/prometheus@11.12.1"}, ""))
 
 			// Add a chart with a directory
-			require.NoError(t, c.Add([]string{"stable/prometheus@11.12.1:custom-dir"}))
+			require.NoError(t, c.Add([]string{"stable/prometheus@11.12.1:custom-dir"}, ""))
 
 			// Add unrelated files and folders
 			require.NoError(t, os.WriteFile(filepath.Join(tempDir, "charts", "foo.txt"), []byte("foo"), 0644))
 			require.NoError(t, os.Mkdir(filepath.Join(tempDir, "charts", "foo"), 0755))
 			require.NoError(t, os.WriteFile(filepath.Join(tempDir, "charts", "foo", "Chart.yaml"), []byte("foo"), 0644))
 
-			require.NoError(t, c.Vendor(prune))
+			require.NoError(t, c.Vendor(prune, ""))
 
 			// Check if files are pruned
 			listResult, err := os.ReadDir(filepath.Join(tempDir, "charts"))
@@ -217,7 +240,41 @@ func TestInvalidChartName(t *testing.T) {
 		Version: "1.0.0",
 	})
 
-	err = c.Vendor(false)
+	err = c.Vendor(false, "")
 	assert.EqualError(t, err, `Validation errors:
  - Chart name "noslash" is not valid. Expecting a repo/name format.`)
 }
+
+func TestConfigFileOption(t *testing.T) {
+	tempDir := t.TempDir()
+	c, err := InitChartfile(filepath.Join(tempDir, Filename))
+	require.NoError(t, err)
+
+	// Don't want to commit credentials so we just verify the "private" repo reference will make
+	// use of this helm config since the InitChartfile does not have a reference to it.
+	require.NoError(t, os.WriteFile(filepath.Join(tempDir, "helmConfig.yaml"), []byte(`
+apiVersion: ""
+generated: "0001-01-01T00:00:00Z"
+repositories:
+- caFile: ""
+  certFile: ""
+  insecure_skip_tls_verify: false
+  keyFile: ""
+  name: private
+  pass_credentials_all: false
+  password: ""
+  url: https://charts.helm.sh/stable
+  username: ""
+`), 0644))
+	c.Manifest.Requires = append(c.Manifest.Requires, Requirement{
+		Chart:   "private/prometheus",
+		Version: "11.12.1",
+	})
+
+	err = c.Vendor(false, filepath.Join(tempDir, "helmConfig.yaml"))
+	assert.NoError(t, err)
+
+	chartContent, err := os.ReadFile(filepath.Join(tempDir, "charts", "prometheus", "Chart.yaml"))
+	assert.NoError(t, err)
+	assert.Contains(t, string(chartContent), `version: 11.12.1`)
+}