Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Admission Control Filter #10230

Closed
wants to merge 64 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
64 commits
Select commit Hold shift + click to select a range
991d39f
wip
Jan 10, 2020
84c1b51
wip
Jan 11, 2020
d3e80e0
wip
tonya11en Jan 13, 2020
668040d
wip
tonya11en Jan 13, 2020
3727a6d
still broken
tonya11en Jan 13, 2020
b91335f
builds wip
Jan 14, 2020
871fe70
stats
tonya11en Jan 14, 2020
cd1d879
thread local
tonya11en Jan 14, 2020
b768ad1
format
tonya11en Jan 14, 2020
7a4678e
tests
tonya11en Jan 15, 2020
d5278b8
Fix bugs and more tests.
tonya11en Jan 16, 2020
34144fd
runtime double
tonya11en Jan 16, 2020
f3ec7f7
runtime double
tonya11en Jan 16, 2020
5e3d1ed
filter config test
tonya11en Jan 17, 2020
5654fa2
wip
tonya11en Jan 17, 2020
10472c0
compiles, wip, test fails
tonya11en Jan 21, 2020
a15ad6e
tests pass
tonya11en Jan 22, 2020
7f30f79
filter disable test
tonya11en Jan 22, 2020
fa3cd1c
Merge remote-tracking branch 'upstream/master' into admctl
tonya11en Jan 22, 2020
29478b0
format and fix merge stuff
tonya11en Jan 22, 2020
6b69513
more filter tests
tonya11en Jan 22, 2020
48043b6
more filter tests
tonya11en Jan 22, 2020
d0b8c50
test stats
tonya11en Jan 22, 2020
5358a78
wip
tonya11en Jan 24, 2020
6bf90d1
Merge remote-tracking branch 'upstream/master' into admctl
tonya11en Mar 2, 2020
fbecc7f
format
tonya11en Mar 2, 2020
c31da84
wip
tonya11en Mar 2, 2020
a9b9f8b
doc: fix SNI FAQ link (#10227)
lizan Mar 2, 2020
e01eda5
builds
Mar 3, 2020
5e97619
wip
tonya11en Mar 3, 2020
fc666b7
wip
tonya11en Mar 3, 2020
d639d37
builds
tonya11en Mar 3, 2020
167133d
progress
tonya11en Mar 3, 2020
598fc5f
passes
tonya11en Mar 3, 2020
bafb45e
format
tonya11en Mar 3, 2020
17fa361
Merge remote-tracking branch 'upstream/master' into admctl
tonya11en Mar 3, 2020
3bde611
format
tonya11en Mar 3, 2020
0d6dc11
Revert "format"
tonya11en Mar 3, 2020
e416db7
grpc status proto
tonya11en Mar 5, 2020
2748f16
wip
tonya11en Mar 6, 2020
0fa0905
wip
tonya11en Mar 6, 2020
72cd42d
Merge remote-tracking branch 'upstream/master' into admctl
tonya11en Mar 6, 2020
8340a73
wip
tonya11en Mar 6, 2020
65c9773
protobuf fixes
tonya11en Mar 6, 2020
dde5ea2
tests pass
tonya11en Mar 6, 2020
74a35a3
small addition to tests. still need to test new behaviors.
tonya11en Mar 6, 2020
dbd2d88
more grpc tests
tonya11en Mar 11, 2020
0359eb4
wip
tonya11en Mar 11, 2020
5bb2747
Merge remote-tracking branch 'upstream/master' into admctl
tonya11en Mar 11, 2020
762b131
format
tonya11en Mar 11, 2020
a29c9eb
tests pass
tonya11en Mar 12, 2020
c83b8fd
more tests and proto stuff
tonya11en Mar 12, 2020
c562c65
fix spelling
tonya11en Mar 12, 2020
b420460
fix api build
tonya11en Mar 12, 2020
935e672
missing protos
tonya11en Mar 12, 2020
cddd71e
http integration test. still need grpc test.
tonya11en Mar 13, 2020
eab6c0f
rm half-baked grpc test
tonya11en Mar 13, 2020
6b893f6
error bars
tonya11en Mar 13, 2020
1379ac5
make test less flaky
tonya11en Mar 13, 2020
9921663
minor test changes
tonya11en Mar 13, 2020
9cb2499
wip
tonya11en Mar 26, 2020
80c219a
Fix the grpc integration test.
tonya11en Mar 26, 2020
43d479b
fix format
tonya11en Mar 26, 2020
20f6036
Kick CI
tonya11en Mar 31, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ extensions/filters/common/original_src @snowp @klarose
/*/extensions/common/aws @lavignes @mattklein123
# adaptive concurrency limit extension.
/*/extensions/filters/http/adaptive_concurrency @tonya11en @mattklein123
# admission control extension.
/*/extensions/filters/http/admission_control @tonya11en @mattklein123
# http inspector
/*/extensions/filters/listener/http_inspector @yxue @PiotrSikora @lizan
# attribute context
Expand Down
4 changes: 4 additions & 0 deletions api/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ proto_library(
"//envoy/config/filter/dubbo/router/v2alpha1:pkg",
"//envoy/config/filter/fault/v2:pkg",
"//envoy/config/filter/http/adaptive_concurrency/v2alpha:pkg",
"//envoy/config/filter/http/admission_control/v2alpha:pkg",
"//envoy/config/filter/http/aws_lambda/v2alpha:pkg",
"//envoy/config/filter/http/aws_request_signing/v2alpha:pkg",
"//envoy/config/filter/http/buffer/v2:pkg",
Expand Down Expand Up @@ -115,6 +116,7 @@ proto_library(
"//envoy/service/tap/v2alpha:pkg",
"//envoy/service/trace/v2:pkg",
"//envoy/type:pkg",
"//envoy/type/grpc/v2:pkg",
"//envoy/type/matcher:pkg",
"//envoy/type/metadata/v2:pkg",
"//envoy/type/tracing/v2:pkg",
Expand Down Expand Up @@ -161,6 +163,7 @@ proto_library(
"//envoy/extensions/common/tap/v3:pkg",
"//envoy/extensions/filters/common/fault/v3:pkg",
"//envoy/extensions/filters/http/adaptive_concurrency/v3:pkg",
"//envoy/extensions/filters/http/admission_control/v3alpha:pkg",
"//envoy/extensions/filters/http/aws_lambda/v3:pkg",
"//envoy/extensions/filters/http/aws_request_signing/v3:pkg",
"//envoy/extensions/filters/http/buffer/v3:pkg",
Expand Down Expand Up @@ -237,6 +240,7 @@ proto_library(
"//envoy/service/status/v3:pkg",
"//envoy/service/tap/v3:pkg",
"//envoy/service/trace/v3:pkg",
"//envoy/type/grpc/v3:pkg",
"//envoy/type/matcher/v3:pkg",
"//envoy/type/metadata/v3:pkg",
"//envoy/type/tracing/v3:pkg",
Expand Down
2 changes: 2 additions & 0 deletions api/docs/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ proto_library(
"//envoy/config/filter/dubbo/router/v2alpha1:pkg",
"//envoy/config/filter/fault/v2:pkg",
"//envoy/config/filter/http/adaptive_concurrency/v2alpha:pkg",
"//envoy/config/filter/http/admission_control/v2alpha:pkg",
"//envoy/config/filter/http/aws_lambda/v2alpha:pkg",
"//envoy/config/filter/http/aws_request_signing/v2alpha:pkg",
"//envoy/config/filter/http/buffer/v2:pkg",
Expand Down Expand Up @@ -121,6 +122,7 @@ proto_library(
"//envoy/service/tap/v2alpha:pkg",
"//envoy/service/trace/v2:pkg",
"//envoy/type:pkg",
"//envoy/type/grpc/v2:pkg",
"//envoy/type/matcher:pkg",
"//envoy/type/metadata/v2:pkg",
"//envoy/type/tracing/v2:pkg",
Expand Down
9 changes: 9 additions & 0 deletions api/envoy/api/v2/core/base.proto
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,15 @@ message RuntimeUInt32 {
string runtime_key = 3 [(validate.rules).string = {min_bytes: 1}];
}

// Runtime derived double with a default when not specified.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR is so large please split this and associated tests into a different PR (if there is anything else that can be split out please do so)

message RuntimeDouble {
// Default value if runtime value is not available.
double default_value = 1;

// Runtime key to get value for comparison. This value is used if defined.
string runtime_key = 2 [(validate.rules).string = {min_bytes: 1}];
}

// Runtime derived bool with a default when not specified.
message RuntimeFeatureFlag {
// Default value if runtime value is not available.
Expand Down
11 changes: 11 additions & 0 deletions api/envoy/config/core/v3/base.proto
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,17 @@ message RuntimeUInt32 {
string runtime_key = 3 [(validate.rules).string = {min_bytes: 1}];
}

// Runtime derived double with a default when not specified.
message RuntimeDouble {
option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.RuntimeDouble";

// Default value if runtime value is not available.
double default_value = 1;

// Runtime key to get value for comparison. This value is used if defined.
string runtime_key = 2 [(validate.rules).string = {min_bytes: 1}];
}

// Runtime derived bool with a default when not specified.
message RuntimeFeatureFlag {
option (udpa.annotations.versioning).previous_message_type =
Expand Down
13 changes: 13 additions & 0 deletions api/envoy/config/filter/http/admission_control/v2alpha/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# DO NOT EDIT. This file is generated by tools/proto_sync.py.

load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package")

licenses(["notice"]) # Apache 2

api_proto_package(
deps = [
"//envoy/api/v2/core:pkg",
"//envoy/type/grpc/v2:pkg",
"@com_github_cncf_udpa//udpa/annotations:pkg",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
syntax = "proto3";

package envoy.config.filter.http.admission_control.v2alpha;

import "envoy/api/v2/core/base.proto";
import "envoy/type/grpc/v2/grpc_status.proto";

import "google/api/annotations.proto";
import "google/protobuf/duration.proto";
import "google/protobuf/wrappers.proto";
import "google/rpc/status.proto";

import "udpa/annotations/migrate.proto";
import "validate/validate.proto";

option java_package = "io.envoyproxy.envoy.config.filter.http.admission_control.v2alpha";
option java_outer_classname = "AdmissionControlProto";
option java_multiple_files = true;
option (udpa.annotations.file_migrate).move_to_package =
"envoy.extensions.filters.http.admission_control.v3alpha";

// [#protodoc-title: Admission Control]
// Admission Control :ref:`configuration overview
// <config_http_filters_admission_control>`.
// [#extension: envoy.filters.http.admission_control]

message AdmissionControl {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of how to define/configure what success means, here is my suggestion:

I would define a oneof which contains a required message which defines how a request is considered successful. I think this will give us future flexibility to potentially add pluggable success criteria. In the near term I would have a message called something like DefaultSuccessCriteria which can be used to configure things like 5xx, gRPC, etc. Maybe let's try that type of shape and have a look at whether we feel it will be extensible enough or not?

enum HttpStatusRange {
Http1xx = 0;
Http2xx = 1;
Http3xx = 2;
Http4xx = 3;
Http5xx = 4;
}

// Default method of specifying what constitutes a successful request. All status codes that
// indicate a successful request must be explicitly specified if not relying on the default
// values.
message DefaultSuccessCriteria {
// If HTTP statuses are unspecified, defaults to 2xx.
repeated HttpStatusRange http_status = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would make this an actual numeric range for flexibility. See Int32Range or similar as something you might potentially use.


// GRPC status codes to consider as request successes. If unspecified, defaults to "OK".
repeated type.grpc.v2.GrpcStatus grpc_status = 2;
}

// If set to false, the admission control filter will operate as a pass-through filter. If the
// message is unspecified, the filter will be enabled.
api.v2.core.RuntimeFeatureFlag enabled = 1;

// Defines how a request is considered a success/failure.
oneof success_criteria {
option (validate.required) = true;

DefaultSuccessCriteria default_success_criteria = 2;
}

// The time window over which the success rate is calculated. The window is rounded to the nearest
// second. Defaults to 120s.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this seems super long. Also, naively I would think we would want a rolling window? Can you clarify how this works in terms of rolling vs. static windows, etc.?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is a sliding window. Each second, we're popping off the success rate information from 121s ago and replacing it with the SR data from 1s ago. If I understood the first question correctly, we both want the same thing w.r.t. the windows. There are no tumbling windows.

I just went with 120s because that's what was mentioned in the SRE handbook. I can run some experiments with different window sizes for the same traffic profile to get a better handle on what most people would want. I can put a TODO in code to make sure we're not stuck with an insane default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that makes sense, I would just update the docs to make that a bit more clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would still clarify that this is a sliding window at 1s granularity, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump still needs more verbiage to talk about sliding, etc.

google.protobuf.Duration sampling_window = 3;

// Rejection probability is defined by the formula::
//
// max(0, (rq_count - aggression_coefficient * rq_success_count) / (rq_count + 1))
//
// The coefficient dictates how aggressively the admission controller will throttle requests as
// the success rate drops. Lower values will cause throttling to kick in at higher success rates
// and result in more aggressive throttling. Any values less than 1.0, will be set to 1.0. If the
// message is unspecified, the coefficient is 2.0.
api.v2.core.RuntimeDouble aggression_coefficient = 4;
}
14 changes: 14 additions & 0 deletions api/envoy/extensions/filters/http/admission_control/v3alpha/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# DO NOT EDIT. This file is generated by tools/proto_sync.py.

load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package")

licenses(["notice"]) # Apache 2

api_proto_package(
deps = [
"//envoy/config/core/v3:pkg",
"//envoy/config/filter/http/admission_control/v2alpha:pkg",
"//envoy/type/grpc/v3:pkg",
"@com_github_cncf_udpa//udpa/annotations:pkg",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
syntax = "proto3";

package envoy.extensions.filters.http.admission_control.v3alpha;

import "envoy/config/core/v3/base.proto";
import "envoy/type/grpc/v3/grpc_status.proto";

import "google/api/annotations.proto";
import "google/protobuf/duration.proto";
import "google/protobuf/wrappers.proto";
import "google/rpc/status.proto";

import "udpa/annotations/versioning.proto";

import "validate/validate.proto";

option java_package = "io.envoyproxy.envoy.extensions.filters.http.admission_control.v3alpha";
option java_outer_classname = "AdmissionControlProto";
option java_multiple_files = true;

// [#protodoc-title: Admission Control]
// Admission Control :ref:`configuration overview
// <config_http_filters_admission_control>`.
// [#extension: envoy.filters.http.admission_control]

message AdmissionControl {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.http.admission_control.v2alpha.AdmissionControl";

enum HttpStatusRange {
Http1xx = 0;
Http2xx = 1;
Http3xx = 2;
Http4xx = 3;
Http5xx = 4;
}

// Default method of specifying what constitutes a successful request. All status codes that
// indicate a successful request must be explicitly specified if not relying on the default
// values.
message DefaultSuccessCriteria {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.http.admission_control.v2alpha.AdmissionControl."
"DefaultSuccessCriteria";

// If HTTP statuses are unspecified, defaults to 2xx.
repeated HttpStatusRange http_status = 1;

// GRPC status codes to consider as request successes. If unspecified, defaults to "OK".
repeated type.grpc.v3.GrpcStatus grpc_status = 2;
}

// If set to false, the admission control filter will operate as a pass-through filter. If the
// message is unspecified, the filter will be enabled.
config.core.v3.RuntimeFeatureFlag enabled = 1;

// Defines how a request is considered a success/failure.
oneof success_criteria {
option (validate.required) = true;

DefaultSuccessCriteria default_success_criteria = 2;
}

// The time window over which the success rate is calculated. The window is rounded to the nearest
// second. Defaults to 120s.
google.protobuf.Duration sampling_window = 3;

// Rejection probability is defined by the formula::
//
// max(0, (rq_count - aggression_coefficient * rq_success_count) / (rq_count + 1))
//
// The coefficient dictates how aggressively the admission controller will throttle requests as
// the success rate drops. Lower values will cause throttling to kick in at higher success rates
// and result in more aggressive throttling. Any values less than 1.0, will be set to 1.0. If the
// message is unspecified, the coefficient is 2.0.
config.core.v3.RuntimeDouble aggression_coefficient = 4;
}
9 changes: 9 additions & 0 deletions api/envoy/type/grpc/v2/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# DO NOT EDIT. This file is generated by tools/proto_sync.py.

load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package")

licenses(["notice"]) # Apache 2

api_proto_package(
deps = ["@com_github_cncf_udpa//udpa/annotations:pkg"],
)
40 changes: 40 additions & 0 deletions api/envoy/type/grpc/v2/grpc_status.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
syntax = "proto3";

package envoy.type.grpc.v2;

import "udpa/annotations/migrate.proto";
import "validate/validate.proto";

option java_package = "io.envoyproxy.envoy.type.grpc.v2";
option java_outer_classname = "GrpcStatusProto";
option java_multiple_files = true;
option (udpa.annotations.file_migrate).move_to_package = "envoy.type.grpc.v3";

// [#protodoc-title: GRPC status codes]

// GRPC response codes supported.
enum Status {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lizan does this exist anywhere else? Any thoughts on defining this in our API?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this doesn't exist. I don't think we should define this as enum but just as int32 (that's what google.rpc.Status does) because in data plane there will be custom defined error codes as well.

OK = 0;
CANCELED = 1;
UNKNOWN = 2;
INVALID_ARGUMENT = 3;
DEADLINE_EXCEEDED = 4;
NOT_FOUND = 5;
ALREADY_EXISTS = 6;
PERMISSION_DENIED = 7;
RESOURCE_EXHAUSTED = 8;
FAILED_PRECONDITION = 9;
ABORTED = 10;
OUT_OF_RANGE = 11;
UNIMPLEMENTED = 12;
INTERNAL = 13;
UNAVAILABLE = 14;
DATA_LOSS = 15;
UNAUTHENTICATED = 16;
}

// GRPC status.
message GrpcStatus {
// Supplies GRPC response code.
Status status = 1 [(validate.rules).enum = {defined_only: true not_in: 0}];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the not_in 0 do?

}
12 changes: 12 additions & 0 deletions api/envoy/type/grpc/v3/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# DO NOT EDIT. This file is generated by tools/proto_sync.py.

load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package")

licenses(["notice"]) # Apache 2

api_proto_package(
deps = [
"//envoy/type/grpc/v2:pkg",
"@com_github_cncf_udpa//udpa/annotations:pkg",
],
)
42 changes: 42 additions & 0 deletions api/envoy/type/grpc/v3/grpc_status.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
syntax = "proto3";

package envoy.type.grpc.v3;

import "udpa/annotations/versioning.proto";

import "validate/validate.proto";

option java_package = "io.envoyproxy.envoy.type.grpc.v3";
option java_outer_classname = "GrpcStatusProto";
option java_multiple_files = true;

// [#protodoc-title: GRPC status codes]

// GRPC response codes supported.
enum Status {
OK = 0;
CANCELED = 1;
UNKNOWN = 2;
INVALID_ARGUMENT = 3;
DEADLINE_EXCEEDED = 4;
NOT_FOUND = 5;
ALREADY_EXISTS = 6;
PERMISSION_DENIED = 7;
RESOURCE_EXHAUSTED = 8;
FAILED_PRECONDITION = 9;
ABORTED = 10;
OUT_OF_RANGE = 11;
UNIMPLEMENTED = 12;
INTERNAL = 13;
UNAVAILABLE = 14;
DATA_LOSS = 15;
UNAUTHENTICATED = 16;
}

// GRPC status.
message GrpcStatus {
option (udpa.annotations.versioning).previous_message_type = "envoy.type.grpc.v2.GrpcStatus";

// Supplies GRPC response code.
Status status = 1 [(validate.rules).enum = {defined_only: true not_in: 0}];
}
2 changes: 2 additions & 0 deletions generated_api_shadow/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ proto_library(
"//envoy/config/filter/dubbo/router/v2alpha1:pkg",
"//envoy/config/filter/fault/v2:pkg",
"//envoy/config/filter/http/adaptive_concurrency/v2alpha:pkg",
"//envoy/config/filter/http/admission_control/v2alpha:pkg",
"//envoy/config/filter/http/buffer/v2:pkg",
"//envoy/config/filter/http/compressor/v2:pkg",
"//envoy/config/filter/http/cors/v2:pkg",
Expand Down Expand Up @@ -129,6 +130,7 @@ proto_library(
"//envoy/extensions/common/tap/v3:pkg",
"//envoy/extensions/filters/common/fault/v3:pkg",
"//envoy/extensions/filters/http/adaptive_concurrency/v3:pkg",
"//envoy/extensions/filters/http/admission_control/v3alpha:pkg",
"//envoy/extensions/filters/http/buffer/v3:pkg",
"//envoy/extensions/filters/http/compressor/v3:pkg",
"//envoy/extensions/filters/http/cors/v3:pkg",
Expand Down
Loading