Merge pull request #106834 from mengjiao-liu/sysctl-allow-slashes

Add support for slash as sysctl separator to Pod securityContext field and to PodSecurityPolicy

pkg/api/pod/util.go

@@ -329,20 +329,6 @@ func usesHugePagesInProjectedEnv(item api.Container) bool {
 	return false
 }
 
-// hasSysctlsWithSlashNames returns true if the sysctl name contains a slash, otherwise it returns false
-func hasSysctlsWithSlashNames(podSpec *api.PodSpec) bool {
-	if podSpec.SecurityContext == nil {
-		return false
-	}
-	securityContext := podSpec.SecurityContext
-	for _, s := range securityContext.Sysctls {
-		if strings.Contains(s.Name, "/") {
-			return true
-		}
-	}
-	return false
-}
-
 func checkContainerUseIndivisibleHugePagesValues(container api.Container) bool {
 	for resourceName, quantity := range container.Resources.Limits {
 		if helper.IsHugePageResourceName(resourceName) {
@@ -434,8 +420,6 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po
 		AllowExpandedDNSConfig: utilfeature.DefaultFeatureGate.Enabled(features.ExpandedDNSConfig) || haveSameExpandedDNSConfig(podSpec, oldPodSpec),
 		// Allow pod spec to use OS field
 		AllowOSField: utilfeature.DefaultFeatureGate.Enabled(features.IdentifyPodOS),
-		// The default sysctl value does not contain a forward slash, and in 1.24 we intend to relax this to be true by default
-		AllowSysctlRegexContainSlash: false,
 	}
 
 	if oldPodSpec != nil {
@@ -457,9 +441,6 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po
 		// if old spec used non-integer multiple of huge page unit size, we must allow it
 		opts.AllowIndivisibleHugePagesValues = usesIndivisibleHugePagesValues(oldPodSpec)
 
-		// if old spec used use relaxed validation for Update requests where the existing object's sysctl contains a slash, we must allow it.
-		opts.AllowSysctlRegexContainSlash = hasSysctlsWithSlashNames(oldPodSpec)
-
 	}
 	if oldPodMeta != nil && !opts.AllowInvalidPodDeletionCost {
 		// This is an update, so validate only if the existing object was valid.

pkg/apis/core/validation/validation.go

@@ -3398,8 +3398,6 @@ type PodValidationOptions struct {
 	AllowExpandedDNSConfig bool
 	// Allow OSField to be set in the pod spec
 	AllowOSField bool
-	// Allow sysctl name to contain a slash
-	AllowSysctlRegexContainSlash bool
 }
 
 // validatePodMetadataAndSpec tests if required fields in the pod.metadata and pod.spec are set,
@@ -4058,9 +4056,6 @@ const (
 	// a sysctl segment regex, concatenated with dots to form a sysctl name
 	SysctlSegmentFmt string = "[a-z0-9]([-_a-z0-9]*[a-z0-9])?"
 
-	// a sysctl name regex
-	SysctlFmt string = "(" + SysctlSegmentFmt + "\\.)*" + SysctlSegmentFmt
-
 	// a sysctl name regex with slash allowed
 	SysctlContainSlashFmt string = "(" + SysctlSegmentFmt + "[\\./])*" + SysctlSegmentFmt
 
@@ -4068,41 +4063,28 @@ const (
 	SysctlMaxLength int = 253
 )
 
-var sysctlRegexp = regexp.MustCompile("^" + SysctlFmt + "$")
-
 var sysctlContainSlashRegexp = regexp.MustCompile("^" + SysctlContainSlashFmt + "$")
 
 // IsValidSysctlName checks that the given string is a valid sysctl name,
-// i.e. matches SysctlFmt (or SysctlContainSlashFmt if canContainSlash is true).
+// i.e. matches SysctlContainSlashFmt.
 // More info:
 //   https://man7.org/linux/man-pages/man8/sysctl.8.html
 //   https://man7.org/linux/man-pages/man5/sysctl.d.5.html
-func IsValidSysctlName(name string, canContainSlash bool) bool {
+func IsValidSysctlName(name string) bool {
 	if len(name) > SysctlMaxLength {
 		return false
 	}
-	if canContainSlash {
 	return sysctlContainSlashRegexp.MatchString(name)
 }
-	return sysctlRegexp.MatchString(name)
-}
 
-func getSysctlFmt(canContainSlash bool) string {
+func validateSysctls(sysctls []core.Sysctl, fldPath *field.Path) field.ErrorList {
-	if canContainSlash {
-		// use relaxed validation everywhere in 1.24
-		return SysctlContainSlashFmt
-	}
-	// Will be removed in 1.24
-	return SysctlFmt
-}
-func validateSysctls(sysctls []core.Sysctl, fldPath *field.Path, allowSysctlRegexContainSlash bool) field.ErrorList {
 	allErrs := field.ErrorList{}
 	names := make(map[string]struct{})
 	for i, s := range sysctls {
 		if len(s.Name) == 0 {
 			allErrs = append(allErrs, field.Required(fldPath.Index(i).Child("name"), ""))
-		} else if !IsValidSysctlName(s.Name, allowSysctlRegexContainSlash) {
+		} else if !IsValidSysctlName(s.Name) {
-			allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("name"), s.Name, fmt.Sprintf("must have at most %d characters and match regex %s", SysctlMaxLength, getSysctlFmt(allowSysctlRegexContainSlash))))
+			allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("name"), s.Name, fmt.Sprintf("must have at most %d characters and match regex %s", SysctlMaxLength, sysctlContainSlashRegexp)))
 		} else if _, ok := names[s.Name]; ok {
 			allErrs = append(allErrs, field.Duplicate(fldPath.Index(i).Child("name"), s.Name))
 		}
@@ -4142,7 +4124,7 @@ func ValidatePodSecurityContext(securityContext *core.PodSecurityContext, spec *
 		}
 
 		if len(securityContext.Sysctls) != 0 {
-			allErrs = append(allErrs, validateSysctls(securityContext.Sysctls, fldPath.Child("sysctls"), opts.AllowSysctlRegexContainSlash)...)
+			allErrs = append(allErrs, validateSysctls(securityContext.Sysctls, fldPath.Child("sysctls"))...)
 		}
 
 		if securityContext.FSGroupChangePolicy != nil {

pkg/apis/core/validation/validation_test.go

@@ -18261,6 +18261,8 @@ func TestIsValidSysctlName(t *testing.T) {
 		"a-b",
 		"abc",
 		"abc.def",
+		"a/b/c/d",
+		"a/b.c",
 	}
 	invalid := []string{
 		"",
@@ -18285,6 +18287,10 @@ func TestIsValidSysctlName(t *testing.T) {
 		"a.abc*",
 		"a.b.*",
 		"Abc",
+		"/",
+		"/a",
+		"a/abc*",
+		"a/b/*",
 		func(n int) string {
 			x := make([]byte, n)
 			for i := range x {
@@ -18294,34 +18300,13 @@ func TestIsValidSysctlName(t *testing.T) {
 		}(256),
 	}
 
-	containSlashesValid := []string{
-		"a/b/c/d",
-		"a/b.c",
-	}
-
-	containSlashesInvalid := []string{
-		"/",
-		"/a",
-		"a/abc*",
-		"a/b/*",
-	}
 	for _, s := range valid {
-		if !IsValidSysctlName(s, false) {
+		if !IsValidSysctlName(s) {
 			t.Errorf("%q expected to be a valid sysctl name", s)
 		}
 	}
 	for _, s := range invalid {
-		if IsValidSysctlName(s, false) {
+		if IsValidSysctlName(s) {
-			t.Errorf("%q expected to be an invalid sysctl name", s)
-		}
-	}
-	for _, s := range containSlashesValid {
-		if !IsValidSysctlName(s, true) {
-			t.Errorf("%q expected to be a valid sysctl name", s)
-		}
-	}
-	for _, s := range containSlashesInvalid {
-		if IsValidSysctlName(s, true) {
 			t.Errorf("%q expected to be an invalid sysctl name", s)
 		}
 	}
@@ -18331,6 +18316,8 @@ func TestValidateSysctls(t *testing.T) {
 	valid := []string{
 		"net.foo.bar",
 		"kernel.shmmax",
+		"net.ipv4.conf.enp3s0/200.forwarding",
+		"net/ipv4/conf/enp3s0.200/forwarding",
 	}
 	invalid := []string{
 		"i..nvalid",
@@ -18342,16 +18329,11 @@ func TestValidateSysctls(t *testing.T) {
 		"kernel.shmmax",
 	}
 
-	containSlashes := []string{
-		"net.ipv4.conf.enp3s0/200.forwarding",
-		"net/ipv4/conf/enp3s0.200/forwarding",
-	}
-
 	sysctls := make([]core.Sysctl, len(valid))
 	for i, sysctl := range valid {
 		sysctls[i].Name = sysctl
 	}
-	errs := validateSysctls(sysctls, field.NewPath("foo"), false)
+	errs := validateSysctls(sysctls, field.NewPath("foo"))
 	if len(errs) != 0 {
 		t.Errorf("unexpected validation errors: %v", errs)
 	}
@@ -18360,7 +18342,7 @@ func TestValidateSysctls(t *testing.T) {
 	for i, sysctl := range invalid {
 		sysctls[i].Name = sysctl
 	}
-	errs = validateSysctls(sysctls, field.NewPath("foo"), false)
+	errs = validateSysctls(sysctls, field.NewPath("foo"))
 	if len(errs) != 2 {
 		t.Errorf("expected 2 validation errors. Got: %v", errs)
 	} else {
@@ -18376,21 +18358,12 @@ func TestValidateSysctls(t *testing.T) {
 	for i, sysctl := range duplicates {
 		sysctls[i].Name = sysctl
 	}
-	errs = validateSysctls(sysctls, field.NewPath("foo"), false)
+	errs = validateSysctls(sysctls, field.NewPath("foo"))
 	if len(errs) != 1 {
 		t.Errorf("unexpected validation errors: %v", errs)
 	} else if errs[0].Type != field.ErrorTypeDuplicate {
 		t.Errorf("expected error type %v, got %v", field.ErrorTypeDuplicate, errs[0].Type)
 	}
-
-	sysctls = make([]core.Sysctl, len(containSlashes))
-	for i, sysctl := range containSlashes {
-		sysctls[i].Name = sysctl
-	}
-	errs = validateSysctls(sysctls, field.NewPath("foo"), true)
-	if len(errs) != 0 {
-		t.Errorf("unexpected validation errors: %v", errs)
-	}
 }
 
 func newNodeNameEndpoint(nodeName string) *core.Endpoints {

pkg/apis/policy/validation/validation.go

@@ -400,30 +400,22 @@ func validatePSPAllowedProcMountTypes(fldPath *field.Path, allowedProcMountTypes
 
 const sysctlPatternSegmentFmt string = "([a-z0-9][-_a-z0-9]*)?[a-z0-9*]"
 
-// SysctlPatternFmt is a regex used for matching valid sysctl patterns.
-const SysctlPatternFmt string = "(" + apivalidation.SysctlSegmentFmt + "\\.)*" + sysctlPatternSegmentFmt
-
 // SysctlContainSlashPatternFmt is a regex that contains a slash used for matching valid sysctl patterns.
 const SysctlContainSlashPatternFmt string = "(" + apivalidation.SysctlSegmentFmt + "[\\./])*" + sysctlPatternSegmentFmt
 
-var sysctlPatternRegexp = regexp.MustCompile("^" + SysctlPatternFmt + "$")
-
 var sysctlContainSlashPatternRegexp = regexp.MustCompile("^" + SysctlContainSlashPatternFmt + "$")
 
 // IsValidSysctlPattern checks if name is a valid sysctl pattern.
-// i.e. matches sysctlPatternRegexp (or sysctlContainSlashPatternRegexp if canContainSlash is true).
+// i.e. matches sysctlContainSlashPatternRegexp.
 // More info:
 //   https://man7.org/linux/man-pages/man8/sysctl.8.html
 //   https://man7.org/linux/man-pages/man5/sysctl.d.5.html
-func IsValidSysctlPattern(name string, canContainSlash bool) bool {
+func IsValidSysctlPattern(name string) bool {
 	if len(name) > apivalidation.SysctlMaxLength {
 		return false
 	}
-	if canContainSlash {
 	return sysctlContainSlashPatternRegexp.MatchString(name)
 }
-	return sysctlPatternRegexp.MatchString(name)
-}
 
 func validatePodSecurityPolicySysctlListsDoNotOverlap(allowedSysctlsFldPath, forbiddenSysctlsFldPath *field.Path, allowedUnsafeSysctls, forbiddenSysctls []string) field.ErrorList {
 	allErrs := field.ErrorList{}
@@ -478,12 +470,12 @@ func validatePodSecurityPolicySysctls(fldPath *field.Path, sysctls []string) fie
 	for i, s := range sysctls {
 		if len(s) == 0 {
 			allErrs = append(allErrs, field.Invalid(fldPath.Index(i), sysctls[i], "empty sysctl not allowed"))
-		} else if !IsValidSysctlPattern(string(s), false) {
+		} else if !IsValidSysctlPattern(string(s)) {
 			allErrs = append(
 				allErrs,
 				field.Invalid(fldPath.Index(i), sysctls[i], fmt.Sprintf("must have at most %d characters and match regex %s",
 					apivalidation.SysctlMaxLength,
-					SysctlPatternFmt,
+					SysctlContainSlashPatternFmt,
 				)),
 			)
 		} else if s[0] == '*' {

pkg/apis/policy/validation/validation_test.go

@@ -524,12 +524,12 @@ func TestValidatePodSecurityPolicy(t *testing.T) {
 		"invalid allowed unsafe sysctl pattern": {
 			psp:         invalidAllowedUnsafeSysctlPattern,
 			errorType:   field.ErrorTypeInvalid,
-			errorDetail: fmt.Sprintf("must have at most 253 characters and match regex %s", SysctlPatternFmt),
+			errorDetail: fmt.Sprintf("must have at most 253 characters and match regex %s", SysctlContainSlashPatternFmt),
 		},
 		"invalid forbidden sysctl pattern": {
 			psp:         invalidForbiddenSysctlPattern,
 			errorType:   field.ErrorTypeInvalid,
-			errorDetail: fmt.Sprintf("must have at most 253 characters and match regex %s", SysctlPatternFmt),
+			errorDetail: fmt.Sprintf("must have at most 253 characters and match regex %s", SysctlContainSlashPatternFmt),
 		},
 		"invalid overlapping sysctl pattern": {
 			psp:         invalidOverlappingSysctls,
@@ -805,6 +805,12 @@ func TestIsValidSysctlPattern(t *testing.T) {
 		"abc*",
 		"a.abc*",
 		"a.b.*",
+		"a/b/c/d",
+		"a/*",
+		"a/b/*",
+		"a.b/c*",
+		"a.b/c.d",
+		"a/b.c/d",
 	}
 	invalid := []string{
 		"",
@@ -823,6 +829,10 @@ func TestIsValidSysctlPattern(t *testing.T) {
 		"a*b",
 		"*a",
 		"Abc",
+		"/",
+		"a/",
+		"/a",
+		"a*/b",
 		func(n int) string {
 			x := make([]byte, n)
 			for i := range x {
@@ -832,12 +842,12 @@ func TestIsValidSysctlPattern(t *testing.T) {
 		}(256),
 	}
 	for _, s := range valid {
-		if !IsValidSysctlPattern(s, false) {
+		if !IsValidSysctlPattern(s) {
 			t.Errorf("%q expected to be a valid sysctl pattern", s)
 		}
 	}
 	for _, s := range invalid {
-		if IsValidSysctlPattern(s, false) {
+		if IsValidSysctlPattern(s) {
 			t.Errorf("%q expected to be an invalid sysctl pattern", s)
 		}
 	}

pkg/kubelet/sysctl/allowlist.go

@@ -48,7 +48,7 @@ func NewAllowlist(patterns []string) (*patternAllowlist, error) {
 	}
 
 	for _, s := range patterns {
-		if !policyvalidation.IsValidSysctlPattern(s, true) {
+		if !policyvalidation.IsValidSysctlPattern(s) {
 			return nil, fmt.Errorf("sysctl %q must have at most %d characters and match regex %s",
 				s,
 				validation.SysctlMaxLength,

test/e2e/common/node/sysctl.go

@@ -175,4 +175,56 @@ var _ = SIGDescribe("Sysctls [LinuxOnly] [NodeConformance]", func() {
 		err := e2epod.WaitForPodFailedReason(f.ClientSet, pod, "SysctlForbidden", f.Timeouts.PodStart)
 		framework.ExpectNoError(err)
 	})
+
+	/*
+	  Release: v1.23
+	  Testname: Sysctl, test sysctls supports slashes
+	  Description: Pod is created with kernel/shm_rmid_forced sysctl. Support slashes as sysctl separator. The '/' separator is also accepted in place of a '.'
+	  [LinuxOnly]: This test is marked as LinuxOnly since Windows does not support sysctls
+	*/
+	ginkgo.It("should support sysctls with slashes as separator [MinimumKubeletVersion:1.23]", func() {
+		pod := testPod()
+		pod.Spec.SecurityContext = &v1.PodSecurityContext{
+			Sysctls: []v1.Sysctl{
+				{
+					Name:  "kernel/shm_rmid_forced",
+					Value: "1",
+				},
+			},
+		}
+		pod.Spec.Containers[0].Command = []string{"/bin/sysctl", "kernel/shm_rmid_forced"}
+
+		ginkgo.By("Creating a pod with the kernel/shm_rmid_forced sysctl")
+		pod = podClient.Create(pod)
+
+		ginkgo.By("Watching for error events or started pod")
+		// watch for events instead of termination of pod because the kubelet deletes
+		// failed pods without running containers. This would create a race as the pod
+		// might have already been deleted here.
+		ev, err := f.PodClient().WaitForErrorEventOrSuccess(pod)
+		framework.ExpectNoError(err)
+		gomega.Expect(ev).To(gomega.BeNil())
+
+		ginkgo.By("Waiting for pod completion")
+		err = e2epod.WaitForPodNoLongerRunningInNamespace(f.ClientSet, pod.Name, f.Namespace.Name)
+		framework.ExpectNoError(err)
+		pod, err = podClient.Get(context.TODO(), pod.Name, metav1.GetOptions{})
+		framework.ExpectNoError(err)
+
+		ginkgo.By("Checking that the pod succeeded")
+		framework.ExpectEqual(pod.Status.Phase, v1.PodSucceeded)
+
+		ginkgo.By("Getting logs from the pod")
+		log, err := e2epod.GetPodLogs(f.ClientSet, f.Namespace.Name, pod.Name, pod.Spec.Containers[0].Name)
+		framework.ExpectNoError(err)
+
+		ginkgo.By("Checking that the sysctl is actually updated")
+		// Note that either "/" or "."  may be used as separators within sysctl variable names.
+		// "kernel.shm_rmid_forced=1" and "kernel/shm_rmid_forced=1" are equivalent.
+		// Run "/bin/sysctl kernel/shm_rmid_forced" command on Linux system
+		// The displayed result is "kernel.shm_rmid_forced=1"
+		// Therefore, the substring that needs to be checked for the obtained pod log is
+		// "kernel.shm_rmid_forced=1" instead of "kernel/shm_rmid_forced=1".
+		gomega.Expect(log).To(gomega.ContainSubstring("kernel.shm_rmid_forced = 1"))
+	})
 })