ratelimitpolicy v1beta3

[eguzki]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 92.50000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.30%. Comparing base (ece13e8) to head (bb71440).
Report is 223 commits behind head on main.

Files with missing lines	Patch %	Lines
api/v1beta3/topology.go	61.53%	5 Missing ⚠️
controllers/httprouteparentrefs_eventmapper.go	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #875      +/-   ##
==========================================
+ Coverage   80.20%   81.30%   +1.09%     
==========================================
  Files          64      102      +38     
  Lines        4492     7089    +2597     
==========================================
+ Hits         3603     5764    +2161     
- Misses        600      897     +297     
- Partials      289      428     +139     

Flag	Coverage Δ
bare-k8s-integration	8.28% <3.75%> (?)
controllers-integration	73.40% <88.75%> (?)
envoygateway-integration	49.89% <82.50%> (?)
gatewayapi-integration	13.60% <10.00%> (?)
integration	?
istio-integration	52.92% <87.50%> (?)
unit	28.70% <26.58%> (-1.33%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api/v1beta1 (u)	90.90% <100.00%> (+19.48%)	⬆️
api/v1beta2 (u)	86.61% <81.94%> (-4.82%)	⬇️
pkg/common (u)	88.13% <ø> (-0.70%)	⬇️
pkg/istio (u)	71.51% <ø> (-2.40%)	⬇️
pkg/log (u)	94.73% <ø> (ø)
pkg/reconcilers (u)	∅ <ø> (∅)
pkg/rlptools (u)	85.00% <ø> (+5.54%)	⬆️
controllers (i)	82.75% <82.84%> (+5.95%)	⬆️

Files with missing lines	Coverage Δ
api/v1beta2/topology.go	61.53% <ø> (ø)
api/v1beta3/ratelimitpolicy_types.go	73.68% <ø> (ø)
controllers/envoygateway_wasm_controller.go	82.30% <100.00%> (ø)
...ollers/limitador_cluster_envoyfilter_controller.go	73.03% <100.00%> (-8.79%)	⬇️
...s/limitador_status_to_rlp_gateway_event_handler.go	81.81% <100.00%> (ø)
...llers/rate_limiting_istio_wasmplugin_controller.go	70.87% <100.00%> (ø)
controllers/ratelimitpolicy_controller.go	81.55% <100.00%> (+7.55%)	⬆️
...lers/ratelimitpolicy_enforced_status_controller.go	83.42% <100.00%> (ø)
controllers/ratelimitpolicy_limits.go	81.35% <100.00%> (-0.91%)	⬇️
controllers/ratelimitpolicy_status.go	95.83% <100.00%> (+14.35%)	⬆️
... and 9 more

... and 48 files with indirect coverage changes

[guicassolato]

Do you mind keeping this one commit out if this PR please? It's done in #893 where it makes more sense IMO, because the functionality for it is also implemented there.

[eguzki]

I need that to implement the removal of routeSelectors. It's part of my assignment on #810

[guicassolato]

Please. Don't do that either. It's also removed in #893, along with the meaning of that.

[eguzki]

I am confused. I thought we agreed on doing the version bumping first together with the route selector removal. That's a change big enough to deserve it's own PR. Then in another PR, the refactor on the state of the world controller.

I think it is reasonable and we should stick to the plan.

[guicassolato]

We can remove the routeSelectors in its own PR if you prefer (although not here), but either way adding the sectionName has no meaning without the task the computes the effect RLPs.

Please refer to #893 to see what's in there. Hopefully it will make more sense.

[eguzki]

adding the sectionName has no meaning without the task the computes the effect RLPs.

Completely disagree. Adding the sectionName and removing the routeSelectors is itself the point of this PR. And it can be done without computing the effective policies and without refactor to to the state of the world controller

[guicassolato]

@eguzki, I see your point, but we are doing the same work twice. The work in #893 is done already, and that branch targets this one in case you want to merge it in one go.

Redoing the same thing in this PR is a waste of your time IMO, first because it's done in there and it will all be part of the same change anyway, but more importantly to me because here it would be a change to the API without the counterpart function that uses this field.

That said, if you really want to work on some part of this particular change still in this PR, the only thing I ask is please do not add sectionName just yet. You can remove routeSelectors and all the logic related to it. I'm fine with rebasing mine. Np.

[eguzki]

With all due respect, @guicassolato, do not change the scope of the issues that we had agree upon in the planning stage on your own. That's not nice. You could be doing some work that someone else had already done because it was enclosed on another issue. That's wasting someones time.

I will remove all the changes that you already did and open PR with only the version bump.

[guicassolato]

@eguzki,

I see where you are coming from. Indeed "Add sectionName field to targetRef" is listed in #810.

In the end, many of these tasks are convoluted I'm afraid. There's very little left to merging policies together without the sectionName and there is no reason to introduce sectionName other than for changing how we let policies target sections of the resources and consequently how we merge policy specs that add definitions only for those sections. IOW, sectionName and effective policies with D&O and merge strategies in context are all mixed together into one.

Unfortunately, the issues in GitHub do not do a great job at setting clearly the boundaries of what's one thing and what's another, especially in this case where lines are in fact blurred. We need more human communication to sync on this kind of stuff IMO.

Apologies if you have perceived as a change of plans. It was never my intention. I'm only trying to be proactive here. Last week we agreed on working on this part of the workflow together. You were kind enough to share a draft. I wanted to add the next bit.

[eguzki]

ready for rewiew @guicassolato

[guicassolato]

Just need to sort out those couple unit tests failing due to now seeing the hostnames in the conditions to call the RL service, but the general change LGTM. In fact, very thorough job on removing the routeSelectors. Thank you, @eguzki!

[eguzki]

rebase to resolve conflicts required and fixed integration tests.

please review @guicassolato Thanks 🙇‍♀️

[guicassolato]

Neat.

[eguzki]

heads up @trepel @azgabur

This is breaking backward compat. More changes coming in!