From ebd9462ea6037003f218c0a5efb17e079cff4c5f Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Fri, 15 Jan 2021 12:18:28 +0000 Subject: [PATCH] union: fix union attempting to update files on a read only file system Before this change, when using an all create method with one of the upstreams being read only, if there was an existing file on the read only remote, it was impossible to update it. This change detects that situation and creates the file on a read/write upstream. This file will shadow the file on the read/only upstream. If it is deleted the read only upstream file will be visible again. Fixes #4929 --- backend/union/entry.go | 12 ++++- backend/union/union_internal_test.go | 67 +++++++++++++++++++++++ backend/union/union_test.go | 79 +++++++++++++--------------- 3 files changed, 114 insertions(+), 44 deletions(-) create mode 100644 backend/union/union_internal_test.go diff --git a/backend/union/entry.go b/backend/union/entry.go index 93184399c..31378e710 100644 --- a/backend/union/entry.go +++ b/backend/union/entry.go @@ -59,7 +59,17 @@ func (d *Directory) candidates() []upstream.Entry { // return an error or update the object properly (rather than e.g. calling panic). func (o *Object) Update(ctx context.Context, in io.Reader, src fs.ObjectInfo, options ...fs.OpenOption) error { entries, err := o.fs.actionEntries(o.candidates()...) - if err != nil { + if err == fs.ErrorPermissionDenied { + // There are no candidates in this object which can be written to + // So attempt to create a new object instead + newO, err := o.fs.put(ctx, in, src, false, options...) + if err != nil { + return err + } + // Update current object + *o = *newO.(*Object) + return nil + } else if err != nil { return err } if len(entries) == 1 { diff --git a/backend/union/union_internal_test.go b/backend/union/union_internal_test.go new file mode 100644 index 000000000..db5367561 --- /dev/null +++ b/backend/union/union_internal_test.go @@ -0,0 +1,67 @@ +package union + +import ( + "bytes" + "context" + "testing" + "time" + + "github.com/rclone/rclone/fs/object" + "github.com/rclone/rclone/fstest" + "github.com/rclone/rclone/fstest/fstests" + "github.com/rclone/rclone/lib/random" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func (f *Fs) TestInternalReadOnly(t *testing.T) { + if f.name != "TestUnionRO" { + t.Skip("Only on RO union") + } + dir := "TestInternalReadOnly" + ctx := context.Background() + rofs := f.upstreams[len(f.upstreams)-1] + assert.False(t, rofs.IsWritable()) + + // Put a file onto the read only fs + contents := random.String(50) + file1 := fstest.NewItem(dir+"/file.txt", contents, time.Now()) + _, obj1 := fstests.PutTestContents(ctx, t, rofs, &file1, contents, true) + + // Check read from readonly fs via union + o, err := f.NewObject(ctx, file1.Path) + require.NoError(t, err) + assert.Equal(t, int64(50), o.Size()) + + // Now call Update on the union Object with new data + contents2 := random.String(100) + file2 := fstest.NewItem(dir+"/file.txt", contents2, time.Now()) + in := bytes.NewBufferString(contents2) + src := object.NewStaticObjectInfo(file2.Path, file2.ModTime, file2.Size, true, nil, nil) + err = o.Update(ctx, in, src) + require.NoError(t, err) + assert.Equal(t, int64(100), o.Size()) + + // Check we read the new object via the union + o, err = f.NewObject(ctx, file1.Path) + require.NoError(t, err) + assert.Equal(t, int64(100), o.Size()) + + // Remove the object + assert.NoError(t, o.Remove(ctx)) + + // Check we read the old object in the read only layer now + o, err = f.NewObject(ctx, file1.Path) + require.NoError(t, err) + assert.Equal(t, int64(50), o.Size()) + + // Remove file and dir from read only fs + assert.NoError(t, obj1.Remove(ctx)) + assert.NoError(t, rofs.Rmdir(ctx, dir)) +} + +func (f *Fs) InternalTest(t *testing.T) { + t.Run("ReadOnly", f.TestInternalReadOnly) +} + +var _ fstests.InternalTester = (*Fs)(nil) diff --git a/backend/union/union_test.go b/backend/union/union_test.go index 676088220..07b3bb230 100644 --- a/backend/union/union_test.go +++ b/backend/union/union_test.go @@ -2,13 +2,15 @@ package union_test import ( + "fmt" + "io/ioutil" "os" - "path/filepath" "testing" _ "github.com/rclone/rclone/backend/local" "github.com/rclone/rclone/fstest" "github.com/rclone/rclone/fstest/fstests" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -24,17 +26,28 @@ func TestIntegration(t *testing.T) { }) } +func makeTestDirs(t *testing.T, n int) (dirs []string, clean func()) { + for i := 1; i <= n; i++ { + dir, err := ioutil.TempDir("", fmt.Sprintf("rclone-union-test-%d", n)) + require.NoError(t, err) + dirs = append(dirs, dir) + } + clean = func() { + for _, dir := range dirs { + err := os.RemoveAll(dir) + assert.NoError(t, err) + } + } + return dirs, clean +} + func TestStandard(t *testing.T) { if *fstest.RemoteName != "" { t.Skip("Skipping as -remote set") } - tempdir1 := filepath.Join(os.TempDir(), "rclone-union-test-standard1") - tempdir2 := filepath.Join(os.TempDir(), "rclone-union-test-standard2") - tempdir3 := filepath.Join(os.TempDir(), "rclone-union-test-standard3") - require.NoError(t, os.MkdirAll(tempdir1, 0744)) - require.NoError(t, os.MkdirAll(tempdir2, 0744)) - require.NoError(t, os.MkdirAll(tempdir3, 0744)) - upstreams := tempdir1 + " " + tempdir2 + " " + tempdir3 + dirs, clean := makeTestDirs(t, 3) + defer clean() + upstreams := dirs[0] + " " + dirs[1] + " " + dirs[2] name := "TestUnion" fstests.Run(t, &fstests.Opt{ RemoteName: name + ":", @@ -54,13 +67,9 @@ func TestRO(t *testing.T) { if *fstest.RemoteName != "" { t.Skip("Skipping as -remote set") } - tempdir1 := filepath.Join(os.TempDir(), "rclone-union-test-ro1") - tempdir2 := filepath.Join(os.TempDir(), "rclone-union-test-ro2") - tempdir3 := filepath.Join(os.TempDir(), "rclone-union-test-ro3") - require.NoError(t, os.MkdirAll(tempdir1, 0744)) - require.NoError(t, os.MkdirAll(tempdir2, 0744)) - require.NoError(t, os.MkdirAll(tempdir3, 0744)) - upstreams := tempdir1 + " " + tempdir2 + ":ro " + tempdir3 + ":ro" + dirs, clean := makeTestDirs(t, 3) + defer clean() + upstreams := dirs[0] + " " + dirs[1] + ":ro " + dirs[2] + ":ro" name := "TestUnionRO" fstests.Run(t, &fstests.Opt{ RemoteName: name + ":", @@ -80,13 +89,9 @@ func TestNC(t *testing.T) { if *fstest.RemoteName != "" { t.Skip("Skipping as -remote set") } - tempdir1 := filepath.Join(os.TempDir(), "rclone-union-test-nc1") - tempdir2 := filepath.Join(os.TempDir(), "rclone-union-test-nc2") - tempdir3 := filepath.Join(os.TempDir(), "rclone-union-test-nc3") - require.NoError(t, os.MkdirAll(tempdir1, 0744)) - require.NoError(t, os.MkdirAll(tempdir2, 0744)) - require.NoError(t, os.MkdirAll(tempdir3, 0744)) - upstreams := tempdir1 + " " + tempdir2 + ":nc " + tempdir3 + ":nc" + dirs, clean := makeTestDirs(t, 3) + defer clean() + upstreams := dirs[0] + " " + dirs[1] + ":nc " + dirs[2] + ":nc" name := "TestUnionNC" fstests.Run(t, &fstests.Opt{ RemoteName: name + ":", @@ -106,13 +111,9 @@ func TestPolicy1(t *testing.T) { if *fstest.RemoteName != "" { t.Skip("Skipping as -remote set") } - tempdir1 := filepath.Join(os.TempDir(), "rclone-union-test-policy11") - tempdir2 := filepath.Join(os.TempDir(), "rclone-union-test-policy12") - tempdir3 := filepath.Join(os.TempDir(), "rclone-union-test-policy13") - require.NoError(t, os.MkdirAll(tempdir1, 0744)) - require.NoError(t, os.MkdirAll(tempdir2, 0744)) - require.NoError(t, os.MkdirAll(tempdir3, 0744)) - upstreams := tempdir1 + " " + tempdir2 + " " + tempdir3 + dirs, clean := makeTestDirs(t, 3) + defer clean() + upstreams := dirs[0] + " " + dirs[1] + " " + dirs[2] name := "TestUnionPolicy1" fstests.Run(t, &fstests.Opt{ RemoteName: name + ":", @@ -132,13 +133,9 @@ func TestPolicy2(t *testing.T) { if *fstest.RemoteName != "" { t.Skip("Skipping as -remote set") } - tempdir1 := filepath.Join(os.TempDir(), "rclone-union-test-policy21") - tempdir2 := filepath.Join(os.TempDir(), "rclone-union-test-policy22") - tempdir3 := filepath.Join(os.TempDir(), "rclone-union-test-policy23") - require.NoError(t, os.MkdirAll(tempdir1, 0744)) - require.NoError(t, os.MkdirAll(tempdir2, 0744)) - require.NoError(t, os.MkdirAll(tempdir3, 0744)) - upstreams := tempdir1 + " " + tempdir2 + " " + tempdir3 + dirs, clean := makeTestDirs(t, 3) + defer clean() + upstreams := dirs[0] + " " + dirs[1] + " " + dirs[2] name := "TestUnionPolicy2" fstests.Run(t, &fstests.Opt{ RemoteName: name + ":", @@ -158,13 +155,9 @@ func TestPolicy3(t *testing.T) { if *fstest.RemoteName != "" { t.Skip("Skipping as -remote set") } - tempdir1 := filepath.Join(os.TempDir(), "rclone-union-test-policy31") - tempdir2 := filepath.Join(os.TempDir(), "rclone-union-test-policy32") - tempdir3 := filepath.Join(os.TempDir(), "rclone-union-test-policy33") - require.NoError(t, os.MkdirAll(tempdir1, 0744)) - require.NoError(t, os.MkdirAll(tempdir2, 0744)) - require.NoError(t, os.MkdirAll(tempdir3, 0744)) - upstreams := tempdir1 + " " + tempdir2 + " " + tempdir3 + dirs, clean := makeTestDirs(t, 3) + defer clean() + upstreams := dirs[0] + " " + dirs[1] + " " + dirs[2] name := "TestUnionPolicy3" fstests.Run(t, &fstests.Opt{ RemoteName: name + ":",