Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issue-42861#Not using auto generated secret tokens #129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

diogoasouza
Copy link
Contributor

issue-42861
This PR changes the impersonator to use projected volumes instead of log-lived SA tokens. This doesn't cover all possible cases in the rancher codebase, just the ones related to mapps and the UI kubectl shell.

Reference docs:
https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#launch-a-pod-using-service-account-token-projection
https://cloud.google.com/kubernetes-engine/docs/how-to/kubernetes-service-accounts#assigning_a_kubernetes_service_account_to_a_pod

@jiaqiluo jiaqiluo requested review from jiaqiluo and a team and removed request for a team September 21, 2023 19:40
@samjustus samjustus requested a review from a team September 22, 2023 16:20
pod.Spec.ServiceAccountName = sa.Name
pod.Spec.AutomountServiceAccountToken = &f
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider using the AutomountServiceAccountToken feature and let k8s manage the token inject (volumes)?
If I understand correctly, it was disabled because we created the Secret and would like to use one Secret for all pods.
Now, We no longer need to maintain the Secret, so can we leave it to K8s?
WDYT? Discussions are welcomed!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested with that before, and it works, but I'm unsure if it handles rotating the tokens.

Copy link
Member

@jiaqiluo jiaqiluo Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, K8s handles the rotation.

To assign a ServiceAccount to a Pod, you set the spec.serviceAccountName field in the Pod specification. Kubernetes then automatically provides the credentials for that ServiceAccount to the Pod. In v1.22 and later, Kubernetes gets a short-lived, automatically rotating token using the TokenRequest API and mounts the token as a projected volume.

Ref: https://kubernetes.io/docs/concepts/security/service-accounts/#assign-to-pod

I would like to hear more thoughts from the maintainer of the repo @rancher/rancher-team-1-neo-dev

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd agree with @jiaqiluo here, there's a lot of extra code add to project volumes that I would expect K8s to do for us. If there's a reason to manually set this up, happy to consider it, but for now I think we should default to the automounted value recommended by upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree as well, I wasn't aware that kubernetes would rotate the tokens automatically if using that flag, just that it does that when using a projected volume

Copy link
Contributor

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure that I understand why this is necessary. The problem caused in rancher/rancher#42861 is caused by the warning message in k8s here, which only triggers when a token being used is listed on a service account. Near as I can tell, this PR isn't changing any logic that adds the created secret to the service account (and I'm not sure it was ever being added - creating a token like this on 1.24 did not seem to cause k8s to add the secret name to the service account).

In addition, when I run this on a 1.27.5 cluster, I don't see any warning messages in the kubectl shell pods produced by the UI, which I would expect to see if they were using these tokens.

@diogoasouza would you mind clarifying what issue this is fixing and how the original defect works?

MountPath: "/var/run/secrets/kubernetes.io/serviceaccount",
ReadOnly: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on why this was removed? I would expect us to still want this to be read-only

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should, yes. It was my mistake

@diogoasouza
Copy link
Contributor Author

I'm not entirely sure that I understand why this is necessary. The problem caused in rancher/rancher#42861 is caused by the warning message in k8s here, which only triggers when a token being used is listed on a service account. Near as I can tell, this PR isn't changing any logic that adds the created secret to the service account (and I'm not sure it was ever being added - creating a token like this on 1.24 did not seem to cause k8s to add the secret name to the service account).

In addition, when I run this on a 1.27.5 cluster, I don't see any warning messages in the kubectl shell pods produced by the UI, which I would expect to see if they were using these tokens.

@diogoasouza would you mind clarifying what issue this is fixing and how the original defect works?

For you to reproduce it, you have to create a downstream cluster using 1.27 as well as the rancher cluster that is also using 1.27.
Right now, we are creating a secret-based token and mounting a volume with it to the helm operation pod, but not associating the service account itself to the pod, it still uses the default one. By changing the code to associate the service account to the pod and enabling the automatic mount of the token, that secret is not created.

This is from the k8s docs:
The kubelet will: request and store the token on behalf of the Pod; make the token available to the Pod at a configurable file path; and refresh the token as it approaches expiration. The kubelet proactively requests rotation for the token if it is older than 80% of its total time-to-live (TTL), or if the token is older than 24 hours.
https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#launch-a-pod-using-service-account-token-projection

@diogoasouza
Copy link
Contributor Author

diogoasouza commented Sep 29, 2023

@MbolotSuse and I discussed this and came to the conclusion that this is not causing the warning logs, but we shouldn't be using secret-based tokens anymore so it's something we should change. With that in mind, we decided that it's best to do this change for 2.8.1, and thus we should wait for that release before merging this pr

@moio moio requested a review from a team as a code owner July 5, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants