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.

Sign up for GitHub

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

Jump to bottom

gateway-api: Supports the number of trusted loadbalancer hops #30662

Merged
merged 1 commit into from Mar 5, 2024
Merged

gateway-api: Supports the number of trusted loadbalancer hops #30662

merged 1 commit into from Mar 5, 2024

Conversation

chaunceyjiang
Copy link
Member

@chaunceyjiang chaunceyjiang commented Feb 7, 2024 •
edited

Currently, cilium supports setting xff_num_trusted_hops, but it only supports ingress.
This PR is to allow GatewayAPI to also set xff_num_trusted_hops.

In this way, we can set different values for the xff_num_trusted_hops of ingress and GatewayAPI.

Additionally, it seems that Istio already supports this feature.

https://istio.io/latest/docs/ops/configuration/traffic-management/network-topologies/

GatewayAPI supports to setting the number of trusted loadbalancer hops

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 7, 2024
@chaunceyjiang
Copy link
Member Author

/test

@chaunceyjiang chaunceyjiang marked this pull request as ready for review February 22, 2024 14:01
@chaunceyjiang chaunceyjiang requested review from a team as code owners February 22, 2024 14:01
@chaunceyjiang
Copy link
Member Author

/cc @sayboras PTAL. Thanks~

@sayboras sayboras changed the title GatewayAPI supports to setting the number of trusted loadbalancer hops gateway-api: Supports the number of trusted loadbalancer hops Feb 27, 2024
sayboras
sayboras approved these changes Feb 27, 2024
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

The change looks good to me, however, I am not sure if we should use the same flag for both Ingress and Gateway.

Let's see what other reviews say.

@sayboras sayboras added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/servicemesh GH issues or PRs regarding servicemesh feature/k8s-gateway-api labels Feb 27, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Feb 27, 2024
pippolo84
pippolo84 requested changes Feb 28, 2024
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR 🙏
A couple of suggestions left inline.

operator/pkg/model/translation/envoy_listener.go Show resolved Hide resolved
operator/option/config.go Outdated Show resolved Hide resolved
@sayboras sayboras added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Feb 28, 2024
@chaunceyjiang chaunceyjiang requested review from a team as code owners March 1, 2024 07:26
@chaunceyjiang
Copy link
Member Author

/test

@pippolo84 pippolo84 self-requested a review March 1, 2024 09:27
pippolo84
pippolo84 requested changes Mar 1, 2024
View reviewed changes
operator/pkg/gateway-api/cell.go Outdated Show resolved Hide resolved
@chaunceyjiang
GatewayAPI supports to setting the number of trusted loadbalancer hops
0ba8f63 
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
@chaunceyjiang
Copy link
Member Author

/test

pippolo84
pippolo84 approved these changes Mar 4, 2024
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 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!

gandro
gandro approved these changes Mar 4, 2024
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Helm LGTM!

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 5, 2024
@youngnick youngnick removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 5, 2024
@youngnick youngnick added this pull request to the merge queue Mar 5, 2024
Merged via the queue into cilium:main with commit 7c7ae03 Mar 5, 2024
62 checks passed
@chaunceyjiang chaunceyjiang deleted the xff branch March 5, 2024 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@pippolo84 pippolo84 pippolo84 approved these changes

@sayboras sayboras sayboras approved these changes

@youngnick youngnick youngnick approved these changes

Assignees
No one assigned
Labels
area/servicemesh GH issues or PRs regarding servicemesh feature/k8s-gateway-api ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@chaunceyjiang @gandro @pippolo84 @sayboras @youngnick