Skip to content

Conversation

@rudrakhp
Copy link
Member

What type of PR is this?

Feat: support mixed address types across BackendRefs in xRoute

What this PR does / why we need it:

To support multiple address types across BackendRefs in xRoute, create XDS cluster per BackendRef.

Which issue(s) this PR fixes:

Related #5667

Release Notes: Yes/No

@rudrakhp rudrakhp requested a review from a team as a code owner April 11, 2025 11:57
@rudrakhp rudrakhp force-pushed the support_mixed_address_types_route branch 2 times, most recently from b477879 to 461782f Compare April 11, 2025 12:36
@codecov
Copy link

codecov bot commented Apr 11, 2025

Codecov Report

Attention: Patch coverage is 97.05882% with 1 line in your changes missing coverage. Please review.

Project coverage is 65.08%. Comparing base (096cb8d) to head (1d45a6f).
Report is 62 commits behind head on main.

Files with missing lines Patch % Lines
internal/ir/xds.go 95.65% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5716      +/-   ##
==========================================
- Coverage   65.19%   65.08%   -0.12%     
==========================================
  Files         214      219       +5     
  Lines       34321    34839     +518     
==========================================
+ Hits        22377    22674     +297     
- Misses      10591    10779     +188     
- Partials     1353     1386      +33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@rudrakhp rudrakhp force-pushed the support_mixed_address_types_route branch from 461782f to cafd52c Compare April 11, 2025 21:16
@rudrakhp rudrakhp requested a review from a team April 14, 2025 17:29
@arkodg
Copy link
Contributor

arkodg commented Apr 14, 2025

looks good @rudrakhp, thanks ! added one comment to improve my understanding of the code base

@rudrakhp rudrakhp force-pushed the support_mixed_address_types_route branch 2 times, most recently from 92ce357 to 6aba619 Compare April 15, 2025 14:50
@rudrakhp
Copy link
Member Author

rudrakhp commented Apr 15, 2025

Potential flake (run):

--- FAIL: TestPermissionCache (0.20s)
    --- FAIL: TestPermissionCache/Cached_permission_should_be_removed_after_expiry (0.02s)
        premissioncache_test.go:175: 
            	Error Trace:	/home/runner/work/gateway/gateway/internal/wasm/premissioncache_test.go:175
            	Error:      	Should be true
            	Test:       	TestPermissionCache/Cached_permission_should_be_removed_after_expiry
            	Messages:   	expired entry should be added after recheck
FAIL

@rudrakhp rudrakhp requested a review from arkodg April 15, 2025 17:58
@zhaohuabing
Copy link
Member

zhaohuabing commented Apr 15, 2025

PR #5725 includes a major refactor of the HTTPRoute status logic. Would it be okay to wait until that's merged before moving forward with this one? Thanks!

@arkodg
Copy link
Contributor

arkodg commented Apr 24, 2025

hey @rudrakhp now that refactor is in, can you rebase ?

@arkodg arkodg added this to the v1.4.0-rc.1 milestone Apr 24, 2025
@rudrakhp rudrakhp force-pushed the support_mixed_address_types_route branch 2 times, most recently from d5c3903 to eed3497 Compare April 24, 2025 21:03
@rudrakhp rudrakhp force-pushed the support_mixed_address_types_route branch from eed3497 to 1d45a6f Compare April 24, 2025 21:29
@rudrakhp
Copy link
Member Author

@arkodg rebased and moved some bits, also do let me know if the first comment is resolved. Thanks!

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team April 24, 2025 21:50
@arkodg arkodg merged commit 162e130 into envoyproxy:main Apr 30, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants