Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a Name field to a IR Route Destination #1788

Merged
merged 14 commits into from
Aug 21, 2023

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Aug 11, 2023

@arkodg arkodg requested a review from a team as a code owner August 11, 2023 20:33
@arkodg arkodg marked this pull request as draft August 11, 2023 20:33
@arkodg arkodg added the release-note Indicates a required release note label Aug 16, 2023
@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #1788 (e706eac) into main (c5c675f) will increase coverage by 0.05%.
The diff coverage is 80.14%.

@@            Coverage Diff             @@
##             main    #1788      +/-   ##
==========================================
+ Coverage   65.05%   65.11%   +0.05%     
==========================================
  Files          86       86              
  Lines       12281    12316      +35     
==========================================
+ Hits         7989     8019      +30     
- Misses       3777     3782       +5     
  Partials      515      515              
Files Changed Coverage Δ
internal/ir/zz_generated.deepcopy.go 13.78% <38.46%> (+0.86%) ⬆️
internal/xds/translator/authentication.go 65.21% <66.66%> (-0.12%) ⬇️
internal/xds/translator/ratelimit.go 86.21% <66.66%> (-0.05%) ⬇️
internal/ir/xds.go 73.60% <70.37%> (+0.79%) ⬆️
internal/xds/translator/route.go 90.76% <83.33%> (+0.10%) ⬆️
internal/xds/translator/translator.go 78.24% <83.33%> (+1.90%) ⬆️
internal/gatewayapi/route.go 88.28% <98.48%> (+0.25%) ⬆️
internal/gatewayapi/filters.go 80.30% <100.00%> (+0.17%) ⬆️
internal/gatewayapi/helpers.go 80.38% <100.00%> (+0.15%) ⬆️
internal/xds/translator/accesslog.go 98.19% <100.00%> (-0.02%) ⬇️
... and 2 more

... and 1 file with indirect coverage changes

@arkodg arkodg marked this pull request as ready for review August 16, 2023 21:57
@arkodg
Copy link
Contributor Author

arkodg commented Aug 16, 2023

looks like I broke the Redirect conformance tests

2023-08-16T22:10:53.4357382Z     --- FAIL: TestGatewayAPIConformance/HTTPRouteRedirectHostAndStatus (60.04s)
2023-08-16T22:10:53.4358026Z     --- FAIL: TestGatewayAPIConformance/HTTPRouteRedirectPath (60.02s)
2023-08-16T22:10:53.4358767Z     --- FAIL: TestGatewayAPIConformance/HTTPRouteRedirectPortAndScheme (60.08s)
2023-08-16T22:10:53.4361651Z     --- FAIL: TestGatewayAPIConformance/HTTPRouteRedirectPort (60.02s)
2023-08-16T22:10:53.4362251Z     --- FAIL: TestGatewayAPIConformance/HTTPRouteRedirectScheme (60.02s)

looking into this rn

Copy link
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

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

Please correct me if I am wrong:

This PR creates one cluster per route rule, eliminating the duplicated clusters when a route is attached to multiple gateway listeners. However, if a backend service is referenced in more than one route, there still are duplicated clusters created for that service. Should we consider creating one cluster per backend service?

@arkodg
Copy link
Contributor Author

arkodg commented Aug 17, 2023

Please correct me if I am wrong:

This PR creates one cluster per route rule, eliminating the duplicated clusters when a route is attached to multiple gateway listeners. However, if a backend service is referenced in more than one route, there still are duplicated clusters created for that service. Should we consider creating one cluster per backend service?

good point, here's the thing, the xds cluster also contains other attributes such as loadbalancing and resiliency (outlier detection) that may be applied at a per xRoute per Rule level via a PolicyAttachment, so we need to create different clusters for those cases :)

AliceProxy
AliceProxy previously approved these changes Aug 17, 2023
Copy link
Member

@AliceProxy AliceProxy left a comment

Choose a reason for hiding this comment

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

Looks good to me and the xDS names are clean

zirain
zirain previously approved these changes Aug 18, 2023
@arkodg
Copy link
Contributor Author

arkodg commented Aug 18, 2023

lets wait another day for #1702 to go in, before this PR goes in, 1702 has already had to rebase a few times

@arkodg arkodg marked this pull request as draft August 18, 2023 00:16
@arkodg arkodg marked this pull request as ready for review August 20, 2023 16:16
* Allows us to reduce and reuse the number
of xds clusters created which are per xRoute per rule
Fixes: envoyproxy#551

* Removes the hostname from the xds cluster name
since the xds cluster name is no longer tied to the
ir route name
Fixes: envoyproxy#1785

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg requested a review from zirain August 21, 2023 05:31
@arkodg arkodg enabled auto-merge (squash) August 21, 2023 05:32
@arkodg arkodg merged commit 4285f4e into envoyproxy:main Aug 21, 2023
20 checks passed
@arkodg arkodg deleted the rm-hostname-ir-name branch August 21, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note Indicates a required release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prometheus stats not working with EG generated cluster names Per HttpRoute(API) cluster creation
4 participants