Merge 769c2724c9 into 62c7e845d1

pkg/action/install.go

@@ -419,8 +419,8 @@ func (i *Install) RunWithContext(ctx context.Context, ch ci.Charter, vals map[st
 		}
 	}
 
-	// Bail out here if it is a dry run
-	if isDryRun(i.DryRunStrategy) {
+	// Bail out early for client-side dry run before any cluster interaction.
+	if isDryRun(i.DryRunStrategy) && i.DryRunStrategy != DryRunServer {
 		rel.Info.Description = "Dry run complete"
 		return rel, nil
 	}
@@ -454,6 +454,36 @@ func (i *Install) RunWithContext(ctx context.Context, ch ci.Charter, vals map[st
 		}
 	}
 
+	// For server-side dry-run, validate resources against the API server after
+	// namespace creation so resources inside that namespace resolve correctly.
+	if i.DryRunStrategy == DryRunServer {
+		if !i.ServerSideApply {
+			return rel, errors.New("--dry-run=server requires --server-side=true")
+		}
+		var err error
+		if len(toBeAdopted) == 0 && len(resources) > 0 {
+			_, err = i.cfg.KubeClient.Create(
+				resources,
+				kube.ClientCreateOptionServerSideApply(true, i.ForceConflicts),
+				kube.ClientCreateOptionDryRun(true),
+			)
+		} else if len(resources) > 0 {
+			_, err = i.cfg.KubeClient.Update(
+				toBeAdopted,
+				resources,
+				kube.ClientUpdateOptionForceReplace(false),
+				kube.ClientUpdateOptionServerSideApply(true, i.ForceConflicts),
+				kube.ClientUpdateOptionDryRun(true),
+				kube.ClientUpdateOptionUpgradeClientSideFieldManager(i.TakeOwnership),
+			)
+		}
+		if err != nil {
+			return rel, err
+		}
+		rel.Info.Description = "Dry run complete"
+		return rel, nil
+	}
+
 	// If Replace is true, we need to supersede the last release.
 	if i.Replace {
 		if err := i.replaceRelease(rel); err != nil {

pkg/action/install_test.go

@@ -451,6 +451,51 @@ func TestInstallRelease_DryRunClient(t *testing.T) {
 	}
 }
 
+func TestInstallRelease_DryRunServerValidation(t *testing.T) {
+	is := assert.New(t)
+	req := require.New(t)
+
+	// Server dry-run with adoption: resources already exist on the cluster;
+	// TakeOwnership bypasses the ownership check and puts them into toBeAdopted,
+	// so the dry-run block exercises the Update path with DryRun=true.
+	config := actionConfigFixtureWithDummyResources(t, createDummyResourceList(false))
+	fakeClient := config.KubeClient.(*kubefake.FailingKubeClient)
+
+	instAction := NewInstall(config)
+	instAction.Namespace = "spaced"
+	instAction.ReleaseName = "test-server-dry-run"
+	instAction.TakeOwnership = true
+
+	expectedErr := errors.New("validation error: unknown field in spec")
+	fakeClient.UpdateError = expectedErr
+	instAction.DryRunStrategy = DryRunServer
+
+	vals := map[string]any{}
+	_, err := instAction.Run(buildChart(withSampleTemplates()), vals)
+
+	req.Error(err)
+	is.Contains(err.Error(), "validation error")
+	// Verify the Update call included DryRun so no real resources are modified.
+	is.True(fakeClient.RecordedUpdateOptions.DryRun, "server dry-run must pass DryRun option to Update")
+
+	// Client dry-run must not reach the kube client at all even when UpdateError is set.
+	config2 := actionConfigFixtureWithDummyResources(t, createDummyResourceList(false))
+	fakeClient2 := config2.KubeClient.(*kubefake.FailingKubeClient)
+	fakeClient2.UpdateError = expectedErr
+
+	instAction2 := NewInstall(config2)
+	instAction2.Namespace = "spaced"
+	instAction2.ReleaseName = "test-client-dry-run"
+	instAction2.DryRunStrategy = DryRunClient
+
+	resi, err := instAction2.Run(buildChart(withSampleTemplates()), vals)
+	req.NoError(err)
+	res, err := releaserToV1Release(resi)
+	req.NoError(err)
+	is.Equal("Dry run complete", res.Info.Description)
+	is.False(fakeClient2.RecordedUpdateOptions.DryRun, "client dry-run must not call Update")
+}
+
 func TestInstallRelease_DryRunHiddenSecret(t *testing.T) {
 	is := assert.New(t)
 	req := require.New(t)

pkg/action/upgrade.go

@@ -397,6 +397,24 @@ func (u *Upgrade) performUpgrade(ctx context.Context, originalRelease, upgradedR
 
 	if isDryRun(u.DryRunStrategy) {
 		u.cfg.Logger().Debug("dry run for release", "name", upgradedRelease.Name)
+		// For server-side dry-run, validate resources against the API server
+		if u.DryRunStrategy == DryRunServer {
+			if !serverSideApply {
+				return upgradedRelease, errors.New("--dry-run=server requires --server-side=true")
+			}
+			upgradeClientSideFieldManager := isReleaseApplyMethodClientSideApply(originalRelease.ApplyMethod)
+			_, err := u.cfg.KubeClient.Update(
+				current,
+				target,
+				kube.ClientUpdateOptionForceReplace(false),
+				kube.ClientUpdateOptionServerSideApply(serverSideApply, u.ForceConflicts),
+				kube.ClientUpdateOptionDryRun(true),
+				kube.ClientUpdateOptionUpgradeClientSideFieldManager(upgradeClientSideFieldManager),
+			)
+			if err != nil {
+				return upgradedRelease, err
+			}
+		}
 		if u.Description != "" {
 			upgradedRelease.Info.Description = u.Description
 		} else {

pkg/action/upgrade_test.go

@@ -619,6 +619,63 @@ func TestUpgradeRelease_DryRun(t *testing.T) {
 	req.Error(err)
 }
 
+func TestUpgradeRelease_DryRunServerValidation(t *testing.T) {
+	is := assert.New(t)
+	req := require.New(t)
+
+	config := actionConfigFixtureWithDummyResources(t, createDummyResourceList(true))
+
+	upAction := NewUpgrade(config)
+	upAction.Namespace = "spaced"
+	upAction.ServerSideApply = "true"
+
+	rel := releaseStub()
+	rel.Name = "test-server-dry-run"
+	rel.Info.Status = common.StatusDeployed
+	rel.ApplyMethod = "ssa"
+	req.NoError(upAction.cfg.Releases.Create(rel))
+
+	expectedErr := errors.New("validation error: unknown field in spec")
+	fakeClient := config.KubeClient.(*kubefake.FailingKubeClient)
+	fakeClient.UpdateError = expectedErr
+	upAction.DryRunStrategy = DryRunServer
+
+	vals := map[string]any{}
+	ctx, done := context.WithCancel(t.Context())
+	_, err := upAction.RunWithContext(ctx, rel.Name, buildChart(), vals)
+	done()
+
+	req.Error(err)
+	is.Contains(err.Error(), "validation error")
+	// Verify the Update call included DryRun so no real resources are modified.
+	is.True(fakeClient.RecordedUpdateOptions.DryRun, "server dry-run must pass DryRun option to Update")
+
+	config2 := actionConfigFixtureWithDummyResources(t, createDummyResourceList(true))
+	config2.KubeClient.(*kubefake.FailingKubeClient).UpdateError = expectedErr
+
+	upAction2 := NewUpgrade(config2)
+	upAction2.Namespace = "spaced"
+
+	rel2 := releaseStub()
+	rel2.Name = "test-client-dry-run"
+	rel2.Info.Status = common.StatusDeployed
+	req.NoError(upAction2.cfg.Releases.Create(rel2))
+
+	upAction2.DryRunStrategy = DryRunClient
+
+	ctx, done = context.WithCancel(t.Context())
+	resi, err := upAction2.RunWithContext(ctx, rel2.Name, buildChart(), vals)
+	done()
+
+	req.NoError(err)
+	res, err := releaserToV1Release(resi)
+	req.NoError(err)
+	is.Equal("Dry run complete", res.Info.Description)
+	// Client dry-run must not reach the kube client Update at all.
+	is.False(config2.KubeClient.(*kubefake.FailingKubeClient).RecordedUpdateOptions.DryRun,
+		"client dry-run must not call Update")
+}
+
 func TestGetUpgradeServerSideValue(t *testing.T) {
 	tests := []struct {
 		name                    string

pkg/kube/client.go

@@ -572,7 +572,7 @@ func (c *Client) BuildTable(reader io.Reader, validate bool) (ResourceList, erro
 		transformRequests)
 }
 
-func (c *Client) update(originals, targets ResourceList, createApplyFunc CreateApplyFunc, updateApplyFunc UpdateApplyFunc) (*Result, error) {
+func (c *Client) update(originals, targets ResourceList, createApplyFunc CreateApplyFunc, updateApplyFunc UpdateApplyFunc, dryRun bool) (*Result, error) {
 	updateErrors := []error{}
 	res := &Result{}
 
@@ -655,7 +655,7 @@ func (c *Client) update(originals, targets ResourceList, createApplyFunc CreateA
 	}
 
 	for _, info := range originals.Difference(targets) {
-		c.Logger().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, "dryRun", dryRun)
 
 		if err := info.Get(); err != nil {
 			c.Logger().Debug(
@@ -681,7 +681,7 @@ func (c *Client) update(originals, targets ResourceList, createApplyFunc CreateA
 			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 {
+		if err := deleteResource(info, metav1.DeletePropagationBackground, dryRun); err != nil {
 			c.Logger().Debug(
 				"failed to delete resource",
 				slog.String("namespace", info.Namespace),
@@ -796,6 +796,43 @@ func ClientUpdateOptionUpgradeClientSideFieldManager(upgradeClientSideFieldManag
 	}
 }
 
+// ParsedCreateOptions holds the resolved values of ClientCreateOption varargs.
+// Intended for use in tests that need to assert which options were passed.
+type ParsedCreateOptions struct {
+	DryRun          bool
+	ServerSideApply bool
+	ForceConflicts  bool
+}
+
+// ParseCreateOptions applies opts and returns the resolved values.
+func ParseCreateOptions(opts []ClientCreateOption) (ParsedCreateOptions, error) {
+	o := clientCreateOptions{serverSideApply: true, fieldValidationDirective: FieldValidationDirectiveStrict}
+	errs := make([]error, 0, len(opts))
+	for _, fn := range opts {
+		errs = append(errs, fn(&o))
+	}
+	return ParsedCreateOptions{DryRun: o.dryRun, ServerSideApply: o.serverSideApply, ForceConflicts: o.forceConflicts}, errors.Join(errs...)
+}
+
+// ParsedUpdateOptions holds the resolved values of ClientUpdateOption varargs.
+// Intended for use in tests that need to assert which options were passed.
+type ParsedUpdateOptions struct {
+	DryRun          bool
+	ServerSideApply bool
+	ForceReplace    bool
+	ForceConflicts  bool
+}
+
+// ParseUpdateOptions applies opts and returns the resolved values.
+func ParseUpdateOptions(opts []ClientUpdateOption) (ParsedUpdateOptions, error) {
+	o := clientUpdateOptions{serverSideApply: true, fieldValidationDirective: FieldValidationDirectiveStrict}
+	errs := make([]error, 0, len(opts))
+	for _, fn := range opts {
+		errs = append(errs, fn(&o))
+	}
+	return ParsedUpdateOptions{DryRun: o.dryRun, ServerSideApply: o.serverSideApply, ForceReplace: o.forceReplace, ForceConflicts: o.forceConflicts}, errors.Join(errs...)
+}
+
 // Update takes the current list of objects and target list of objects and
 // creates resources that don't already exist, updates resources that have been
 // modified in the target configuration, and deletes resources from the current
@@ -831,6 +868,10 @@ func (c *Client) Update(originals, targets ResourceList, options ...ClientUpdate
 		return &Result{}, errors.New("invalid operation: cannot use server-side apply and force replace together")
 	}
 
+	if updateOptions.dryRun && !updateOptions.serverSideApply {
+		return &Result{}, errors.New("invalid operation: dry run requires server-side apply")
+	}
+
 	createApplyFunc := c.makeCreateApplyFunc(
 		updateOptions.serverSideApply,
 		updateOptions.forceConflicts,
@@ -902,7 +943,7 @@ func (c *Client) Update(originals, targets ResourceList, options ...ClientUpdate
 		}
 	}
 
-	return c.update(originals, targets, createApplyFunc, makeUpdateApplyFunc())
+	return c.update(originals, targets, createApplyFunc, makeUpdateApplyFunc(), updateOptions.dryRun)
 }
 
 // Delete deletes Kubernetes resources specified in the resources list with
@@ -915,7 +956,7 @@ func (c *Client) Delete(resources ResourceList, policy metav1.DeletionPropagatio
 	mtx := sync.Mutex{}
 	err := perform(resources, func(target *resource.Info) error {
 		c.Logger().Debug("starting delete resource", "namespace", target.Namespace, "name", target.Name, "kind", target.Mapping.GroupVersionKind.Kind)
-		err := deleteResource(target, policy)
+		err := deleteResource(target, policy, false)
 		if err == nil || apierrors.IsNotFound(err) {
 			if err != nil {
 				c.Logger().Debug(
@@ -1062,11 +1103,14 @@ func createResource(info *resource.Info) error {
 		})
 }
 
-func deleteResource(info *resource.Info, policy metav1.DeletionPropagation) error {
+func deleteResource(info *resource.Info, policy metav1.DeletionPropagation, dryRun bool) error {
 	return retry.RetryOnConflict(
 		retry.DefaultRetry,
 		func() error {
 			opts := &metav1.DeleteOptions{PropagationPolicy: &policy}
+			if dryRun {
+				opts.DryRun = []string{metav1.DryRunAll}
+			}
 			_, err := resource.NewHelper(info.Client, info.Mapping).WithFieldManager(getManagedFieldsManager()).DeleteWithOptions(info.Namespace, info.Name, opts)
 			return err
 		})

pkg/kube/client_test.go

@@ -19,6 +19,7 @@ package kube
 import (
 	"bytes"
 	"context"
+	"encoding/json"
 	"errors"
 	"io"
 	"net/http"
@@ -592,6 +593,87 @@ func TestUpdate(t *testing.T) {
 	}
 }
 
+// TestUpdateDryRunDelete verifies that the dry-run option is propagated to the
+// deletions Update performs when a resource present in the original release is
+// absent from the target. Without this, a server-side dry-run upgrade would
+// really remove pruned resources from the cluster.
+func TestUpdateDryRunDelete(t *testing.T) {
+	for name, dryRun := range map[string]bool{"dry run": true, "normal": false} {
+		t.Run(name, func(t *testing.T) {
+			listOriginal := newPodList("starfish", "squid")
+			listTarget := newPodList("starfish")
+
+			expectedDryRun := ""
+			var expectedDeleteDryRun []string
+			if dryRun {
+				expectedDryRun = "All"
+				expectedDeleteDryRun = []string{metav1.DryRunAll}
+			}
+
+			c := newTestClient(t)
+			cb := func(_ []RequestResponseAction, req *http.Request) (*http.Response, error) {
+				p, m := req.URL.Path, req.Method
+
+				switch {
+				case p == "/namespaces/default/pods/starfish" && m == http.MethodGet:
+					return newResponse(http.StatusOK, &listOriginal.Items[0])
+				case p == "/namespaces/default/pods/starfish" && m == http.MethodPatch:
+					assert.Equal(t, expectedDryRun, req.URL.Query().Get("dryRun"))
+					return newResponse(http.StatusOK, &listTarget.Items[0])
+				case p == "/namespaces/default/pods/squid" && m == http.MethodGet:
+					return newResponse(http.StatusOK, &listOriginal.Items[1])
+				case p == "/namespaces/default/pods/squid" && m == http.MethodDelete:
+					// DeleteOptions are sent in the request body, not the query string
+					defer req.Body.Close()
+					data, err := io.ReadAll(req.Body)
+					require.NoError(t, err)
+
+					opts := metav1.DeleteOptions{}
+					require.NoError(t, json.Unmarshal(data, &opts))
+					assert.Equal(t, expectedDeleteDryRun, opts.DryRun)
+
+					return newResponse(http.StatusOK, &listOriginal.Items[1])
+				}
+
+				t.FailNow()
+				return nil, nil
+			}
+
+			client := NewRequestResponseLogClient(t, cb)
+			c.Factory.(*cmdtesting.TestFactory).UnstructuredClient = &fake.RESTClient{
+				NegotiatedSerializer: unstructuredSerializer,
+				Client:               fake.CreateHTTPClient(client.Do),
+			}
+
+			first, err := c.Build(objBody(&listOriginal), false)
+			require.NoError(t, err)
+
+			second, err := c.Build(objBody(&listTarget), false)
+			require.NoError(t, err)
+
+			result, err := c.Update(
+				first,
+				second,
+				ClientUpdateOptionServerSideApply(true, false),
+				ClientUpdateOptionDryRun(dryRun))
+			require.NoError(t, err)
+
+			assert.Len(t, result.Deleted, 1, "expected 1 resource deleted")
+			actions := []string{}
+			for _, action := range client.Actions {
+				actions = append(actions, action.Request.URL.Path+":"+action.Request.Method)
+			}
+
+			assert.Equal(t, []string{
+				"/namespaces/default/pods/starfish:GET",
+				"/namespaces/default/pods/starfish:PATCH",
+				"/namespaces/default/pods/squid:GET",
+				"/namespaces/default/pods/squid:DELETE",
+			}, actions)
+		})
+	}
+}
+
 func TestBuild(t *testing.T) {
 	tests := []struct {
 		name      string

pkg/kube/fake/failing_kube_client.go

@@ -50,7 +50,11 @@ type FailingKubeClient struct {
 	WaitDuration           time.Duration
 	// RecordedWaitOptions stores the WaitOptions passed to GetWaiter for testing
 	RecordedWaitOptions []kube.WaitOption
-	mu                  sync.Mutex
+	// RecordedCreateOptions stores the parsed options from the last Create call.
+	RecordedCreateOptions kube.ParsedCreateOptions
+	// RecordedUpdateOptions stores the parsed options from the last Update call.
+	RecordedUpdateOptions kube.ParsedUpdateOptions
+	mu                    sync.Mutex
 }
 
 var _ kube.Interface = &FailingKubeClient{}
@@ -67,6 +71,11 @@ type FailingKubeWaiter struct {
 
 // Create returns the configured error if set or prints
 func (f *FailingKubeClient) Create(resources kube.ResourceList, options ...kube.ClientCreateOption) (*kube.Result, error) {
+	if parsed, err := kube.ParseCreateOptions(options); err == nil {
+		f.mu.Lock()
+		f.RecordedCreateOptions = parsed
+		f.mu.Unlock()
+	}
 	if f.CreateError != nil {
 		return nil, f.CreateError
 	}
@@ -125,6 +134,11 @@ func (f *FailingKubeWaiter) WatchUntilReady(resources kube.ResourceList, d time.
 
 // Update returns the configured error if set or prints
 func (f *FailingKubeClient) Update(r, modified kube.ResourceList, options ...kube.ClientUpdateOption) (*kube.Result, error) {
+	if parsed, err := kube.ParseUpdateOptions(options); err == nil {
+		f.mu.Lock()
+		f.RecordedUpdateOptions = parsed
+		f.mu.Unlock()
+	}
 	if f.UpdateError != nil {
 		return &kube.Result{}, f.UpdateError
 	}