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

CFP: cilium ingress should have an option to set the number of trusted loadbalancer hops #27952

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

chaunceyjiang
Copy link
Member

  1. Add a new global config item that sets the number of trusted hops for external load balancers for Ingress (or Gateway API) config. Default to 0, which produces the current behavior.
  2. Plumb that value to the relevant Envoy configs so that it takes effect for all ingress listeners. (As opposed to Layer 7 policy enforcement listeners)

Fixes: #24292

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Fixes: #issue-number

cilium ingress should have an option to set the number of trusted loadbalancer hops

@chaunceyjiang chaunceyjiang requested review from a team as code owners September 5, 2023 14:36
@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 Sep 5, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Sep 5, 2023
@chaunceyjiang
Copy link
Member Author

/cc @christarazi @meyskens @joamaki Please take a look. Thanks~

Copy link
Contributor

@youngnick youngnick 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 this PR @chaunceyjiang, the changes look good, but could you add some tests in envoy_http_connection_manager_test.go and envoy_listener_test.go? Just basic ones that ensure that the default is set correctly, and setting a separate value is also correct?

@chaunceyjiang
Copy link
Member Author

could you add some tests in envoy_http_connection_manager_test.go and envoy_listener_test.go? Just basic ones that ensure that the default is set correctly, and setting a separate value is also correct?

Hi @youngnick Done. PATL. Thanks~

@youngnick youngnick added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Sep 11, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 11, 2023
(as per Envoy config xff_num_trusted_hops)

Fixes: cilium#24292

Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
@meyskens
Copy link
Member

Assigned @youngnick here for sig-servicemesh as my work cycles on this area are currently limited

@chaunceyjiang
Copy link
Member Author

@meyskens Thanks~

@chaunceyjiang
Copy link
Member Author

Hi @youngnick, could you help me with triggering E2E testing?

@chaunceyjiang
Copy link
Member Author

Hi, @joamaki Ready for review , Please take a look. Thanks~

@youngnick
Copy link
Contributor

/test

@chaunceyjiang
Copy link
Member Author

📋 Test Report
❌ 2/45 tests failed (2/288 actions), 10 tests skipped, 1 scenarios skipped:
Test [no-policies]:
  ❌ no-policies/pod-to-service/curl-2: cilium-test/client2-646b88fb9b-fms7m (10.0.1.220) -> cilium-test/echo-other-node (echo-other-node:8080)
Test [allow-all-except-world]:
  ❌ allow-all-except-world/pod-to-service/curl-2: cilium-test/client2-646b88fb9b-fms7m (10.0.1.220) -> cilium-test/echo-other-node (echo-other-node:8080)
Error: Process completed with exit code 1.

It seems GitHub is having troubles.

@youngnick
Copy link
Contributor

/test ci-ipsec-upgrade

@chaunceyjiang
Copy link
Member Author

chaunceyjiang commented Sep 18, 2023

/test ci-ipsec-upgrade

Thanks~ @youngnick It seems like this action has no effect. (The time does not match.)

image

@chaunceyjiang
Copy link
Member Author

Friendly ping @youngnick , Can you help me re-trigger the GitHub action again?

@lmb
Copy link
Contributor

lmb commented Sep 26, 2023

I've triggered a run for you. Since the check is not marked as required we can merge without it being green. What's still missing is review from @joamaki

@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 Oct 4, 2023
@joamaki joamaki merged commit ec91f1c into cilium:main Oct 4, 2023
60 of 61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/k8s-ingress kind/community-contribution This was a contribution made by a community member. 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
6 participants