Skip to content

gateway-api: Fix Gateway reconciler failure when TLSRoute CRD is not installed #38874

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
Merged
merged 1 commit into from
Apr 18, 2025
Merged

gateway-api: Fix Gateway reconciler failure when TLSRoute CRD is not installed #38874

merged 1 commit into from
Apr 18, 2025

Conversation

syedazeez337
Copy link
Contributor

Description

This PR fixes an issue where the Gateway API reconciler fails when the experimental TLSRoute CRD is not installed. According to the documentation, the experimental Gateway API CRDs are not required if TLSRoutes are not going to be used. However, the current implementation tries to list TLSRoutes unconditionally, which fails with an error when the CRD is not installed.

The fix adds a check to determine if the TLSRoute CRD is supported in the cluster before attempting to list TLSRoutes. When the CRD is not installed, the Gateway reconciler now uses an empty TLSRouteList instead of failing with an error.

Related issues

Fixes: #38420

How has this been tested

I've tested this by:

  1. Creating a unit test for the HasTLSRouteSupport helper function
  2. Verifying that the Gateway reconciler works correctly when the TLSRoute CRD is not installed

Checklist

@syedazeez337 syedazeez337 requested a review from a team as a code owner April 10, 2025 18:26
@syedazeez337 syedazeez337 requested a review from sayboras April 10, 2025 18:26
@maintainer-s-little-helper
Copy link

Commit 10bd55d does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 10, 2025
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Apr 10, 2025
@syedazeez337 syedazeez337 force-pushed the fix-gateway-tlsroute-crd branch from 10bd55d to 0661bfa Compare April 10, 2025 18:37
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 10, 2025
@maintainer-s-little-helper
Copy link

Commit c772fb0 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 13, 2025
@moinuddin14
Copy link

/test

@syedazeez337 syedazeez337 force-pushed the fix-gateway-tlsroute-crd branch from c772fb0 to 8b7a0af Compare April 14, 2025 03:19
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 14, 2025
@syedazeez337
Copy link
Contributor Author

/release-note-bug-fix

@sayboras sayboras added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Apr 14, 2025
@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 Apr 14, 2025
@sayboras sayboras force-pushed the fix-gateway-tlsroute-crd branch from 8b7a0af to 14e22b2 Compare April 14, 2025 11:07
sayboras
sayboras reviewed Apr 14, 2025
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.

Thanks for your PR, I have couple of comments as per below.

@maintainer-s-little-helper
Copy link

Commits 5d1fdd2, 954c9cd do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 14, 2025
@syedazeez337
Copy link
Contributor Author

Thank you @sayboras for your feedback! I've implemented your suggested changes to simplify the code:

  1. Always initialize tlsRouteList as an empty list
  2. Only perform the List operation if the TLSRoute CRD is supported
  3. Remove conditional checks when filtering TLSRoutes since an empty list will result in empty filtered results

This approach maintains the core functionality while making the code cleaner and more consistent with the existing codebase style.

Let me know if you'd like me to make any additional changes!

@syedazeez337 syedazeez337 force-pushed the fix-gateway-tlsroute-crd branch from 954c9cd to 3db9a7e Compare April 14, 2025 15:49
@maintainer-s-little-helper
Copy link

Commit 5d1fdd2 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@syedazeez337 syedazeez337 force-pushed the fix-gateway-tlsroute-crd branch from 3db9a7e to 55b86b1 Compare April 14, 2025 16:00
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 14, 2025
@maintainer-s-little-helper
Copy link

Commit b61ae4f does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 14, 2025
@syedazeez337 syedazeez337 force-pushed the fix-gateway-tlsroute-crd branch from b61ae4f to f8d4f61 Compare April 14, 2025 16:47
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 14, 2025
@maintainer-s-little-helper
Copy link

Commit e171692 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 14, 2025
@syedazeez337
gateway-api: Fix Gateway reconciler failure when TLSRoute CRD is not …
eacfb91 
…installed

Signed-off-by: Syed Azeez <syedazeez337@gmail.com>
@syedazeez337 syedazeez337 force-pushed the fix-gateway-tlsroute-crd branch from e171692 to eacfb91 Compare April 14, 2025 17:07
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 14, 2025
@youngnick
Copy link
Contributor

This LGTM but we won't be able to merge it without the "Signed-off-by:".

I use

git commit --amend --sign

To update that when I forget, personally.

@sayboras sayboras self-requested a review April 16, 2025 06:36
sayboras
sayboras approved these changes Apr 16, 2025
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.

Thanks and LGTM ✔️

@sayboras
Copy link
Member

/test

@aditighag aditighag enabled auto-merge April 18, 2025 17:32
@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 Apr 18, 2025
@aditighag aditighag added this pull request to the merge queue Apr 18, 2025
Merged via the queue into cilium:main with commit 10204f3 Apr 18, 2025
68 of 69 checks passed
@liyihuang
Copy link
Contributor

Just some FYI, I created a follow up PR to enhance this one

@liyihuang liyihuang mentioned this pull request May 1, 2025
Merged
11 tasks
@pippolo84 pippolo84 added the needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch label May 5, 2025
@pippolo84
Copy link
Member

Marked this as eligible for backporting to v1.17, see this message for more info.

@sayboras sayboras mentioned this pull request May 7, 2025
Merged
9 tasks
@sayboras sayboras added backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. and removed needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch labels May 7, 2025
@github-actions github-actions bot added backport-done/1.17 The backport for Cilium 1.17.x for this PR is done. and removed backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. labels May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sayboras sayboras sayboras approved these changes

Assignees
No one assigned
Labels
backport-done/1.17 The backport for Cilium 1.17.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
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Gateway reconciler fails if experimental TLSRoute CRD is not installed
7 participants
@syedazeez337 @moinuddin14 @youngnick @sayboras @liyihuang @pippolo84 @aditighag