Quota admission errors if usage is negative

lavalamp · Aug 11, 2016 · 5cca4b0 · 5cca4b0
1 parent 2fb7cae
commit 5cca4b0
Show file tree

Hide file tree

Showing 4 changed files with 116 additions and 0 deletions.
diff --git 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
@@ -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
@@ -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 := &quotaAdmission{
+		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
@@ -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 {