ext_authz: Avoid calling check multiple times by dio · Pull Request #13288 · envoyproxy/envoy

dio · 2020-09-28T00:23:12Z

Commit Message: This patch makes sure the filter sends exactly only one check request when the buffer is full, while there might be more data to be decoded.

Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A
Fixes #13260

Signed-off-by: Dhi Aurrahman dio@tetrate.io

This patch makes sure the filter sends check request once when buffer is full, while there might be more data to be decoded. Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

junr03 · 2020-09-29T16:48:52Z

@dio @brectanus-sigsci reported new evidence on the related issue. Can you take a look? I can review after that is settled

/wait

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio · 2020-09-30T05:38:59Z

/retest

repokitteh-read-only · 2020-09-30T05:39:04Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13288 (comment) was created by @dio.

see: more, trace.

brectanus-sigsci · 2020-10-07T15:52:34Z

@dio @brectanus-sigsci reported new evidence on the related issue. Can you take a look? I can review after that is settled

/wait

@junr03 @dio Seems good to review now. Just pinging on this. Thanks!

junr03

lgtm. although, I am less familiar than I would like with the filter requirements. Just to confirm @brectanus-sigsci this patch fixes your issue and is the expected behavior?

test/extensions/filters/http/ext_authz/ext_authz_test.cc

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

mattklein123

One quick question, thanks for fixing!

/wait-any

mattklein123 · 2020-10-14T03:14:36Z

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

+  if (buffer_data_ && !skip_check_) {
+    // When the filter is asked to buffer the data but the buffer is full, it skips buffering more
+    // data for the next iteration.
+    buffer_data_ = !isBufferFull();
+  }


Can you just do buffer_data_ = false; here? When we continue after a pause aren't we done and never want to buffer again?

Yes, you're right. Thank you for this. Updated.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

mattklein123 · 2020-10-14T15:26:27Z

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

+    // After sending the out the check request, we don't need to buffer the data anymore.
+    buffer_data_ = false;


Sorry what I mean is just remove the if statement and universally set it to false. I think that should be fine?

/wait

Yeah, sorry, my fault. Pushed an update.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

brectanus-sigsci · 2020-10-14T19:20:14Z

@dio Can you create another docker image with the latest changes so I can retest?

dio · 2020-10-14T19:42:09Z

@brectanus-sigsci sorry for the delay, but here it is: docker pull dio123/envoy:ext-authz-13260-2.

brectanus-sigsci · 2020-10-14T21:31:26Z

@dio Retest. Still looks good. Thanks so much for getting this fixed!

mattklein123 · 2020-10-14T22:32:06Z

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

-    // After sending the out the check request, we don't need to buffer the data anymore.
-    buffer_data_ = false;
-  }
+  // After sending the out the check request, we don't need to buffer the data anymore.


Suggested change

// After sending the out the check request, we don't need to buffer the data anymore.

// After sending the check request, we don't need to buffer the data anymore.

/wait

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

mattklein123

Thanks!

mattklein123 · 2020-10-15T15:24:03Z

Sorry merge main one more time?

/wait

mattklein123 · 2020-10-15T22:36:29Z

Merge main once #13598 merges. Thanks!

/wait

lizan · 2020-10-15T23:45:01Z

/retest

repokitteh-read-only · 2020-10-15T23:45:07Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

* master: (22 commits) delay health checks until transport socket secrets are ready. (envoyproxy#13516) test, oauth2: Make sure config test runs field validation (envoyproxy#13496) [http] swap codec implementations to default new (envoyproxy#13579) wasm: update proxy-wasm-cpp-host (envoyproxy#13606) postgres: do not copy and linearize received data when it is not going to be used (envoyproxy#13393) configs: Update configs v2 -> v3 (envoyproxy#13562) http2: Remove RELEASE_ASSERTs in sendPendingFrames() error handling (envoyproxy#13546) dependencies: track untracked implied dependencies, wrapup dashboard. (envoyproxy#13571) listener: add match all filter chain (envoyproxy#13449) fix mistakes in docstrings (envoyproxy#13603) ratelimit: add route entry metadata to ratelimit actions (envoyproxy#13269) cluster manager: avoid immediate activation for dynamic inserted cluster when initialize (envoyproxy#12783) ext_authz: Avoid calling check multiple times (envoyproxy#13288) docs: Unexclude remaining configs from validation (envoyproxy#13534) build: update rules_rust to allow Rustc in RBE (envoyproxy#13595) docs: Update sphinxext.rediraffe (envoyproxy#13589) Deprecate moonjit support on Windows before beta (envoyproxy#13541) dependencies: bump LuaJIT to 2.1 branch HEAD @ e9af1ab. (envoyproxy#13474) docs: add TLS stats to cluster stats doc (envoyproxy#13561) ci: stop building alpine-debug images in favor of ubuntu-based debug image (envoyproxy#13598) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

ext_authz: Avoid calling check multiple times

9170b5a

This patch makes sure the filter sends check request once when buffer is full, while there might be more data to be decoded. Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio mentioned this pull request Sep 28, 2020

ext_authz gRPC with a request body may be called multiple times for the same request #13260

Closed

mattklein123 assigned junr03 Sep 28, 2020

Merge remote-tracking branch 'upstream/master'

9ec4b4b

dio marked this pull request as ready for review September 28, 2020 04:57

repokitteh-read-only bot added the waiting label Sep 29, 2020

dio added 2 commits September 30, 2020 02:17

Fix

a119e1c

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Merge remote-tracking branch 'upstream/master'

63eeb70

repokitteh-read-only bot removed the waiting label Sep 30, 2020

junr03 reviewed Oct 7, 2020

View reviewed changes

test/extensions/filters/http/ext_authz/ext_authz_test.cc Outdated Show resolved Hide resolved

dio added 3 commits October 7, 2020 21:51

Review comment

9b86c49

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Merge remote-tracking branch 'upstream/master'

7d5666c

Merge remote-tracking branch 'upstream/master'

e5fdc1a

junr03 previously approved these changes Oct 13, 2020

View reviewed changes

mattklein123 reviewed Oct 14, 2020

View reviewed changes

repokitteh-read-only bot added the waiting:any label Oct 14, 2020

mattklein123 self-assigned this Oct 14, 2020

Always false

be49c6c

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio dismissed junr03’s stale review via be49c6c October 14, 2020 04:25

repokitteh-read-only bot removed the waiting:any label Oct 14, 2020

mattklein123 reviewed Oct 14, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 14, 2020

Always always false

9933f6d

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

repokitteh-read-only bot removed the waiting label Oct 14, 2020

Merge remote-tracking branch 'upstream/master'

006a377

mattklein123 reviewed Oct 14, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 14, 2020

Comment

a27a465

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

repokitteh-read-only bot removed the waiting label Oct 14, 2020

mattklein123 approved these changes Oct 14, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master'

c81744b

repokitteh-read-only bot added the waiting label Oct 15, 2020

Merge remote-tracking branch 'upstream/master'

921f580

repokitteh-read-only bot removed the waiting label Oct 15, 2020

repokitteh-read-only bot added the waiting label Oct 15, 2020

lizan merged commit 2196214 into envoyproxy:master Oct 16, 2020

		// After sending the out the check request, we don't need to buffer the data anymore.
		buffer_data_ = false;

	// After sending the out the check request, we don't need to buffer the data anymore.
	// After sending the check request, we don't need to buffer the data anymore.

Conversation

dio commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junr03 commented Sep 29, 2020

Uh oh!

dio commented Sep 30, 2020

Uh oh!

repokitteh-read-only bot commented Sep 30, 2020

Uh oh!

brectanus-sigsci commented Oct 7, 2020

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

dio Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

dio Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

brectanus-sigsci commented Oct 14, 2020

Uh oh!

dio commented Oct 14, 2020

Uh oh!

brectanus-sigsci commented Oct 14, 2020

Uh oh!

mattklein123 Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

dio Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Oct 15, 2020

Uh oh!

mattklein123 commented Oct 15, 2020

Uh oh!

lizan commented Oct 15, 2020

Uh oh!

repokitteh-read-only bot commented Oct 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dio commented Sep 28, 2020 •

edited

Loading