Skip to content

Refresh GKE OAuth2 tokens#32673

Merged
potiuk merged 3 commits into
apache:mainfrom
fdemiane:gkeoperatorfix
Jul 20, 2023
Merged

Refresh GKE OAuth2 tokens#32673
potiuk merged 3 commits into
apache:mainfrom
fdemiane:gkeoperatorfix

Conversation

@fdemiane

Copy link
Copy Markdown
Contributor

closes: #31648

When GKEStartPodOperator is taking more than one hour to complete, it is failing with "unauthorised" error messages because the OAuth2 token that was used wasn't being refreshed before API calls.

In this PR we are making sure to refresh expired tokens before API calls to kubernetes.

@boring-cyborg boring-cyborg Bot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers provider:google Google (including GCP) related issues labels Jul 18, 2023
@eladkal
eladkal requested a review from hussein-awala July 18, 2023 13:09

@hussein-awala hussein-awala left a comment

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.

I attempted a similar solution previously, but without using refresh_api_key_hook. However, I believe that yours should work and resolve the issue. LGTM.

Just a small nit comment

Comment on lines 394 to 397

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.

You could always set the api_key and use the _get_token method, which return the old api_key if it is still valid and a new one if not.

Suggested change
creds = self.get_credentials()
if not GKEPodHook._is_credentials_valid(creds):
GKEPodHook._refresh_token(creds)
configuration.api_key = {"authorization": creds.token}
configuration.api_key = {"authorization": self._get_token(self.get_credentials())}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review! Fixed

@hussein-awala

Copy link
Copy Markdown
Member

You should fix the failed tests before merging, for static checks, here is the doc.

@fdemiane
fdemiane marked this pull request as ready for review July 19, 2023 10:23

@kosteev kosteev left a comment

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.

LGTM

@kosteev

kosteev commented Jul 20, 2023

Copy link
Copy Markdown
Contributor

@potiuk @hussein-awala If it is approved, can we merge this PR?

@potiuk
potiuk merged commit 848c69a into apache:main Jul 20, 2023
@boring-cyborg

boring-cyborg Bot commented Jul 20, 2023

Copy link
Copy Markdown

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Getting Unauthorized error messages with GKEStartPodOperator if pod execution is over 1 hour

4 participants