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.

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

fix (kubernetes-client-api) :Add new autoOAuthToken field in Config to differentiate between user and autoconfigured tokens (#4802) #4951

Merged
merged 2 commits into from
May 24, 2023

Conversation

rohanKanojia
Copy link
Member

@rohanKanojia rohanKanojia commented Mar 6, 2023

Description

Fix #4802

  • Add a new string field autoOAuthToken in Config, this field would be
    used by Config internally to store token obtained during
    autoconfiguration process.
  • Remove setOAuthToken references from token interceptors and Config, it
    would only be called by user (during new ConfigBuilder().withOAuthToken("...") call)
    whenever custom token is required.
  • getOAuthToken would give most priority to token from provider, then
    actual value of oauthToken and finally autoOAuthToken value. All
    interceptors would keep using getOAuthToken to get resolved OAuth
    token value

Updated TokenRefreshInterceptor / OpenShiftOAuthInterceptor to match these expectations:

Orignally posted by @shawkins:

  • if it's a user token(oauthToken is set), don't do any refresh logic at all.
  • if we don't have an oauth token provider, then every minute we could do a proactive "soft" refresh the Config (like the current refresh method) - that may do some file reads or even some exec/system calls, so it's best if it's not happening inline with the http requests and if applicable and the token is nearing expiration do the oidc call.
  • If there is an auth failure and there's no user token or token provider we should start with the soft refresh
  • if a token is now there and came from the execconfig or something other than the kubeconfig (service account or env), then use it.
  • If the token still isn't there or came from the kubeconfig and the kubeconfig provides the appropriate AuthProvider, then we should do the oidc refresh

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@@ -94,6 +94,9 @@ public void before(BasicBuilder builder, HttpRequest httpRequest, RequestTags ta

@Override
public CompletableFuture<Boolean> afterFailure(Builder builder, HttpResponse<?> response, RequestTags tags) {
if (config.isTokenNonRefreshable()) {
return CompletableFuture.completedFuture(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Returning true here and in the other interceptor means to try the request again, but since no modifications have been made on the builder, it would just re-issue the existing request.

Including the token provider in isTokenNonRefreshable could be confusing as it may refresh. That is:

if token source == user, then do nothing
if token source == provider, then attempt to get the token again and try once more.

@rohanKanojia rohanKanojia force-pushed the pr/issue4802 branch 11 times, most recently from 014e0da to e80e14a Compare March 10, 2023 21:36
@rohanKanojia rohanKanojia changed the title fix (kubernetes-client-api) : Track sources from where OAuthToken gets set (#4802) fix (kubernetes-client-api) :Add new autoOAuthToken field in Config to differentiate between user and autoconfigured tokens (#4802) Mar 10, 2023
@rohanKanojia rohanKanojia force-pushed the pr/issue4802 branch 2 times, most recently from e453068 to a2c43b8 Compare March 10, 2023 21:42
@rohanKanojia rohanKanojia marked this pull request as ready for review March 13, 2023 06:20
@rohanKanojia rohanKanojia force-pushed the pr/issue4802 branch 2 times, most recently from 7b636cd to 3732033 Compare March 22, 2023 10:21
Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

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

This looks good. It's the easiest way to do this with the existing code structure.

  • the user OauthTokenProvider will take precedence.
  • the user oauthToken will be used next
  • if neither of those were used, then every minute there will be a soft refresh or an open id refresh is the token is near expiration. The code was already doing this, but didn't check for expiration, so we probably can resolve the main thrust of Support for the expiration of the token #2112. The other issues mentioned on there likely remain.
  • on unauthorized failure there will be a last chance refresh

Improvements that can be done later:

  • Don't update the Config - move the autoOathToken and related refresh logic off of the Config and onto to the refresh interceptors. But this requires a lot of changing of how we "refresh" the Config.
  • don't re-attempt the request if the token hasn't actually changed from the initial request
  • coordinate the refresh Coordinate token refresh #4704 - and perform the time based soft refresh in a non IO thread.

@rohanKanojia rohanKanojia force-pushed the pr/issue4802 branch 2 times, most recently from 1091825 to 14fd505 Compare April 13, 2023 14:46
@shawkins
Copy link
Contributor

@rohanKanojia @manusa opened a pr against this pr that updates to latest (as of yesterday), and takes the changes a little further rohanKanojia@d4fba9d - which further clarifies the behavior and tries to localize it better into the TokenRefreshInterceptor. The only thing that may be odd is that I removed some checks for empty string because they were not being applied consistently - they can of course be added back.

@manusa manusa added this to the 6.7.0 milestone May 3, 2023
@shawkins shawkins self-requested a review May 22, 2023 20:22
@shawkins
Copy link
Contributor

LGTM, should be ready after resolving conflicts.

rohanKanojia and others added 2 commits May 24, 2023 11:38
…to differentiate between user and autoconfigured tokens (fabric8io#4802)

+ Add a new string field autoOAuthToken in Config, this field would be
  used by Config internally to store token obtained during
  autoconfiguration process.
+ Remove setOAuthToken references from token interceptors and Config, it
  would only be called by user (during `new ConfigBuilder().withOAuthToken("...")` call)
  whenever custom token is required.
+ getOAuthToken would give most priority to token from provider, then
  actual value of `oauthToken` and finally `autoOAuthToken` value. All
  interceptors would keep using getOAuthToken to get resolved OAuth
  token value

Signed-off-by: Rohan Kumar <rohaan@redhat.com>
Comment on lines +616 to +620
if (autoToken) {
assertEquals("token", config.getAutoOAuthToken());
} else {
assertEquals("token", config.getOauthToken());
}
Copy link
Member

Choose a reason for hiding this comment

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

This will need refactoring assertConfig was already shady as it was.
Adding a boolean parameter and a conditional assert based on that boolean, makes it even more unclear.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

87.3% 87.3% Coverage
0.0% 0.0% Duplication

@manusa manusa merged commit 39b526c into fabric8io:master May 24, 2023
@rohanKanojia rohanKanojia deleted the pr/issue4802 branch May 24, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config.refresh() erases token specified when building initial config
4 participants