Upplication / Amazon-S3-FileSystem-NIO2

An S3 File System Provider for Java 7
MIT License
122 stars 67 forks source link

Incompatible as a FileSystem with Apache MINA SSHD #56

Closed rossdrew closed 8 years ago

rossdrew commented 8 years ago

Creating an Apache MINA SFTP server and using the S3FileSystemProvider doesn't work as MINA uses this readAttributes

public Map<String, Object> readAttributes(Path path, String attributes, LinkOption... options) throws IOException

which is not implemented and results in directories not being listed as expected.

jarnaiz commented 8 years ago

Hello Rossdrew,

thanks for your report.

I will try to implement the method this week, i think is not a hard task.

bye!

rossdrew commented 8 years ago

I've implemented it already in a rather hacky fashion and it then works (at least to list directory contents) with MINA. I'm not really sure the best way to get file permissions though.

jarnaiz commented 8 years ago

If you can give me some tips/info about what kind of parameters (LinkOptions and attributes) need to read the Apache MINA. Can help me a lot :)

thanks!

rossdrew commented 8 years ago

My hacky method (for S3FileAttributes)is as follows. I've hard-coded the permissions as I'm not sure where I would get that information from at this point...

public Map<String, Object> getAsMap(){
    Map<java.lang.String, java.lang.Object> map = new HashMap<>();
    map.put("fileKey",fileKey());
    map.put("isOther",isOther());
    map.put("isRegularFile",isRegularFile());
    map.put("isDirectory",isDirectory());
    map.put("isSymbolicLink",isSymbolicLink());
    map.put("size",size());
    map.put("lastModifiedTime",lastModifiedTime());
    map.put("lastAccessTime",lastAccessTime());
    map.put("permissions", PosixFilePermissions.fromString("rw-rw----"));
    //extended, uig, gid, owner, group, acl didn't seem to be required.

    return map;
}

Without the permissions implemented, MINA will call getFile() which is also unimplemented here and throws another UnsupportedOperationException

jarnaiz commented 8 years ago

Hello Rossdrew,

I hope version 1.3.0 solve your problem :)

Thanks for the feedback!

rossdrew commented 8 years ago

It doesn't I'm afraid, as, currently

public <A extends BasicFileAttributes> A readAttributes(Path path, Class<A> type, LinkOption... options) throws IOException

doesn't return permissions as an attribute, as required by MINA. As that information is never really collected anywhere. At this point, I'm just running with a hack to readAttributes or S3FileSystemProvider

    @Override
    public Map<String, Object> readAttributes(Path path, String attributes, LinkOption... options) throws IOException {
        if (attributes == null) {
            throw new IllegalArgumentException("Attributes null");
        }

        if (attributes.contains(":") && !attributes.contains("basic:")) {
            throw new UnsupportedOperationException(format("attributes %s are not supported, only basic are supported", attributes));
        }

        Map<String, Object> attributeMap = null;
        if (attributes.equals("*") || attributes.equals("basic:*")) {
            BasicFileAttributes attr = readAttributes(path, BasicFileAttributes.class, options);
            attributeMap = AttributesUtils.fileAttributeToMap(attr);
        } else if (attributes.contains(",")) {
            String[] filters = attributes.split(",");
            BasicFileAttributes attr = readAttributes(path, BasicFileAttributes.class, options);
            attributeMap = AttributesUtils.fileAttributeToMap(attr, filters);
        }

        if (attributeMap != null) {
            //XXX Hack as I have no idea how to get Amazon permissions
            attributeMap.put("permissions", PosixFilePermissions.fromString("rw-rw----"));
            return attributeMap;
        }

        throw new UnsupportedOperationException();
    }

FYI: There are also other reasons this wont work with MINA, as I've found out. For example, it' requires an implementation of S3FileChannel in order to do an FTP GET.

jarnaiz commented 8 years ago

Ups... I think I missundertand you :/

What its the subclass needed for Apache MINA? s3fs only support BasicFileAttributes and S3FileAttributes and dont have the key "permissions".

In your first comment you talk to me about the method:

public Map<String, Object> readAttributes(Path path, String attributes, LinkOption... options) throws IOException

And now:

public <A extends BasicFileAttributes> A readAttributes(Path path, Class<A> type, LinkOption... options) throws IOException

Can you writte me some test or spike so we can reproduce your expected behaviour?

Sorry!

rossdrew commented 8 years ago

Yes, it requires the Map<String, Object> return type but your new implementation calls the one with the <A extends BasicFileAttributes> return type, which has no permissions

I was unaware that permissions isn't included in basic attributes however. It seems I have bigger problems here as Apache MINA SSHD requires the permissions attribute.

My project is pretty small if you fancy running it to improve your project I just provide my own S3FilesystemFactory to MINAs SftpSubsystem via SshServer in my SFTPService class and MINA SSHD does the rest. The intention was to create an SFTP front-end for S3.

rossdrew commented 8 years ago

So FYI, I've gotten it working with MINA SSHD with a few hacks. Namely adding permissions and a FileChannel option. I would submit a pull request but it's a little too hacky, I have a feeling you'll be able to do this better than me :)

public class S3FileSystemProviderWithFileChannel extends S3FileSystemProvider {
    /**
     * Just a FileChannel interface that points to a SeekableByteChannel
     */
    @Override
    public FileChannel newFileChannel(Path path,
                                      Set<? extends OpenOption> options,
                                      FileAttribute<?>... attrs)
            throws IOException
    {
        S3Path s3Path = toS3Path(path);
        return new S3FileChannel(s3Path, options);
    }

    /**
     * Private in original library so needs to be duplicated
     */
    private S3Path toS3Path(Path path) {
        Preconditions.checkArgument(path instanceof S3Path, "path must be an instance of %s", S3Path.class.getName());
        return (S3Path) path;
    }

    @Override
    public Map<String, Object> readAttributes(Path path, String attributes, LinkOption... options) throws IOException {
        Map<String, Object> attributeMap = super.readAttributes(path, attributes, options);

        if (attributeMap != null) {
            //XXX Hack as I have no idea how to get Amazon permissions as S3ObjectSummary (Amazon AWS SDK) doesn't contain any
            attributeMap.put("permissions", PosixFilePermissions.fromString("rw-rw----"));
        }

        return attributeMap;
    }
}
jarnaiz commented 8 years ago

Hi @rossdrew

I implemented the method readAttributes with PosixFileAttributes.class in tag 1.4.0. The conversion between S3ACLPermission and PosixPermission is very simple and limitated but I think you can use it without any workaround in your project and the implementation can be improved in the future.

cheers!

robmoore commented 7 years ago

I noticed that S3FileSystem still has code which doesn't include posix as a member.

    @Override
    public Set<String> supportedFileAttributeViews() {
        return ImmutableSet.of("basic");
    }

When the Mina sshd code attempts to check file attributes, it only sees basic so it skips the code path for posix attributes and fails due to an unimplemented toFile() method in S3Path. It's calling toFile() as a fallback since it tries to see whether it can get them from File instead.

It seems like posix should be added as an option.

robmoore commented 7 years ago

Just wanted to bump this issue to make sure my comment was seen. Should I create another ticket?