diff --git a/pkg/quota/resources.go b/pkg/quota/resources.go index 39c50b1bd3..94e0f6b3c9 100644 --- a/pkg/quota/resources.go +++ b/pkg/quota/resources.go @@ -169,6 +169,18 @@ func IsZero(a api.ResourceList) bool { return true } +// IsNegative returns the set of resource names that have a negative value. +func IsNegative(a api.ResourceList) []api.ResourceName { + results := []api.ResourceName{} + zero := resource.MustParse("0") + for k, v := range a { + if v.Cmp(zero) < 0 { + results = append(results, k) + } + } + return results +} + // ToSet takes a list of resource names and converts to a string set func ToSet(resourceNames []api.ResourceName) sets.String { result := sets.NewString() diff --git a/pkg/quota/resources_test.go b/pkg/quota/resources_test.go index 37d79032b8..e87763691e 100644 --- a/pkg/quota/resources_test.go +++ b/pkg/quota/resources_test.go @@ -256,3 +256,37 @@ func TestIsZero(t *testing.T) { } } } + +func TestIsNegative(t *testing.T) { + testCases := map[string]struct { + a api.ResourceList + expected []api.ResourceName + }{ + "empty": { + a: api.ResourceList{}, + expected: []api.ResourceName{}, + }, + "some-negative": { + a: api.ResourceList{ + api.ResourceCPU: resource.MustParse("-10"), + api.ResourceMemory: resource.MustParse("0"), + }, + expected: []api.ResourceName{api.ResourceCPU}, + }, + "all-negative": { + a: api.ResourceList{ + api.ResourceCPU: resource.MustParse("-200m"), + api.ResourceMemory: resource.MustParse("-1Gi"), + }, + expected: []api.ResourceName{api.ResourceCPU, api.ResourceMemory}, + }, + } + for testName, testCase := range testCases { + actual := IsNegative(testCase.a) + actualSet := ToSet(actual) + expectedSet := ToSet(testCase.expected) + if !actualSet.Equal(expectedSet) { + t.Errorf("%s expected: %v, actual: %v", testName, expectedSet, actualSet) + } + } +} diff --git a/plugin/pkg/admission/resourcequota/admission_test.go b/plugin/pkg/admission/resourcequota/admission_test.go index 4a44d28ae7..51823c9612 100644 --- a/plugin/pkg/admission/resourcequota/admission_test.go +++ b/plugin/pkg/admission/resourcequota/admission_test.go @@ -73,6 +73,15 @@ func validPod(name string, numContainers int, resources api.ResourceRequirements return pod } +func validPersistentVolumeClaim(name string, resources api.ResourceRequirements) *api.PersistentVolumeClaim { + return &api.PersistentVolumeClaim{ + ObjectMeta: api.ObjectMeta{Name: name, Namespace: "test"}, + Spec: api.PersistentVolumeClaimSpec{ + Resources: resources, + }, + } +} + func TestPrettyPrint(t *testing.T) { toResourceList := func(resources map[api.ResourceName]string) api.ResourceList { resourceList := api.ResourceList{} @@ -871,3 +880,49 @@ func TestAdmissionSetsMissingNamespace(t *testing.T) { t.Errorf("Got unexpected pod namespace: %q != %q", newPod.Namespace, namespace) } } + +// TestAdmitRejectsNegativeUsage verifies that usage for any measured resource cannot be negative. +func TestAdmitRejectsNegativeUsage(t *testing.T) { + resourceQuota := &api.ResourceQuota{ + ObjectMeta: api.ObjectMeta{Name: "quota", Namespace: "test", ResourceVersion: "124"}, + Status: api.ResourceQuotaStatus{ + Hard: api.ResourceList{ + api.ResourcePersistentVolumeClaims: resource.MustParse("3"), + api.ResourceRequestsStorage: resource.MustParse("100Gi"), + }, + Used: api.ResourceList{ + api.ResourcePersistentVolumeClaims: resource.MustParse("1"), + api.ResourceRequestsStorage: resource.MustParse("10Gi"), + }, + }, + } + kubeClient := fake.NewSimpleClientset(resourceQuota) + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{"namespace": cache.MetaNamespaceIndexFunc}) + stopCh := make(chan struct{}) + defer close(stopCh) + + quotaAccessor, _ := newQuotaAccessor(kubeClient) + quotaAccessor.indexer = indexer + go quotaAccessor.Run(stopCh) + evaluator := NewQuotaEvaluator(quotaAccessor, install.NewRegistry(kubeClient), nil, 5, stopCh) + + defer utilruntime.HandleCrash() + handler := "aAdmission{ + Handler: admission.NewHandler(admission.Create, admission.Update), + evaluator: evaluator, + } + indexer.Add(resourceQuota) + // verify quota rejects negative pvc storage requests + newPvc := validPersistentVolumeClaim("not-allowed-pvc", getResourceRequirements(api.ResourceList{api.ResourceStorage: resource.MustParse("-1Gi")}, api.ResourceList{})) + err := handler.Admit(admission.NewAttributesRecord(newPvc, nil, api.Kind("PersistentVolumeClaim").WithVersion("version"), newPvc.Namespace, newPvc.Name, api.Resource("persistentvolumeclaims").WithVersion("version"), "", admission.Create, nil)) + if err == nil { + t.Errorf("Expected an error because the pvc has negative storage usage") + } + + // verify quota accepts non-negative pvc storage requests + newPvc = validPersistentVolumeClaim("not-allowed-pvc", getResourceRequirements(api.ResourceList{api.ResourceStorage: resource.MustParse("1Gi")}, api.ResourceList{})) + err = handler.Admit(admission.NewAttributesRecord(newPvc, nil, api.Kind("PersistentVolumeClaim").WithVersion("version"), newPvc.Namespace, newPvc.Name, api.Resource("persistentvolumeclaims").WithVersion("version"), "", admission.Create, nil)) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } +} diff --git a/plugin/pkg/admission/resourcequota/controller.go b/plugin/pkg/admission/resourcequota/controller.go index 1337f06666..81752684b3 100644 --- a/plugin/pkg/admission/resourcequota/controller.go +++ b/plugin/pkg/admission/resourcequota/controller.go @@ -358,6 +358,12 @@ func (e *quotaEvaluator) checkRequest(quotas []api.ResourceQuota, a admission.At // on updates, we need to subtract the previous measured usage // if usage shows no change, just return since it has no impact on quota deltaUsage := evaluator.Usage(inputObject) + + // ensure that usage for input object is never negative (this would mean a resource made a negative resource requirement) + if negativeUsage := quota.IsNegative(deltaUsage); len(negativeUsage) > 0 { + return nil, admission.NewForbidden(a, fmt.Errorf("quota usage is negative for resource(s): %s", prettyPrintResourceNames(negativeUsage))) + } + if admission.Update == op { prevItem := a.GetOldObject() if prevItem == nil { @@ -499,6 +505,15 @@ func prettyPrint(item api.ResourceList) string { return strings.Join(parts, ",") } +func prettyPrintResourceNames(a []api.ResourceName) string { + values := []string{} + for _, value := range a { + values = append(values, string(value)) + } + sort.Strings(values) + return strings.Join(values, ",") +} + // hasUsageStats returns true if for each hard constraint there is a value for its current usage func hasUsageStats(resourceQuota *api.ResourceQuota) bool { for resourceName := range resourceQuota.Status.Hard {