Handle 404 as success in recipe deletion (#6467)

# 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

<!--

Please select **one** of the following options that describes your
change and delete the others. Clearly identifying the type of change you
are making will help us review your PR faster, and is used in authoring
release notes.

If you are making a bug fix or functionality change to Radius and do not
have an associated issue link please create one now.

-->

- 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).

<!--

Please update the following to link the associated issue. This is
required for some kinds of changes (see above).

-->

Fixes: #6468

## Auto-generated summary

<!--
GitHub Copilot for docs will auto-generate a summary of the PR
-->

<!--
copilot:all
-->
### <samp>🤖 Generated by Copilot at f2f60c4</samp>

### Summary
🧪🐛🚀

<!--
1.  🧪 for adding unit tests
2.  🐛 for fixing a formatting issue
3.  🚀 for enhancing and improving the code
-->
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

@@ -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,16 +29,18 @@ 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
 	}
 
 	// 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

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")
+	}
+}

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
 		}

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
 	}
 

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{