Use ftp w/ TLS for default IERS download (CDDIS) by eteq · Pull Request #9964 · astropy/astropy

eteq · 2020-02-20T00:48:35Z

This is an attempt to address #9963. It is quite a bit hackier that I would like, but I don't really see an obvious way to do any better without circumventing the caching machinery. I'm also very curious to see if the tests pass - it worked for me locally, but some of these remote tests behave quite differently on the CI...

One subtle side-effect of this implementation is that any ftp IERS URLs now use TLS with no way to turn it off. There are some ways I can imagine to avoid that if we think it is important (e.g. use "ftps" instead of "ftp" to indicate the tls version), but I'm not really sure how/if it's important at all.

pep8speaks · 2020-02-20T00:48:39Z

Hello @eteq 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2020-04-24 19:43:21 UTC

bsipocz · 2020-02-20T01:05:58Z

interesting, test are complaining that they should be remote_data

mhvk · 2020-02-20T02:29:47Z

    """
    kwargs.setdefault('http_headers', {'User-Agent': 'astropy/iers',
                                       'Accept': '*/*'})
+    kwarg.setdefault('ftp_tls', True)


Typo: should be kwargs

it worked for me locally

Wait a minute, how did the test even pass with this typo?

The tests did fail! (At least remote_data) That's how I found out.

@mhvk , sorry for the confusion, but I meant how @eteq got it to work locally.

ah, yes, now you mention it, I wondered about that too!

Good question. I think it's because I tested an earlier version to prove it works and was in a hurry at the end so didn't run the later versions with --remote-data... 🐑 - I cooked up a hacky version first, tested that, but then didn't with the cleaned-up version.

mhvk · 2020-02-20T02:30:31Z

Certainly for a temporary work-around this looks fine. It is strange that CDDIS doesn't provide anonymous download via https.

pllim

Yeah, does seem hacky and I don't know how I feel about download_data having more and more options. But like you pointed out, we do not have a lot of choices here other than creating our own mirror.

pllim · 2020-02-20T19:13:06Z

        http_headers = {}

+    if ftp_tls:
+        urlopener = urllib.request.build_opener(_FTPTLSHandler())


Should there be a check to raise exception if this option is given but URL is not FTP?

I don't think so. This is pretty deep into some hacky layers of urllib, but based on https://docs.python.org/3/library/urllib.request.html#urllib.request.build_opener it looks to me like this just replaces the default FTP handler with the new one. So if the user passes in a non-ftp url a different handler takes care of it and this one is just skipped.

pllim · 2020-02-20T19:15:51Z

I think this needs a change log too.

eteq · 2020-02-25T23:18:08Z

Added changelog and typo fix. There's still one remaining error (see, I did run the tests this time!) which I'll have to debug later.

Re: @mhvk's:

It is strange that CDDIS doesn't provide anonymous download via https.

Yeah I thought so too. It's possible there's some undocumented way but I figured it was safer to use what they support on their web site instead...

pllim

I think you code broke the monkeypatching in test_data.py.

pllim · 2020-02-26T16:03:33Z

+astropy.utils
+^^^^^^^^^^^^^
+
+- ``astropy.utils.data.download_file` now supports FTPS/FTP over TLS. (#9964)


Why is there a new feature entry? Also parenthesis should be square brackets.

New feature entry because this really is a new feature of allowing FTPS for any use of download_file. And that's actually kinda a big deal. It's just we're forced to do that to enable the bug fix, otherwise I would have had this be 4.1.

(parenths was a typo - fixed in the rebased version)

Do we have a precedence to add "new feature" in a bug fix change log? I'll let @bsipocz decide.

I think it's fine. It's not an API change, but non user oriented utils feature that helps to resolve a bug.

We occasionally had utils new features in bugfix releases in the past, e.g. for the decorators. E.g. 2.0.12 or 1.3.1.

bsipocz · 2020-02-28T23:54:44Z

 astropy.utils
 ^^^^^^^^^^^^^

+- The default IERS server has been updated to use the FTPS server hosted by


this has to come down to the 4.0.1 section, too

bsipocz · 2020-02-28T23:56:26Z

test failures are related

pllim · 2020-04-03T17:21:24Z

@eteq , do you have plans to wrap this up and address the monkeypatching failure?

eteq · 2020-04-24T19:20:43Z

I think this is all set now, although @pllim and/or @bsipocz might want to give it a finally look. The monkeypatch is now working locally (we shall see if travis agrees...). I'm not especially proud of the monkeypatching solution, but hey a hack on a hack is elegant code right? 😉

pllim

Just a minor typo left on change log, otherwise LGTM. Thanks!

bsipocz

I don't think doubling hacks are negating each other to become elegant, but agree that a hack that works here is more than good enough.

Co-Authored-By: P. L. Lim <[email protected]>

bsipocz · 2020-04-24T19:43:41Z

Added the changelog changes so this doesn't queue twice in CI

bsipocz · 2020-04-24T22:16:05Z

Thanks @eteq!

Use ftp w/ TLS for default IERS download (CDDIS)

bsipocz · 2020-04-29T03:09:54Z

+
    req = urllib.request.Request(source_url, headers=http_headers)
-    with urllib.request.urlopen(req, timeout=timeout) as remote:
+    with urlopener.open(req, timeout=timeout) as remote:


@eteq - this breaks astroquery

Let's discuss at #10227.

aarchiba · 2020-07-31T17:29:45Z

#10437 now defaults to "if no-TLS fails retry with TLS"; I think special handling of IERS URLs could go away.

eteq added the utils.iers label Feb 20, 2020

astropy-bot Bot added the utils label Feb 20, 2020

eteq changed the title ~~Use ftps for iers cddis~~ Use ftp w/ TLS for default IERS download (CDDIS) Feb 20, 2020

eteq added this to the v4.0.1 milestone Feb 20, 2020

mhvk reviewed Feb 20, 2020

View reviewed changes

pllim reviewed Feb 20, 2020

View reviewed changes

pllim reviewed Feb 26, 2020

View reviewed changes

eteq force-pushed the use-ftps-for-iers-cddis branch from f15fa79 to 1a8036b Compare February 26, 2020 16:56

bsipocz reviewed Feb 28, 2020

View reviewed changes

bsipocz modified the milestones: v4.0.1, v4.0.2 Mar 21, 2020

eteq added 6 commits April 24, 2020 13:45

Allow FTP TLS as an optional handler for downloads

719aaba

Switch iers to use ftp with TLS

7bece86

pep8

212f297

fix kwargs typo

a09c419

add changelog entry

169cce5

update monkeypatching to work with builder instead of urlopen

6bb2086

eteq force-pushed the use-ftps-for-iers-cddis branch from 1a8036b to 6bb2086 Compare April 24, 2020 19:16

update changelog section

989d3ef

eteq force-pushed the use-ftps-for-iers-cddis branch from 0de9fce to 989d3ef Compare April 24, 2020 19:19

pllim reviewed Apr 24, 2020

View reviewed changes

Comment thread CHANGES.rst Outdated

bsipocz reviewed Apr 24, 2020

View reviewed changes

Comment thread CHANGES.rst Outdated

bsipocz approved these changes Apr 24, 2020

View reviewed changes

fix changelog

84e855c

Co-Authored-By: P. L. Lim <[email protected]>

bsipocz added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Apr 24, 2020

bsipocz merged commit 87e15ce into astropy:master Apr 24, 2020

bsipocz added a commit that referenced this pull request Apr 28, 2020

Merge pull request #9964 from eteq/use-ftps-for-iers-cddis

1ae5f62

Use ftp w/ TLS for default IERS download (CDDIS)

bsipocz added a commit that referenced this pull request Apr 28, 2020

Merge pull request #9964 from eteq/use-ftps-for-iers-cddis

06a6902

Use ftp w/ TLS for default IERS download (CDDIS)

mhvk mentioned this pull request Apr 28, 2020

IERS warning during astropy.coordinates for dates that should be covered by IERS data #10219

Closed

bsipocz reviewed Apr 29, 2020

View reviewed changes

bsipocz mentioned this pull request Apr 29, 2020

Change in default data download handling breaks astroquery #10227

Closed

pllim mentioned this pull request Apr 29, 2020

TST: utils monkeypatch compatibility with astropy 4.1/4.0.2 astropy/astroquery#1711

Merged

pllim mentioned this pull request Jul 31, 2020

Lock-free cache implementation #10437

Merged

pllim mentioned this pull request Aug 28, 2020

Backport PR 10437 to LTS #10683

Merged

pllim mentioned this pull request Jan 19, 2021

Fix certificate error for IERS data download on Windows #10434

Merged

Conversation

eteq commented Feb 20, 2020 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-04-24 19:43:21 UTC

Uh oh!

bsipocz commented Feb 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eteq Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Feb 20, 2020

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Feb 20, 2020

Uh oh!

eteq commented Feb 25, 2020

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Feb 28, 2020

Uh oh!

pllim commented Apr 3, 2020

Uh oh!

eteq commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bsipocz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Apr 24, 2020

Uh oh!

bsipocz commented Apr 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarchiba commented Jul 31, 2020

Uh oh!

Reviewers

Assignees

eteq commented Feb 20, 2020 •

edited by pllim

Loading

pep8speaks commented Feb 20, 2020 •

edited

Loading

pllim Feb 20, 2020 •

edited

Loading

eteq Feb 25, 2020 •

edited

Loading

eteq commented Apr 24, 2020 •

edited

Loading

bsipocz left a comment •

edited

Loading