From 42a89ea81e45bb407bc63e468e1b4fc1e7fec1bf Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Fri, 12 Apr 2019 16:23:12 -0400 Subject: [PATCH 1/2] Short-circuit quota admission rejection on zero-delta updates --- .../pkg/admission/resourcequota/controller.go | 46 +++++++++---------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/plugin/pkg/admission/resourcequota/controller.go b/plugin/pkg/admission/resourcequota/controller.go index 1c109d2713..abefe34b79 100644 --- a/plugin/pkg/admission/resourcequota/controller.go +++ b/plugin/pkg/admission/resourcequota/controller.go @@ -468,29 +468,6 @@ func CheckRequest(quotas []corev1.ResourceQuota, a admission.Attributes, evaluat restrictedResourcesSet.Insert(localRestrictedResourcesSet.List()...) } - // verify that for every resource that had limited by default consumption - // enabled that there was a corresponding quota that covered its use. - // if not, we reject the request. - hasNoCoveringQuota := limitedResourceNamesSet.Difference(restrictedResourcesSet) - if len(hasNoCoveringQuota) > 0 { - return quotas, admission.NewForbidden(a, fmt.Errorf("insufficient quota to consume: %v", strings.Join(hasNoCoveringQuota.List(), ","))) - } - - // verify that for every scope that had limited access enabled - // that there was a corresponding quota that covered it. - // if not, we reject the request. - scopesHasNoCoveringQuota, err := evaluator.UncoveredQuotaScopes(limitedScopes, restrictedScopes) - if err != nil { - return quotas, err - } - if len(scopesHasNoCoveringQuota) > 0 { - return quotas, fmt.Errorf("insufficient quota to match these scopes: %v", scopesHasNoCoveringQuota) - } - - if len(interestingQuotaIndexes) == 0 { - return quotas, nil - } - // Usage of some resources cannot be counted in isolation. For example, when // the resource represents a number of unique references to external // resource. In such a case an evaluator needs to process other objects in @@ -537,6 +514,29 @@ func CheckRequest(quotas []corev1.ResourceQuota, a admission.Attributes, evaluat return quotas, nil } + // verify that for every resource that had limited by default consumption + // enabled that there was a corresponding quota that covered its use. + // if not, we reject the request. + hasNoCoveringQuota := limitedResourceNamesSet.Difference(restrictedResourcesSet) + if len(hasNoCoveringQuota) > 0 { + return quotas, admission.NewForbidden(a, fmt.Errorf("insufficient quota to consume: %v", strings.Join(hasNoCoveringQuota.List(), ","))) + } + + // verify that for every scope that had limited access enabled + // that there was a corresponding quota that covered it. + // if not, we reject the request. + scopesHasNoCoveringQuota, err := evaluator.UncoveredQuotaScopes(limitedScopes, restrictedScopes) + if err != nil { + return quotas, err + } + if len(scopesHasNoCoveringQuota) > 0 { + return quotas, fmt.Errorf("insufficient quota to match these scopes: %v", scopesHasNoCoveringQuota) + } + + if len(interestingQuotaIndexes) == 0 { + return quotas, nil + } + outQuotas, err := copyQuotas(quotas) if err != nil { return nil, err From 7b1b46b98c72fa7f2ba73ed778f930cb69f6a776 Mon Sep 17 00:00:00 2001 From: Mansi Agarwal Date: Fri, 26 Apr 2019 13:13:41 -0700 Subject: [PATCH 2/2] Accept admission request if resource is being deleted --- .../admission/resourcequota/admission_test.go | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/plugin/pkg/admission/resourcequota/admission_test.go b/plugin/pkg/admission/resourcequota/admission_test.go index 5de5a0eab4..1d05c52bad 100644 --- a/plugin/pkg/admission/resourcequota/admission_test.go +++ b/plugin/pkg/admission/resourcequota/admission_test.go @@ -2163,3 +2163,93 @@ func TestAdmitLimitedScopeWithCoverQuota(t *testing.T) { } } + +// TestAdmitZeroDeltaUsageWithoutCoveringQuota verifies that resource quota is not required for zero delta requests. +func TestAdmitZeroDeltaUsageWithoutCoveringQuota(t *testing.T) { + + kubeClient := fake.NewSimpleClientset() + stopCh := make(chan struct{}) + defer close(stopCh) + + informerFactory := informers.NewSharedInformerFactory(kubeClient, controller.NoResyncPeriodFunc()) + quotaAccessor, _ := newQuotaAccessor() + quotaAccessor.client = kubeClient + quotaAccessor.lister = informerFactory.Core().V1().ResourceQuotas().Lister() + + // disable services unless there is a covering quota. + config := &resourcequotaapi.Configuration{ + LimitedResources: []resourcequotaapi.LimitedResource{ + { + Resource: "services", + MatchContains: []string{"services"}, + }, + }, + } + quotaConfiguration := install.NewQuotaConfigurationForAdmission() + evaluator := NewQuotaEvaluator(quotaAccessor, quotaConfiguration.IgnoredResources(), generic.NewRegistry(quotaConfiguration.Evaluators()), nil, config, 5, stopCh) + + handler := &QuotaAdmission{ + Handler: admission.NewHandler(admission.Create, admission.Update), + evaluator: evaluator, + } + + existingService := &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "service", Namespace: "test", ResourceVersion: "1"}, + Spec: api.ServiceSpec{Type: api.ServiceTypeLoadBalancer}, + } + newService := &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "service", Namespace: "test"}, + Spec: api.ServiceSpec{Type: api.ServiceTypeLoadBalancer}, + } + + err := handler.Validate(admission.NewAttributesRecord(newService, existingService, api.Kind("Service").WithVersion("version"), newService.Namespace, newService.Name, corev1.Resource("services").WithVersion("version"), "", admission.Update, false, nil), nil) + if err != nil { + t.Errorf("unexpected error: %v", err) + } +} + +// TestAdmitRejectDeltaUsageWithoutCoveringQuota verifies that resource quota is required for non zero delta requests. +func TestAdmitRejectDeltaUsageWithoutCoveringQuota(t *testing.T) { + kubeClient := fake.NewSimpleClientset() + stopCh := make(chan struct{}) + defer close(stopCh) + + informerFactory := informers.NewSharedInformerFactory(kubeClient, controller.NoResyncPeriodFunc()) + quotaAccessor, _ := newQuotaAccessor() + quotaAccessor.client = kubeClient + quotaAccessor.lister = informerFactory.Core().V1().ResourceQuotas().Lister() + + // disable services unless there is a covering quota. + config := &resourcequotaapi.Configuration{ + LimitedResources: []resourcequotaapi.LimitedResource{ + { + Resource: "services", + MatchContains: []string{"services"}, + }, + }, + } + quotaConfiguration := install.NewQuotaConfigurationForAdmission() + evaluator := NewQuotaEvaluator(quotaAccessor, quotaConfiguration.IgnoredResources(), generic.NewRegistry(quotaConfiguration.Evaluators()), nil, config, 5, stopCh) + + handler := &QuotaAdmission{ + Handler: admission.NewHandler(admission.Create, admission.Update), + evaluator: evaluator, + } + + existingService := &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "service", Namespace: "test", ResourceVersion: "1"}, + Spec: api.ServiceSpec{Type: api.ServiceTypeLoadBalancer}, + } + newService := &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "service", Namespace: "test"}, + Spec: api.ServiceSpec{ + Type: api.ServiceTypeNodePort, + Ports: []api.ServicePort{{Port: 1234}}, + }, + } + + err := handler.Validate(admission.NewAttributesRecord(newService, existingService, api.Kind("Service").WithVersion("version"), newService.Namespace, newService.Name, corev1.Resource("services").WithVersion("version"), "", admission.Update, false, nil), nil) + if err == nil { + t.Errorf("Expected an error for consuming a limited resource without quota.") + } +}