changing permission for kubeconfig file inside the shell pod #166

diogoasouza · 2024-03-04T23:35:18Z

This PR changes the permission of the kubeconfig file used in the shell container used by the impersonator. It was using 0644 which was causing warnings when using helm. This changes the permission to 0600.

Explanation:
Kubernetes always mounts configMaps and Secret volumes as readOnly, regardless of the flag passed when creating the mount. This is not reflected in the documentation of the readOnly flag. To work around that, I'm using an init-container to copy the content to an emptyDir volume, changing the permission and using that volume in the shell container

pkg/podimpersonation/podimpersonation.go

rohitsakala · 2024-03-05T04:55:51Z

pkg/podimpersonation/podimpersonation.go

+			},
+		},
+		v1.Volume{
+			Name: "user-kubeconfig-map",


nit: user-kube-configmap

rohitsakala · 2024-03-05T05:02:27Z

pkg/podimpersonation/podimpersonation.go

 			pod.Spec.Containers[i].VolumeMounts = append(container.VolumeMounts, v1.VolumeMount{
 				Name:      "user-kubeconfig",
-				ReadOnly:  true,
 				MountPath: envvar.Value,
 				SubPath:   "config",


Do we need this subpath?

We only use what's inside the config key of the ConfigMap. We don't need it, but I think it's good to specify a SubPath if we know exactly what we need

@diogoasouza this is not a configmap. This is only emptyDir volume.

@rohitsakala Yes, sorry if I wasn't clear. I meant that we only copy the config from the configMap to this volume and that's why we only need to mount this path to the container

pkg/podimpersonation/podimpersonation.go

diogoasouza · 2024-03-07T23:26:18Z

Can I get a review from someone on the frameworks team?
@rancher/rancher-squad-frameworks

rohitsakala · 2024-03-08T05:44:02Z

pkg/podimpersonation/podimpersonation.go

+
+			pod.Spec.InitContainers = []v1.Container{
+				{
+					Name:         "init-container",


Can we rename to something descriptive ? instead of init-container

rohitsakala · 2024-03-08T08:21:13Z

pkg/podimpersonation/podimpersonation.go

+			}
+
+			pod.Spec.InitContainers = []v1.Container{
+				{


Due to the new init-container when I click on the Kubectl shell button on Rancher Manager, it is taking 18 seconds to load the shell.

Before the init-container it takes only 4 seconds to load the shell.

This is a serious degradation of user experience.

Maybe due to this, I find the sed solution better, if we can't find a way to mitigate this user experience issue.

It was the imagePullPolicy that was causing this. The default value is Always and I changed it to PullIfNotPresent. Should be fine now

tomleb · 2024-03-08T13:46:10Z

Can you link a GH issue associated with this change?

I'm also not very familiar with this area but I'll do my best at reviewing this.

From what I understand, we want to mount some kubeconfig that's stored on a ConfigMap and we want it to have 0600 privileges to avoid a warning from the helm binary.

Is it not possible to mount this file with the defaultMode set to 0600? I see you're using it for the one in the init container, but why not in the container itself directly?

Here's an example (taken and modified from the upstream k8s docs):

---
apiVersion: v1
kind: ConfigMap
metadata:
  name: game-config
  namespace: default
data:
  game.properties: |
    enemies=aliens
    lives=3
    enemies.cheat=true
    enemies.cheat.level=noGoodRotten
    secret.code.passphrase=UUDDLRLRBABAS
    secret.code.allowed=true
    secret.code.lives=30    
  ui.properties: |
    color.good=purple
    color.bad=yellow
    allow.textmode=true
    how.nice.to.look=fairlyNice    
---
apiVersion: v1
kind: Pod
metadata:
  name: dapi-test-pod
spec:
  containers:
    - name: test-container
      image: registry.k8s.io/busybox
      command: [ "/bin/sh", "-c", "ls -l /etc/config/" ]
      volumeMounts:
      - name: game-config
        subPath: game.properties
        mountPath: /etc/config/game.properties
  volumes:
    - name: game-config
      configMap:
        name: game-config
        defaultMode: 0600
  restartPolicy: Never

Looking at the logs of the pod:

$ kubectl logs dapi-test-pod
total 4
-rw-------    1 root     root           162 Mar  8 13:12 game.properties

The mode bits are as expected.

diogoasouza · 2024-03-08T19:03:03Z

@tomleb Hi Tom, thanks for the review.
Regarding that, it's true that we can change the defaultMode, the problem is that the user that runs inside the shell container is not the root user. Setting the runAsUser in the container does not change the ownership of the files inside the mounted volume as well

pkg/podimpersonation/podimpersonation.go

tomleb

One nit but otherwise LGTM.

Thanks for adding some tests.

tomleb · 2024-03-13T13:40:04Z

pkg/podimpersonation/podimpersonation.go

 	for i, container := range pod.Spec.Containers {
 		for _, envvar := range container.Env {
 			if envvar.Name != "KUBECONFIG" {
 				continue
 			}

+			vmount := v1.VolumeMount{


nit: Could add a comment to explain why we're doing this since it's not obvious at first

tomleb · 2024-03-18T17:03:36Z

Needs one more approval from frameworks folks.

Also this PR will need someone from your team to QA it 👍

diogoasouza · 2024-03-18T17:10:56Z

Needs one more approval from frameworks folks.

Also this PR will need someone from your team to QA it 👍

Sure! I'll put it in our QA backlog after it's merged

JonCrowther

Everything looks good. Just found a minor typo

JonCrowther · 2024-03-27T14:07:50Z

pkg/podimpersonation/podimpersonation_test.go

+
+			assert.Len(t, pod.Spec.Volumes, len(p.Spec.Volumes)+4, "expected four new volumes")
+			if len(tc.envVars) != 0 {
+				assert.Len(t, pod.Spec.Containers[0].VolumeMounts, len(p.Spec.Containers[0].VolumeMounts)+1, "expeted kubeconfig volume to be mounted")


Suggested change

assert.Len(t, pod.Spec.Containers[0].VolumeMounts, len(p.Spec.Containers[0].VolumeMounts)+1, "expeted kubeconfig volume to be mounted")

assert.Len(t, pod.Spec.Containers[0].VolumeMounts, len(p.Spec.Containers[0].VolumeMounts)+1, "expected kubeconfig volume to be mounted")

diogoasouza · 2024-04-03T17:18:26Z

I think it has the required approvals, can you merge it @tomleb ?

rohitsakala reviewed Mar 5, 2024

View reviewed changes

pkg/podimpersonation/podimpersonation.go Outdated Show resolved Hide resolved

rohitsakala reviewed Mar 5, 2024

View reviewed changes

diogoasouza force-pushed the change-kubeconfig-permission branch from 1b210e0 to ecbe3b5 Compare March 5, 2024 21:06

diogoasouza marked this pull request as ready for review March 5, 2024 21:06

diogoasouza mentioned this pull request Mar 5, 2024

filtering warning message about kubeconfig file perm mode rancher/shell#220

Closed

pjbgf reviewed Mar 7, 2024

View reviewed changes

pkg/podimpersonation/podimpersonation.go Outdated Show resolved Hide resolved

diogoasouza force-pushed the change-kubeconfig-permission branch from ecbe3b5 to 663d58d Compare March 7, 2024 19:01

rohitsakala reviewed Mar 8, 2024

View reviewed changes

diogoasouza force-pushed the change-kubeconfig-permission branch from 663d58d to 28fdee2 Compare March 8, 2024 19:55

tomleb reviewed Mar 8, 2024

View reviewed changes

pkg/podimpersonation/podimpersonation.go Outdated Show resolved Hide resolved

diogoasouza force-pushed the change-kubeconfig-permission branch 2 times, most recently from 902ba4d to dcea050 Compare March 11, 2024 19:34

rohitsakala approved these changes Mar 12, 2024

View reviewed changes

tomleb reviewed Mar 12, 2024

View reviewed changes

pkg/podimpersonation/podimpersonation.go Outdated Show resolved Hide resolved

diogoasouza force-pushed the change-kubeconfig-permission branch from dcea050 to 6013cb3 Compare March 12, 2024 16:38

rohitsakala approved these changes Mar 13, 2024

View reviewed changes

tomleb approved these changes Mar 13, 2024

View reviewed changes

diogoasouza force-pushed the change-kubeconfig-permission branch from 272a8b7 to 27f864a Compare March 13, 2024 17:52

tomleb requested a review from a team March 14, 2024 19:59

JonCrowther reviewed Mar 27, 2024

View reviewed changes

diogoasouza force-pushed the change-kubeconfig-permission branch from 27f864a to 355b4e0 Compare March 28, 2024 22:35

changing permission for kubeconfig file inside the shell pod

c287ff0

diogoasouza force-pushed the change-kubeconfig-permission branch from 355b4e0 to 3654549 Compare March 28, 2024 22:45

creating unit tests for the changes done

7eb213c

diogoasouza force-pushed the change-kubeconfig-permission branch from 3654549 to 7eb213c Compare April 1, 2024 16:34

JonCrowther approved these changes Apr 2, 2024

View reviewed changes

tomleb merged commit 46e3638 into rancher:master Apr 3, 2024
1 check passed

diogoasouza mentioned this pull request May 17, 2024

[Feature] K8s 1.29 Support rancher/rancher#43110

Closed

32 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changing permission for kubeconfig file inside the shell pod #166

changing permission for kubeconfig file inside the shell pod #166

diogoasouza commented Mar 4, 2024 •

edited

Loading

rohitsakala Mar 5, 2024 •

edited

Loading

diogoasouza Mar 5, 2024

rohitsakala Mar 5, 2024

diogoasouza Mar 5, 2024

rohitsakala Mar 8, 2024

diogoasouza Mar 11, 2024

diogoasouza commented Mar 7, 2024

rohitsakala Mar 8, 2024

diogoasouza Mar 8, 2024

rohitsakala Mar 8, 2024

diogoasouza Mar 8, 2024

tomleb commented Mar 8, 2024

diogoasouza commented Mar 8, 2024

tomleb left a comment

tomleb Mar 13, 2024

tomleb commented Mar 18, 2024

diogoasouza commented Mar 18, 2024

JonCrowther left a comment

JonCrowther Mar 27, 2024

diogoasouza Mar 28, 2024

diogoasouza commented Apr 3, 2024

	assert.Len(t, pod.Spec.Containers[0].VolumeMounts, len(p.Spec.Containers[0].VolumeMounts)+1, "expeted kubeconfig volume to be mounted")
	assert.Len(t, pod.Spec.Containers[0].VolumeMounts, len(p.Spec.Containers[0].VolumeMounts)+1, "expected kubeconfig volume to be mounted")

changing permission for kubeconfig file inside the shell pod #166

changing permission for kubeconfig file inside the shell pod #166

Conversation

diogoasouza commented Mar 4, 2024 • edited Loading

rohitsakala Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diogoasouza commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomleb commented Mar 8, 2024

diogoasouza commented Mar 8, 2024

tomleb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomleb commented Mar 18, 2024

diogoasouza commented Mar 18, 2024

JonCrowther left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diogoasouza commented Apr 3, 2024

diogoasouza commented Mar 4, 2024 •

edited

Loading

rohitsakala Mar 5, 2024 •

edited

Loading