From 0a4757359edcdfaad2e19e8ee3518188bd3cfa1b Mon Sep 17 00:00:00 2001 From: Feruzjon Muyassarov Date: Mon, 30 Mar 2026 16:47:34 +0300 Subject: [PATCH] [v3 backport] Fix rollback for missing resources Signed-off-by: Feruzjon Muyassarov --- pkg/kube/client.go | 16 +++++++- pkg/kube/client_test.go | 84 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/pkg/kube/client.go b/pkg/kube/client.go index 6e7142119..13d33a4a0 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -392,7 +392,8 @@ func (c *Client) update(original, target ResourceList, force, threeWayMerge bool } helper := resource.NewHelper(info.Client, info.Mapping).WithFieldManager(getManagedFieldsManager()) - if _, err := helper.Get(info.Namespace, info.Name); err != nil { + currentObj, err := helper.Get(info.Namespace, info.Name) + if err != nil { if !apierrors.IsNotFound(err) { return errors.Wrap(err, "could not get information about the resource") } @@ -413,7 +414,18 @@ func (c *Client) update(original, target ResourceList, force, threeWayMerge bool originalInfo := original.Get(info) if originalInfo == nil { kind := info.Mapping.GroupVersionKind.Kind - return errors.Errorf("no %s with the name %q found", kind, info.Name) + + c.Log("resource %s %q in namespace %q exists on cluster but not in original release, using cluster state as baseline", + kind, info.Name, info.Namespace) + + if err := updateResource(c, info, currentObj, force, threeWayMerge); err != nil { + c.Log("error updating the resource %q:\n\t %v", info.Name, err) + updateErrors = append(updateErrors, err.Error()) + } + // Because we check for errors later, append the info regardless + res.Updated = append(res.Updated, info) + + return nil } if err := updateResource(c, info, originalInfo.Object, force, threeWayMerge); err != nil { diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index 793f9b7c1..65bf33e53 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -346,6 +346,90 @@ func TestUpdateThreeWayMerge(t *testing.T) { testUpdate(t, true) } +// TestUpdateRollbackMissingOriginal tests the rollback scenario where: +// - Revision 1 had "newpod" +// - Revision 2 removed "newpod" but the upgrade failed (original list is empty) +// - Cluster still has "newpod" from Revision 1, but with different spec than the rollback target +// - Rolling back to Revision 1 (target with "newpod") should succeed using cluster state as baseline +// and must issue a real PATCH to reconcile the diff, not silently skip due to an empty patch. +func TestUpdateRollbackMissingOriginal(t *testing.T) { + // target is Revision 1: newpod with port 443 (https). + listTarget := newPodList("newpod") + listTarget.Items[0].Spec.Containers[0].Ports = []v1.ContainerPort{ + {Name: "https", ContainerPort: 443}, + } + + // clusterPod is what the cluster currently has: newpod with port 80 (http). + // It deliberately differs from listTarget so that createPatch produces a + // non-empty patch and the PATCH request is actually issued. + clusterPod := newPod("newpod") + + var actions []string + c := newTestClient(t) + c.Factory.(*cmdtesting.TestFactory).UnstructuredClient = &fake.RESTClient{ + NegotiatedSerializer: unstructuredSerializer, + Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { + p, m := req.URL.Path, req.Method + actions = append(actions, p+":"+m) + t.Logf("got request %s %s", p, m) + switch { + case p == "/namespaces/default/pods/newpod" && m == "GET": + // Return the cluster state (port 80), which differs from the + // rollback target (port 443), ensuring a non-empty patch. + return newResponse(200, &clusterPod) + case p == "/namespaces/default/pods/newpod" && m == "PATCH": + return newResponse(200, &listTarget.Items[0]) + default: + t.Fatalf("unexpected request: %s %s", req.Method, req.URL.Path) + return nil, nil + } + }), + } + + // original is empty (Revision 2 had removed "newpod" but upgrade failed) + original, err := c.Build(objBody(&v1.PodList{}), false) + if err != nil { + t.Fatal(err) + } + // target is Revision 1 which had "newpod" + target, err := c.Build(objBody(&listTarget), false) + if err != nil { + t.Fatal(err) + } + + result, err := c.Update(original, target, false) + if err != nil { + t.Fatalf("expected no error during rollback, got: %v", err) + } + + if len(result.Created) != 0 { + t.Errorf("expected 0 resources created, got %d", len(result.Created)) + } + if len(result.Updated) != 1 { + t.Errorf("expected 1 resource updated, got %d", len(result.Updated)) + } + if len(result.Deleted) != 0 { + t.Errorf("expected 0 resources deleted, got %d", len(result.Deleted)) + } + + // Assert that the rollback path issued at least one real PATCH (not just + // GETs), and that no POST or DELETE was issued. + var patchCount int + for _, a := range actions { + switch { + case strings.HasSuffix(a, ":POST"): + t.Errorf("unexpected POST during rollback (actions: %v)", actions) + case strings.HasSuffix(a, ":DELETE"): + t.Errorf("unexpected DELETE during rollback (actions: %v)", actions) + case strings.HasSuffix(a, ":PATCH"): + patchCount++ + } + } + if patchCount == 0 { + t.Errorf("expected at least one PATCH request during rollback, got actions: %v", actions) + } +} + func TestBuild(t *testing.T) { tests := []struct { name string