sftpgo s3afero compatibility

[piedshag]

Hello,

Sftpgo supports using a s3 backend but I've come across some compatibility issues with gofakes3 when using the s3afero backend.

I've managed to resolve the following issues through a couple of changes:

• To create a directory, sftpgo will write a 0 byte blob with content type application/x-directory and key directory_name/. This is the way that AWS recommends creating folders but it doesn't work with a fs backend. To counter this, if we notice that the file being written is 0 bytes we will delete it and create a directory in it's place.

• The other issues are to do with how sftpgo interprets folders. If it is changing directory it will first run a head lookup on the key without the trailing "/", if that fails, it will check to see if it is a folder, so we want to return key missing if it checks on a directory without the trailing "/". It checks differently when you're running a recursive get, it will first list out all the folders then mark directories if they have the trailing "/".

I've made the changes here, it adds functionality for mkdir, changing directories, recursive get and recursive put. I've only made them for the multibucket option, but they could be extended to the other options.

I'm interested to hear your thoughts on potentially getting this merged into master. Are these changes you'd be happy to merge and if so, would you prefer I open a PR to work on getting these changes merged?

[johannesboyne]

Hey @piedshag,

thanks a lot for starting the discussion.

I'm not at all familiar with the Sftpgo codebase so in many ways I can probably not comment very well. In general, the more compatibility we can get the better, especially, if it helps to test code. So if you're using Sftpgo and and would like to test it with a fake s3 backend like gofakes3, I would say it makes sense to consider adding this functionality. I'm just wondering, whether the s3afero backend is the right place. I would assume most would just use the in-memory backend for testing.

The only "concern" I would see: what happens if someone adds 0 byte files for testing purposes and thus would now get directories instead. What could be interesting though is adding this functionality as a flag while instantiating gofakes3. Making it configurable and thus backwards compatible. WDYT?

[shabbyrobe]

I confess I haven't thought this through fully, but my first reaction is to ask if this could be done with a wrapper/middleware backend? That way, any backend can be put behind it, and users of the API can choose whether a backend is wrapped with this support?

[piedshag]

Hey @johannesboyne @shabbyrobe, thanks for getting back to me.

The only "concern" I would see: what happens if someone adds 0 byte files for testing purposes and thus would now get directories instead.

Thanks for raising that, I have changed my fork so that it checks both the file is 0 bytes and the content-type is "application/x-directory". There shouldn't be any accidental directories this way.

What could be interesting though is adding this functionality as a flag while instantiating gofakes3. Making it configurable and thus backwards compatible

I confess I haven't thought this through fully, but my first reaction is to ask if this could be done with a wrapper/middleware backend? That way, any backend can be put behind it, and users of the API can choose whether a backend is wrapped with this support?

I think the changes would bring gofakes3 more in line with the s3 API and wouldn't conflict with the current uses. I have tested sftpgo out with the other backends and it seems that we would have to make a couple of changes like this to make them compatible as well. The changes are small and I think they would be a good default feature to boost gofakes3 compatibility.

[shabbyrobe]

I think the changes would bring gofakes3 more in line with the s3 API

There is a place in the repo where I was testing some assumptions a while back, I think it would be worth adding something to it that proves this assumption first: https://github.com/johannesboyne/gofakes3/tree/master/internal/s3assumer

[johannesboyne]

I will take a further look during the easter break! Thank you for your patience ;-)

[johannesboyne]

I am happy to push this further and clearly see the why. A wrapper would be cool even though looking at your comment @piedshag from 16 days ago, it is considerably low effort to make the changes directly in the backend though. Have you already moved forward with this in your fork?

[piedshag]

Hi @johannesboyne, thanks for getting back to me. I'm just finalising some changes with my fork but I will submit a PR when its ready.