Skip to content

FTP Support#1608

Closed
clearswift wants to merge 6 commits intoapache:masterfrom
clearswift:master
Closed

FTP Support#1608
clearswift wants to merge 6 commits intoapache:masterfrom
clearswift:master

Conversation

@clearswift
Copy link
Copy Markdown

At the moment TS blocks requests for FTP URLs with an 'Unsupported Protocol' error message. This precludes TS from simply forwarding requests for FTP URLs to an upstream (parent) proxy.
We propose a fix to allow TS to forward FTP URLs to an upstream (parent) proxy; where this proxy supports HTTP/FTP conversion (e.g. Squid) this would enable FTP resources to be retrieved via TS. This change would also allow protocol plugins to be developed that support FTP URLs.

FTP Support would be optionally enabled with a new configuration setting in records.config preserving existing functionality if not present or set to the default value of false.
e.g.
proxy.config.ftp_enabled=1

@clearswift clearswift changed the title FTP functionality FTP Support Mar 27, 2017
@clearswift clearswift mentioned this pull request Mar 28, 2017
@zwoop zwoop added this to the 7.2.0 milestone Mar 28, 2017

MgmtInt body_factory_response_max_size;

MgmtByte forward_proxy_ftp_enabled;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you move this up where other MgmtByte's are? That avoids unnecessary padding of the bytes (as it is above, you would waste at least 3 bytes in the struct).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I just spoke to our guru and we can easily do this.

HttpEstablishStaticConfigLongLong(c.synthetic_port, "proxy.config.admin.synthetic_port");

// FTP protocol enabled
HttpEstablishStaticConfigByte(c.forward_proxy_ftp_enabled, "proxy.config.ftp_enabled");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we make this overridable as well? That would allow you to turn on /off this feature per remap rule (or via a plugin).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is that usable, though? TS is going to dealt with the request header by that point and already committed to HTTP or FTP.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Do we need to change this? If so, could you provide some more details on what you want us to do.

@@ -0,0 +1,123 @@
'''
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If at all possible, can you convert this to the new AU test system? @dragon512 can perhaps assist as well.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Is there documentation for the new AU system?

Copy link
Copy Markdown
Contributor

@dragon512 dragon512 Mar 28, 2017

Choose a reason for hiding this comment

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

for the test engine what I have at the moment is here:
https://bitbucket.org/dragon512/reusable-gold-testing-system/wiki/Home
For stuff extended for the traffic server is here:
https://github.com/apache/trafficserver/blob/master/tests/getting_started.md

I would suggest look at some of the test to get a basic feel of what your test probably will look like.

@clearswift
Copy link
Copy Markdown
Author

We decided that we need to dedicate some time to these issues and do not anticipate having an update this week as we are busy on other matters.

@zwoop zwoop modified the milestones: 7.2.0, 8.0.0 Apr 25, 2017
@bryancall
Copy link
Copy Markdown
Contributor

[approve ci]

@zwoop
Copy link
Copy Markdown
Contributor

zwoop commented Jun 8, 2017

You guys still working on this? Since autest is gone, that'll need to be removed or changed to use the new autest. Also, going back to my comment, it'd be nice if proxy.config.ftp_enabled was an overridable configuration (such that you can turn it on per e.g. remap rule).

@clearswift
Copy link
Copy Markdown
Author

clearswift commented Jun 12, 2017

It looks like we will not be able to work on this for the next few months at least. Can we ignore this request for the time being? Is is easier for me to just close the pull request for the moment and then in the future create a new one? Thank-you.

Conflicts:
	proxy/http/HttpConfig.h
	tests/tools/sessionvalidation/sessionvalidation.py
@bryancall
Copy link
Copy Markdown
Contributor

[approve ci]

@clearswift
Copy link
Copy Markdown
Author

Closing for the time being.

@clearswift clearswift closed this Jul 21, 2017
@clearswift clearswift removed their assignment Aug 16, 2017
@zwoop zwoop removed this from the 8.0.0 milestone May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants