fix #4698: refining authentication mechanisms to be clearer #4702
Conversation
|
@manusa and others. Upon further review the openshift logic probably does need to stay somewhat intertwined. So with this change the openshift logic will try to use the token (which could also be from a tokenprovider). If that fails, it will then see if a username and password are supplied in the config passed into the interceptor. If they are supplied, then we'll authenticate to get a fresh token - if we get a one, it will be persisted in the kubeconfig. If a username and password aren't supplied we'll try to refresh the config (just like the base kubernetes logic) and use the updated token (we don't bother detecting if it hasn't actually changed). Just like the kuberentes token refresh we set the token on the current config when there's been a possible change - this is an alternative to the atomicreference approach - I've modified here so that it's a volatile field for better multi-threaded behavior. This also refactored the kuberentes token refresh such that the config passed into the interceptor is used to determine key details of the kubeconfig - what file it came from, and what the current context is. Without that, we were relying on the static / default logic. What's not to like:
|
shawkins
requested review from
manusa,
oscerd,
rohanKanojia and
sunix
as code owners
|
oc behavior:
After the config is updated, then it is saved here https://github.com/openshift/oc/blob/a4cfdc79461db44d482b315e4e894f32d6c68ae2/pkg/cli/login/login.go#L179 We seem to be reasonable mimicking this behavior. The only additional corner case I can think of is that the existing code was somehow trying to account for the case of using the OpenShiftClient against a non-openshift server that was configured to use basic auth, in which case it was trying to do the basic auth first. However this does not seem to be supported by oc, so it should be acceptable to not do this. Also oc does support a few other auth schemes - https://github.com/openshift/oc/blob/33f8aed30246e726258c51be27c21c60a2b005f4/pkg/helpers/tokencmd/request_token.go#L92 - but that is beyond the scope of this issue. |
|
Test runs not withstanding this should be ready... |
openshift logic now functions more like the kube logic and will persist in the kubeconfig, update the current config, and use the config refresh logic to check for updated tokens.
|
SonarCloud Quality Gate failed. |
Description
Fix #4698
Addresses #4698: this makes auth mechanisms more distinct. It gives full control over setting the auth header to the token interceptors (which could be renamed).
The proposed behavior:
For kubernetes: if username and password is supplied, then only basic auth will be used. If not, then the token logic will be used.
For openshift: there is some confusing overlap between the usage of username / password and setting the token / token provider. It makes sense to me to keep these distinct. So if a username and password are provided, then we'll use that generate a token - after the first request(s) fail, once there has been a successful refresh the token will be reused. If a username / password are not set, then we'll use the token from the config, which may also be provided by a token provider.
All of this would be added to appropriate documentation if it seems good.
There is additional logic to guard against sharing the token with requests where the user has used withRequestConfig to override the username / password. @manusa seemed open to another issue that would deprecate withRequestConfig (either completely or specific fields).
Type of change
test, version modification, documentation, etc.)
Checklist