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

fix: add check for existence of HTTPRoute in KongUpstreamPolicy controller #5780

Merged

Conversation

randmonkey
Copy link
Contributor

@randmonkey randmonkey commented Mar 29, 2024

What this PR does / why we need it:

Add check of existence of HTTPRoutes in KongUpstreamPolicy controller and only enable the ancestor status update of related KongUpstreamPolicy when HTTPRoute CRD exists before KIC starts.
Removes the dependency of HTTPRoute of KongUpstreamPolicy.

Which issue this PR fixes:

fixes #5729

Special notes for your reviewer:

We may need to backport it to 3.1.x.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@randmonkey randmonkey force-pushed the fix/kongupstreampolicy_does_not_dependent_on_httproute branch from 106b560 to 0b4e91a Compare March 29, 2024 10:19
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 86.20690% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 73.9%. Comparing base (d82716c) to head (d9c4984).
Report is 7 commits behind head on main.

Files Patch % Lines
...ers/configuration/kongupstreampolicy_controller.go 71.4% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #5780     +/-   ##
=======================================
- Coverage   73.9%   73.9%   -0.1%     
=======================================
  Files        176     176             
  Lines      18187   18185      -2     
=======================================
- Hits       13456   13452      -4     
- Misses      3734    3736      +2     
  Partials     997     997             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@randmonkey randmonkey marked this pull request as ready for review March 29, 2024 12:55
@randmonkey randmonkey requested a review from a team as a code owner March 29, 2024 12:55
@randmonkey randmonkey marked this pull request as draft March 29, 2024 12:56
@randmonkey randmonkey force-pushed the fix/kongupstreampolicy_does_not_dependent_on_httproute branch from 0b4e91a to ea72a43 Compare April 1, 2024 03:19
@randmonkey randmonkey marked this pull request as ready for review April 1, 2024 03:47
Copy link
Contributor

@czeslavo czeslavo left a comment

Choose a reason for hiding this comment

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

I think we should add an envtest in which we ensure that KongUpstreamPolicies used with Services and Ingress work as expected when Gateway API CRDs are not installed. I'm afraid this could still be a problem because of this HTTPRoute list in the original reconciler.

internal/manager/controllerdef.go Outdated Show resolved Hide resolved
internal/manager/controllerdef.go Outdated Show resolved Hide resolved
@randmonkey randmonkey marked this pull request as draft April 2, 2024 10:13
@randmonkey randmonkey marked this pull request as ready for review April 2, 2024 10:13
@pull-request-size pull-request-size bot added size/XL and removed size/M labels Apr 3, 2024
@randmonkey randmonkey force-pushed the fix/kongupstreampolicy_does_not_dependent_on_httproute branch from 971593a to 20b9b6e Compare April 3, 2024 10:13
@randmonkey
Copy link
Contributor Author

I think we should add an envtest in which we ensure that KongUpstreamPolicies used with Services and Ingress work as expected when Gateway API CRDs are not installed. I'm afraid this could still be a problem because of this HTTPRoute list in the original reconciler.

Removed the extra dynamic controller and added a CRDExists check for HTTPRoute. When HTTPRoute exists, KongUpstreamPolicy controller enables features requiring HTTPRoute. Also added test cases in envtest to verify that KongUpstreamPolicy works both with and without HTTPRoute.

@randmonkey randmonkey requested a review from czeslavo April 3, 2024 10:20
@randmonkey randmonkey changed the title fix: add HTTPRoute controller for watching HTTPRoute to enqueue KongUpstreamPolicy needing to update ancestor status fix: add check for existence of HTTPRoute in KongUpstreamPolicy controller Apr 3, 2024
@randmonkey randmonkey mentioned this pull request Apr 3, 2024
29 tasks
@czeslavo czeslavo added this to the KIC v3.1.x milestone Apr 3, 2024
@czeslavo czeslavo enabled auto-merge (squash) April 3, 2024 17:02
@czeslavo czeslavo disabled auto-merge April 3, 2024 17:02
@czeslavo czeslavo enabled auto-merge (squash) April 3, 2024 17:03
@czeslavo czeslavo merged commit 30a2991 into main Apr 3, 2024
58 checks passed
@czeslavo czeslavo deleted the fix/kongupstreampolicy_does_not_dependent_on_httproute branch April 3, 2024 17:33
@team-k8s-bot
Copy link
Collaborator

The backport to release/3.1.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release/3.1.x release/3.1.x
# Navigate to the new working tree
cd .worktrees/backport-release/3.1.x
# Create a new branch
git switch --create backport-5780-to-release/3.1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 30a2991056044636804da46c7847b44e9497de50
# Push it to GitHub
git push --set-upstream origin backport-5780-to-release/3.1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release/3.1.x

Then, create a pull request where the base branch is release/3.1.x and the compare/head branch is backport-5780-to-release/3.1.x.

czeslavo pushed a commit that referenced this pull request Apr 3, 2024
…oller (#5780)

* add HTTPRoute controller for watching HTTPRoute to enqueue KongUpstreamPolicy needing to update ancestor status

* add CHANGELOG

* do not use dynamic CRD controller and and envtest cases

* Apply suggestions from code review

---------

Co-authored-by: Grzegorz Burzyński <czeslavo@gmail.com>
(cherry picked from commit 30a2991)
czeslavo pushed a commit that referenced this pull request Apr 3, 2024
…oller (#5780)

* add HTTPRoute controller for watching HTTPRoute to enqueue KongUpstreamPolicy needing to update ancestor status

* add CHANGELOG

* do not use dynamic CRD controller and and envtest cases

* Apply suggestions from code review

---------

Co-authored-by: Grzegorz Burzyński <czeslavo@gmail.com>
(cherry picked from commit 30a2991)
czeslavo added a commit that referenced this pull request Apr 3, 2024
… KongUpstreamPolicy controller (#5806)

* fix: add check for existence of HTTPRoute in KongUpstreamPolicy controller (#5780)

* add HTTPRoute controller for watching HTTPRoute to enqueue KongUpstreamPolicy needing to update ancestor status

* add CHANGELOG

* do not use dynamic CRD controller and and envtest cases

* Apply suggestions from code review

---------

Co-authored-by: Grzegorz Burzyński <czeslavo@gmail.com>
(cherry picked from commit 30a2991)

* Update CHANGELOG.md

---------

Co-authored-by: Tao Yi <tao.yi@konghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KongUpstreamPolicy controller depends on HTTPRoute CRD presence
3 participants