BF: use time.time (no timezone offset) while checking expiration for AWS credentials #4927
Conversation
There was a problem hiding this comment.
with time.localtime() we are getting local time without any time zone, calendar.timegm
then produces seconds since epoch for that in GMT thus again - no zone.
Hmm, I don't think that's correct. There is a time zone there:
$ python -c 'import time; print(time.localtime().tm_zone)'
EDT
It's just that calendar.timegm expects the time tuple to be in GMT, according to its docstring and implementation:
def timegm(tuple):
"""Unrelated but handy function to calculate Unix timestamp from GMT."""
year, month, day, hour, minute, second = tuple[:6]
days = datetime.date(year, month, 1).toordinal() - _EPOCH_ORD + day - 1
hours = days*24 + hour
minutes = hours*60 + minute
seconds = minutes*60 + second
return secondsBut either way...
With time.time() we seems to be getting seconds since epoch properly.
... I think the switch to time.time is good.
| @@ -212,7 +211,7 @@ def is_expired(self): | |||
| if not exp: | |||
| return True | |||
| exp_epoch = iso8601_to_epoch(exp) | |||
| expire_in = (exp_epoch - calendar.timegm(time.localtime())) / 3600. | |||
| expire_in = (exp_epoch - time.time()) / 3600. | |||
There was a problem hiding this comment.
Do you expect that the expiration field will be in something other than UTC? If so, iso8601_to_epoch suffers from the same issue and the calculation can still be off.
def iso8601_to_epoch(datestr):
"""Given ISO 8601 date/time format, return in seconds since epoch
iso8601 is used to parse properly the time zone information, which
can't be parsed with standard datetime strptime
"""
return calendar.timegm(iso8601.parse_date(datestr).timetuple())I'd say that should use utctimetuple:
diff --git a/datalad/support/network.py b/datalad/support/network.py
index bc9211f08..b291ebbfa 100644
--- a/datalad/support/network.py
+++ b/datalad/support/network.py
@@ -202,7 +202,7 @@ def iso8601_to_epoch(datestr):
iso8601 is used to parse properly the time zone information, which
can't be parsed with standard datetime strptime
"""
- return calendar.timegm(iso8601.parse_date(datestr).timetuple())
+ return calendar.timegm(iso8601.parse_date(datestr).utctimetuple())
def __urlopen_requests(url):
diff --git a/datalad/support/tests/test_network.py b/datalad/support/tests/test_network.py
index f585f3d8a..49237f44a 100644
--- a/datalad/support/tests/test_network.py
+++ b/datalad/support/tests/test_network.py
@@ -529,8 +529,8 @@ def test_is_ssh():
def test_iso8601_to_epoch():
epoch = 1467901515
eq_(iso8601_to_epoch('2016-07-07T14:25:15+00:00'), epoch)
- # zone information is actually not used
- eq_(iso8601_to_epoch('2016-07-07T14:25:15+11:00'), epoch)
+ eq_(iso8601_to_epoch('2016-07-07T14:25:15+11:00'),
+ epoch - 11 * 60 * 60)
eq_(iso8601_to_epoch('2016-07-07T14:25:15Z'), epoch)
eq_(iso8601_to_epoch('2016-07-07T14:25:15'), epoch)
|
oh hoh -- thank you @kyleam for the detailed review! I will tune it up tomorrow following your recommendations -- indeed we should use zone information! |
…AWS S3 credential calendar.timegm expects the time tuple to be in GMT time tuple. time.localtime() has time zone information but it is not part of the time tuple. calendar.timegm then produced seconds since epoch for that in GMT thus zone information was not used. With time.time() we seems to be getting seconds since epoch properly. I kept running into S3 downloads failing while credential reported to be expiring in some hours to come
Thanks @kyleam for the patch
yarikoptic
force-pushed
the
bf-s3-expire
branch
from
c6253b4
to
3a85e9a
Compare
|
ok, forced pushed tuneup to initial commit message + your patch. thanks again @kyleam |
Codecov Report
@@ Coverage Diff @@
## maint #4927 +/- ##
==========================================
- Coverage 89.69% 89.67% -0.02%
==========================================
Files 289 289
Lines 40477 40477
==========================================
- Hits 36305 36297 -8
- Misses 4172 4180 +8
Continue to review full report at Codecov.
|
|
look -- even |
with time.localtime() we are getting local time without any time zone, calendar.timegm
then produces seconds since epoch for that in GMT thus again - no zone.
With time.time() we seems to be getting seconds since epoch properly.
I kept running into S3 downloads failing while credential reported to
be expiring in some hours to come