Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

request server: add WithStartDirectory option #498

Merged
merged 2 commits into from Mar 3, 2022
Merged

Conversation

drakkan
Copy link
Collaborator

@drakkan drakkan commented Mar 1, 2022

Alternative approach to support a start directory.

I tested #497 and it works fine but I don't like that pkg/sftp clean the path and I have to clean the raw path again in my code.

This approach seems cleaner and the RealPathFileLister is useless if we go this way, we can remove this interface.

The only drawback is that NewRequest now requires an already cleaned path. Prior to this patch the path passed to NewRequest was converted to an absolute UNIX path using / as base. I'll try to fix this compatibility issue later

@drakkan drakkan marked this pull request as draft March 1, 2022 17:14
@drakkan drakkan marked this pull request as ready for review March 2, 2022 10:32
This was referenced Mar 2, 2022
request-interfaces.go Outdated Show resolved Hide resolved
request-server_test.go Outdated Show resolved Hide resolved
request-server_test.go Outdated Show resolved Hide resolved
request-server_test.go Outdated Show resolved Hide resolved
request-server_test.go Outdated Show resolved Hide resolved
request.go Outdated Show resolved Hide resolved
@drakkan
Copy link
Collaborator Author

drakkan commented Mar 2, 2022

Hi @puellanivis

thanks for your review, I'll fix the PR based on your comments.

I know that removing the RealPathLister is a breaking change, but it does not work for real use cases. It only works if you have a client that initially requests the start directory and then sends absolute paths based on the response. I think it is very unlikely that anyone actually uses this interface. If someone is really using it, they should fix their code by migrating to the new server option.

@puellanivis
Copy link
Collaborator

We can just label the interface as deprecated, and say that implementing it won’t actually do anything. The same as we have ErrSshFxOk. Saying “they should fix their code” is all fine and good, but semver is pretty strict on this, and we need to bump the major version in order to remove it.

@drakkan
Copy link
Collaborator Author

drakkan commented Mar 3, 2022

Thank you!

@drakkan drakkan merged commit 59b2472 into pkg:master Mar 3, 2022
@drakkan drakkan deleted the startdir branch July 15, 2022 10:23
@shanehooker
Copy link

I am not finding an example of how to use this with a real file system. I am a bit confused by the InMemHandler(). I would have prefered the WithStartDirectory() to have been a method applied to NewServer() which would have done the same thing as starting 'sftp-server -d [startDirectory]'.

Is there an example of how this can be called and work with a real file system?

@puellanivis
Copy link
Collaborator

The reason for NewRequestServer over NewServer is that the latter can only operate upon the local OS filesystem. Meaning, you cannot establish some other backing store, like s3, or even a caching temporary filesystem held in memory, lost during system restart (that is InMemoryHandler). So, we have elected to essentially “deprecate” NewServer as an insuffienciently extendable interface.

sftp-server -d [startDirectory] works by first doing an os.Chdir() before starting up the service, and letting the inherent OS filesystem management take care of the rest. You should be able to get the NewServer to work the same way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants