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

GatewayAPI: Fix listener parsing #27695

Merged
merged 1 commit into from Aug 29, 2023
Merged

Conversation

Managarmrr
Copy link
Contributor

Previously, parsing the listeners of a Gateway object only worked correctly, if only one listener of a given type (HTTP or TLS) was present, as looping over them was incorrectly handled.

Fixes: #27533
Fixes: 677e8b4 ("Implement TLSRoute")

Fix Gateway managed services not exposing all ports

@Managarmrr Managarmrr requested a review from a team as a code owner August 25, 2023 09:06
@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 Aug 25, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Aug 25, 2023
@joestringer joestringer added release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Aug 25, 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 Aug 25, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.2 Aug 25, 2023
@joestringer
Copy link
Member

Hi @Managarmrr , thanks for the contribution. I can't speak for the details, will leave review up to the assigned CODEOWNERS. But I will point out one thing - Cilium relies on linear git history, so before accepting this PR the merge commit will need to be dropped. Consider either rebasing against tip of main + force pushing the PR to update, or use the drop-down from "update branch" at the bottom of the page to do a rebase against the main branch.

I've kicked off the initial testsuites to give CI feedback for now.

@joestringer
Copy link
Member

/test

Previously, parsing the listeners of a Gateway object only worked correctly,
if only one listener of a given type (HTTP or TLS) was present, as looping
over them was incorrectly handled.

Fixes: cilium#27533
Fixes: 677e8b4 ("Implement TLSRoute")

Signed-off-by: Patrick Reich <patrick@neodyme.io>
@Managarmrr
Copy link
Contributor Author

Hi @Managarmrr , thanks for the contribution. I can't speak for the details, will leave review up to the assigned CODEOWNERS. But I will point out one thing - Cilium relies on linear git history, so before accepting this PR the merge commit will need to be dropped. Consider either rebasing against tip of main + force pushing the PR to update, or use the drop-down from "update branch" at the bottom of the page to do a rebase against the main branch.

I've kicked off the initial testsuites to give CI feedback for now.

Thanks for the heads up, for some reson I expected GitHub to perform a rebase on main when I pressed that button. Fixed it now.

@joestringer
Copy link
Member

/test

Copy link
Member

@meyskens meyskens left a comment

Choose a reason for hiding this comment

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

Good catch! LGTM, thanks :)

@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 Aug 28, 2023
@meyskens meyskens added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Aug 28, 2023
@meyskens
Copy link
Member

(sorry for the label remove and add, pressed the wrong buttons i trust on the bot here)

@aditighag aditighag merged commit 042765e into cilium:main Aug 29, 2023
60 checks passed
@jibi jibi mentioned this pull request Sep 4, 2023
16 tasks
@jibi jibi added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Sep 4, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.2 Sep 4, 2023
@jibi jibi added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Sep 7, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.14 to Backport done to v1.14 in 1.14.2 Sep 7, 2023
@Managarmrr Managarmrr deleted the bugfix/27533 branch September 13, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. 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/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.14.2
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

Gateway's Listener to Service is missing Port in Service LoadBalancer
5 participants