Update dashboard's validateToken not to use k8s api server.#4043
Conversation
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
antgamdia
left a comment
antgamdia
left a comment
There was a problem hiding this comment.
Thanks for the changes, +1 anyway, but have a look at the TokenReview comment
| try { | ||
| await Axios.get(url.api.k8s.base(cluster) + "/", { | ||
| headers: { Authorization: `Bearer ${token}` }, | ||
| await this.resourcesClient(token).CheckNamespaceExists({ |
There was a problem hiding this comment.
+1 to this approach; however, I wonder whether we should use an alternative function here. I mean, at first glance, calling CheckNamespaceExists for the validateToken function seems a bit misleading.
Perhaps we could use the can-i operation there or, even better, directly use the k8s object aimed at validating tokens, the TokenReview one (more info):
{
"apiVersion": "authentication.k8s.io/v1",
"kind": "TokenReview",
"spec": {
"token": "014fbff9a07c...",
}
}There was a problem hiding this comment.
Yeah, I'd initially gone along this direction, adding a new endpoint to the resources plugin (didn't think of using the TokenReview - that would have been a good match), but the reason I stopped was that this functionality (for validateToken) is relevant only for the "demo-only" functionality of using service account tokens to authenticate with Kubeapps.
The existing functionality just tries to get the root URL of the k8s API server and I realised that the endpoint is actually arbitrary - the check just needs to see a 200 or 403 - either is fine.
+1 to switch it over to a can-i endpoint, once we have one available in the resources plugin.
| // TODO(absoludity): tried using `expect(fn()).rejects.toThrow()` but it seems we need | ||
| // to upgrade jest for `toThrow()` to work with async. |
There was a problem hiding this comment.
Just wondering if this ToDo still applies after having applied several upgrades to the jest deps.
There was a problem hiding this comment.
Nope, it works with the .toThrow now in an async function. Thanks for the nudge, updated.
| // TODO(minelson): Move to a shared helper for plugins interacting | ||
| // with the k8s cluster. |
There was a problem hiding this comment.
+1, we are using this function also in the flux and carvel plugins
Signed-off-by: Michael Nelson <minelson@vmware.com>
2 participants
Signed-off-by: Michael Nelson minelson@vmware.com
Description of the change
Part one of removing
shared/Auth's use of the k8s API server, this PR updates thevalidateTokencheck, used when Kubeapps is configured with token authentication, so that it hits the resources API (checkNamespace), rather than hitting the k8s API server directly.To do so, I had to correct the error handling in the resources plugin to differentiate between 401s and 403s (and will later follow up as per the TODO there).
I also fixed an issue where the dashboard was checking cookie auth based only on the loginURL being set, which it is by default anyway (which resulted in an extra error being displayed when token login failed).
Benefits
One more action no longer hitting k8s API.
Possible drawbacks
Applicable issues
Additional information