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

Openshift authorization is executed for every request parallelized. #4673

Closed
ismadirolas opened this issue Dec 14, 2022 · 7 comments
Closed
Milestone

Comments

@ismadirolas
Copy link

Is your enhancement related to a problem? Please describe

If I create an OpenshiftClient with a custom executor service and an OkHttpClientFactory like this:

new KubernetesClientBuilder().withConfig(config)
        .withHttpClientFactory(factory)
        .withTaskExecutor(executorService).build().adapt(OpenShiftClient.class);

when we execute parallel calls, for example, this.openShiftClient.deploymentConfigs().inNamespace(namespace) in many namespaces, every request returns 403 first, then the 2 authorization requests are executed, and then the original request is repeated successfully. This generates a large number of unnecessary requests.

Describe the solution you'd like

I think the problem arises because the AtomicReference with the token inside OpenShiftOAuthInterceptor is not shared between threads, perhaps because the future creation is not using the executor service provided in the client creation.

Describe alternatives you've considered

No response

Additional context

It occurs from the 6.x.x version that added the async requests.

@shawkins
Copy link
Contributor

There are three sharing issues that need to be addressed here. The first is that the change to allow for the RequestConfig to be applied per request is creating different instances of the OpenShiftOAuthInterceptor - which do not share that same oauthToken reference. That is easy to address.

The next issue is that using the adapt call multiple times will create different OpenShiftOAuthInterceptors - that doesn't affect your usage, but would be great to address. I'll definitely address this as part of the work to eliminate the need for the openshift-client jar.

The third is that there is no synchronization between concurrent requests - each will attempt to authorize. The regular TokenRefreshInterceptor has the same behavior. This will be more nuanced to address as ideally we don't want to block the other threads.

@ismadirolas
Copy link
Author

Thanks for the answer,
I think one big step could be if you send 1000 requests on a pool of 20, maybe the first 20 request attempt to authenticate, but when one of them obtains and saves a token, then the other 980 should use it.

@ismadirolas
Copy link
Author

ismadirolas commented Dec 16, 2022

hello @shawkins , thanks for the quick answer and release, now the the token is shared, but I see that the if condition here:

@Override
  public void before(BasicBuilder builder, HttpHeaders headers) {
    String token = oauthToken.get();
    // avoid overwriting basic auth token with stale bearer token
    if (Utils.isNotNullOrEmpty(token)
        && (headers.headers(AUTHORIZATION).isEmpty() || Utils.isNullOrEmpty(headers.headers(AUTHORIZATION).get(0)))) {
      setAuthHeader(builder, token);
    }
  }

never pass throw because the first request has Basic authorization, so the result is the same, first 403, then authorization requests and then 200 with the correct header.
We configure username and password at the client creation because I think it is necessary for the authorize request made from OpenShiftOAuthInterceptor. There is a way to configure client to not send Basic authorization header for openshift?

Thanks for the help.

@shawkins
Copy link
Contributor

There is a way to configure client to not send Basic authorization header for openshift?

@ismadirolas not that I know of - this is how the logic has been for quite some time. If you want to propose a change / enhancement, please turn the above comment into a new issue.

@ismadirolas
Copy link
Author

ismadirolas commented Dec 19, 2022

Hi @shawkins , I've created a new issue #4698

@shawkins
Copy link
Contributor

@ismadirolas thank you. I'll split off one more, then we'll be able to close this one with pr that was already committed.

@shawkins
Copy link
Contributor

Marking the simple issue of sharing the token resolved in 6.3.1 - there are 3 follow up issues to be addressed.

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

No branches or pull requests

3 participants