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

feat: KongUpstreamPolicy accepted condition #5185

Merged
merged 5 commits into from
Dec 15, 2023

Conversation

mlavacca
Copy link
Member

@mlavacca mlavacca commented Nov 16, 2023

What this PR does / why we need it:

Which issue this PR fixes:

Fixes #4932
Special notes for your reviewer:

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

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: 135 lines in your changes are missing coverage. Please review.

Comparison is base (48ad16e) 73.5% compared to head (e972c1e) 75.8%.
Report is 3 commits behind head on main.

Files Patch % Lines
...ers/configuration/kongupstreampolicy_controller.go 29.5% 86 Missing and 7 partials ⚠️
...trollers/configuration/kongupstreampolicy_utils.go 71.2% 29 Missing and 13 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #5185     +/-   ##
=======================================
+ Coverage   73.5%   75.8%   +2.3%     
=======================================
  Files        170     172      +2     
  Lines      19584   19825    +241     
=======================================
+ Hits       14398   15039    +641     
+ Misses      4348    3933    -415     
- Partials     838     853     +15     

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

@mlavacca mlavacca force-pushed the mlavacca/kong-upstream-policy-status branch from 85ea9d6 to 8f51be1 Compare November 29, 2023 18:14
@mlavacca mlavacca changed the title feat: kongUpstreamPolicy status feat: KongUpstreamPolicy accepted condition Nov 30, 2023
@mlavacca mlavacca force-pushed the mlavacca/kong-upstream-policy-status branch 4 times, most recently from 741531c to 4ee9eff Compare December 6, 2023 09:51
@mlavacca mlavacca marked this pull request as ready for review December 6, 2023 10:10
@mlavacca mlavacca requested a review from a team as a code owner December 6, 2023 10:10
Copy link
Contributor

@randmonkey randmonkey left a comment

Choose a reason for hiding this comment

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

The implementation seems to assumpted that HTTPRoute is always present in the cluster, but gateway API are not installed by default so there is possibility that HTTPRoute does not exist in the cluster. I am not sure if this will cause unexpected behavior such as controller crash or endless reconcilng loop.

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.

Before merging, I think we should cover the reconciler with envtests to verify the status is populated as expected. We could also cover all the added helpers with unit tests (I'd highly encourage to do so).

Another important point is that KongUpstreamPolicy might be attached not only to Services that are used as HTTPRoutes' backends, but also with any other Gateway API Route that we support and netv1.Ingress as well. I suggest we create a separate issue to handle all other Routes and Ingress references similarly to how we handle HTTPRoute in this PR.

@mlavacca
Copy link
Member Author

I created some follow-up issues to this one to avoid having is a gigantic PR

Before merging, I think we should cover the reconciler with envtests to verify the status is populated as expected. We could also cover all the added helpers with unit tests (I'd highly encourage to do so).

Here is an issue about adding integration tests: #5292. In addition to that, I'll add some unit tests for the helpers in this PR. Does it sound good to you?

Another important point is that KongUpstreamPolicy might be attached not only to Services that are used as HTTPRoutes' backends, but also with any other Gateway API Route that we support and netv1.Ingress as well. I suggest we create a separate issue to handle all other Routes and Ingress references similarly to how we handle HTTPRoute in this PR.

And here is the issue about checking the ingress: #5293

@czeslavo
Copy link
Contributor

@mlavacca Yes, that makes sense to me, thanks for creating follow-up issues!

@mlavacca
Copy link
Member Author

The implementation seems to assumpted that HTTPRoute is always present in the cluster, but gateway API are not installed by default so there is possibility that HTTPRoute does not exist in the cluster. I am not sure if this will cause unexpected behavior such as controller crash or endless reconcilng loop.

The controller does not start unless the gateway API is installed in the cluster, as it is wrapped by the dynamicCRDcontroller

{
Enabled: c.KongUpstreamPolicyEnabled,
Controller: &crds.DynamicCRDController{
Manager: mgr,
Log: ctrl.LoggerFrom(ctx).WithName("controllers").WithName("Dynamic/KongUpstreamPolicy"),
CacheSyncTimeout: c.CacheSyncTimeout,
RequiredCRDs: []schema.GroupVersionResource{
{
Group: gatewayv1.GroupVersion.Group,
Version: gatewayv1.GroupVersion.Version,
Resource: "httproutes",
},
},
Controller: &configuration.KongUpstreamPolicyReconciler{
Client: mgr.GetClient(),
Log: ctrl.LoggerFrom(ctx).WithName("controllers").WithName("KongUpstreamPolicy"),
Scheme: mgr.GetScheme(),
DataplaneClient: dataplaneClient,
CacheSyncTimeout: c.CacheSyncTimeout,
},
},

@mlavacca mlavacca force-pushed the mlavacca/kong-upstream-policy-status branch 2 times, most recently from a61e0e5 to 9334fcf Compare December 12, 2023 17:49
@mlavacca mlavacca force-pushed the mlavacca/kong-upstream-policy-status branch 4 times, most recently from 2253f94 to 5297b5e Compare December 13, 2023 15:55
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
@mlavacca mlavacca force-pushed the mlavacca/kong-upstream-policy-status branch from 5297b5e to cd1bdb1 Compare December 13, 2023 16:01
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
@mlavacca mlavacca force-pushed the mlavacca/kong-upstream-policy-status branch from cd1bdb1 to 4c0520b Compare December 13, 2023 16:36
randmonkey
randmonkey previously approved these changes Dec 14, 2023
Co-authored-by: Grzegorz Burzyński <czeslavo@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
@mlavacca mlavacca merged commit d0141d5 into main Dec 15, 2023
37 checks passed
@mlavacca mlavacca deleted the mlavacca/kong-upstream-policy-status branch December 15, 2023 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Populate KongUpstreamPolicy status for every attached object in the controller
3 participants