feat(providers): Add support for Azure immutability protection #3412

KastenMike · 2023-10-27T12:10:21Z

Adds the option to set retention for Azure & extend them as part of full maintenance ObjectLocking. They'd need to set the mode to Locked and run the command kopia maintenance set --extend-object-locks true like on S3.

Only includes Locked since Unlocked is not recommended by Microsoft. PutBlob with an invalid type (e.g. Locked for S3) will fail. The same validation check could also be done on ExtendBlobRetention or it can be left to fail naturally...technically PutBlob can also be left to fail naturally rather than having the guard clause like opts.HasRetentionOptions() && !opts.RetentionMode.IsValidAzure()...instead put on Azure CLI would just fail like "COMPLIANCE is unknown retention type"

$HOME/bin/kopia repository set-parameters --retention-mode=Locked --retention-period=72h0m0s
putblob mode: Locked and retention 72h0m0s...
putblob mode: Locked and retention 72h0m0s...

$HOME/bin/kopia repository set-parameters --retention-mode=COMPLIANCE --retention-period=54h0m0s
ERROR error updating repository parameters: error setting parameters: unable to write blobcfg blob: PutBlob() failed for "kopia.blobcfg": blob retention mode is not valid for Azure: unsupported put-blob option

repo/blob/azure/azure_storage_test.go

codecov · 2023-11-12T20:05:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0d60d8e) 75.79% compared to head (76450b2) 75.83%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3412      +/-   ##
==========================================
+ Coverage   75.79%   75.83%   +0.03%     
==========================================
  Files         465      465              
  Lines       37180    37180              
==========================================
+ Hits        28182    28194      +12     
+ Misses       7066     7056      -10     
+ Partials     1932     1930       -2

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

just dont merge until ready to merge kopia#3412

repo/blob/azure/azure_storage_test.go

Shrekster · 2023-11-17T02:11:15Z

cli/command_repository_create.go

 	cmd.Flag("create-only", "Create repository, but don't connect to it.").Short('c').BoolVar(&c.createOnly)
 	cmd.Flag("format-version", "Force a particular repository format version (1, 2 or 3, 0==default)").IntVar(&c.createFormatVersion)
-	cmd.Flag("retention-mode", "Set the blob retention-mode for supported storage backends.").EnumVar(&c.retentionMode, blob.Governance.String(), blob.Compliance.String())
+	cmd.Flag("retention-mode", "Set the blob retention-mode for supported storage backends.").EnumVar(&c.retentionMode, blob.Governance.String(), blob.Compliance.String(), blob.Locked.String())


While Kopia works with multiple tiers underneath, but the storage APIs and CLIs are designed to work seamlessly across all with minimal storage specific couplings. There are a few places where it is just unavoidable to expose storage specific parameters to the CLI/API.

Maybe we should rename the retention-modes in Kopia from "governance" / "compliance" to something more generic like "WORM Mode" / "RBAC Mode" or similar to keep things generic between S3/GCS/Azure/etc. However, this can be done in a separate PR; no need to overload this PR.

I am wondering whether we need to make an exception for Azure immutability as well. For all intents and purposes of Azure usage within Kopia, ain't Azure "locked" mode is just the same as the governance mode ? So, for Azure can we use the existing mode of Kopia called "governance" as its locked mode ? That way, we'll not need to introduce a new mode and going forward we can make things more generic and rename governance to "WORM Mode" or something more generic so it doesn't sound like AWSish.

Locked is more like Compliance mode (Governance is not so secure as certain users can override the protections).
The main reasons against using Compliance:

Azure is kind of like a 2nd class citizen then

In the azure_storage we'd need to replace Compliance with Locked in put & extend methods
Can be done but looks a bit strange imo - if GCP eventually adds support then they may also have different names at that point too

"WORM Mode" / "RBAC Mode" I guess would be a breaking change for existing setups

I've updated it to use COMPLIANCE although I didn't update #3405 - it could be updated when this is merged to make use of the blob.Locked from this PR

Thanks Mike. You have valid points. I am not hard on it, but thanks for making the change. Next time when we make the change of "WORM Mode" / "RBAC Mode" then we'll have to do less number of these.

I'm leaning more to going back to Locked because of the ugly if check

No worries, thanks for trying.

What is the ugly IF check BTW ?

We should try to provide an uniform interface in the CLI. We only had a single immutability implementation (for S3) and unfortunately we did not anticipate creating that interface.

Here's a straw-man proposal, please beat it and find holes.

What about accepting all these (case-insensitive) values in the CLI for both S3 and Azure.

"Locked"

"Compliance"

"Unlocked"

"Governance"

And having an internal translation to a canonical {locked, unlocked} representation.
And then in each provider translate from the canonical representation to the provider specific setting.

For S3 object locking:

locked => "Compliance"

unlocked => "Governance"

For Azure immutability:

locked => "Locked"

unlocked => "Unlocked"

Thoughts?

Again, let's do this in a separate PR, and merge this one after making the requested changes.

As mentioned in the other comment I don't think we should add an Unlocked option considering this is meant to be protecting backups. Microsoft describes Unlocked as only to be used for test purposes and to be switched to Locked as soon as possible

Accepting all values for all makes it kind of strange for understanding e.g. Locked means nothing to S3; but then Compliance means nothing to Azure. But at least this way there is less code (no need to replace Locked with Compliance in S3 code) and we can just add a note in the docs like "Whether you set Compliance or Governance for Azure, Locked protection will always be used"

@Shrekster I removed the if opts.RetentionMode == blob.Compliance in this commit by hardcoding Locked in the exposed PutBlob rather than having a check inside putBlob and in the process made it possible to set by Compliance or Governance for Azure

repo/blob/azure/azure_storage_test.go

julio-lopez

Let's merge this after addressing the comments about:

preserving the configured retention mode (locked vs. unlocked)
the changes for the test

and making sure tests pass in the test/providers job.

Let's keep the (kopia) CLI changes out and do that in a separate PR.

julio-lopez · 2023-11-30T00:45:29Z

repo/blob/azure/azure_storage.go


-	_, err := az.putBlob(ctx, b, data, opts)
+	_, err := az.putBlob(ctx, b, data, blob.PutOptions{
+		RetentionMode:   blob.Locked,


This should respect the configured option. It should not be hardcoded to locked.

@KastenMike IIUC, is this "forcing" immutability, even if immutability has not been set by the user and is not supported by the container?

I agree that "locked" should be the only mode that is allowed for Azure immutability. However, this cannot be hardcoded here, we need to keep the options that are set by the user. Lets do the validation and enforcement in the CLI PR.

Does it cause issues (for example, errors) to specify a policy mode when putting a blob in a container that does not support immutability?
It appears that it does break the current functionality. The providers test is failing due to this change.

https://github.com/kopia/kopia/actions/runs/7053235548/job/19199911672?pr=3412

good point :) seems I went to extreme trying to remove the if statement before 😄 fixed

repo/blob/storage.go

repo/blob/azure/azure_storage_test.go

julio-lopez · 2023-11-30T05:02:38Z

repo/blob/azure/azure_storage.go

 	}

 	if opts.HasRetentionOptions() {
+		// kopia delete marker blob can be Unlocked rather than Compliance


Not sure this comment is relevant. Whether a blob is locked or not depends on how the user configured it, that is, the option specified in the configuration should be respected.

Suggested change

// kopia delete marker blob can be Unlocked rather than Compliance

// kopia delete marker blob must be "Unlocked", so it cannot be overridden to "Locked" here.

repo/blob/azure/azure_storage_test.go

…re-immu-prot # Conflicts: # repo/blob/azure/azure_storage.go # repo/blob/azure/azure_storage_test.go

julio-lopez · 2023-11-30T23:09:39Z

repo/blob/azure/azure_immu_test.go

+)
+
+// TestAzureStorageImmutabilityProtection runs through the behavior of Azure immutability protection.
+func TestAzureStorageImmutabilityProtection(t *testing.T) {


Test looks good.
Filed #3476 to followup on the test cleanup.

julio-lopez

Thanks Mike

julio-lopez · 2023-12-02T05:18:52Z

repo/blob/azure/azure_storage.go

 	}

-	_, err := az.putBlob(ctx, b, data, opts)
+	hardcodedOpts := blob.PutOptions{


nit: not sure about the variable name.
Anyway, merging now and we'll modify it separately.

julio-lopez · 2023-12-02T05:20:20Z

repo/blob/azure/azure_storage.go

 	}

 	if opts.HasRetentionOptions() {
+		// kopia delete marker blob can be Unlocked rather than Compliance


Suggested change

// kopia delete marker blob can be Unlocked rather than Compliance

// kopia delete marker blob must be "Unlocked", so it cannot be overridden to "Locked" here.

feat(repository): Add support for Azure immutability protection

585ef25

KastenMike commented Oct 27, 2023

View reviewed changes

repo/blob/azure/azure_storage_test.go Outdated Show resolved Hide resolved

KastenMike requested review from Shrekster, jkowalski and julio-lopez November 2, 2023 20:03

KastenMike mentioned this pull request Nov 8, 2023

feat(repository): Add Azure PIT support #3407

Merged

tweak

0faaed2

KastenMike added a commit to KastenMike/kopia that referenced this pull request Nov 12, 2023

remove protection

1a6f935

just dont merge until ready to merge kopia#3412

KastenMike added 2 commits November 12, 2023 21:30

lint

098686e

t.Cleanup

a8ab25c

KastenMike assigned jkowalski Nov 15, 2023

KastenMike commented Nov 15, 2023

View reviewed changes

repo/blob/azure/azure_storage_test.go Outdated Show resolved Hide resolved

julio-lopez assigned julio-lopez and unassigned jkowalski Nov 16, 2023

julio-lopez removed the request for review from jkowalski November 16, 2023 01:13

Shrekster reviewed Nov 17, 2023

View reviewed changes

repo/blob/azure/azure_storage_test.go Outdated Show resolved Hide resolved

KastenMike added 2 commits November 17, 2023 11:37

use COMPLIANCE instead of Locked

50f8ded

allow for unlocked command

bee24b3

julio-lopez changed the title ~~feat(repository): Add support for Azure immutability protection~~ feat(providers): Add support for Azure immutability protection Nov 29, 2023

spring cleaning

454f115

julio-lopez reviewed Nov 30, 2023

View reviewed changes

KastenMike added 3 commits November 30, 2023 15:07

Merge branch 'kasten' of ssh://github.com/kastenhq/kopia into add-Azu…

c78e6fc

…re-immu-prot # Conflicts: # repo/blob/azure/azure_storage.go # repo/blob/azure/azure_storage_test.go

new test file + testImmutableContainerEnv

670f93c

add flake comment

f7ca63f

julio-lopez reviewed Nov 30, 2023

View reviewed changes

julio-lopez mentioned this pull request Nov 30, 2023

Testing: create container for Azure immutability testing during test execution #3477

Open

julio-lopez requested a review from redgoat650 November 30, 2023 23:24

only set locked mode when HasRetentionOptions

76450b2

julio-lopez approved these changes Dec 2, 2023

View reviewed changes

julio-lopez merged commit 936ed13 into kopia:master Dec 2, 2023

julio-lopez deleted the add-Azure-immu-prot branch December 2, 2023 06:07

	// kopia delete marker blob can be Unlocked rather than Compliance
	// kopia delete marker blob must be "Unlocked", so it cannot be overridden to "Locked" here.

feat(providers): Add support for Azure immutability protection #3412

feat(providers): Add support for Azure immutability protection #3412

Uh oh!

Conversation

KastenMike commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Nov 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

KastenMike Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

julio-lopez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julio-lopez Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

julio-lopez left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

KastenMike commented Oct 27, 2023 •

edited

Loading

codecov bot commented Nov 12, 2023 •

edited

Loading

KastenMike Nov 21, 2023 •

edited

Loading

julio-lopez Nov 30, 2023 •

edited

Loading