Skip to content

xds/extauthz: add clientInterceptor implementation for gRFC A92. - #9300

Open
Pranjali-2501 wants to merge 10 commits into
grpc:masterfrom
Pranjali-2501:a92-fitler-changes
Open

xds/extauthz: add clientInterceptor implementation for gRFC A92.#9300
Pranjali-2501 wants to merge 10 commits into
grpc:masterfrom
Pranjali-2501:a92-fitler-changes

Conversation

@Pranjali-2501

@Pranjali-2501 Pranjali-2501 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

This PR implements the client-side filter and interceptor logic along with unit and end-to-end tests for the ext_authz HTTP filter as specified in gRFC A92: xDS external authorization HTTP Filter. This PR also adds client-side metrics support for ext_authz.

RELEASE NOTES: N/A

@Pranjali-2501 Pranjali-2501 added this to the 1.84 Release milestone Aug 10, 2026
@Pranjali-2501 Pranjali-2501 added Type: Feature New features or improvements in behavior Area: xDS Includes everything xDS related, including LB policies used with xDS. labels Aug 10, 2026
@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.96680% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.13%. Comparing base (30ce1d5) to head (b4a9192).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
internal/xds/httpfilter/ext_authz/ext_authz.go 88.95% 13 Missing and 6 partials ⚠️
...rnal/xds/httpfilter/ext_authz/internal/internal.go 71.42% 5 Missing and 1 partial ⚠️
internal/testutils/stats/test_metrics_recorder.go 88.88% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9300      +/-   ##
==========================================
- Coverage   83.26%   83.13%   -0.14%     
==========================================
  Files         423      424       +1     
  Lines       35236    35482     +246     
==========================================
+ Hits        29341    29497     +156     
- Misses       4402     4458      +56     
- Partials     1493     1527      +34     
Files with missing lines Coverage Δ
internal/xds/httpfilter/extconfig.go 97.27% <100.00%> (+0.17%) ⬆️
internal/testutils/stats/test_metrics_recorder.go 79.06% <88.88%> (+1.79%) ⬆️
...rnal/xds/httpfilter/ext_authz/internal/internal.go 71.42% <71.42%> (ø)
internal/xds/httpfilter/ext_authz/ext_authz.go 89.20% <88.95%> (+9.20%) ⬆️

... and 39 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Pranjali-2501

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request implements the client-side filter and interceptor for the external authorization (ext_authz) HTTP filter in gRPC-Go, including refcounted channel sharing, header filtering, and header/response header mutations. Feedback is provided regarding a potential race condition in NewStream when incrementing the refcount of a closing channel, a timestamp inconsistency from calling time.Now() twice, and a potential nil-pointer panic in clientStream.Header() if the underlying stream returns nil metadata.

Comment on lines +381 to +385
// Increment authzClient's refcount so the Check RPC keeps the connection
// open even if the interceptor is closed concurrently. Decrement is deferred
// to release the reference when NewStream completes.
i.authzClient.Increment()
defer i.authzClient.Decrement()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

There is a potential race condition if Close() is called concurrently with NewStream. If Close() runs after the i.closed.Load() check but before i.authzClient.Increment(), the refcount can drop to 0 and trigger channel cleanup. Calling Increment() on a 0-refcount object is invalid and can lead to panics or use-after-free bugs. Using TryIncrement() instead ensures we safely attempt to increment the refcount and gracefully fail if the channel is already being cleaned up.

Suggested change
// Increment authzClient's refcount so the Check RPC keeps the connection
// open even if the interceptor is closed concurrently. Decrement is deferred
// to release the reference when NewStream completes.
i.authzClient.Increment()
defer i.authzClient.Decrement()
// Increment authzClient's refcount so the Check RPC keeps the connection
// open even if the interceptor is closed concurrently. Decrement is deferred
// to release the reference when NewStream completes.
if !i.authzClient.TryIncrement() {
return nil, status.Errorf(codes.Unavailable, "extauthz: interceptor is closed")
}
defer i.authzClient.Decrement()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The DocString for TryIncrement says "to avoid compareAndSwap overhead, it should not be used in the hot path".

@easwars and @mbissa , What are your thoughts on this?

Comment thread internal/xds/httpfilter/ext_authz/ext_authz.go Outdated
Comment thread internal/xds/httpfilter/ext_authz/ext_authz.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: xDS Includes everything xDS related, including LB policies used with xDS. Type: Feature New features or improvements in behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants