Merge pull request #37009 from sjenning/fix-perms-with-fsgroup

Automatic merge from submit-queue (batch tested with PRs 38294, 37009, 36778, 38130, 37835)

fix permissions when using fsGroup

Currently, when an fsGroup is specified, the permissions of the defaultMode are not respected and all files created by the atomic writer have mode 777.  This is because in `SetVolumeOwnership()` the `filepath.Walk` includes the symlinks created by the atomic writer.  The symlinks have mode 777 when read from `info.Mode()`.  However, when the are chmod'ed later, the chmod applies to the file the symlink points to, not the symlink itself, resulting in the wrong mode for the underlying file.

This PR skips chmod/chown for symlinks in the walk since those operations are carried out on the underlying file which will be included elsewhere in the walk.

xref https://bugzilla.redhat.com/show_bug.cgi?id=1384458

@derekwaynecarr @pmorie

pkg/volume/volume_linux.go

@@ -51,6 +51,17 @@ func SetVolumeOwnership(mounter Mounter, fsGroup *int64) error {
 			return err
 		}
 
+		// chown and chmod pass through to the underlying file for symlinks.
+		// Symlinks have a mode of 777 but this really doesn't mean anything.
+		// The permissions of the underlying file are what matter.
+		// However, if one reads the mode of a symlink then chmods the symlink
+		// with that mode, it changes the mode of the underlying file, overridden
+		// the defaultMode and permissions initialized by the volume plugin, which
+		// is not what we want; thus, we skip chown/chmod for symlinks.
+		if info.Mode()&os.ModeSymlink != 0 {
+			return nil
+		}
+
 		stat, ok := info.Sys().(*syscall.Stat_t)
 		if !ok {
 			return nil

test/e2e/common/configmap.go

@@ -41,6 +41,11 @@ var _ = framework.KubeDescribe("ConfigMap", func() {
 		doConfigMapE2EWithoutMappings(f, 0, 0, &defaultMode)
 	})
 
+	It("should be consumable from pods in volume as non-root with defaultMode and fsGroup set [Feature:FSGroup]", func() {
+		defaultMode := int32(0440) /* setting fsGroup sets mode to at least 440 */
+		doConfigMapE2EWithoutMappings(f, 1000, 1001, &defaultMode)
+	})
+
 	It("should be consumable from pods in volume as non-root [Conformance]", func() {
 		doConfigMapE2EWithoutMappings(f, 1000, 0, nil)
 	})
@@ -343,14 +348,10 @@ func doConfigMapE2EWithoutMappings(f *framework.Framework, uid, fsGroup int64, d
 		defaultMode = &mode
 	}
 
-	// Just check file mode if fsGroup is not set. If fsGroup is set, the
-	// final mode is adjusted and we are not testing that case.
+	modeString := fmt.Sprintf("%v", os.FileMode(*defaultMode))
 	output := []string{
 		"content of file \"/etc/configmap-volume/data-1\": value-1",
-	}
-	if fsGroup == 0 {
-		modeString := fmt.Sprintf("%v", os.FileMode(*defaultMode))
-		output = append(output, "mode of file \"/etc/configmap-volume/data-1\": "+modeString)
+		"mode of file \"/etc/configmap-volume/data-1\": " + modeString,
 	}
 	f.TestContainerOutput("consume configMaps", pod, 0, output)
 }

test/e2e/common/downwardapi_volume.go

@@ -81,6 +81,21 @@ var _ = framework.KubeDescribe("Downward API volume", func() {
 		})
 	})
 
+	It("should provide podname as non-root with fsgroup and defaultMode [Feature:FSGroup]", func() {
+		podName := "metadata-volume-" + string(uuid.NewUUID())
+		uid := int64(1001)
+		gid := int64(1234)
+		mode := int32(0440) /* setting fsGroup sets mode to at least 440 */
+		pod := downwardAPIVolumePodForModeTest(podName, "/etc/podname", &mode, nil)
+		pod.Spec.SecurityContext = &v1.PodSecurityContext{
+			RunAsUser: &uid,
+			FSGroup:   &gid,
+		}
+		f.TestContainerOutput("downward API volume plugin", pod, 0, []string{
+			"mode of file \"/etc/podname\": -r--r-----",
+		})
+	})
+
 	It("should update labels on modification [Conformance]", func() {
 		labels := map[string]string{}
 		labels["key1"] = "value1"

test/e2e/common/secrets.go

@@ -31,12 +31,19 @@ var _ = framework.KubeDescribe("Secrets", func() {
 	f := framework.NewDefaultFramework("secrets")
 
 	It("should be consumable from pods in volume [Conformance]", func() {
-		doSecretE2EWithoutMapping(f, nil /* default mode */, "secret-test-"+string(uuid.NewUUID()))
+		doSecretE2EWithoutMapping(f, nil /* default mode */, "secret-test-"+string(uuid.NewUUID()), nil, nil)
 	})
 
 	It("should be consumable from pods in volume with defaultMode set [Conformance]", func() {
 		defaultMode := int32(0400)
-		doSecretE2EWithoutMapping(f, &defaultMode, "secret-test-"+string(uuid.NewUUID()))
+		doSecretE2EWithoutMapping(f, &defaultMode, "secret-test-"+string(uuid.NewUUID()), nil, nil)
+	})
+
+	It("should be consumable from pods in volume as non-root with defaultMode and fsGroup set [Conformance]", func() {
+		defaultMode := int32(0440) /* setting fsGroup sets mode to at least 440 */
+		fsGroup := int64(1001)
+		uid := int64(1000)
+		doSecretE2EWithoutMapping(f, &defaultMode, "secret-test-"+string(uuid.NewUUID()), &fsGroup, &uid)
 	})
 
 	It("should be consumable from pods in volume with mappings [Conformance]", func() {
@@ -66,7 +73,7 @@ var _ = framework.KubeDescribe("Secrets", func() {
 		if secret2, err = f.ClientSet.Core().Secrets(namespace2.Name).Create(secret2); err != nil {
 			framework.Failf("unable to create test secret %s: %v", secret2.Name, err)
 		}
-		doSecretE2EWithoutMapping(f, nil /* default mode */, secret2.Name)
+		doSecretE2EWithoutMapping(f, nil /* default mode */, secret2.Name, nil, nil)
 	})
 
 	It("should be consumable in multiple volumes in a pod [Conformance]", func() {
@@ -201,7 +208,7 @@ func secretForTest(namespace, name string) *v1.Secret {
 	}
 }
 
-func doSecretE2EWithoutMapping(f *framework.Framework, defaultMode *int32, secretName string) {
+func doSecretE2EWithoutMapping(f *framework.Framework, defaultMode *int32, secretName string, fsGroup *int64, uid *int64) {
 	var (
 		volumeName      = "secret-volume"
 		volumeMountPath = "/etc/secret-volume"
@@ -256,6 +263,13 @@ func doSecretE2EWithoutMapping(f *framework.Framework, defaultMode *int32, secre
 		defaultMode = &mode
 	}
 
+	if fsGroup != nil || uid != nil {
+		pod.Spec.SecurityContext = &v1.PodSecurityContext{
+			FSGroup:   fsGroup,
+			RunAsUser: uid,
+		}
+	}
+
 	modeString := fmt.Sprintf("%v", os.FileMode(*defaultMode))
 	expectedOutput := []string{
 		"content of file \"/etc/secret-volume/data-1\": value-1",