ext-authz: fix missing UAEX flag on Denied CheckResponse

[pims]

This fixes a bug in which the UAEX flag is not set prior to calling
callback->sendLocalReply(...).

Commit Message: ext-authz: fix missing UAEX flag on Denied CheckResponse
Additional Description: See #18964
Risk Level: low
Testing: manual
Docs Changes: n/a
Release Notes: ext-authz: fix missing UAEX flag on Denied CheckResponse

Fixes #18964

[repokitteh-read-only]

Hi @pims, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #18965 was opened by pims.

see: more, trace.

[yanavlasov]

@pims thanks for fixing this. Can you also add a test case for the fixed problem, please?

[pims]

@yanavlasov thanks for taking a look at this PR.

I've updated the tests to reflect the new call sequence.
To expand a little bit on why this PR exists.

Original code

decoder_callbacks_->sendLocalReply(response->status_code, …);
decoder_callbacks_->streamInfo().setResponseFlag(
        StreamInfo::ResponseFlag::UnauthorizedExternalService);

My understanding is that the code intends to set the UnauthorizedExternalService response flag, but that flag is never visible in Access Logs.

After re-ordering the calls, the unit tests were failing.
In order to understand why, I looked at the fault filter for inspiration/confirmation that the expected behavior is:

// source/extensions/filters/http/fault/fault_filter.cc

decoder_callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::FaultInjected);
decoder_callbacks_->sendLocalReply(http_status_code, "fault filter abort", …);

// test/extensions/filters/http/fault/fault_filter_test.cc
EXPECT_CALL(decoder_filter_callbacks_.stream_info_,
            setResponseFlag(StreamInfo::ResponseFlag::DelayInjected));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
            filter_->decodeHeaders(request_headers_, false));

Following the above pattern, tests are now passing.

EXPECT_CALL(filter_callbacks_.stream_info_,
            setResponseFlag(Envoy::StreamInfo::ResponseFlag::UnauthorizedExternalService));
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
            filter_->decodeHeaders(request_headers_, false));

I'm very new to the Envoy code base, so please let me know if my assumptions aren't correct.
Thanks again for reviewing this PR.

[pims]

I forced push to fix the DCO as instructed in https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md#fixing-dco

[pims]

@dio @soulxu let me know if there’s anything I can do to make this PR worthy of merging.

[dio]

Looks good. Thank you. A small optional request:

And could you add an entry in release notes that we fixed the bug?

[dio]

This makes sense.

Optional: Mind to add a comment for future readers, that is why we set it prior to calling sendLocalReply.

[soulxu]

also can put setResponseFlag and sendLocalReply into a inline method, but currently this is enough for me.

[soulxu]

This is LGTM. Thanks you. and sorry for review late.

[soulxu]

also can put setResponseFlag and sendLocalReply into a inline method, but currently this is enough for me.

[pims]

@dio added a note to the release notes about this bug fix, following the format you used for the network filter bug fix.

@soulxu thanks for the review.

[soulxu]

still LGTM

[pims]

@dio gentle nudge. Anything else you think should be added to this PR for it to be merged?

I have found a similar bug in another filter, and was hoping to follow this PR’s template when submitting a bug fix.

[dio]

Looks good. Thanks!