diff --git a/pkg/armrpc/frontend/defaultoperation/defaultasyncdelete.go b/pkg/armrpc/frontend/defaultoperation/defaultasyncdelete.go index a2c38bfe4e..a06a8a21f8 100644 --- a/pkg/armrpc/frontend/defaultoperation/defaultasyncdelete.go +++ b/pkg/armrpc/frontend/defaultoperation/defaultasyncdelete.go @@ -42,6 +42,8 @@ func NewDefaultAsyncDelete[P interface { } // Run executes asynchronous delete operation by validating the request, executing custom delete filters, and starting async job, and returns an async response. +// When the "force" query parameter is set to "true", the provisioning state check is skipped, +// allowing deletion of resources stuck in non-terminal states (e.g., "Updating"). func (e *DefaultAsyncDelete[P, T]) Run(ctx context.Context, w http.ResponseWriter, req *http.Request) (rest.Response, error) { serviceCtx := v1.ARMRequestContextFromContext(ctx) old, etag, err := e.GetResource(ctx, serviceCtx.ResourceID) @@ -53,8 +55,16 @@ func (e *DefaultAsyncDelete[P, T]) Run(ctx context.Context, w http.ResponseWrite return rest.NewNoContentResponse(), nil } - if r, err := e.PrepareResource(ctx, req, nil, old, etag); r != nil || err != nil { - return r, err + force := req.URL.Query().Get("force") == "true" + if force { + // When force-deleting, skip the provisioning state check but still validate the ETag. + if err := ctrl.ValidateETag(*serviceCtx, etag); err != nil { + return rest.NewPreconditionFailedResponse(serviceCtx.ResourceID.String(), err.Error()), nil + } + } else { + if r, err := e.PrepareResource(ctx, req, nil, old, etag); r != nil || err != nil { + return r, err + } } for _, filter := range e.DeleteFilters() { diff --git a/pkg/armrpc/frontend/defaultoperation/defaultasyncdelete_test.go b/pkg/armrpc/frontend/defaultoperation/defaultasyncdelete_test.go index 64db0a26b3..0b62311f10 100644 --- a/pkg/armrpc/frontend/defaultoperation/defaultasyncdelete_test.go +++ b/pkg/armrpc/frontend/defaultoperation/defaultasyncdelete_test.go @@ -44,12 +44,17 @@ func TestDefaultAsyncDelete(t *testing.T) { qErr error saveErr error rejectedByFilter bool + force bool code int }{ - {"async-delete-non-existing-resource-no-etag", "", v1.ProvisioningStateNone, &database.ErrNotFound{}, nil, nil, false, http.StatusNoContent}, - {"async-delete-existing-resource-blocked-by-filter", "", v1.ProvisioningStateSucceeded, nil, nil, nil, true, http.StatusConflict}, - {"async-delete-existing-resource-not-in-terminal-state", "", v1.ProvisioningStateUpdating, nil, nil, nil, false, http.StatusConflict}, - {"async-delete-existing-resource-success", "", v1.ProvisioningStateSucceeded, nil, nil, nil, false, http.StatusAccepted}, + {"async-delete-non-existing-resource-no-etag", "", v1.ProvisioningStateNone, &database.ErrNotFound{}, nil, nil, false, false, http.StatusNoContent}, + {"async-delete-existing-resource-blocked-by-filter", "", v1.ProvisioningStateSucceeded, nil, nil, nil, true, false, http.StatusConflict}, + {"async-delete-existing-resource-not-in-terminal-state", "", v1.ProvisioningStateUpdating, nil, nil, nil, false, false, http.StatusConflict}, + {"async-delete-existing-resource-success", "", v1.ProvisioningStateSucceeded, nil, nil, nil, false, false, http.StatusAccepted}, + {"async-force-delete-existing-resource-in-updating-state", "", v1.ProvisioningStateUpdating, nil, nil, nil, false, true, http.StatusAccepted}, + {"async-force-delete-existing-resource-in-accepted-state", "", v1.ProvisioningStateAccepted, nil, nil, nil, false, true, http.StatusAccepted}, + {"async-force-delete-existing-resource-bad-etag", "\"incorrect-etag\"", v1.ProvisioningStateUpdating, nil, nil, nil, false, true, http.StatusPreconditionFailed}, + {"async-force-delete-existing-resource-blocked-by-filter", "", v1.ProvisioningStateUpdating, nil, nil, nil, true, true, http.StatusConflict}, } for _, tt := range deleteCases { @@ -63,6 +68,12 @@ func TestDefaultAsyncDelete(t *testing.T) { require.NoError(t, err) req.Header.Set("If-Match", tt.etag) + if tt.force { + q := req.URL.Query() + q.Set("force", "true") + req.URL.RawQuery = q.Encode() + } + ctx := rpctest.NewARMRequestContext(req) _, appDataModel, _ := loadTestResurce() @@ -81,7 +92,7 @@ func TestDefaultAsyncDelete(t *testing.T) { }, tt.getErr). Times(1) - if tt.getErr == nil && !tt.rejectedByFilter && appDataModel.InternalMetadata.AsyncProvisioningState.IsTerminal() { + if tt.getErr == nil && !tt.rejectedByFilter && tt.code != http.StatusPreconditionFailed && (appDataModel.InternalMetadata.AsyncProvisioningState.IsTerminal() || tt.force) { expectedOptions := statusmanager.QueueOperationOptions{ OperationTimeout: asyncOperationTimeout, RetryAfter: asyncOperationRetryAfter, diff --git a/pkg/cli/clients/clients.go b/pkg/cli/clients/clients.go index 8cb7a7b389..cc9df7fc62 100644 --- a/pkg/cli/clients/clients.go +++ b/pkg/cli/clients/clients.go @@ -70,6 +70,8 @@ type DeleteOptions struct { ProgressText string // ProgressChan is a channel used to signal progress of the deletion operation. ProgressChan chan<- ResourceProgress + // Force indicates whether to force delete resources that are in a non-terminal provisioning state. + Force bool } type ResourceStatus string @@ -177,7 +179,8 @@ type ApplicationsManagementClient interface { CreateOrUpdateResource(ctx context.Context, resourceType string, resourceNameOrID string, resource *generated.GenericResource) (generated.GenericResource, error) // DeleteResource deletes a resource by its type and name (or id). - DeleteResource(ctx context.Context, resourceType string, resourceNameOrID string) (bool, error) + // When force is true, the delete will proceed even if the resource is in a non-terminal provisioning state. + DeleteResource(ctx context.Context, resourceType string, resourceNameOrID string, force bool) (bool, error) // ListApplications lists all applications in the configured scope. ListApplications(ctx context.Context) ([]corerp.ApplicationResource, error) @@ -195,7 +198,8 @@ type ApplicationsManagementClient interface { CreateApplicationIfNotFound(ctx context.Context, applicationNameOrID string, resource *corerp.ApplicationResource) error // DeleteApplication deletes an application and all of its resources by its name (or id). - DeleteApplication(ctx context.Context, applicationNameOrID string) (bool, error) + // When force is true, resources in non-terminal provisioning states will be force-deleted. + DeleteApplication(ctx context.Context, applicationNameOrID string, force bool) (bool, error) // ListEnvironments lists all environments in the configured scope (assumes configured scope is a resource group). ListEnvironments(ctx context.Context) ([]corerp.EnvironmentResource, error) diff --git a/pkg/cli/clients/management.go b/pkg/cli/clients/management.go index 0ffc95fc81..e131be004b 100644 --- a/pkg/cli/clients/management.go +++ b/pkg/cli/clients/management.go @@ -63,7 +63,7 @@ func (amc *UCPApplicationsManagementClient) ListResourcesOfType(ctx context.Cont } results := []generated.GenericResource{} - client, err := amc.getGenericClient(amc.RootScope, resourceType, apiVersions) + client, err := amc.getGenericClient(amc.RootScope, resourceType, apiVersions, false) if err != nil { return nil, err } @@ -139,7 +139,7 @@ func (amc *UCPApplicationsManagementClient) GetResource(ctx context.Context, res return generated.GenericResource{}, err } - client, err := amc.getGenericClient(scope, resourceType, apiVersions) + client, err := amc.getGenericClient(scope, resourceType, apiVersions, false) if err != nil { return generated.GenericResource{}, err } @@ -178,7 +178,7 @@ func (amc *UCPApplicationsManagementClient) CreateOrUpdateResource(ctx context.C } // DeleteResource deletes a resource by its type and name (or id). -func (amc *UCPApplicationsManagementClient) DeleteResource(ctx context.Context, resourceType string, resourceNameOrID string) (bool, error) { +func (amc *UCPApplicationsManagementClient) DeleteResource(ctx context.Context, resourceType string, resourceNameOrID string, force bool) (bool, error) { apiVersions, err := amc.getApiVersionsForResourceType(ctx, resourceType) if err != nil { return false, err @@ -189,7 +189,8 @@ func (amc *UCPApplicationsManagementClient) DeleteResource(ctx context.Context, return false, err } - client, err := amc.getGenericClient(scope, resourceType, apiVersions) + var client genericResourceClient + client, err = amc.getGenericClient(scope, resourceType, apiVersions, force) if err != nil { return false, err } @@ -357,7 +358,7 @@ func (amc *UCPApplicationsManagementClient) CreateApplicationIfNotFound(ctx cont } // DeleteApplication deletes an application and all of its resources by its name (or id). -func (amc *UCPApplicationsManagementClient) DeleteApplication(ctx context.Context, applicationNameOrID string) (bool, error) { +func (amc *UCPApplicationsManagementClient) DeleteApplication(ctx context.Context, applicationNameOrID string, force bool) (bool, error) { scope, name, err := amc.extractScopeAndName(applicationNameOrID) if err != nil { return false, err @@ -373,7 +374,7 @@ func (amc *UCPApplicationsManagementClient) DeleteApplication(ctx context.Contex g, groupCtx := errgroup.WithContext(ctx) for _, resource := range resources { g.Go(func() error { - _, err := amc.DeleteResource(groupCtx, *resource.Type, *resource.ID) + _, err := amc.DeleteResource(groupCtx, *resource.Type, *resource.ID, force) if err != nil && !clientv2.Is404Error(err) { return err } @@ -387,7 +388,8 @@ func (amc *UCPApplicationsManagementClient) DeleteApplication(ctx context.Contex return false, err } - client, err := amc.createApplicationClient(scope) + var client applicationResourceClient + client, err = amc.createApplicationClient(scope, force) if err != nil { return false, err } @@ -641,7 +643,7 @@ func (amc *UCPApplicationsManagementClient) DeleteEnvironment(ctx context.Contex } for _, application := range applications { - _, err := amc.DeleteApplication(ctx, *application.ID) + _, err := amc.DeleteApplication(ctx, *application.ID, false) if err != nil { return false, err } @@ -748,7 +750,7 @@ func (amc *UCPApplicationsManagementClient) DeleteResourceGroup(ctx context.Cont for _, resource := range resources { g.Go(func() error { // Delete each resource using its full ID to ensure correct scope - _, err := amc.DeleteResource(groupCtx, *resource.Type, *resource.ID) + _, err := amc.DeleteResource(groupCtx, *resource.Type, *resource.ID, false) if err != nil && !clientv2.Is404Error(err) { return err } @@ -804,7 +806,7 @@ func (amc *UCPApplicationsManagementClient) ListResourcesInResourceGroup(ctx con continue // Skip this resource type if we can't get API versions } - client, err := amc.getGenericClient(groupScope, resourceType, apiVersions) + client, err := amc.getGenericClient(groupScope, resourceType, apiVersions, false) if err != nil { continue } @@ -869,7 +871,7 @@ func (amc *UCPApplicationsManagementClient) ListResourcesOfTypeInResourceGroup(c return nil, err } - client, err := amc.getGenericClient(groupScope, resourceType, apiVersions) + client, err := amc.getGenericClient(groupScope, resourceType, apiVersions, false) if err != nil { return nil, err } @@ -1198,13 +1200,23 @@ func (amc *UCPApplicationsManagementClient) CreateOrUpdateLocation(ctx context.C return response.LocationResource, nil } -func (amc *UCPApplicationsManagementClient) createApplicationClient(scope string) (applicationResourceClient, error) { - if amc.applicationResourceClientFactory == nil { - // Generated client doesn't like the leading '/' in the scope. - return corerpv20231001.NewApplicationsClient(strings.TrimPrefix(scope, resources.SegmentSeparator), &aztoken.AnonymousCredential{}, amc.ClientOptions) +// createApplicationClient creates an application resource client for the specified scope. +// When no applicationResourceClientFactory is configured and force is true, a per-call policy is added +// that appends force=true to the request URL query string. +func (amc *UCPApplicationsManagementClient) createApplicationClient(scope string, force ...bool) (applicationResourceClient, error) { + if amc.applicationResourceClientFactory != nil { + return amc.applicationResourceClientFactory(scope) } - return amc.applicationResourceClientFactory(scope) + forceEnabled := len(force) > 0 && force[0] + clientOptions := amc.ClientOptions + if forceEnabled { + opts := withForceDeletePolicy(*amc.ClientOptions) + clientOptions = &opts + } + + // Generated client doesn't like the leading '/' in the scope. + return corerpv20231001.NewApplicationsClient(strings.TrimPrefix(scope, resources.SegmentSeparator), &aztoken.AnonymousCredential{}, clientOptions) } func (amc *UCPApplicationsManagementClient) createRecipePackClient(scope string) (recipePackResourceClient, error) { @@ -1377,23 +1389,60 @@ func (amc *UCPApplicationsManagementClient) getApiVersionsForResourceType(ctx co } // getGenericClient returns a generic resource client for the specified scope and resource type. -// if apiVersions is empty, it uses the default version i.e 2023-10-01-preview else uses any version supported by the resource type. -func (amc *UCPApplicationsManagementClient) getGenericClient(scope, resourceType string, apiVersions []string) (client genericResourceClient, err error) { +// If apiVersions is empty, it uses the default version (2023-10-01-preview), else uses any version supported by the resource type. +// When no genericResourceClientFactory is configured and force is true, a per-call policy is added +// that appends force=true to the request URL query string. This is used for force-deleting resources +// that are in a non-terminal provisioning state. Factory-based configurations (used in tests) bypass +// the force policy since mock clients do not exercise the HTTP pipeline. +func (amc *UCPApplicationsManagementClient) getGenericClient(scope, resourceType string, apiVersions []string, force bool) (client genericResourceClient, err error) { // Radius.Core resources require a specific API version. // Eventually version 2023-10-01-preview will be removed along with Applications.Core resources. // Then we will not need this special case. if strings.HasPrefix(resourceType, "Radius.Core") { apiVersions = []string{"2025-08-01-preview"} } - if len(apiVersions) == 0 { - client, err = amc.createGenericClient(scope, resourceType) - } else { - client, err = amc.createGenericClient(scope, resourceType, apiVersions[0]) + + if amc.genericResourceClientFactory != nil { + return amc.genericResourceClientFactory(scope, resourceType) } - if err != nil { - return nil, err + clientOptions := *amc.ClientOptions + + if force { + clientOptions = withForceDeletePolicy(clientOptions) + } + + if len(apiVersions) != 0 { + clientOptions.APIVersion = apiVersions[0] + } + + return generated.NewGenericResourcesClient(resourceType, strings.TrimPrefix(scope, resources.SegmentSeparator), &aztoken.AnonymousCredential{}, &clientOptions) +} + +// withForceDeletePolicy returns a copy of opts with forceDeletePolicy appended to PerCallPolicies. +func withForceDeletePolicy(opts arm.ClientOptions) arm.ClientOptions { + opts.PerCallPolicies = append( + append([]policy.Policy{}, opts.PerCallPolicies...), + &forceDeletePolicy{}, + ) + return opts +} + +// forceDeletePolicy is a per-call pipeline policy that appends force=true to DELETE request URL query strings. +type forceDeletePolicy struct{} + +func (p *forceDeletePolicy) Do(req *policy.Request) (*http.Response, error) { + rawReq := req.Raw() + if rawReq.Method != http.MethodDelete { + return req.Next() + } + + q := rawReq.URL.Query() + if _, ok := q["force"]; ok { + return req.Next() } - return client, err + q.Set("force", "true") + rawReq.URL.RawQuery = q.Encode() + return req.Next() } diff --git a/pkg/cli/clients/management_test.go b/pkg/cli/clients/management_test.go index 9bda729b1e..d54adfcdc4 100644 --- a/pkg/cli/clients/management_test.go +++ b/pkg/cli/clients/management_test.go @@ -19,12 +19,16 @@ package clients import ( "context" "fmt" + "io" "net/http" "reflect" "strconv" + "strings" "testing" "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" v1 "github.com/radius-project/radius/pkg/armrpc/api/v1" "github.com/radius-project/radius/pkg/cli/clients_new/generated" @@ -662,7 +666,7 @@ func Test_Resource(t *testing.T) { BeginDelete(gomock.Any(), testResourceName, gomock.Any()). Return(poller(&generated.GenericResourcesClientDeleteResponse{}), nil) - deleted, err := client.DeleteResource(context.Background(), testResourceType, testResourceID) + deleted, err := client.DeleteResource(context.Background(), testResourceType, testResourceID, false) require.NoError(t, err) require.True(t, deleted) }) @@ -718,6 +722,253 @@ func Test_Resource(t *testing.T) { }) } +func Test_ForceDeletePolicy(t *testing.T) { + t.Run("adds force=true query parameter to DELETE requests", func(t *testing.T) { + p := &forceDeletePolicy{} + + // Create a minimal pipeline with a transport that captures the request URL. + var capturedURL string + pipeline := runtime.NewPipeline("test", "v1.0.0", runtime.PipelineOptions{ + PerCall: []policy.Policy{p}, + }, &policy.ClientOptions{ + Transport: &mockTransport{ + do: func(req *http.Request) (*http.Response, error) { + capturedURL = req.URL.String() + return &http.Response{ + StatusCode: http.StatusOK, + Body: http.NoBody, + }, nil + }, + }, + }) + + req, err := runtime.NewRequest(context.Background(), http.MethodDelete, "http://localhost/test?api-version=2023-10-01-preview") + require.NoError(t, err) + + _, err = pipeline.Do(req) + require.NoError(t, err) + require.Contains(t, capturedURL, "force=true") + require.Contains(t, capturedURL, "api-version=2023-10-01-preview") + }) + + t.Run("skips non-DELETE requests", func(t *testing.T) { + p := &forceDeletePolicy{} + + var capturedURL string + pipeline := runtime.NewPipeline("test", "v1.0.0", runtime.PipelineOptions{ + PerCall: []policy.Policy{p}, + }, &policy.ClientOptions{ + Transport: &mockTransport{ + do: func(req *http.Request) (*http.Response, error) { + capturedURL = req.URL.String() + return &http.Response{ + StatusCode: http.StatusOK, + Body: http.NoBody, + }, nil + }, + }, + }) + + req, err := runtime.NewRequest(context.Background(), http.MethodGet, "http://localhost/test?api-version=2023-10-01-preview") + require.NoError(t, err) + + _, err = pipeline.Do(req) + require.NoError(t, err) + require.NotContains(t, capturedURL, "force=true") + }) +} + +func Test_DeleteResource_ForceQueryParameter(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + force bool + expectQP bool + }{ + { + name: "force=true adds force query parameter", + force: true, + expectQP: true, + }, + { + name: "force=false does not add force query parameter", + force: false, + expectQP: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var capturedURLs []string + transport := &mockTransport{ + do: func(req *http.Request) (*http.Response, error) { + capturedURLs = append(capturedURLs, req.URL.String()) + header := http.Header{} + header.Set("Content-Type", "application/json") + return &http.Response{ + StatusCode: http.StatusOK, + Header: header, + Body: io.NopCloser(strings.NewReader(`{"status": "Succeeded"}`)), + Request: req, + }, nil + }, + } + + ctrl := gomock.NewController(t) + rpClient := NewMockresourceProviderClient(ctrl) + rpClient.EXPECT(). + GetProviderSummary(gomock.Any(), "local", "Applications.Test", gomock.Any()). + Return(ucp.ResourceProvidersClientGetProviderSummaryResponse{ + ResourceProviderSummary: ucp.ResourceProviderSummary{ + Name: new("Applications.Test"), + ResourceTypes: map[string]*ucp.ResourceProviderSummaryResourceType{ + "testResource": { + APIVersions: map[string]*ucp.ResourceTypeSummaryResultAPIVersion{ + version: {}, + }, + }, + }, + }, + }, nil) + + client := &UCPApplicationsManagementClient{ + RootScope: testScope, + ClientOptions: &arm.ClientOptions{ + ClientOptions: policy.ClientOptions{ + Transport: transport, + }, + }, + resourceProviderClientFactory: func() (resourceProviderClient, error) { + return rpClient, nil + }, + } + + // We don't check the return values - we only care about the captured URLs. + _, _ = client.DeleteResource(context.Background(), "Applications.Test/testResource", testScope+"/providers/Applications.Test/testResource/myresource", tt.force) + + require.NotEmpty(t, capturedURLs, "expected at least one HTTP request") + foundForce := false + for _, u := range capturedURLs { + if strings.Contains(u, "force=true") { + foundForce = true + break + } + } + if tt.expectQP { + require.True(t, foundForce, "expected force=true in request URL, got URLs: %v", capturedURLs) + } else { + require.False(t, foundForce, "did not expect force=true in request URL, got URLs: %v", capturedURLs) + } + }) + } +} + +func Test_DeleteApplication_ForceQueryParameter(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + force bool + expectQP bool + }{ + { + name: "force=true adds force query parameter to app delete", + force: true, + expectQP: true, + }, + { + name: "force=false does not add force query parameter to app delete", + force: false, + expectQP: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var appDeleteURLs []string + transport := &mockTransport{ + do: func(req *http.Request) (*http.Response, error) { + // Capture URLs of DELETE requests to the applications endpoint + if req.Method == http.MethodDelete && strings.Contains(req.URL.Path, "Applications.Core/applications") { + appDeleteURLs = append(appDeleteURLs, req.URL.String()) + } + header := http.Header{} + header.Set("Content-Type", "application/json") + return &http.Response{ + StatusCode: http.StatusOK, + Header: header, + Body: io.NopCloser(strings.NewReader(`{"status": "Succeeded"}`)), + Request: req, + }, nil + }, + } + + ctrl := gomock.NewController(t) + genericMock := NewMockgenericResourceClient(ctrl) + rpClient := NewMockresourceProviderClient(ctrl) + + // Mock for ListResourcesInApplication: return empty list (no child resources) + rpClient.EXPECT(). + NewListProviderSummariesPager("local", gomock.Any()). + Return(pager([]ucp.ResourceProvidersClientListProviderSummariesResponse{ + { + PagedResourceProviderSummary: ucp.PagedResourceProviderSummary{ + Value: []*ucp.ResourceProviderSummary{}, + NextLink: new("0"), + }, + }, + })) + + // Use genericResourceClientFactory so listing doesn't go through transport + // but leave applicationResourceClientFactory nil so app delete goes through transport + client := &UCPApplicationsManagementClient{ + RootScope: testScope, + ClientOptions: &arm.ClientOptions{ + ClientOptions: policy.ClientOptions{ + Transport: transport, + }, + }, + genericResourceClientFactory: func(scope string, resourceType string) (genericResourceClient, error) { + return genericMock, nil + }, + resourceProviderClientFactory: func() (resourceProviderClient, error) { + return rpClient, nil + }, + } + + // We don't check the return values - we only care about the captured URLs. + _, _ = client.DeleteApplication(context.Background(), testScope+"/providers/Applications.Core/applications/test-app", tt.force) + + require.NotEmpty(t, appDeleteURLs, "expected at least one DELETE request to applications endpoint") + foundForce := false + for _, u := range appDeleteURLs { + if strings.Contains(u, "force=true") { + foundForce = true + break + } + } + if tt.expectQP { + require.True(t, foundForce, "expected force=true in app DELETE URL, got URLs: %v", appDeleteURLs) + } else { + require.False(t, foundForce, "did not expect force=true in app DELETE URL, got URLs: %v", appDeleteURLs) + } + }) + } +} + +type mockTransport struct { + do func(req *http.Request) (*http.Response, error) +} + +func (m *mockTransport) Do(req *http.Request) (*http.Response, error) { + return m.do(req) +} + func Test_Application(t *testing.T) { t.Parallel() createClient := func(wrapped applicationResourceClient) *UCPApplicationsManagementClient { @@ -968,7 +1219,7 @@ func Test_Application(t *testing.T) { return corerp.ApplicationsClientDeleteResponse{}, nil }) - deleted, err := client.DeleteApplication(context.Background(), testResourceID) + deleted, err := client.DeleteApplication(context.Background(), testResourceID, false) require.NoError(t, err) require.True(t, deleted) }) @@ -1048,7 +1299,7 @@ func Test_Application(t *testing.T) { return corerp.ApplicationsClientDeleteResponse{}, nil }) - deleted, err := client.DeleteApplication(context.Background(), testResourceID) + deleted, err := client.DeleteApplication(context.Background(), testResourceID, false) require.NoError(t, err) require.True(t, deleted) }) @@ -1087,7 +1338,7 @@ func Test_Application(t *testing.T) { // Delete should NOT be called when ListResourcesInApplication fails with non-404 error // No expectation set for mock.Delete() - deleted, err := client.DeleteApplication(context.Background(), testResourceID) + deleted, err := client.DeleteApplication(context.Background(), testResourceID, false) require.Error(t, err) require.False(t, deleted) // Verify the error is propagated correctly diff --git a/pkg/cli/clients/mock_applicationsclient.go b/pkg/cli/clients/mock_applicationsclient.go index e90cc45d92..745c46139f 100644 --- a/pkg/cli/clients/mock_applicationsclient.go +++ b/pkg/cli/clients/mock_applicationsclient.go @@ -391,18 +391,18 @@ func (c *MockApplicationsManagementClientCreateOrUpdateResourceTypeCall) DoAndRe } // DeleteApplication mocks base method. -func (m *MockApplicationsManagementClient) DeleteApplication(arg0 context.Context, arg1 string) (bool, error) { +func (m *MockApplicationsManagementClient) DeleteApplication(arg0 context.Context, arg1 string, arg2 bool) (bool, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteApplication", arg0, arg1) + ret := m.ctrl.Call(m, "DeleteApplication", arg0, arg1, arg2) ret0, _ := ret[0].(bool) ret1, _ := ret[1].(error) return ret0, ret1 } // DeleteApplication indicates an expected call of DeleteApplication. -func (mr *MockApplicationsManagementClientMockRecorder) DeleteApplication(arg0, arg1 any) *MockApplicationsManagementClientDeleteApplicationCall { +func (mr *MockApplicationsManagementClientMockRecorder) DeleteApplication(arg0, arg1, arg2 any) *MockApplicationsManagementClientDeleteApplicationCall { mr.mock.ctrl.T.Helper() - call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteApplication", reflect.TypeOf((*MockApplicationsManagementClient)(nil).DeleteApplication), arg0, arg1) + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteApplication", reflect.TypeOf((*MockApplicationsManagementClient)(nil).DeleteApplication), arg0, arg1, arg2) return &MockApplicationsManagementClientDeleteApplicationCall{Call: call} } @@ -418,13 +418,13 @@ func (c *MockApplicationsManagementClientDeleteApplicationCall) Return(arg0 bool } // Do rewrite *gomock.Call.Do -func (c *MockApplicationsManagementClientDeleteApplicationCall) Do(f func(context.Context, string) (bool, error)) *MockApplicationsManagementClientDeleteApplicationCall { +func (c *MockApplicationsManagementClientDeleteApplicationCall) Do(f func(context.Context, string, bool) (bool, error)) *MockApplicationsManagementClientDeleteApplicationCall { c.Call = c.Call.Do(f) return c } // DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockApplicationsManagementClientDeleteApplicationCall) DoAndReturn(f func(context.Context, string) (bool, error)) *MockApplicationsManagementClientDeleteApplicationCall { +func (c *MockApplicationsManagementClientDeleteApplicationCall) DoAndReturn(f func(context.Context, string, bool) (bool, error)) *MockApplicationsManagementClientDeleteApplicationCall { c.Call = c.Call.DoAndReturn(f) return c } @@ -508,18 +508,18 @@ func (c *MockApplicationsManagementClientDeleteRecipePackCall) DoAndReturn(f fun } // DeleteResource mocks base method. -func (m *MockApplicationsManagementClient) DeleteResource(arg0 context.Context, arg1, arg2 string) (bool, error) { +func (m *MockApplicationsManagementClient) DeleteResource(arg0 context.Context, arg1, arg2 string, arg3 bool) (bool, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteResource", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "DeleteResource", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(bool) ret1, _ := ret[1].(error) return ret0, ret1 } // DeleteResource indicates an expected call of DeleteResource. -func (mr *MockApplicationsManagementClientMockRecorder) DeleteResource(arg0, arg1, arg2 any) *MockApplicationsManagementClientDeleteResourceCall { +func (mr *MockApplicationsManagementClientMockRecorder) DeleteResource(arg0, arg1, arg2, arg3 any) *MockApplicationsManagementClientDeleteResourceCall { mr.mock.ctrl.T.Helper() - call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteResource", reflect.TypeOf((*MockApplicationsManagementClient)(nil).DeleteResource), arg0, arg1, arg2) + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteResource", reflect.TypeOf((*MockApplicationsManagementClient)(nil).DeleteResource), arg0, arg1, arg2, arg3) return &MockApplicationsManagementClientDeleteResourceCall{Call: call} } @@ -535,13 +535,13 @@ func (c *MockApplicationsManagementClientDeleteResourceCall) Return(arg0 bool, a } // Do rewrite *gomock.Call.Do -func (c *MockApplicationsManagementClientDeleteResourceCall) Do(f func(context.Context, string, string) (bool, error)) *MockApplicationsManagementClientDeleteResourceCall { +func (c *MockApplicationsManagementClientDeleteResourceCall) Do(f func(context.Context, string, string, bool) (bool, error)) *MockApplicationsManagementClientDeleteResourceCall { c.Call = c.Call.Do(f) return c } // DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockApplicationsManagementClientDeleteResourceCall) DoAndReturn(f func(context.Context, string, string) (bool, error)) *MockApplicationsManagementClientDeleteResourceCall { +func (c *MockApplicationsManagementClientDeleteResourceCall) DoAndReturn(f func(context.Context, string, string, bool) (bool, error)) *MockApplicationsManagementClientDeleteResourceCall { c.Call = c.Call.DoAndReturn(f) return c } diff --git a/pkg/cli/cmd/app/delete/delete.go b/pkg/cli/cmd/app/delete/delete.go index a6b23af773..6a90ebd29f 100644 --- a/pkg/cli/cmd/app/delete/delete.go +++ b/pkg/cli/cmd/app/delete/delete.go @@ -62,6 +62,9 @@ rad app delete my-app # Delete specified application in a specified resource group rad app delete my-app --group my-group + +# Force delete an application with resources stuck in a non-terminal state +rad app delete my-app --force `, Args: cobra.MaximumNArgs(1), RunE: framework.RunCommand(runner), @@ -71,6 +74,7 @@ rad app delete my-app --group my-group commonflags.AddResourceGroupFlag(cmd) commonflags.AddApplicationNameFlag(cmd) commonflags.AddConfirmationFlag(cmd) + commonflags.AddForceFlag(cmd) return cmd, runner } @@ -87,6 +91,7 @@ type Runner struct { EnvironmentName string Scope string Confirm bool + Force bool Workspace *workspaces.Workspace } @@ -145,6 +150,11 @@ func (r *Runner) Validate(cmd *cobra.Command, args []string) error { return err } + r.Force, err = cmd.Flags().GetBool("force") + if err != nil { + return err + } + return nil } @@ -175,6 +185,11 @@ func (r *Runner) Run(ctx context.Context) error { if err != nil { return err } + + if r.Force { + r.Output.LogInfo("WARNING: Force deleting an application. Resources in non-terminal states may leave orphaned external resources that require manual cleanup.") + } + if !r.Confirm { confirmed, err := prompt.YesOrNoPrompt(fmt.Sprintf(deleteConfirmation, r.ApplicationName, environmentID.Name()), prompt.ConfirmNo, r.InputPrompter) if err != nil { @@ -191,6 +206,7 @@ func (r *Runner) Run(ctx context.Context) error { deleted, err := r.Delete.DeleteApplicationWithProgress(ctx, client, clients.DeleteOptions{ ApplicationNameOrID: r.ApplicationName, ProgressText: progressText, + Force: r.Force, }) if err != nil { if strings.Contains(err.Error(), "not found") { diff --git a/pkg/cli/cmd/app/delete/delete_test.go b/pkg/cli/cmd/app/delete/delete_test.go index 3ce36e9712..f7c2d765b8 100644 --- a/pkg/cli/cmd/app/delete/delete_test.go +++ b/pkg/cli/cmd/app/delete/delete_test.go @@ -529,4 +529,68 @@ func Test_Delete(t *testing.T) { require.Equal(t, expected, outputSink.Writes) }) + + t.Run("Success: Force Delete Application", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + appManagementClient := clients.NewMockApplicationsManagementClient(ctrl) + deleteMock := delete.NewMockInterface(ctrl) + + appManagementClient.EXPECT(). + GetApplication(gomock.Any(), "test-app"). + Return(v20231001preview.ApplicationResource{ + Properties: &v20231001preview.ApplicationProperties{ + Environment: new("/planes/radius/local/resourceGroups/default/providers/Applications.Core/environments/default"), + }, + }, nil). + Times(1) + + progressText := fmt.Sprintf("Deleting application '%s' from environment '%s'...", "test-app", "default") + deleteMock.EXPECT(). + DeleteApplicationWithProgress( + gomock.Any(), + appManagementClient, + clients.DeleteOptions{ + ApplicationNameOrID: "test-app", + ProgressText: progressText, + Force: true, + }, + ). + Return(true, nil). + Times(1) + + workspace := &workspaces.Workspace{ + Connection: map[string]any{ + "kind": "kubernetes", + "context": "kind-kind", + }, + Name: "kind-kind", + Scope: "/planes/radius/local/resourceGroups/test-group", + Environment: "/planes/radius/local/resourceGroups/default/providers/Applications.Core/environments/default", + } + outputSink := &output.MockOutput{} + runner := &Runner{ + Delete: deleteMock, + ConnectionFactory: &connections.MockFactory{ApplicationsManagementClient: appManagementClient}, + Workspace: workspace, + Output: outputSink, + ApplicationName: "test-app", + EnvironmentName: "default", + Confirm: true, + Force: true, + } + + err := runner.Run(context.Background()) + require.NoError(t, err) + + // Verify warning was logged before the delete result + require.GreaterOrEqual(t, len(outputSink.Writes), 2) + warningOutput, ok := outputSink.Writes[0].(output.LogOutput) + require.True(t, ok) + require.Contains(t, warningOutput.Format, "WARNING") + lastOutput, ok := outputSink.Writes[len(outputSink.Writes)-1].(output.LogOutput) + require.True(t, ok) + require.Equal(t, "Application %s deleted successfully", lastOutput.Format) + }) } diff --git a/pkg/cli/cmd/commonflags/flags.go b/pkg/cli/cmd/commonflags/flags.go index 0b45c34a69..8ba368f817 100644 --- a/pkg/cli/cmd/commonflags/flags.go +++ b/pkg/cli/cmd/commonflags/flags.go @@ -70,6 +70,11 @@ func AddConfirmationFlag(cmd *cobra.Command) { cmd.Flags().BoolP("yes", "y", false, "The confirmation flag") } +// AddForceFlag adds a flag to the given command that allows the user to force an operation even when the resource is in a non-terminal state. +func AddForceFlag(cmd *cobra.Command) { + cmd.Flags().Bool("force", false, "Force the operation even if the resource is in a non-terminal provisioning state") +} + // AddEnvironmentNameFlag adds a flag to the given command that allows the user to specify an environment name. func AddEnvironmentNameFlag(cmd *cobra.Command) { cmd.Flags().StringP("environment", "e", "", "The environment name") diff --git a/pkg/cli/cmd/resource/delete/delete.go b/pkg/cli/cmd/resource/delete/delete.go index 8b8ce57336..c4e764f39d 100644 --- a/pkg/cli/cmd/resource/delete/delete.go +++ b/pkg/cli/cmd/resource/delete/delete.go @@ -55,7 +55,10 @@ func NewCommand(factory framework.Factory) (*cobra.Command, framework.Runner) { sample list of resourceType: Applications.Core/containers, Applications.Core/gateways, Applications.Dapr/daprPubSubBrokers, Applications.Core/extenders, Applications.Datastores/mongoDatabases, Applications.Messaging/rabbitMQMessageQueues, Applications.Datastores/redisCaches, Applications.Datastores/sqlDatabases, Applications.Dapr/daprStateStores, Applications.Dapr/daprSecretStores # Delete a container named orders -rad resource delete Applications.Core/containers orders`, +rad resource delete Applications.Core/containers orders + +# Force delete a resource that is stuck in a non-terminal state +rad resource delete Applications.Core/containers orders --force`, Args: cobra.ExactArgs(2), RunE: framework.RunCommand(runner), } @@ -64,6 +67,7 @@ rad resource delete Applications.Core/containers orders`, commonflags.AddWorkspaceFlag(cmd) commonflags.AddResourceGroupFlag(cmd) commonflags.AddConfirmationFlag(cmd) + commonflags.AddForceFlag(cmd) return cmd, runner } @@ -80,6 +84,7 @@ type Runner struct { InputPrompter prompt.Interface Confirm bool + Force bool } // NewRunner creates a new instance of the `rad resource delete` runner. @@ -129,6 +134,12 @@ func (r *Runner) Validate(cmd *cobra.Command, args []string) error { } r.Confirm = yes + force, err := cmd.Flags().GetBool("force") + if err != nil { + return err + } + r.Force = force + return nil } @@ -151,6 +162,10 @@ func (r *Runner) Run(ctx context.Context) error { return err } + if r.Force { + r.Output.LogInfo("WARNING: Force deleting a resource in a non-terminal state may leave orphaned external resources that require manual cleanup.") + } + // Prompt user to confirm deletion if !r.Confirm { var promptMessage string @@ -172,7 +187,7 @@ func (r *Runner) Run(ctx context.Context) error { } } - deleted, err := client.DeleteResource(ctx, r.FullyQualifiedResourceTypeName, r.ResourceName) + deleted, err := client.DeleteResource(ctx, r.FullyQualifiedResourceTypeName, r.ResourceName, r.Force) if err != nil { return err } diff --git a/pkg/cli/cmd/resource/delete/delete_test.go b/pkg/cli/cmd/resource/delete/delete_test.go index f2cfe7654d..af03bedc6e 100644 --- a/pkg/cli/cmd/resource/delete/delete_test.go +++ b/pkg/cli/cmd/resource/delete/delete_test.go @@ -502,6 +502,51 @@ func Test_Run(t *testing.T) { } require.Equal(t, expected, outputSink.Writes) })*/ + + t.Run("Success (force deleted)", func(t *testing.T) { + ctrl := gomock.NewController(t) + + appManagementClient := clients.NewMockApplicationsManagementClient(ctrl) + appManagementClient.EXPECT(). + GetResource(gomock.Any(), "Applications.Core/containers", "test-container"). + Return(generated.GenericResource{ + Properties: map[string]interface{}{ + "environment": "/planes/radius/local/resourceGroups/test-group/providers/Applications.Core/environments/my-test-env", + "application": "/planes/radius/local/resourceGroups/test-group/providers/Applications.Core/applications/my-test-app", + }, + }, nil). + Times(1) + appManagementClient.EXPECT(). + DeleteResource(gomock.Any(), "Applications.Core/containers", "test-container", true). + Return(true, nil). + Times(1) + + outputSink := &output.MockOutput{} + + runner := &Runner{ + ConnectionFactory: &connections.MockFactory{ApplicationsManagementClient: appManagementClient}, + Output: outputSink, + Workspace: &workspaces.Workspace{}, + FullyQualifiedResourceTypeName: "Applications.Core/containers", + ResourceName: "test-container", + Format: "table", + Confirm: true, + Force: true, + } + + err := runner.Run(context.Background()) + require.NoError(t, err) + + expected := []any{ + output.LogOutput{ + Format: "WARNING: Force deleting a resource in a non-terminal state may leave orphaned external resources that require manual cleanup.", + }, + output.LogOutput{ + Format: "Resource deleted", + }, + } + require.Equal(t, expected, outputSink.Writes) + }) }) -} +} \ No newline at end of file diff --git a/pkg/cli/delete/delete.go b/pkg/cli/delete/delete.go index e165b5827c..70dfd21c1a 100644 --- a/pkg/cli/delete/delete.go +++ b/pkg/cli/delete/delete.go @@ -90,7 +90,7 @@ func DeleteApplicationWithProgress(ctx context.Context, amc clients.Applications } } - deleted, err := amc.DeleteApplication(ctx, options.ApplicationNameOrID) + deleted, err := amc.DeleteApplication(ctx, options.ApplicationNameOrID, options.Force) if err == nil { for _, resource := range resourcesList { if resource.ID != nil { diff --git a/pkg/cli/delete/delete_test.go b/pkg/cli/delete/delete_test.go index ee990682bf..3202f6cabd 100644 --- a/pkg/cli/delete/delete_test.go +++ b/pkg/cli/delete/delete_test.go @@ -111,7 +111,7 @@ func Test_DeleteApplicationWithProgress_ErrorScenarios(t *testing.T) { Times(1) appManagementClient.EXPECT(). - DeleteApplication(gomock.Any(), "test-app"). + DeleteApplication(gomock.Any(), "test-app", false). Return(true, nil). Times(1) @@ -169,7 +169,7 @@ func Test_DeleteApplicationWithProgress_ErrorScenarios(t *testing.T) { Times(1) appManagementClient.EXPECT(). - DeleteApplication(gomock.Any(), "test-app"). + DeleteApplication(gomock.Any(), "test-app", false). Return(true, nil). Times(1) @@ -205,7 +205,7 @@ func Test_DeleteApplicationWithProgress_ErrorScenarios(t *testing.T) { Times(1) appManagementClient.EXPECT(). - DeleteApplication(gomock.Any(), "test-app"). + DeleteApplication(gomock.Any(), "test-app", false). Return(true, nil). Times(1) @@ -218,4 +218,37 @@ func Test_DeleteApplicationWithProgress_ErrorScenarios(t *testing.T) { require.NoError(t, err) require.True(t, deleted) }) + + t.Run("Success: Force flag is passed through to DeleteApplication", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + appManagementClient := clients.NewMockApplicationsManagementClient(ctrl) + + appManagementClient.EXPECT(). + ListResourcesInApplication(gomock.Any(), "test-app"). + Return([]generated.GenericResource{}, nil). + Times(1) + + appManagementClient.EXPECT(). + GetApplication(gomock.Any(), "test-app"). + Return(corerp.ApplicationResource{}, fmt.Errorf("not found")). + Times(1) + + // Verify that DeleteApplication is called with force=true + appManagementClient.EXPECT(). + DeleteApplication(gomock.Any(), "test-app", true). + Return(true, nil). + Times(1) + + options := clients.DeleteOptions{ + ApplicationNameOrID: "test-app", + ProgressText: "Deleting application...", + Force: true, + } + + deleted, err := DeleteApplicationWithProgress(context.Background(), appManagementClient, options) + require.NoError(t, err) + require.True(t, deleted) + }) } diff --git a/test/functional-portable/cli/noncloud/cli_test.go b/test/functional-portable/cli/noncloud/cli_test.go index 9c8b015c34..4d33ff8903 100644 --- a/test/functional-portable/cli/noncloud/cli_test.go +++ b/test/functional-portable/cli/noncloud/cli_test.go @@ -605,7 +605,7 @@ func Test_CLI_Delete(t *testing.T) { }) //ignore response for tests - _, err = options.ManagementClient.DeleteResource(ctx, "Applications.Core/containers", "containerY") + _, err = options.ManagementClient.DeleteResource(ctx, "Applications.Core/containers", "containerY", false) require.NoErrorf(t, err, "failed to delete resource containerY") err = DeleteAppWithoutDeletingResources(t, ctx, options, appNameUnassociatedResources) require.NoErrorf(t, err, "failed to delete application %s", appNameUnassociatedResources) @@ -618,7 +618,7 @@ func Test_CLI_Delete(t *testing.T) { require.NoErrorf(t, err, "failed to delete %s", appNameEmptyResources) //ignore response for tests - _, err = options.ManagementClient.DeleteResource(ctx, "Applications.Core/containers", "containerX") + _, err = options.ManagementClient.DeleteResource(ctx, "Applications.Core/containers", "containerX", false) require.NoErrorf(t, err, "failed to delete resource containerX") }) diff --git a/test/validation/shared.go b/test/validation/shared.go index 68b872b1f2..a09f81ca95 100644 --- a/test/validation/shared.go +++ b/test/validation/shared.go @@ -22,7 +22,6 @@ import ( "net/http" "strings" "testing" - "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/radius-project/radius/pkg/cli" @@ -119,27 +118,9 @@ func DeleteRPResourceSilent(ctx context.Context, cli *radcli.CLI, client clients return cli.ApplicationDelete(ctx, resource.Name) } else { // Handle other resource types (like ExtendersResource, ContainersResource, etc.) - - // Retry deletion with exponential backoff for 409 Conflict errors - // Resources may be stuck in "Updating" state after failed deployments - maxRetries := 5 - var err error - for attempt := range maxRetries { - _, err = client.DeleteResource(ctx, resource.Type, resource.Name) - if err == nil { - break - } - - // Check if it's a 409 Conflict error (resource is updating) - if strings.Contains(err.Error(), "409") && strings.Contains(err.Error(), "Conflict") { - if attempt < maxRetries-1 { - waitTime := time.Duration(1<