From 7a84bcded47c021d1ebb37a9263017afe987d77f Mon Sep 17 00:00:00 2001 From: "Chris S. Kim" Date: Wed, 30 Aug 2023 10:01:29 -0400 Subject: [PATCH] PR feedback --- .../tests/config-entries/config_entries_test.go | 2 -- .../fixtures/bases/crds-oss/servicedefaults.yaml | 6 ++++-- charts/consul/templates/crd-servicedefaults.yaml | 3 --- control-plane/api/v1alpha1/servicedefaults_types.go | 12 +++++------- .../api/v1alpha1/servicedefaults_types_test.go | 4 ++-- .../bases/consul.hashicorp.com_servicedefaults.yaml | 10 +++++----- 6 files changed, 16 insertions(+), 21 deletions(-) diff --git a/acceptance/tests/config-entries/config_entries_test.go b/acceptance/tests/config-entries/config_entries_test.go index a289db078e..81b0a75ff4 100644 --- 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 index c924096322..d0d1fe73bb 100644 --- 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 index a34412f62a..2e7528a41c 100644 --- 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 index f960058075..7b9d3ea69c 100644 --- a/control-plane/api/v1alpha1/servicedefaults_types.go +++ b/control-plane/api/v1alpha1/servicedefaults_types.go @@ -220,7 +220,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. @@ -284,7 +284,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 { @@ -305,7 +305,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 { @@ -328,11 +328,11 @@ 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")) @@ -340,8 +340,6 @@ func (irl InstanceLevelRateLimits) validate(path *field.Path) field.ErrorList { 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 index 807919dd65..2292e55791 100644 --- 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 index 741919d77f..b0a37d19a5 100644 --- 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,