feat(repository): Add Azure PIT support #3407

KastenMike · 2023-10-26T13:41:14Z

Adds support for Azure PIT

This reverts commit de34324.

This reverts commit 629a20b.

This reverts commit 695e37a.

codecov · 2023-11-01T00:30:31Z

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (174f614) 75.80% compared to head (7a2fc25) 75.82%.
Report is 21 commits behind head on master.

Files	Patch %	Lines
cli/storage_azure.go	26.66%	10 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3407      +/-   ##
==========================================
+ Coverage   75.80%   75.82%   +0.01%     
==========================================
  Files         465      465              
  Lines       37165    37180      +15     
==========================================
+ Hits        28174    28192      +18     
- Misses       7059     7061       +2     
+ Partials     1932     1927       -5

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

julio-lopez · 2023-11-01T05:09:20Z

@KastenMike see initial comments

repo/blob/azure/azure_storage.go

julio-lopez · 2023-10-31T23:56:25Z

repo/blob/azure/azure_storage.go


+func (az *azStorage) getBlobName(it *azblobmodels.BlobItem) string {
+	n := *it.Name
+	return n[len(az.Prefix):]


Use strings.TrimPrefix here, it is more robust.

Also additional defensive programming should be applied here to prevent a panic due to a nil dereference. Given the Azure SDK API, where all the structs have pointers, then it makes sense to add helpers that either (a) validate and return an error; or (b) return sane default values.

Suggested change

return n[len(az.Prefix):]

return strings.TrimPrefix(toString(*it.Name), az.prefix)

toString would be defined as:

func toString(s *string) string { return stringDefault(s, "") }

This code has been in kopia for a long time so I guess azure returns an error rather than null values for the common metadata. I just extracted it to a method

julio-lopez · 2023-11-01T02:35:08Z

repo/blob/azure/azure_storage.go

+	return n[len(az.Prefix):]
+}
+
+func (az *azStorage) getBlobMeta(it *azblobmodels.BlobItem) blob.Metadata {


check for it != nil

The same as above~ the code has been in since February without issue so I guess there is always a blob item if the loop is entered

Fair enough. The hardening can be done separately.

julio-lopez · 2023-11-01T02:39:58Z

repo/blob/azure/azure_storage.go

+func (az *azStorage) getBlobMeta(it *azblobmodels.BlobItem) blob.Metadata {
+	bm := blob.Metadata{
+		BlobID: blob.ID(az.getBlobName(it)),
+		Length: *it.Properties.ContentLength,


Check to ensure that:

it.Properties != nil

it.Properties.ContentLength != nil

before dereferencing.

julio-lopez · 2023-11-01T02:41:23Z

repo/blob/azure/azure_storage.go

+	}
+
+	// see if we have 'Kopiamtime' metadata, if so - trust it.
+	if t, ok := timestampmeta.FromValue(stringDefault(it.Metadata["kopiamtime"], "")); ok {


nit: after adding toString, then it can be used here.

Suggested change

if t, ok := timestampmeta.FromValue(stringDefault(it.Metadata["kopiamtime"], "")); ok {

if t, ok := timestampmeta.FromValue(toString(it.Metadata["kopiamtime"])); ok {

julio-lopez · 2023-11-01T02:42:38Z

repo/blob/azure/azure_storage.go

+	if t, ok := timestampmeta.FromValue(stringDefault(it.Metadata["kopiamtime"], "")); ok {
+		bm.Timestamp = t
+	} else {
+		bm.Timestamp = *it.Properties.LastModified


ditto: validate the chain to ensure there are no nil components

repo/blob/azure/azure_pit.go

julio-lopez

LG overall.

I am not completely sure about the handling of the deletion marker.
And a few inline comments.

julio-lopez · 2023-11-03T23:25:18Z

repo/blob/azure/azure_storage.go

+	return n[len(az.Prefix):]
+}
+
+func (az *azStorage) getBlobMeta(it *azblobmodels.BlobItem) blob.Metadata {


Fair enough. The hardening can be done separately.

repo/blob/azure/azure_versioned_test.go

repo/blob/azure/azure_pit.go

repo/blob/azure/azure_versioned_test.go

julio-lopez · 2023-11-04T01:32:51Z

Provider tests are failing for Azure Blob
https://github.com/kopia/kopia/actions/runs/6751751502/job/18356268523?pr=3407

julio-lopez · 2023-11-08T05:32:26Z

repo/blob/azure/azure_versioned_test.go

+	"github.com/kopia/kopia/repo/format"
+)
+
+func TestGetBlobVersions(t *testing.T) {


@KastenMike This test and the one below are failing in the "test/providers" job. The likely cause is that the buckets used in the test do not have the required setup, that is version-level immutability enabled.

That's the only thing left to merge this PR.

Did you re-run the tests? I cannot see run history & you didn't link a new run. Now it doesn't check for version-level immutability

OK I see it in Actions. Someone should enable versioning on the related storage account for it to work https://learn.microsoft.com/en-us/azure/storage/blobs/versioning-enable?tabs=portal

In the process whoever sets it could enable version-level immutability on the container since it seems the SDK doesn't support it and it's required for #3412 tests

@jkowalski is it something you'd be able to enable?

Mike: this requires a little more than that.

These 2 tests need to run in a different setup. That means:

setting up a separate storage account with version-level immutability enabled (as you mentioned).

create a new bucket with version-level immutability, or have the test create it with the desired settings.

these tests need to use a separate 'credentials setting' from the environment. The actual credentials may be the same, but the storage account and bucket would be different.

the GH environment needs to be updated with these new cred information.

Why does it require a separate storage account? you could simply have the same one running new + existing tests but with 2 new settings changed. When turning on versioning it gives the option to use Azure's BLM which can be set to delete versions older than 1 day

@KastenMike For what I understand, the storage account currently used in the tests does not have versioning, nor immutability enabled. I thought that version immutability needed to be enabled at the storage account level. This is what I recall but I could be mistaken. I'll double check.

Also, we don't want to enable versioning for the default tests, since the default / common use case does not require nor use versioning by default.

If no separate storage account is needed, then it is a matter of simply creating a separate container with the appropriate settings.

PTAL at https://github.com/kopia/kopia/compare/azure-immutability-setup

This contains the setup code for the storage account and the container. PLMK if that makes sense, if there's anything missing or that needs to be changed.

Thanks.

Setting ImmutableStorageWithVersioning to true via CLI is automatically making versioning be enabled too? In the UI they make you do 2 separate actions. A policy isn’t needed on the container~ only versioning + ImmutableStorageWithVersioning
If the intention would be to run it repeatedly like the createContainer currently then it needs checks for "already exists" errors also on the SA & container

Behavior observed using the console: attempting to enable "version immutability" in the container requires enabling "versioning" in the storage account. It wasn't clear at that moment what the implication was for existing buckets, so we created a separate storage account and bucket.

See comment below

julio-lopez · 2023-11-08T05:36:35Z

@redgoat650 Do you have any comments?

repo/blob/azure/azure_versioned_test.go

julio-lopez · 2023-11-28T22:50:54Z

@KastenMike FYI #3471 was missing in the setup. All good now.

julio-lopez

@KastenMike 🥇Thanks for doing this.

LGTM ✅

KastenMike added 8 commits October 21, 2023 18:51

local only (#4)

695e37a

other changes (#5)

629a20b

on create (#6)

de34324

Revert "on create (#6)" (#7)

7e63dad

This reverts commit de34324.

Revert "other changes (#5)" (#8)

1d722ca

This reverts commit 629a20b.

Revert "local only (#4)" (#9)

af920a4

This reverts commit 695e37a.

Merge branch 'kopia:master' into master

0d0480d

feat(repository): Add Azure PIT support

b660a29

KastenMike changed the title ~~Azure pit support~~ feat(repository): Add Azure PIT support Oct 26, 2023

KastenMike added 4 commits October 26, 2023 15:42

previous checks

817d219

working version with kopia DM now removed

496ad26

only compare versions, remove KopiaDeleteMarker checks

959b94b

lint

fabf3fe

KastenMike mentioned this pull request Oct 26, 2023

feat(repository): Add support for Azure DeleteBlob operations where immutability protection is on #3394

Merged

KastenMike added 2 commits October 26, 2023 17:11

correct comment, need caffeine

c63e463

clarifying comment

9a98f0a

julio-lopez requested review from julio-lopez and redgoat650 November 1, 2023 05:09

julio-lopez reviewed Nov 1, 2023

View reviewed changes

switch from IsImmutableStorageWithVersioning to a versioning check

1eff871

julio-lopez reviewed Nov 4, 2023

View reviewed changes

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

KastenMike added 3 commits November 4, 2023 16:08

create format.KopiaRepositoryBlobID in tests + other tweaks

193795c

remove defer deletes

1999d86

comment + lint

ec44723

julio-lopez reviewed Nov 8, 2023

View reviewed changes

createContainer + t.Cleanup

8ffd4f8

julio-lopez reviewed Nov 28, 2023

View reviewed changes

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

KastenMike added 2 commits November 28, 2023 11:59

env var update

9512263

KOPIA_AZURE_TEST_IMMUTABLE_CONTAINER only for PIT tests

7a2fc25

julio-lopez approved these changes Nov 28, 2023

View reviewed changes

julio-lopez merged commit d4a380f into kopia:master Nov 28, 2023

julio-lopez requested a review from Shrekster November 28, 2023 22:57

julio-lopez mentioned this pull request Nov 30, 2023

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

Merged

KastenMike deleted the Azure-PIT-support branch September 26, 2024 15:01

	return n[len(az.Prefix):]
	return strings.TrimPrefix(toString(*it.Name), az.prefix)

	if t, ok := timestampmeta.FromValue(stringDefault(it.Metadata["kopiamtime"], "")); ok {
	if t, ok := timestampmeta.FromValue(toString(it.Metadata["kopiamtime"])); ok {

feat(repository): Add Azure PIT support #3407

feat(repository): Add Azure PIT support #3407

Uh oh!

Conversation

KastenMike commented Oct 26, 2023

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

julio-lopez commented Nov 1, 2023

Uh oh!

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!

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!

julio-lopez left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julio-lopez commented Nov 4, 2023

Uh oh!

julio-lopez Nov 8, 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!

KastenMike Nov 8, 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!

KastenMike Nov 8, 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!

KastenMike Nov 16, 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!

julio-lopez Nov 17, 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!

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

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Nov 1, 2023 •

edited

Loading

julio-lopez left a comment •

edited

Loading

julio-lopez Nov 8, 2023 •

edited

Loading

KastenMike Nov 8, 2023 •

edited

Loading

KastenMike Nov 8, 2023 •

edited

Loading

KastenMike Nov 16, 2023 •

edited

Loading

julio-lopez Nov 17, 2023 •

edited

Loading

julio-lopez Nov 28, 2023 •

edited

Loading

julio-lopez commented Nov 28, 2023 •

edited

Loading