ext_authz: allow headers propagation when allow_debugging_failures is set #24845

agrawroh · 2023-01-10T06:46:36Z

Background

Currently, the ext_authz filter [Link] doesn't propagate the headers sent from the upstream service back to the client in case of errors. These errors could be useful for debugging purposes. (See #14001)

Changes

This PR adds a new boolean field called allow_debugging_failures in ext_authz filter which could be used to control whether or not to propagate the response headers received from the upstream service in case of 5XX errors. It's defaulted to false to maintain the current behavior.

Commit Message: Allow upstream headers propagation when allow_debugging_failures is set for ext_authz filter.
Additional Description: Adds a boolean field in ext_authz filter to control whether or not the response headers received from upstream should be propagated back to the client in case of errors.
Risk Level: Low
Testing: Unit Tests
Docs Changes: Added description of this new field in the docs.
Release Notes: Added
Platform Specific Features: N/A

Signed-off-by: Rohit Agrawal rohit.agrawal@databricks.com

repokitteh-read-only · 2023-01-10T06:46:40Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #24845 was opened by agrawroh.

see: more, trace.

repokitteh-read-only · 2023-01-10T06:46:48Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #24845 was opened by agrawroh.

see: more, trace.

agrawroh · 2023-01-10T09:26:09Z

/retest

source/extensions/filters/http/ext_authz/ext_authz.cc

wbpcode · 2023-01-10T11:47:03Z

cc @mattklein123 for API change and quick review to this design.

Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>

mattklein123

High level question/comment to get started. Thank you!

/wait

mattklein123 · 2023-01-12T17:40:36Z

api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto

+  //
+  //  2. When set to false, the filter would return a 403 or `status_on_error` (if set) and would
+  //  not propagate any response body or headers back to the client. This is the default behavior.
+  bool allow_debugging_failures = 18;


I think I would name this propagate_response_on_failure ?

Also, is this feature HTTP specific as it seems to pivot on 5xx? Should this actually be part of the HttpService object vs. global?

What about adding a field analogous to allowed_headers for allowed_response_headers?

Also, is this feature HTTP specific as it seems to pivot on 5xx? Should this actually be part of the HttpService object vs. global?

Sounds reasonable.

Thanks, @mattklein123. That sounds reasonable to me as well.

@ggreenway Sorry, I'm not able to understand the intention behind adding allowed_response_header. Is the idea to limit what could be propagated back to the client when there are failures? We expose two lists allowed_client_headers and allowed_upstream_headers today which serves similar purpose.

Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>

…r-allow-headers

changelogs/current.yaml

…r-allow-headers

Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>

mattklein123

Looks like this is on the right track. A few comments, thanks.

/wait

mattklein123 · 2023-01-17T16:20:35Z

source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc

@@ -114,6 +114,9 @@ ClientConfig::ClientConfig(const envoy::extensions::filters::http::ext_authz::v3
          config.http_service().authorization_response().allowed_upstream_headers())),
      upstream_header_to_append_matchers_(toUpstreamMatchers(
          config.http_service().authorization_response().allowed_upstream_headers_to_append())),
+      propagate_response_on_failure_(config.has_http_service()


Nit: Since the default is false you don't need the has check. You can just directly get the value from the default http_service() object.

mattklein123 · 2023-01-17T16:21:38Z

api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto

@@ -250,6 +250,16 @@ message HttpService {

  // Settings used for controlling authorization response metadata.
  AuthorizationResponse authorization_response = 8;
+
+  //  This changes the filter's behaviour on errors:


The code appears to only do this for 5xx. Can you clarify this? Is there any reason not to do this for 4xx also? This seems generally useful as well?

mattklein123 · 2023-01-17T16:24:08Z

source/extensions/filters/http/ext_authz/ext_authz.h

+        propagate_response_on_failure_(config.has_http_service()
+                                           ? config.http_service().propagate_response_on_failure()
+                                           : false),


Same comment

mattklein123 · 2023-01-17T16:24:34Z

test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc

+// Verify that when a call to authorization server returns a 5xx and `propagate_response_on_failure`
+// is set then we preserve the original status code, headers, etc.


Please add an integration test for this.

mattklein123 · 2023-01-17T16:25:32Z

source/extensions/filters/http/ext_authz/ext_authz.cc

-      decoder_callbacks_->sendLocalReply(
-          config_->statusOnError(), EMPTY_STRING, nullptr, absl::nullopt,
-          Filters::Common::ExtAuthz::ResponseCodeDetails::get().AuthzError);
+      if (config_->propagateResponseOnFailure()) {


It seems odd to me that this logic is split across multiple files and that there is additional logic here for things like stats. Why do we additionally increment stats here? Also, if the header processing is done in the other file can't this file just stay a unified path?

lizan · 2023-01-20T00:55:40Z

/retest

repokitteh-read-only · 2023-01-20T00:55:49Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #24845 (comment) was created by @lizan.

see: more, trace.

github-actions · 2023-02-19T04:01:31Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2023-02-26T08:01:22Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

repokitteh-read-only bot added the api label Jan 10, 2023

repokitteh-read-only bot assigned lizan Jan 10, 2023

agrawroh force-pushed the extauthz-error-allow-headers branch 2 times, most recently from 7e6168e to f5fc138 Compare January 10, 2023 07:21

wbpcode reviewed Jan 10, 2023

View reviewed changes

source/extensions/filters/http/ext_authz/ext_authz.cc Outdated Show resolved Hide resolved

wbpcode assigned mattklein123 and wbpcode and unassigned lizan Jan 10, 2023

agrawroh force-pushed the extauthz-error-allow-headers branch from f5fc138 to 6021b8d Compare January 10, 2023 12:10

ext_authz: Allow headers propagation with allow_debugging_failures

6ccd206

Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>

agrawroh force-pushed the extauthz-error-allow-headers branch from 6021b8d to 6ccd206 Compare January 10, 2023 12:25

agrawroh marked this pull request as ready for review January 11, 2023 19:20

agrawroh requested a review from ggreenway as a code owner January 11, 2023 19:20

mattklein123 requested changes Jan 12, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 12, 2023

agrawroh added 2 commits January 12, 2023 18:43

Addressed Comments From @mattklein123

d8cbb9b

Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>

Merge branch 'main' of github.com:envoyproxy/envoy into extauthz-erro…

2754dd7

…r-allow-headers

repokitteh-read-only bot removed the waiting label Jan 13, 2023

wbpcode reviewed Jan 17, 2023

View reviewed changes

changelogs/current.yaml Outdated Show resolved Hide resolved

agrawroh added 2 commits January 16, 2023 22:50

Merge branch 'main' of github.com:envoyproxy/envoy into extauthz-erro…

ea16a3f

…r-allow-headers

Addressed Comments From @wbpcode

2e70f00

Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>

agrawroh requested review from mattklein123 and wbpcode and removed request for ggreenway, mattklein123 and wbpcode January 17, 2023 06:53

agrawroh requested a review from mattklein123 January 17, 2023 06:53

mattklein123 requested changes Jan 17, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 17, 2023

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 19, 2023

github-actions bot closed this Feb 26, 2023

mikehuston mentioned this pull request May 7, 2024

Support for sending response body, headers, and status code to client on Extauthz filter CheckStatus::Error #34005

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ext_authz: allow headers propagation when allow_debugging_failures is set #24845

ext_authz: allow headers propagation when allow_debugging_failures is set #24845

agrawroh commented Jan 10, 2023 •

edited

repokitteh-read-only bot commented Jan 10, 2023

repokitteh-read-only bot commented Jan 10, 2023

agrawroh commented Jan 10, 2023

wbpcode commented Jan 10, 2023

mattklein123 left a comment

mattklein123 Jan 12, 2023

ggreenway Jan 12, 2023

wbpcode Jan 13, 2023

agrawroh Jan 13, 2023

mattklein123 left a comment

mattklein123 Jan 17, 2023

mattklein123 Jan 17, 2023

mattklein123 Jan 17, 2023

mattklein123 Jan 17, 2023

mattklein123 Jan 17, 2023

lizan commented Jan 20, 2023

repokitteh-read-only bot commented Jan 20, 2023

github-actions bot commented Feb 19, 2023

github-actions bot commented Feb 26, 2023

		// Verify that when a call to authorization server returns a 5xx and `propagate_response_on_failure`
		// is set then we preserve the original status code, headers, etc.

ext_authz: allow headers propagation when allow_debugging_failures is set #24845

ext_authz: allow headers propagation when allow_debugging_failures is set #24845

Conversation

agrawroh commented Jan 10, 2023 • edited

Background

Changes

repokitteh-read-only bot commented Jan 10, 2023

repokitteh-read-only bot commented Jan 10, 2023

agrawroh commented Jan 10, 2023

wbpcode commented Jan 10, 2023

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan commented Jan 20, 2023

repokitteh-read-only bot commented Jan 20, 2023

github-actions bot commented Feb 19, 2023

github-actions bot commented Feb 26, 2023

agrawroh commented Jan 10, 2023 •

edited