From b1d4dc680dcf7d6c8c8213c16b7fc9daa80a9f53 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Tue, 21 Oct 2025 22:48:51 +0100 Subject: [PATCH 01/14] feat: reinstate logger parameter to actions package Fixes: #31399 Signed-off-by: Evans Mungai --- pkg/action/action.go | 41 +++++++++++++-- pkg/action/action_test.go | 2 +- pkg/action/history.go | 4 +- pkg/action/install.go | 22 ++++---- pkg/action/rollback.go | 23 ++++---- pkg/action/uninstall.go | 12 ++--- pkg/action/upgrade.go | 30 +++++------ pkg/cmd/root.go | 2 +- pkg/kube/client.go | 65 +++++++++++++++-------- pkg/storage/driver/cfgmaps.go | 44 ++++++++++++---- pkg/storage/driver/memory.go | 24 ++++++++- pkg/storage/driver/secrets.go | 28 ++++++++-- pkg/storage/driver/sql.go | 99 +++++++++++++++++++++-------------- pkg/storage/storage.go | 53 +++++++++++++------ 14 files changed, 306 insertions(+), 143 deletions(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index fd75b85d3..4641b7427 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -29,6 +29,7 @@ import ( "slices" "strings" "sync" + "sync/atomic" "text/template" "time" @@ -109,9 +110,19 @@ 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 + // logger is an slog.Logger pointer to use with the Configuration instance + logger atomic.Pointer[slog.Logger] + + // Mutex is an exclusive lock for concurrent access to the action mutex sync.Mutex } +func NewConfiguration() *Configuration { + c := &Configuration{} + c.SetLogger(slog.Default()) + return c +} + const ( // filenameAnnotation is the annotation key used to store the original filename // information in manifest annotations for post-rendering reconstruction. @@ -376,8 +387,8 @@ func (cfg *Configuration) getCapabilities() (*common.Capabilities, error) { apiVersions, err := GetVersionSet(dc) if err != nil { if discovery.IsGroupDiscoveryFailedError(err) { - slog.Warn("the kubernetes server has an orphaned API service", slog.Any("error", err)) - slog.Warn("to fix this, kubectl delete apiservice ") + cfg.Logger().Warn("the kubernetes server has an orphaned API service", slog.Any("error", err)) + cfg.Logger().Warn("to fix this, kubectl delete apiservice ") } else { return nil, fmt.Errorf("could not get apiVersions from Kubernetes: %w", err) } @@ -476,13 +487,14 @@ func GetVersionSet(client discovery.ServerResourcesInterface) (common.VersionSet // recordRelease with an update operation in case reuse has been set. func (cfg *Configuration) recordRelease(r *release.Release) { if err := cfg.Releases.Update(r); err != nil { - slog.Warn("failed to update release", "name", r.Name, "revision", r.Version, slog.Any("error", err)) + cfg.Logger().Warn("failed to update release", "name", r.Name, "revision", r.Version, slog.Any("error", err)) } } // Init initializes the action configuration func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namespace, helmDriver string) error { kc := kube.New(getter) + kc.SetLogger(cfg.Logger()) lazyClient := &lazyClient{ namespace: namespace, @@ -493,9 +505,11 @@ func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namesp switch helmDriver { case "secret", "secrets", "": d := driver.NewSecrets(newSecretClient(lazyClient)) + d.SetLogger(cfg.Logger()) store = storage.Init(d) case "configmap", "configmaps": d := driver.NewConfigMaps(newConfigMapClient(lazyClient)) + d.SetLogger(cfg.Logger()) store = storage.Init(d) case "memory": var d *driver.Memory @@ -510,11 +524,13 @@ func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namesp if d == nil { d = driver.NewMemory() } + d.SetLogger(cfg.Logger()) d.SetNamespace(namespace) store = storage.Init(d) case "sql": d, err := driver.NewSQL( os.Getenv("HELM_DRIVER_SQL_CONNECTION_STRING"), + cfg.Logger(), namespace, ) if err != nil { @@ -528,6 +544,7 @@ func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namesp cfg.RESTClientGetter = getter cfg.KubeClient = kc cfg.Releases = store + cfg.Releases.SetLogger(cfg.Logger()) cfg.HookOutputFunc = func(_, _, _ string) io.Writer { return io.Discard } return nil @@ -538,6 +555,24 @@ func (cfg *Configuration) SetHookOutputFunc(hookOutputFunc func(_, _, _ string) cfg.HookOutputFunc = hookOutputFunc } +// Logger returns the logger for the Configuration. If nil, returns slog.Default(). +func (cfg *Configuration) Logger() *slog.Logger { + if lg := cfg.logger.Load(); lg != nil { + return lg + } + return slog.Default() // We rarely get here, just be defensive +} + +// SetLogger sets the logger for the Configuration. If nil, sets the default logger. +func (cfg *Configuration) SetLogger(newLogger *slog.Logger) { + // Only set logger if it's currently nil + if newLogger == nil { + cfg.logger.Store(slog.Default()) // We never want to set the logger to nil + return + } + cfg.logger.Store(newLogger) +} + func determineReleaseSSApplyMethod(serverSideApply bool) release.ApplyMethod { if serverSideApply { return release.ApplyMethodServerSideApply diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index 06329095e..8f9159dd1 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -344,7 +344,7 @@ func TestConfiguration_Init(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cfg := &Configuration{} + cfg := NewConfiguration() actualErr := cfg.Init(nil, "default", tt.helmDriver) if tt.expectErr { diff --git a/pkg/action/history.go b/pkg/action/history.go index dc3ab51d4..90307c79b 100644 --- a/pkg/action/history.go +++ b/pkg/action/history.go @@ -17,8 +17,6 @@ limitations under the License. package action import ( - "log/slog" - "fmt" chartutil "helm.sh/helm/v4/pkg/chart/v2/util" @@ -55,6 +53,6 @@ func (h *History) Run(name string) ([]release.Releaser, error) { return nil, fmt.Errorf("release name is invalid: %s", name) } - slog.Debug("getting history for release", "release", name) + h.cfg.Logger().Debug("getting history for release", "release", name) return h.cfg.Releases.History(name) } diff --git a/pkg/action/install.go b/pkg/action/install.go index e4b23aceb..433b9a8d8 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -193,7 +193,7 @@ func (i *Install) installCRDs(crds []chart.CRD) error { // If the error is CRD already exists, continue. if apierrors.IsAlreadyExists(err) { crdName := res[0].Name - slog.Debug("CRD is already present. Skipping", "crd", crdName) + i.cfg.Logger().Debug("CRD is already present. Skipping", "crd", crdName) continue } return fmt.Errorf("failed to install CRD %s: %w", obj.Name, err) @@ -221,7 +221,7 @@ func (i *Install) installCRDs(crds []chart.CRD) error { return err } - slog.Debug("clearing discovery cache") + i.cfg.Logger().Debug("clearing discovery cache") discoveryClient.Invalidate() _, _ = discoveryClient.ServerGroups() @@ -234,7 +234,7 @@ func (i *Install) installCRDs(crds []chart.CRD) error { return err } if resettable, ok := restMapper.(meta.ResettableRESTMapper); ok { - slog.Debug("clearing REST mapper cache") + i.cfg.Logger().Debug("clearing REST mapper cache") resettable.Reset() } } @@ -267,24 +267,24 @@ func (i *Install) RunWithContext(ctx context.Context, ch ci.Charter, vals map[st if interactWithServer(i.DryRunStrategy) { if err := i.cfg.KubeClient.IsReachable(); err != nil { - slog.Error(fmt.Sprintf("cluster reachability check failed: %v", err)) + i.cfg.Logger().Error(fmt.Sprintf("cluster reachability check failed: %v", err)) return nil, fmt.Errorf("cluster reachability check failed: %w", err) } } // HideSecret must be used with dry run. Otherwise, return an error. if !isDryRun(i.DryRunStrategy) && i.HideSecret { - slog.Error("hiding Kubernetes secrets requires a dry-run mode") + i.cfg.Logger().Error("hiding Kubernetes secrets requires a dry-run mode") return nil, errors.New("hiding Kubernetes secrets requires a dry-run mode") } if err := i.availableName(); err != nil { - slog.Error("release name check failed", slog.Any("error", err)) + i.cfg.Logger().Error("release name check failed", slog.Any("error", err)) return nil, fmt.Errorf("release name check failed: %w", err) } if err := chartutil.ProcessDependencies(chrt, vals); err != nil { - slog.Error("chart dependencies processing failed", slog.Any("error", err)) + i.cfg.Logger().Error("chart dependencies processing failed", slog.Any("error", err)) return nil, fmt.Errorf("chart dependencies processing failed: %w", err) } @@ -293,7 +293,7 @@ func (i *Install) RunWithContext(ctx context.Context, ch ci.Charter, vals map[st if crds := chrt.CRDObjects(); interactWithServer(i.DryRunStrategy) && !i.SkipCRDs && len(crds) > 0 { // On dry run, bail here if isDryRun(i.DryRunStrategy) { - slog.Warn("This chart or one of its subcharts contains CRDs. Rendering may fail or contain inaccuracies.") + i.cfg.Logger().Warn("This chart or one of its subcharts contains CRDs. Rendering may fail or contain inaccuracies.") } else if err := i.installCRDs(crds); err != nil { return nil, err } @@ -313,7 +313,7 @@ func (i *Install) RunWithContext(ctx context.Context, ch ci.Charter, vals map[st mem.SetNamespace(i.Namespace) i.cfg.Releases = storage.Init(mem) } else if interactWithServer(i.DryRunStrategy) && len(i.APIVersions) > 0 { - slog.Debug("API Version list given outside of client only mode, this list will be ignored") + i.cfg.Logger().Debug("API Version list given outside of client only mode, this list will be ignored") } // Make sure if RollbackOnFailure is set, that wait is set as well. This makes it so @@ -539,7 +539,7 @@ func (i *Install) performInstall(rel *release.Release, toBeAdopted kube.Resource // One possible strategy would be to do a timed retry to see if we can get // this stored in the future. if err := i.recordRelease(rel); err != nil { - slog.Error("failed to record the release", slog.Any("error", err)) + i.cfg.Logger().Error("failed to record the release", slog.Any("error", err)) } return rel, nil @@ -548,7 +548,7 @@ func (i *Install) performInstall(rel *release.Release, toBeAdopted kube.Resource func (i *Install) failRelease(rel *release.Release, err error) (*release.Release, error) { rel.SetStatus(rcommon.StatusFailed, fmt.Sprintf("Release %q failed: %s", i.ReleaseName, err.Error())) if i.RollbackOnFailure { - slog.Debug("install failed and rollback-on-failure is set, uninstalling release", "release", i.ReleaseName) + i.cfg.Logger().Debug("install failed and rollback-on-failure is set, uninstalling release", "release", i.ReleaseName) uninstall := NewUninstall(i.cfg) uninstall.DisableHooks = i.DisableHooks uninstall.KeepHistory = false diff --git a/pkg/action/rollback.go b/pkg/action/rollback.go index 992f6979f..f75ca2429 100644 --- a/pkg/action/rollback.go +++ b/pkg/action/rollback.go @@ -19,7 +19,6 @@ package action import ( "bytes" "fmt" - "log/slog" "strings" "time" @@ -76,26 +75,26 @@ func (r *Rollback) Run(name string) error { r.cfg.Releases.MaxHistory = r.MaxHistory - slog.Debug("preparing rollback", "name", name) + r.cfg.Logger().Debug("preparing rollback", "name", name) currentRelease, targetRelease, serverSideApply, err := r.prepareRollback(name) if err != nil { return err } if !isDryRun(r.DryRunStrategy) { - slog.Debug("creating rolled back release", "name", name) + r.cfg.Logger().Debug("creating rolled back release", "name", name) if err := r.cfg.Releases.Create(targetRelease); err != nil { return err } } - slog.Debug("performing rollback", "name", name) + r.cfg.Logger().Debug("performing rollback", "name", name) if _, err := r.performRollback(currentRelease, targetRelease, serverSideApply); err != nil { return err } if !isDryRun(r.DryRunStrategy) { - slog.Debug("updating status for rolled back release", "name", name) + r.cfg.Logger().Debug("updating status for rolled back release", "name", name) if err := r.cfg.Releases.Update(targetRelease); err != nil { return err } @@ -151,7 +150,7 @@ func (r *Rollback) prepareRollback(name string) (*release.Release, *release.Rele return nil, nil, false, fmt.Errorf("release has no %d version", previousVersion) } - slog.Debug("rolling back", "name", name, "currentVersion", currentRelease.Version, "targetVersion", previousVersion) + r.cfg.Logger().Debug("rolling back", "name", name, "currentVersion", currentRelease.Version, "targetVersion", previousVersion) previousReleasei, err := r.cfg.Releases.Get(name, previousVersion) if err != nil { @@ -194,7 +193,7 @@ func (r *Rollback) prepareRollback(name string) (*release.Release, *release.Rele func (r *Rollback) performRollback(currentRelease, targetRelease *release.Release, serverSideApply bool) (*release.Release, error) { if isDryRun(r.DryRunStrategy) { - slog.Debug("dry run", "name", targetRelease.Name) + r.cfg.Logger().Debug("dry run", "name", targetRelease.Name) return targetRelease, nil } @@ -214,7 +213,7 @@ func (r *Rollback) performRollback(currentRelease, targetRelease *release.Releas return targetRelease, err } } else { - slog.Debug("rollback hooks disabled", "name", targetRelease.Name) + r.cfg.Logger().Debug("rollback hooks disabled", "name", targetRelease.Name) } // It is safe to use "forceOwnership" here because these are resources currently rendered by the chart. @@ -232,21 +231,21 @@ func (r *Rollback) performRollback(currentRelease, targetRelease *release.Releas if err != nil { msg := fmt.Sprintf("Rollback %q failed: %s", targetRelease.Name, err) - slog.Warn(msg) + r.cfg.Logger().Warn(msg) currentRelease.Info.Status = common.StatusSuperseded targetRelease.Info.Status = common.StatusFailed targetRelease.Info.Description = msg r.cfg.recordRelease(currentRelease) r.cfg.recordRelease(targetRelease) if r.CleanupOnFail { - slog.Debug("cleanup on fail set, cleaning up resources", "count", len(results.Created)) + r.cfg.Logger().Debug("cleanup on fail set, cleaning up resources", "count", len(results.Created)) _, errs := r.cfg.KubeClient.Delete(results.Created, metav1.DeletePropagationBackground) if errs != nil { return targetRelease, fmt.Errorf( "an error occurred while cleaning up resources. original rollback error: %w", fmt.Errorf("unable to cleanup resources: %w", joinErrors(errs, ", "))) } - slog.Debug("resource cleanup complete") + r.cfg.Logger().Debug("resource cleanup complete") } return targetRelease, err } @@ -288,7 +287,7 @@ func (r *Rollback) performRollback(currentRelease, targetRelease *release.Releas if err != nil { return nil, err } - slog.Debug("superseding previous deployment", "version", rel.Version) + r.cfg.Logger().Debug("superseding previous deployment", "version", rel.Version) rel.Info.Status = common.StatusSuperseded r.cfg.recordRelease(rel) } diff --git a/pkg/action/uninstall.go b/pkg/action/uninstall.go index 4ce6068ec..0cb31be43 100644 --- a/pkg/action/uninstall.go +++ b/pkg/action/uninstall.go @@ -119,7 +119,7 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) return nil, fmt.Errorf("the release named %q is already deleted", name) } - slog.Debug("uninstall: deleting release", "name", name) + u.cfg.Logger().Debug("uninstall: deleting release", "name", name) rel.Info.Status = common.StatusUninstalling rel.Info.Deleted = time.Now() rel.Info.Description = "Deletion in progress (or silently failed)" @@ -131,18 +131,18 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) return res, err } } else { - slog.Debug("delete hooks disabled", "release", name) + u.cfg.Logger().Debug("delete hooks disabled", "release", name) } // From here on out, the release is currently considered to be in StatusUninstalling // state. if err := u.cfg.Releases.Update(rel); err != nil { - slog.Debug("uninstall: Failed to store updated release", slog.Any("error", err)) + u.cfg.Logger().Debug("uninstall: Failed to store updated release", slog.Any("error", err)) } deletedResources, kept, errs := u.deleteRelease(rel) if errs != nil { - slog.Debug("uninstall: Failed to delete release", slog.Any("error", errs)) + u.cfg.Logger().Debug("uninstall: Failed to delete release", slog.Any("error", errs)) return nil, fmt.Errorf("failed to delete release: %s", name) } @@ -170,7 +170,7 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) } if !u.KeepHistory { - slog.Debug("purge requested", "release", name) + u.cfg.Logger().Debug("purge requested", "release", name) err := u.purgeReleases(rels...) if err != nil { errs = append(errs, fmt.Errorf("uninstall: Failed to purge the release: %w", err)) @@ -185,7 +185,7 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) } if err := u.cfg.Releases.Update(rel); err != nil { - slog.Debug("uninstall: Failed to store updated release", slog.Any("error", err)) + u.cfg.Logger().Debug("uninstall: Failed to store updated release", slog.Any("error", err)) } if len(errs) > 0 { diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 932743ce9..aded99dca 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -185,7 +185,7 @@ func (u *Upgrade) RunWithContext(ctx context.Context, name string, ch chart.Char return nil, fmt.Errorf("release name is invalid: %s", name) } - slog.Debug("preparing upgrade", "name", name) + u.cfg.Logger().Debug("preparing upgrade", "name", name) currentRelease, upgradedRelease, serverSideApply, err := u.prepareUpgrade(name, chrt, vals) if err != nil { return nil, err @@ -193,7 +193,7 @@ func (u *Upgrade) RunWithContext(ctx context.Context, name string, ch chart.Char u.cfg.Releases.MaxHistory = u.MaxHistory - slog.Debug("performing update", "name", name) + u.cfg.Logger().Debug("performing update", "name", name) res, err := u.performUpgrade(ctx, currentRelease, upgradedRelease, serverSideApply) if err != nil { return res, err @@ -201,7 +201,7 @@ func (u *Upgrade) RunWithContext(ctx context.Context, name string, ch chart.Char // Do not update for dry runs if !isDryRun(u.DryRunStrategy) { - slog.Debug("updating status for upgraded release", "name", name) + u.cfg.Logger().Debug("updating status for upgraded release", "name", name) if err := u.cfg.Releases.Update(upgradedRelease); err != nil { return res, err } @@ -308,7 +308,7 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chartv2.Chart, vals map[str return nil, nil, false, err } - slog.Debug("determined release apply method", slog.Bool("server_side_apply", serverSideApply), slog.String("previous_release_apply_method", lastRelease.ApplyMethod)) + u.cfg.Logger().Debug("determined release apply method", slog.Bool("server_side_apply", serverSideApply), slog.String("previous_release_apply_method", lastRelease.ApplyMethod)) // Store an upgraded release. upgradedRelease := &release.Release{ @@ -391,7 +391,7 @@ func (u *Upgrade) performUpgrade(ctx context.Context, originalRelease, upgradedR }) if isDryRun(u.DryRunStrategy) { - slog.Debug("dry run for release", "name", upgradedRelease.Name) + u.cfg.Logger().Debug("dry run for release", "name", upgradedRelease.Name) if len(u.Description) > 0 { upgradedRelease.Info.Description = u.Description } else { @@ -400,7 +400,7 @@ func (u *Upgrade) performUpgrade(ctx context.Context, originalRelease, upgradedR return upgradedRelease, nil } - slog.Debug("creating upgraded release", "name", upgradedRelease.Name) + u.cfg.Logger().Debug("creating upgraded release", "name", upgradedRelease.Name) if err := u.cfg.Releases.Create(upgradedRelease); err != nil { return nil, err } @@ -457,7 +457,7 @@ func (u *Upgrade) releasingUpgrade(c chan<- resultMessage, upgradedRelease *rele return } } else { - slog.Debug("upgrade hooks disabled", "name", upgradedRelease.Name) + u.cfg.Logger().Debug("upgrade hooks disabled", "name", upgradedRelease.Name) } upgradeClientSideFieldManager := isReleaseApplyMethodClientSideApply(originalRelease.ApplyMethod) && serverSideApply // Update client-side field manager if transitioning from client-side to server-side apply @@ -515,13 +515,13 @@ func (u *Upgrade) releasingUpgrade(c chan<- resultMessage, upgradedRelease *rele func (u *Upgrade) failRelease(rel *release.Release, created kube.ResourceList, err error) (*release.Release, error) { msg := fmt.Sprintf("Upgrade %q failed: %s", rel.Name, err) - slog.Warn("upgrade failed", "name", rel.Name, slog.Any("error", err)) + u.cfg.Logger().Warn("upgrade failed", "name", rel.Name, slog.Any("error", err)) rel.Info.Status = rcommon.StatusFailed rel.Info.Description = msg u.cfg.recordRelease(rel) if u.CleanupOnFail && len(created) > 0 { - slog.Debug("cleanup on fail set", "cleaning_resources", len(created)) + u.cfg.Logger().Debug("cleanup on fail set", "cleaning_resources", len(created)) _, errs := u.cfg.KubeClient.Delete(created, metav1.DeletePropagationBackground) if errs != nil { return rel, fmt.Errorf( @@ -533,11 +533,11 @@ func (u *Upgrade) failRelease(rel *release.Release, created kube.ResourceList, e ), ) } - slog.Debug("resource cleanup complete") + u.cfg.Logger().Debug("resource cleanup complete") } if u.RollbackOnFailure { - slog.Debug("Upgrade failed and rollback-on-failure is set, rolling back to previous successful release") + u.cfg.Logger().Debug("Upgrade failed and rollback-on-failure is set, rolling back to previous successful release") // As a protection, get the last successful release before rollback. // If there are no successful releases, bail out @@ -594,13 +594,13 @@ func (u *Upgrade) failRelease(rel *release.Release, created kube.ResourceList, e func (u *Upgrade) reuseValues(chart *chartv2.Chart, current *release.Release, newVals map[string]interface{}) (map[string]interface{}, error) { if u.ResetValues { // If ResetValues is set, we completely ignore current.Config. - slog.Debug("resetting values to the chart's original version") + u.cfg.Logger().Debug("resetting values to the chart's original version") return newVals, nil } // If the ReuseValues flag is set, we always copy the old values over the new config's values. if u.ReuseValues { - slog.Debug("reusing the old release's values") + u.cfg.Logger().Debug("reusing the old release's values") // We have to regenerate the old coalesced values: oldVals, err := util.CoalesceValues(current.Chart, current.Config) @@ -617,7 +617,7 @@ func (u *Upgrade) reuseValues(chart *chartv2.Chart, current *release.Release, ne // If the ResetThenReuseValues flag is set, we use the new chart's values, but we copy the old config's values over the new config's values. if u.ResetThenReuseValues { - slog.Debug("merging values from old release to new values") + u.cfg.Logger().Debug("merging values from old release to new values") newVals = util.CoalesceTables(newVals, current.Config) @@ -625,7 +625,7 @@ func (u *Upgrade) reuseValues(chart *chartv2.Chart, current *release.Release, ne } if len(newVals) == 0 && len(current.Config) > 0 { - slog.Debug("copying values from old release", "name", current.Name, "version", current.Version) + u.cfg.Logger().Debug("copying values from old release", "name", current.Name, "version", current.Version) newVals = current.Config } return newVals, nil diff --git a/pkg/cmd/root.go b/pkg/cmd/root.go index 48dbd760d..c23362628 100644 --- a/pkg/cmd/root.go +++ b/pkg/cmd/root.go @@ -103,7 +103,7 @@ By default, the default directories depend on the Operating System. The defaults var settings = cli.New() func NewRootCmd(out io.Writer, args []string, logSetup func(bool)) (*cobra.Command, error) { - actionConfig := new(action.Configuration) + actionConfig := action.NewConfiguration() cmd, err := newRootCmdWithConfig(actionConfig, out, args, logSetup) if err != nil { return nil, err diff --git a/pkg/kube/client.go b/pkg/kube/client.go index 30c014ad5..b290cf721 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -30,6 +30,7 @@ import ( "reflect" "strings" "sync" + "sync/atomic" jsonpatch "github.com/evanphx/json-patch/v5" v1 "k8s.io/api/core/v1" @@ -81,6 +82,8 @@ type Client struct { Factory Factory // Namespace allows to bypass the kubeconfig file for the choice of the namespace Namespace string + // logger is an slog.Logger pointer to use with the kube client + logger atomic.Pointer[slog.Logger] Waiter kubeClient kubernetes.Interface @@ -177,6 +180,7 @@ func New(getter genericclioptions.RESTClientGetter) *Client { c := &Client{ Factory: factory, } + c.SetLogger(slog.Default()) return c } @@ -258,7 +262,7 @@ func ClientCreateOptionFieldValidationDirective(fieldValidationDirective FieldVa // Create creates Kubernetes resources specified in the resource list. func (c *Client) Create(resources ResourceList, options ...ClientCreateOption) (*Result, error) { - slog.Debug("creating resource(s)", "resources", len(resources)) + c.Logger().Debug("creating resource(s)", "resources", len(resources)) createOptions := clientCreateOptions{ serverSideApply: true, // Default to server-side apply @@ -275,11 +279,11 @@ func (c *Client) Create(resources ResourceList, options ...ClientCreateOption) ( makeCreateApplyFunc := func() func(target *resource.Info) error { if createOptions.serverSideApply { - slog.Debug("using server-side apply for resource creation", slog.Bool("forceConflicts", createOptions.forceConflicts), slog.Bool("dryRun", createOptions.dryRun), slog.String("fieldValidationDirective", string(createOptions.fieldValidationDirective))) + c.Logger().Debug("using server-side apply for resource creation", slog.Bool("forceConflicts", createOptions.forceConflicts), slog.Bool("dryRun", createOptions.dryRun), slog.String("fieldValidationDirective", string(createOptions.fieldValidationDirective))) return func(target *resource.Info) error { err := patchResourceServerSide(target, createOptions.dryRun, createOptions.forceConflicts, createOptions.fieldValidationDirective) - logger := slog.With( + logger := c.Logger().With( slog.String("namespace", target.Namespace), slog.String("name", target.Name), slog.String("gvk", target.Mapping.GroupVersionKind.String())) @@ -294,7 +298,7 @@ func (c *Client) Create(resources ResourceList, options ...ClientCreateOption) ( } } - slog.Debug("using client-side apply for resource creation") + c.Logger().Debug("using client-side apply for resource creation") return createResource } @@ -349,7 +353,7 @@ func (c *Client) Get(resources ResourceList, related bool) (map[string][]runtime objs, err = c.getSelectRelationPod(info, objs, isTable, &podSelectors) if err != nil { - slog.Warn("get the relation pod is failed", slog.Any("error", err)) + c.Logger().Warn("get the relation pod is failed", slog.Any("error", err)) } } } @@ -367,7 +371,7 @@ func (c *Client) getSelectRelationPod(info *resource.Info, objs map[string][]run if info == nil { return objs, nil } - slog.Debug("get relation pod of object", "namespace", info.Namespace, "name", info.Name, "kind", info.Mapping.GroupVersionKind.Kind) + c.Logger().Debug("get relation pod of object", "namespace", info.Namespace, "name", info.Name, "kind", info.Mapping.GroupVersionKind.Kind) selector, ok, _ := getSelectorFromObject(info.Object) if !ok { return objs, nil @@ -504,7 +508,7 @@ func (c *Client) update(originals, targets ResourceList, updateApplyFunc UpdateA updateErrors := []error{} res := &Result{} - slog.Debug("checking resources for changes", "resources", len(targets)) + c.Logger().Debug("checking resources for changes", "resources", len(targets)) err := targets.Visit(func(target *resource.Info, err error) error { if err != nil { return err @@ -525,7 +529,7 @@ func (c *Client) update(originals, targets ResourceList, updateApplyFunc UpdateA } kind := target.Mapping.GroupVersionKind.Kind - slog.Debug("created a new resource", "namespace", target.Namespace, "name", target.Name, "kind", kind) + c.Logger().Debug("created a new resource", "namespace", target.Namespace, "name", target.Name, "kind", kind) return nil } @@ -553,22 +557,22 @@ func (c *Client) update(originals, targets ResourceList, updateApplyFunc UpdateA } for _, info := range originals.Difference(targets) { - slog.Debug("deleting resource", "namespace", info.Namespace, "name", info.Name, "kind", info.Mapping.GroupVersionKind.Kind) + c.Logger().Debug("deleting resource", "namespace", info.Namespace, "name", info.Name, "kind", info.Mapping.GroupVersionKind.Kind) if err := info.Get(); err != nil { - slog.Debug("unable to get object", "namespace", info.Namespace, "name", info.Name, "kind", info.Mapping.GroupVersionKind.Kind, slog.Any("error", err)) + c.Logger().Debug("unable to get object", "namespace", info.Namespace, "name", info.Name, "kind", info.Mapping.GroupVersionKind.Kind, slog.Any("error", err)) continue } annotations, err := metadataAccessor.Annotations(info.Object) if err != nil { - slog.Debug("unable to get annotations", "namespace", info.Namespace, "name", info.Name, "kind", info.Mapping.GroupVersionKind.Kind, slog.Any("error", err)) + c.Logger().Debug("unable to get annotations", "namespace", info.Namespace, "name", info.Name, "kind", info.Mapping.GroupVersionKind.Kind, slog.Any("error", err)) } if annotations != nil && annotations[ResourcePolicyAnno] == KeepPolicy { - slog.Debug("skipping delete due to annotation", "namespace", info.Namespace, "name", info.Name, "kind", info.Mapping.GroupVersionKind.Kind, "annotation", ResourcePolicyAnno, "value", KeepPolicy) + c.Logger().Debug("skipping delete due to annotation", "namespace", info.Namespace, "name", info.Name, "kind", info.Mapping.GroupVersionKind.Kind, "annotation", ResourcePolicyAnno, "value", KeepPolicy) continue } if err := deleteResource(info, metav1.DeletePropagationBackground); err != nil { - slog.Debug("failed to delete resource", "namespace", info.Namespace, "name", info.Name, "kind", info.Mapping.GroupVersionKind.Kind, slog.Any("error", err)) + c.Logger().Debug("failed to delete resource", "namespace", info.Namespace, "name", info.Name, "kind", info.Mapping.GroupVersionKind.Kind, slog.Any("error", err)) continue } res.Deleted = append(res.Deleted, info) @@ -706,23 +710,23 @@ func (c *Client) Update(originals, targets ResourceList, options ...ClientUpdate makeUpdateApplyFunc := func() UpdateApplyFunc { if updateOptions.forceReplace { - slog.Debug( + c.Logger().Debug( "using resource replace update strategy", slog.String("fieldValidationDirective", string(updateOptions.fieldValidationDirective))) return func(original, target *resource.Info) error { if err := replaceResource(target, updateOptions.fieldValidationDirective); err != nil { - slog.Debug("error replacing the resource", "namespace", target.Namespace, "name", target.Name, "kind", target.Mapping.GroupVersionKind.Kind, slog.Any("error", err)) + c.Logger().Debug("error replacing the resource", "namespace", target.Namespace, "name", target.Name, "kind", target.Mapping.GroupVersionKind.Kind, slog.Any("error", err)) return err } originalObject := original.Object kind := target.Mapping.GroupVersionKind.Kind - slog.Debug("replace succeeded", "name", original.Name, "initialKind", originalObject.GetObjectKind().GroupVersionKind().Kind, "kind", kind) + c.Logger().Debug("replace succeeded", "name", original.Name, "initialKind", originalObject.GetObjectKind().GroupVersionKind().Kind, "kind", kind) return nil } } else if updateOptions.serverSideApply { - slog.Debug( + c.Logger().Debug( "using server-side apply for resource update", slog.Bool("forceConflicts", updateOptions.forceConflicts), slog.Bool("dryRun", updateOptions.dryRun), @@ -730,7 +734,7 @@ func (c *Client) Update(originals, targets ResourceList, options ...ClientUpdate slog.Bool("upgradeClientSideFieldManager", updateOptions.upgradeClientSideFieldManager)) return func(original, target *resource.Info) error { - logger := slog.With( + logger := c.Logger().With( slog.String("namespace", target.Namespace), slog.String("name", target.Name), slog.String("gvk", target.Mapping.GroupVersionKind.String())) @@ -738,7 +742,7 @@ func (c *Client) Update(originals, targets ResourceList, options ...ClientUpdate if updateOptions.upgradeClientSideFieldManager { patched, err := upgradeClientSideFieldManager(original, updateOptions.dryRun, updateOptions.fieldValidationDirective) if err != nil { - slog.Debug("Error patching resource to replace CSA field management", slog.Any("error", err)) + c.Logger().Debug("Error patching resource to replace CSA field management", slog.Any("error", err)) return err } @@ -758,7 +762,7 @@ func (c *Client) Update(originals, targets ResourceList, options ...ClientUpdate } } - slog.Debug("using client-side apply for resource update", slog.Bool("threeWayMergeForUnstructured", updateOptions.threeWayMergeForUnstructured)) + c.Logger().Debug("using client-side apply for resource update", slog.Bool("threeWayMergeForUnstructured", updateOptions.threeWayMergeForUnstructured)) return func(original, target *resource.Info) error { return patchResourceClientSide(original.Object, target, updateOptions.threeWayMergeForUnstructured) } @@ -776,11 +780,11 @@ func (c *Client) Delete(resources ResourceList, policy metav1.DeletionPropagatio res := &Result{} mtx := sync.Mutex{} err := perform(resources, func(target *resource.Info) error { - slog.Debug("starting delete resource", "namespace", target.Namespace, "name", target.Name, "kind", target.Mapping.GroupVersionKind.Kind) + c.Logger().Debug("starting delete resource", "namespace", target.Namespace, "name", target.Name, "kind", target.Mapping.GroupVersionKind.Kind) err := deleteResource(target, policy) if err == nil || apierrors.IsNotFound(err) { if err != nil { - slog.Debug("ignoring delete failure", "namespace", target.Namespace, "name", target.Name, "kind", target.Mapping.GroupVersionKind.Kind, slog.Any("error", err)) + c.Logger().Debug("ignoring delete failure", "namespace", target.Namespace, "name", target.Name, "kind", target.Mapping.GroupVersionKind.Kind, slog.Any("error", err)) } mtx.Lock() defer mtx.Unlock() @@ -1187,3 +1191,20 @@ func (e *joinedErrors) Error() string { func (e *joinedErrors) Unwrap() []error { return e.errs } + +// logger returns the logger for the Client. If nil, returns slog.Default(). +func (c *Client) Logger() *slog.Logger { + if lg := c.logger.Load(); lg != nil { + return lg + } + return slog.Default() // We rarely get here, just be defensive +} + +func (c *Client) SetLogger(newLogger *slog.Logger) { + // Only set logger if it's currently nil + if newLogger == nil { + c.logger.Store(slog.Default()) // We never want to set the logger to nil + return + } + c.logger.Store(newLogger) +} diff --git a/pkg/storage/driver/cfgmaps.go b/pkg/storage/driver/cfgmaps.go index ada148158..e9093b8aa 100644 --- a/pkg/storage/driver/cfgmaps.go +++ b/pkg/storage/driver/cfgmaps.go @@ -22,6 +22,7 @@ import ( "log/slog" "strconv" "strings" + "sync/atomic" "time" v1 "k8s.io/api/core/v1" @@ -44,14 +45,18 @@ const ConfigMapsDriverName = "ConfigMap" // ConfigMapsInterface. type ConfigMaps struct { impl corev1.ConfigMapInterface + // logger is an slog.Logger pointer to use the driver + logger atomic.Pointer[slog.Logger] } // NewConfigMaps initializes a new ConfigMaps wrapping an implementation of // the kubernetes ConfigMapsInterface. func NewConfigMaps(impl corev1.ConfigMapInterface) *ConfigMaps { - return &ConfigMaps{ + c := &ConfigMaps{ impl: impl, } + c.SetLogger(slog.Default()) + return c } // Name returns the name of the driver. @@ -69,13 +74,13 @@ func (cfgmaps *ConfigMaps) Get(key string) (release.Releaser, error) { return nil, ErrReleaseNotFound } - slog.Debug("failed to get release", "key", key, slog.Any("error", err)) + cfgmaps.Logger().Debug("failed to get release", "key", key, slog.Any("error", err)) return nil, err } // found the configmap, decode the base64 data string r, err := decodeRelease(obj.Data["release"]) if err != nil { - slog.Debug("failed to decode data", "key", key, slog.Any("error", err)) + cfgmaps.Logger().Debug("failed to decode data", "key", key, slog.Any("error", err)) return nil, err } r.Labels = filterSystemLabels(obj.Labels) @@ -92,7 +97,7 @@ func (cfgmaps *ConfigMaps) List(filter func(release.Releaser) bool) ([]release.R list, err := cfgmaps.impl.List(context.Background(), opts) if err != nil { - slog.Debug("failed to list releases", slog.Any("error", err)) + cfgmaps.Logger().Debug("failed to list releases", slog.Any("error", err)) return nil, err } @@ -103,7 +108,7 @@ func (cfgmaps *ConfigMaps) List(filter func(release.Releaser) bool) ([]release.R for _, item := range list.Items { rls, err := decodeRelease(item.Data["release"]) if err != nil { - slog.Debug("failed to decode release", "item", item, slog.Any("error", err)) + cfgmaps.Logger().Debug("failed to decode release", "item", item, slog.Any("error", err)) continue } @@ -131,7 +136,7 @@ func (cfgmaps *ConfigMaps) Query(labels map[string]string) ([]release.Releaser, list, err := cfgmaps.impl.List(context.Background(), opts) if err != nil { - slog.Debug("failed to query with labels", slog.Any("error", err)) + cfgmaps.Logger().Debug("failed to query with labels", slog.Any("error", err)) return nil, err } @@ -143,7 +148,7 @@ func (cfgmaps *ConfigMaps) Query(labels map[string]string) ([]release.Releaser, for _, item := range list.Items { rls, err := decodeRelease(item.Data["release"]) if err != nil { - slog.Debug("failed to decode release", slog.Any("error", err)) + cfgmaps.Logger().Debug("failed to decode release", slog.Any("error", err)) continue } rls.Labels = item.Labels @@ -175,7 +180,7 @@ func (cfgmaps *ConfigMaps) Create(key string, rls release.Releaser) error { // create a new configmap to hold the release obj, err := newConfigMapsObject(key, rel, lbs) if err != nil { - slog.Debug("failed to encode release", "name", rac.Name(), slog.Any("error", err)) + cfgmaps.Logger().Debug("failed to encode release", "name", rac.Name(), slog.Any("error", err)) return err } // push the configmap object out into the kubiverse @@ -184,7 +189,7 @@ func (cfgmaps *ConfigMaps) Create(key string, rls release.Releaser) error { return ErrReleaseExists } - slog.Debug("failed to create release", slog.Any("error", err)) + cfgmaps.Logger().Debug("failed to create release", slog.Any("error", err)) return err } return nil @@ -208,13 +213,13 @@ func (cfgmaps *ConfigMaps) Update(key string, rel release.Releaser) error { // create a new configmap object to hold the release obj, err := newConfigMapsObject(key, rls, lbs) if err != nil { - slog.Debug("failed to encode release", "name", rls.Name, slog.Any("error", err)) + cfgmaps.Logger().Debug("failed to encode release", "name", rls.Name, slog.Any("error", err)) return err } // push the configmap object out into the kubiverse _, err = cfgmaps.impl.Update(context.Background(), obj, metav1.UpdateOptions{}) if err != nil { - slog.Debug("failed to update release", slog.Any("error", err)) + cfgmaps.Logger().Debug("failed to update release", slog.Any("error", err)) return err } return nil @@ -276,3 +281,20 @@ func newConfigMapsObject(key string, rls *rspb.Release, lbs labels) (*v1.ConfigM Data: map[string]string{"release": s}, }, nil } + +// logger returns the logger for the ConfigMaps driver. If nil, returns slog.Default(). +func (cfgmaps *ConfigMaps) Logger() *slog.Logger { + if lg := cfgmaps.logger.Load(); lg != nil { + return lg + } + return slog.Default() // We rarely get here, just be defensive +} + +func (cfgmaps *ConfigMaps) SetLogger(newLogger *slog.Logger) { + // Only set logger if it's currently nil + if newLogger == nil { + cfgmaps.logger.Store(slog.Default()) // We never want to set the logger to nil + return + } + cfgmaps.logger.Store(newLogger) +} diff --git a/pkg/storage/driver/memory.go b/pkg/storage/driver/memory.go index 352fe2c6a..7b25ef74c 100644 --- a/pkg/storage/driver/memory.go +++ b/pkg/storage/driver/memory.go @@ -17,9 +17,11 @@ limitations under the License. package driver import ( + "log/slog" "strconv" "strings" "sync" + "sync/atomic" "helm.sh/helm/v4/pkg/release" ) @@ -42,11 +44,15 @@ type Memory struct { namespace string // A map of namespaces to releases cache map[string]memReleases + // logger is an slog.Logger pointer to use the driver + logger atomic.Pointer[slog.Logger] } // NewMemory initializes a new memory driver. func NewMemory() *Memory { - return &Memory{cache: map[string]memReleases{}, namespace: "default"} + m := &Memory{cache: map[string]memReleases{}, namespace: "default"} + m.SetLogger(slog.Default()) + return m } // SetNamespace sets a specific namespace in which releases will be accessed. @@ -247,3 +253,19 @@ func (mem *Memory) rlock() func() { // ```defer unlock(mem.rlock())```, locks mem for reading at the // call point of defer and unlocks upon exiting the block. func unlock(fn func()) { fn() } + +func (mem *Memory) Logger() *slog.Logger { + if lg := mem.logger.Load(); lg != nil { + return lg + } + return slog.Default() // We rarely get here, just be defensive +} + +func (mem *Memory) SetLogger(newLogger *slog.Logger) { + // Only set logger if it's currently nil + if newLogger == nil { + mem.logger.Store(slog.Default()) // We never want to set the logger to nil + return + } + mem.logger.Store(newLogger) +} diff --git a/pkg/storage/driver/secrets.go b/pkg/storage/driver/secrets.go index 1f5ce75ac..63b21cfdd 100644 --- a/pkg/storage/driver/secrets.go +++ b/pkg/storage/driver/secrets.go @@ -22,6 +22,7 @@ import ( "log/slog" "strconv" "strings" + "sync/atomic" "time" v1 "k8s.io/api/core/v1" @@ -44,14 +45,18 @@ const SecretsDriverName = "Secret" // SecretsInterface. type Secrets struct { impl corev1.SecretInterface + // logger is an slog.Logger pointer to use the driver + logger atomic.Pointer[slog.Logger] } // NewSecrets initializes a new Secrets wrapping an implementation of // the kubernetes SecretsInterface. func NewSecrets(impl corev1.SecretInterface) *Secrets { - return &Secrets{ + s := &Secrets{ impl: impl, } + s.SetLogger(slog.Default()) + return s } // Name returns the name of the driver. @@ -98,7 +103,7 @@ func (secrets *Secrets) List(filter func(release.Releaser) bool) ([]release.Rele for _, item := range list.Items { rls, err := decodeRelease(string(item.Data["release"])) if err != nil { - slog.Debug("list failed to decode release", "key", item.Name, slog.Any("error", err)) + secrets.Logger().Debug("list failed to decode release", "key", item.Name, slog.Any("error", err)) continue } @@ -137,7 +142,7 @@ func (secrets *Secrets) Query(labels map[string]string) ([]release.Releaser, err for _, item := range list.Items { rls, err := decodeRelease(string(item.Data["release"])) if err != nil { - slog.Debug("failed to decode release", "key", item.Name, slog.Any("error", err)) + secrets.Logger().Debug("failed to decode release", "key", item.Name, slog.Any("error", err)) continue } rls.Labels = item.Labels @@ -273,3 +278,20 @@ func newSecretsObject(key string, rls *rspb.Release, lbs labels) (*v1.Secret, er Data: map[string][]byte{"release": []byte(s)}, }, nil } + +// logger returns the logger for the Secrets driver. If nil, returns slog.Default(). +func (secrets *Secrets) Logger() *slog.Logger { + if lg := secrets.logger.Load(); lg != nil { + return lg + } + return slog.Default() // We rarely get here, just be defensive +} + +func (secrets *Secrets) SetLogger(newLogger *slog.Logger) { + // Only set logger if it's currently nil + if newLogger == nil { + secrets.logger.Store(slog.Default()) // We never want to set the logger to nil + return + } + secrets.logger.Store(newLogger) +} diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index 0020d2436..f02dda597 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -22,6 +22,7 @@ import ( "maps" "sort" "strconv" + "sync/atomic" "time" "github.com/jmoiron/sqlx" @@ -89,6 +90,8 @@ type SQL struct { db *sqlx.DB namespace string statementBuilder sq.StatementBuilderType + // logger is an slog.Logger pointer to use the driver + logger atomic.Pointer[slog.Logger] } // Name returns the name of the driver. @@ -109,13 +112,13 @@ func (s *SQL) checkAlreadyApplied(migrations []*migrate.Migration) bool { records, err := migrate.GetMigrationRecords(s.db.DB, postgreSQLDialect) migrate.SetDisableCreateTable(false) if err != nil { - slog.Debug("failed to get migration records", slog.Any("error", err)) + s.Logger().Debug("failed to get migration records", slog.Any("error", err)) return false } for _, record := range records { if _, ok := migrationsIDs[record.Id]; ok { - slog.Debug("found previous migration", "id", record.Id, "appliedAt", record.AppliedAt) + s.Logger().Debug("found previous migration", "id", record.Id, "appliedAt", record.AppliedAt) delete(migrationsIDs, record.Id) } } @@ -123,7 +126,7 @@ func (s *SQL) checkAlreadyApplied(migrations []*migrate.Migration) bool { // check if all migrations applied if len(migrationsIDs) != 0 { for id := range migrationsIDs { - slog.Debug("find unapplied migration", "id", id) + s.Logger().Debug("find unapplied migration", "id", id) } return false } @@ -157,9 +160,9 @@ func (s *SQL) ensureDBSetup() error { CREATE INDEX ON %s (%s); CREATE INDEX ON %s (%s); CREATE INDEX ON %s (%s); - + GRANT ALL ON %s TO PUBLIC; - + ALTER TABLE %s ENABLE ROW LEVEL SECURITY; `, sqlReleaseTableName, @@ -209,7 +212,7 @@ func (s *SQL) ensureDBSetup() error { %s VARCHAR(%d) ); CREATE INDEX ON %s (%s, %s); - + GRANT ALL ON %s TO PUBLIC; ALTER TABLE %s ENABLE ROW LEVEL SECURITY; `, @@ -277,7 +280,7 @@ type SQLReleaseCustomLabelWrapper struct { } // NewSQL initializes a new sql driver. -func NewSQL(connectionString string, namespace string) (*SQL, error) { +func NewSQL(connectionString string, logger *slog.Logger, namespace string) (*SQL, error) { db, err := sqlx.Connect(postgreSQLDialect, connectionString) if err != nil { return nil, err @@ -293,6 +296,7 @@ func NewSQL(connectionString string, namespace string) (*SQL, error) { } driver.namespace = namespace + driver.SetLogger(logger) return driver, nil } @@ -309,24 +313,24 @@ func (s *SQL) Get(key string) (release.Releaser, error) { query, args, err := qb.ToSql() if err != nil { - slog.Debug("failed to build query", slog.Any("error", err)) + s.Logger().Debug("failed to build query", slog.Any("error", err)) return nil, err } // Get will return an error if the result is empty if err := s.db.Get(&record, query, args...); err != nil { - slog.Debug("got SQL error when getting release", "key", key, slog.Any("error", err)) + s.Logger().Debug("got SQL error when getting release", "key", key, slog.Any("error", err)) return nil, ErrReleaseNotFound } release, err := decodeRelease(record.Body) if err != nil { - slog.Debug("failed to decode data", "key", key, slog.Any("error", err)) + s.Logger().Debug("failed to decode data", "key", key, slog.Any("error", err)) return nil, err } if release.Labels, err = s.getReleaseCustomLabels(key, s.namespace); err != nil { - slog.Debug("failed to get release custom labels", "namespace", s.namespace, "key", key, slog.Any("error", err)) + s.Logger().Debug("failed to get release custom labels", "namespace", s.namespace, "key", key, slog.Any("error", err)) return nil, err } @@ -347,13 +351,13 @@ func (s *SQL) List(filter func(release.Releaser) bool) ([]release.Releaser, erro query, args, err := sb.ToSql() if err != nil { - slog.Debug("failed to build query", slog.Any("error", err)) + s.Logger().Debug("failed to build query", slog.Any("error", err)) return nil, err } var records = []SQLReleaseWrapper{} if err := s.db.Select(&records, query, args...); err != nil { - slog.Debug("failed to list", slog.Any("error", err)) + s.Logger().Debug("failed to list", slog.Any("error", err)) return nil, err } @@ -361,12 +365,12 @@ func (s *SQL) List(filter func(release.Releaser) bool) ([]release.Releaser, erro for _, record := range records { release, err := decodeRelease(record.Body) if err != nil { - slog.Debug("failed to decode release", "record", record, slog.Any("error", err)) + s.Logger().Debug("failed to decode release", "record", record, slog.Any("error", err)) continue } if release.Labels, err = s.getReleaseCustomLabels(record.Key, record.Namespace); err != nil { - slog.Debug("failed to get release custom labels", "namespace", record.Namespace, "key", record.Key, slog.Any("error", err)) + s.Logger().Debug("failed to get release custom labels", "namespace", record.Namespace, "key", record.Key, slog.Any("error", err)) return nil, err } maps.Copy(release.Labels, getReleaseSystemLabels(release)) @@ -394,7 +398,7 @@ func (s *SQL) Query(labels map[string]string) ([]release.Releaser, error) { if _, ok := labelMap[key]; ok { sb = sb.Where(sq.Eq{key: labels[key]}) } else { - slog.Debug("unknown label", "key", key) + s.Logger().Debug("unknown label", "key", key) return nil, fmt.Errorf("unknown label %s", key) } } @@ -407,13 +411,13 @@ func (s *SQL) Query(labels map[string]string) ([]release.Releaser, error) { // Build our query query, args, err := sb.ToSql() if err != nil { - slog.Debug("failed to build query", slog.Any("error", err)) + s.Logger().Debug("failed to build query", slog.Any("error", err)) return nil, err } var records = []SQLReleaseWrapper{} if err := s.db.Select(&records, query, args...); err != nil { - slog.Debug("failed to query with labels", slog.Any("error", err)) + s.Logger().Debug("failed to query with labels", slog.Any("error", err)) return nil, err } @@ -425,12 +429,12 @@ func (s *SQL) Query(labels map[string]string) ([]release.Releaser, error) { for _, record := range records { release, err := decodeRelease(record.Body) if err != nil { - slog.Debug("failed to decode release", "record", record, slog.Any("error", err)) + s.Logger().Debug("failed to decode release", "record", record, slog.Any("error", err)) continue } if release.Labels, err = s.getReleaseCustomLabels(record.Key, record.Namespace); err != nil { - slog.Debug("failed to get release custom labels", "namespace", record.Namespace, "key", record.Key, slog.Any("error", err)) + s.Logger().Debug("failed to get release custom labels", "namespace", record.Namespace, "key", record.Key, slog.Any("error", err)) return nil, err } @@ -459,13 +463,13 @@ func (s *SQL) Create(key string, rel release.Releaser) error { body, err := encodeRelease(rls) if err != nil { - slog.Debug("failed to encode release", slog.Any("error", err)) + s.Logger().Debug("failed to encode release", slog.Any("error", err)) return err } transaction, err := s.db.Beginx() if err != nil { - slog.Debug("failed to start SQL transaction", slog.Any("error", err)) + s.Logger().Debug("failed to start SQL transaction", slog.Any("error", err)) return fmt.Errorf("error beginning transaction: %v", err) } @@ -494,7 +498,7 @@ func (s *SQL) Create(key string, rel release.Releaser) error { int(time.Now().Unix()), ).ToSql() if err != nil { - slog.Debug("failed to build insert query", slog.Any("error", err)) + s.Logger().Debug("failed to build insert query", slog.Any("error", err)) return err } @@ -508,17 +512,17 @@ func (s *SQL) Create(key string, rel release.Releaser) error { Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace}). ToSql() if buildErr != nil { - slog.Debug("failed to build select query", "error", buildErr) + s.Logger().Debug("failed to build select query", "error", buildErr) return err } var record SQLReleaseWrapper if err := transaction.Get(&record, selectQuery, args...); err == nil { - slog.Debug("release already exists", "key", key) + s.Logger().Debug("release already exists", "key", key) return ErrReleaseExists } - slog.Debug("failed to store release in SQL database", "key", key, slog.Any("error", err)) + s.Logger().Debug("failed to store release in SQL database", "key", key, slog.Any("error", err)) return err } @@ -541,13 +545,13 @@ func (s *SQL) Create(key string, rel release.Releaser) error { if err != nil { defer transaction.Rollback() - slog.Debug("failed to build insert query", slog.Any("error", err)) + s.Logger().Debug("failed to build insert query", slog.Any("error", err)) return err } if _, err := transaction.Exec(insertLabelsQuery, args...); err != nil { defer transaction.Rollback() - slog.Debug("failed to write Labels", slog.Any("error", err)) + s.Logger().Debug("failed to write Labels", slog.Any("error", err)) return err } } @@ -570,7 +574,7 @@ func (s *SQL) Update(key string, rel release.Releaser) error { body, err := encodeRelease(rls) if err != nil { - slog.Debug("failed to encode release", slog.Any("error", err)) + s.Logger().Debug("failed to encode release", slog.Any("error", err)) return err } @@ -587,12 +591,12 @@ func (s *SQL) Update(key string, rel release.Releaser) error { ToSql() if err != nil { - slog.Debug("failed to build update query", slog.Any("error", err)) + s.Logger().Debug("failed to build update query", slog.Any("error", err)) return err } if _, err := s.db.Exec(query, args...); err != nil { - slog.Debug("failed to update release in SQL database", "key", key, slog.Any("error", err)) + s.Logger().Debug("failed to update release in SQL database", "key", key, slog.Any("error", err)) return err } @@ -603,7 +607,7 @@ func (s *SQL) Update(key string, rel release.Releaser) error { func (s *SQL) Delete(key string) (release.Releaser, error) { transaction, err := s.db.Beginx() if err != nil { - slog.Debug("failed to start SQL transaction", slog.Any("error", err)) + s.Logger().Debug("failed to start SQL transaction", slog.Any("error", err)) return nil, fmt.Errorf("error beginning transaction: %v", err) } @@ -614,20 +618,20 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace}). ToSql() if err != nil { - slog.Debug("failed to build select query", slog.Any("error", err)) + s.Logger().Debug("failed to build select query", slog.Any("error", err)) return nil, err } var record SQLReleaseWrapper err = transaction.Get(&record, selectQuery, args...) if err != nil { - slog.Debug("release not found", "key", key, slog.Any("error", err)) + s.Logger().Debug("release not found", "key", key, slog.Any("error", err)) return nil, ErrReleaseNotFound } release, err := decodeRelease(record.Body) if err != nil { - slog.Debug("failed to decode release", "key", key, slog.Any("error", err)) + s.Logger().Debug("failed to decode release", "key", key, slog.Any("error", err)) transaction.Rollback() return nil, err } @@ -639,18 +643,18 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { Where(sq.Eq{sqlReleaseTableNamespaceColumn: s.namespace}). ToSql() if err != nil { - slog.Debug("failed to build delete query", slog.Any("error", err)) + s.Logger().Debug("failed to build delete query", slog.Any("error", err)) return nil, err } _, err = transaction.Exec(deleteQuery, args...) if err != nil { - slog.Debug("failed perform delete query", slog.Any("error", err)) + s.Logger().Debug("failed perform delete query", slog.Any("error", err)) return release, err } if release.Labels, err = s.getReleaseCustomLabels(key, s.namespace); err != nil { - slog.Debug("failed to get release custom labels", "namespace", s.namespace, "key", key, slog.Any("error", err)) + s.Logger().Debug("failed to get release custom labels", "namespace", s.namespace, "key", key, slog.Any("error", err)) return nil, err } @@ -661,7 +665,7 @@ func (s *SQL) Delete(key string) (release.Releaser, error) { ToSql() if err != nil { - slog.Debug("failed to build delete Labels query", slog.Any("error", err)) + s.Logger().Debug("failed to build delete Labels query", slog.Any("error", err)) return nil, err } _, err = transaction.Exec(deleteCustomLabelsQuery, args...) @@ -702,3 +706,20 @@ func getReleaseSystemLabels(rls *rspb.Release) map[string]string { "version": strconv.Itoa(rls.Version), } } + +// logger returns the logger for the SQL driver. If nil, returns slog.Default(). +func (s *SQL) Logger() *slog.Logger { + if lg := s.logger.Load(); lg != nil { + return lg + } + return slog.Default() // We rarely get here, just be defensive +} + +func (s *SQL) SetLogger(newLogger *slog.Logger) { + // Only set logger if it's currently nil + if newLogger == nil { + s.logger.Store(slog.Default()) // We never want to set the logger to nil + return + } + s.logger.Store(newLogger) +} diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 4603a1de6..86c5428df 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -21,6 +21,7 @@ import ( "fmt" "log/slog" "strings" + "sync/atomic" "helm.sh/helm/v4/pkg/release" "helm.sh/helm/v4/pkg/release/common" @@ -44,13 +45,16 @@ type Storage struct { // be retained, including the most recent release. Values of 0 or less are // ignored (meaning no limits are imposed). MaxHistory int + + // logger is an slog.Logger pointer to use the storage engine + logger atomic.Pointer[slog.Logger] } // Get retrieves the release from storage. An error is returned // if the storage driver failed to fetch the release, or the // release identified by the key, version pair does not exist. func (s *Storage) Get(name string, version int) (release.Releaser, error) { - slog.Debug("getting release", "key", makeKey(name, version)) + s.Logger().Debug("getting release", "key", makeKey(name, version)) return s.Driver.Get(makeKey(name, version)) } @@ -62,7 +66,7 @@ func (s *Storage) Create(rls release.Releaser) error { if err != nil { return err } - slog.Debug("creating release", "key", makeKey(rac.Name(), rac.Version())) + s.Logger().Debug("creating release", "key", makeKey(rac.Name(), rac.Version())) if s.MaxHistory > 0 { // Want to make space for one more release. if err := s.removeLeastRecent(rac.Name(), s.MaxHistory-1); err != nil && @@ -81,7 +85,7 @@ func (s *Storage) Update(rls release.Releaser) error { if err != nil { return err } - slog.Debug("updating release", "key", makeKey(rac.Name(), rac.Version())) + s.Logger().Debug("updating release", "key", makeKey(rac.Name(), rac.Version())) return s.Driver.Update(makeKey(rac.Name(), rac.Version()), rls) } @@ -89,14 +93,14 @@ func (s *Storage) Update(rls release.Releaser) error { // the storage backend fails to delete the release or if the release // does not exist. func (s *Storage) Delete(name string, version int) (release.Releaser, error) { - slog.Debug("deleting release", "key", makeKey(name, version)) + s.Logger().Debug("deleting release", "key", makeKey(name, version)) return s.Driver.Delete(makeKey(name, version)) } // ListReleases returns all releases from storage. An error is returned if the // storage backend fails to retrieve the releases. func (s *Storage) ListReleases() ([]release.Releaser, error) { - slog.Debug("listing all releases in storage") + s.Logger().Debug("listing all releases in storage") return s.List(func(_ release.Releaser) bool { return true }) } @@ -118,13 +122,13 @@ func releaserToV1Release(rel release.Releaser) (*rspb.Release, error) { // ListUninstalled returns all releases with Status == UNINSTALLED. An error is returned // if the storage backend fails to retrieve the releases. func (s *Storage) ListUninstalled() ([]release.Releaser, error) { - slog.Debug("listing uninstalled releases in storage") + s.Logger().Debug("listing uninstalled releases in storage") return s.List(func(rls release.Releaser) bool { rel, err := releaserToV1Release(rls) if err != nil { // This will only happen if calling code does not pass the proper types. This is // a problem with the application and not user data. - slog.Error("unable to convert release to typed release", slog.Any("error", err)) + s.Logger().Error("unable to convert release to typed release", slog.Any("error", err)) panic(fmt.Sprintf("unable to convert release to typed release: %s", err)) } return relutil.StatusFilter(common.StatusUninstalled).Check(rel) @@ -134,13 +138,13 @@ func (s *Storage) ListUninstalled() ([]release.Releaser, error) { // ListDeployed returns all releases with Status == DEPLOYED. An error is returned // if the storage backend fails to retrieve the releases. func (s *Storage) ListDeployed() ([]release.Releaser, error) { - slog.Debug("listing all deployed releases in storage") + s.Logger().Debug("listing all deployed releases in storage") return s.List(func(rls release.Releaser) bool { rel, err := releaserToV1Release(rls) if err != nil { // This will only happen if calling code does not pass the proper types. This is // a problem with the application and not user data. - slog.Error("unable to convert release to typed release", slog.Any("error", err)) + s.Logger().Error("unable to convert release to typed release", slog.Any("error", err)) panic(fmt.Sprintf("unable to convert release to typed release: %s", err)) } return relutil.StatusFilter(common.StatusDeployed).Check(rel) @@ -187,7 +191,7 @@ func releaseListToV1List(ls []release.Releaser) ([]*rspb.Release, error) { // DeployedAll returns all deployed releases with the provided name, or // returns driver.NewErrNoDeployedReleases if not found. func (s *Storage) DeployedAll(name string) ([]release.Releaser, error) { - slog.Debug("getting deployed releases", "name", name) + s.Logger().Debug("getting deployed releases", "name", name) ls, err := s.Query(map[string]string{ "name": name, @@ -206,7 +210,7 @@ func (s *Storage) DeployedAll(name string) ([]release.Releaser, error) { // History returns the revision history for the release with the provided name, or // returns driver.ErrReleaseNotFound if no such release name exists. func (s *Storage) History(name string) ([]release.Releaser, error) { - slog.Debug("getting release history", "name", name) + s.Logger().Debug("getting release history", "name", name) return s.Query(map[string]string{"name": name, "owner": "helm"}) } @@ -274,7 +278,7 @@ func (s *Storage) removeLeastRecent(name string, maximum int) error { } } - slog.Debug("pruned records", "count", len(toDelete), "release", name, "errors", len(errs)) + s.Logger().Debug("pruned records", "count", len(toDelete), "release", name, "errors", len(errs)) switch c := len(errs); c { case 0: return nil @@ -289,7 +293,7 @@ func (s *Storage) deleteReleaseVersion(name string, version int) error { key := makeKey(name, version) _, err := s.Delete(name, version) if err != nil { - slog.Debug("error pruning release", "key", key, slog.Any("error", err)) + s.Logger().Debug("error pruning release", "key", key, slog.Any("error", err)) return err } return nil @@ -297,7 +301,7 @@ func (s *Storage) deleteReleaseVersion(name string, version int) error { // Last fetches the last revision of the named release. func (s *Storage) Last(name string) (release.Releaser, error) { - slog.Debug("getting last revision", "name", name) + s.Logger().Debug("getting last revision", "name", name) h, err := s.History(name) if err != nil { return nil, err @@ -331,7 +335,26 @@ func Init(d driver.Driver) *Storage { if d == nil { d = driver.NewMemory() } - return &Storage{ + s := &Storage{ Driver: d, } + s.SetLogger(slog.Default()) + return s +} + +// logger returns the logger for the Storage. If nil, returns slog.Default(). +func (s *Storage) Logger() *slog.Logger { + if lg := s.logger.Load(); lg != nil { + return lg + } + return slog.Default() // We rarely get here, just be defensive +} + +func (s *Storage) SetLogger(newLogger *slog.Logger) { + // Only set logger if it's currently nil + if newLogger == nil { + s.logger.Store(slog.Default()) // We never want to set the logger to nil + return + } + s.logger.Store(newLogger) } From 7a5816b106ae2aca4dc952ca580c09979177d65b Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Tue, 21 Oct 2025 23:53:09 +0100 Subject: [PATCH 02/14] Self review changes Signed-off-by: Evans Mungai --- pkg/action/action.go | 1 - pkg/kube/client.go | 2 +- pkg/storage/driver/cfgmaps.go | 2 +- pkg/storage/driver/memory.go | 2 +- pkg/storage/driver/secrets.go | 2 +- pkg/storage/driver/sql.go | 6 +++--- pkg/storage/storage.go | 2 +- 7 files changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index 4641b7427..cde915989 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -530,7 +530,6 @@ func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namesp case "sql": d, err := driver.NewSQL( os.Getenv("HELM_DRIVER_SQL_CONNECTION_STRING"), - cfg.Logger(), namespace, ) if err != nil { diff --git a/pkg/kube/client.go b/pkg/kube/client.go index a8023516a..88456ca23 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -1204,7 +1204,7 @@ func (c *Client) Logger() *slog.Logger { if lg := c.logger.Load(); lg != nil { return lg } - return slog.Default() // We rarely get here, just be defensive + return slog.Default() // We rarely get here, just being defensive } func (c *Client) SetLogger(newLogger *slog.Logger) { diff --git a/pkg/storage/driver/cfgmaps.go b/pkg/storage/driver/cfgmaps.go index e9093b8aa..73934a94b 100644 --- a/pkg/storage/driver/cfgmaps.go +++ b/pkg/storage/driver/cfgmaps.go @@ -287,7 +287,7 @@ func (cfgmaps *ConfigMaps) Logger() *slog.Logger { if lg := cfgmaps.logger.Load(); lg != nil { return lg } - return slog.Default() // We rarely get here, just be defensive + return slog.Default() // We rarely get here, just being defensive } func (cfgmaps *ConfigMaps) SetLogger(newLogger *slog.Logger) { diff --git a/pkg/storage/driver/memory.go b/pkg/storage/driver/memory.go index 7b25ef74c..ed9444260 100644 --- a/pkg/storage/driver/memory.go +++ b/pkg/storage/driver/memory.go @@ -258,7 +258,7 @@ func (mem *Memory) Logger() *slog.Logger { if lg := mem.logger.Load(); lg != nil { return lg } - return slog.Default() // We rarely get here, just be defensive + return slog.Default() // We rarely get here, just being defensive } func (mem *Memory) SetLogger(newLogger *slog.Logger) { diff --git a/pkg/storage/driver/secrets.go b/pkg/storage/driver/secrets.go index 63b21cfdd..7cdddb585 100644 --- a/pkg/storage/driver/secrets.go +++ b/pkg/storage/driver/secrets.go @@ -284,7 +284,7 @@ func (secrets *Secrets) Logger() *slog.Logger { if lg := secrets.logger.Load(); lg != nil { return lg } - return slog.Default() // We rarely get here, just be defensive + return slog.Default() // We rarely get here, just being defensive } func (secrets *Secrets) SetLogger(newLogger *slog.Logger) { diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index f02dda597..8a0c7789b 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -280,7 +280,7 @@ type SQLReleaseCustomLabelWrapper struct { } // NewSQL initializes a new sql driver. -func NewSQL(connectionString string, logger *slog.Logger, namespace string) (*SQL, error) { +func NewSQL(connectionString string, namespace string) (*SQL, error) { db, err := sqlx.Connect(postgreSQLDialect, connectionString) if err != nil { return nil, err @@ -296,7 +296,7 @@ func NewSQL(connectionString string, logger *slog.Logger, namespace string) (*SQ } driver.namespace = namespace - driver.SetLogger(logger) + driver.SetLogger(slog.Default()) return driver, nil } @@ -712,7 +712,7 @@ func (s *SQL) Logger() *slog.Logger { if lg := s.logger.Load(); lg != nil { return lg } - return slog.Default() // We rarely get here, just be defensive + return slog.Default() // We rarely get here, just being defensive } func (s *SQL) SetLogger(newLogger *slog.Logger) { diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 86c5428df..290bca2ba 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -347,7 +347,7 @@ func (s *Storage) Logger() *slog.Logger { if lg := s.logger.Load(); lg != nil { return lg } - return slog.Default() // We rarely get here, just be defensive + return slog.Default() // We rarely get here, just being defensive } func (s *Storage) SetLogger(newLogger *slog.Logger) { From 50e43f401719c4c5bcd1ddaf953726a460725539 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 22 Oct 2025 00:40:03 +0100 Subject: [PATCH 03/14] nil logger should be handled by discard handler Signed-off-by: Evans Mungai --- pkg/action/action.go | 3 +-- pkg/kube/client.go | 3 +-- pkg/storage/driver/cfgmaps.go | 3 +-- pkg/storage/driver/memory.go | 3 +-- pkg/storage/driver/secrets.go | 3 +-- pkg/storage/driver/sql.go | 3 +-- pkg/storage/storage.go | 3 +-- 7 files changed, 7 insertions(+), 14 deletions(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index cde915989..92ca778cb 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -564,9 +564,8 @@ func (cfg *Configuration) Logger() *slog.Logger { // SetLogger sets the logger for the Configuration. If nil, sets the default logger. func (cfg *Configuration) SetLogger(newLogger *slog.Logger) { - // Only set logger if it's currently nil if newLogger == nil { - cfg.logger.Store(slog.Default()) // We never want to set the logger to nil + cfg.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs return } cfg.logger.Store(newLogger) diff --git a/pkg/kube/client.go b/pkg/kube/client.go index 88456ca23..cdd95d66f 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -1208,9 +1208,8 @@ func (c *Client) Logger() *slog.Logger { } func (c *Client) SetLogger(newLogger *slog.Logger) { - // Only set logger if it's currently nil if newLogger == nil { - c.logger.Store(slog.Default()) // We never want to set the logger to nil + c.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs return } c.logger.Store(newLogger) diff --git a/pkg/storage/driver/cfgmaps.go b/pkg/storage/driver/cfgmaps.go index 73934a94b..4d356a2c4 100644 --- a/pkg/storage/driver/cfgmaps.go +++ b/pkg/storage/driver/cfgmaps.go @@ -291,9 +291,8 @@ func (cfgmaps *ConfigMaps) Logger() *slog.Logger { } func (cfgmaps *ConfigMaps) SetLogger(newLogger *slog.Logger) { - // Only set logger if it's currently nil if newLogger == nil { - cfgmaps.logger.Store(slog.Default()) // We never want to set the logger to nil + cfgmaps.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs return } cfgmaps.logger.Store(newLogger) diff --git a/pkg/storage/driver/memory.go b/pkg/storage/driver/memory.go index ed9444260..dc950df36 100644 --- a/pkg/storage/driver/memory.go +++ b/pkg/storage/driver/memory.go @@ -262,9 +262,8 @@ func (mem *Memory) Logger() *slog.Logger { } func (mem *Memory) SetLogger(newLogger *slog.Logger) { - // Only set logger if it's currently nil if newLogger == nil { - mem.logger.Store(slog.Default()) // We never want to set the logger to nil + mem.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs return } mem.logger.Store(newLogger) diff --git a/pkg/storage/driver/secrets.go b/pkg/storage/driver/secrets.go index 7cdddb585..d4a18e966 100644 --- a/pkg/storage/driver/secrets.go +++ b/pkg/storage/driver/secrets.go @@ -288,9 +288,8 @@ func (secrets *Secrets) Logger() *slog.Logger { } func (secrets *Secrets) SetLogger(newLogger *slog.Logger) { - // Only set logger if it's currently nil if newLogger == nil { - secrets.logger.Store(slog.Default()) // We never want to set the logger to nil + secrets.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs return } secrets.logger.Store(newLogger) diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index 8a0c7789b..df7d18ae2 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -716,9 +716,8 @@ func (s *SQL) Logger() *slog.Logger { } func (s *SQL) SetLogger(newLogger *slog.Logger) { - // Only set logger if it's currently nil if newLogger == nil { - s.logger.Store(slog.Default()) // We never want to set the logger to nil + s.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs return } s.logger.Store(newLogger) diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 290bca2ba..dd8218897 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -351,9 +351,8 @@ func (s *Storage) Logger() *slog.Logger { } func (s *Storage) SetLogger(newLogger *slog.Logger) { - // Only set logger if it's currently nil if newLogger == nil { - s.logger.Store(slog.Default()) // We never want to set the logger to nil + s.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs return } s.logger.Store(newLogger) From 9c32e34d60f20498e513935076e38af3b53169d9 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 22 Oct 2025 13:29:37 +0100 Subject: [PATCH 04/14] Add logger to sql driver and ensure storage has logger Signed-off-by: Evans Mungai --- internal/logging/logging.go | 8 ++++++++ pkg/action/action.go | 2 +- pkg/storage/storage.go | 10 +++++++++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/internal/logging/logging.go b/internal/logging/logging.go index 2e8208d08..8fd3082b5 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -85,3 +85,11 @@ func NewLogger(debugEnabled DebugEnabledFunc) *slog.Logger { return slog.New(dynamicHandler) } + +// LoggerSetterGetter is an interface that can set and get a logger +type LoggerSetterGetter interface { + // SetLogger sets the logger for the object + SetLogger(logger *slog.Logger) + // Logger returns the logger for the object + Logger() *slog.Logger +} diff --git a/pkg/action/action.go b/pkg/action/action.go index 92ca778cb..75575b3f3 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -535,6 +535,7 @@ func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namesp if err != nil { return fmt.Errorf("unable to instantiate SQL driver: %w", err) } + d.SetLogger(cfg.Logger()) store = storage.Init(d) default: return fmt.Errorf("unknown driver %q", helmDriver) @@ -543,7 +544,6 @@ func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namesp cfg.RESTClientGetter = getter cfg.KubeClient = kc cfg.Releases = store - cfg.Releases.SetLogger(cfg.Logger()) cfg.HookOutputFunc = func(_, _, _ string) io.Writer { return io.Discard } return nil diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index dd8218897..e55c628cf 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -23,6 +23,7 @@ import ( "strings" "sync/atomic" + "helm.sh/helm/v4/internal/logging" "helm.sh/helm/v4/pkg/release" "helm.sh/helm/v4/pkg/release/common" rspb "helm.sh/helm/v4/pkg/release/v1" @@ -338,7 +339,14 @@ func Init(d driver.Driver) *Storage { s := &Storage{ Driver: d, } - s.SetLogger(slog.Default()) + + // Get logger from driver if it implements the LoggerSetterGetter interface + if ls, ok := d.(logging.LoggerSetterGetter); ok { + ls.SetLogger(s.Logger()) + } else { + // If the driver does not implement the LoggerSetterGetter interface, set the default logger + s.SetLogger(slog.Default()) + } return s } From 5ab4ca5490d34307b0d322b6ba76a645d274a78e Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 22 Oct 2025 13:55:12 +0100 Subject: [PATCH 05/14] Embed logging functionality to DRY code Signed-off-by: Evans Mungai --- internal/logging/logging.go | 26 ++++++++++++++++++++++++++ pkg/action/action.go | 25 ++++--------------------- pkg/kube/client.go | 24 +++++------------------- pkg/storage/driver/cfgmaps.go | 23 ++++------------------- pkg/storage/driver/memory.go | 21 +++------------------ pkg/storage/driver/secrets.go | 22 +++------------------- pkg/storage/driver/sql.go | 22 +++------------------- pkg/storage/storage.go | 21 ++------------------- 8 files changed, 50 insertions(+), 134 deletions(-) diff --git a/internal/logging/logging.go b/internal/logging/logging.go index 8fd3082b5..0e95a9875 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -20,6 +20,7 @@ import ( "context" "log/slog" "os" + "sync/atomic" ) // DebugEnabledFunc is a function type that determines if debug logging is enabled @@ -93,3 +94,28 @@ type LoggerSetterGetter interface { // Logger returns the logger for the object Logger() *slog.Logger } + +type LogHolder struct { + // logger is an slog.Logger pointer to use the driver + logger atomic.Pointer[slog.Logger] +} + +// Logger returns the logger for the LogHolder. If nil, returns slog.Default(). +func (l *LogHolder) Logger() *slog.Logger { + if lg := l.logger.Load(); lg != nil { + return lg + } + return slog.Default() // We rarely get here, just being defensive +} + +// SetLogger sets the logger for the LogHolder. If nil, sets the default logger. +func (l *LogHolder) SetLogger(newLogger *slog.Logger) { + if newLogger == nil { + l.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs + return + } + l.logger.Store(newLogger) +} + +// Ensure LogHolder implements LoggerSetterGetter +var _ LoggerSetterGetter = &LogHolder{} diff --git a/pkg/action/action.go b/pkg/action/action.go index 75575b3f3..446dcd79d 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -29,7 +29,6 @@ import ( "slices" "strings" "sync" - "sync/atomic" "text/template" "time" @@ -41,6 +40,7 @@ import ( "sigs.k8s.io/kustomize/kyaml/kio" kyaml "sigs.k8s.io/kustomize/kyaml/yaml" + "helm.sh/helm/v4/internal/logging" "helm.sh/helm/v4/pkg/chart/common" chart "helm.sh/helm/v4/pkg/chart/v2" chartutil "helm.sh/helm/v4/pkg/chart/v2/util" @@ -110,11 +110,11 @@ 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 - // logger is an slog.Logger pointer to use with the Configuration instance - logger atomic.Pointer[slog.Logger] - // Mutex is an exclusive lock for concurrent access to the action mutex sync.Mutex + + // Embed a LogHolder to provide logger functionality + logging.LogHolder } func NewConfiguration() *Configuration { @@ -554,23 +554,6 @@ func (cfg *Configuration) SetHookOutputFunc(hookOutputFunc func(_, _, _ string) cfg.HookOutputFunc = hookOutputFunc } -// Logger returns the logger for the Configuration. If nil, returns slog.Default(). -func (cfg *Configuration) Logger() *slog.Logger { - if lg := cfg.logger.Load(); lg != nil { - return lg - } - return slog.Default() // We rarely get here, just be defensive -} - -// SetLogger sets the logger for the Configuration. If nil, sets the default logger. -func (cfg *Configuration) SetLogger(newLogger *slog.Logger) { - if newLogger == nil { - cfg.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs - return - } - cfg.logger.Store(newLogger) -} - func determineReleaseSSApplyMethod(serverSideApply bool) release.ApplyMethod { if serverSideApply { return release.ApplyMethodServerSideApply diff --git a/pkg/kube/client.go b/pkg/kube/client.go index cdd95d66f..9fe26bcc9 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -30,7 +30,6 @@ import ( "reflect" "strings" "sync" - "sync/atomic" jsonpatch "github.com/evanphx/json-patch/v5" v1 "k8s.io/api/core/v1" @@ -40,6 +39,8 @@ import ( "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" + "helm.sh/helm/v4/internal/logging" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -82,11 +83,12 @@ type Client struct { Factory Factory // Namespace allows to bypass the kubeconfig file for the choice of the namespace Namespace string - // logger is an slog.Logger pointer to use with the kube client - logger atomic.Pointer[slog.Logger] Waiter kubeClient kubernetes.Interface + + // Embed a LogHolder to provide logger functionality + logging.LogHolder } var _ Interface = (*Client)(nil) @@ -1198,19 +1200,3 @@ func (e *joinedErrors) Error() string { func (e *joinedErrors) Unwrap() []error { return e.errs } - -// logger returns the logger for the Client. If nil, returns slog.Default(). -func (c *Client) Logger() *slog.Logger { - if lg := c.logger.Load(); lg != nil { - return lg - } - return slog.Default() // We rarely get here, just being defensive -} - -func (c *Client) SetLogger(newLogger *slog.Logger) { - if newLogger == nil { - c.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs - return - } - c.logger.Store(newLogger) -} diff --git a/pkg/storage/driver/cfgmaps.go b/pkg/storage/driver/cfgmaps.go index 4d356a2c4..81e22ef40 100644 --- a/pkg/storage/driver/cfgmaps.go +++ b/pkg/storage/driver/cfgmaps.go @@ -22,7 +22,6 @@ import ( "log/slog" "strconv" "strings" - "sync/atomic" "time" v1 "k8s.io/api/core/v1" @@ -32,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" + "helm.sh/helm/v4/internal/logging" "helm.sh/helm/v4/pkg/release" rspb "helm.sh/helm/v4/pkg/release/v1" ) @@ -45,8 +45,9 @@ const ConfigMapsDriverName = "ConfigMap" // ConfigMapsInterface. type ConfigMaps struct { impl corev1.ConfigMapInterface - // logger is an slog.Logger pointer to use the driver - logger atomic.Pointer[slog.Logger] + + // Embed a LogHolder to provide logger functionality + logging.LogHolder } // NewConfigMaps initializes a new ConfigMaps wrapping an implementation of @@ -281,19 +282,3 @@ func newConfigMapsObject(key string, rls *rspb.Release, lbs labels) (*v1.ConfigM Data: map[string]string{"release": s}, }, nil } - -// logger returns the logger for the ConfigMaps driver. If nil, returns slog.Default(). -func (cfgmaps *ConfigMaps) Logger() *slog.Logger { - if lg := cfgmaps.logger.Load(); lg != nil { - return lg - } - return slog.Default() // We rarely get here, just being defensive -} - -func (cfgmaps *ConfigMaps) SetLogger(newLogger *slog.Logger) { - if newLogger == nil { - cfgmaps.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs - return - } - cfgmaps.logger.Store(newLogger) -} diff --git a/pkg/storage/driver/memory.go b/pkg/storage/driver/memory.go index dc950df36..ec7470aac 100644 --- a/pkg/storage/driver/memory.go +++ b/pkg/storage/driver/memory.go @@ -21,8 +21,8 @@ import ( "strconv" "strings" "sync" - "sync/atomic" + "helm.sh/helm/v4/internal/logging" "helm.sh/helm/v4/pkg/release" ) @@ -44,8 +44,8 @@ type Memory struct { namespace string // A map of namespaces to releases cache map[string]memReleases - // logger is an slog.Logger pointer to use the driver - logger atomic.Pointer[slog.Logger] + // Embed a LogHolder to provide logger functionality + logging.LogHolder } // NewMemory initializes a new memory driver. @@ -253,18 +253,3 @@ func (mem *Memory) rlock() func() { // ```defer unlock(mem.rlock())```, locks mem for reading at the // call point of defer and unlocks upon exiting the block. func unlock(fn func()) { fn() } - -func (mem *Memory) Logger() *slog.Logger { - if lg := mem.logger.Load(); lg != nil { - return lg - } - return slog.Default() // We rarely get here, just being defensive -} - -func (mem *Memory) SetLogger(newLogger *slog.Logger) { - if newLogger == nil { - mem.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs - return - } - mem.logger.Store(newLogger) -} diff --git a/pkg/storage/driver/secrets.go b/pkg/storage/driver/secrets.go index d4a18e966..2c29c436b 100644 --- a/pkg/storage/driver/secrets.go +++ b/pkg/storage/driver/secrets.go @@ -22,7 +22,6 @@ import ( "log/slog" "strconv" "strings" - "sync/atomic" "time" v1 "k8s.io/api/core/v1" @@ -32,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" + "helm.sh/helm/v4/internal/logging" "helm.sh/helm/v4/pkg/release" rspb "helm.sh/helm/v4/pkg/release/v1" ) @@ -45,8 +45,8 @@ const SecretsDriverName = "Secret" // SecretsInterface. type Secrets struct { impl corev1.SecretInterface - // logger is an slog.Logger pointer to use the driver - logger atomic.Pointer[slog.Logger] + // Embed a LogHolder to provide logger functionality + logging.LogHolder } // NewSecrets initializes a new Secrets wrapping an implementation of @@ -278,19 +278,3 @@ func newSecretsObject(key string, rls *rspb.Release, lbs labels) (*v1.Secret, er Data: map[string][]byte{"release": []byte(s)}, }, nil } - -// logger returns the logger for the Secrets driver. If nil, returns slog.Default(). -func (secrets *Secrets) Logger() *slog.Logger { - if lg := secrets.logger.Load(); lg != nil { - return lg - } - return slog.Default() // We rarely get here, just being defensive -} - -func (secrets *Secrets) SetLogger(newLogger *slog.Logger) { - if newLogger == nil { - secrets.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs - return - } - secrets.logger.Store(newLogger) -} diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index df7d18ae2..c1e73e2dd 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -22,7 +22,6 @@ import ( "maps" "sort" "strconv" - "sync/atomic" "time" "github.com/jmoiron/sqlx" @@ -33,6 +32,7 @@ import ( // Import pq for postgres dialect _ "github.com/lib/pq" + "helm.sh/helm/v4/internal/logging" "helm.sh/helm/v4/pkg/release" rspb "helm.sh/helm/v4/pkg/release/v1" ) @@ -90,8 +90,8 @@ type SQL struct { db *sqlx.DB namespace string statementBuilder sq.StatementBuilderType - // logger is an slog.Logger pointer to use the driver - logger atomic.Pointer[slog.Logger] + // Embed a LogHolder to provide logger functionality + logging.LogHolder } // Name returns the name of the driver. @@ -706,19 +706,3 @@ func getReleaseSystemLabels(rls *rspb.Release) map[string]string { "version": strconv.Itoa(rls.Version), } } - -// logger returns the logger for the SQL driver. If nil, returns slog.Default(). -func (s *SQL) Logger() *slog.Logger { - if lg := s.logger.Load(); lg != nil { - return lg - } - return slog.Default() // We rarely get here, just being defensive -} - -func (s *SQL) SetLogger(newLogger *slog.Logger) { - if newLogger == nil { - s.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs - return - } - s.logger.Store(newLogger) -} diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index e55c628cf..d8b141dc9 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -21,7 +21,6 @@ import ( "fmt" "log/slog" "strings" - "sync/atomic" "helm.sh/helm/v4/internal/logging" "helm.sh/helm/v4/pkg/release" @@ -47,8 +46,8 @@ type Storage struct { // ignored (meaning no limits are imposed). MaxHistory int - // logger is an slog.Logger pointer to use the storage engine - logger atomic.Pointer[slog.Logger] + // Embed a LogHolder to provide logger functionality + logging.LogHolder } // Get retrieves the release from storage. An error is returned @@ -349,19 +348,3 @@ func Init(d driver.Driver) *Storage { } return s } - -// logger returns the logger for the Storage. If nil, returns slog.Default(). -func (s *Storage) Logger() *slog.Logger { - if lg := s.logger.Load(); lg != nil { - return lg - } - return slog.Default() // We rarely get here, just being defensive -} - -func (s *Storage) SetLogger(newLogger *slog.Logger) { - if newLogger == nil { - s.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs - return - } - s.logger.Store(newLogger) -} From b6eca1c0f1694e575c3795f5f9d710cc0f386f60 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 22 Oct 2025 15:26:48 +0100 Subject: [PATCH 06/14] Refactor logging functionality to use slog.Handler Signed-off-by: Evans Mungai --- internal/logging/logging.go | 12 ++++++------ pkg/action/action.go | 12 ++++++------ pkg/kube/client.go | 2 +- pkg/storage/driver/cfgmaps.go | 2 +- pkg/storage/driver/memory.go | 2 +- pkg/storage/driver/secrets.go | 2 +- pkg/storage/driver/sql.go | 2 +- pkg/storage/storage.go | 4 ++-- 8 files changed, 19 insertions(+), 19 deletions(-) diff --git a/internal/logging/logging.go b/internal/logging/logging.go index 0e95a9875..1c7f54e89 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -89,14 +89,14 @@ func NewLogger(debugEnabled DebugEnabledFunc) *slog.Logger { // LoggerSetterGetter is an interface that can set and get a logger type LoggerSetterGetter interface { - // SetLogger sets the logger for the object - SetLogger(logger *slog.Logger) - // Logger returns the logger for the object + // SetLogger sets a new slog.Handler + SetLogger(newLogger slog.Handler) + // Logger returns the slog.Logger created from the slog.Handler Logger() *slog.Logger } type LogHolder struct { - // logger is an slog.Logger pointer to use the driver + // logger is an atomic.Pointer[slog.Logger] to store the slog.Logger logger atomic.Pointer[slog.Logger] } @@ -109,12 +109,12 @@ func (l *LogHolder) Logger() *slog.Logger { } // SetLogger sets the logger for the LogHolder. If nil, sets the default logger. -func (l *LogHolder) SetLogger(newLogger *slog.Logger) { +func (l *LogHolder) SetLogger(newLogger slog.Handler) { if newLogger == nil { l.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs return } - l.logger.Store(newLogger) + l.logger.Store(slog.New(newLogger)) } // Ensure LogHolder implements LoggerSetterGetter diff --git a/pkg/action/action.go b/pkg/action/action.go index 446dcd79d..04afdb133 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -119,7 +119,7 @@ type Configuration struct { func NewConfiguration() *Configuration { c := &Configuration{} - c.SetLogger(slog.Default()) + c.SetLogger(slog.Default().Handler()) return c } @@ -494,7 +494,7 @@ func (cfg *Configuration) recordRelease(r *release.Release) { // Init initializes the action configuration func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namespace, helmDriver string) error { kc := kube.New(getter) - kc.SetLogger(cfg.Logger()) + kc.SetLogger(cfg.Logger().Handler()) lazyClient := &lazyClient{ namespace: namespace, @@ -505,11 +505,11 @@ func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namesp switch helmDriver { case "secret", "secrets", "": d := driver.NewSecrets(newSecretClient(lazyClient)) - d.SetLogger(cfg.Logger()) + d.SetLogger(cfg.Logger().Handler()) store = storage.Init(d) case "configmap", "configmaps": d := driver.NewConfigMaps(newConfigMapClient(lazyClient)) - d.SetLogger(cfg.Logger()) + d.SetLogger(cfg.Logger().Handler()) store = storage.Init(d) case "memory": var d *driver.Memory @@ -524,7 +524,7 @@ func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namesp if d == nil { d = driver.NewMemory() } - d.SetLogger(cfg.Logger()) + d.SetLogger(cfg.Logger().Handler()) d.SetNamespace(namespace) store = storage.Init(d) case "sql": @@ -535,7 +535,7 @@ func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namesp if err != nil { return fmt.Errorf("unable to instantiate SQL driver: %w", err) } - d.SetLogger(cfg.Logger()) + d.SetLogger(cfg.Logger().Handler()) store = storage.Init(d) default: return fmt.Errorf("unknown driver %q", helmDriver) diff --git a/pkg/kube/client.go b/pkg/kube/client.go index 9fe26bcc9..4442a0d91 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -182,7 +182,7 @@ func New(getter genericclioptions.RESTClientGetter) *Client { c := &Client{ Factory: factory, } - c.SetLogger(slog.Default()) + c.SetLogger(slog.Default().Handler()) return c } diff --git a/pkg/storage/driver/cfgmaps.go b/pkg/storage/driver/cfgmaps.go index 81e22ef40..5af432d8a 100644 --- a/pkg/storage/driver/cfgmaps.go +++ b/pkg/storage/driver/cfgmaps.go @@ -56,7 +56,7 @@ func NewConfigMaps(impl corev1.ConfigMapInterface) *ConfigMaps { c := &ConfigMaps{ impl: impl, } - c.SetLogger(slog.Default()) + c.SetLogger(slog.Default().Handler()) return c } diff --git a/pkg/storage/driver/memory.go b/pkg/storage/driver/memory.go index ec7470aac..7ea4a014a 100644 --- a/pkg/storage/driver/memory.go +++ b/pkg/storage/driver/memory.go @@ -51,7 +51,7 @@ type Memory struct { // NewMemory initializes a new memory driver. func NewMemory() *Memory { m := &Memory{cache: map[string]memReleases{}, namespace: "default"} - m.SetLogger(slog.Default()) + m.SetLogger(slog.Default().Handler()) return m } diff --git a/pkg/storage/driver/secrets.go b/pkg/storage/driver/secrets.go index 2c29c436b..85f3497e7 100644 --- a/pkg/storage/driver/secrets.go +++ b/pkg/storage/driver/secrets.go @@ -55,7 +55,7 @@ func NewSecrets(impl corev1.SecretInterface) *Secrets { s := &Secrets{ impl: impl, } - s.SetLogger(slog.Default()) + s.SetLogger(slog.Default().Handler()) return s } diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index c1e73e2dd..b6ea3916d 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -296,7 +296,7 @@ func NewSQL(connectionString string, namespace string) (*SQL, error) { } driver.namespace = namespace - driver.SetLogger(slog.Default()) + driver.SetLogger(slog.Default().Handler()) return driver, nil } diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index d8b141dc9..07dc12c7b 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -341,10 +341,10 @@ func Init(d driver.Driver) *Storage { // Get logger from driver if it implements the LoggerSetterGetter interface if ls, ok := d.(logging.LoggerSetterGetter); ok { - ls.SetLogger(s.Logger()) + ls.SetLogger(s.Logger().Handler()) } else { // If the driver does not implement the LoggerSetterGetter interface, set the default logger - s.SetLogger(slog.Default()) + s.SetLogger(slog.Default().Handler()) } return s } From d67b17b6ec966c22afbfc2a6ec5b6f0f32c58813 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 22 Oct 2025 15:30:02 +0100 Subject: [PATCH 07/14] Minor name change Signed-off-by: Evans Mungai --- internal/logging/logging.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/logging/logging.go b/internal/logging/logging.go index 1c7f54e89..2fca0457f 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -90,7 +90,7 @@ func NewLogger(debugEnabled DebugEnabledFunc) *slog.Logger { // LoggerSetterGetter is an interface that can set and get a logger type LoggerSetterGetter interface { // SetLogger sets a new slog.Handler - SetLogger(newLogger slog.Handler) + SetLogger(newHandler slog.Handler) // Logger returns the slog.Logger created from the slog.Handler Logger() *slog.Logger } @@ -109,12 +109,12 @@ func (l *LogHolder) Logger() *slog.Logger { } // SetLogger sets the logger for the LogHolder. If nil, sets the default logger. -func (l *LogHolder) SetLogger(newLogger slog.Handler) { - if newLogger == nil { +func (l *LogHolder) SetLogger(newHandler slog.Handler) { + if newHandler == nil { l.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs return } - l.logger.Store(slog.New(newLogger)) + l.logger.Store(slog.New(newHandler)) } // Ensure LogHolder implements LoggerSetterGetter From 99fe9b3b9b2bb8c924fe1a4aaedfbe7d4077841a Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 22 Oct 2025 16:20:16 +0100 Subject: [PATCH 08/14] Update logging to use a param for the logger Signed-off-by: Evans Mungai --- internal/logging/logging.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/logging/logging.go b/internal/logging/logging.go index 2fca0457f..90cccc0cd 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -97,6 +97,7 @@ type LoggerSetterGetter interface { type LogHolder struct { // logger is an atomic.Pointer[slog.Logger] to store the slog.Logger + // We use atomic.Pointer for thread safety logger atomic.Pointer[slog.Logger] } From e63b4d92cbc5d9d2bca279555eaa86b2a2fe9cbf Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 22 Oct 2025 19:08:41 +0100 Subject: [PATCH 09/14] Update internal/logging/logging.go Co-authored-by: George Jenkins Signed-off-by: Evans Mungai --- internal/logging/logging.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/logging/logging.go b/internal/logging/logging.go index 90cccc0cd..b8faf859e 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -106,7 +106,7 @@ func (l *LogHolder) Logger() *slog.Logger { if lg := l.logger.Load(); lg != nil { return lg } - return slog.Default() // We rarely get here, just being defensive + return slog.New(slog.DiscardHandler) // Should never be reached } // SetLogger sets the logger for the LogHolder. If nil, sets the default logger. From aed687eaa161da939b9052bf1b13371b4f1cdc32 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 22 Oct 2025 19:18:32 +0100 Subject: [PATCH 10/14] Add config options to NewConfiguration() Signed-off-by: Evans Mungai --- pkg/action/action.go | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index 04afdb133..1d16ba7b8 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -117,9 +117,26 @@ type Configuration struct { logging.LogHolder } -func NewConfiguration() *Configuration { +type ConfigurationOption func(c *Configuration) + +// Override the default logging handler +// If unspecified, the default logger will be used +func ConfigurationSetLogger(h slog.Handler) ConfigurationOption { + return func(c *Configuration) { + c.SetLogger(h) + } +} + +func NewConfiguration(options ...ConfigurationOption) *Configuration { c := &Configuration{} - c.SetLogger(slog.Default().Handler()) + for _, o := range options { + o(c) + } + + if c.Logger() == nil { + c.SetLogger(slog.Default().Handler()) + } + return c } From a112bf5aa6a86433f1b1e0702ded21d3a65790ff Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Thu, 23 Oct 2025 09:53:25 +0100 Subject: [PATCH 11/14] Remove non-reachable code Signed-off-by: Evans Mungai --- pkg/action/action.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index 1d16ba7b8..1104386d2 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -133,10 +133,6 @@ func NewConfiguration(options ...ConfigurationOption) *Configuration { o(c) } - if c.Logger() == nil { - c.SetLogger(slog.Default().Handler()) - } - return c } From 28bc76c3643a7c33dd0838a37d43945061735ed9 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Thu, 23 Oct 2025 10:44:17 +0100 Subject: [PATCH 12/14] Add tests for logging LogHolder Signed-off-by: Evans Mungai --- internal/logging/logging_test.go | 115 +++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) create mode 100644 internal/logging/logging_test.go diff --git a/internal/logging/logging_test.go b/internal/logging/logging_test.go new file mode 100644 index 000000000..5d354e330 --- /dev/null +++ b/internal/logging/logging_test.go @@ -0,0 +1,115 @@ +/* +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 logging + +import ( + "bytes" + "log/slog" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestLogHolder_Logger(t *testing.T) { + t.Run("should return new logger with a then set handler", func(t *testing.T) { + holder := &LogHolder{} + buf := &bytes.Buffer{} + handler := slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug}) + + holder.SetLogger(handler) + logger := holder.Logger() + + assert.NotNil(t, logger) + + // Test that the logger works + logger.Info("test message") + assert.Contains(t, buf.String(), "test message") + }) + + t.Run("should return discard - defaultlogger when no handler is set", func(t *testing.T) { + holder := &LogHolder{} + logger := holder.Logger() + + assert.Equal(t, slog.Default().Handler(), logger.Handler()) + }) +} + +func TestLogHolder_SetLogger(t *testing.T) { + t.Run("sets logger with valid handler", func(t *testing.T) { + holder := &LogHolder{} + buf := &bytes.Buffer{} + handler := slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug}) + + holder.SetLogger(handler) + logger := holder.Logger() + + assert.NotNil(t, logger) + + // Compare the handler directly + assert.Equal(t, handler, logger.Handler()) + }) + + t.Run("sets discard logger with nil handler", func(t *testing.T) { + holder := &LogHolder{} + + holder.SetLogger(nil) + logger := holder.Logger() + + assert.NotNil(t, logger) + + assert.Equal(t, slog.Handler(slog.DiscardHandler), logger.Handler()) + }) + + t.Run("can replace existing logger", func(t *testing.T) { + holder := &LogHolder{} + + // Set first logger + buf1 := &bytes.Buffer{} + handler1 := slog.NewTextHandler(buf1, &slog.HandlerOptions{Level: slog.LevelDebug}) + holder.SetLogger(handler1) + + logger1 := holder.Logger() + assert.Equal(t, handler1, logger1.Handler()) + + // Replace with second logger + buf2 := &bytes.Buffer{} + handler2 := slog.NewTextHandler(buf2, &slog.HandlerOptions{Level: slog.LevelDebug}) + holder.SetLogger(handler2) + + logger2 := holder.Logger() + assert.Equal(t, handler2, logger2.Handler()) + }) +} + +func TestLogHolder_InterfaceCompliance(t *testing.T) { + t.Run("implements LoggerSetterGetter interface", func(_ *testing.T) { + var _ LoggerSetterGetter = &LogHolder{} + }) + + t.Run("interface methods work correctly", func(t *testing.T) { + var holder LoggerSetterGetter = &LogHolder{} + + buf := &bytes.Buffer{} + handler := slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug}) + + holder.SetLogger(handler) + logger := holder.Logger() + + assert.NotNil(t, logger) + assert.Equal(t, handler, logger.Handler()) + }) +} From a94878fb8946b04dd9dfaa0d906fe823ff509423 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Thu, 23 Oct 2025 15:42:34 +0100 Subject: [PATCH 13/14] Fix failing test Signed-off-by: Evans Mungai --- internal/logging/logging_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/logging/logging_test.go b/internal/logging/logging_test.go index 5d354e330..75e6c4025 100644 --- a/internal/logging/logging_test.go +++ b/internal/logging/logging_test.go @@ -44,7 +44,7 @@ func TestLogHolder_Logger(t *testing.T) { holder := &LogHolder{} logger := holder.Logger() - assert.Equal(t, slog.Default().Handler(), logger.Handler()) + assert.Equal(t, slog.Handler(slog.DiscardHandler), logger.Handler()) }) } From 2ddeb50fa61c0d0617dd6f130d0c6a43dd5e89f8 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Thu, 23 Oct 2025 18:09:58 +0100 Subject: [PATCH 14/14] Set default logger in Configuration constructor Signed-off-by: Evans Mungai --- pkg/action/action.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/action/action.go b/pkg/action/action.go index 1104386d2..9555006be 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -129,6 +129,8 @@ func ConfigurationSetLogger(h slog.Handler) ConfigurationOption { func NewConfiguration(options ...ConfigurationOption) *Configuration { c := &Configuration{} + c.SetLogger(slog.Default().Handler()) + for _, o := range options { o(c) }