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

Use fsGroup instead of initContainer for setting appropriate file owners. #803

Open
AleksandarSavchev opened this issue May 27, 2024 · 5 comments
Assignees
Labels
area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/enhancement Enhancement, improvement, extension

Comments

@AleksandarSavchev
Copy link
Member

How to categorize this issue?

/area quality
/kind enhancement

What would you like to be added:
Currently etcd uses initContainers to change file owners:

sts.Spec.Template.Spec.InitContainers = []corev1.Container{
{
Name: "change-permissions",
Image: c.values.InitContainerImage,
ImagePullPolicy: corev1.PullIfNotPresent,
Command: []string{"sh", "-c", "--"},
Args: []string{"chown -R 65532:65532 /var/etcd/data"},
VolumeMounts: getEtcdVolumeMounts(c.values),
SecurityContext: &corev1.SecurityContext{
RunAsGroup: pointer.Int64(0),
RunAsNonRoot: pointer.Bool(false),
RunAsUser: pointer.Int64(0),
},
},
}
if c.values.BackupStore != nil {
// Special container to change permissions of backup bucket folder to 65532 (nonroot)
// Only used with local provider
prov, _ := utils.StorageProviderFromInfraProvider(c.values.BackupStore.Provider)
if prov == utils.Local {
sts.Spec.Template.Spec.InitContainers = append(sts.Spec.Template.Spec.InitContainers, corev1.Container{
Name: "change-backup-bucket-permissions",
Image: c.values.InitContainerImage,
ImagePullPolicy: corev1.PullIfNotPresent,
Command: []string{"sh", "-c", "--"},
Args: []string{fmt.Sprintf("chown -R 65532:65532 /home/nonroot/%s", *c.values.BackupStore.Container)},
VolumeMounts: getBackupRestoreVolumeMounts(c),
SecurityContext: &corev1.SecurityContext{
RunAsGroup: pointer.Int64(0),
RunAsNonRoot: pointer.Bool(false),
RunAsUser: pointer.Int64(0),
},
})
}
}

I would like these initContainers to be removed and their functionality replaced with the use of fsGroup: 65532 added here:
sts.Spec.Template.Spec.SecurityContext = &corev1.PodSecurityContext{
RunAsGroup: pointer.Int64(65532),
RunAsNonRoot: pointer.Bool(true),
RunAsUser: pointer.Int64(65532),
}

This will set group owner 65532 for mounted files and would make these files accessible for the etcd pod.

Why is this needed:
Remove unnecessary etcd containers and avoid changing owners on PV

@gardener-robot gardener-robot added area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/enhancement Enhancement, improvement, extension labels May 27, 2024
@renormalize
Copy link
Member

@unmarshall @shreyas-s-rao should we do this in #777 or should we avoid adding more changes to it?

@renormalize
Copy link
Member

I've tested this out with AWS, and fsGroup is sufficient to eliminate the use of init containers as mentioned in the issue description with older volumes which run with etcd-custom-image in root:root.

Testing has to be performed with all providers to ensure that fsGroup is implemented by all CSI drivers. All access modes aren't supported for OpenStack, as written in kubernetes/cloud-provider-openstack#2075 (comment).

I'm currently unsure if fsGroup would work with the local provider.

@shreyas-s-rao
Copy link
Contributor

@unmarshall @aaronfern I remember that the init container change-permissions was mainly meant to migrate from etcd-custom-image to etcd-wrapper, so that wrapper being a distroless image, can be allowed to own the etcd data directory that was created and managed by etcd-custom-image which ran on a non-distroless image container as root. Is my understanding correct?

Now that we no longer support etcd-custom-image, can we get rid of this init container? Will we still require the fsGroup to be added, possibly for local provider where the backup bucket (directory) is created by a different provisioner, but needs to be owned by etcd-backup-restore before starting operations on it?

@renormalize would you like to work on this issue?

@unmarshall
Copy link
Contributor

Yes we can remove the init container now. This is no longer required.

@renormalize
Copy link
Member

@shreyas-s-rao sure I'll take it up next once I'm done with the current issue I am working on.

@renormalize renormalize self-assigned this Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/enhancement Enhancement, improvement, extension
Projects
None yet
Development

No branches or pull requests

5 participants