Merge pull request #77367 from mansi-a/automated-cherry-pick-of-#76299-upstream-release-1.14

Automated cherry pick of #76299: Short-circuit quota admission rejection on zero-delta

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.")
+	}
+}

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