Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[AIRFLOW-5117] support refreshing EKS api tokens #5731

Merged
merged 1 commit into from
Nov 12, 2019
Merged

[AIRFLOW-5117] support refreshing EKS api tokens #5731

merged 1 commit into from
Nov 12, 2019

Conversation

houqp
Copy link
Member

@houqp houqp commented Aug 6, 2019 •
edited by dimberman
Loading

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:

This PR patches upstream k8s client to check for EKS api token refresh. Without this, Airflow K8S integration won't work if not deployed in cluster.

When Airflow scheduler or worker is not deployed in K8S cluster, all K8S API call will fail with 401 error after running for about 14min.

This is due to EKS enforces a 15min timeout for all API tokens but the official k8S python client doesn't support token refresh.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

see: tests/kubernetes/test_client.py

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

  • Passes flake8

@houqp
Copy link
Member Author

houqp commented Aug 6, 2019

upstream patch for swagger-codegen: swagger-api/swagger-codegen#9624. I will work the PR for k8s client once the swagger-codegen PR is approved and merged.

@houqp houqp force-pushed the eks branch 11 times, most recently from 107cdbe to d5c474f Compare August 8, 2019 00:12
@houqp
Copy link
Member Author

houqp commented Aug 8, 2019

@ashb tests added, ready for review :)

@houqp
Copy link
Member Author

houqp commented Aug 8, 2019

Looks like upstream k8s client is moving to a new openapi code generator, I have sent my code gen fix to openapi-generator as well. It will probably take awhile for the fix to land in upstream k8s client given they are still in the process of migrating to openapi-generator.

All my upstream fix activities can be tracked at kubernetes-client/python#741.

@dimberman
Copy link
Contributor

@houqp I'm a bit conflicted about this. On the one hand this is a very important bug that we need to address, on the other hand I'd want to minimize how much k8s logic is in airflow. Would it be possible to further isolate this so it would be easier to remove once the necessary fixes are implemented?

@houqp houqp force-pushed the eks branch 2 times, most recently from 856fedd to e4f6d7a Compare August 11, 2019 23:54
@houqp
Copy link
Member Author

houqp commented Aug 11, 2019

@dimberman my original patch added comment for every function and Class that can be safely removed once patch lands in upstream. I have pushed a new change to move most of the patching logic into a separate module to make it easier to follow.

ashb
ashb reviewed Aug 12, 2019
View reviewed changes
scripts/ci/pylint_todo.txt Outdated Show resolved Hide resolved
dimberman
dimberman previously requested changes Aug 12, 2019
View reviewed changes
Copy link
Contributor

@dimberman dimberman left a comment

Choose a reason for hiding this comment

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

Other than @ashb 's pylint comment and a a nit on my end this LGTM

airflow/kubernetes/refresh_config.py Show resolved Hide resolved
@mik-laj mik-laj added the k8s label Aug 15, 2019
@bhtucker
Copy link

bhtucker commented Jan 28, 2020 •
edited
Loading

Hey @danielpoonwj --

This appears to be a Python 3.7 dependency. The problem is the same as in this SO question .

Looks like there's already an issue for this: https://issues.apache.org/jira/browse/AIRFLOW-6585 and a PR: #7153

@Bruschkov Bruschkov mentioned this pull request Feb 3, 2020
Closed
@dgzlopes dgzlopes mentioned this pull request Mar 2, 2020
Open
@houqp houqp deleted the eks branch July 9, 2020 20:01
@dstandish dstandish mentioned this pull request Jan 7, 2022
Merged
dstandish added a commit to astronomer/airflow that referenced this pull request Mar 9, 2022
@dstandish
Remove RefreshConfiguration workaround for K8s token refreshing
a4d0ddb 
A workaround was added (apache#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).
dstandish added a commit to astronomer/airflow that referenced this pull request Mar 16, 2022
@dstandish
Remove RefreshConfiguration workaround for K8s token refreshing
574c63e 
A workaround was added (apache#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).
dstandish added a commit that referenced this pull request Mar 16, 2022
@dstandish
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
7bd165f 
A workaround was added (#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).
shuhoy pushed a commit to shuhoy/airflow that referenced this pull request Mar 17, 2022
@dstandish @shuhoy
Remove RefreshConfiguration workaround for K8s token refreshing (apac…
8abb75a 
…he#20759)

A workaround was added (apache#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).
potiuk pushed a commit that referenced this pull request Mar 26, 2022
@dstandish @potiuk
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
aafd768 
A workaround was added (#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).

(cherry picked from commit 7bd165f)
potiuk pushed a commit that referenced this pull request Mar 26, 2022
@dstandish @potiuk
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
8376f76 
A workaround was added (#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).

(cherry picked from commit 7bd165f)
ephraimbuddy pushed a commit that referenced this pull request Mar 26, 2022
@dstandish @ephraimbuddy
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
58313cd 
A workaround was added (#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).

(cherry picked from commit 7bd165f)
ephraimbuddy pushed a commit that referenced this pull request Mar 26, 2022
@dstandish @ephraimbuddy
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
d39197f 
A workaround was added (#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).

(cherry picked from commit 7bd165f)
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jun 4, 2022
@dstandish
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
5765f8e 
A workaround was added (apache/airflow#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).

(cherry picked from commit 7bd165fbe2cbbfa8208803ec352c5d16ca2bd3ec)

GitOrigin-RevId: d39197fd13b0d96c2ab84ca3f1f13391dbf59572
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jul 10, 2022
@dstandish
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
1d077ef 
A workaround was added (apache/airflow#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).

GitOrigin-RevId: 7bd165fbe2cbbfa8208803ec352c5d16ca2bd3ec
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Aug 30, 2022
@dstandish
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
56bf0e1 
A workaround was added (apache/airflow#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).

GitOrigin-RevId: 7bd165fbe2cbbfa8208803ec352c5d16ca2bd3ec
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 4, 2022
@dstandish
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
cbbcc64 
A workaround was added (apache/airflow#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).

GitOrigin-RevId: 7bd165fbe2cbbfa8208803ec352c5d16ca2bd3ec
aglipska pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 7, 2022
@dstandish
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
5d63756 
A workaround was added (apache/airflow#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).

GitOrigin-RevId: 7bd165fbe2cbbfa8208803ec352c5d16ca2bd3ec
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Dec 7, 2022
@dstandish
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
fa4786d 
A workaround was added (apache/airflow#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).

GitOrigin-RevId: 7bd165fbe2cbbfa8208803ec352c5d16ca2bd3ec
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jan 27, 2023
@dstandish
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
182aecf 
A workaround was added (apache/airflow#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).

GitOrigin-RevId: 7bd165fbe2cbbfa8208803ec352c5d16ca2bd3ec
kosteev pushed a commit to kosteev/composer-airflow-test-copybara that referenced this pull request Sep 12, 2024
@dstandish
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
37e84a9 
A workaround was added (apache/airflow#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).

GitOrigin-RevId: 7bd165fbe2cbbfa8208803ec352c5d16ca2bd3ec
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 18, 2024
@dstandish
Remove RefreshConfiguration workaround for K8s token refreshing (#20759)
10116a5 
A workaround was added (apache/airflow#5731) to handle the refreshing of EKS tokens.  It was necessary because of an upstream bug.  It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).

GitOrigin-RevId: 7bd165fbe2cbbfa8208803ec352c5d16ca2bd3ec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashb ashb ashb approved these changes

@dimberman dimberman Awaiting requested review from dimberman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@houqp @dimberman @ashb @codecov-io @danielpoonwj @bhtucker @mik-laj