Skip to content

Commit

Permalink
Fix a minioWatcher.Download() bug
Browse files Browse the repository at this point in the history
- Add `MinioConfig.URL` field to set bucket config by URL
- Use the URL field to specify a local fileblob bucket for testing
- Add a unit test for the `minioWatcher.Download()` method
- Fix a bug discovered by the test
  • Loading branch information
djjuhasz committed Mar 1, 2024
1 parent de41b2f commit e934211
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 12 deletions.
1 change: 1 addition & 0 deletions internal/watcher/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ type MinioConfig struct {
Secret string
Token string
Bucket string
URL string

RetentionPeriod *time.Duration
StripTopLevelDir bool
Expand Down
6 changes: 5 additions & 1 deletion internal/watcher/minio.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ func NewMinioWatcher(ctx context.Context, logger logr.Logger, config *MinioConfi
Profile: config.Profile,
Region: config.Region,
PathStyle: config.PathStyle,
URL: config.URL,
}

if config.RedisFailedList == "" {
Expand Down Expand Up @@ -163,7 +164,7 @@ func (w *minioWatcher) Download(ctx context.Context, dest, key string) error {
}
defer reader.Close()

writer, err := os.Open(dest) // #nosec G304 -- trusted file path.
writer, err := os.Create(dest) // #nosec G304 -- trusted file path.
if err != nil {
return fmt.Errorf("error creating writer: %w", err)

Check warning on line 169 in internal/watcher/minio.go

View check run for this annotation

Codecov / codecov/patch

internal/watcher/minio.go#L169

Added line #L169 was not covered by tests
}
Expand All @@ -173,5 +174,8 @@ func (w *minioWatcher) Download(ctx context.Context, dest, key string) error {
return fmt.Errorf("error copying blob: %w", err)

Check warning on line 174 in internal/watcher/minio.go

View check run for this annotation

Codecov / codecov/patch

internal/watcher/minio.go#L174

Added line #L174 was not covered by tests
}

// Try to set the file mode but ignore any errors.
_ = os.Chmod(dest, 0o600)

return nil
}
53 changes: 42 additions & 11 deletions internal/watcher/minio_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@ import (

"github.com/alicebob/miniredis/v2"
"github.com/go-logr/logr"
"gotest.tools/v3/assert"
"gotest.tools/v3/fs"
"gotest.tools/v3/poll"

"github.com/artefactual-sdps/enduro/internal/watcher"
)

func newWatcher(t *testing.T) (*miniredis.Miniredis, watcher.Watcher) {
func newWatcher(t *testing.T, updateCfg func(c *watcher.MinioConfig)) (*miniredis.Miniredis, watcher.Watcher) {
t.Helper()

m, err := miniredis.Run()
Expand All @@ -24,7 +26,9 @@ func newWatcher(t *testing.T) (*miniredis.Miniredis, watcher.Watcher) {
}

dur := time.Duration(time.Second)
config := watcher.MinioConfig{

// Default config.
config := &watcher.MinioConfig{
Name: "minio-watcher",
RedisAddress: fmt.Sprintf("redis://%s", m.Addr()),
RedisList: "minio-events",
Expand All @@ -40,9 +44,12 @@ func newWatcher(t *testing.T) (*miniredis.Miniredis, watcher.Watcher) {
StripTopLevelDir: true,
}

var w watcher.Watcher
logger := logr.Discard()
w, err = watcher.NewMinioWatcher(context.Background(), logger, &config)
// Modify default config.
if updateCfg != nil {
updateCfg(config)
}

w, err := watcher.NewMinioWatcher(context.Background(), logr.Discard(), config)
if err != nil {
t.Fatal(err)
}
Expand All @@ -57,7 +64,7 @@ func cleanup(t *testing.T, m *miniredis.Miniredis) {
}

func TestWatcherReturnsErrWhenNoMessages(t *testing.T) {
m, w := newWatcher(t)
m, w := newWatcher(t, nil)
defer cleanup(t, m)

// TODO: slow test, should inject smaller timeout.
Expand All @@ -80,7 +87,7 @@ func TestWatcherReturnsErrWhenNoMessages(t *testing.T) {
}

func TestWatcherReturnsErrOnInvalidMessages(t *testing.T) {
m, w := newWatcher(t)
m, w := newWatcher(t, nil)
defer cleanup(t, m)

m.Lpush("minio-events", "{}")
Expand All @@ -103,7 +110,7 @@ func TestWatcherReturnsErrOnInvalidMessages(t *testing.T) {
}

func TestWatcherReturnsErrOnMessageInWrongBucket(t *testing.T) {
m, w := newWatcher(t)
m, w := newWatcher(t, nil)
defer cleanup(t, m)

// Message with a bucket we're not watching.
Expand Down Expand Up @@ -180,7 +187,7 @@ func TestWatcherReturnsErrOnMessageInWrongBucket(t *testing.T) {
}

func TestWatcherReturnsOnValidMessage(t *testing.T) {
m, w := newWatcher(t)
m, w := newWatcher(t, nil)
defer cleanup(t, m)

m.Lpush("minio-events", `[
Expand Down Expand Up @@ -254,7 +261,7 @@ func TestWatcherReturnsOnValidMessage(t *testing.T) {
}

func TestWatcherReturnsDecodedObjectKey(t *testing.T) {
m, w := newWatcher(t)
m, w := newWatcher(t, nil)
defer cleanup(t, m)

// Message with an encoded object key
Expand Down Expand Up @@ -292,7 +299,7 @@ func TestWatcherReturnsDecodedObjectKey(t *testing.T) {
}

func TestWatcherReturnsErrOnInvalidObjectKey(t *testing.T) {
m, w := newWatcher(t)
m, w := newWatcher(t, nil)
defer cleanup(t, m)

// Message with an invalid encoded object key
Expand Down Expand Up @@ -328,3 +335,27 @@ func TestWatcherReturnsErrOnInvalidObjectKey(t *testing.T) {

poll.WaitOn(t, check, poll.WithTimeout(time.Second*3))
}

func TestMinioWatcherDownload(t *testing.T) {
t.Run("Downloads a file", func(t *testing.T) {
t.Parallel()

wd := fs.NewDir(t, "enduro-test-minio-watcher",
fs.WithFile("test", "A test file."),
)
m, w := newWatcher(t, func(c *watcher.MinioConfig) {
c.URL = fmt.Sprintf("file://%s", wd.Path())
})
defer cleanup(t, m)

dest := fs.NewDir(t, "enduro-test-minio-watcher")
err := w.Download(context.Background(), dest.Join("test"), "test")
assert.NilError(t, err)
assert.Assert(t, fs.Equal(
dest.Path(),
fs.Expected(t,
fs.WithFile("test", "A test file.", fs.WithMode(0o600)),
),
))
})
}

0 comments on commit e934211

Please sign in to comment.