-
Notifications
You must be signed in to change notification settings - Fork 357
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
feat: add body to ext auth #4671
base: main
Are you sure you want to change the base?
feat: add body to ext auth #4671
Conversation
Signed-off-by: Aurélien Pillevesse <[email protected]>
Signed-off-by: Aurélien Pillevesse <[email protected]>
497ebb8
to
574815d
Compare
The testing part (https://gateway.envoyproxy.io/contributions/develop/#testing) seems not to work on my computer so I tried to modify Don't really know how to do it better. |
it would be good to send API first. |
From |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4671 +/- ##
==========================================
- Coverage 65.54% 65.51% -0.04%
==========================================
Files 211 211
Lines 31946 31953 +7
==========================================
- Hits 20940 20935 -5
- Misses 9762 9771 +9
- Partials 1244 1247 +3 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Aurélien Pillevesse <[email protected]>
528f373
to
9f63579
Compare
Signed-off-by: Aurélien Pillevesse <[email protected]>
c8baa00
to
3bd62e3
Compare
Signed-off-by: Aurélien Pillevesse <[email protected]>
internal/xds/translator/extauth.go
Outdated
config.WithRequestBody = &extauthv3.BufferSettings{ | ||
MaxRequestBytes: 1024, | ||
AllowPartialMessage: false, | ||
PackAsBytes: false, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arbitrary value (but must be greater than 0) for the MaxRequestBytes
field, given that the AllowPartialMessage
field is always equal to false.
Signed-off-by: Aurélien Pillevesse <[email protected]>
34067fd
to
1b6eb87
Compare
cc @guydc |
Signed-off-by: Aurélien Pillevesse <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good to me, just one comment on the docs and please rebase + fix test code generation
@@ -33,6 +33,10 @@ type ExtAuth struct { | |||
// +optional | |||
HeadersToExtAuth []string `json:"headersToExtAuth,omitempty"` | |||
|
|||
// BodyToExtAuth defines the Body to Ext Auth configuration. | |||
// +optional | |||
BodyToExtAuth *BodyToExtAuth `json:"bodyToExtAuth,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment about the maximum body size supported for this option? If I understand the docs correctly, when the body size is greater than 1024, the request would be blocked with status code 413 instead of being sent to ext-auth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my understanding, the maximum body size will not be take into account because in the BufferSettings
object, AllowPartialMessage
is hardcoded to false for the moment (so I put 1024
as random value).
extauthv3.BufferSettings{
MaxRequestBytes: 1024,
AllowPartialMessage: false,
PackAsBytes: false,
}
In my opinion, it's better the release this feature like that (so basic feature first) and in next iteration for this feature, allow the possibility for partial message.
So for the moment, all the body will be sent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm agree with that if the AllowPartialMessage
values is false by default I think we can open another PR about that. This feature is very important because now we cannot add a body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested the following config:
static_resources:
listeners:
- address:
socket_address:
address: 0.0.0.0
port_value: 8000
filter_chains:
- filters:
- name: envoy.filters.network.http_connection_manager
typed_config:
"@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
codec_type: AUTO
stat_prefix: ingress_http
route_config:
name: local_route
virtual_hosts:
- name: upstream
domains:
- "*"
typed_per_filter_config:
envoy.filters.http.ext_authz:
"@type": type.googleapis.com/envoy.extensions.filters.http.ext_authz.v3.ExtAuthzPerRoute
check_settings:
with_request_body:
max_request_bytes: 1
allow_partial_message: false
routes:
- match:
prefix: "/"
route:
cluster: upstream-service
http_filters:
- name: envoy.filters.http.ext_authz
typed_config:
"@type": type.googleapis.com/envoy.extensions.filters.http.ext_authz.v3.ExtAuthz
http_service:
server_uri:
uri: ext_authz
cluster: ext_authz-http-service
timeout: 0.250s
- name: envoy.filters.http.router
typed_config:
"@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
clusters:
- name: upstream-service
type: STRICT_DNS
lb_policy: ROUND_ROBIN
load_assignment:
cluster_name: upstream-service
endpoints:
- lb_endpoints:
- endpoint:
address:
socket_address:
address: upstream-service
port_value: 8080
- name: ext_authz-http-service
type: STRICT_DNS
lb_policy: ROUND_ROBIN
load_assignment:
cluster_name: ext_authz-http-service
endpoints:
- lb_endpoints:
- endpoint:
address:
socket_address:
address: ext_authz-http-service
port_value: 9002
Specifying a body large enough will lead to request being rejected with 413 without being inspected by the ext-auth server.
curl -I http://localhost:8000
HTTP/1.1 403 Forbidden
date: Fri, 22 Nov 2024 15:30:44 GMT
server: envoy
content-length: 0
No body: rejected due to fail closed (ext auth doesn't really exist in this example).
curl -X POST http://localhost:8000 -d "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
[...]
< HTTP/1.1 413 Payload Too Large
< content-length: 17
< content-type: text/plain
< date: Fri, 22 Nov 2024 15:33:56 GMT
< server: envoy
< connection: close
Payload Too Large
Large body: rejected before being processed by ext auth.
Signed-off-by: Aurélien Pillevesse <[email protected]>
Signed-off-by: Aurélien Pillevesse <[email protected]>
Signed-off-by: Aurélien Pillevesse <[email protected]>
Signed-off-by: Aurélien Pillevesse <[email protected]>
What type of PR is this?
Add gesture for body to ext auth
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #4431