Quota can look for a previous object from admission rather than do a lookup

pkg/quota/evaluator/core/services.go

@@ -38,6 +38,7 @@ func NewServiceEvaluator(kubeClient clientset.Interface) quota.Evaluator {
 		InternalGroupKind: api.Kind("Service"),
 		InternalOperationResources: map[admission.Operation][]api.ResourceName{
 			admission.Create: allResources,
+			admission.Update: allResources,
 		},
 		MatchedResourceNames: allResources,
 		MatchesScopeFunc:     generic.MatchesNoScopeFunc,

plugin/pkg/admission/resourcequota/admission_test.go

@@ -243,6 +243,97 @@ func TestAdmitBelowQuotaLimit(t *testing.T) {
 	}
 }
 
+// TestAdmitHandlesOldObjects verifies that admit handles updates correctly with old objects
+func TestAdmitHandlesOldObjects(t *testing.T) {
+	// in this scenario, the old quota was based on a service type=loadbalancer
+	resourceQuota := &api.ResourceQuota{
+		ObjectMeta: api.ObjectMeta{Name: "quota", Namespace: "test", ResourceVersion: "124"},
+		Status: api.ResourceQuotaStatus{
+			Hard: api.ResourceList{
+				api.ResourceServices:              resource.MustParse("10"),
+				api.ResourceServicesLoadBalancers: resource.MustParse("10"),
+				api.ResourceServicesNodePorts:     resource.MustParse("10"),
+			},
+			Used: api.ResourceList{
+				api.ResourceServices:              resource.MustParse("1"),
+				api.ResourceServicesLoadBalancers: resource.MustParse("1"),
+				api.ResourceServicesNodePorts:     resource.MustParse("0"),
+			},
+		},
+	}
+
+	// start up quota system
+	kubeClient := fake.NewSimpleClientset(resourceQuota)
+	indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{"namespace": cache.MetaNamespaceIndexFunc})
+	evaluator, _ := newQuotaEvaluator(kubeClient, install.NewRegistry(kubeClient))
+	evaluator.indexer = indexer
+	stopCh := make(chan struct{})
+	defer close(stopCh)
+	defer utilruntime.HandleCrash()
+	go evaluator.Run(5, stopCh)
+	handler := &quotaAdmission{
+		Handler:   admission.NewHandler(admission.Create, admission.Update),
+		evaluator: evaluator,
+	}
+	indexer.Add(resourceQuota)
+
+	// old service was a load balancer, but updated version is a node port.
+	oldService := &api.Service{
+		ObjectMeta: api.ObjectMeta{Name: "service", Namespace: "test"},
+		Spec:       api.ServiceSpec{Type: api.ServiceTypeLoadBalancer},
+	}
+	newService := &api.Service{
+		ObjectMeta: api.ObjectMeta{Name: "service", Namespace: "test"},
+		Spec:       api.ServiceSpec{Type: api.ServiceTypeNodePort},
+	}
+	err := handler.Admit(admission.NewAttributesRecord(newService, oldService, api.Kind("Service").WithVersion("version"), newService.Namespace, newService.Name, api.Resource("services").WithVersion("version"), "", admission.Update, nil))
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+	if len(kubeClient.Actions()) == 0 {
+		t.Errorf("Expected a client action")
+	}
+
+	// the only action should have been to update the quota (since we should not have fetched the previous item)
+	expectedActionSet := sets.NewString(
+		strings.Join([]string{"update", "resourcequotas", "status"}, "-"),
+	)
+	actionSet := sets.NewString()
+	for _, action := range kubeClient.Actions() {
+		actionSet.Insert(strings.Join([]string{action.GetVerb(), action.GetResource().Resource, action.GetSubresource()}, "-"))
+	}
+	if !actionSet.HasAll(expectedActionSet.List()...) {
+		t.Errorf("Expected actions:\n%v\n but got:\n%v\nDifference:\n%v", expectedActionSet, actionSet, expectedActionSet.Difference(actionSet))
+	}
+
+	// verify usage decremented the loadbalancer, and incremented the nodeport, but kept the service the same.
+	decimatedActions := removeListWatch(kubeClient.Actions())
+	lastActionIndex := len(decimatedActions) - 1
+	usage := decimatedActions[lastActionIndex].(testcore.UpdateAction).GetObject().(*api.ResourceQuota)
+	expectedUsage := api.ResourceQuota{
+		Status: api.ResourceQuotaStatus{
+			Hard: api.ResourceList{
+				api.ResourceServices:              resource.MustParse("10"),
+				api.ResourceServicesLoadBalancers: resource.MustParse("10"),
+				api.ResourceServicesNodePorts:     resource.MustParse("10"),
+			},
+			Used: api.ResourceList{
+				api.ResourceServices:              resource.MustParse("1"),
+				api.ResourceServicesLoadBalancers: resource.MustParse("0"),
+				api.ResourceServicesNodePorts:     resource.MustParse("1"),
+			},
+		},
+	}
+	for k, v := range expectedUsage.Status.Used {
+		actual := usage.Status.Used[k]
+		actualValue := actual.String()
+		expectedValue := v.String()
+		if expectedValue != actualValue {
+			t.Errorf("Usage Used: Key: %v, Expected: %v, Actual: %v", k, expectedValue, actualValue)
+		}
+	}
+}
+
 // TestAdmitExceedQuotaLimit verifies that if a pod exceeded allowed usage that its rejected during admission.
 func TestAdmitExceedQuotaLimit(t *testing.T) {
 	resourceQuota := &api.ResourceQuota{

plugin/pkg/admission/resourcequota/controller.go

@@ -322,8 +322,6 @@ func (e *quotaEvaluator) checkQuotas(quotas []api.ResourceQuota, admissionAttrib
 // that capture what the usage would be if the request succeeded.  It return an error if the is insufficient quota to satisfy the request
 func (e *quotaEvaluator) checkRequest(quotas []api.ResourceQuota, a admission.Attributes) ([]api.ResourceQuota, error) {
 	namespace := a.GetNamespace()
-	name := a.GetName()
-
 	evaluators := e.registry.Evaluators()
 	evaluator, found := evaluators[a.GetKind().GroupKind()]
 	if !found {
@@ -382,9 +380,9 @@ func (e *quotaEvaluator) checkRequest(quotas []api.ResourceQuota, a admission.At
 	// if usage shows no change, just return since it has no impact on quota
 	deltaUsage := evaluator.Usage(inputObject)
 	if admission.Update == op {
-		prevItem, err := evaluator.Get(namespace, name)
+		prevItem := a.GetOldObject()
-		if err != nil {
+		if prevItem == nil {
-			return nil, admission.NewForbidden(a, fmt.Errorf("Unable to get previous: %v", err))
+			return nil, admission.NewForbidden(a, fmt.Errorf("Unable to get previous usage since prior version of object was not found"))
 		}
 		prevUsage := evaluator.Usage(prevItem)
 		deltaUsage = quota.Subtract(deltaUsage, prevUsage)