From d16fd6a915c4794baf589ffc51e43d142ab12dca Mon Sep 17 00:00:00 2001 From: PannagaRamamanohara Date: Thu, 11 Jul 2024 13:41:28 -0400 Subject: [PATCH 1/2] pkg/volume: Use QuotaMonitoring in UserNamespace Enable LocalStorageCapacityIsolationFSQuotaMonitoring only when hostUsers in PodSpec is set to false. Modify unit tests and e2e tests to verify Signed-off-by: PannagaRamamanohara --- pkg/kubelet/userns/userns_manager.go | 6 ++ pkg/volume/emptydir/empty_dir.go | 20 +++++-- pkg/volume/util/fsquota/quota_linux.go | 22 +++++--- pkg/volume/util/fsquota/quota_linux_test.go | 59 +++++++++++--------- pkg/volume/util/fsquota/quota_unsupported.go | 6 +- test/e2e_node/quota_lsci_test.go | 18 +++--- 6 files changed, 83 insertions(+), 48 deletions(-) diff --git a/pkg/kubelet/userns/userns_manager.go b/pkg/kubelet/userns/userns_manager.go index 41f7094cc4b..73167783297 100644 --- a/pkg/kubelet/userns/userns_manager.go +++ b/pkg/kubelet/userns/userns_manager.go @@ -386,6 +386,8 @@ func (m *UsernsManager) createUserNs(pod *v1.Pod) (userNs userNamespace, err err func (m *UsernsManager) GetOrCreateUserNamespaceMappings(pod *v1.Pod, runtimeHandler string) (*runtimeapi.UserNamespace, error) { featureEnabled := utilfeature.DefaultFeatureGate.Enabled(features.UserNamespacesSupport) + // TODO: If the default value for hostUsers ever changes, change the default value of + // userNamespacesEnabled as well if pod == nil || pod.Spec.HostUsers == nil { // if the feature is enabled, specify to use the node mode... if featureEnabled { @@ -512,3 +514,7 @@ func (m *UsernsManager) CleanupOrphanedPodUsernsAllocations(pods []*v1.Pod, runn return nil } + +func EnabledUserNamespacesSupport() bool { + return utilfeature.DefaultFeatureGate.Enabled(features.UserNamespacesSupport) +} diff --git a/pkg/volume/emptydir/empty_dir.go b/pkg/volume/emptydir/empty_dir.go index d4f94213954..09569b9956b 100644 --- a/pkg/volume/emptydir/empty_dir.go +++ b/pkg/volume/emptydir/empty_dir.go @@ -34,6 +34,7 @@ import ( v1helper "k8s.io/kubernetes/pkg/apis/core/v1/helper" "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/kubelet/cm" + usernamespacefeature "k8s.io/kubernetes/pkg/kubelet/userns" "k8s.io/kubernetes/pkg/volume" volumeutil "k8s.io/kubernetes/pkg/volume/util" "k8s.io/kubernetes/pkg/volume/util/fsquota" @@ -294,12 +295,19 @@ func (ed *emptyDir) assignQuota(dir string, mounterSize *resource.Quantity) erro if mounterSize != nil { // Deliberately shadow the outer use of err as noted // above. - hasQuotas, err := fsquota.SupportsQuotas(ed.mounter, dir) + // hostUsers field is interpreted as true by default, so a pod is by default + // not confined by a user namespace. + userNamespacesEnabled := false + if usernamespacefeature.EnabledUserNamespacesSupport() { + userNamespacesEnabled = ed.pod.Spec.HostUsers != nil && !*ed.pod.Spec.HostUsers + } + hasQuotas, err := fsquota.SupportsQuotas(ed.mounter, dir, userNamespacesEnabled) + klog.V(3).Infof("assignQuota called, hasQuotas = %t userNamespacesEnabled = %t", hasQuotas, userNamespacesEnabled) if err != nil { klog.V(3).Infof("Unable to check for quota support on %s: %s", dir, err.Error()) } else if hasQuotas { - klog.V(4).Infof("emptydir trying to assign quota %v on %s", mounterSize, dir) - if err := fsquota.AssignQuota(ed.mounter, dir, ed.pod.UID, mounterSize); err != nil { + klog.V(3).Infof("emptydir trying to assign quota %v on %s", mounterSize, dir) + if err := fsquota.AssignQuota(ed.mounter, dir, ed.pod.UID, mounterSize, userNamespacesEnabled); err != nil { klog.V(3).Infof("Set quota on %s failed %s", dir, err.Error()) return err } @@ -514,7 +522,11 @@ func (ed *emptyDir) TearDownAt(dir string) error { func (ed *emptyDir) teardownDefault(dir string) error { if utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolationFSQuotaMonitoring) { // Remove any quota - err := fsquota.ClearQuota(ed.mounter, dir) + userNamespacesEnabled := false + if usernamespacefeature.EnabledUserNamespacesSupport() { + userNamespacesEnabled = ed.pod.Spec.HostUsers != nil && !*ed.pod.Spec.HostUsers + } + err := fsquota.ClearQuota(ed.mounter, dir, userNamespacesEnabled) if err != nil { klog.Warningf("Warning: Failed to clear quota on %s: %v", dir, err) } diff --git a/pkg/volume/util/fsquota/quota_linux.go b/pkg/volume/util/fsquota/quota_linux.go index 33b74ab3b5f..ef24cedb249 100644 --- a/pkg/volume/util/fsquota/quota_linux.go +++ b/pkg/volume/util/fsquota/quota_linux.go @@ -225,11 +225,11 @@ func GetQuotaOnDir(m mount.Interface, path string) (common.QuotaID, error) { return getApplier(path).GetQuotaOnDir(path) } -func clearQuotaOnDir(m mount.Interface, path string) error { +func clearQuotaOnDir(m mount.Interface, path string, userNamespacesEnabled bool) error { // Since we may be called without path being in the map, // we explicitly have to check in this case. klog.V(4).Infof("clearQuotaOnDir %s", path) - supportsQuotas, err := SupportsQuotas(m, path) + supportsQuotas, err := SupportsQuotas(m, path, userNamespacesEnabled) if err != nil { // Log-and-continue instead of returning an error for now // due to unspecified backwards compatibility concerns (a subject to revise) @@ -269,11 +269,17 @@ func clearQuotaOnDir(m mount.Interface, path string) error { // don't cache the result because nothing will clean it up. // However, do cache the device->applier map; the number of devices // is bounded. -func SupportsQuotas(m mount.Interface, path string) (bool, error) { +// User namespaces prevent changes to project IDs on the filesystem, +// ensuring xfs-quota metrics' reliability; hence, userNamespacesEnabled is checked. +func SupportsQuotas(m mount.Interface, path string, userNamespacesEnabled bool) (bool, error) { if !enabledQuotasForMonitoring() { klog.V(3).Info("SupportsQuotas called, but quotas disabled") return false, nil } + if !userNamespacesEnabled { + klog.V(3).Info("SupportQuotas called and LocalStorageCapacityIsolationFSQuotaMonitoring enabled, but pod is not in a user namespace") + return false, nil + } supportsQuotasLock.Lock() defer supportsQuotasLock.Unlock() if supportsQuotas, ok := supportsQuotasMap[path]; ok { @@ -307,12 +313,12 @@ func SupportsQuotas(m mount.Interface, path string) (bool, error) { // AssignQuota chooses the quota ID based on the pod UID and path. // If the pod UID is identical to another one known, it may (but presently // doesn't) choose the same quota ID as other volumes in the pod. -func AssignQuota(m mount.Interface, path string, poduid types.UID, bytes *resource.Quantity) error { //nolint:staticcheck +func AssignQuota(m mount.Interface, path string, poduid types.UID, bytes *resource.Quantity, userNamespacesEnabled bool) error { //nolint:staticcheck if bytes == nil { return fmt.Errorf("attempting to assign null quota to %s", path) } ibytes := bytes.Value() - if ok, err := SupportsQuotas(m, path); !ok { + if ok, err := SupportsQuotas(m, path, userNamespacesEnabled); !ok { return fmt.Errorf("quotas not supported on %s: %v", path, err) } quotaLock.Lock() @@ -410,7 +416,7 @@ func GetInodes(path string) (*resource.Quantity, error) { } // ClearQuota -- remove the quota assigned to a directory -func ClearQuota(m mount.Interface, path string) error { +func ClearQuota(m mount.Interface, path string, userNamespacesEnabled bool) error { klog.V(3).Infof("ClearQuota %s", path) if !enabledQuotasForMonitoring() { return fmt.Errorf("clearQuota called, but quotas disabled") @@ -426,7 +432,7 @@ func ClearQuota(m mount.Interface, path string) error { // be found, which needs to be cleaned up. defer delete(supportsQuotasMap, path) defer clearApplier(path) - return clearQuotaOnDir(m, path) + return clearQuotaOnDir(m, path, userNamespacesEnabled) } _, ok = podQuotaMap[poduid] if !ok { @@ -443,7 +449,7 @@ func ClearQuota(m mount.Interface, path string) error { } count, ok := podDirCountMap[poduid] if count <= 1 || !ok { - err = clearQuotaOnDir(m, path) + err = clearQuotaOnDir(m, path, userNamespacesEnabled) // This error should be noted; we still need to clean up // and otherwise handle in the same way. if err != nil { diff --git a/pkg/volume/util/fsquota/quota_linux_test.go b/pkg/volume/util/fsquota/quota_linux_test.go index 5c50027832f..30c8ede4d23 100644 --- a/pkg/volume/util/fsquota/quota_linux_test.go +++ b/pkg/volume/util/fsquota/quota_linux_test.go @@ -366,19 +366,19 @@ func (v testVolumeQuota) GetInodes(_ string, _ common.QuotaID) (int64, error) { return 1, nil } -func fakeSupportsQuotas(path string) (bool, error) { +func fakeSupportsQuotas(path string, userNamespacesEnabled bool) (bool, error) { dummySetFSInfo(path) - return SupportsQuotas(dummyQuotaTest(), path) + return SupportsQuotas(dummyQuotaTest(), path, userNamespacesEnabled) } -func fakeAssignQuota(path string, poduid types.UID, bytes int64) error { +func fakeAssignQuota(path string, poduid types.UID, bytes int64, userNamespacesEnabled bool) error { dummySetFSInfo(path) - return AssignQuota(dummyQuotaTest(), path, poduid, resource.NewQuantity(bytes, resource.DecimalSI)) + return AssignQuota(dummyQuotaTest(), path, poduid, resource.NewQuantity(bytes, resource.DecimalSI), userNamespacesEnabled) } -func fakeClearQuota(path string) error { +func fakeClearQuota(path string, userNamespacesEnabled bool) error { dummySetFSInfo(path) - return ClearQuota(dummyQuotaTest(), path) + return ClearQuota(dummyQuotaTest(), path, userNamespacesEnabled) } type quotaTestCase struct { @@ -391,6 +391,7 @@ type quotaTestCase struct { expectedProjid string supportsQuota bool expectsSetQuota bool + userNamespacesEnabled bool deltaExpectedPodQuotaCount int deltaExpectedDirQuotaCount int deltaExpectedQuotaPodCount int @@ -444,59 +445,64 @@ volume1048581:1048581 var quotaTestCases = []quotaTestCase{ { - "SupportsQuotaOnQuotaVolume", + "SupportsQuotaOnQuotaVolumeWithUserNamespace", "/quota1/a", "", 1024, "Supports", "", "", - true, true, 0, 0, 0, 0, 1, 1, 0, 0, 1, 1, 1, + true, true, true, 0, 0, 0, 0, 1, 1, 0, 0, 1, 1, 1, + }, + { + "SupportsQuotaOnQuotaVolumeWithoutUserNamespace", + "/quota1/a", "", 1024, "Supports", "", "", + true, true, false, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, }, { "AssignQuotaFirstTime", "/quota1/a", "", 1024, "Set", projects1, projid1, - true, true, 1, 1, 1, 1, 0, 0, 1, 1, 0, 0, 0, + true, true, true, 1, 1, 1, 1, 0, 0, 1, 1, 0, 0, 0, }, { "AssignQuotaFirstTime", "/quota1/b", "x", 1024, "Set", projects2, projid2, - true, true, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1, 1, + true, true, true, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1, 1, }, { "AssignQuotaFirstTime", "/quota2/b", "x", 1024, "Set", projects3, projid3, - true, true, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, + true, true, true, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, }, { "AssignQuotaSecondTimeWithSameSize", "/quota1/b", "x", 1024, "Set", projects3, projid3, - true, true, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + true, true, true, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, }, { "AssignQuotaSecondTimeWithDifferentSize", "/quota2/b", "x", 2048, "Set", projects3, projid3, - true, false, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + true, false, true, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, }, { "ClearQuotaFirstTime", "/quota1/b", "", 1024, "Clear", projects4, projid4, - true, true, -1, -1, -1, -1, 0, -1, -1, -1, -1, -1, -1, + true, true, true, -1, -1, -1, -1, 0, -1, -1, -1, -1, -1, -1, }, { "SupportsQuotaOnNonQuotaVolume", "/noquota/a", "", 1024, "Supports", projects4, projid4, - false, false, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + false, false, true, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, }, { "ClearQuotaFirstTime", "/quota1/a", "", 1024, "Clear", projects5, projid5, - true, true, -1, -1, -1, -1, 0, -1, -1, -1, -1, -1, -1, + true, true, true, -1, -1, -1, -1, 0, -1, -1, -1, -1, -1, -1, }, { "ClearQuotaSecondTime", "/quota1/a", "", 1024, "Clear", projects5, projid5, - true, false, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + true, false, true, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, }, { "ClearQuotaFirstTime", "/quota2/b", "", 1024, "Clear", "", "", - true, true, -1, -1, -1, -1, 0, -1, -1, -1, -1, -1, -1, + true, true, true, -1, -1, -1, -1, 0, -1, -1, -1, -1, -1, -1, }, } @@ -534,20 +540,21 @@ func runCaseEnabled(t *testing.T, testcase quotaTestCase, seq int) bool { var err error switch testcase.op { case "Supports": - supports, err := fakeSupportsQuotas(testcase.path) + supports, err := fakeSupportsQuotas(testcase.path, testcase.userNamespacesEnabled) if err != nil { fail = true t.Errorf("Case %v (%s, %s, %v) Got error in fakeSupportsQuotas: %v", seq, testcase.name, testcase.path, true, err) } - if supports != testcase.supportsQuota { + expectedSupport := testcase.supportsQuota && testcase.userNamespacesEnabled + if supports != expectedSupport { fail = true - t.Errorf("Case %v (%s, %s, %v) fakeSupportsQuotas got %v, expect %v", seq, testcase.name, testcase.path, true, supports, testcase.supportsQuota) + t.Errorf("Case %v (%s, %s, userNamespacesEnabled: %v) fakeSupportsQuotas got %v, expect %v", seq, testcase.name, testcase.path, testcase.userNamespacesEnabled, supports, expectedSupport) } return fail case "Set": - err = fakeAssignQuota(testcase.path, testcase.poduid, testcase.bytes) + err = fakeAssignQuota(testcase.path, testcase.poduid, testcase.bytes, testcase.userNamespacesEnabled) case "Clear": - err = fakeClearQuota(testcase.path) + err = fakeClearQuota(testcase.path, testcase.userNamespacesEnabled) case "GetConsumption": _, err = GetConsumption(testcase.path) case "GetInodes": @@ -571,15 +578,15 @@ func runCaseDisabled(t *testing.T, testcase quotaTestCase, seq int) bool { var supports bool switch testcase.op { case "Supports": - if supports, _ = fakeSupportsQuotas(testcase.path); supports { + if supports, _ = fakeSupportsQuotas(testcase.path, testcase.userNamespacesEnabled); supports { t.Errorf("Case %v (%s, %s, %v) supports quotas but shouldn't", seq, testcase.name, testcase.path, false) return true } return false case "Set": - err = fakeAssignQuota(testcase.path, testcase.poduid, testcase.bytes) + err = fakeAssignQuota(testcase.path, testcase.poduid, testcase.bytes, testcase.userNamespacesEnabled) case "Clear": - err = fakeClearQuota(testcase.path) + err = fakeClearQuota(testcase.path, testcase.userNamespacesEnabled) case "GetConsumption": _, err = GetConsumption(testcase.path) case "GetInodes": diff --git a/pkg/volume/util/fsquota/quota_unsupported.go b/pkg/volume/util/fsquota/quota_unsupported.go index c5b89a69707..31d3e4f655e 100644 --- a/pkg/volume/util/fsquota/quota_unsupported.go +++ b/pkg/volume/util/fsquota/quota_unsupported.go @@ -39,12 +39,12 @@ func GetQuotaOnDir(_ mount.Interface, _ string) (common.QuotaID, error) { } // SupportsQuotas -- dummy implementation -func SupportsQuotas(_ mount.Interface, _ string) (bool, error) { +func SupportsQuotas(_ mount.Interface, _ string, _ bool) (bool, error) { return false, errNotImplemented } // AssignQuota -- dummy implementation -func AssignQuota(_ mount.Interface, _ string, _ types.UID, _ *resource.Quantity) error { +func AssignQuota(_ mount.Interface, _ string, _ types.UID, _ *resource.Quantity, _ bool) error { return errNotImplemented } @@ -59,6 +59,6 @@ func GetInodes(_ string) (*resource.Quantity, error) { } // ClearQuota -- dummy implementation -func ClearQuota(_ mount.Interface, _ string) error { +func ClearQuota(_ mount.Interface, _ string, _ bool) error { return errNotImplemented } diff --git a/test/e2e_node/quota_lsci_test.go b/test/e2e_node/quota_lsci_test.go index a0cfd494895..cd804623233 100644 --- a/test/e2e_node/quota_lsci_test.go +++ b/test/e2e_node/quota_lsci_test.go @@ -43,7 +43,7 @@ const ( LSCIQuotaFeature = features.LocalStorageCapacityIsolationFSQuotaMonitoring ) -func runOneQuotaTest(f *framework.Framework, quotasRequested bool) { +func runOneQuotaTest(f *framework.Framework, quotasRequested bool, userNamespacesEnabled bool) { evictionTestTimeout := 10 * time.Minute sizeLimit := resource.MustParse("100Mi") useOverLimit := 101 /* Mb */ @@ -63,7 +63,10 @@ func runOneQuotaTest(f *framework.Framework, quotasRequested bool) { defer withFeatureGate(LSCIQuotaFeature, quotasRequested)() // TODO: remove hardcoded kubelet volume directory path // framework.TestContext.KubeVolumeDir is currently not populated for node e2e - if quotasRequested && !supportsQuotas("/var/lib/kubelet") { + if !supportsUserNS(ctx, f) { + e2eskipper.Skipf("runtime does not support user namespaces") + } + if quotasRequested && !supportsQuotas("/var/lib/kubelet", userNamespacesEnabled) { // No point in running this as a positive test if quotas are not // enabled on the underlying filesystem. e2eskipper.Skipf("Cannot run LocalStorageCapacityIsolationFSQuotaMonitoring on filesystem without project quota enabled") @@ -98,11 +101,12 @@ func runOneQuotaTest(f *framework.Framework, quotasRequested bool) { // pod that creates a file, deletes it, and writes data to it. If // quotas are used to monitor, it will detect this deleted-but-in-use // file; if du is used to monitor, it will not detect this. -var _ = SIGDescribe("LocalStorageCapacityIsolationFSQuotaMonitoring", framework.WithSlow(), framework.WithSerial(), framework.WithDisruptive(), feature.LocalStorageCapacityIsolationQuota, nodefeature.LSCIQuotaMonitoring, func() { +var _ = SIGDescribe("LocalStorageCapacityIsolationFSQuotaMonitoring", framework.WithSlow(), framework.WithSerial(), framework.WithDisruptive(), feature.LocalStorageCapacityIsolationQuota, nodefeature.LSCIQuotaMonitoring, nodefeature.UserNamespacesSupport, feature.UserNamespacesSupport, func() { f := framework.NewDefaultFramework("localstorage-quota-monitoring-test") f.NamespacePodSecurityLevel = admissionapi.LevelPrivileged - runOneQuotaTest(f, true) - runOneQuotaTest(f, false) + runOneQuotaTest(f, true, true) + runOneQuotaTest(f, true, false) + runOneQuotaTest(f, false, true) addAfterEachForCleaningUpPods(f) }) @@ -152,7 +156,7 @@ func diskConcealingPod(name string, diskConsumedMB int, volumeSource *v1.VolumeS // Don't bother returning an error; if something goes wrong, // simply treat it as "no". -func supportsQuotas(dir string) bool { - supportsQuota, err := fsquota.SupportsQuotas(mount.New(""), dir) +func supportsQuotas(dir string, userNamespacesEnabled bool) bool { + supportsQuota, err := fsquota.SupportsQuotas(mount.New(""), dir, userNamespacesEnabled) return supportsQuota && err == nil } From 7df640d197456cad18debb11b96b4130be995145 Mon Sep 17 00:00:00 2001 From: PannagaRamamanohara Date: Mon, 22 Jul 2024 10:18:21 -0400 Subject: [PATCH 2/2] Promote FSQuotaMonitoring flag to beta --- pkg/features/kube_features.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index e898379de5b..7e813cbffdc 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -1085,7 +1085,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS LegacyServiceAccountTokenCleanUp: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // GA in 1.30; remove in 1.32 - LocalStorageCapacityIsolationFSQuotaMonitoring: {Default: false, PreRelease: featuregate.Alpha}, + LocalStorageCapacityIsolationFSQuotaMonitoring: {Default: true, PreRelease: featuregate.Beta}, LogarithmicScaleDown: {Default: true, PreRelease: featuregate.GA, LockToDefault: true},