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

EndpointSlice to IR Route Destinations #1494

Merged
merged 13 commits into from
Sep 26, 2023
Merged

EndpointSlice to IR Route Destinations #1494

merged 13 commits into from
Sep 26, 2023

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Jun 6, 2023

Relates to #1256

@arkodg arkodg requested a review from a team as a code owner June 6, 2023 03:56
@arkodg arkodg marked this pull request as draft June 6, 2023 03:57
@arkodg arkodg mentioned this pull request Jun 6, 2023
3 tasks
@arkodg
Copy link
Contributor Author

arkodg commented Jun 15, 2023

this issue is blocked on #1105 which is blocked on #1492, because we would like to provide a way to fallback to ClusterIP Loadbalancing support for users who are running on systems with EndpointSlice disabled or prefer L3 Loadbalancing via ClusterIP / kube-proxy before enabling EndpointSlice by default

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Jul 15, 2023
@arkodg arkodg added this to the 0.6.0-rc1 milestone Jul 27, 2023
@github-actions github-actions bot removed the stale label Jul 27, 2023
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #1494 (fc2c67b) into main (34d477f) will increase coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is 92.22%.

@@            Coverage Diff             @@
##             main    #1494      +/-   ##
==========================================
+ Coverage   65.44%   65.47%   +0.02%     
==========================================
  Files          88       88              
  Lines       13031    13097      +66     
==========================================
+ Hits         8528     8575      +47     
- Misses       3979     3993      +14     
- Partials      524      529       +5     
Files Coverage Δ
internal/cmd/egctl/translate.go 83.12% <100.00%> (+0.04%) ⬆️
internal/gatewayapi/filters.go 79.90% <100.00%> (ø)
internal/gatewayapi/helpers.go 83.52% <ø> (+2.39%) ⬆️
internal/gatewayapi/resource.go 64.58% <100.00%> (+7.08%) ⬆️
internal/gatewayapi/translator.go 97.33% <ø> (ø)
internal/ir/xds.go 75.51% <100.00%> (+1.82%) ⬆️
internal/xds/translator/accesslog.go 98.22% <100.00%> (+0.03%) ⬆️
internal/xds/translator/authentication.go 73.46% <100.00%> (+0.32%) ⬆️
internal/xds/translator/cluster.go 100.00% <100.00%> (ø)
internal/xds/translator/ratelimit.go 86.34% <100.00%> (+0.13%) ⬆️
... and 4 more

... and 5 files with indirect coverage changes

@arkodg arkodg removed the stale label Sep 19, 2023
@arkodg arkodg marked this pull request as ready for review September 19, 2023 20:17
arkodg added a commit to arkodg/gateway that referenced this pull request Sep 19, 2023
@arkodg arkodg requested review from a team, zhaohuabing, qicz and Xunzhuo and removed request for a team September 20, 2023 06:32
GatewayControllerName: egv1alpha1.GatewayControllerName,
GatewayClassName: v1beta1.ObjectName(resources.GatewayClass.Name),
GlobalRateLimitEnabled: true,
EndpointRoutingDisabled: true,
Copy link
Member

Choose a reason for hiding this comment

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

I understand that we want the routing behavior to be backward compatible, but I think using endpoints as default to route traffic allows EG to bypass kube-proxy's overhead and fully leverage Envoy's advanced capabilities: more LB algorithms, session sticky, etc.

Copy link
Contributor Author

@arkodg arkodg Sep 20, 2023

Choose a reason for hiding this comment

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

I have disabled this in egctl x translate, because if we enable it, it will be mandatory, and all translations will fail if EndpointSlice is not provided by the user, this config is usually generated by k8s and is not part of user config, I was planning on raising a separate GH issue on dealing with this convenience / correctness dilemma

zirain pushed a commit that referenced this pull request Sep 21, 2023
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.

LGTM. Just need to fix the lint and test.

Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
add another level of indirection in RouteDestination
called DestinationSetting

Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Comment on lines +433 to +436
ds := &ir.DestinationSetting{
Weight: ptr.To(uint32(1)),
Endpoints: []*ir.DestinationEndpoint{ir.NewDestEndpoint(host, uint32(port))},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be simplified to a function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can this refactor be done in a follow up ?
this PR is changing 255 files most of which are YAML, and will affect other PRs

Signed-off-by: Arko Dasgupta <[email protected]>
@arkodg arkodg merged commit a785be8 into envoyproxy:main Sep 26, 2023
18 checks passed
@arkodg arkodg deleted the ep-gapi branch September 26, 2023 23:16
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.

3 participants