Add support for Immutable Storage on container #1144

xperiandri · 2024-10-24T20:36:17Z

Implemented:

ImmutableStorageWithVersioning
RequireInfrastructureEncryptionThis PR closes #

I have read the contributing guidelines and have completed the following:

Tested my code end-to-end against a live Azure subscription.
Updated the documentation in the docs folder for the affected changes.
Written unit tests against the modified code that I have made.
Updated the release notes with a new entry for this PR.
Checked the coding standards outlined in the contributions guide and ensured my code adheres to them.

If I haven't completed any of the tasks above, I include the reasons why here:
Which test would you recommend to modify or implement?

Below is a minimal example configuration that includes the new features, which can be used to deploy to Azure:

let storage = storageAccount {
    name "wormtest222"
    sku Farmer.Storage.Sku.Standard_GRS
    disable_blob_public_access
    add_private_container
        "container"
        (blobContainerImmutabilityPolicies {
            allow_protected_append_writes AllAppendAllowed
            immutability_period_since_creation (365<Days> * 5)
        })
}

xperiandri · 2024-10-25T17:44:04Z

@ninjarobot do you have any remarks?

src/Farmer/Arm/Storage.fs

isaacabraham · 2024-10-26T18:03:44Z

You want to write at least one test confirmation the JSON that is emitted in the generate ARM template is correct i.e. checking fields etc.

isaacabraham · 2024-10-26T18:06:36Z

From an API design, what happens if you don't want to set any policies - do you need to provide a policy regardless (bear in mind computation expressions don't allow optional arguments AFAIK, and overloads must all have the same number of arguments)? I think perhaps creating a dedicated container { } CE which can have members on it including e.g.:

name
accessibility (public / private)
immutability_policy

would be a better way to go.

isaacabraham · 2024-10-26T18:28:03Z

Also can you make an issue first which clearly outlines what this PR is designed to solve - normally we create issues, get them approved, and then create a PR to resolve them (in that order :-))

ninjarobot

Looks great, thanks for adding this support for newer storage account functionality. I made a few minor suggestions you can take or leave as applicable.

src/Farmer/Types.fs

ninjarobot · 2024-10-26T21:41:53Z

src/Farmer/Builders/Builders.Storage.fs

-                DefaultToOAuthAuthentication = this.DefaultToOAuthAuthentication
+                DnsZoneType = this.DnsZoneType
+                EnableHierarchicalNamespace = this.EnableDataLake
+                ImmutableStorageWithVersioning = None // TODO: Implement


This looks implemented, so can remove the TODO, right?

@ninjarobot I mean here that there is not such operation in the builder. That is what needs to be implemented

Let's use GitHub issues rather than TODO comments to track possible enhancements. Would you mind creating one @xperiandri?

src/Farmer/Builders/Builders.Storage.fs

src/Farmer/Types.fs

isaacabraham

Before merging, we need to modify the API - the use of a CE in this regard isn't going to work, from what I can see, from a usability perspective.

xperiandri · 2024-10-27T15:31:03Z

From an API design, what happens if you don't want to set any policies - do you need to provide a policy regardless (bear in mind computation expressions don't allow optional arguments AFAIK, and overloads must all have the same number of arguments)?

@isaacabraham, actually it works fine as all the tests pass. I verified that it works both ways and allow that optional parameter to be specified and omitted

isaacabraham · 2024-10-27T18:59:18Z

@isaacabraham, actually it works fine as all the tests pass. I verified that it works both ways and allow that optional parameter to be specified and omitted

That's good :-) Nonetheless, I think moving to a dedicated container { } CE at this stage makes more sense.

This PR still needs tests for the new code that's been created as well as the other points mentioned (including an issue / issues that reflect the actual changes that are being made here - it's not so much about upgrading the versions but about immutable blobs etc.).

Happy to talk through with you the API choices and anything else that's needed.

xperiandri · 2024-10-27T19:35:26Z

I thought about that but considered immutable policy as not so big change to introduce container CE.
However further container customization support may need a CE

xperiandri · 2024-10-28T17:44:26Z

I've implemented tests. Let me know if I miss something

xperiandri · 2024-10-28T19:18:15Z

Before merging, we need to modify the API - the use of a CE in this regard isn't going to work, from what I can see, from a usability perspective.

@isaacabraham my idea was to add now and preserve in the future ability to specify immutability policy
And add blobContainer CE and operations to storageAccount CE accepting BlobContainerConfig along with supporting additional container settings.
I have no intent to add them right now as it widens the scope of this PR.
Do you think blobContainer should be coupled with these changes?

isaacabraham · 2024-10-31T00:43:16Z

I think so, yes. If you look elsewhere in the Farmer API, you won't really much in the way of CEs being supplied as secondary arguments to a custom keyword, and I really would like to retain the pattern that we have.

You can repurpose the CE you've already built to actually be the container one - just add in the extra name parameter. You can then add that as an overload for the top-level CE add_private_container member.

What do you think?

Thanks for adding the tests.

xperiandri · 2024-11-15T14:52:36Z

@isaacabraham I've implemented storageBlobContainer CE as you suggested.
Any other remarks?

mattgallagher92 · 2025-03-21T13:01:15Z

@xperiandri I've looked at the parts where you had TODOs in more depth based on ImmutableStorageWithVersioning. I see that there are two cases:

The immutableStorageWithVersioning field in the ARM template has a representation in the StorageAccount type that implements IArmResource, which affects the generated JSON. However there is no corresponding configuration available in the builder, and no test that the generated JSON is correct.
The extendedLocation and identity fields will always be se 62FB t to null in the generated JSON.

I have a few questions:

Why did you add them if they're not used? Are the fields required in the ARM template JSON?
If not, why not just leave them out of the Farmer codebase altogether?
Why have you made immutableStorageWithVersioning configurable via the F# type but not done the same for extendedLocation and identity?

xperiandri · 2025-03-21T13:06:19Z

Why did you add them if they're not used? Are the fields required in the ARM template JSON?

Not required, just to mark they exist but not implemented

If not, why not just leave them out of the Farmer codebase altogether?

I can if you prefer

Why have you made immutableStorageWithVersioning configurable via the F# type but not done the same for extendedLocation and identity

I only implemented the piece I need and the scope defined by the PR name

mattgallagher92 · 2025-03-21T15:19:47Z

Why have you made immutableStorageWithVersioning configurable via the F# type but not done the same for extendedLocation and identity

I only implemented the piece I need and the scope defined by the PR name

So you're using it directly on the Farmer.Arm.Storage.StorageAccount type rather than via the computation expression?

xperiandri · 2025-03-21T20:19:24Z

src/Farmer/Builders/Builders.Storage.fs

+type BlobContainerImmutabilityPoliciesBuilder() =
+    member _.Yield _ =
+        BlobContainerImmutabilityPoliciesConfig.Empty
+
+    member _.Run state : BlobContainerImmutabilityPoliciesConfig = state
+
+    /// Sets the AllowProtectedAppendWrites property.
+    [<CustomOperation "allow_protected_append_writes">]
+    member _.AllowProtectedAppendWrites(state: BlobContainerImmutabilityPoliciesConfig, value) = {
+        state with
+            AllowProtectedAppendWrites = Some value
+    }
+
+    /// Sets the ImmutabilityPeriodSinceCreation property.
+    [<CustomOperation "immutability_period_since_creation">]
+    member _.ImmutabilityPeriodSinceCreation(state: BlobContainerImmutabilityPoliciesConfig, value) = {
+        state with
+            ImmutabilityPeriodSinceCreation = Some value
+    }


So you're using it directly on the Farmer.Arm.Storage.StorageAccount type rather than via the computation expression?

@mattgallagher92 no, here is the builder

That affects Farmer.Arm.Storage.BlobContainers.ImmutabilityPolicies. I'm referring to Farmer.Arm.Storage.StorageAccount.

farmer/src/Farmer/Arm/Storage.fs

Lines 127 to 136 in e9678e5

ImmutableStorageWithVersioning:

{|

Enable: bool option

ImmutabilityPolicy:

{|

AllowProtectedAppendWrites: bool option

ImmutabilityPeriodSinceCreation: int<Days> option

State: ImmutabilityPolicyState option

|} option

|} option

I still have not got what your question is about.
Isaac asked to create a container CE.
I need immutability policies, so I also created CE for them.
So now we have blob container and blob container immutability policies CEs, and corresponding properties for them within the StorageAccount record.

Could you describe again what inconsistency you have found?

Sure 🙂 Thanks for your patience and sorry that this is dragging on so long. I don't have a lot of time available to review this (and unfortunately I'm going to be unavailable over the next few weeks).

When I search for references to the ImmutableStorageWithVersioning property on Farmer.Arm.Storage.StorageAccount, I see that it's used in 4 places. 3 of those places set it to None, and the remaining place uses it via Option.map, which will therefore also always return None. As far as I can see, there doesn't seem to be any means of setting it to anything other than None via a builder, so it seems to be that the Option.map code is not being tested and is perhaps even redundant.

Given it doesn't seem to be possible to set it via builder, I wondered whether you were setting the field directly on the record somehow, or whether this is something that you don't need.

@mattgallagher92 fixed, please review again

@xperiandri I see a commit labelled Implemented storage account level immutability policy setting and builder, but I don't actually see the implementation in the diff?

…lder

Fixed parsing version number on non-English cultures

…and builder

mattgallagher92 · 2025-06-13T13:46:18Z

@xperiandri I've re-reviewed. The unresolved conversations are the things that have not been addressed.

Note for other maintainers: I'm leaving Compositional IT, so I'm not going to be able to re-review this. There are a few unresolved conversations, but I think that this is good enough to go in as it is. Hopefully Andrii can easily add the missing builder implementation that is mentioned in the latest commit message (e0e6cea), but not actually present in the diff. Once that's done, the other changes are minor ones that maintainers can make before merging.

If preferred for whatever reason, I think that it would be reasonable to merge the code as is and make the outstanding tweaks later - the code is good enough.

xperiandri · 2025-06-13T16:02:13Z

I thought I have finally implemented all the builders

xperiandri force-pushed the storage-2023-05-01 branch 3 times, most recently from de707a0 to 3c3a3d8 Compare October 25, 2024 12:51

xperiandri commented Oct 25, 2024

View reviewed changes

src/Farmer/Arm/Storage.fs Show resolved Hide resolved

ninjarobot approved these changes Oct 26, 2024

View reviewed changes

ninjarobot added this to the 1.9.3 milestone Oct 26, 2024

isaacabraham self-requested a review October 27, 2024 14:54

isaacabraham requested changes Oct 27, 2024

View reviewed changes

xperiandri force-pushed the storage-2023-05-01 branch from ae28dca to 1949e2f Compare October 28, 20 8000 24 17:43

ninjarobot modified the milestones: 1.9.3, 1.9.4 Nov 3, 2024

isaacabraham changed the title ~~Updated Microsoft.Storage/storageAccounts to 2023-05-01~~ Add support for Immutable Storage on container Nov 5, 2024

ninjarobot modified the milestones: 1.9.4, 1.9.5, 1.9.6 Nov 6, 2024

xperiandri force-pushed the storage-2023-05-01 branch 2 times, most recently from 6a01d5e to 21088d9 Compare November 15, 2024 14:52

ninjarobot requested a review from isaacabraham November 16, 2024 13:40

xperiandri added 2 commits March 17, 2025 12:49

Fixed extension methods usage

458ef8a

Implemented blob container immutability policies of version 2023-05-01

5b52cf4

xperiandri force-pushed the storage-2023-05-01 branch from a7a767f to 6c60511 Compare March 17, 2025 08:52

This was referenced Mar 21, 2025

[Storage] Support extended location #1193

Open

[Storage] Support user assigned identity #1192

Open

xperiandri commented Mar 21, 2025

View reviewed changes

xperiandri force-pushed the storage-2023-05-01 branch from de818e5 to e9678e5 Compare March 28, 2025 12:11

xperiandri added 9 commits April 21, 2025 13:48

Added release notes and formatted code

a47928a

Added blobContainerImmutabilityPolicies usage sample

c190350

Fixed tests related to storage changes

d833fc6

Fixed code review remarks by @ninjarobot

6207b3d

Implemented Blob storage immutability policy builder tests

1cc8743

Implemented Blob storage container builder and tests

fd30133

Fixed PR review comments by @ mattgallagher92

70207b1

Implemented storage account level immutability policy setting and bui…

b28543f

…lder

Fixed Functions versioned runtime string output

8110a9b

Fixed parsing version number on non-English cultures

xperiandri force-pushed the storage-2023-05-01 branch from 303d80b to 8110a9b Compare April 21, 2025 09:52

fixup! Implemented storage account level immutability policy setting …

e0e6cea

…and builder

ninjarobot modified the milestones: 1.9.13, 1.9.14, 1.9.15 May 12, 2025

ninjarobot modified the milestones: 1.9.15, 1.9.16 May 27, 2025

FundOurselves closed this by deleting the head repository Jun 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for Immutable Storage on container #1144

Add support for Immutable Storage on container #1144

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	ImmutableStorageWithVersioning:
	{\|
	Enable: bool option
	ImmutabilityPolicy:
	{\|
	AllowProtectedAppendWrites: bool option
	ImmutabilityPeriodSinceCreation: int<Days> option
	State: ImmutabilityPolicyState option
	\|} option
	\|} option

Add support for Immutable Storage on container #1144

Add support for Immutable Storage on container #1144

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!