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

[Vlatombe]

Describe the bug

Relates to #4519

This is essentially the same problem but using a fixed token instead of a kubeconfig file when building the initial configuration.

Fabric8 Kubernetes Client version

6.3.1

Steps to reproduce

Instantiate the client using

KubernetesClient client = new KubernetesClientBuilder().withConfig(new ConfigBuilder().withMasterUrl("https://url-to-my-kubernetes-master").withCaCertData("my-ca-cert-data").withOauthToken("my-token").build()).build();

and also have a file at /var/run/secrets/kubernetes.io/serviceaccount/token with some different token.

After 1 minute, calls start failing with Unauthorized errors.

Expected behavior

Calls don't fail after 1 minute

Runtime

Kubernetes (vanilla)

Kubernetes API Server version

1.23

Environment

Amazon

Fabric8 Kubernetes Client Logs

No response

Additional context

No response

[shawkins]

Yes this is some problematic behavior of the refresh logic that was noticed in #4702 as well. As you note this will happen only if you specify the token and have another token that can be picked up by the refresh logic - we're not currently tracking if the token and other fields were set from the auto configure or after the fact by the user.

As a workaround I think you can prevent this from happening if you call config.setAutoConfigure(false) on the config you pass to the builder.

This does not affect the openshift logic with #4702 as it will only try to refresh on a authentication failure, however the base token refresh interceptor will attempt to proactively refresh - which is causing this issue. There is currently no way to adjust that 1 minute interval.

[Vlatombe]

I found new ConfigBuilder(Config.empty()); to be effective to prevent even the initial autoconfiguration.

[shawkins]

To recap - we will currently bypass refresh if autoconfigure is false and there is no associated kubeconfig file. If there isn't a kubeconfig file that is found which provides a competing token, you can either start with an empty / non-auto configured builder, or use withAutoConfigure

new ConfigBuilder().withMasterUrl("https://url-to-my-kubernetes-master").withCaCertData("my-ca-cert-data").withOauthToken("my-token").withAutoConfigure(false).build()

@manusa @rohanKanojia @Vlatombe The broader question is over expected behavior - if you call withOauthToken, or withOauthTokenProvider, should we track that so that the config is not proactively refreshed? If the token does expire, then should we try the more typical refresh mechanisms?

[Vlatombe]

I would expect that a Configuration built using withOauthToken in the ConfigBuilder doesn't attempt to refresh it. Since it was provided by the caller on creation, it is expected either to be permanently valid, or up to the caller to refresh it by its own means.
Dunno about withOauthTokenProvider

[shawkins]

Another thing that will work is using:

...withConfig(new ConfigBuilder().withOauthTokenProvider(() -> "token")...

Even if the logic then finds another oauth token, it won't actually be used. I don't know if it's documented anywhere, but the token provider trumps setting the oauth token.

I would expect that a Configuration built using withOauthToken in the ConfigBuilder doesn't attempt to refresh it.

Both the token refresh interceptor and the openshift logic seemed to assume that regardless of how that token was set that it would still be refreshed. I'm honestly not sure if that is a mistake or if anyone is relying on that behavior.

We can do one or both of theses:

1. document that users should specify withOauthTokenProvider(() -> "token") to indicate a static token.
2. treat withOauthToken the same as 1 - but it's not entirely clear if this is everyone's expectation.

We should also:

1. not refresh the config in the way we currently are if an oauth token provider is set, as it's unnecessary.
2. If we don't do 2 above, then document what would trigger the usage of different token

[shawkins]

The Config logic is very confusing at this point wrt oauth tokens. We have ~5 token sources:

1. OAuthTokenProvider (highest priority)
2. env property / system variable (for several paths this takes precedence as it's applied last), will also be refreshed if autoConfigure=true
3. kubeconfig file (tried first with autoConfigure, or can be provided via a fromKubeconfig method), will be refreshed
   • the token can be from the authproviderconfig access token or id
   • or from the authproviderconfig execconfig
4. service account token file (tried second with autoConfigure - if no kubeconfig is found), will be refreshed if autoConfigure=true
5. kubeconfig provided as contents in the fromKubeconfig method without a file. This won't directly refresh - the logic currently doesn't refresh the env properties / system variables with this path either.

As mentioned in this issue, directly setting the oauthtoken on the config does not inhibit the refresh logic for 2-5. 3 and 5 also can provide the authproviderconfig to refresh the token from server - with the additional consideration that openshift token refresh is different (based just on username and password).

I'm not confident that anyone using anything other than a default autoconfigured config would know / understand the interplay of options.

[shawkins]

We may want to expand this issue to a few additional scenarios. Looking at kubectl explicit behavior, there are other differences with our handling:

1. using --username --password to specify basic auth. Instead our config logic will also pickup the username from a env/sys property or from the AuthInfo associated with the context/user. That seems to muddle whether the user actually wants to require the use of basic auth.
2. using --token to specify a fixed token. Config.setOAuthToken is not equivalent as it does not force the specified token to be used.
3. kubectl set-credentials - we don't have an explicit mechanism. Some of the RequestConfig logic must have been geared toward scenarios like this, but did not directly manipulate the kubeconfig. We're also looking at removing the ability to set username, password, oauthtoken, and oauthtokenprovider in the RequestConfig moving forward.

Other ill-defined behavior:

1. It's not clear with the fromKubeconfig method whether env / system properties should be consulted. I did add that recently, but it's not happening on a refresh when you don't specify a kubeconfig file path nor if the disableAutoConfig returns true - which in the autoconfig case we only consult on the first load, and not subsequently.

[rohanKanojia]

@shawkins : Do you think it makes sense to give user the option to control whether token should be refreshed or not? Interested users would just use new ConfigBuilder().withOauthToken("my-token").withRefreshEnabled(false).build() to disable token refresh

[shawkins]

@rohanKanojia I'm not sure what the right approach is given how overloaded the Config is wrt authentication handling. It may not be clear to users when they should opt out of refresh.

Ideally we'd stop internally modifying the oauthtoken field (that would match the user expectation here) - if the user has set a value, it should remain that. That means how we are looking up the token in a refreshable way from the ENV, service account file, AuthInfo, AuthProvider, or ExecCredential should be done elsewhere - in particular as part of the refresh logic and not as part of the Config load. It's also not quite clear what the relationship is of this refresh to the client/server refreshes performed by the TokenRefreshInterceptor and the OpenShiftOAuthInterceptor. Based upon https://github.com/kubernetes/client-go/blob/master/plugin/pkg/client/auth/oidc/oidc.go and and a similar bit of code that was in fabric8 (but invalid for openshift) it appears that OIDC refresh is only used as a last resort if nothing else has set a token and it considers the expiration - not a proactive 1 minute refresh.

There's also the OAuthTokenProvider to consider as it trumps all of this - at the very least it probably doesn't belong on the Config now that we have the KuberentesClientBuilder. It also seems like we should be able to encapsulate some of our refresh logic via this, but that looks like a very forced fit currently.