clusterapi: refresh kubeconfig bearer tokens for management and workload kubeconfigs dynamically

[cnmcavoy]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Continuously update the bearer token provided by a kube config file for the clusterapi provider.

Which issue(s) this PR fixes:

Fixes #4784

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[cnmcavoy]

/retest

[k8s-ci-robot]

@cnmcavoy: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cnmcavoy
Once this PR has been reviewed and has the lgtm label, please assign bigdarkclown for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• cluster-autoscaler/OWNERS

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

[elmiko]

thanks for the patch @cnmcavoy , i am a little out of my depth with some of this token wrapping stuff. i wonder if we might bug @stlaz or @liggitt to give us a comment on this?

[elmiko]

i'm still keen to see this get included, @cnmcavoy have you done some local testing of the refresh?

[cnmcavoy]

i'm still keen to see this get included, @cnmcavoy have you done some local testing of the refresh?

Indeed has been using a fork of the CA with this patch included for a month. Initially this PR only covered the kube config in the clusterapi provider package, but that led us to discover there is a third kubeconfig in use for leader election. I updated the PR earlier to address that issue and we haven't discovered anything new since.

[elmiko]

thanks for the update @cnmcavoy , i'm inclined to merge this based on your comments but i will bring up the PR at the CAPI meeting today just to see if we can get an extra review or two. i will plan to merge this on friday if there is no further activity.

[jackfrancis]

/assign

[jackfrancis]

@cnmcavoy code seems sane to me, is there an appropriate slot in the Cluster API provider documentation that we can use to describe this new feature? I'd be happy to build a cluster-autoscaler image from this branch and run through that test flow and report back (which would have the benefit of testing both the functionality and the documentation 😀).

[cnmcavoy]

@jackfrancis sure! I added a note after the various configuration modes between management and workload clusters.

[enj]

Why is this not using something like HTTPWrappersForConfig which invokes NewBearerAuthWithRefreshRoundTripper to handle this flow? You should not be writing any custom code to handle automatic token refresh when client-go already does it for you.

[enj]

Loading the entire kubeconfig but only honoring the bearer token changing doesn't really make sense. The bearer token should be in a distinct file that is reloaded, the rest of the contents should remain static.

[cnmcavoy]

https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler/cloudprovider/clusterapi#autoscaler-running-anywhere-with-separate-kubeconfigs-for-management-and-workload-clusters

Cluster Autoscaler can be configured in a mode where it is running in Cluster "1", but the cluster and pods it needs to scale for and manage are in cluster "3", and the cloud configuration and node groups to scale are in cluster "2". In this scenario, there is no service account bearer token, except for running in Cluster "1" due to these being separate clusters.

The contents of the file we are loading is created by the CAPA awscluster controller, only has a bearer token, and is shaped like this:

apiVersion: v1
clusters:
- cluster:
    certificate-authority-data: LS0t...
    server: https://XXXXX.us-east-2.eks.amazonaws.com
  name: cluster-name
contexts:
- context:
    cluster: cluster-3
    user: cluster-3-capi-admin
  name: cluster-3-capi-admin@cluster-3
current-context: cluster-3-capi-admin@cluster-3
kind: Config
preferences: {}
users:
- name: cluster-3-capi-admin
  user:
    token: k8s-aws-v1...

Are you suggesting that CAPA should encode a second secret value with just the token value contents above, and have the Cluster Autoscaler reload that contents, to simulate the behavior of a file mounted service account bearer token? Or something else?

[enj]

Right if you change the YAML to:

apiVersion: v1
clusters:
- cluster:
    certificate-authority-data: LS0t...
    server: https://XXXXX.us-east-2.eks.amazonaws.com
  name: cluster-name
contexts:
- context:
    cluster: cluster-3
    user: cluster-3-capi-admin
  name: cluster-3-capi-admin@cluster-3
current-context: cluster-3-capi-admin@cluster-3
kind: Config
preferences: {}
users:
- name: cluster-3-capi-admin
  user:
    tokenFile: location_of_file

And have the contents of location_of_file be:

k8s-aws-v1...

Then client-go will handle the contents of location_of_file changing for you.

[enj]

You need to implement utilnet.RoundTripperWrapper

[k8s-ci-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[RichardoC]

Is this still needed given the Kubernetes client-go library has been updated to a version that should fix this?

[elmiko]

@RichardoC that link to the aws docs is only about eks clusters though, is there a link to an issue in client-go that might talk about this?

the capi provider doesn't necessarily use eks, so we would still want a refresh here (as i understand it).

[RichardoC]

This functionality is upstream k8s behaviour, just couldn't find the link earlier https://kubernetes.io/docs/reference/access-authn-authz/service-accounts-admin/#auto-generated-legacy-serviceaccount-token-clean-up
My best guess is client-go was fixed with kubernetes/client-go@66e83da#diff-a6baf0c4cd67228defd6ac03faa4c9922f5cf4dd2b27672509043b33e3d881f7R289 specifically the NewBearerAuthWithRefreshRoundTripper codepath then this shouldn't be needed?

[cnmcavoy]

@RichardoC I think there is some confusion here, this change was about kubeconfig's with an embedded service account token expiring. This differs from the built-in client-go handling of service account tokens, where you are correct, client-go does automatically refresh. So this is still an active issue for CAPI users with EKS clusters.

However, I went to a SIG-APIMachinery meeting last year to discuss better approaches to handling this and they suggested changes downstream, in how the CAPI project manage their kubeconfigs, to avoid this type of issue entirely. So I'm pursuing that over in kubernetes-sigs/cluster-api-provider-aws#4648

If all goes well, there will likely need to be some small autoscaler changes from that, but only for it's helm chart / docs. I'll open a new PR for that work.

[elmiko]

@cnmcavoy thanks for the update and continuing to follow this issue. happy to hear about the status, and thanks for closing this.