Merge pull request #97451 from pacoxu/fix/cluster-ip-quota

bugfix: nodeport quota check failure result into failing to create a clusterip service

pkg/quota/v1/evaluator/core/BUILD

@@ -46,6 +46,7 @@ go_test(
     embed = [":go_default_library"],
     deps = [
         "//pkg/apis/core:go_default_library",
+        "//pkg/features:go_default_library",
         "//pkg/util/node:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library",
@@ -54,6 +55,9 @@ go_test(
         "//staging/src/k8s.io/apimachinery/pkg/util/clock:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/quota/v1:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/quota/v1/generic:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
+        "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library",
+        "//vendor/k8s.io/utils/pointer:go_default_library",
     ],
 )
 

pkg/quota/v1/evaluator/core/services.go

@@ -26,8 +26,10 @@ import (
 	"k8s.io/apiserver/pkg/admission"
 	quota "k8s.io/apiserver/pkg/quota/v1"
 	"k8s.io/apiserver/pkg/quota/v1/generic"
+	"k8s.io/apiserver/pkg/util/feature"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	k8s_api_v1 "k8s.io/kubernetes/pkg/apis/core/v1"
+	"k8s.io/kubernetes/pkg/features"
 )
 
 // the name used for object count quota
@@ -128,14 +130,33 @@ func (p *serviceEvaluator) Usage(item runtime.Object) (corev1.ResourceList, erro
 		value := resource.NewQuantity(int64(ports), resource.DecimalSI)
 		result[corev1.ResourceServicesNodePorts] = *value
 	case corev1.ServiceTypeLoadBalancer:
-		// load balancer services need to count node ports and load balancers
+		// load balancer services need to count node ports. If creation of node ports
-		value := resource.NewQuantity(int64(ports), resource.DecimalSI)
+		// is suppressed only ports with explicit NodePort values are counted.
-		result[corev1.ResourceServicesNodePorts] = *value
+		// nodeports won't be allocated yet, so we can't simply count the actual values.
+		// We need to look at the intent.
+		if feature.DefaultFeatureGate.Enabled(features.ServiceLBNodePortControl) &&
+			svc.Spec.AllocateLoadBalancerNodePorts != nil &&
+			*svc.Spec.AllocateLoadBalancerNodePorts == false {
+			result[corev1.ResourceServicesNodePorts] = *portsWithNodePorts(svc)
+		} else {
+			value := resource.NewQuantity(int64(ports), resource.DecimalSI)
+			result[corev1.ResourceServicesNodePorts] = *value
+		}
 		result[corev1.ResourceServicesLoadBalancers] = *(resource.NewQuantity(1, resource.DecimalSI))
 	}
 	return result, nil
 }
 
+func portsWithNodePorts(svc *corev1.Service) *resource.Quantity {
+	count := 0
+	for _, p := range svc.Spec.Ports {
+		if p.NodePort != 0 {
+			count++
+		}
+	}
+	return resource.NewQuantity(int64(count), resource.DecimalSI)
+}
+
 // UsageStats calculates aggregate usage for the object.
 func (p *serviceEvaluator) UsageStats(options quota.UsageStatsOptions) (quota.UsageStats, error) {
 	return generic.CalculateUsageStats(options, p.listFuncByNamespace, generic.MatchesNoScopeFunc, p.Usage)

pkg/quota/v1/evaluator/core/services_test.go

@@ -24,7 +24,11 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	quota "k8s.io/apiserver/pkg/quota/v1"
 	"k8s.io/apiserver/pkg/quota/v1/generic"
+	"k8s.io/apiserver/pkg/util/feature"
+	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	api "k8s.io/kubernetes/pkg/apis/core"
+	"k8s.io/kubernetes/pkg/features"
+	utilpointer "k8s.io/utils/pointer"
 )
 
 func TestServiceEvaluatorMatchesResources(t *testing.T) {
@@ -52,8 +56,9 @@ func TestServiceEvaluatorMatchesResources(t *testing.T) {
 func TestServiceEvaluatorUsage(t *testing.T) {
 	evaluator := NewServiceEvaluator(nil)
 	testCases := map[string]struct {
-		service *api.Service
+		service                         *api.Service
-		usage   corev1.ResourceList
+		usage                           corev1.ResourceList
+		serviceLBNodePortControlEnabled bool
 	}{
 		"loadbalancer": {
 			service: &api.Service{
@@ -86,6 +91,27 @@ func TestServiceEvaluatorUsage(t *testing.T) {
 				generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"),
 			},
 		},
+		"loadbalancer_2_ports": {
+			service: &api.Service{
+				Spec: api.ServiceSpec{
+					Type: api.ServiceTypeLoadBalancer,
+					Ports: []api.ServicePort{
+						{
+							Port: 27443,
+						},
+						{
+							Port: 27444,
+						},
+					},
+				},
+			},
+			usage: corev1.ResourceList{
+				corev1.ResourceServicesNodePorts:     resource.MustParse("2"),
+				corev1.ResourceServicesLoadBalancers: resource.MustParse("1"),
+				corev1.ResourceServices:              resource.MustParse("1"),
+				generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"),
+			},
+		},
 		"clusterip": {
 			service: &api.Service{
 				Spec: api.ServiceSpec{
@@ -138,15 +164,113 @@ func TestServiceEvaluatorUsage(t *testing.T) {
 				generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"),
 			},
 		},
+		"nodeports-disabled": {
+			service: &api.Service{
+				Spec: api.ServiceSpec{
+					Type: api.ServiceTypeLoadBalancer,
+					Ports: []api.ServicePort{
+						{
+							Port: 27443,
+						},
+						{
+							Port: 27444,
+						},
+					},
+					AllocateLoadBalancerNodePorts: utilpointer.BoolPtr(false),
+				},
+			},
+			usage: corev1.ResourceList{
+				corev1.ResourceServices:              resource.MustParse("1"),
+				corev1.ResourceServicesNodePorts:     resource.MustParse("0"),
+				corev1.ResourceServicesLoadBalancers: resource.MustParse("1"),
+				generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"),
+			},
+			serviceLBNodePortControlEnabled: true,
+		},
+		"nodeports-default-enabled": {
+			service: &api.Service{
+				Spec: api.ServiceSpec{
+					Type: api.ServiceTypeLoadBalancer,
+					Ports: []api.ServicePort{
+						{
+							Port:     27443,
+							NodePort: 32001,
+						},
+						{
+							Port:     27444,
+							NodePort: 32002,
+						},
+					},
+					AllocateLoadBalancerNodePorts: nil,
+				},
+			},
+			usage: corev1.ResourceList{
+				corev1.ResourceServices:              resource.MustParse("1"),
+				corev1.ResourceServicesNodePorts:     resource.MustParse("2"),
+				corev1.ResourceServicesLoadBalancers: resource.MustParse("1"),
+				generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"),
+			},
+		},
+		"nodeports-explicitly-enabled": {
+			service: &api.Service{
+				Spec: api.ServiceSpec{
+					Type: api.ServiceTypeLoadBalancer,
+					Ports: []api.ServicePort{
+						{
+							Port: 27443,
+						},
+						{
+							Port: 27444,
+						},
+					},
+					AllocateLoadBalancerNodePorts: utilpointer.BoolPtr(true),
+				},
+			},
+			usage: corev1.ResourceList{
+				corev1.ResourceServices:              resource.MustParse("1"),
+				corev1.ResourceServicesNodePorts:     resource.MustParse("2"),
+				corev1.ResourceServicesLoadBalancers: resource.MustParse("1"),
+				generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"),
+			},
+			serviceLBNodePortControlEnabled: true,
+		},
+		"nodeports-disabled-but-specified": {
+			service: &api.Service{
+				Spec: api.ServiceSpec{
+					Type: api.ServiceTypeLoadBalancer,
+					Ports: []api.ServicePort{
+						{
+							Port:     27443,
+							NodePort: 32001,
+						},
+						{
+							Port:     27444,
+							NodePort: 32002,
+						},
+					},
+					AllocateLoadBalancerNodePorts: utilpointer.BoolPtr(false),
+				},
+			},
+			usage: corev1.ResourceList{
+				corev1.ResourceServices:              resource.MustParse("1"),
+				corev1.ResourceServicesNodePorts:     resource.MustParse("2"),
+				corev1.ResourceServicesLoadBalancers: resource.MustParse("1"),
+				generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"),
+			},
+			serviceLBNodePortControlEnabled: true,
+		},
 	}
 	for testName, testCase := range testCases {
-		actual, err := evaluator.Usage(testCase.service)
+		t.Run(testName, func(t *testing.T) {
-		if err != nil {
+			defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.ServiceLBNodePortControl, testCase.serviceLBNodePortControlEnabled)()
-			t.Errorf("%s unexpected error: %v", testName, err)
+			actual, err := evaluator.Usage(testCase.service)
-		}
+			if err != nil {
-		if !quota.Equals(testCase.usage, actual) {
+				t.Errorf("%s unexpected error: %v", testName, err)
-			t.Errorf("%s expected: %v, actual: %v", testName, testCase.usage, actual)
+			}
-		}
+			if !quota.Equals(testCase.usage, actual) {
+				t.Errorf("%s expected: %v, actual: %v", testName, testCase.usage, actual)
+			}
+		})
 	}
 }
 

staging/src/k8s.io/apiserver/pkg/admission/plugin/resourcequota/controller.go

@@ -510,7 +510,10 @@ func CheckRequest(quotas []corev1.ResourceQuota, a admission.Attributes, evaluat
 		}
 	}
 
-	if quota.IsZero(deltaUsage) {
+	// ignore items in deltaUsage with zero usage
+	deltaUsage = quota.RemoveZeros(deltaUsage)
+	// if there is no remaining non-zero usage, short-circuit and return
+	if len(deltaUsage) == 0 {
 		return quotas, nil
 	}
 

staging/src/k8s.io/apiserver/pkg/quota/v1/resources.go

@@ -226,6 +226,17 @@ func IsZero(a corev1.ResourceList) bool {
 	return true
 }
 
+// RemoveZeros returns a new resource list that only has no zero values
+func RemoveZeros(a corev1.ResourceList) corev1.ResourceList {
+	result := corev1.ResourceList{}
+	for key, value := range a {
+		if !value.IsZero() {
+			result[key] = value
+		}
+	}
+	return result
+}
+
 // IsNegative returns the set of resource names that have a negative value.
 func IsNegative(a corev1.ResourceList) []corev1.ResourceName {
 	results := []corev1.ResourceName{}

staging/src/k8s.io/apiserver/pkg/quota/v1/resources_test.go

@@ -287,6 +287,50 @@ func TestIsZero(t *testing.T) {
 	}
 }
 
+func TestRemoveZeros(t *testing.T) {
+	testCases := map[string]struct {
+		a        corev1.ResourceList
+		expected corev1.ResourceList
+	}{
+		"empty": {
+			a:        corev1.ResourceList{},
+			expected: corev1.ResourceList{},
+		},
+		"all-zeros": {
+			a: corev1.ResourceList{
+				corev1.ResourceCPU:    resource.MustParse("0"),
+				corev1.ResourceMemory: resource.MustParse("0"),
+			},
+			expected: corev1.ResourceList{},
+		},
+		"some-zeros": {
+			a: corev1.ResourceList{
+				corev1.ResourceCPU:     resource.MustParse("0"),
+				corev1.ResourceMemory:  resource.MustParse("0"),
+				corev1.ResourceStorage: resource.MustParse("100Gi"),
+			},
+			expected: corev1.ResourceList{
+				corev1.ResourceStorage: resource.MustParse("100Gi"),
+			},
+		},
+		"non-zero": {
+			a: corev1.ResourceList{
+				corev1.ResourceCPU:    resource.MustParse("200m"),
+				corev1.ResourceMemory: resource.MustParse("1Gi"),
+			},
+			expected: corev1.ResourceList{
+				corev1.ResourceCPU:    resource.MustParse("200m"),
+				corev1.ResourceMemory: resource.MustParse("1Gi"),
+			},
+		},
+	}
+	for testName, testCase := range testCases {
+		if result := RemoveZeros(testCase.a); !Equals(result, testCase.expected) {
+			t.Errorf("%s expected: %v, actual: %v", testName, testCase.expected, result)
+		}
+	}
+}
+
 func TestIsNegative(t *testing.T) {
 	testCases := map[string]struct {
 		a        corev1.ResourceList

test/e2e/apimachinery/resource_quota.go

@@ -36,6 +36,7 @@ import (
 	"k8s.io/kubernetes/test/e2e/framework"
 	"k8s.io/kubernetes/test/utils/crd"
 	imageutils "k8s.io/kubernetes/test/utils/image"
+	"k8s.io/utils/pointer"
 
 	"github.com/onsi/ginkgo"
 )
@@ -100,23 +101,37 @@ var _ = SIGDescribe("ResourceQuota", func() {
 		framework.ExpectNoError(err)
 
 		ginkgo.By("Creating a Service")
-		service := newTestServiceForQuota("test-service", v1.ServiceTypeClusterIP)
+		service := newTestServiceForQuota("test-service", v1.ServiceTypeClusterIP, false)
 		service, err = f.ClientSet.CoreV1().Services(f.Namespace.Name).Create(context.TODO(), service, metav1.CreateOptions{})
 		framework.ExpectNoError(err)
 
+		ginkgo.By("Creating a NodePort Service")
+		nodeport := newTestServiceForQuota("test-service-np", v1.ServiceTypeNodePort, false)
+		nodeport, err = f.ClientSet.CoreV1().Services(f.Namespace.Name).Create(context.TODO(), nodeport, metav1.CreateOptions{})
+		framework.ExpectNoError(err)
+
+		ginkgo.By("Not allowing a LoadBalancer Service with NodePort to be created that exceeds remaining quota")
+		loadbalancer := newTestServiceForQuota("test-service-lb", v1.ServiceTypeLoadBalancer, false)
+		_, err = f.ClientSet.CoreV1().Services(f.Namespace.Name).Create(context.TODO(), loadbalancer, metav1.CreateOptions{})
+		framework.ExpectError(err)
+
 		ginkgo.By("Ensuring resource quota status captures service creation")
 		usedResources = v1.ResourceList{}
 		usedResources[v1.ResourceQuotas] = resource.MustParse(strconv.Itoa(c + 1))
-		usedResources[v1.ResourceServices] = resource.MustParse("1")
+		usedResources[v1.ResourceServices] = resource.MustParse("2")
+		usedResources[v1.ResourceServicesNodePorts] = resource.MustParse("1")
 		err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources)
 		framework.ExpectNoError(err)
 
-		ginkgo.By("Deleting a Service")
+		ginkgo.By("Deleting Services")
 		err = f.ClientSet.CoreV1().Services(f.Namespace.Name).Delete(context.TODO(), service.Name, metav1.DeleteOptions{})
 		framework.ExpectNoError(err)
+		err = f.ClientSet.CoreV1().Services(f.Namespace.Name).Delete(context.TODO(), nodeport.Name, metav1.DeleteOptions{})
+		framework.ExpectNoError(err)
 
 		ginkgo.By("Ensuring resource quota status released usage")
 		usedResources[v1.ResourceServices] = resource.MustParse("0")
+		usedResources[v1.ResourceServicesNodePorts] = resource.MustParse("0")
 		err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources)
 		framework.ExpectNoError(err)
 	})
@@ -1625,7 +1640,7 @@ func newTestReplicaSetForQuota(name, image string, replicas int32) *appsv1.Repli
 }
 
 // newTestServiceForQuota returns a simple service
-func newTestServiceForQuota(name string, serviceType v1.ServiceType) *v1.Service {
+func newTestServiceForQuota(name string, serviceType v1.ServiceType, allocateLoadBalancerNodePorts bool) *v1.Service {
 	return &v1.Service{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: name,
@@ -1636,6 +1651,7 @@ func newTestServiceForQuota(name string, serviceType v1.ServiceType) *v1.Service
 				Port:       80,
 				TargetPort: intstr.FromInt(80),
 			}},
+			AllocateLoadBalancerNodePorts: pointer.BoolPtr(allocateLoadBalancerNodePorts),
 		},
 	}
 }