PR feedback

hashicorp · Sep 5, 2023 · 19be2ce · 19be2ce
1 parent f3202d7
commit 19be2ce
Show file tree

Hide file tree

Showing 6 changed files with 16 additions and 21 deletions.
diff --git a/acceptance/tests/config-entries/config_entries_test.go b/acceptance/tests/config-entries/config_entries_test.go
@@ -234,8 +234,6 @@ func TestController(t *testing.T) {
 					require.Equal(r, 100.0, rateLimitIPConfigEntry.KV.WriteRate)
 					require.Equal(r, 100.0, rateLimitIPConfigEntry.Tenancy.ReadRate)
 					require.Equal(r, 100.0, rateLimitIPConfigEntry.Tenancy.WriteRate)
-					// require.Equal(r, 100.0, rateLimitIPConfigEntry.PreparedQuery.ReadRate)
-					// require.Equal(r, 100.0, rateLimitIPConfigEntry.PreparedQuery.WriteRate)
 					require.Equal(r, 100.0, rateLimitIPConfigEntry.Session.ReadRate)
 					require.Equal(r, 100.0, rateLimitIPConfigEntry.Session.WriteRate)
 					require.Equal(r, 100.0, rateLimitIPConfigEntry.Txn.ReadRate)

diff --git a/acceptance/tests/fixtures/bases/crds-oss/servicedefaults.yaml b/acceptance/tests/fixtures/bases/crds-oss/servicedefaults.yaml
@@ -36,11 +36,13 @@ spec:
       requestsPerSecond: 1234
       requestsMaxBurst: 2345
       routes:
-        - pathExact: "/foo"
+        - pathExact: "/exact"
           requestsPerSecond: 222
           requestsMaxBurst: 333
-        - pathPrefix: "/admin"
+        - pathPrefix: "/prefix"
           requestsPerSecond: 444
+        - pathRegex: "/regex"
+          requestsPerSecond: 555
   envoyExtensions:
     - name: builtin/aws/lambda
       required: false

diff --git a/charts/consul/templates/crd-servicedefaults.yaml b/charts/consul/templates/crd-servicedefaults.yaml
@@ -221,9 +221,6 @@ spec:
                           routes. For a given request, the first matching route will
                           be applied, if any. Overrides any top-level configuration.
                         items:
-                          description: InstanceLevelRouteRateLimits represents rate
-                            limit configuration applied to a route matching one of
-                            PathExact/PathPrefix/PathRegex.
                           properties:
                             pathExact:
                               description: Exact path to match. Exactly one of PathExact,

diff --git a/control-plane/api/v1alpha1/servicedefaults_types.go b/control-plane/api/v1alpha1/servicedefaults_types.go
@@ -222,7 +222,7 @@ type ServiceDefaultsDestination struct {
 
 // RateLimits is rate limiting configuration that is applied to
 // inbound traffic for a service.
-// Rate limiting is a Consul enterprise feature.
+// Rate limiting is a Consul Enterprise feature.
 type RateLimits struct {
 	// InstanceLevel represents rate limit configuration
 	// that is applied per service instance.
@@ -286,7 +286,7 @@ func (irl InstanceLevelRateLimits) validate(path *field.Path) field.ErrorList {
 	var allErrs field.ErrorList
 
 	// Track if RequestsPerSecond is set in at least one place in the config
-	isRatelimitSet := irl.RequestsPerSecond > 0
+	isRateLimitSet := irl.RequestsPerSecond > 0
 
 	// Top-level RequestsPerSecond can be 0 (unset) or a positive number.
 	if irl.RequestsPerSecond < 0 {
@@ -307,7 +307,7 @@ func (irl InstanceLevelRateLimits) validate(path *field.Path) field.ErrorList {
 		allErrs = append(allErrs,
 			field.Invalid(path.Child("requestsMaxBurst"),
 				irl.RequestsMaxBurst,
-				"RequestsMaxBurst must be greater than 0"))
+				"RequestsMaxBurst must be positive"))
 	}
 
 	for i, route := range irl.Routes {
@@ -330,20 +330,18 @@ func (irl InstanceLevelRateLimits) validate(path *field.Path) field.ErrorList {
 		if route.RequestsMaxBurst < 0 {
 			allErrs = append(allErrs, field.Invalid(
 				path.Child("routes").Index(i).Child("requestsMaxBurst"),
-				route.RequestsMaxBurst, "RequestsMaxBurst must be greater than 0"))
+				route.RequestsMaxBurst, "RequestsMaxBurst must be positive"))
 		}
 	}
 
-	if !isRatelimitSet {
+	if !isRateLimitSet {
 		allErrs = append(allErrs, field.Invalid(
 			path.Child("requestsPerSecond"),
 			irl.RequestsPerSecond, "At least one of top-level or route-level RequestsPerSecond must be set"))
 	}
 	return allErrs
 }
 
-// InstanceLevelRouteRateLimits represents rate limit configuration
-// applied to a route matching one of PathExact/PathPrefix/PathRegex.
 type InstanceLevelRouteRateLimits struct {
 	// Exact path to match. Exactly one of PathExact, PathPrefix, or PathRegex must be specified.
 	PathExact string `json:"pathExact,omitempty"`

diff --git a/control-plane/api/v1alpha1/servicedefaults_types_test.go b/control-plane/api/v1alpha1/servicedefaults_types_test.go
@@ -1459,7 +1459,7 @@ func TestServiceDefaults_Validate(t *testing.T) {
 					},
 				},
 			},
-			expectedErrMsg: `servicedefaults.consul.hashicorp.com "my-service" is invalid: spec.rateLimits.instanceLevel.requestsMaxBurst: Invalid value: -1: RequestsMaxBurst must be greater than 0`,
+			expectedErrMsg: `servicedefaults.consul.hashicorp.com "my-service" is invalid: spec.rateLimits.instanceLevel.requestsMaxBurst: Invalid value: -1: RequestsMaxBurst must be positive`,
 		},
 		"rateLimits.instanceLevel.routes (invalid path)": {
 			input: &ServiceDefaults{
@@ -1522,7 +1522,7 @@ func TestServiceDefaults_Validate(t *testing.T) {
 					},
 				},
 			},
-			expectedErrMsg: `servicedefaults.consul.hashicorp.com "my-service" is invalid: spec.rateLimits.instanceLevel.routes[0].requestsMaxBurst: Invalid value: -1: RequestsMaxBurst must be greater than 0`,
+			expectedErrMsg: `servicedefaults.consul.hashicorp.com "my-service" is invalid: spec.rateLimits.instanceLevel.routes[0].requestsMaxBurst: Invalid value: -1: RequestsMaxBurst must be positive`,
 		},
 		"rateLimits.requestsMaxBurst (top-level and route-level unset)": {
 			input: &ServiceDefaults{

diff --git a/control-plane/config/crd/bases/consul.hashicorp.com_servicedefaults.yaml b/control-plane/config/crd/bases/consul.hashicorp.com_servicedefaults.yaml
@@ -187,7 +187,7 @@ spec:
                 type: string
               rateLimits:
                 description: RateLimits is rate limiting configuration that is applied
-                  to inbound traffic for a service. Rate limiting is a Consul Enterprise
+                  to inbound traffic for a service. Rate limiting is a Consul enterprise
                   feature.
                 properties:
                   instanceLevel:
@@ -211,13 +211,13 @@ spec:
                         type: integer
                       routes:
                         description: Routes is a list of rate limits applied to specific
-                          routes. Overrides any top-level configuration.
+                          routes. For a given request, the first matching route will
+                          be applied, if any. Overrides any top-level configuration.
                         items:
-                          description: InstanceLevelRouteRateLimits represents rate
-                            limit configuration applied to a route matching one of
-                            PathExact/PathPrefix/PathRegex.
                           properties:
                             pathExact:
+                              description: Exact path to match. Exactly one of PathExact,
+                                PathPrefix, or PathRegex must be specified.
                               type: string
                             pathPrefix:
                               description: Prefix to match. Exactly one of PathExact,