diff --git a/pkg/action/install.go b/pkg/action/install.go index 6fc919366..899bdd7ac 100644 --- a/pkg/action/install.go +++ b/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 { diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index 2d83abe27..eb0a53767 100644 --- a/pkg/action/install_test.go +++ b/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) diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 85904124b..9b307db8a 100644 --- a/pkg/action/upgrade.go +++ b/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 { diff --git a/pkg/action/upgrade_test.go b/pkg/action/upgrade_test.go index 53419b6a8..cc019d75e 100644 --- a/pkg/action/upgrade_test.go +++ b/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 diff --git a/pkg/kube/client.go b/pkg/kube/client.go index 03a2761f3..d8797717e 100644 --- a/pkg/kube/client.go +++ b/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 }) diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index 60f6b6fa1..231f83178 100644 --- a/pkg/kube/client_test.go +++ b/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 diff --git a/pkg/kube/fake/failing_kube_client.go b/pkg/kube/fake/failing_kube_client.go index 75d0c8de1..2b8e1fe06 100644 --- a/pkg/kube/fake/failing_kube_client.go +++ b/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 }