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: rateLimitDeployment ignoring pod labels and annotation merge #4228

Merged
merged 19 commits into from
Oct 1, 2024

Conversation

oscarboher
Copy link
Contributor

What type of PR is this?
fix
What this PR does / why we need it:
rateLimitDeployment.pod.labels where not applying to rate limit pods.
prometheus annotations where ignored if rateLimitDeployment.pod.annotations was defined

Which issue(s) this PR fixes:
Fixes #4226

@oscarboher oscarboher closed this Sep 12, 2024
Signed-off-by: Oscar Boher <[email protected]>
@oscarboher oscarboher reopened this Sep 12, 2024
@oscarboher oscarboher marked this pull request as ready for review September 18, 2024 09:41
@oscarboher oscarboher requested a review from a team as a code owner September 18, 2024 09:41
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 65.69%. Comparing base (4b8c2f5) to head (1eb1c07).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
...tructure/kubernetes/ratelimit/resource_provider.go 85.71% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4228   +/-   ##
=======================================
  Coverage   65.69%   65.69%           
=======================================
  Files         200      200           
  Lines       24095    24101    +6     
=======================================
+ Hits        15829    15833    +4     
- Misses       7134     7135    +1     
- Partials     1132     1133    +1     

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

podLabels := rateLimitLabels()
if r.rateLimitDeployment.Pod.Labels != nil {
maps.Copy(podLabels, r.rateLimitDeployment.Pod.Labels)
maps.Copy(podLabels, rateLimitLabels())
Copy link
Contributor

Choose a reason for hiding this comment

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

can L195 be removed ? since L192 is already setting it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So maps.Copy(dest, src) overwrites keys existing in both maps with the src value. I want to make sure noone can override one of the selector labels from the rateLimitLabels, so I'm doing the last maps.Copy to overwrite with the selector labels in case someone added one of those labels in their values.

Also the destination map must not be nil so I cannot do something like:

podLabels := r.rateLimitDeployment.Pod.Labels
maps.Copy(podLabels, rateLimitLabels())

as it will fail if no pod labels are provided.
It looks a bit funny, I just didn't want to implement a function that merges two maps, but maybe it's worth it for clarity's sake. Let me know if you think so.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah thats a good way to ensure the labels can't be overridden
nit: this as a code comment would be helpful so the next dev gets the "why"

arkodg
arkodg previously approved these changes Sep 25, 2024
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team September 25, 2024 20:57
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team October 1, 2024 00:08
@zirain zirain changed the title fix: fix rateLimitDeployment ignoring pod labels and annotation merge fix: rateLimitDeployment ignoring pod labels and annotation merge Oct 1, 2024
@zirain zirain merged commit cf84927 into envoyproxy:main Oct 1, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EnvoyGateway rateLimitDeployment ignores pod labels
4 participants