From 3bff902d8f1722a070bb6a4104d629adb3fac049 Mon Sep 17 00:00:00 2001 From: ChadiDridi Date: Thu, 27 Aug 2026 23:33:44 +0100 Subject: [PATCH] fix(action): do not overwrite an existing namespace with --create-namespace --create-namespace is documented as creating the release namespace only when it is not present, and the namespace is not part of the release. It was created with the same apply method as the chart resources, so with server-side apply Helm sent an apply patch for a minimal Namespace object. On a namespace that already exists that patch makes Helm a field manager of a namespace it does not own and strips the labels and annotations set by whoever created it, for example a namespace deployed by another chart. The namespace is now always created client-side, so an existing namespace answers AlreadyExists and is left untouched, regardless of --server-side. ResolveClientCreateOptions is added to pkg/kube so that code wrapping or faking Interface.Create can see how the resources it was handed are meant to be created; Client.Create now resolves its options through the same helper. Closes #31767 Signed-off-by: ChadiDridi --- pkg/action/install.go | 9 ++++- pkg/action/install_test.go | 77 ++++++++++++++++++++++++++++++++++++++ pkg/kube/client.go | 55 ++++++++++++++++++++++----- pkg/kube/client_test.go | 28 ++++++++++++++ 4 files changed, 158 insertions(+), 11 deletions(-) diff --git a/pkg/action/install.go b/pkg/action/install.go index 605c423bc..8647f2481 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -447,9 +447,16 @@ func (i *Install) RunWithContext(ctx context.Context, ch ci.Charter, vals map[st return nil, err } + // The namespace is not part of the release, it is only a precondition for + // installing into it, and --create-namespace is documented as creating it + // only when it is not present. A client-side create is used so that an + // existing namespace reports AlreadyExists and is left untouched. A + // server-side apply would instead make Helm a field manager of a + // namespace it does not own and drop labels and annotations set by + // whoever created it. if _, err := i.cfg.KubeClient.Create( resourceList, - kube.ClientCreateOptionServerSideApply(i.ServerSideApply, false)); err != nil && !apierrors.IsAlreadyExists(err) { + kube.ClientCreateOptionServerSideApply(false, false)); err != nil && !apierrors.IsAlreadyExists(err) { return nil, err } } diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index 2d83abe27..30b69b6c0 100644 --- a/pkg/action/install_test.go +++ b/pkg/action/install_test.go @@ -35,6 +35,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" + v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -43,6 +44,7 @@ import ( "k8s.io/cli-runtime/pkg/resource" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest/fake" + "sigs.k8s.io/yaml" ci "helm.sh/helm/v4/pkg/chart" @@ -1253,3 +1255,78 @@ func TestInstallRelease_WaitOptionsPassedDownstream(t *testing.T) { // Verify that WaitOptions were passed to GetWaiter is.NotEmpty(failer.RecordedWaitOptions, "WaitOptions should be passed to GetWaiter") } + +// namespaceRecordingKubeClient records the resources passed to Create and +// answers with AlreadyExists for the namespace, the way the API server answers +// a client-side create for a namespace that is already there. +type namespaceRecordingKubeClient struct { + kubefake.PrintingKubeClient + createdNamespaces []string + namespaceCreateOpts kube.ClientCreateOptions +} + +// Build returns a resource for the namespace manifest so that the namespace +// reaches Create; everything else keeps the printing client behaviour. +func (c *namespaceRecordingKubeClient) Build(r io.Reader, validate bool) (kube.ResourceList, error) { + manifest, err := io.ReadAll(r) + if err != nil { + return nil, err + } + if !strings.Contains(string(manifest), "kind: Namespace") { + return c.PrintingKubeClient.Build(bytes.NewReader(manifest), validate) + } + + var ns v1.Namespace + if err := yaml.Unmarshal(manifest, &ns); err != nil { + return nil, err + } + return kube.ResourceList{{ + Name: ns.Name, + Mapping: &meta.RESTMapping{ + GroupVersionKind: schema.GroupVersionKind{Version: "v1", Kind: "Namespace"}, + }, + Object: &ns, + }}, nil +} + +func (c *namespaceRecordingKubeClient) Create(resources kube.ResourceList, options ...kube.ClientCreateOption) (*kube.Result, error) { + for _, r := range resources { + if r.Mapping != nil && r.Mapping.GroupVersionKind.Kind == "Namespace" { + resolved, err := kube.ResolveClientCreateOptions(options...) + if err != nil { + return nil, err + } + c.createdNamespaces = append(c.createdNamespaces, r.Name) + c.namespaceCreateOpts = resolved + return nil, apierrors.NewAlreadyExists(schema.GroupResource{Resource: "namespaces"}, r.Name) + } + } + return c.PrintingKubeClient.Create(resources, options...) +} + +// TestInstallReleaseWithCreateNamespaceExisting checks that installing into a +// namespace that already exists succeeds and that Helm only ever asks for the +// namespace to be created, so an existing namespace keeps the labels and +// annotations set by whoever created it. +func TestInstallReleaseWithCreateNamespaceExisting(t *testing.T) { + is := assert.New(t) + req := require.New(t) + + config := actionConfigFixture(t) + recorder := &namespaceRecordingKubeClient{PrintingKubeClient: kubefake.PrintingKubeClient{Out: io.Discard}} + config.KubeClient = recorder + + instAction := installActionWithConfig(config) + instAction.CreateNamespace = true + + res, err := instAction.Run(buildChart(), map[string]any{}) + req.NoError(err, "install into an existing namespace should succeed") + + rel, err := releaserToV1Release(res) + req.NoError(err) + is.Equal("spaced", rel.Namespace) + is.Equal([]string{"spaced"}, recorder.createdNamespaces) + // A server-side apply would make Helm a field manager of the namespace and + // drop the labels and annotations it does not set itself. + is.False(recorder.namespaceCreateOpts.ServerSideApply, "the namespace must be created client-side") +} diff --git a/pkg/kube/client.go b/pkg/kube/client.go index ba881c560..b3c48904d 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -275,6 +275,49 @@ type clientCreateOptions struct { type ClientCreateOption func(*clientCreateOptions) error +// ClientCreateOptions reports the settings a list of ClientCreateOption +// resolves to. It lets callers that wrap or fake Interface.Create see how the +// resources they were handed are meant to be created. +type ClientCreateOptions struct { + ServerSideApply bool + ForceConflicts bool + DryRun bool + FieldValidationDirective FieldValidationDirective +} + +// ResolveClientCreateOptions applies options on top of the defaults used by +// Client.Create and returns the result. +func ResolveClientCreateOptions(options ...ClientCreateOption) (ClientCreateOptions, error) { + resolved, err := resolveClientCreateOptions(options...) + if err != nil { + return ClientCreateOptions{}, err + } + + return ClientCreateOptions{ + ServerSideApply: resolved.serverSideApply, + ForceConflicts: resolved.forceConflicts, + DryRun: resolved.dryRun, + FieldValidationDirective: resolved.fieldValidationDirective, + }, nil +} + +func resolveClientCreateOptions(options ...ClientCreateOption) (clientCreateOptions, error) { + createOptions := clientCreateOptions{ + serverSideApply: true, // Default to server-side apply + fieldValidationDirective: FieldValidationDirectiveStrict, + } + + errs := make([]error, 0, len(options)) + for _, o := range options { + errs = append(errs, o(&createOptions)) + } + if err := errors.Join(errs...); err != nil { + return clientCreateOptions{}, err + } + + return createOptions, nil +} + // ClientCreateOptionServerSideApply enables performing object apply server-side // see: https://kubernetes.io/docs/reference/using-api/server-side-apply/ // @@ -353,16 +396,8 @@ func (c *Client) makeCreateApplyFunc(serverSideApply, forceConflicts, dryRun boo func (c *Client) Create(resources ResourceList, options ...ClientCreateOption) (*Result, error) { c.Logger().Debug("creating resource(s)", "resources", len(resources)) - createOptions := clientCreateOptions{ - serverSideApply: true, // Default to server-side apply - fieldValidationDirective: FieldValidationDirectiveStrict, - } - - errs := make([]error, 0, len(options)) - for _, o := range options { - errs = append(errs, o(&createOptions)) - } - if err := errors.Join(errs...); err != nil { + createOptions, err := resolveClientCreateOptions(options...) + if err != nil { return nil, fmt.Errorf("invalid client create option(s): %w", err) } diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index 86574a3e5..19f5f8096 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -2272,3 +2272,31 @@ func createManifest(t *testing.T, manifest string, require.NoError(t, err) require.NoError(t, fakeClient.Tracker().Create(mapping.Resource, obj, obj.GetNamespace())) } + +func TestResolveClientCreateOptions(t *testing.T) { + t.Run("defaults", func(t *testing.T) { + opts, err := ResolveClientCreateOptions() + require.NoError(t, err) + assert.True(t, opts.ServerSideApply) + assert.False(t, opts.ForceConflicts) + assert.False(t, opts.DryRun) + assert.Equal(t, FieldValidationDirectiveStrict, opts.FieldValidationDirective) + }) + + t.Run("options are applied", func(t *testing.T) { + opts, err := ResolveClientCreateOptions( + ClientCreateOptionServerSideApply(false, false), + ClientCreateOptionDryRun(true), + ClientCreateOptionFieldValidationDirective(FieldValidationDirectiveIgnore), + ) + require.NoError(t, err) + assert.False(t, opts.ServerSideApply) + assert.True(t, opts.DryRun) + assert.Equal(t, FieldValidationDirectiveIgnore, opts.FieldValidationDirective) + }) + + t.Run("invalid combination", func(t *testing.T) { + _, err := ResolveClientCreateOptions(ClientCreateOptionServerSideApply(false, true)) + require.Error(t, err) + }) +}