Skip to content

Commit

Permalink
change dir permissions
Browse files Browse the repository at this point in the history
  • Loading branch information
pweil- committed Jan 17, 2017
1 parent 17f8d82 commit 16f29eb
Show file tree
Hide file tree
Showing 6 changed files with 71 additions and 12 deletions.
56 changes: 55 additions & 1 deletion pkg/kubelet/dockertools/docker_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ import (
utilstrings "k8s.io/kubernetes/pkg/util/strings"
"k8s.io/kubernetes/pkg/util/term"
utilversion "k8s.io/kubernetes/pkg/util/version"
"k8s.io/kubernetes/pkg/volume"
)

const (
Expand Down Expand Up @@ -793,7 +794,11 @@ func (dm *DockerManager) runContainer(
supplementalGids := dm.runtimeHelper.GetExtraSupplementalGroupsForPod(pod)
securityContextProvider := securitycontext.NewSimpleSecurityContextProvider()
securityContextProvider.ModifyContainerConfig(pod, container, dockerOpts.Config)
securityContextProvider.ModifyHostConfig(pod, container, dockerOpts.HostConfig, supplementalGids)
isRemap, remapErr := dm.isRemapEnvironment()
if remapErr != nil {
glog.Warningf("error determining remap environment: %v", remapErr)
}
securityContextProvider.ModifyHostConfig(pod, container, dockerOpts.HostConfig, supplementalGids, isRemap)
createResp, err := dm.client.CreateContainer(dockerOpts)
if err != nil {
dm.recorder.Eventf(ref, v1.EventTypeWarning, events.FailedToCreateContainer, "Failed to create docker container %q of pod %q with error: %v", container.Name, format.Pod(pod), err)
Expand Down Expand Up @@ -2307,6 +2312,11 @@ func (dm *DockerManager) SyncPod(pod *v1.Pod, _ v1.PodStatus, podStatus *kubecon
return
}

// ensure containers will have access to the right directories before proceeding
if err := dm.ensureDirectoryAccess(pod); err != nil {
utilruntime.HandleError(fmt.Errorf("error changing permissions: %v", err))
}

// Start regular containers
for idx := range containerChanges.ContainersToStart {
container := &pod.Spec.Containers[idx]
Expand All @@ -2333,6 +2343,50 @@ func (dm *DockerManager) SyncPod(pod *v1.Pod, _ v1.PodStatus, podStatus *kubecon
return
}

func (dm *DockerManager) ensureDirectoryAccess(pod *v1.Pod) error {
remap, err := dm.isRemapEnvironment()
if err != nil {
glog.Errorf("unable to determine if in remapping environment: %v", err)
return nil
}
if !remap {
return nil
}

if pod.Spec.SecurityContext == nil {
glog.Errorf("found remapping environment but pod has nil security context, unable to ensure directory ownership")
return nil
}
if pod.Spec.SecurityContext.FSGroup == nil {
glog.Errorf("found remapping environment but pod has nil FSGroup, unable to ensure directory ownership")
return nil

This comment has been minimized.

Copy link
@php-coder

php-coder Jan 18, 2017

Why these 2 conditions return nil instead of an error? If we're in remapping environment and we didn't invoke SetOwnershipForPath(), we'll fail later in attempt to mount /dev/termination-log (because it's created inside /var/lib/kubelet/pods/{id}/containers/volumes-inspector but user doesn't have access to read the content of /var/lib/kubelet/pods/{id}). Let's fail fast.

This comment has been minimized.

Copy link
@pweil-

pweil- Jan 18, 2017

Author Owner

they could return. I was considering these to be 'nothing to do' situations. Doesn't mean we'll necessarily fail - the subgid could be the root group and this would work.

This comment has been minimized.

Copy link
@php-coder

php-coder Jan 18, 2017

they could return

What did you mean?

Doesn't mean we'll necessarily fail - the subgid could be the root group and this would work.

Yes, but it looks like a rare situation, isn't it? I believe that in most of other cases user will face ugly errors like this when using emptyDir:

container_linux.go:247: starting container process caused "process_linux.go:359: container init caused "rootfs_linux.go:53: mounting \"/var/lib/kubelet/pods/b86e33fc-ddaa-11e6-8b91-080027242396/volumes/kubernetes.ioempty-dir/empty\" to rootfs \"/var/lib/docker/427680.427680/devicemapper/mnt/ff184d8a83901bd3a3974a30b1a245e80bbb19649cc167de11954c1a631046bc/rootfs\" at \"/tmp/empty\" caused \"stat /var/lib/kubelet/pods/b86e33fc-ddaa-11e6-8b91-080027242396/volumes/kubernetes.ioempty-dir/empty: permission denied\"""

and this for pod without volumes:

container_linux.go:247: starting container process caused "process_linux.go:359: container init caused "rootfs_linux.go:53: mounting \"/var/lib/kubelet/pods/32eb95d0-ddac-11e6-8b91-080027242396/containers/volumes-inspector/ec664322\" to rootfs \"/var/lib/docker/427680.427680/devicemapper/mnt/54b36f4c1d1e625c8efce731cf7dbbe5d200632aca06268ff22e24313377d759/rootfs\" at \"/dev/termination-log\" caused \"stat /var/lib/kubelet/pods/32eb95d0-ddac-11e6-8b91-080027242396/containers/volumes-inspector/ec664322: permission denied\"""

The only way to understand what happened is to read the kubelet.log and find the warning here (and it also a bit hard because there are many lines with non-useful messages). In other words I think that this approach is not user friendly :(

This comment has been minimized.

Copy link
@pweil-

pweil- Jan 18, 2017

Author Owner

What did you mean?

I mean we can just return the error or log as a warning.

Yes, but it looks like a rare situation, isn't it? I believe that in most of other cases user will face ugly errors like this when using emptyDir

Yes, this is unlikely to be successful. I have no argument over returning the errors, I just didn't in the prototype. .

}

glog.V(5).Infof("ensuring directory ownership for pod %s is set to FSGroup %d", pod.UID, *pod.Spec.SecurityContext.FSGroup)
return volume.SetOwnershipForPath(dm.runtimeHelper.GetPodDir(pod.UID), false, int(*pod.Spec.SecurityContext.FSGroup))
}

func (dm *DockerManager) isRemapEnvironment() (bool, error){
// docker 1.13+ allows us to see if it is userns remapping from the security options
info, err := dm.client.Info()
if err != nil {
return false, err
}
for _, opt := range info.SecurityOptions {
if strings.Contains(opt, "userns") {
return true, nil
}
}

// if not in the options let's have a fallback flag
if true {
return true, nil
}

// not remapping
return false, nil
}

// tryContainerStart attempts to pull and start the container, returning an error and a reason string if the start
// was not successful.
func (dm *DockerManager) tryContainerStart(container *v1.Container, pod *v1.Pod, podStatus *kubecontainer.PodStatus, pullSecrets []v1.Secret, namespaceMode, pidMode, podIP string) (err error, reason string) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/kubelet/kubelet.go
Original file line number Diff line number Diff line change
Expand Up @@ -1107,10 +1107,10 @@ type Kubelet struct {
// 3. the plugins directory
func (kl *Kubelet) setupDataDirs() error {
kl.rootDirectory = path.Clean(kl.rootDirectory)
if err := os.MkdirAll(kl.getRootDir(), 0750); err != nil {
if err := os.MkdirAll(kl.getRootDir(), 0755); err != nil {
return fmt.Errorf("error creating root directory: %v", err)
}
if err := os.MkdirAll(kl.getPodsDir(), 0750); err != nil {
if err := os.MkdirAll(kl.getPodsDir(), 0755); err != nil {
return fmt.Errorf("error creating pods directory: %v", err)
}
if err := os.MkdirAll(kl.getPluginsDir(), 0750); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/securitycontext/fake.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ type FakeSecurityContextProvider struct{}

func (p FakeSecurityContextProvider) ModifyContainerConfig(pod *v1.Pod, container *v1.Container, config *dockercontainer.Config) {
}
func (p FakeSecurityContextProvider) ModifyHostConfig(pod *v1.Pod, container *v1.Container, hostConfig *dockercontainer.HostConfig, supplementalGids []int64) {
func (p FakeSecurityContextProvider) ModifyHostConfig(pod *v1.Pod, container *v1.Container, hostConfig *dockercontainer.HostConfig, supplementalGids []int64, isRemap bool) {
}

// ValidInternalSecurityContextWithContainerDefaults creates a valid security context provider based on
Expand Down
6 changes: 4 additions & 2 deletions pkg/securitycontext/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (p SimpleSecurityContextProvider) ModifyContainerConfig(pod *v1.Pod, contai
// ModifyHostConfig is called before the Docker runContainer call. The
// security context provider can make changes to the HostConfig, affecting
// security options, whether the container is privileged, volume binds, etc.
func (p SimpleSecurityContextProvider) ModifyHostConfig(pod *v1.Pod, container *v1.Container, hostConfig *dockercontainer.HostConfig, supplementalGids []int64) {
func (p SimpleSecurityContextProvider) ModifyHostConfig(pod *v1.Pod, container *v1.Container, hostConfig *dockercontainer.HostConfig, supplementalGids []int64, isRemap bool) {
// Apply supplemental groups
if container.Name != leaky.PodInfraContainerName {
// TODO: We skip application of supplemental groups to the
Expand All @@ -65,7 +65,9 @@ func (p SimpleSecurityContextProvider) ModifyHostConfig(pod *v1.Pod, container *
for _, group := range pod.Spec.SecurityContext.SupplementalGroups {
hostConfig.GroupAdd = append(hostConfig.GroupAdd, strconv.Itoa(int(group)))
}
if pod.Spec.SecurityContext.FSGroup != nil {
// only add the FSGroup to the config if we are not in a remap environment. For remap environments
// is is assumed that the root user is remapped to the FSGroup on the host.
if pod.Spec.SecurityContext.FSGroup != nil && !isRemap {
hostConfig.GroupAdd = append(hostConfig.GroupAdd, strconv.Itoa(int(*pod.Spec.SecurityContext.FSGroup)))
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/securitycontext/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ type SecurityContextProvider interface {
// - pod: the pod to modify the docker hostconfig for
// - container: the container to modify the hostconfig for
// - supplementalGids: additional supplemental GIDs associated with the pod's volumes
ModifyHostConfig(pod *v1.Pod, container *v1.Container, hostConfig *dockercontainer.HostConfig, supplementalGids []int64)
ModifyHostConfig(pod *v1.Pod, container *v1.Container, hostConfig *dockercontainer.HostConfig, supplementalGids []int64, isRemap bool)
}

const (
Expand Down
13 changes: 8 additions & 5 deletions pkg/volume/volume_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,17 @@ const (
// fsGroup, and sets SetGid so that newly created files are owned by
// fsGroup. If fsGroup is nil nothing is done.
func SetVolumeOwnership(mounter Mounter, fsGroup *int64) error {

if fsGroup == nil {
return nil
}
return SetOwnershipForPath(mounter.GetPath(), mounter.GetAttributes().ReadOnly, int(*fsGroup))

}

func SetOwnershipForPath(path string, readOnly bool, gid int) error {
chownRunner := chown.New()
chmodRunner := chmod.New()
return filepath.Walk(mounter.GetPath(), func(path string, info os.FileInfo, err error) error {
return filepath.Walk(path, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
Expand All @@ -72,13 +75,13 @@ func SetVolumeOwnership(mounter Mounter, fsGroup *int64) error {
return nil
}

err = chownRunner.Chown(path, int(stat.Uid), int(*fsGroup))
err = chownRunner.Chown(path, int(stat.Uid), gid)
if err != nil {
glog.Errorf("Chown failed on %v: %v", path, err)
}

mask := rwMask
if mounter.GetAttributes().ReadOnly {
if readOnly {
mask = roMask
}

Expand All @@ -93,4 +96,4 @@ func SetVolumeOwnership(mounter Mounter, fsGroup *int64) error {

return nil
})
}
}

0 comments on commit 16f29eb

Please sign in to comment.