Custom support for File Share Protected Items

[thomas11]

Resolves #1420

Note: the integration test succeeds in creating and deleting the resources under test. However, it fails at the end because the storage account cannot be deleted as it's locked. I wasn't yet able to figure out why or how to remove the lock.

The recoveryservices.ProtectedItem represents a resource that's backed up via Azure Backup Service (which confusingly has the namespace Microsoft.RecoveryServices). ProtectedItem is a sort of union type with one member per Azure resource that can be protected, e.g., AzureVmWorkloadProtectedItem.

One of these types, AzureFileshareProtectedItem, is special in that it requires a file share name as input that's not the name of the file share as given by the user and as shown by the portal, but an internal "system name". Therefore, attempting to use AzureFileshareProtectedItem in the standard way is not possible, because for file share "Foo", "protectedItemName": "Foo" will not work.

Instead, we need to look up the system name for the user-visible name "Foo" first, and use it instead.

My first attempt (branch tkappler/tkappler/recoveryservices-1420) was to implement a standard custom resource with custom CRUD methods that use the system name, after looking it up via the recoveryservices Azure SDK. That worked, but is a lot of custom code when the actual CRUD operations are completely standard, once the system name is known.

So in this PR I instead extended the CustomResource abstraction with a PreCreate method that modifies the user-given input when a resource is created. The name could be improved. This allows the provider to check if PreCreate is implemented for the current resource type, if yes, use it to transform the input, then carry on with the standard create code path. In this case, input

  "protectedItemName": "Foo"

becomes, after PreCreate looks up the system name via the Azure SDK,

{
  "protectedItemName": "azurefileshare;339f98592ed6329dc5f83bdbb8675bd3528bf58d2246d5e172615186febdc45c",
  "__friendlyProtectedItemName": "Foo"
}

When the ProtectedItem to be created is not a File Share, PreCreate does nothing.

[github-actions]

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

[codecov]

Codecov Report

Attention: Patch coverage is 46.77419% with 99 lines in your changes missing coverage. Please review.

Project coverage is 56.80%. Comparing base (3145291) to head (24abf52).

Files with missing lines	Patch %	Lines
...sources/customresources/custom_recoveryservices.go	46.30%	60 Missing and 20 partials ⚠️
provider/pkg/azure/client.go	50.00%	10 Missing ⚠️
provider/pkg/gen/schema.go	0.00%	2 Missing and 1 partial ⚠️
provider/pkg/provider/provider.go	66.66%	1 Missing and 2 partials ⚠️
...r/pkg/resources/customresources/customresources.go	40.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3809      +/-   ##
==========================================
- Coverage   56.95%   56.80%   -0.16%     
==========================================
  Files          78       79       +1     
  Lines       12062    12246     +184     
==========================================
+ Hits         6870     6956      +86     
- Misses       4692     4766      +74     
- Partials      500      524      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mjeffryes]

I think we need to rethink the design of
CustomResource a bit, but I'm eager to get something to the customer here so don't want to block on that.

RE the locked storage account, my quick search implies that the recoveryservice-protected item is probably adding a blob with a retention policy which is blocking the delete. I have no idea how you'd go about removing that, and that would obviously be the first choice. However, one workaround could be to just provision a well-known storage account in a separate stack that we leave up and just use it via stack references in the test.

[danielrbradley]

Not 100% sure yet that mutating the input set is the right way to go here.

Mutating inputs is the job of the Check function. It's somewhat irregular for the Create or Update method to modify Inputs. That said, Check generally shouldn't do network calls as I think it's currently not run in parallel between resources and so is a performance bottleneck.

Going back to the reason we're needing to modify the inputs: as far as I can tell from the code, this is in order to fix the protectedItemName so that the id is generated correctly which is done external to the custom Create method and passed in.

A first alternate would be to not generate the id (and queryParams) before calling the custom Create method but instead pass the crudClient into the custom resource methods so the custom implementation can choose how to generate its own id.

A second alternative would be to allow custom resources to override specifically how they generate their own id (and perhaps queryParams). E.g. GetIdAndQuery func(ctx context.Context, inputs resource.PropertyMap, crudClient crud.ResourceCrudClient) (string, map[string]any, error)

[danielrbradley]

I see the unit test changed from:

- azurefileshare;339f9859
+ azurefileshare%3B339f9859

Was this just a typo or does the deep copy do some weird double encoding?

[thomas11]

I see the unit test changed from:

- azurefileshare;339f9859
+ azurefileshare%3B339f9859

Was this just a typo or does the deep copy do some weird double encoding?

It's actually our crudClient.PrepareAzureRESTIdAndQuery(inputsCopy) that does it. It URL-encodes each parameter - which is correct and was wrong in the previous code. The ; turns to %3B. The Azure API seems to accept both forms.