Skip to content

Add Ratelimit API #1767

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Add Ratelimit API #1767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
567dd7d
add rate limit
hzxuzhonghu Nov 19, 2020
ab20dd7
auto gen
hzxuzhonghu Dec 2, 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
283 changes: 283 additions & 0 deletions kubernetes/customresourcedefinitions.gen.yaml
View file
Open in desktop

Large diffs are not rendered by default.

719 changes: 561 additions & 158 deletions mesh/v1alpha1/config.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

30 changes: 29 additions & 1 deletion mesh/v1alpha1/config.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

syntax = "proto3";

import "google/api/field_behavior.proto";
import "google/protobuf/duration.proto";
import "google/protobuf/wrappers.proto";
import "mesh/v1alpha1/proxy.proto";
Expand Down Expand Up @@ -536,8 +537,11 @@ message MeshConfig {
// Note, currently at most 1 extension provider is allowed.
repeated ExtensionProvider extension_providers = 57;

// Configuration for an external rate limit service.
RateLimitService service = 58;

// $hide_from_docs
// Next available field number: 58
// Next available field number: 59
reserved 1;
reserved "mixer_check_server";
reserved 2;
Expand Down Expand Up @@ -631,3 +635,27 @@ message Certificate {
// multiple DNS names.
repeated string dns_names = 2;
}

// RateLimitService describes the configuration for an external rate limit service provider.
message RateLimitService {
Copy link
Contributor

Choose a reason for hiding this comment

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

As usual: do we expect the rate limit service to be the same in all namespaces and pods ? No possible use case where different namespaces would use different rate services ?

Why not add it to ProxyConfig instead, and use the existing pattern of RemoteService ?

If you want to keep it in MeshConfig - maybe the ExtensionProvider (which was just added) would be needed, but I don't think we have an approved design on using ExtensionProvider.

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally, i think this should be global. It is easy to operate. I understand ProxyConfig is used to generate configs for proxy in proxy side. But rls is used in istiod. As ExtensionProvider , it is only for external authz.

// REQUIRED. Specifies the service that implements rate limit service.
// The format is "[<Namespace>/]<Hostname>". The <Hostname> is the full qualified host name in the Istio service
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we use this pattern in some configs, like Gateway, for a specific purpose ( delegation ).

Do we need this here ? If there is a strong use cases - I will request any implementation before we unhide this API will have test cases for each option. Maybe start with the simple solution first, of using a ServiceEntry/DestinationRule/etc in istio-system and not provide too much complexity of allowing arbitrary namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should not enforce it has to be in istio-system.

// registry defined by the Kubernetes service or ServiceEntry. The <Namespace> is the namespace of the Kubernetes
// service or ServiceEntry object, and can be omitted if the <Hostname> alone can decide the service unambiguously
// (normally this means there is only 1 such host name in the service registry).
//
// Example: "rls.foo.svc.cluster.local" or "bar/rls.example.com".
string host = 1 [(google.api.field_behavior) = REQUIRED];

// REQUIRED. Specifies the port of the service.
uint32 port = 2 [(google.api.field_behavior) = REQUIRED];

// The filter’s behaviour in case the rate limiting service does not respond back.
// When it is set to true, Envoy will allow traffic in case of communication failure
// between rate limiting service and the proxy.
bool fail_open = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Default bool value is false - i.e. fail close. I don't think that's correct for a rate service - I would rather have the setting be 'fail_closed', so the user needs to explicitly add it if he wants this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, this is the behavior of the legacy mixer.


// The timeout for the rate limit service RPC.
// If not set, this defaults to 20ms.
google.protobuf.Duration timeout = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

For all services we use in Istio, we need to document how auth is going to be performed. Are all rate services using standard Istio mTLS or do we need to support other mechanisms ?

Also, is this service subject to normal Istio APIs - DestinationRule for example ? If yes ( and I assume it should ), do we want to duplicate timeout here ?

Copy link
Member Author

@hzxuzhonghu hzxuzhonghu Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The timeout here is from the application view(envoy->rls). DR can be used, but it will require an additional config

}
34 changes: 30 additions & 4 deletions mesh/v1alpha1/istio.mesh.v1alpha1.gen.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
"h2UpgradePolicy": {
"$ref": "#/components/schemas/istio.mesh.v1alpha1.MeshConfig.H2UpgradePolicy"
},
"service": {
"$ref": "#/components/schemas/istio.mesh.v1alpha1.RateLimitService"
},
"outboundTrafficPolicy": {
"$ref": "#/components/schemas/istio.mesh.v1alpha1.MeshConfig.OutboundTrafficPolicy"
},
Expand Down Expand Up @@ -521,6 +524,29 @@
}
]
},
"istio.mesh.v1alpha1.RateLimitService": {
"description": "RateLimitService describes the configuration for an external rate limit service provider.",
"type": "object",
"properties": {
"host": {
"description": "REQUIRED. Specifies the service that implements rate limit service. The format is \"[\u003cNamespace\u003e/]\u003cHostname\u003e\". The \u003cHostname\u003e is the full qualified host name in the Istio service registry defined by the Kubernetes service or ServiceEntry. The \u003cNamespace\u003e is the namespace of the Kubernetes service or ServiceEntry object, and can be omitted if the \u003cHostname\u003e alone can decide the service unambiguously (normally this means there is only 1 such host name in the service registry).",
"type": "string",
"format": "string"
},
"port": {
"description": "REQUIRED. Specifies the port of the service.",
"type": "integer"
},
"timeout": {
"description": "The timeout for the rate limit service RPC. If not set, this defaults to 20ms.",
"type": "string"
},
"failOpen": {
"description": "The filter’s behaviour in case the rate limiting service does not respond back. When it is set to true, Envoy will allow traffic in case of communication failure between rate limiting service and the proxy.",
"type": "boolean"
}
}
},
"istio.mesh.v1alpha1.MeshConfig.AuthPolicy": {
"type": "string",
"enum": [
Expand Down Expand Up @@ -557,15 +583,15 @@
"type": "string",
"format": "string"
},
"failOpen": {
"description": "If true, the user request will be allowed even if the communication with the authorization service has failed, or if the authorization service has returned a HTTP 5xx error. Default is false and the request will be rejected with \"Forbidden\" response.",
"type": "boolean"
},
"pathPrefix": {
"description": "Sets a prefix to the value of authorization request header *Path*. For example, setting this to \"/check\" for an original user request at path \"/admin\" will cause the authorization check request to be sent to the authorization service at the path \"/check/admin\" instead of \"/admin\".",
"type": "string",
"format": "string"
},
"failOpen": {
"description": "If true, the user request will be allowed even if the communication with the authorization service has failed, or if the authorization service has returned a HTTP 5xx error. Default is false and the request will be rejected with \"Forbidden\" response.",
"type": "boolean"
},
"statusOnError": {
"description": "Sets the HTTP status that is returned to the client when there is a network error to the authorization service. The default status is \"403\" (HTTP Forbidden).",
"type": "string",
Expand Down
83 changes: 82 additions & 1 deletion mesh/v1alpha1/istio.mesh.v1alpha1.pb.html
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading