From aedc5f26f72ecd89aa9c4f114998d3a606e2597a Mon Sep 17 00:00:00 2001 From: Will Smith Date: Fri, 13 Oct 2023 20:48:43 +0100 Subject: [PATCH] Handle 404 as success in recipe deletion (#6467) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description * Fixes an issue if you try to delete a recipe with nested azure resources. For some resources (like Azure ServiceBus and Azure SQL), if the top-level resource gets deleted, we still try to delete the sub-resources. This results in a 404 error, which we should handle in our code as "already deleted". ## Type of change - This pull request fixes a bug in Radius and has an approved issue (issue link required). - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). Fixes: https://github.com/radius-project/radius/issues/6468 ## Auto-generated summary ### ๐Ÿค– Generated by Copilot at f2f60c4 ### Summary ๐Ÿงช๐Ÿ›๐Ÿš€ This pull request enhances the `Is404Error` function in the `pkg/cli/clients` package to handle more types of 404 errors, and uses it to improve the error handling of the `deleteAzureResource` function in the `pkg/portableresources/processors` package. It also adds unit tests for both functions and fixes a minor formatting issue in the `pkg/corerp/handlers` package. > _Sing, O Muse, of the code review that tested the skillful programmers_ > _Who sought to enhance the `Is404Error` function, a helper of clients_ > _And to improve the `deleteAzureResource` function, a destroyer of ARM_ > _And to fix the formatting of the log message, a guide of the proxy_ ### Walkthrough * Add unit tests for `Is404Error` function in `errors_test.go` ([link](https://github.com/radius-project/radius/pull/6467/files?diff=unified&w=0#diff-d1d1bc081d6d8848bd347c3ef8d06b3d5aeeb4e19b42ed5a8b26ef1a1c005536R1-R65)) * Import `net/http` package to use `http.StatusNotFound` constant in `Is404Error` function ([link](https://github.com/radius-project/radius/pull/6467/files?diff=unified&w=0#diff-9d16b3ee1ba941f7b1f44e52d83f3baf301e00f1ba95a85ddfe48b2db62d1ae1R22)) * Update comment of `Is404Error` function to reflect additional condition ([link](https://github.com/radius-project/radius/pull/6467/files?diff=unified&w=0#diff-9d16b3ee1ba941f7b1f44e52d83f3baf301e00f1ba95a85ddfe48b2db62d1ae1L31-R35)) * Add condition to check if error is a `ResponseError` with a `StatusCode` of 404 in `Is404Error` function ([link](https://github.com/radius-project/radius/pull/6467/files?diff=unified&w=0#diff-9d16b3ee1ba941f7b1f44e52d83f3baf301e00f1ba95a85ddfe48b2db62d1ae1L40-R43)) * Use `fmt.Sprintf` to format log message with proxy name in `checkHTTPProxyStatus` function ([link](https://github.com/radius-project/radius/pull/6467/files?diff=unified&w=0#diff-397d63d65f44b2c85cd035e75632e13cbcd0d89c0610f44e818bd5bd809e1138L140-R140)) * Import `clients` package to use `Is404Error` function in `deleteAzureResource` function ([link](https://github.com/radius-project/radius/pull/6467/files?diff=unified&w=0#diff-ba263c7aa6158e7fdb51e32b1e42473d51fe6dbba4cd83b78bd0291d0465d629R29)) * Add condition to return nil if error is a 404 error in `deleteAzureResource` function ([link](https://github.com/radius-project/radius/pull/6467/files?diff=unified&w=0#diff-ba263c7aa6158e7fdb51e32b1e42473d51fe6dbba4cd83b78bd0291d0465d629R121-R124), [link](https://github.com/radius-project/radius/pull/6467/files?diff=unified&w=0#diff-ba263c7aa6158e7fdb51e32b1e42473d51fe6dbba4cd83b78bd0291d0465d629R130-R134)) * Add subtest to `Test_Delete_ARM` function to test deletion of ARM resource with 404 error ([link](https://github.com/radius-project/radius/pull/6467/files?diff=unified&w=0#diff-d0fbe2ec4dd28b47316f47c86d3a80f36e234b6dd2135cb47394d81cfcb8fe66R129-R155)) --- pkg/cli/clients/errors.go | 9 ++- pkg/cli/clients/errors_test.go | 65 +++++++++++++++++++ .../handlers/kubernetes_http_proxy_waiter.go | 2 +- .../processors/resourceclient.go | 21 ++++++ .../processors/resourceclient_test.go | 43 ++++++++++++ 5 files changed, 136 insertions(+), 4 deletions(-) create mode 100644 pkg/cli/clients/errors_test.go diff --git a/pkg/cli/clients/errors.go b/pkg/cli/clients/errors.go index d53f3da464..5dd20fee58 100644 --- a/pkg/cli/clients/errors.go +++ b/pkg/cli/clients/errors.go @@ -19,6 +19,7 @@ package clients import ( "encoding/json" "errors" + "net/http" "github.com/Azure/azure-sdk-for-go/sdk/azcore" v1 "github.com/radius-project/radius/pkg/armrpc/api/v1" @@ -28,8 +29,10 @@ import ( // Is404Error returns true if the error is a 404 payload from an autorest operation. // -// "Is404Error" checks if the given error is a 404 error by checking if it is a ResponseError with an ErrorCode of -// "NotFound" or an ErrorResponse with an Error Code of "NotFound". +// "Is404Error" checks if the given error is a 404 error by checking if it is one of: +// a ResponseError with an ErrorCode of "NotFound", or +// a ResponseError with a StatusCode of 404, or +// an ErrorResponse with an Error Code of "NotFound". func Is404Error(err error) bool { if err == nil { return false @@ -37,7 +40,7 @@ func Is404Error(err error) bool { // The error might already be an ResponseError responseError := &azcore.ResponseError{} - if errors.As(err, &responseError) && responseError.ErrorCode == v1.CodeNotFound { + if errors.As(err, &responseError) && responseError.ErrorCode == v1.CodeNotFound || responseError.StatusCode == http.StatusNotFound { return true } else if errors.As(err, &responseError) { return false diff --git a/pkg/cli/clients/errors_test.go b/pkg/cli/clients/errors_test.go new file mode 100644 index 0000000000..c9f8626320 --- /dev/null +++ b/pkg/cli/clients/errors_test.go @@ -0,0 +1,65 @@ +/* +Copyright 2023 The Radius Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package clients + +import ( + "errors" + "net/http" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + v1 "github.com/radius-project/radius/pkg/armrpc/api/v1" +) + +func TestIs404Error(t *testing.T) { + var err error + + // Test with a ResponseError with an ErrorCode of "NotFound" + err = &azcore.ResponseError{ErrorCode: v1.CodeNotFound} + if !Is404Error(err) { + t.Errorf("Expected Is404Error to return true for ResponseError with ErrorCode of 'NotFound', but it returned false") + } + + // Test with a ResponseError with a StatusCode of 404 + err = &azcore.ResponseError{StatusCode: http.StatusNotFound} + if !Is404Error(err) { + t.Errorf("Expected Is404Error to return true for ResponseError with StatusCode of 404, but it returned false") + } + + // Test with an ErrorResponse with an Error Code of "NotFound" + err = errors.New(`{"error": {"code": "NotFound"}}`) + if !Is404Error(err) { + t.Errorf("Expected Is404Error to return true for ErrorResponse with Error Code of 'NotFound', but it returned false") + } + + // Test with an ErrorResponse with a different Error Code + err = errors.New(`{"error": {"code": "SomeOtherCode"}}`) + if Is404Error(err) { + t.Errorf("Expected Is404Error to return false for ErrorResponse with Error Code of 'SomeOtherCode', but it returned true") + } + + // Test with a different error type + err = errors.New("Some other error") + if Is404Error(err) { + t.Errorf("Expected Is404Error to return false for error of type %T, but it returned true", err) + } + + // Test with a nil error + if Is404Error(nil) { + t.Errorf("Expected Is404Error to return false for nil error, but it returned true") + } +} diff --git a/pkg/corerp/handlers/kubernetes_http_proxy_waiter.go b/pkg/corerp/handlers/kubernetes_http_proxy_waiter.go index 03edcfd88b..55435bdb61 100644 --- a/pkg/corerp/handlers/kubernetes_http_proxy_waiter.go +++ b/pkg/corerp/handlers/kubernetes_http_proxy_waiter.go @@ -137,7 +137,7 @@ func (handler *httpProxyWaiter) checkHTTPProxyStatus(ctx context.Context, dynami if len(p.Spec.Includes) == 0 && len(p.Spec.Routes) > 0 { // This is a route HTTP proxy. We will not validate deployment completion for it and return success here - logger.Info("Not validating the deployment of route HTTP proxy for ", p.Name) + logger.Info(fmt.Sprintf("Not validating the deployment of route HTTP proxy for %s", p.Name)) doneCh <- nil return true } diff --git a/pkg/portableresources/processors/resourceclient.go b/pkg/portableresources/processors/resourceclient.go index 8543a99e86..1332b29073 100644 --- a/pkg/portableresources/processors/resourceclient.go +++ b/pkg/portableresources/processors/resourceclient.go @@ -26,6 +26,7 @@ import ( "github.com/radius-project/radius/pkg/azure/armauth" "github.com/radius-project/radius/pkg/azure/clientv2" aztoken "github.com/radius-project/radius/pkg/azure/tokencredentials" + "github.com/radius-project/radius/pkg/cli/clients" "github.com/radius-project/radius/pkg/cli/clients_new/generated" "github.com/radius-project/radius/pkg/sdk" "github.com/radius-project/radius/pkg/trace" @@ -117,11 +118,21 @@ func (c *resourceClient) deleteAzureResource(ctx context.Context, id resources.I poller, err := client.BeginDeleteByID(ctx, id.String(), apiVersion, &armresources.ClientBeginDeleteByIDOptions{}) if err != nil { + if clients.Is404Error(err) { + // If the resource that we want to delete doesn't exist, we don't need to delete it. + return nil + } + return err } _, err = poller.PollUntilDone(ctx, nil) if err != nil { + if clients.Is404Error(err) { + // If the resource that we want to delete doesn't exist, we don't need to delete it. + return nil + } + return err } @@ -175,11 +186,21 @@ func (c *resourceClient) deleteUCPResource(ctx context.Context, id resources.ID) poller, err := client.BeginDelete(ctx, id.Name(), nil) if err != nil { + if clients.Is404Error(err) { + // If the resource that we want to delete doesn't exist, we don't need to delete it. + return nil + } + return err } _, err = poller.PollUntilDone(ctx, nil) if err != nil { + if clients.Is404Error(err) { + // If the resource that we want to delete doesn't exist, we don't need to delete it. + return nil + } + return err } diff --git a/pkg/portableresources/processors/resourceclient_test.go b/pkg/portableresources/processors/resourceclient_test.go index bdc9667f9a..cd1b55fe06 100644 --- a/pkg/portableresources/processors/resourceclient_test.go +++ b/pkg/portableresources/processors/resourceclient_test.go @@ -126,6 +126,33 @@ func Test_Delete_ARM(t *testing.T) { require.NoError(t, err) }) + t.Run("success - deletion returns 404", func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc(ARMResourceID, handleNotFound(t)) + mux.HandleFunc(ARMProviderPath, handleJSONResponse(t, armresources.Provider{ + Namespace: to.Ptr("Microsoft.Compute"), + ResourceTypes: []*armresources.ProviderResourceType{ + { + ResourceType: to.Ptr("anotherType"), + APIVersions: []*string{}, + }, + { + ResourceType: to.Ptr("virtualMachines"), + DefaultAPIVersion: to.Ptr(ARMAPIVersion), + }, + }, + }, 200)) + + server := httptest.NewServer(mux) + defer server.Close() + + c := NewResourceClient(newArmOptions(server.URL), nil, nil, nil) + c.armClientOptions = newClientOptions(server.Client(), server.URL) + + err := c.Delete(context.Background(), ARMResourceID) + require.NoError(t, err) + }) + t.Run("failure - lookup API Version - provider not found", func(t *testing.T) { mux := http.NewServeMux() mux.HandleFunc(ARMProviderPath, handleNotFound(t)) @@ -282,6 +309,22 @@ func Test_Delete_UCP(t *testing.T) { require.NoError(t, err) }) + t.Run("success - delete returns 404", func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc(AWSResourceID, handleNotFound(t)) + + server := httptest.NewServer(mux) + defer server.Close() + + connection, err := sdk.NewDirectConnection(server.URL) + require.NoError(t, err) + + c := NewResourceClient(nil, connection, nil, nil) + + err = c.Delete(context.Background(), AWSResourceID) + require.NoError(t, err) + }) + t.Run("failure - delete fails", func(t *testing.T) { mux := http.NewServeMux() mux.HandleFunc(AWSResourceID, handleJSONResponse(t, v1.ErrorResponse{