From 637fe79e7fee3ad5abed6a6325e4dacd01b8e5af Mon Sep 17 00:00:00 2001 From: Rohan CJ Date: Wed, 11 Sep 2024 17:21:51 +0530 Subject: [PATCH 1/2] fix: init manifests fail if chart artifact name differs from chart name removed test that no longer tested anything Signed-off-by: Rohan CJ --- pkg/controllers/deploy/deploy.go | 76 ++++++++++++--------------- pkg/controllers/deploy/deploy_test.go | 43 --------------- 2 files changed, 35 insertions(+), 84 deletions(-) delete mode 100644 pkg/controllers/deploy/deploy_test.go diff --git a/pkg/controllers/deploy/deploy.go b/pkg/controllers/deploy/deploy.go index 9161431bf..d1a689ecd 100644 --- a/pkg/controllers/deploy/deploy.go +++ b/pkg/controllers/deploy/deploy.go @@ -52,6 +52,9 @@ const ( VClusterDeployConfigMapNamespace = "kube-system" ) +// default name for base64 bundle names when chart is not provided +const defaultBundleName = "chart-bundle" + type Deployer struct { Log loghelper.Logger @@ -341,7 +344,7 @@ func (r *Deployer) initiateUpgrade(ctx context.Context, chart vclusterconfig.Exp if err != nil { return err } else if path == "" { - return fmt.Errorf("couldn't find chart") + return fmt.Errorf("couldn't find chart %q", chart.Chart.Name) } values := chart.Values @@ -376,51 +379,35 @@ func (r *Deployer) initiateUpgrade(ctx context.Context, chart vclusterconfig.Exp return nil } -func getTarballPath(helmWorkDir, repo, name, version string) (tarballPath, tarballDir string) { +// getTarballDir is the location the chart should be pulled to. Chart names can be unpredictable so the temporary directory should be unique +func getTarballDir(helmWorkDir, repo, name, version string) (tarballDir string) { var repoDir string // hashing the name so that slashes in url characters and other unaccounted-for characters - // don't fail making the directory - if repo != "" { - repoDigest := sha256.Sum256([]byte(repo)) - repoDir = hex.EncodeToString(repoDigest[0:])[0:10] - } - // empty repoDir is ignored + // don't fail when creating the directory + repoDigest := sha256.Sum256([]byte(repo + name + version)) + repoDir = hex.EncodeToString(repoDigest[0:])[0:10] tarballDir = filepath.Join(helmWorkDir, repoDir) - tarballPath = filepath.Join(tarballDir, fmt.Sprintf("%s-%s.tgz", name, version)) - return tarballPath, tarballDir + return tarballDir } func (r *Deployer) findChart(chart vclusterconfig.ExperimentalDeployHelm) (string, error) { - tarballPath, tarballDir := getTarballPath(HelmWorkDir, chart.Chart.Repo, chart.Chart.Name, chart.Chart.Version) + tarballDir := getTarballDir(HelmWorkDir, chart.Chart.Repo, chart.Chart.Name, chart.Chart.Version) + r.Log.Debugf("tarballdir for chart: %q", tarballDir) // if version specified, look for specific file - if chart.Chart.Version != "" { - pathsToTry := []string{tarballPath} - // try with alternate names as well - if chart.Chart.Version[0] != 'v' { - tarballPathWithV, _ := getTarballPath(HelmWorkDir, chart.Chart.Repo, chart.Chart.Name, fmt.Sprintf("v%s", chart.Chart.Version)) - pathsToTry = append(pathsToTry, tarballPathWithV) - } - for _, path := range pathsToTry { - _, err := os.Stat(path) - if err == nil { - return path, nil - } else if !os.IsNotExist(err) { - return "", err - } - } - // if version not specified, look for any version - } else { - files, err := os.ReadDir(tarballDir) - if os.IsNotExist(err) { - return "", nil - } else if err != nil { - return "", err - } - for _, f := range files { - if strings.HasPrefix(f.Name(), chart.Chart.Name+"-") { - return filepath.Join(tarballDir, f.Name()), nil - } + + files, err := os.ReadDir(tarballDir) + if os.IsNotExist(err) { + return "", nil + } else if err != nil { + return "", err + } + for _, f := range files { + name := f.Name() + r.Log.Debugf("checking %q is chart", name) + if strings.HasPrefix(f.Name(), chart.Chart.Name) || strings.HasPrefix(f.Name(), defaultBundleName) { + r.Log.Debugf("%q is chart", name) + return filepath.Join(tarballDir, f.Name()), nil } } @@ -434,7 +421,7 @@ func (r *Deployer) initiateInstall(ctx context.Context, chart vclusterconfig.Exp if err != nil { return err } else if path == "" { - return fmt.Errorf("couldn't find chart") + return fmt.Errorf("couldn't find chart: %q", chart.Chart.Name) } values := chart.Values @@ -500,7 +487,7 @@ func (r *Deployer) pullChartArchive(ctx context.Context, chart vclusterconfig.Ex // check if tarball exists if tarballPath == "" { - tarballPath, tarballDir := getTarballPath(HelmWorkDir, chart.Chart.Repo, chart.Chart.Name, chart.Chart.Version) + tarballDir := getTarballDir(HelmWorkDir, chart.Chart.Repo, chart.Chart.Name, chart.Chart.Version) err := os.MkdirAll(tarballDir, 0755) if err != nil { return err @@ -511,7 +498,14 @@ func (r *Deployer) pullChartArchive(ctx context.Context, chart vclusterconfig.Ex return err } - err = os.WriteFile(tarballPath, bytes, 0666) + bundleName := chart.Chart.Name + if bundleName == "" { + bundleName = defaultBundleName + } + + chartPath := filepath.Join(tarballDir, bundleName+".tar.gz") + r.Log.Debugf("writing bundle to tarball: %q", chartPath) + err = os.WriteFile(chartPath, bytes, 0666) if err != nil { return errors.Wrap(err, "write bundle to file") } diff --git a/pkg/controllers/deploy/deploy_test.go b/pkg/controllers/deploy/deploy_test.go deleted file mode 100644 index 19832dccc..000000000 --- a/pkg/controllers/deploy/deploy_test.go +++ /dev/null @@ -1,43 +0,0 @@ -package deploy - -import ( - "fmt" - "path/filepath" - "testing" - - "gotest.tools/v3/assert" -) - -func TestGetTarballPath(t *testing.T) { - testTable := []struct { - desc string - repo string - name string - version string - - expectedTarballPath string - }{ - { - desc: "no repo", - repo: "", - name: "abc", - version: "v1.2", - - expectedTarballPath: "/tmp/abc-v1.2.tgz", - }, - { - desc: "with repo", - repo: "http://helmrepo.io/foo", - name: "abc", - version: "v1.2", - - expectedTarballPath: fmt.Sprintf("/tmp/%s/abc-v1.2.tgz", "17e8c9faf0"), - }, - } - for _, testCase := range testTable { - t.Logf("running test: %q", testCase.desc) - tarballPath, _ := getTarballPath("/tmp/", testCase.repo, testCase.name, testCase.version) - assert.Equal(t, tarballPath, testCase.expectedTarballPath) - assert.Equal(t, filepath.Dir(tarballPath), filepath.Dir(testCase.expectedTarballPath)) - } -} From 723b65776f52c4d17ecb6f7429d0861a8a28b7c2 Mon Sep 17 00:00:00 2001 From: Rohan CJ Date: Wed, 11 Sep 2024 17:22:50 +0530 Subject: [PATCH 2/2] chore: update go.mod Signed-off-by: Rohan CJ --- go.mod | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index e755c2f9d..33dde1816 100644 --- a/go.mod +++ b/go.mod @@ -39,6 +39,7 @@ require ( github.com/spf13/cobra v1.8.0 github.com/spf13/pflag v1.0.5 github.com/vmware-labs/yaml-jsonpath v0.3.2 + github.com/wk8/go-ordered-map/v2 v2.1.8 go.uber.org/atomic v1.11.0 golang.org/x/mod v0.18.0 golang.org/x/sync v0.7.0 @@ -101,7 +102,6 @@ require ( github.com/russross/blackfriday/v2 v2.1.0 // indirect github.com/sergi/go-diff v1.3.1 // indirect github.com/stoewer/go-strcase v1.3.0 // indirect - github.com/wk8/go-ordered-map/v2 v2.1.8 // indirect go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.22.0 // indirect go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.22.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20240604185151-ef581f913117 // indirect @@ -145,7 +145,7 @@ require ( github.com/google/gofuzz v1.2.0 // indirect github.com/google/pprof v0.0.0-20240424215950-a892ee059fd6 // indirect github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect - github.com/google/uuid v1.6.0 // indirect + github.com/google/uuid v1.6.0 github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7 // indirect github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 // indirect github.com/imdario/mergo v0.3.16 // indirect