S3 FS implementation #90

rgarcia · 2016-05-22T01:01:25Z

This is a mostly-working/mostly-tested implementation of S3 as an FS.

There are some gaps in the implementation, but it is otherwise pretty functional:

It doesn't support non-offset == 0 reads or writes
It doesn't support Truncate
It doesn't support Seek with whence set to 2
It doesn't support opening a file in append mode

All of these things increase the complexity of the implementation significantly (some are questionably well-defined, e.g. append in an eventually-consistent FS like S3), so for now I've left them out.

In order to reduce clutter in the root package github.com/spf13/afero, I put the whole implementation + tests in a sub package, github.com/spf13/afero/s3. In order to leverage the generic Fs tests in afero_test.go, I copy-pasted them into a sub package github.com/spf13/afero/tests. I also added a few tests to this file as I went along.

In order to run the tests you must pass a bucket: go test -v github.com/spf13/afero/s3 -s3bucket <test bucket>. You also need to set AWS_REGION, AWS_ACCESS_KEY_ID, and AWS_SECRET_ACCESS_KEY.

Let me know what you think 😄

CLAassistant · 2016-05-22T01:01:26Z

All committers have signed the CLA.

dim · 2016-06-28T14:15:13Z

👍

ns-cweber · 2017-02-18T14:00:27Z

Could this get some love? I am very interested in using this in my project. Depending on a fork is never ideal.

ns-cweber · 2017-02-18T14:48:31Z

s3/s3_fs.go

+		if os.IsNotExist(err) {
+			return fs.OpenFile(name, os.O_CREATE, 0777)
+		}
+		return (*S3File)(nil), err


Any reason you're returning a valid interface to a nil pointer? While everyone should be checking the error, CacheOnReadFs checks the interface for nil, and this breaks in hard-to-troubleshoot ways. While I don't think anyone should be ignoring the error, returning a valid interface isn't doing anyone any favors either.

ns-cweber · 2017-02-18T15:18:35Z

s3/s3_fs.go

+		if strings.Contains(err.Error(), "404") {
+			statDir, err := fs.statDirectory(name)
+			return statDir, err
+		}


Rather than checking the string, I think it would be better to check the error code:

if err, ok := err.(awserr.Error); ok { if err.Code() == "NotFound" { return fs.statDirectory(name) } }

Here's a runnable example to play around with (just manipulate the bucket name):

package main import ( "log" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/s3" ) func main() { sess, err := session.NewSession() if err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(-1) } if _, err := s3.New(sess).HeadObject(&s3.HeadObjectInput{ Bucket: aws.String("bucketName"), Key: aws.String("/missing/key/"), }); err != nil { if err, ok := err.(awserr.Error); ok { fmt.Println("CODE:", err.Code()) } } }

nicored · 2017-04-10T12:45:01Z

s3/s3_test.go

+
+var s3bucket string
+
+func init() {


This should be done as an integration test. Instead, you could Monkey Patch s3 methods (mock cannot really be done).

// my_test.go ... monkey.PatchInstanceMethod(reflect.TypeOf(s3Session), "GetObject", s3_mocks.GetObjectMockFn()) ...

// mocks_s3.go type ClosingBuffer struct { *bytes.Buffer } func (cb *ClosingBuffer) Close() error { return nil } func GetObjectMockFn() func(_ *s3.S3, _ *s3.GetObjectInput) (*s3.GetObjectOutput, error) { return func(_ *s3.S3, _ *s3.GetObjectInput) (*s3.GetObjectOutput, error) { cb := &ClosingBuffer{bytes.NewBufferString(`This is the content of a file`)} var rc io.ReadCloser rc = cb rc.Close() return &s3.GetObjectOutput{ Body: rc, }, nil } }

Maybe Minio as a semi-functional mock?

Which would mean that your unit tests would depend on Minio, which I'm against.
Your unit tests should not depend on external services to work, but Minio would be a good candidate for integration testing, but then I'd rather connect my tests to a test bucket in s3.
I've had a chat with a few people who run their unit tests as integration tests to avoid to have to re-write parts of their tests, which sounds wrong to me.
Here, we're using the aws sdk, which we assume is stable and has been tested, so what you want to is just test your app logic and mock what functions will return, and allow you to also test how you handle errors you'd not get when aws behaves fine. It gives you more flexibility in your unit tests to catch all edge cases you can think of. I do agree that writing mocks can often add complexity in your tests, but it's worth it.

Yeah, I can see where you're coming from. Using S3 itself as an integration test sounds potentially slow (and $) too, but I guess that depends on a lot of factors.

With mock vs integration... neither way seems to be perfect, both being needed. 😉

Arguably, storage is really really cheap these days, 0.0245 per gb for standard storage.

nicored

Some concerns about unit tests

jcftang · 2017-10-05T10:44:35Z

I know this has been sitting around for a while, but it would be nice to see if one of the s3 backends could be merged in

aviau · 2018-04-27T20:42:06Z

Any news on this? I would also love to see this merged.

@rgarcia Would you rebase your PR?

aviau · 2018-07-19T21:31:51Z

Someone should probably move this PR to absfs/afero

0xmichalis · 2020-03-27T19:26:49Z

path.go

@@ -24,7 +24,7 @@ import (
 // readDirNames reads the directory named by dirname and returns


Can you also update the function name in the comment?

0xmichalis · 2020-03-27T19:32:24Z

s3/s3_file.go

+		panic("read after close")
+	}
+	if f.offset != 0 {
+		panic("TODO: non-offset == 0 read")


Maybe open a follow/up issue to track this.

On a second thought, this should probably be fixed from the get-go.

0xmichalis · 2020-03-27T19:35:46Z

s3/s3_file.go

+	case 1:
+		f.offset += int(offset)
+	case 2:
+		// can probably do this if we had GetObjectOutput (ContentLength)


Seems to be a thing now

https://godoc.org/github.com/aws/aws-sdk-go/service/s3#GetObjectOutput

0xmichalis · 2020-03-27T19:37:33Z

s3/s3_file.go

+		panic("write after close")
+	}
+	if f.offset != 0 {
+		panic("TODO: non-offset == 0 write")


This seems like it should be fixed, otherwise Write in existing files is not useful?

0xmichalis · 2020-03-27T19:43:35Z

s3/s3_file.go

+		Bucket:               aws.String(f.bucket),
+		Key:                  aws.String(f.name),
+		Body:                 readSeeker,
+		ServerSideEncryption: aws.String("AES256"),


For my own info. how is the key managed server-side? Is it fully managed by Amazon?

0xmichalis · 2020-03-27T19:44:32Z

s3/s3_fs.go

+			Key:    aws.String(name),
+		})
+	}
+	return file, err


nit:

return file, nil

0xmichalis · 2020-03-27T19:49:25Z

s3/s3_fs.go

+		Key:    aws.String(nameClean),
+	})
+	if err != nil {
+		if strings.Contains(err.Error(), "404") {


awserr is a thing so please try to cast to typed errors

https://docs.aws.amazon.com/sdk-for-go/api/aws/awserr/

0xmichalis · 2020-03-27T19:51:34Z

@rgarcia if you have time addressing the outstanding comments, I can help with merging.

rgarcia · 2020-03-27T19:53:19Z

Thanks for the review @Kargakis and hi everyone. I made this PR almost 4 years ago and since then my team's priorities have shifted and we no longer use afero. So unfortunately it's very unlikely that I will find the time to finish the work here.

dim · 2020-03-30T08:32:29Z

Thanks @rgarcia, that's why we have come up with our own spin on afero https://github.com/bsm/bfs, it has adapters for in-memory, file system, S3, GCS, FTP and soon AZ Blob

fclairamb · 2020-05-20T21:08:29Z

Hi guys,

I reworked a fork of this implementation here and I'm interested by your feedback on it. It's based on @wreulicke's fork of @aviau's fork of @rgarcia's code and basically fixes some limitations it had. The biggest limitation I had is that it didn't handle the streaming (for upload or download) of files. I used a pipe and a go-routine to handle that.

In my humble opinion:

Each Fs implementation should be in its own repository so that each developer can choose the one he wants
You should recommend some of them or even create an afero-contrib github organization where you share the core Fs

I intend to build some other Afero.Fs for a multi-backend ftp server which was initially only a library.

0xmichalis · 2020-05-20T21:18:54Z

Hi Florent, if you want to submit your S3 backend as a PR to this repo, I'll happily help with reviewing and merging your work.

…

-------- Original Message --------

On 20 May 2020, 23:08, Florent Clairambault wrote: Hi guys, I reworked a fork of this implementation [here](https://github.com/fclairamb/afero-s3) and I'm interested by your feedback on it. It's based on ***@***.***(https://github.com/wreulicke)'s fork of ***@***.***(https://github.com/aviau)'s fork of ***@***.***(https://github.com/rgarcia)'s code and basically fixes some limitations it had. The biggest limitation I had is that it didn't handle the streaming (for upload or download) of files. I used a pipe and a go-routine to handle that. In my humble opinion: - Each Fs implementation should be in its own repository so that each developer can choose the one he wants - You should recommend some of them or even create an afero-contrib github organization where you share the core Fs I intend to build some other Afero.Fs for a multi-backend [ftp server](https://github.com/fclairamb/ftpserver) which was initially only [a library](https://github.com/fclairamb/ftpserverlib). — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#90 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AA5YK7YOBNT3SZJIW6AXGGTRSRBFZANCNFSM4CEPSALA).

pinpox · 2022-06-17T15:28:26Z

Is there still work being done on this?

S3 FS implementation

378e4fb

ns-cweber reviewed Feb 18, 2017

View reviewed changes

nicored reviewed Apr 10, 2017

View reviewed changes

jcftang mentioned this pull request May 17, 2017

feat: add preliminary FileSystem for S3 #123

Closed

Spedge mentioned this pull request Jan 4, 2018

S3 Storage Backend specialedge/hangar-api#17

Open

mrjoshuak added a commit to absfs/afero that referenced this pull request Aug 20, 2018

s3fs merge afero PR spf13#90

22a7703

mrjoshuak mentioned this pull request Aug 20, 2018

S3fs absfs/afero#6

Open

0xmichalis reviewed Mar 27, 2020

View reviewed changes

s3/s3_fs.go

Key: aws.String(name),

})

}

return file, err

Copy link

Collaborator

0xmichalis Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

return file, nil

0xmichalis reviewed Mar 27, 2020

View reviewed changes

0xmichalis mentioned this pull request Mar 27, 2020

[RFC] Adding S3 support #11

Open

francislavoie mentioned this pull request Sep 12, 2020

Feature Request: Operating system interface for file server caddyserver/caddy#3720

Closed

0xmichalis closed this Sep 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3 FS implementation #90

S3 FS implementation #90

rgarcia commented May 22, 2016

CLAassistant commented May 22, 2016 •

edited

dim commented Jun 28, 2016

ns-cweber commented Feb 18, 2017

ns-cweber Feb 18, 2017

ns-cweber Feb 18, 2017 •

edited

ns-cweber Feb 18, 2017 •

edited

nicored Apr 10, 2017 •

edited

justinclift May 28, 2017

nicored May 29, 2017

justinclift May 29, 2017

nicored May 29, 2017

nicored left a comment

jcftang commented Oct 5, 2017

aviau commented Apr 27, 2018 •

edited

aviau commented Jul 19, 2018 •

edited

0xmichalis Mar 27, 2020

0xmichalis Mar 27, 2020

0xmichalis Mar 27, 2020

0xmichalis Mar 27, 2020

0xmichalis Mar 27, 2020

0xmichalis Mar 27, 2020

0xmichalis Mar 27, 2020

0xmichalis Mar 27, 2020

0xmichalis commented Mar 27, 2020

rgarcia commented Mar 27, 2020

dim commented Mar 30, 2020

fclairamb commented May 20, 2020

0xmichalis commented May 20, 2020 via email

pinpox commented Jun 17, 2022

		@@ -24,7 +24,7 @@ import (
		// readDirNames reads the directory named by dirname and returns

S3 FS implementation #90

S3 FS implementation #90

Conversation

rgarcia commented May 22, 2016

CLAassistant commented May 22, 2016 • edited

dim commented Jun 28, 2016

ns-cweber commented Feb 18, 2017

Choose a reason for hiding this comment

ns-cweber Feb 18, 2017 • edited

Choose a reason for hiding this comment

ns-cweber Feb 18, 2017 • edited

Choose a reason for hiding this comment

nicored Apr 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicored left a comment

Choose a reason for hiding this comment

jcftang commented Oct 5, 2017

aviau commented Apr 27, 2018 • edited

aviau commented Jul 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Mar 27, 2020

rgarcia commented Mar 27, 2020

dim commented Mar 30, 2020

fclairamb commented May 20, 2020

0xmichalis commented May 20, 2020 via email

pinpox commented Jun 17, 2022

CLAassistant commented May 22, 2016 •

edited

ns-cweber Feb 18, 2017 •

edited

ns-cweber Feb 18, 2017 •

edited

nicored Apr 10, 2017 •

edited

aviau commented Apr 27, 2018 •

edited

aviau commented Jul 19, 2018 •

edited