Allow token refresh for GCP #402

macsux · 2020-04-06T19:16:16Z

No description provided.

macsux · 2020-04-06T19:16:57Z

tg123 · 2020-04-06T21:49:55Z

IMHO, it is better to have token refresh via rest api than via cli

macsux · 2020-04-06T22:15:12Z

While I agree, you would need to setup a whole oauth2 authentication mechanism from scratch. The current method is to read token from kubectl config. This works well for local runs when you're using current context of kubectl, and not appropriate for service deployments. To setup standalone auth, you will need to provide clientid, secret and manage token lifecycle manually. I think both should be supported - kubectl for local and rest when you're doing actual development. As a side note, current implementation of local kubectl config is deeply flawed for all providers. It only reads current access token when kubernetes object is constructed. Once that token is expired, the app will stop worrying.

…

On Mon., Apr. 6, 2020, 5:50 p.m. Boshi Lian, ***@***.***> wrote: IMHO, it is better to have token refresh via rest api than via cli — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#402 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAINFWASO635S2CBG6FG5ETRLJFBDANCNFSM4MCQ6NWQ> .

macsux · 2020-04-06T22:29:06Z

As we're supporting reading tokens out of kubectl config, there's limited information available there. There's no way to gain access to clientid/secret/refreshtoken using current approach. Given that it is still desirable to be able to allow apps to authenticate using creds found in current kubectl context, I propose the current authenticator be renamed to something like KubectlGcp, and another one built to support token management via REST for when you want to configure authentication explicitly.

brendandburns · 2020-04-07T19:47:31Z

src/KubernetesClient/KubernetesClientConfiguration.ConfigFile.cs

-                                }
-
-                                AccessToken = config["access-token"];
+                                TokenProvider = new GcpTokenProvider(config["cmd-path"]);


I don't know the expected details of a GCP kubeconfig, but is this field present in all configs?

Are the GCP kubeconfig contents documented anywhere?

I don't believe google "documents" it as it automatically configures context for you, but it is the way they do it. To be safe I can make this behavior apply only when the specified field is present

Yeah, I think I prefer that.

I found where this is coming from and in case of "gcp" it will always be present as it's hardcoded behavior of official go client. https://github.com/kubernetes/client-go/blob/master/plugin/pkg/client/auth/gcp/gcp.go

Imo no reason to do the extra check

Sounds good, can you add a pointer to this "documentation" in a comment

e.g.

``// docs: https://github.com/kubernetes/client-go/blob/master/plugin/pkg/client/auth/gcp/gcp.go#L58`

src/KubernetesClient/Authentication/GcpTokenProvider.cs

brendandburns · 2020-04-07T19:48:25Z

src/KubernetesClient/Authentication/GcpTokenProvider.cs

+
+namespace k8s.Authentication
+{
+    public class GcpTokenProvider : ITokenProvider


This file needs a unit test.

How would you expect the unit test to look? This class is 90% call to an external process which a) doesn't exist in unit test environment
b) requires to be authenticated / configured in order to respond correctly

I can certainly try to create a shell script to mock of what gcloud cli returns, though I question the value of such test. Beyond invoking external command, the rest of this class is just two lines of json querying.

There's sufficient logic (e.g. return code != 0 -> failure, etc) that I think it deserves a test.

There's an equivalent test for the Javascript client here:

https://github.com/kubernetes-client/javascript/blob/master/src/exec_auth_test.ts

brendandburns · 2020-04-07T19:49:29Z

src/KubernetesClient/KubernetesClientConfiguration.cs

@@ -76,5 +77,7 @@ public partial class KubernetesClientConfiguration
        /// </summary>
        /// <value>The access token.</value>
        public string AccessToken { get; set; }


I feel like instead of both having an AccessToken field and a TokenProvider field, we should just switch to TokenProvider everywhere and use a StaticTokenProvider or somesuch for the static token case.

The issue atm is tokens actually expire. This is very noticeable with GCP provider which issues short lived tokens - KubernetesClient stops working after about 10 minutes. After that token is done, you need to interact with kubectl to force it to get a new token, and re-read it by recreating KubernetesClient.

Yeah, I get the problem, what I'm suggesting is that we remove the AccessToken field in favor of a TokenProvider everywhere.

I'm trying to do this for AKS, but the cluster that I just spun up seems to use certs for authentication, as I got client-key-data, client-certificate-data, and token in my config file. The current code seems to be written where there's access_token. Under what scenario would this be so?

I feel like if we're supporting kubectl current context authentication, then we need to support all different types of authentication that kubectl uses, which includes

OIDC - discover endpoints automatically

external CLI to get token (like gcp)

hardcoded / static (aks seems to use static token + cert)

Oauth2 - hardcoded endpoints

Would be good to see a sample of what AWS EKS config looks like, I don't have one.

Seems like this is where all the different built in authenticators are defined
https://github.com/kubernetes/client-go/tree/master/plugin/pkg/client/auth

Based on my research, it seems there are 3 out of the box authentication providers that kubectl uses: gcp, azure & oidc, and each of these would translate to their own unique token provider. They are as listed in the above URL. These act very much like HttpMessageHandlers. The selection of the provider is dependent on

users: user: auth-provider: name: XXX

My implementation for gcp is actually correct. Ideally, we want to have separate implementation for the other two types of providers. I propose that is done in a separate PR.

I won't be able to code for azure plugin because I'm lacking an AKS cluster that is integrated with Azure AD. I can't set this up as you need to be Azure tenant admin to configure this, and I don't have those rights in my org. If someone wants to provide me with AKS cluster that is enabled for Azure AD integration, I would be happy to create that token authenticator. The instructions on how to set this up are here: https://docs.microsoft.com/en-us/azure/aks/azure-ad-integration

brendandburns · 2020-04-13T16:25:02Z

I'm ok with the TokenProvider refactor in a different PR. Please add a unit test for the GCP Token Provider and I think this is ready to merge.

tests/KubernetesClient.Tests/Authentication/GcpTokenProvider.cs

tests/KubernetesClient.Tests/OperatingSystem.cs

brendandburns · 2020-04-22T16:14:52Z

Looks good, but I think there is a stray unneeded test cases in there?

macsux · 2020-04-22T16:16:47Z

I think there is a stray unneeded test cases in there
What do you mean?

brendandburns · 2020-04-22T16:57:19Z

tests/KubernetesClient.Tests/Authentication/GcpTokenProvider.cs doesn't appear to actually be testing anything? There are no asserts in that file, it just creates the token provider.

macsux · 2020-04-22T17:11:25Z

Accidental checked in file - removed

brendandburns · 2020-04-23T18:43:44Z

@macsux I resolved some merge/rebase conflicts, can you double check the code looks right and then this is ready to merge.

Thanks for the patience.

brendandburns · 2020-04-23T18:57:34Z

Looks like there are some formatting fixes needed. Please run dotnet format --exclude src/KubernetesClient/generated

brendandburns · 2020-04-28T22:32:28Z

/lgtm
/approve

k8s-ci-robot · 2020-04-28T22:32:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, macsux

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [brendandburns]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from brendandburns and krabhishek8260 April 6, 2020 19:16

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 6, 2020

k8s-ci-robot assigned brendandburns Apr 6, 2020

brendandburns reviewed Apr 7, 2020

View reviewed changes

src/KubernetesClient/Authentication/GcpTokenProvider.cs Outdated Show resolved Hide resolved

brendandburns reviewed Apr 7, 2020

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 7, 2020

brendandburns mentioned this pull request Apr 20, 2020

Meta: Code review for larger PRs #426

Closed

macsux force-pushed the feature/authenticators branch 2 times, most recently from 5489e8f to 572e0e1 Compare April 20, 2020 18:40

brendandburns reviewed Apr 22, 2020

View reviewed changes

tests/KubernetesClient.Tests/Authentication/GcpTokenProvider.cs Outdated Show resolved Hide resolved

brendandburns reviewed Apr 22, 2020

View reviewed changes

tests/KubernetesClient.Tests/OperatingSystem.cs Show resolved Hide resolved

macsux force-pushed the feature/authenticators branch 2 times, most recently from 53757b8 to 1d1ff1b Compare April 23, 2020 21:05

Allow token refresh for GCP

8a4b7dc

macsux force-pushed the feature/authenticators branch from 1d1ff1b to 8a4b7dc Compare April 26, 2020 22:25

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 28, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2020

k8s-ci-robot merged commit ae9dd04 into kubernetes-client:master Apr 28, 2020

Allow token refresh for GCP #402

Allow token refresh for GCP #402

Uh oh!

Conversation

macsux commented Apr 6, 2020

Uh oh!

macsux commented Apr 6, 2020

Uh oh!

tg123 commented Apr 6, 2020

Uh oh!

macsux commented Apr 6, 2020 via email

Uh oh!

macsux commented Apr 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Apr 13, 2020

Uh oh!

Uh oh!

Uh oh!

brendandburns commented Apr 22, 2020

Uh oh!

macsux commented Apr 22, 2020

Uh oh!

brendandburns commented Apr 22, 2020

Uh oh!

macsux commented Apr 22, 2020

Uh oh!

brendandburns commented Apr 23, 2020

Uh oh!

brendandburns commented Apr 23, 2020

Uh oh!

brendandburns commented Apr 28, 2020

Uh oh!

k8s-ci-robot commented Apr 28, 2020

Uh oh!

Uh oh!