diff --git a/pkg/quota/evaluator/core/services.go b/pkg/quota/evaluator/core/services.go index ec1a90282a..818a5b1c21 100644 --- a/pkg/quota/evaluator/core/services.go +++ b/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, diff --git a/plugin/pkg/admission/resourcequota/admission_test.go b/plugin/pkg/admission/resourcequota/admission_test.go index cfa3c71286..7d65080013 100644 --- a/plugin/pkg/admission/resourcequota/admission_test.go +++ b/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 := "aAdmission{ + 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{ diff --git a/plugin/pkg/admission/resourcequota/controller.go b/plugin/pkg/admission/resourcequota/controller.go index 495e076f5e..5431c9e163 100644 --- a/plugin/pkg/admission/resourcequota/controller.go +++ b/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) - if err != nil { - return nil, admission.NewForbidden(a, fmt.Errorf("Unable to get previous: %v", err)) + prevItem := a.GetOldObject() + if prevItem == nil { + 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)