diff --git a/.golangci.yml b/.golangci.yml index 156dd0509..a9b13c35f 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -30,6 +30,7 @@ linters: - thelper - unused - usestdlibvars + - usetesting exclusions: generated: lax diff --git a/go.mod b/go.mod index f08dfc7a1..66de5f821 100644 --- a/go.mod +++ b/go.mod @@ -31,9 +31,9 @@ require ( github.com/spf13/cobra v1.9.1 github.com/spf13/pflag v1.0.6 github.com/stretchr/testify v1.10.0 - golang.org/x/crypto v0.38.0 + golang.org/x/crypto v0.39.0 golang.org/x/term v0.32.0 - golang.org/x/text v0.25.0 + golang.org/x/text v0.26.0 gopkg.in/yaml.v3 v3.0.1 k8s.io/api v0.33.1 k8s.io/apiextensions-apiserver v0.33.1 @@ -154,13 +154,13 @@ require ( go.opentelemetry.io/otel/sdk/metric v1.32.0 // indirect go.opentelemetry.io/otel/trace v1.34.0 // indirect go.opentelemetry.io/proto/otlp v1.4.0 // indirect - golang.org/x/mod v0.24.0 // indirect - golang.org/x/net v0.39.0 // indirect + golang.org/x/mod v0.25.0 // indirect + golang.org/x/net v0.40.0 // indirect golang.org/x/oauth2 v0.29.0 // indirect - golang.org/x/sync v0.14.0 // indirect + golang.org/x/sync v0.15.0 // indirect golang.org/x/sys v0.33.0 // indirect golang.org/x/time v0.11.0 // indirect - golang.org/x/tools v0.32.0 // indirect + golang.org/x/tools v0.33.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20241209162323-e6fa225c2576 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20241209162323-e6fa225c2576 // indirect google.golang.org/grpc v1.68.1 // indirect diff --git a/go.sum b/go.sum index 7fd86290d..29ce847c9 100644 --- a/go.sum +++ b/go.sum @@ -384,16 +384,16 @@ golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5y golang.org/x/crypto v0.13.0/go.mod h1:y6Z2r+Rw4iayiXXAIxJIDAJ1zMW4yaTpebo8fPOliYc= golang.org/x/crypto v0.14.0/go.mod h1:MVFd36DqK4CsrnJYDkBA3VC4m2GkXAM0PvzMCn4JQf4= golang.org/x/crypto v0.15.0/go.mod h1:4ChreQoLWfG3xLDer1WdlH5NdlQ3+mwnQq1YTKY+72g= -golang.org/x/crypto v0.38.0 h1:jt+WWG8IZlBnVbomuhg2Mdq0+BBQaHbtqHEFEigjUV8= -golang.org/x/crypto v0.38.0/go.mod h1:MvrbAqul58NNYPKnOra203SB9vpuZW0e+RRZV+Ggqjw= +golang.org/x/crypto v0.39.0 h1:SHs+kF4LP+f+p14esP5jAoDpHU8Gu/v9lFRK6IT5imM= +golang.org/x/crypto v0.39.0/go.mod h1:L+Xg3Wf6HoL4Bn4238Z6ft6KfEpN0tJGo53AAPC632U= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.12.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.14.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= -golang.org/x/mod v0.24.0 h1:ZfthKaKaT4NrhGVZHO1/WDTwGES4De8KtWO0SIbNJMU= -golang.org/x/mod v0.24.0/go.mod h1:IXM97Txy2VM4PJ3gI61r1YEk/gAj6zAHN3AdZt6S9Ww= +golang.org/x/mod v0.25.0 h1:n7a+ZbQKQA/Ysbyb0/6IbB1H/X41mKgbhfv7AfG/44w= +golang.org/x/mod v0.25.0/go.mod h1:IXM97Txy2VM4PJ3gI61r1YEk/gAj6zAHN3AdZt6S9Ww= golang.org/x/net v0.0.0-20181114220301-adae6a3d119a/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190613194153-d28f0bde5980/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= @@ -407,8 +407,8 @@ golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg= golang.org/x/net v0.15.0/go.mod h1:idbUs1IY1+zTqbi8yxTbhexhEEk5ur9LInksu6HrEpk= golang.org/x/net v0.17.0/go.mod h1:NxSsAGuq816PNPmqtQdLE42eU2Fs7NoRIZrHJAlaCOE= golang.org/x/net v0.18.0/go.mod h1:/czyP5RqHAH4odGYxBJ1qz0+CE5WZ+2j1YgoEo8F2jQ= -golang.org/x/net v0.39.0 h1:ZCu7HMWDxpXpaiKdhzIfaltL9Lp31x/3fCP11bc6/fY= -golang.org/x/net v0.39.0/go.mod h1:X7NRbYVEA+ewNkCNyJ513WmMdQ3BineSwVtN2zD/d+E= +golang.org/x/net v0.40.0 h1:79Xs7wF06Gbdcg4kdCCIQArK11Z1hr5POQ6+fIYHNuY= +golang.org/x/net v0.40.0/go.mod h1:y0hY0exeL2Pku80/zKK7tpntoX23cqL3Oa6njdgRtds= golang.org/x/oauth2 v0.29.0 h1:WdYw2tdTK1S8olAzWHdgeqfy+Mtm9XNhv/xJsY65d98= golang.org/x/oauth2 v0.29.0/go.mod h1:onh5ek6nERTohokkhCD/y2cV4Do3fxFHFuAejCkRWT8= golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= @@ -421,8 +421,8 @@ golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.3.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= golang.org/x/sync v0.4.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= golang.org/x/sync v0.5.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= -golang.org/x/sync v0.14.0 h1:woo0S4Yywslg6hp4eUFjTVOyKt0RookbpAHG4c1HmhQ= -golang.org/x/sync v0.14.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= +golang.org/x/sync v0.15.0 h1:KWH3jNZsfyT6xfAfKiz6MRNmd46ByHDYaZ7KSkCtdW8= +golang.org/x/sync v0.15.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20181116152217-5ac8a444bdc5/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= @@ -462,8 +462,8 @@ golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.13.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= -golang.org/x/text v0.25.0 h1:qVyWApTSYLk/drJRO5mDlNYskwQznZmkpV2c8q9zls4= -golang.org/x/text v0.25.0/go.mod h1:WEdwpYrmk1qmdHvhkSTNPm3app7v4rsT8F2UD6+VHIA= +golang.org/x/text v0.26.0 h1:P42AVeLghgTYr4+xUnTRKDMqpar+PtX7KWuNQL21L8M= +golang.org/x/text v0.26.0/go.mod h1:QK15LZJUUQVJxhz7wXgxSy/CJaTFjd0G+YLonydOVQA= golang.org/x/time v0.11.0 h1:/bpjEDfN9tkoN/ryeYHnv5hcMlc8ncjMcM4XBk5NWV0= golang.org/x/time v0.11.0/go.mod h1:CDIdPxbZBQxdj6cxyCIdrNogrJKMJ7pr37NYpMcMDSg= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= @@ -474,8 +474,8 @@ golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU= golang.org/x/tools v0.13.0/go.mod h1:HvlwmtVNQAhOuCjW7xxvovg8wbNq7LwfXh/k7wXUl58= golang.org/x/tools v0.15.0/go.mod h1:hpksKq4dtpQWS1uQ61JkdqWM3LscIS6Slf+VVkm+wQk= -golang.org/x/tools v0.32.0 h1:Q7N1vhpkQv7ybVzLFtTjvQya2ewbwNDZzUgfXGqtMWU= -golang.org/x/tools v0.32.0/go.mod h1:ZxrU41P/wAbZD8EDa6dDCa6XfpkhJ7HFMjHJXfBDu8s= +golang.org/x/tools v0.33.0 h1:4qz2S3zmRxbGIhDIAgjxvFutSvH5EfnsYrRBj0UI0bc= +golang.org/x/tools v0.33.0/go.mod h1:CIJMaWEY88juyUfo7UbgPqbC8rU2OqfAV1h2Qp0oMYI= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/internal/test/ensure/ensure.go b/internal/test/ensure/ensure.go index c131e6da5..a72f48c2d 100644 --- a/internal/test/ensure/ensure.go +++ b/internal/test/ensure/ensure.go @@ -29,12 +29,12 @@ import ( func HelmHome(t *testing.T) { t.Helper() base := t.TempDir() - os.Setenv(xdg.CacheHomeEnvVar, base) - os.Setenv(xdg.ConfigHomeEnvVar, base) - os.Setenv(xdg.DataHomeEnvVar, base) - os.Setenv(helmpath.CacheHomeEnvVar, "") - os.Setenv(helmpath.ConfigHomeEnvVar, "") - os.Setenv(helmpath.DataHomeEnvVar, "") + t.Setenv(xdg.CacheHomeEnvVar, base) + t.Setenv(xdg.ConfigHomeEnvVar, base) + t.Setenv(xdg.DataHomeEnvVar, base) + t.Setenv(helmpath.CacheHomeEnvVar, "") + t.Setenv(helmpath.ConfigHomeEnvVar, "") + t.Setenv(helmpath.DataHomeEnvVar, "") } // TempFile ensures a temp file for unit testing purposes. @@ -49,7 +49,7 @@ func TempFile(t *testing.T, name string, data []byte) string { t.Helper() path := t.TempDir() filename := filepath.Join(path, name) - if err := os.WriteFile(filename, data, 0755); err != nil { + if err := os.WriteFile(filename, data, 0o755); err != nil { t.Fatal(err) } return path diff --git a/pkg/action/action.go b/pkg/action/action.go index 6905f3f44..40194dfd7 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -26,6 +26,7 @@ import ( "path" "path/filepath" "strings" + "sync" "text/template" "k8s.io/apimachinery/pkg/api/meta" @@ -86,6 +87,8 @@ type Configuration struct { // HookOutputFunc called with container name and returns and expects writer that will receive the log output. HookOutputFunc func(namespace, pod, container string) io.Writer + + mutex sync.Mutex } // renderResources renders the templates in a chart diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 591371e44..1213e87e2 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -49,13 +49,7 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, for i, h := range executingHooks { // Set default delete policy to before-hook-creation - if len(h.DeletePolicies) == 0 { - // TODO(jlegrone): Only apply before-hook-creation delete policy to run to completion - // resources. For all other resource types update in place if a - // resource with the same name already exists and is owned by the - // current release. - h.DeletePolicies = []release.HookDeletePolicy{release.HookBeforeHookCreation} - } + cfg.hookSetDeletePolicy(h) if err := cfg.deleteHookByPolicy(h, release.HookBeforeHookCreation, waitStrategy, timeout); err != nil { return err @@ -154,7 +148,7 @@ func (cfg *Configuration) deleteHookByPolicy(h *release.Hook, policy release.Hoo if h.Kind == "CustomResourceDefinition" { return nil } - if hookHasDeletePolicy(h, policy) { + if cfg.hookHasDeletePolicy(h, policy) { resources, err := cfg.KubeClient.Build(bytes.NewBufferString(h.Manifest), false) if err != nil { return fmt.Errorf("unable to build kubernetes object for deleting hook %s: %w", h.Path, err) @@ -188,10 +182,26 @@ func (cfg *Configuration) deleteHooksByPolicy(hooks []*release.Hook, policy rele // hookHasDeletePolicy determines whether the defined hook deletion policy matches the hook deletion polices // supported by helm. If so, mark the hook as one should be deleted. -func hookHasDeletePolicy(h *release.Hook, policy release.HookDeletePolicy) bool { +func (cfg *Configuration) hookHasDeletePolicy(h *release.Hook, policy release.HookDeletePolicy) bool { + cfg.mutex.Lock() + defer cfg.mutex.Unlock() return slices.Contains(h.DeletePolicies, policy) } +// hookSetDeletePolicy determines whether the defined hook deletion policy matches the hook deletion polices +// supported by helm. If so, mark the hook as one should be deleted. +func (cfg *Configuration) hookSetDeletePolicy(h *release.Hook) { + cfg.mutex.Lock() + defer cfg.mutex.Unlock() + if len(h.DeletePolicies) == 0 { + // TODO(jlegrone): Only apply before-hook-creation delete policy to run to completion + // resources. For all other resource types update in place if a + // resource with the same name already exists and is owned by the + // current release. + h.DeletePolicies = []release.HookDeletePolicy{release.HookBeforeHookCreation} + } +} + // outputLogsByPolicy outputs a pods logs if the hook policy instructs it to func (cfg *Configuration) outputLogsByPolicy(h *release.Hook, releaseNamespace string, policy release.HookOutputLogPolicy) error { if !hookHasOutputLogPolicy(h, policy) { diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index dabd57b22..6c2c91d0a 100644 --- a/pkg/action/install_test.go +++ b/pkg/action/install_test.go @@ -131,7 +131,7 @@ func TestInstallRelease(t *testing.T) { instAction := installAction(t) vals := map[string]interface{}{} - ctx, done := context.WithCancel(context.Background()) + ctx, done := context.WithCancel(t.Context()) res, err := instAction.RunWithContext(ctx, buildChart(), vals) if err != nil { t.Fatalf("Failed install: %s", err) @@ -447,7 +447,9 @@ func TestInstallReleaseIncorrectTemplate_DryRun(t *testing.T) { instAction.DryRun = true vals := map[string]interface{}{} _, err := instAction.Run(buildChart(withSampleIncludingIncorrectTemplates()), vals) - expectedErr := "\"hello/templates/incorrect\" at <.Values.bad.doh>: nil pointer evaluating interface {}.doh" + expectedErr := `hello/templates/incorrect:1:10 + executing "hello/templates/incorrect" at <.Values.bad.doh>: + nil pointer evaluating interface {}.doh` if err == nil { t.Fatalf("Install should fail containing error: %s", expectedErr) } @@ -557,7 +559,7 @@ func TestInstallRelease_Wait_Interrupted(t *testing.T) { instAction.WaitStrategy = kube.StatusWatcherStrategy vals := map[string]interface{}{} - ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := context.WithCancel(t.Context()) time.AfterFunc(time.Second, cancel) goroutines := runtime.NumGoroutine() @@ -641,7 +643,7 @@ func TestInstallRelease_Atomic_Interrupted(t *testing.T) { instAction.Atomic = true vals := map[string]interface{}{} - ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := context.WithCancel(t.Context()) time.AfterFunc(time.Second, cancel) goroutines := runtime.NumGoroutine() diff --git a/pkg/action/upgrade_test.go b/pkg/action/upgrade_test.go index 4476bc44d..e20955560 100644 --- a/pkg/action/upgrade_test.go +++ b/pkg/action/upgrade_test.go @@ -57,7 +57,7 @@ func TestUpgradeRelease_Success(t *testing.T) { upAction.WaitStrategy = kube.StatusWatcherStrategy vals := map[string]interface{}{} - ctx, done := context.WithCancel(context.Background()) + ctx, done := context.WithCancel(t.Context()) res, err := upAction.RunWithContext(ctx, rel.Name, buildChart(), vals) done() req.NoError(err) @@ -384,7 +384,6 @@ func TestUpgradeRelease_Pending(t *testing.T) { } func TestUpgradeRelease_Interrupted_Wait(t *testing.T) { - is := assert.New(t) req := require.New(t) @@ -400,8 +399,7 @@ func TestUpgradeRelease_Interrupted_Wait(t *testing.T) { upAction.WaitStrategy = kube.StatusWatcherStrategy vals := map[string]interface{}{} - ctx := context.Background() - ctx, cancel := context.WithCancel(ctx) + ctx, cancel := context.WithCancel(t.Context()) time.AfterFunc(time.Second, cancel) res, err := upAction.RunWithContext(ctx, rel.Name, buildChart(), vals) @@ -409,11 +407,9 @@ func TestUpgradeRelease_Interrupted_Wait(t *testing.T) { req.Error(err) is.Contains(res.Info.Description, "Upgrade \"interrupted-release\" failed: context canceled") is.Equal(res.Info.Status, release.StatusFailed) - } func TestUpgradeRelease_Interrupted_Atomic(t *testing.T) { - is := assert.New(t) req := require.New(t) @@ -429,8 +425,7 @@ func TestUpgradeRelease_Interrupted_Atomic(t *testing.T) { upAction.Atomic = true vals := map[string]interface{}{} - ctx := context.Background() - ctx, cancel := context.WithCancel(ctx) + ctx, cancel := context.WithCancel(t.Context()) time.AfterFunc(time.Second, cancel) res, err := upAction.RunWithContext(ctx, rel.Name, buildChart(), vals) @@ -446,7 +441,7 @@ func TestUpgradeRelease_Interrupted_Atomic(t *testing.T) { } func TestMergeCustomLabels(t *testing.T) { - var tests = [][3]map[string]string{ + tests := [][3]map[string]string{ {nil, nil, map[string]string{}}, {map[string]string{}, map[string]string{}, map[string]string{}}, {map[string]string{"k1": "v1", "k2": "v2"}, nil, map[string]string{"k1": "v1", "k2": "v2"}}, @@ -551,7 +546,7 @@ func TestUpgradeRelease_DryRun(t *testing.T) { upAction.DryRun = true vals := map[string]interface{}{} - ctx, done := context.WithCancel(context.Background()) + ctx, done := context.WithCancel(t.Context()) res, err := upAction.RunWithContext(ctx, rel.Name, buildChart(withSampleSecret()), vals) done() req.NoError(err) @@ -567,7 +562,7 @@ func TestUpgradeRelease_DryRun(t *testing.T) { upAction.HideSecret = true vals = map[string]interface{}{} - ctx, done = context.WithCancel(context.Background()) + ctx, done = context.WithCancel(t.Context()) res, err = upAction.RunWithContext(ctx, rel.Name, buildChart(withSampleSecret()), vals) done() req.NoError(err) @@ -583,7 +578,7 @@ func TestUpgradeRelease_DryRun(t *testing.T) { upAction.DryRun = false vals = map[string]interface{}{} - ctx, done = context.WithCancel(context.Background()) + ctx, done = context.WithCancel(t.Context()) _, err = upAction.RunWithContext(ctx, rel.Name, buildChart(withSampleSecret()), vals) done() req.Error(err) diff --git a/pkg/chart/v2/util/chartfile.go b/pkg/chart/v2/util/chartfile.go index 6748c6a91..1f9c712b2 100644 --- a/pkg/chart/v2/util/chartfile.go +++ b/pkg/chart/v2/util/chartfile.go @@ -39,7 +39,7 @@ func LoadChartfile(filename string) (*chart.Metadata, error) { return y, err } -// StrictLoadChartFile loads a Chart.yaml into a *chart.Metadata using a strict unmarshaling +// StrictLoadChartfile loads a Chart.yaml into a *chart.Metadata using a strict unmarshaling func StrictLoadChartfile(filename string) (*chart.Metadata, error) { b, err := os.ReadFile(filename) if err != nil { diff --git a/pkg/cli/environment_test.go b/pkg/cli/environment_test.go index 8a3b87936..52326eeff 100644 --- a/pkg/cli/environment_test.go +++ b/pkg/cli/environment_test.go @@ -38,7 +38,6 @@ func TestSetNamespace(t *testing.T) { if settings.namespace != "testns" { t.Errorf("Expected namespace testns, got %s", settings.namespace) } - } func TestEnvSettings(t *testing.T) { @@ -126,7 +125,7 @@ func TestEnvSettings(t *testing.T) { defer resetEnv()() for k, v := range tt.envvars { - os.Setenv(k, v) + t.Setenv(k, v) } flags := pflag.NewFlagSet("testing", pflag.ContinueOnError) @@ -233,10 +232,7 @@ func TestEnvOrBool(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { if tt.env != "" { - t.Cleanup(func() { - os.Unsetenv(tt.env) - }) - os.Setenv(tt.env, tt.val) + t.Setenv(tt.env, tt.val) } actual := envBoolOr(tt.env, tt.def) if actual != tt.expected { diff --git a/pkg/cmd/create_test.go b/pkg/cmd/create_test.go index 26eabbfc3..103cd3bc0 100644 --- a/pkg/cmd/create_test.go +++ b/pkg/cmd/create_test.go @@ -33,7 +33,7 @@ func TestCreateCmd(t *testing.T) { ensure.HelmHome(t) cname := "testchart" dir := t.TempDir() - defer testChdir(t, dir)() + defer t.Chdir(dir) // Run a create if _, _, err := executeActionCommand("create " + cname); err != nil { @@ -64,19 +64,19 @@ func TestCreateStarterCmd(t *testing.T) { ensure.HelmHome(t) cname := "testchart" defer resetEnv()() - os.MkdirAll(helmpath.CachePath(), 0755) - defer testChdir(t, helmpath.CachePath())() + os.MkdirAll(helmpath.CachePath(), 0o755) + defer t.Chdir(helmpath.CachePath()) // Create a starter. starterchart := helmpath.DataPath("starters") - os.MkdirAll(starterchart, 0755) + os.MkdirAll(starterchart, 0o755) if dest, err := chartutil.Create("starterchart", starterchart); err != nil { t.Fatalf("Could not create chart: %s", err) } else { t.Logf("Created %s", dest) } tplpath := filepath.Join(starterchart, "starterchart", "templates", "foo.tpl") - if err := os.WriteFile(tplpath, []byte("test"), 0644); err != nil { + if err := os.WriteFile(tplpath, []byte("test"), 0o644); err != nil { t.Fatalf("Could not write template: %s", err) } @@ -122,7 +122,6 @@ func TestCreateStarterCmd(t *testing.T) { if !found { t.Error("Did not find foo.tpl") } - } func TestCreateStarterAbsoluteCmd(t *testing.T) { @@ -132,19 +131,19 @@ func TestCreateStarterAbsoluteCmd(t *testing.T) { // Create a starter. starterchart := helmpath.DataPath("starters") - os.MkdirAll(starterchart, 0755) + os.MkdirAll(starterchart, 0o755) if dest, err := chartutil.Create("starterchart", starterchart); err != nil { t.Fatalf("Could not create chart: %s", err) } else { t.Logf("Created %s", dest) } tplpath := filepath.Join(starterchart, "starterchart", "templates", "foo.tpl") - if err := os.WriteFile(tplpath, []byte("test"), 0644); err != nil { + if err := os.WriteFile(tplpath, []byte("test"), 0o644); err != nil { t.Fatalf("Could not write template: %s", err) } - os.MkdirAll(helmpath.CachePath(), 0755) - defer testChdir(t, helmpath.CachePath())() + os.MkdirAll(helmpath.CachePath(), 0o755) + defer t.Chdir(helmpath.CachePath()) starterChartPath := filepath.Join(starterchart, "starterchart") diff --git a/pkg/cmd/helpers_test.go b/pkg/cmd/helpers_test.go index b48f802b5..5d71fecad 100644 --- a/pkg/cmd/helpers_test.go +++ b/pkg/cmd/helpers_test.go @@ -149,15 +149,3 @@ func resetEnv() func() { settings = cli.New() } } - -func testChdir(t *testing.T, dir string) func() { - t.Helper() - old, err := os.Getwd() - if err != nil { - t.Fatal(err) - } - if err := os.Chdir(dir); err != nil { - t.Fatal(err) - } - return func() { os.Chdir(old) } -} diff --git a/pkg/cmd/package_test.go b/pkg/cmd/package_test.go index 54358fc12..b17684aa6 100644 --- a/pkg/cmd/package_test.go +++ b/pkg/cmd/package_test.go @@ -111,9 +111,9 @@ func TestPackage(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { cachePath := t.TempDir() - defer testChdir(t, cachePath)() + defer t.Chdir(cachePath) - if err := os.MkdirAll("toot", 0777); err != nil { + if err := os.MkdirAll("toot", 0o777); err != nil { t.Fatal(err) } diff --git a/pkg/cmd/plugin_test.go b/pkg/cmd/plugin_test.go index bc0f7de48..74f7a276a 100644 --- a/pkg/cmd/plugin_test.go +++ b/pkg/cmd/plugin_test.go @@ -79,7 +79,6 @@ func TestManuallyProcessArgs(t *testing.T) { t.Errorf("expected unknown flag %d to be %q, got %q", i, expectUnknown[i], k) } } - } func TestLoadPlugins(t *testing.T) { @@ -327,7 +326,6 @@ func checkCommand(t *testing.T, plugins []*cobra.Command, tests []staticCompleti } func TestPluginDynamicCompletion(t *testing.T) { - tests := []cmdTestCase{{ name: "completion for plugin", cmd: "__complete args ''", @@ -364,7 +362,7 @@ func TestLoadPlugins_HelmNoPlugins(t *testing.T) { settings.PluginsDirectory = "testdata/helmhome/helm/plugins" settings.RepositoryConfig = "testdata/helmhome/helm/repository" - os.Setenv("HELM_NO_PLUGINS", "1") + t.Setenv("HELM_NO_PLUGINS", "1") out := bytes.NewBuffer(nil) cmd := &cobra.Command{} @@ -377,7 +375,6 @@ func TestLoadPlugins_HelmNoPlugins(t *testing.T) { } func TestPluginCmdsCompletion(t *testing.T) { - tests := []cmdTestCase{{ name: "completion for plugin update", cmd: "__complete plugin update ''", diff --git a/pkg/cmd/repo_add_test.go b/pkg/cmd/repo_add_test.go index cfa610611..aa6c4eaad 100644 --- a/pkg/cmd/repo_add_test.go +++ b/pkg/cmd/repo_add_test.go @@ -50,7 +50,7 @@ func TestRepoAddCmd(t *testing.T) { defer srv2.Stop() tmpdir := filepath.Join(t.TempDir(), "path-component.yaml/data") - if err := os.MkdirAll(tmpdir, 0777); err != nil { + if err := os.MkdirAll(tmpdir, 0o777); err != nil { t.Fatal(err) } repoFile := filepath.Join(tmpdir, "repositories.yaml") @@ -99,7 +99,7 @@ func TestRepoAdd(t *testing.T) { forceUpdate: false, repoFile: repoFile, } - os.Setenv(xdg.CacheHomeEnvVar, rootDir) + t.Setenv(xdg.CacheHomeEnvVar, rootDir) if err := o.run(io.Discard); err != nil { t.Error(err) @@ -153,7 +153,7 @@ func TestRepoAddCheckLegalName(t *testing.T) { forceUpdate: false, repoFile: repoFile, } - os.Setenv(xdg.CacheHomeEnvVar, rootDir) + t.Setenv(xdg.CacheHomeEnvVar, rootDir) wantErrorMsg := fmt.Sprintf("repository name (%s) contains '/', please specify a different name without '/'", testRepoName) diff --git a/pkg/cmd/repo_update.go b/pkg/cmd/repo_update.go index 9f4a603ae..6547446f0 100644 --- a/pkg/cmd/repo_update.go +++ b/pkg/cmd/repo_update.go @@ -113,14 +113,19 @@ func updateCharts(repos []*repo.ChartRepository, out io.Writer) error { var wg sync.WaitGroup failRepoURLChan := make(chan string, len(repos)) + writeMutex := sync.Mutex{} for _, re := range repos { wg.Add(1) go func(re *repo.ChartRepository) { defer wg.Done() if _, err := re.DownloadIndexFile(); err != nil { + writeMutex.Lock() + defer writeMutex.Unlock() fmt.Fprintf(out, "...Unable to get an update from the %q chart repository (%s):\n\t%s\n", re.Config.Name, re.Config.URL, err) failRepoURLChan <- re.Config.URL } else { + writeMutex.Lock() + defer writeMutex.Unlock() fmt.Fprintf(out, "...Successfully got an update from the %q chart repository\n", re.Config.Name) } }(re) diff --git a/pkg/cmd/root_test.go b/pkg/cmd/root_test.go index 9521a5aa2..84e3d9ed2 100644 --- a/pkg/cmd/root_test.go +++ b/pkg/cmd/root_test.go @@ -80,7 +80,7 @@ func TestRootCmd(t *testing.T) { ensure.HelmHome(t) for k, v := range tt.envvars { - os.Setenv(k, v) + t.Setenv(k, v) } if _, _, err := executeActionCommand(tt.args); err != nil { diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 750eb7f1d..6e47a0e39 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -34,6 +34,18 @@ import ( chartutil "helm.sh/helm/v4/pkg/chart/v2/util" ) +// taken from https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=141 +// > "template: %s: executing %q at <%s>: %s" +var execErrFmt = regexp.MustCompile(`^template: (?P(?U).+): executing (?P(?U).+) at (?P(?U).+): (?P(?U).+)(?P( template:.*)?)$`) + +// taken from https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=138 +// > "template: %s: %s" +var execErrFmtWithoutTemplate = regexp.MustCompile(`^template: (?P(?U).+): (?P.*)(?P( template:.*)?)$`) + +// taken from https://cs.opensource.google/go/go/+/refs/tags/go1.23.6:src/text/template/exec.go;l=191 +// > "template: no template %q associated with template %q" +var execErrNoTemplateAssociated = regexp.MustCompile(`^template: no template (?P.*) associated with template (?P(.*)?)$`) + // Engine is an implementation of the Helm rendering implementation for templates. type Engine struct { // If strict is enabled, template rendering will fail if a template references @@ -303,7 +315,7 @@ func (e Engine) render(tpls map[string]renderable) (rendered map[string]string, vals["Template"] = chartutil.Values{"Name": filename, "BasePath": tpls[filename].basePath} var buf strings.Builder if err := t.ExecuteTemplate(&buf, filename, vals); err != nil { - return map[string]string{}, cleanupExecError(filename, err) + return map[string]string{}, reformatExecErrorMsg(filename, err) } // Work around the issue where Go will emit "" even if Options(missing=zero) @@ -329,7 +341,33 @@ func cleanupParseError(filename string, err error) error { return fmt.Errorf("parse error at (%s): %s", string(location), errMsg) } -func cleanupExecError(filename string, err error) error { +type TraceableError struct { + location string + message string + executedFunction string +} + +func (t TraceableError) String() string { + var errorString strings.Builder + if t.location != "" { + fmt.Fprintf(&errorString, "%s\n ", t.location) + } + if t.executedFunction != "" { + fmt.Fprintf(&errorString, "%s\n ", t.executedFunction) + } + if t.message != "" { + fmt.Fprintf(&errorString, "%s\n", t.message) + } + return errorString.String() +} + +// reformatExecErrorMsg takes an error message for template rendering and formats it into a formatted +// multi-line error string +func reformatExecErrorMsg(filename string, err error) error { + // This function matches the error message against regex's for the text/template package. + // If the regex's can parse out details from that error message such as the line number, template it failed on, + // and error description, then it will construct a new error that displays these details in a structured way. + // If there are issues with parsing the error message, the err passed into the function should return instead. if _, isExecError := err.(template.ExecError); !isExecError { return err } @@ -348,8 +386,46 @@ func cleanupExecError(filename string, err error) error { if len(parts) >= 2 { return fmt.Errorf("execution error at (%s): %s", string(location), parts[1]) } + current := err + fileLocations := []TraceableError{} + for current != nil { + var traceable TraceableError + if matches := execErrFmt.FindStringSubmatch(current.Error()); matches != nil { + templateName := matches[execErrFmt.SubexpIndex("templateName")] + functionName := matches[execErrFmt.SubexpIndex("functionName")] + locationName := matches[execErrFmt.SubexpIndex("location")] + errMsg := matches[execErrFmt.SubexpIndex("errMsg")] + traceable = TraceableError{ + location: templateName, + message: errMsg, + executedFunction: "executing " + functionName + " at " + locationName + ":", + } + } else if matches := execErrFmtWithoutTemplate.FindStringSubmatch(current.Error()); matches != nil { + templateName := matches[execErrFmt.SubexpIndex("templateName")] + errMsg := matches[execErrFmt.SubexpIndex("errMsg")] + traceable = TraceableError{ + location: templateName, + message: errMsg, + } + } else if matches := execErrNoTemplateAssociated.FindStringSubmatch(current.Error()); matches != nil { + traceable = TraceableError{ + message: current.Error(), + } + } else { + return err + } + if len(fileLocations) == 0 || fileLocations[len(fileLocations)-1] != traceable { + fileLocations = append(fileLocations, traceable) + } + current = errors.Unwrap(current) + } + + var finalErrorString strings.Builder + for _, fileLocation := range fileLocations { + fmt.Fprintf(&finalErrorString, "%s", fileLocation.String()) + } - return err + return errors.New(strings.TrimSpace(finalErrorString.String())) } func sortTemplates(tpls map[string]renderable) []string { diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 68e0158fa..f4228fbd7 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -24,6 +24,8 @@ import ( "testing" "text/template" + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -1289,16 +1291,82 @@ func TestRenderTplMissingKeyString(t *testing.T) { t.Errorf("Expected error, got %v", out) return } - switch err.(type) { - case (template.ExecError): - errTxt := fmt.Sprint(err) - if !strings.Contains(errTxt, "noSuchKey") { - t.Errorf("Expected error to contain 'noSuchKey', got %s", errTxt) - } - default: - // Some unexpected error. - t.Fatal(err) + errTxt := fmt.Sprint(err) + if !strings.Contains(errTxt, "noSuchKey") { + t.Errorf("Expected error to contain 'noSuchKey', got %s", errTxt) + } + +} + +func TestNestedHelpersProducesMultilineStacktrace(t *testing.T) { + c := &chart.Chart{ + Metadata: &chart.Metadata{Name: "NestedHelperFunctions"}, + Templates: []*chart.File{ + {Name: "templates/svc.yaml", Data: []byte( + `name: {{ include "nested_helper.name" . }}`, + )}, + {Name: "templates/_helpers_1.tpl", Data: []byte( + `{{- define "nested_helper.name" -}}{{- include "common.names.get_name" . -}}{{- end -}}`, + )}, + {Name: "charts/common/templates/_helpers_2.tpl", Data: []byte( + `{{- define "common.names.get_name" -}}{{- .Values.nonexistant.key | trunc 63 | trimSuffix "-" -}}{{- end -}}`, + )}, + }, + } + + expectedErrorMessage := `NestedHelperFunctions/templates/svc.yaml:1:9 + executing "NestedHelperFunctions/templates/svc.yaml" at : + error calling include: +NestedHelperFunctions/templates/_helpers_1.tpl:1:39 + executing "nested_helper.name" at : + error calling include: +NestedHelperFunctions/charts/common/templates/_helpers_2.tpl:1:49 + executing "common.names.get_name" at <.Values.nonexistant.key>: + nil pointer evaluating interface {}.key` + + v := chartutil.Values{} + + val, _ := chartutil.CoalesceValues(c, v) + vals := map[string]interface{}{ + "Values": val.AsMap(), } + _, err := Render(c, vals) + + assert.NotNil(t, err) + assert.Equal(t, expectedErrorMessage, err.Error()) +} + +func TestMultilineNoTemplateAssociatedError(t *testing.T) { + c := &chart.Chart{ + Metadata: &chart.Metadata{Name: "multiline"}, + Templates: []*chart.File{ + {Name: "templates/svc.yaml", Data: []byte( + `name: {{ include "nested_helper.name" . }}`, + )}, + {Name: "templates/test.yaml", Data: []byte( + `{{ toYaml .Values }}`, + )}, + {Name: "charts/common/templates/_helpers_2.tpl", Data: []byte( + `{{ toYaml .Values }}`, + )}, + }, + } + + expectedErrorMessage := `multiline/templates/svc.yaml:1:9 + executing "multiline/templates/svc.yaml" at : + error calling include: +template: no template "nested_helper.name" associated with template "gotpl"` + + v := chartutil.Values{} + + val, _ := chartutil.CoalesceValues(c, v) + vals := map[string]interface{}{ + "Values": val.AsMap(), + } + _, err := Render(c, vals) + + assert.NotNil(t, err) + assert.Equal(t, expectedErrorMessage, err.Error()) } func TestRenderCustomTemplateFuncs(t *testing.T) { diff --git a/pkg/gates/gates_test.go b/pkg/gates/gates_test.go index 6bdd17ed6..4d77199e6 100644 --- a/pkg/gates/gates_test.go +++ b/pkg/gates/gates_test.go @@ -23,14 +23,13 @@ import ( const name string = "HELM_EXPERIMENTAL_FEATURE" func TestIsEnabled(t *testing.T) { - os.Unsetenv(name) g := Gate(name) if g.IsEnabled() { t.Errorf("feature gate shows as available, but the environment variable %s was not set", name) } - os.Setenv(name, "1") + t.Setenv(name, "1") if !g.IsEnabled() { t.Errorf("feature gate shows as disabled, but the environment variable %s was set", name) diff --git a/pkg/helmpath/home_unix_test.go b/pkg/helmpath/home_unix_test.go index 6e4189bc9..a64c9bcd6 100644 --- a/pkg/helmpath/home_unix_test.go +++ b/pkg/helmpath/home_unix_test.go @@ -16,7 +16,6 @@ package helmpath import ( - "os" "runtime" "testing" @@ -24,9 +23,9 @@ import ( ) func TestHelmHome(t *testing.T) { - os.Setenv(xdg.CacheHomeEnvVar, "/cache") - os.Setenv(xdg.ConfigHomeEnvVar, "/config") - os.Setenv(xdg.DataHomeEnvVar, "/data") + t.Setenv(xdg.CacheHomeEnvVar, "/cache") + t.Setenv(xdg.ConfigHomeEnvVar, "/config") + t.Setenv(xdg.DataHomeEnvVar, "/data") isEq := func(t *testing.T, got, expected string) { t.Helper() if expected != got { @@ -40,7 +39,7 @@ func TestHelmHome(t *testing.T) { isEq(t, DataPath(), "/data/helm") // test to see if lazy-loading environment variables at runtime works - os.Setenv(xdg.CacheHomeEnvVar, "/cache2") + t.Setenv(xdg.CacheHomeEnvVar, "/cache2") isEq(t, CachePath(), "/cache2/helm") } diff --git a/pkg/helmpath/lazypath_unix_test.go b/pkg/helmpath/lazypath_unix_test.go index 534735d10..4b0f2429b 100644 --- a/pkg/helmpath/lazypath_unix_test.go +++ b/pkg/helmpath/lazypath_unix_test.go @@ -16,7 +16,6 @@ package helmpath import ( - "os" "path/filepath" "testing" @@ -32,15 +31,13 @@ const ( ) func TestDataPath(t *testing.T) { - os.Unsetenv(xdg.DataHomeEnvVar) - expected := filepath.Join(homedir.HomeDir(), ".local", "share", appName, testFile) if lazy.dataPath(testFile) != expected { t.Errorf("expected '%s', got '%s'", expected, lazy.dataPath(testFile)) } - os.Setenv(xdg.DataHomeEnvVar, "/tmp") + t.Setenv(xdg.DataHomeEnvVar, "/tmp") expected = filepath.Join("/tmp", appName, testFile) @@ -50,15 +47,13 @@ func TestDataPath(t *testing.T) { } func TestConfigPath(t *testing.T) { - os.Unsetenv(xdg.ConfigHomeEnvVar) - expected := filepath.Join(homedir.HomeDir(), ".config", appName, testFile) if lazy.configPath(testFile) != expected { t.Errorf("expected '%s', got '%s'", expected, lazy.configPath(testFile)) } - os.Setenv(xdg.ConfigHomeEnvVar, "/tmp") + t.Setenv(xdg.ConfigHomeEnvVar, "/tmp") expected = filepath.Join("/tmp", appName, testFile) @@ -68,15 +63,13 @@ func TestConfigPath(t *testing.T) { } func TestCachePath(t *testing.T) { - os.Unsetenv(xdg.CacheHomeEnvVar) - expected := filepath.Join(homedir.HomeDir(), ".cache", appName, testFile) if lazy.cachePath(testFile) != expected { t.Errorf("expected '%s', got '%s'", expected, lazy.cachePath(testFile)) } - os.Setenv(xdg.CacheHomeEnvVar, "/tmp") + t.Setenv(xdg.CacheHomeEnvVar, "/tmp") expected = filepath.Join("/tmp", appName, testFile) diff --git a/pkg/ignore/rules.go b/pkg/ignore/rules.go index 5281c3d59..3511c2d40 100644 --- a/pkg/ignore/rules.go +++ b/pkg/ignore/rules.go @@ -170,10 +170,10 @@ func (r *Rules) parseRule(rule string) error { rule = strings.TrimSuffix(rule, "/") } - if strings.HasPrefix(rule, "/") { + if after, ok := strings.CutPrefix(rule, "/"); ok { // Require path matches the root path. p.match = func(n string, _ os.FileInfo) bool { - rule = strings.TrimPrefix(rule, "/") + rule = after ok, err := filepath.Match(rule, n) if err != nil { slog.Error("failed to compile", "rule", rule, slog.Any("error", err)) diff --git a/pkg/kube/ready_test.go b/pkg/kube/ready_test.go index 9d1dfd272..db0d02cbe 100644 --- a/pkg/kube/ready_test.go +++ b/pkg/kube/ready_test.go @@ -60,7 +60,7 @@ func Test_ReadyChecker_IsReady_Pod(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &corev1.Pod{}, Name: "foo", Namespace: defaultNamespace}, }, pod: newPodWithCondition("foo", corev1.ConditionTrue), @@ -75,7 +75,7 @@ func Test_ReadyChecker_IsReady_Pod(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &corev1.Pod{}, Name: "foo", Namespace: defaultNamespace}, }, pod: newPodWithCondition("bar", corev1.ConditionTrue), @@ -90,7 +90,7 @@ func Test_ReadyChecker_IsReady_Pod(t *testing.T) { checkJobs: tt.fields.checkJobs, pausedAsReady: tt.fields.pausedAsReady, } - if _, err := c.client.CoreV1().Pods(defaultNamespace).Create(context.TODO(), tt.pod, metav1.CreateOptions{}); err != nil { + if _, err := c.client.CoreV1().Pods(defaultNamespace).Create(t.Context(), tt.pod, metav1.CreateOptions{}); err != nil { t.Errorf("Failed to create Pod error: %v", err) return } @@ -132,7 +132,7 @@ func Test_ReadyChecker_IsReady_Job(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &batchv1.Job{}, Name: "foo", Namespace: defaultNamespace}, }, job: newJob("bar", 1, intToInt32(1), 1, 0), @@ -147,7 +147,7 @@ func Test_ReadyChecker_IsReady_Job(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &batchv1.Job{}, Name: "foo", Namespace: defaultNamespace}, }, job: newJob("foo", 1, intToInt32(1), 1, 0), @@ -162,7 +162,7 @@ func Test_ReadyChecker_IsReady_Job(t *testing.T) { checkJobs: tt.fields.checkJobs, pausedAsReady: tt.fields.pausedAsReady, } - if _, err := c.client.BatchV1().Jobs(defaultNamespace).Create(context.TODO(), tt.job, metav1.CreateOptions{}); err != nil { + if _, err := c.client.BatchV1().Jobs(defaultNamespace).Create(t.Context(), tt.job, metav1.CreateOptions{}); err != nil { t.Errorf("Failed to create Job error: %v", err) return } @@ -204,7 +204,7 @@ func Test_ReadyChecker_IsReady_Deployment(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &appsv1.Deployment{}, Name: "foo", Namespace: defaultNamespace}, }, replicaSet: newReplicaSet("foo", 0, 0, true), @@ -220,7 +220,7 @@ func Test_ReadyChecker_IsReady_Deployment(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &appsv1.Deployment{}, Name: "foo", Namespace: defaultNamespace}, }, replicaSet: newReplicaSet("foo", 0, 0, true), @@ -236,11 +236,11 @@ func Test_ReadyChecker_IsReady_Deployment(t *testing.T) { checkJobs: tt.fields.checkJobs, pausedAsReady: tt.fields.pausedAsReady, } - if _, err := c.client.AppsV1().Deployments(defaultNamespace).Create(context.TODO(), tt.deployment, metav1.CreateOptions{}); err != nil { + if _, err := c.client.AppsV1().Deployments(defaultNamespace).Create(t.Context(), tt.deployment, metav1.CreateOptions{}); err != nil { t.Errorf("Failed to create Deployment error: %v", err) return } - if _, err := c.client.AppsV1().ReplicaSets(defaultNamespace).Create(context.TODO(), tt.replicaSet, metav1.CreateOptions{}); err != nil { + if _, err := c.client.AppsV1().ReplicaSets(defaultNamespace).Create(t.Context(), tt.replicaSet, metav1.CreateOptions{}); err != nil { t.Errorf("Failed to create ReplicaSet error: %v", err) return } @@ -281,7 +281,7 @@ func Test_ReadyChecker_IsReady_PersistentVolumeClaim(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &corev1.PersistentVolumeClaim{}, Name: "foo", Namespace: defaultNamespace}, }, pvc: newPersistentVolumeClaim("foo", corev1.ClaimPending), @@ -296,7 +296,7 @@ func Test_ReadyChecker_IsReady_PersistentVolumeClaim(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &corev1.PersistentVolumeClaim{}, Name: "foo", Namespace: defaultNamespace}, }, pvc: newPersistentVolumeClaim("bar", corev1.ClaimPending), @@ -311,7 +311,7 @@ func Test_ReadyChecker_IsReady_PersistentVolumeClaim(t *testing.T) { checkJobs: tt.fields.checkJobs, pausedAsReady: tt.fields.pausedAsReady, } - if _, err := c.client.CoreV1().PersistentVolumeClaims(defaultNamespace).Create(context.TODO(), tt.pvc, metav1.CreateOptions{}); err != nil { + if _, err := c.client.CoreV1().PersistentVolumeClaims(defaultNamespace).Create(t.Context(), tt.pvc, metav1.CreateOptions{}); err != nil { t.Errorf("Failed to create PersistentVolumeClaim error: %v", err) return } @@ -352,7 +352,7 @@ func Test_ReadyChecker_IsReady_Service(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &corev1.Service{}, Name: "foo", Namespace: defaultNamespace}, }, svc: newService("foo", corev1.ServiceSpec{Type: corev1.ServiceTypeLoadBalancer, ClusterIP: ""}), @@ -367,7 +367,7 @@ func Test_ReadyChecker_IsReady_Service(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &corev1.Service{}, Name: "foo", Namespace: defaultNamespace}, }, svc: newService("bar", corev1.ServiceSpec{Type: corev1.ServiceTypeExternalName, ClusterIP: ""}), @@ -382,7 +382,7 @@ func Test_ReadyChecker_IsReady_Service(t *testing.T) { checkJobs: tt.fields.checkJobs, pausedAsReady: tt.fields.pausedAsReady, } - if _, err := c.client.CoreV1().Services(defaultNamespace).Create(context.TODO(), tt.svc, metav1.CreateOptions{}); err != nil { + if _, err := c.client.CoreV1().Services(defaultNamespace).Create(t.Context(), tt.svc, metav1.CreateOptions{}); err != nil { t.Errorf("Failed to create Service error: %v", err) return } @@ -423,7 +423,7 @@ func Test_ReadyChecker_IsReady_DaemonSet(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &appsv1.DaemonSet{}, Name: "foo", Namespace: defaultNamespace}, }, ds: newDaemonSet("foo", 0, 0, 1, 0, true), @@ -438,7 +438,7 @@ func Test_ReadyChecker_IsReady_DaemonSet(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &appsv1.DaemonSet{}, Name: "foo", Namespace: defaultNamespace}, }, ds: newDaemonSet("bar", 0, 1, 1, 1, true), @@ -453,7 +453,7 @@ func Test_ReadyChecker_IsReady_DaemonSet(t *testing.T) { checkJobs: tt.fields.checkJobs, pausedAsReady: tt.fields.pausedAsReady, } - if _, err := c.client.AppsV1().DaemonSets(defaultNamespace).Create(context.TODO(), tt.ds, metav1.CreateOptions{}); err != nil { + if _, err := c.client.AppsV1().DaemonSets(defaultNamespace).Create(t.Context(), tt.ds, metav1.CreateOptions{}); err != nil { t.Errorf("Failed to create DaemonSet error: %v", err) return } @@ -494,7 +494,7 @@ func Test_ReadyChecker_IsReady_StatefulSet(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &appsv1.StatefulSet{}, Name: "foo", Namespace: defaultNamespace}, }, ss: newStatefulSet("foo", 1, 0, 0, 1, true), @@ -509,7 +509,7 @@ func Test_ReadyChecker_IsReady_StatefulSet(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &appsv1.StatefulSet{}, Name: "foo", Namespace: defaultNamespace}, }, ss: newStatefulSet("bar", 1, 0, 1, 1, true), @@ -524,7 +524,7 @@ func Test_ReadyChecker_IsReady_StatefulSet(t *testing.T) { checkJobs: tt.fields.checkJobs, pausedAsReady: tt.fields.pausedAsReady, } - if _, err := c.client.AppsV1().StatefulSets(defaultNamespace).Create(context.TODO(), tt.ss, metav1.CreateOptions{}); err != nil { + if _, err := c.client.AppsV1().StatefulSets(defaultNamespace).Create(t.Context(), tt.ss, metav1.CreateOptions{}); err != nil { t.Errorf("Failed to create StatefulSet error: %v", err) return } @@ -565,7 +565,7 @@ func Test_ReadyChecker_IsReady_ReplicationController(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &corev1.ReplicationController{}, Name: "foo", Namespace: defaultNamespace}, }, rc: newReplicationController("foo", false), @@ -580,7 +580,7 @@ func Test_ReadyChecker_IsReady_ReplicationController(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &corev1.ReplicationController{}, Name: "foo", Namespace: defaultNamespace}, }, rc: newReplicationController("bar", false), @@ -595,7 +595,7 @@ func Test_ReadyChecker_IsReady_ReplicationController(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &corev1.ReplicationController{}, Name: "foo", Namespace: defaultNamespace}, }, rc: newReplicationController("foo", true), @@ -610,7 +610,7 @@ func Test_ReadyChecker_IsReady_ReplicationController(t *testing.T) { checkJobs: tt.fields.checkJobs, pausedAsReady: tt.fields.pausedAsReady, } - if _, err := c.client.CoreV1().ReplicationControllers(defaultNamespace).Create(context.TODO(), tt.rc, metav1.CreateOptions{}); err != nil { + if _, err := c.client.CoreV1().ReplicationControllers(defaultNamespace).Create(t.Context(), tt.rc, metav1.CreateOptions{}); err != nil { t.Errorf("Failed to create ReplicationController error: %v", err) return } @@ -651,7 +651,7 @@ func Test_ReadyChecker_IsReady_ReplicaSet(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &appsv1.ReplicaSet{}, Name: "foo", Namespace: defaultNamespace}, }, rs: newReplicaSet("foo", 1, 1, true), @@ -666,7 +666,7 @@ func Test_ReadyChecker_IsReady_ReplicaSet(t *testing.T) { pausedAsReady: false, }, args: args{ - ctx: context.TODO(), + ctx: t.Context(), resource: &resource.Info{Object: &appsv1.ReplicaSet{}, Name: "foo", Namespace: defaultNamespace}, }, rs: newReplicaSet("bar", 1, 1, false), @@ -1014,12 +1014,12 @@ func Test_ReadyChecker_podsReadyForObject(t *testing.T) { t.Run(tt.name, func(t *testing.T) { c := NewReadyChecker(fake.NewClientset()) for _, pod := range tt.existPods { - if _, err := c.client.CoreV1().Pods(defaultNamespace).Create(context.TODO(), &pod, metav1.CreateOptions{}); err != nil { + if _, err := c.client.CoreV1().Pods(defaultNamespace).Create(t.Context(), &pod, metav1.CreateOptions{}); err != nil { t.Errorf("Failed to create Pod error: %v", err) return } } - got, err := c.podsReadyForObject(context.TODO(), tt.args.namespace, tt.args.obj) + got, err := c.podsReadyForObject(t.Context(), tt.args.namespace, tt.args.obj) if (err != nil) != tt.wantErr { t.Errorf("podsReadyForObject() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/pkg/kube/resource.go b/pkg/kube/resource.go index 600f256b3..d88b171f0 100644 --- a/pkg/kube/resource.go +++ b/pkg/kube/resource.go @@ -81,5 +81,5 @@ func (r ResourceList) Intersect(rs ResourceList) ResourceList { // isMatchingInfo returns true if infos match on Name and GroupVersionKind. func isMatchingInfo(a, b *resource.Info) bool { - return a.Name == b.Name && a.Namespace == b.Namespace && a.Mapping.GroupVersionKind.Kind == b.Mapping.GroupVersionKind.Kind && a.Mapping.GroupVersionKind.Group == b.Mapping.GroupVersionKind.Group + return a.Name == b.Name && a.Namespace == b.Namespace && a.Mapping.GroupVersionKind == b.Mapping.GroupVersionKind } diff --git a/pkg/kube/resource_test.go b/pkg/kube/resource_test.go index c405ca382..ccc613c1b 100644 --- a/pkg/kube/resource_test.go +++ b/pkg/kube/resource_test.go @@ -59,3 +59,42 @@ func TestResourceList(t *testing.T) { t.Error("expected intersect to return bar") } } + +func TestIsMatchingInfo(t *testing.T) { + gvk := schema.GroupVersionKind{Group: "group1", Version: "version1", Kind: "pod"} + resourceInfo := resource.Info{Name: "name1", Namespace: "namespace1", Mapping: &meta.RESTMapping{GroupVersionKind: gvk}} + + gvkDiffGroup := schema.GroupVersionKind{Group: "diff", Version: "version1", Kind: "pod"} + resourceInfoDiffGroup := resource.Info{Name: "name1", Namespace: "namespace1", Mapping: &meta.RESTMapping{GroupVersionKind: gvkDiffGroup}} + if isMatchingInfo(&resourceInfo, &resourceInfoDiffGroup) { + t.Error("expected resources not equal") + } + + gvkDiffVersion := schema.GroupVersionKind{Group: "group1", Version: "diff", Kind: "pod"} + resourceInfoDiffVersion := resource.Info{Name: "name1", Namespace: "namespace1", Mapping: &meta.RESTMapping{GroupVersionKind: gvkDiffVersion}} + if isMatchingInfo(&resourceInfo, &resourceInfoDiffVersion) { + t.Error("expected resources not equal") + } + + gvkDiffKind := schema.GroupVersionKind{Group: "group1", Version: "version1", Kind: "deployment"} + resourceInfoDiffKind := resource.Info{Name: "name1", Namespace: "namespace1", Mapping: &meta.RESTMapping{GroupVersionKind: gvkDiffKind}} + if isMatchingInfo(&resourceInfo, &resourceInfoDiffKind) { + t.Error("expected resources not equal") + } + + resourceInfoDiffName := resource.Info{Name: "diff", Namespace: "namespace1", Mapping: &meta.RESTMapping{GroupVersionKind: gvk}} + if isMatchingInfo(&resourceInfo, &resourceInfoDiffName) { + t.Error("expected resources not equal") + } + + resourceInfoDiffNamespace := resource.Info{Name: "name1", Namespace: "diff", Mapping: &meta.RESTMapping{GroupVersionKind: gvk}} + if isMatchingInfo(&resourceInfo, &resourceInfoDiffNamespace) { + t.Error("expected resources not equal") + } + + gvkEqual := schema.GroupVersionKind{Group: "group1", Version: "version1", Kind: "pod"} + resourceInfoEqual := resource.Info{Name: "name1", Namespace: "namespace1", Mapping: &meta.RESTMapping{GroupVersionKind: gvkEqual}} + if !isMatchingInfo(&resourceInfo, &resourceInfoEqual) { + t.Error("expected resources to be equal") + } +} diff --git a/pkg/plugin/installer/base_test.go b/pkg/plugin/installer/base_test.go index f4dd6d6be..732ac7927 100644 --- a/pkg/plugin/installer/base_test.go +++ b/pkg/plugin/installer/base_test.go @@ -14,7 +14,6 @@ limitations under the License. package installer // import "helm.sh/helm/v4/pkg/plugin/installer" import ( - "os" "testing" ) @@ -37,12 +36,11 @@ func TestPath(t *testing.T) { for _, tt := range tests { - os.Setenv("HELM_PLUGINS", tt.helmPluginsDir) + t.Setenv("HELM_PLUGINS", tt.helmPluginsDir) baseIns := newBase(tt.source) baseInsPath := baseIns.Path() if baseInsPath != tt.expectPath { t.Errorf("expected name %s, got %s", tt.expectPath, baseInsPath) } - os.Unsetenv("HELM_PLUGINS") } } diff --git a/pkg/plugin/installer/local_installer_test.go b/pkg/plugin/installer/local_installer_test.go index b28920af4..9effcd2c4 100644 --- a/pkg/plugin/installer/local_installer_test.go +++ b/pkg/plugin/installer/local_installer_test.go @@ -20,12 +20,14 @@ import ( "path/filepath" "testing" + "helm.sh/helm/v4/internal/test/ensure" "helm.sh/helm/v4/pkg/helmpath" ) var _ Installer = new(LocalInstaller) func TestLocalInstaller(t *testing.T) { + ensure.HelmHome(t) // Make a temp dir tdir := t.TempDir() if err := os.WriteFile(filepath.Join(tdir, "plugin.yaml"), []byte{}, 0644); err != nil { diff --git a/pkg/postrender/exec_test.go b/pkg/postrender/exec_test.go index 2b091cc12..a10ad2cc4 100644 --- a/pkg/postrender/exec_test.go +++ b/pkg/postrender/exec_test.go @@ -60,11 +60,7 @@ func TestGetFullPath(t *testing.T) { t.Run("binary in PATH resolves correctly", func(t *testing.T) { testpath := setupTestingScript(t) - realPath := os.Getenv("PATH") - os.Setenv("PATH", filepath.Dir(testpath)) - defer func() { - os.Setenv("PATH", realPath) - }() + t.Setenv("PATH", filepath.Dir(testpath)) fullPath, err := getFullPath(filepath.Base(testpath)) is.NoError(err) @@ -183,7 +179,7 @@ func setupTestingScript(t *testing.T) (filepath string) { t.Fatalf("unable to write tempfile for testing: %s", err) } - err = f.Chmod(0755) + err = f.Chmod(0o755) if err != nil { t.Fatalf("unable to make tempfile executable for testing: %s", err) } diff --git a/pkg/provenance/sign_test.go b/pkg/provenance/sign_test.go index 69a6dad5b..9a60fd19c 100644 --- a/pkg/provenance/sign_test.go +++ b/pkg/provenance/sign_test.go @@ -276,7 +276,7 @@ func TestDecodeSignature(t *testing.T) { t.Fatal(err) } - f, err := os.CreateTemp("", "helm-test-sig-") + f, err := os.CreateTemp(t.TempDir(), "helm-test-sig-") if err != nil { t.Fatal(err) } diff --git a/pkg/registry/client.go b/pkg/registry/client.go index 63160cd07..6cfa09a5a 100644 --- a/pkg/registry/client.go +++ b/pkg/registry/client.go @@ -100,27 +100,8 @@ func NewClient(options ...ClientOption) (*Client, error) { client.credentialsFile = helmpath.ConfigPath(CredentialsFileBasename) } if client.httpClient == nil { - type cloner[T any] interface { - Clone() T - } - - // try to copy (clone) the http.DefaultTransport so any mutations we - // perform on it (e.g. TLS config) are not reflected globally - // follow https://github.com/golang/go/issues/39299 for a more elegant - // solution in the future - transport := http.DefaultTransport - if t, ok := transport.(cloner[*http.Transport]); ok { - transport = t.Clone() - } else if t, ok := transport.(cloner[http.RoundTripper]); ok { - // this branch will not be used with go 1.20, it was added - // optimistically to try to clone if the http.DefaultTransport - // implementation changes, still the Clone method in that case - // might not return http.RoundTripper... - transport = t.Clone() - } - client.httpClient = &http.Client{ - Transport: retry.NewTransport(transport), + Transport: NewTransport(client.debug), } } @@ -297,6 +278,11 @@ func ensureTLSConfig(client *auth.Client) (*tls.Config, error) { switch t := t.Base.(type) { case *http.Transport: transport = t + case *LoggingTransport: + switch t := t.RoundTripper.(type) { + case *http.Transport: + transport = t + } } } diff --git a/pkg/registry/client_test.go b/pkg/registry/client_test.go index 8fc392336..2ffd691c2 100644 --- a/pkg/registry/client_test.go +++ b/pkg/registry/client_test.go @@ -17,7 +17,6 @@ limitations under the License. package registry import ( - "context" "io" "testing" @@ -31,7 +30,7 @@ import ( func TestTagManifestTransformsReferences(t *testing.T) { memStore := memory.New() client := &Client{out: io.Discard} - ctx := context.Background() + ctx := t.Context() refWithPlus := "test-registry.io/charts/test:1.0.0+metadata" expectedRef := "test-registry.io/charts/test:1.0.0_metadata" // + becomes _ diff --git a/pkg/registry/transport.go b/pkg/registry/transport.go new file mode 100644 index 000000000..7b9c6744b --- /dev/null +++ b/pkg/registry/transport.go @@ -0,0 +1,175 @@ +/* +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 registry + +import ( + "bytes" + "fmt" + "io" + "log/slog" + "mime" + "net/http" + "strings" + "sync/atomic" + + "oras.land/oras-go/v2/registry/remote/retry" +) + +var ( + // requestCount records the number of logged request-response pairs and will + // be used as the unique id for the next pair. + requestCount uint64 + + // toScrub is a set of headers that should be scrubbed from the log. + toScrub = []string{ + "Authorization", + "Set-Cookie", + } +) + +// payloadSizeLimit limits the maximum size of the response body to be printed. +const payloadSizeLimit int64 = 16 * 1024 // 16 KiB + +// LoggingTransport is an http.RoundTripper that keeps track of the in-flight +// request and add hooks to report HTTP tracing events. +type LoggingTransport struct { + http.RoundTripper +} + +// NewTransport creates and returns a new instance of LoggingTransport +func NewTransport(debug bool) *retry.Transport { + type cloner[T any] interface { + Clone() T + } + + // try to copy (clone) the http.DefaultTransport so any mutations we + // perform on it (e.g. TLS config) are not reflected globally + // follow https://github.com/golang/go/issues/39299 for a more elegant + // solution in the future + transport := http.DefaultTransport + if t, ok := transport.(cloner[*http.Transport]); ok { + transport = t.Clone() + } else if t, ok := transport.(cloner[http.RoundTripper]); ok { + // this branch will not be used with go 1.20, it was added + // optimistically to try to clone if the http.DefaultTransport + // implementation changes, still the Clone method in that case + // might not return http.RoundTripper... + transport = t.Clone() + } + if debug { + transport = &LoggingTransport{RoundTripper: transport} + } + + return retry.NewTransport(transport) +} + +// RoundTrip calls base round trip while keeping track of the current request. +func (t *LoggingTransport) RoundTrip(req *http.Request) (resp *http.Response, err error) { + id := atomic.AddUint64(&requestCount, 1) - 1 + + slog.Debug("Request", "id", id, "url", req.URL, "method", req.Method, "header", logHeader(req.Header)) + resp, err = t.RoundTripper.RoundTrip(req) + if err != nil { + slog.Debug("Response", "id", id, "error", err) + } else if resp != nil { + slog.Debug("Response", "id", id, "status", resp.Status, "header", logHeader(resp.Header), "body", logResponseBody(resp)) + } else { + slog.Debug("Response", "id", id, "response", "nil") + } + + return resp, err +} + +// logHeader prints out the provided header keys and values, with auth header scrubbed. +func logHeader(header http.Header) string { + if len(header) > 0 { + headers := []string{} + for k, v := range header { + for _, h := range toScrub { + if strings.EqualFold(k, h) { + v = []string{"*****"} + } + } + headers = append(headers, fmt.Sprintf(" %q: %q", k, strings.Join(v, ", "))) + } + return strings.Join(headers, "\n") + } + return " Empty header" +} + +// logResponseBody prints out the response body if it is printable and within size limit. +func logResponseBody(resp *http.Response) string { + if resp.Body == nil || resp.Body == http.NoBody { + return " No response body to print" + } + + // non-applicable body is not printed and remains untouched for subsequent processing + contentType := resp.Header.Get("Content-Type") + if contentType == "" { + return " Response body without a content type is not printed" + } + if !isPrintableContentType(contentType) { + return fmt.Sprintf(" Response body of content type %q is not printed", contentType) + } + + buf := bytes.NewBuffer(nil) + body := resp.Body + // restore the body by concatenating the read body with the remaining body + resp.Body = struct { + io.Reader + io.Closer + }{ + Reader: io.MultiReader(buf, body), + Closer: body, + } + // read the body up to limit+1 to check if the body exceeds the limit + if _, err := io.CopyN(buf, body, payloadSizeLimit+1); err != nil && err != io.EOF { + return fmt.Sprintf(" Error reading response body: %v", err) + } + + readBody := buf.String() + if len(readBody) == 0 { + return " Response body is empty" + } + if containsCredentials(readBody) { + return " Response body redacted due to potential credentials" + } + if len(readBody) > int(payloadSizeLimit) { + return readBody[:payloadSizeLimit] + "\n...(truncated)" + } + return readBody +} + +// isPrintableContentType returns true if the contentType is printable. +func isPrintableContentType(contentType string) bool { + mediaType, _, err := mime.ParseMediaType(contentType) + if err != nil { + return false + } + + switch mediaType { + case "application/json", // JSON types + "text/plain", "text/html": // text types + return true + } + return strings.HasSuffix(mediaType, "+json") +} + +// containsCredentials returns true if the body contains potential credentials. +func containsCredentials(body string) bool { + return strings.Contains(body, `"token"`) || strings.Contains(body, `"access_token"`) +} diff --git a/pkg/registry/transport_test.go b/pkg/registry/transport_test.go new file mode 100644 index 000000000..b4990c526 --- /dev/null +++ b/pkg/registry/transport_test.go @@ -0,0 +1,399 @@ +/* +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 registry + +import ( + "bytes" + "errors" + "io" + "net/http" + "testing" +) + +var errMockRead = errors.New("mock read error") + +type errorReader struct{} + +func (e *errorReader) Read(_ []byte) (n int, err error) { + return 0, errMockRead +} + +func Test_isPrintableContentType(t *testing.T) { + tests := []struct { + name string + contentType string + want bool + }{ + { + name: "Empty content type", + contentType: "", + want: false, + }, + { + name: "General JSON type", + contentType: "application/json", + want: true, + }, + { + name: "General JSON type with charset", + contentType: "application/json; charset=utf-8", + want: true, + }, + { + name: "Random type with application/json prefix", + contentType: "application/jsonwhatever", + want: false, + }, + { + name: "Manifest type in JSON", + contentType: "application/vnd.oci.image.manifest.v1+json", + want: true, + }, + { + name: "Manifest type in JSON with charset", + contentType: "application/vnd.oci.image.manifest.v1+json; charset=utf-8", + want: true, + }, + { + name: "Random content type in JSON", + contentType: "application/whatever+json", + want: true, + }, + { + name: "Plain text type", + contentType: "text/plain", + want: true, + }, + { + name: "Plain text type with charset", + contentType: "text/plain; charset=utf-8", + want: true, + }, + { + name: "Random type with text/plain prefix", + contentType: "text/plainnnnn", + want: false, + }, + { + name: "HTML type", + contentType: "text/html", + want: true, + }, + { + name: "Plain text type with charset", + contentType: "text/html; charset=utf-8", + want: true, + }, + { + name: "Random type with text/html prefix", + contentType: "text/htmlllll", + want: false, + }, + { + name: "Binary type", + contentType: "application/octet-stream", + want: false, + }, + { + name: "Unknown type", + contentType: "unknown/unknown", + want: false, + }, + { + name: "Invalid type", + contentType: "text/", + want: false, + }, + { + name: "Random string", + contentType: "random123!@#", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isPrintableContentType(tt.contentType); got != tt.want { + t.Errorf("isPrintableContentType() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_logResponseBody(t *testing.T) { + tests := []struct { + name string + resp *http.Response + want string + wantData []byte + }{ + { + name: "Nil body", + resp: &http.Response{ + Body: nil, + Header: http.Header{"Content-Type": []string{"application/json"}}, + }, + want: " No response body to print", + }, + { + name: "No body", + wantData: nil, + resp: &http.Response{ + Body: http.NoBody, + ContentLength: 100, // in case of HEAD response, the content length is set but the body is empty + Header: http.Header{"Content-Type": []string{"application/json"}}, + }, + want: " No response body to print", + }, + { + name: "Empty body", + wantData: []byte(""), + resp: &http.Response{ + Body: io.NopCloser(bytes.NewReader([]byte(""))), + ContentLength: 0, + Header: http.Header{"Content-Type": []string{"text/plain"}}, + }, + want: " Response body is empty", + }, + { + name: "Unknown content length", + wantData: []byte("whatever"), + resp: &http.Response{ + Body: io.NopCloser(bytes.NewReader([]byte("whatever"))), + ContentLength: -1, + Header: http.Header{"Content-Type": []string{"text/plain"}}, + }, + want: "whatever", + }, + { + name: "Missing content type header", + wantData: []byte("whatever"), + resp: &http.Response{ + Body: io.NopCloser(bytes.NewReader([]byte("whatever"))), + ContentLength: 8, + }, + want: " Response body without a content type is not printed", + }, + { + name: "Empty content type header", + wantData: []byte("whatever"), + resp: &http.Response{ + Body: io.NopCloser(bytes.NewReader([]byte("whatever"))), + ContentLength: 8, + Header: http.Header{"Content-Type": []string{""}}, + }, + want: " Response body without a content type is not printed", + }, + { + name: "Non-printable content type", + wantData: []byte("binary data"), + resp: &http.Response{ + Body: io.NopCloser(bytes.NewReader([]byte("binary data"))), + ContentLength: 11, + Header: http.Header{"Content-Type": []string{"application/octet-stream"}}, + }, + want: " Response body of content type \"application/octet-stream\" is not printed", + }, + { + name: "Body at the limit", + wantData: bytes.Repeat([]byte("a"), int(payloadSizeLimit)), + resp: &http.Response{ + Body: io.NopCloser(bytes.NewReader(bytes.Repeat([]byte("a"), int(payloadSizeLimit)))), + ContentLength: payloadSizeLimit, + Header: http.Header{"Content-Type": []string{"text/plain"}}, + }, + want: string(bytes.Repeat([]byte("a"), int(payloadSizeLimit))), + }, + { + name: "Body larger than limit", + wantData: bytes.Repeat([]byte("a"), int(payloadSizeLimit+1)), + resp: &http.Response{ + Body: io.NopCloser(bytes.NewReader(bytes.Repeat([]byte("a"), int(payloadSizeLimit+1)))), // 1 byte larger than limit + ContentLength: payloadSizeLimit + 1, + Header: http.Header{"Content-Type": []string{"text/plain"}}, + }, + want: string(bytes.Repeat([]byte("a"), int(payloadSizeLimit))) + "\n...(truncated)", + }, + { + name: "Printable content type within limit", + wantData: []byte("data"), + resp: &http.Response{ + Body: io.NopCloser(bytes.NewReader([]byte("data"))), + ContentLength: 4, + Header: http.Header{"Content-Type": []string{"text/plain"}}, + }, + want: "data", + }, + { + name: "Actual body size is larger than content length", + wantData: []byte("data"), + resp: &http.Response{ + Body: io.NopCloser(bytes.NewReader([]byte("data"))), + ContentLength: 3, // mismatched content length + Header: http.Header{"Content-Type": []string{"text/plain"}}, + }, + want: "data", + }, + { + name: "Actual body size is larger than content length and exceeds limit", + wantData: bytes.Repeat([]byte("a"), int(payloadSizeLimit+1)), + resp: &http.Response{ + Body: io.NopCloser(bytes.NewReader(bytes.Repeat([]byte("a"), int(payloadSizeLimit+1)))), // 1 byte larger than limit + ContentLength: 1, // mismatched content length + Header: http.Header{"Content-Type": []string{"text/plain"}}, + }, + want: string(bytes.Repeat([]byte("a"), int(payloadSizeLimit))) + "\n...(truncated)", + }, + { + name: "Actual body size is smaller than content length", + wantData: []byte("data"), + resp: &http.Response{ + Body: io.NopCloser(bytes.NewReader([]byte("data"))), + ContentLength: 5, // mismatched content length + Header: http.Header{"Content-Type": []string{"text/plain"}}, + }, + want: "data", + }, + { + name: "Body contains token", + resp: &http.Response{ + Body: io.NopCloser(bytes.NewReader([]byte(`{"token":"12345"}`))), + ContentLength: 17, + Header: http.Header{"Content-Type": []string{"application/json"}}, + }, + wantData: []byte(`{"token":"12345"}`), + want: " Response body redacted due to potential credentials", + }, + { + name: "Body contains access_token", + resp: &http.Response{ + Body: io.NopCloser(bytes.NewReader([]byte(`{"access_token":"12345"}`))), + ContentLength: 17, + Header: http.Header{"Content-Type": []string{"application/json"}}, + }, + wantData: []byte(`{"access_token":"12345"}`), + want: " Response body redacted due to potential credentials", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := logResponseBody(tt.resp); got != tt.want { + t.Errorf("logResponseBody() = %v, want %v", got, tt.want) + } + // validate the response body + if tt.resp.Body != nil { + readBytes, err := io.ReadAll(tt.resp.Body) + if err != nil { + t.Errorf("failed to read body after logResponseBody(), err= %v", err) + } + if !bytes.Equal(readBytes, tt.wantData) { + t.Errorf("resp.Body after logResponseBody() = %v, want %v", readBytes, tt.wantData) + } + if closeErr := tt.resp.Body.Close(); closeErr != nil { + t.Errorf("failed to close body after logResponseBody(), err= %v", closeErr) + } + } + }) + } +} + +func Test_logResponseBody_error(t *testing.T) { + tests := []struct { + name string + resp *http.Response + want string + }{ + { + name: "Error reading body", + resp: &http.Response{ + Body: io.NopCloser(&errorReader{}), + ContentLength: 10, + Header: http.Header{"Content-Type": []string{"text/plain"}}, + }, + want: " Error reading response body: mock read error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := logResponseBody(tt.resp); got != tt.want { + t.Errorf("logResponseBody() = %v, want %v", got, tt.want) + } + if closeErr := tt.resp.Body.Close(); closeErr != nil { + t.Errorf("failed to close body after logResponseBody(), err= %v", closeErr) + } + }) + } +} + +func Test_containsCredentials(t *testing.T) { + tests := []struct { + name string + body string + want bool + }{ + { + name: "Contains token keyword", + body: `{"token": "12345"}`, + want: true, + }, + { + name: "Contains quoted token keyword", + body: `whatever "token" blah`, + want: true, + }, + { + name: "Contains unquoted token keyword", + body: `whatever token blah`, + want: false, + }, + { + name: "Contains access_token keyword", + body: `{"access_token": "12345"}`, + want: true, + }, + { + name: "Contains quoted access_token keyword", + body: `whatever "access_token" blah`, + want: true, + }, + { + name: "Contains unquoted access_token keyword", + body: `whatever access_token blah`, + want: false, + }, + { + name: "Does not contain credentials", + body: `{"key": "value"}`, + want: false, + }, + { + name: "Empty body", + body: ``, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := containsCredentials(tt.body); got != tt.want { + t.Errorf("containsCredentials() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/repo/chartrepo_test.go b/pkg/repo/chartrepo_test.go index bc15560c9..05e034dd8 100644 --- a/pkg/repo/chartrepo_test.go +++ b/pkg/repo/chartrepo_test.go @@ -70,7 +70,7 @@ func TestIndexCustomSchemeDownload(t *testing.T) { } repo.CachePath = t.TempDir() - tempIndexFile, err := os.CreateTemp("", "test-repo") + tempIndexFile, err := os.CreateTemp(t.TempDir(), "test-repo") if err != nil { t.Fatalf("Failed to create temp index file: %v", err) } diff --git a/pkg/repo/repo_test.go b/pkg/repo/repo_test.go index c2087ebbe..bdaa61eda 100644 --- a/pkg/repo/repo_test.go +++ b/pkg/repo/repo_test.go @@ -197,7 +197,7 @@ func TestWriteFile(t *testing.T) { }, ) - file, err := os.CreateTemp("", "helm-repo") + file, err := os.CreateTemp(t.TempDir(), "helm-repo") if err != nil { t.Errorf("failed to create test-file (%v)", err) } diff --git a/pkg/repo/repotest/server.go b/pkg/repo/repotest/server.go index b366572d8..ea9d5290c 100644 --- a/pkg/repo/repotest/server.go +++ b/pkg/repo/repotest/server.go @@ -16,7 +16,6 @@ limitations under the License. package repotest import ( - "context" "crypto/tls" "fmt" "net/http" @@ -91,10 +90,7 @@ type Server struct { // The temp dir will be removed by testing package automatically when test finished. func NewTempServer(t *testing.T, options ...ServerOption) *Server { t.Helper() - docrootTempDir, err := os.MkdirTemp("", "helm-repotest-") - if err != nil { - t.Fatal(err) - } + docrootTempDir := t.TempDir() srv := newServer(t, docrootTempDir, options...) @@ -173,7 +169,7 @@ func NewOCIServer(t *testing.T, dir string) (*OCIServer, error) { t.Fatal("error generating bcrypt password for test htpasswd file") } htpasswdPath := filepath.Join(dir, testHtpasswdFileBasename) - err = os.WriteFile(htpasswdPath, []byte(fmt.Sprintf("%s:%s\n", testUsername, string(pwBytes))), 0644) + err = os.WriteFile(htpasswdPath, []byte(fmt.Sprintf("%s:%s\n", testUsername, string(pwBytes))), 0o644) if err != nil { t.Fatalf("error creating test htpasswd file") } @@ -197,7 +193,7 @@ func NewOCIServer(t *testing.T, dir string) (*OCIServer, error) { registryURL := fmt.Sprintf("localhost:%d", port) - r, err := registry.NewRegistry(context.Background(), config) + r, err := registry.NewRegistry(t.Context(), config) if err != nil { t.Fatal(err) } @@ -331,7 +327,7 @@ func (s *Server) CopyCharts(origin string) ([]string, error) { if err != nil { return []string{}, err } - if err := os.WriteFile(newname, data, 0644); err != nil { + if err := os.WriteFile(newname, data, 0o644); err != nil { return []string{}, err } copied[i] = newname @@ -355,7 +351,7 @@ func (s *Server) CreateIndex() error { } ifile := filepath.Join(s.docroot, "index.yaml") - return os.WriteFile(ifile, d, 0644) + return os.WriteFile(ifile, d, 0o644) } func (s *Server) start() { @@ -407,5 +403,5 @@ func setTestingRepository(url, fname string) error { Name: "test", URL: url, }) - return r.WriteFile(fname, 0640) + return r.WriteFile(fname, 0o640) }