[AIRFLOW-6585] Fixed Timestamp bug in RefreshKubeConfigLoader #11219
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
|
|
@ashb you reviewed this before. Can you have a look? |
airflow/kubernetes/refresh_config.py
Outdated
| import yaml | ||
| from kubernetes.client import Configuration | ||
| from kubernetes.config.exec_provider import ExecProvider | ||
| from kubernetes.config.kube_config import KUBE_CONFIG_DEFAULT_LOCATION, KubeConfigLoader | ||
|
|
||
|
|
||
| def _parse_timestamp(ts_str: str) -> int: | ||
| parsed_dt = pendulum.parse(ts_str) | ||
| if isinstance(parsed_dt, pendulum.DateTime): |
There was a problem hiding this comment.
What's this for? Doesn't pendulum throw an error if it can't parse?
There was a problem hiding this comment.
The problem is, that pendulum.parse type annotations says it return typing.Union[Date, Time, DateTime, Duration], however not all of these implement timetuple().
One of the CI tests (I guess it was mypy) yelled at me for not covering the other possible return types. I was not able to find an input string which didn't result in either a parser error or in a return type DateTime so it doesn't really matter I guess.
There was a problem hiding this comment.
Ah, cast from the typing module or a # type: comment is probably the way to go then
There was a problem hiding this comment.
Ah, yes, cast sounds like it was made exactly for cases like this. Never heard about it before. I adjusted the code.
| ts = _parse_timestamp("2020-01-13T13:42:20Z") | ||
| self.assertEqual(1578922940, ts) | ||
|
|
||
| def test_parse_timestamp_should_convert_regular_timezone_to_unix_timestamp(self): |
There was a problem hiding this comment.
Could you test an error condition too?
|
@ashb is this ok or do you want a different approach regarding pendulum? |
|
Awesome work, congrats on your first merged pull request! |
…#11219) Co-authored-by: Jan Brusch <jan.brusch@neuland-bfi.de> Co-authored-by: Marian Cepok <marian.cepok@neuland-bfi.de>
…#11219) Co-authored-by: Jan Brusch <jan.brusch@neuland-bfi.de> Co-authored-by: Marian Cepok <marian.cepok@neuland-bfi.de>
Jira
https://issues.apache.org/jira/browse/AIRFLOW-6585
Description
This is basically #7153 which I extended by using pendulum to parse time strings as asked in #7153 (review) . Since that PR is closed I opened a new one.
Original description:
When using the KubernetesPodOperator on an aws kubernetes cluster, the aws-iam-authenticator is used to obtain kubernetes authentication tokens. The aws tokens contain ISO-8601 formatted timestamps, which couldn't be parsed in case of a "Z" (Zulu Time) timezone.