Repository: kopia/kopia
Pull request #3192 (2023-08-03 16:39:00)

fix(repository): Storage config blob caching
Fix minor issue with caching the storage blob config blob settings in-memory which could cause other blob PUTs after a reconfiguration to have incorrect settings

Most of PR is tests similar to the retention extension tests to ensure changing repo settings works as expected

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kopia/kopia/pull/3192?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) Report
Patch coverage: **`100.00%`** and project coverage change: **`-0.02%`** :warning:
> Comparison is base [(`a620349`)](https://app.codecov.io/gh/kopia/kopia/commit/a620349db8131c59b2a3b43fba6e0c2503fc3c9a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) 75.41% compared to head [(`2420f0e`)](https://app.codecov.io/gh/kopia/kopia/pull/3192?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) 75.40%.
> Report is 1 commits behind head on master.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #3192      +/-   ##
==========================================
- Coverage   75.41%   75.40%   -0.02%     
==========================================
  Files         459      459              
  Lines       36445    36449       +4     
==========================================
- Hits        27484    27483       -1     
- Misses       7043     7048       +5     
  Partials     1918     1918              
```


| [Files Changed](https://app.codecov.io/gh/kopia/kopia/pull/3192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) | Coverage Δ | |
|---|---|---|
| [repo/format/format\_set\_parameters.go](https://app.codecov.io/gh/kopia/kopia/pull/3192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-cmVwby9mb3JtYXQvZm9ybWF0X3NldF9wYXJhbWV0ZXJzLmdv) | `66.66% <100.00%> (+2.66%)` | :arrow_up: |

... and [6 files with indirect coverage changes](https://app.codecov.io/gh/kopia/kopia/pull/3192/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/kopia/kopia/pull/3192?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).


Patch:
diff --git a/repo/format/format_set_parameters.go b/repo/format/format_set_parameters.go
index e417f5c7..62f33989 100644
--- a/repo/format/format_set_parameters.go
+++ b/repo/format/format_set_parameters.go
@@ -38,6 +38,11 @@ func (m *Manager) SetParameters(
 		return errors.Wrap(err, "unable to write blobcfg blob")
 	}
 
+	// At this point the new blobcfg is persisted in the blob layer. Setting this
+	// here also ensures the call below properly sets retention on the kopia
+	// repository blob.
+	m.blobCfgBlob = blobcfg
+
 	if err := m.j.WriteKopiaRepositoryBlob(ctx, m.blobs, m.blobCfgBlob); err != nil {
 		return errors.Wrap(err, "unable to write format blob")
 	}


Test patch:
diff --git a/repo/format/format_manager_test.go b/repo/format/format_manager_test.go
index 78e6176d..9753ad62 100644
--- a/repo/format/format_manager_test.go
+++ b/repo/format/format_manager_test.go
@@ -6,9 +6,11 @@
 	"time"
 
 	"github.com/pkg/errors"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/kopia/kopia/internal/blobtesting"
+	"github.com/kopia/kopia/internal/cache"
 	"github.com/kopia/kopia/internal/epoch"
 	"github.com/kopia/kopia/internal/faketime"
 	"github.com/kopia/kopia/internal/feature"
@@ -169,6 +171,179 @@ func TestInitialize(t *testing.T) {
 		format.ErrAlreadyInitialized)
 }
 
+func TestInitializeWithRetention(t *testing.T) {
+	ctx := testlogging.Context(t)
+
+	ta := faketime.NewClockTimeWithOffset(0)
+	nowFunc := ta.NowFunc()
+
+	st := blobtesting.NewVersionedMapStorage(nowFunc).(cache.Storage)
+	blobCache := format.NewMemoryBlobCache(nowFunc)
+	mode := blob.Governance
+	period := time.Hour * 48
+
+	// success
+	require.NoError(t, format.Initialize(
+		ctx,
+		st,
+		&format.KopiaRepositoryJSON{},
+		rc,
+		format.BlobStorageConfiguration{
+			RetentionMode:   mode,
+			RetentionPeriod: period,
+		},
+		"some-password",
+	))
+
+	mgr, err := format.NewManagerWithCache(ctx, st, cacheDuration, "some-password", nowFunc, blobCache)
+	require.NoError(t, err, "getting format manager")
+
+	// New retention parameters should be available from the format manager.
+	blobCfg := mustGetBlobStorageConfiguration(t, mgr)
+	assert.Equal(t, mode, blobCfg.RetentionMode)
+	assert.Equal(t, period, blobCfg.RetentionPeriod)
+
+	// Attempting to touch the blobs the format manager writes should return
+	// errors as they should have retention enabled. Mod time adjustment (duration
+	// param) doesn't matter in this context.
+	_, err = st.TouchBlob(ctx, format.KopiaRepositoryBlobID, time.Minute)
+	assert.ErrorIs(t, err, blobtesting.ErrBlobLocked, "Altering locked repo blob should fail")
+
+	_, err = st.TouchBlob(ctx, format.KopiaBlobCfgBlobID, time.Minute)
+	assert.ErrorIs(t, err, blobtesting.ErrBlobLocked, "Altering locked blob storage config should fail")
+
+	// Advance the clock to just before the retention period expires and check
+	// if we can modifiy the blob. Blobs should still be protected so TouchBlob
+	// should still fail.
+	ta.Advance((time.Hour * 48) - time.Minute)
+	_, err = st.TouchBlob(ctx, format.KopiaRepositoryBlobID, time.Minute)
+	assert.ErrorIs(t, err, blobtesting.ErrBlobLocked, "Altering locked repo blob should fail")
+
+	_, err = st.TouchBlob(ctx, format.KopiaBlobCfgBlobID, time.Minute)
+	assert.ErrorIs(t, err, blobtesting.ErrBlobLocked, "Altering locked blob storage config should fail")
+
+	// Advance the clock so the retention period has passed. Calling TouchBlob
+	// should now succeed.
+	ta.Advance(time.Minute)
+
+	_, err = st.TouchBlob(ctx, format.KopiaRepositoryBlobID, time.Minute)
+	assert.NoError(t, err, "Altering expired repo blob failed")
+
+	_, err = st.TouchBlob(ctx, format.KopiaBlobCfgBlobID, time.Minute)
+	assert.NoError(t, err, "Altering expired blob storage config failed")
+}
+
+func TestUpdateRetention(t *testing.T) {
+	ctx := testlogging.Context(t)
+
+	ta := faketime.NewClockTimeWithOffset(0)
+	nowFunc := ta.NowFunc()
+
+	st := blobtesting.NewVersionedMapStorage(nowFunc).(cache.Storage)
+	blobCache := format.NewMemoryBlobCache(nowFunc)
+	mode := blob.Governance
+	period := time.Hour * 48
+
+	// success
+	require.NoError(t, format.Initialize(ctx, st, &format.KopiaRepositoryJSON{}, rc, format.BlobStorageConfiguration{}, "some-password"))
+
+	mgr, err := format.NewManagerWithCache(ctx, st, cacheDuration, "some-password", nowFunc, blobCache)
+	require.NoError(t, err, "getting format manager")
+
+	mp := mustGetMutableParameters(t, mgr)
+	rf := mustGetRequiredFeatures(t, mgr)
+
+	err = mgr.SetParameters(
+		ctx,
+		mp,
+		format.BlobStorageConfiguration{
+			RetentionMode:   mode,
+			RetentionPeriod: period,
+		},
+		rf,
+	)
+	require.NoError(t, err, "setting repo parameters")
+
+	// New retention parameters should be available from the format manager.
+	blobCfg := mustGetBlobStorageConfiguration(t, mgr)
+	assert.Equal(t, mode, blobCfg.RetentionMode)
+	assert.Equal(t, period, blobCfg.RetentionPeriod)
+
+	// Attempting to touch the blobs the format manager writes should return
+	// errors as they should have retention enabled. Mod time adjustment (duration
+	// param) doesn't matter in this context.
+	_, err = st.TouchBlob(ctx, format.KopiaRepositoryBlobID, time.Minute)
+	assert.ErrorIs(t, err, blobtesting.ErrBlobLocked, "Altering locked repo blob should fail")
+
+	_, err = st.TouchBlob(ctx, format.KopiaBlobCfgBlobID, time.Minute)
+	assert.ErrorIs(t, err, blobtesting.ErrBlobLocked, "Altering locked blob storage config should fail")
+
+	// Advance the clock to just before the retention period expires and check
+	// if we can modifiy the blob. Blobs should still be protected so TouchBlob
+	// should still fail.
+	ta.Advance((time.Hour * 48) - time.Minute)
+	_, err = st.TouchBlob(ctx, format.KopiaRepositoryBlobID, time.Minute)
+	assert.ErrorIs(t, err, blobtesting.ErrBlobLocked, "Altering locked repo blob should fail")
+
+	_, err = st.TouchBlob(ctx, format.KopiaBlobCfgBlobID, time.Minute)
+	assert.ErrorIs(t, err, blobtesting.ErrBlobLocked, "Altering locked blob storage config should fail")
+
+	// Advance the clock so the retention period has passed. Calling TouchBlob
+	// should now succeed.
+	ta.Advance(time.Minute)
+
+	_, err = st.TouchBlob(ctx, format.KopiaRepositoryBlobID, time.Minute)
+	assert.NoError(t, err, "Altering expired repo blob failed")
+
+	_, err = st.TouchBlob(ctx, format.KopiaBlobCfgBlobID, time.Minute)
+	assert.NoError(t, err, "Altering expired blob storage config failed")
+}
+
+func TestUpdateRetentionNegativeValue(t *testing.T) {
+	ctx := testlogging.Context(t)
+
+	startTime := time.Date(2020, 1, 1, 12, 0, 0, 0, time.UTC)
+	ta := faketime.NewTimeAdvance(startTime, 0)
+	nowFunc := ta.NowFunc()
+
+	st := blobtesting.NewVersionedMapStorage(nowFunc).(cache.Storage)
+	blobCache := format.NewMemoryBlobCache(nowFunc)
+	mode := blob.Governance
+	period := -time.Hour * 48
+
+	// success
+	require.NoError(t, format.Initialize(ctx, st, &format.KopiaRepositoryJSON{}, rc, format.BlobStorageConfiguration{}, "some-password"))
+
+	mgr, err := format.NewManagerWithCache(ctx, st, cacheDuration, "some-password", nowFunc, blobCache)
+	require.NoError(t, err, "getting format manager")
+
+	mp := mustGetMutableParameters(t, mgr)
+	rf := mustGetRequiredFeatures(t, mgr)
+
+	err = mgr.SetParameters(
+		ctx,
+		mp,
+		format.BlobStorageConfiguration{
+			RetentionMode:   mode,
+			RetentionPeriod: period,
+		},
+		rf,
+	)
+	require.Error(t, err, "setting repo parameters")
+
+	// Old retention parameters should be available from the format manager.
+	blobCfg := mustGetBlobStorageConfiguration(t, mgr)
+	assert.Empty(t, blobCfg.RetentionMode)
+	assert.Zero(t, blobCfg.RetentionPeriod)
+
+	// Retention wasn't set so no error should occur.
+	_, err = st.TouchBlob(ctx, format.KopiaRepositoryBlobID, time.Minute)
+	assert.NoError(t, err, "altering repo blob")
+
+	_, err = st.TouchBlob(ctx, format.KopiaBlobCfgBlobID, time.Minute)
+	assert.NoError(t, err, "altering storage config")
+}
+
 func TestChangePassword(t *testing.T) {
 	ctx := testlogging.Context(t)