-
Notifications
You must be signed in to change notification settings - Fork 96
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
Config.DialContext hook and http.RoundTripper support #31
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution. Can you help me understand why Config.DialContext hook is needed?
client.go
Outdated
@@ -369,15 +379,18 @@ func (c *Client) openConn(idx int, host string) (pconn *persistentConn, err erro | |||
|
|||
var conn net.Conn | |||
|
|||
ctx := context.Background() | |||
if c.config.Timeout != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config.Timeout has a default so it will never be 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saw that. I’m not sure I agree with the design decision (versus defaulting timeout if zero at runtime), but it’s your library. Happy to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (719b580). I also noted where the library should wire up a parent Context
in the future to allow overall FTP operations to be governed by a caller’s Context
.
transport.go
Outdated
// Transport implements the http.RoundTripper interface. | ||
// Typical usage would be to register a Transport to handle | ||
// ftp:// and/or ftps:// URLs with http.Transport.RegisterProtocol. | ||
type Transport struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we name this HTTPTransport
so it isn't confused for an ftp transport?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think bare Transport
works better because it is an FTP transport that happens to implement the http.RoundTripper
interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eliminated the separate Transport
struct in favor of a method on Config
.
To answer your question (“why do you need a Our systems connect to many (i.e. hundreds) of systems that allow-list our IP addresses. We maintain proxy hosts with static IPs at various cloud providers and our production hosts connect to the remote hosts via an SSH+SOCKS5 tunnel. We need to be able to connect to remote FTP servers and control how the network connection is established. Our backend has dialers (with a Your FTP library was the most mature/robust, so we forked it to add the |
While we’re on the subject of naming things, it feels itchy to me that the Would you be opposed to adding a top-level function |
Agreed.
Bad name is bad, but having a bad name and another name doesn't help much at this point. Once modules are the thing we can do a v2 and go crazy renaming. |
Re: |
This enables clients to specify their own connection logic, to permit creation of connections through a proxy (for IP whitelisting).
Not all net.Conn implementations provide a valid RemoteAddr(), in particular crypto/ssh.Conn: https://godoc.org/golang.org/x/crypto/ssh#Client.Dial
This enables clients to handle GET requests of ftp:// or ftps:// URLs with a normal http.Client using http.Transport.RegisterProtocol().
Also note where parent Context should be wired up in future.
…RL.User is non-nil
- Update proftpd to 1.3.7a and remove monkey patches - Update pure-ftpd to 1.0.49 - Parameterized version numbers - Check-before-download to speed up repeated builds
This is disabled in .travis.yml
…ously get error and set res.Status
This PR implements two related features that we needed in production to enable downloads of files from FTP(S) servers that filter connections to IP allow-lists.
Config.DialContext
hook, mirrored from the implementation innet/http.Transport
. If set, this is used instead ofnet.Dial
to establish TCP connections.http.RoundTripper
interface onConfig
to allow anhttp.Client
downloadftp://
URLs.A few additional things for 2021:
go.mod
file (required for Go 1.16+ support).