Merge pull request #31393 from benoittgt/12299

Return errors during upgrade when the deletion of resources fails

pkg/kube/client.go

@@ -569,10 +569,17 @@ func (c *Client) update(originals, targets ResourceList, updateApplyFunc UpdateA
 		}
 		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))
+			if !apierrors.IsNotFound(err) {
+				updateErrors = append(updateErrors, fmt.Errorf("failed to delete resource %s: %w", info.Name, err))
+			}
 			continue
 		}
 		res.Deleted = append(res.Deleted, info)
 	}
+
+	if len(updateErrors) != 0 {
+		return res, joinErrors(updateErrors, " && ")
+	}
 	return res, nil
 }
 

pkg/kube/client_test.go

@@ -321,6 +321,7 @@ func TestUpdate(t *testing.T) {
 		ThreeWayMergeForUnstructured bool
 		ServerSideApply              bool
 		ExpectedActions              []string
+		ExpectedError                string
 	}
 
 	expectedActionsClientSideApply := []string{
@@ -336,6 +337,8 @@ func TestUpdate(t *testing.T) {
 		"/namespaces/default/pods:POST", // retry due to 409
 		"/namespaces/default/pods/squid:GET",
 		"/namespaces/default/pods/squid:DELETE",
+		"/namespaces/default/pods/notfound:GET",
+		"/namespaces/default/pods/notfound:DELETE",
 	}
 
 	expectedActionsServerSideApply := []string{
@@ -351,11 +354,13 @@ func TestUpdate(t *testing.T) {
 		"/namespaces/default/pods:POST", // retry due to 409
 		"/namespaces/default/pods/squid:GET",
 		"/namespaces/default/pods/squid:DELETE",
+		"/namespaces/default/pods/notfound:GET",
+		"/namespaces/default/pods/notfound:DELETE",
 	}
 
 	testCases := map[string]testCase{
 		"client-side apply": {
-			OriginalPods: newPodList("starfish", "otter", "squid"),
+			OriginalPods: newPodList("starfish", "otter", "squid", "notfound"),
 			TargetPods: func() v1.PodList {
 				listTarget := newPodList("starfish", "otter", "dolphin")
 				listTarget.Items[0].Spec.Containers[0].Ports = []v1.ContainerPort{{Name: "https", ContainerPort: 443}}
@@ -365,9 +370,10 @@ func TestUpdate(t *testing.T) {
 			ThreeWayMergeForUnstructured: false,
 			ServerSideApply:              false,
 			ExpectedActions:              expectedActionsClientSideApply,
+			ExpectedError:                "",
 		},
 		"client-side apply (three-way merge for unstructured)": {
-			OriginalPods: newPodList("starfish", "otter", "squid"),
+			OriginalPods: newPodList("starfish", "otter", "squid", "notfound"),
 			TargetPods: func() v1.PodList {
 				listTarget := newPodList("starfish", "otter", "dolphin")
 				listTarget.Items[0].Spec.Containers[0].Ports = []v1.ContainerPort{{Name: "https", ContainerPort: 443}}
@@ -377,9 +383,10 @@ func TestUpdate(t *testing.T) {
 			ThreeWayMergeForUnstructured: true,
 			ServerSideApply:              false,
 			ExpectedActions:              expectedActionsClientSideApply,
+			ExpectedError:                "",
 		},
 		"serverSideApply": {
-			OriginalPods: newPodList("starfish", "otter", "squid"),
+			OriginalPods: newPodList("starfish", "otter", "squid", "notfound"),
 			TargetPods: func() v1.PodList {
 				listTarget := newPodList("starfish", "otter", "dolphin")
 				listTarget.Items[0].Spec.Containers[0].Ports = []v1.ContainerPort{{Name: "https", ContainerPort: 443}}
@@ -389,6 +396,23 @@ func TestUpdate(t *testing.T) {
 			ThreeWayMergeForUnstructured: false,
 			ServerSideApply:              true,
 			ExpectedActions:              expectedActionsServerSideApply,
+			ExpectedError:                "",
+		},
+		"serverSideApply with forbidden deletion": {
+			OriginalPods: newPodList("starfish", "otter", "squid", "notfound", "forbidden"),
+			TargetPods: func() v1.PodList {
+				listTarget := newPodList("starfish", "otter", "dolphin")
+				listTarget.Items[0].Spec.Containers[0].Ports = []v1.ContainerPort{{Name: "https", ContainerPort: 443}}
+
+				return listTarget
+			}(),
+			ThreeWayMergeForUnstructured: false,
+			ServerSideApply:              true,
+			ExpectedActions: append(expectedActionsServerSideApply,
+				"/namespaces/default/pods/forbidden:GET",
+				"/namespaces/default/pods/forbidden:DELETE",
+			),
+			ExpectedError: "failed to delete resource forbidden:",
 		},
 	}
 
@@ -444,6 +468,22 @@ func TestUpdate(t *testing.T) {
 					return newResponse(http.StatusOK, &listTarget.Items[1])
 				case p == "/namespaces/default/pods/squid" && m == http.MethodGet:
 					return newResponse(http.StatusOK, &listTarget.Items[2])
+				case p == "/namespaces/default/pods/notfound" && m == http.MethodGet:
+					// Resource exists in original but will simulate not found on delete
+					return newResponse(http.StatusOK, &listOriginal.Items[3])
+				case p == "/namespaces/default/pods/notfound" && m == http.MethodDelete:
+					// Simulate a not found during deletion; should not cause update to fail
+					return newResponse(http.StatusNotFound, notFoundBody())
+				case p == "/namespaces/default/pods/forbidden" && m == http.MethodGet:
+					return newResponse(http.StatusOK, &listOriginal.Items[4])
+				case p == "/namespaces/default/pods/forbidden" && m == http.MethodDelete:
+					// Simulate RBAC forbidden that should cause update to fail
+					return newResponse(http.StatusForbidden, &metav1.Status{
+						Status:  metav1.StatusFailure,
+						Message: "pods \"forbidden\" is forbidden: User \"test-user\" cannot delete resource \"pods\" in API group \"\" in the namespace \"default\"",
+						Reason:  metav1.StatusReasonForbidden,
+						Code:    http.StatusForbidden,
+					})
 				default:
 				}
 
@@ -471,7 +511,13 @@ func TestUpdate(t *testing.T) {
 				ClientUpdateOptionForceReplace(false),
 				ClientUpdateOptionServerSideApply(tc.ServerSideApply, false),
 				ClientUpdateOptionUpgradeClientSideFieldManager(true))
-			require.NoError(t, err)
+
+			if tc.ExpectedError != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.ExpectedError)
+			} else {
+				require.NoError(t, err)
+			}
 
 			assert.Len(t, result.Created, 1, "expected 1 resource created, got %d", len(result.Created))
 			assert.Len(t, result.Updated, 2, "expected 2 resource updated, got %d", len(result.Updated))