fix: rateLimitDeployment ignoring pod labels and annotation merge by oscarboher · Pull Request #4228 · envoyproxy/gateway

oscarboher · 2024-09-12T20:37:22Z

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

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

…defined Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

…er/gateway into fix/ratelimit-deployment-labels

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

internal/infrastructure/kubernetes/ratelimit/resource_provider.go

codecov · 2024-09-19T05:11:09Z

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 56 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.

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

…er/gateway into fix/ratelimit-deployment-labels

arkodg · 2024-09-24T19:05:20Z

internal/infrastructure/kubernetes/ratelimit/resource_provider.go

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


can L195 be removed ? since L192 is already setting it

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.

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"

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

arkodg

LGTM thanks !

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

…er/gateway into fix/ratelimit-deployment-labels

arkodg

LGTM thanks !

…voyproxy#4228) * fix labels and annotation merges for rate limit deployment Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * fix tests and label merge Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * fix annotation merge if prometheus was disabled and annotations were defined Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * renamed labels and annotations to specify they apply to pods only Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * linter Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * fix resource provider tests to new annotation behavior Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * go linter Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * fix gen-check Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * pod labels selector comment Signed-off-by: Oscar Boher <oscar.boher@scopely.com> --------- Signed-off-by: Oscar Boher <oscar.boher@scopely.com> Co-authored-by: zirain <zirain2009@gmail.com> Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com> (cherry picked from commit cf84927) Signed-off-by: Guy Daich <guy.daich@sap.com>

* build(deps): bump github.com/docker/docker from 27.2.0+incompatible to 27.3.1+incompatible (#4303) build(deps): bump github.com/docker/docker Bumps [github.com/docker/docker](https://github.com/docker/docker) from 27.2.0+incompatible to 27.3.1+incompatible. - [Release notes](https://github.com/docker/docker/releases) - [Commits](moby/moby@v27.2.0...v27.3.1) --- updated-dependencies: - dependency-name: github.com/docker/docker dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com> (cherry picked from commit cd4e9b1) Signed-off-by: Guy Daich <guy.daich@sap.com> * bugfix: EG loglevel error for admin and metrics module (#4340) * bugfix: EG loglevel error for admin and metrics module Signed-off-by: qicz <qiczzhu@gmail.com> * fix ut Signed-off-by: qicz <qiczzhu@gmail.com> * polish Signed-off-by: qicz <qiczzhu@gmail.com> --------- Signed-off-by: qicz <qiczzhu@gmail.com> (cherry picked from commit 9b161ee) Signed-off-by: Guy Daich <guy.daich@sap.com> * fix: some status updates are discarded by the status updater (#4337) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> (cherry picked from commit 14830c7) Signed-off-by: Guy Daich <guy.daich@sap.com> * fix: Unsupported listener protocol type error for nil supportKinds assign gateway status. (#4345) (cherry picked from commit 7babca9) Signed-off-by: Guy Daich <guy.daich@sap.com> * set invalid Listener.SupportedKinds to empty list (#4352) * set invalid Listener.SupportedKinds to empty list Fixes: #4216 Relates to https://kubernetes.slack.com/archives/CR0H13KGA/p1727457195236889 Signed-off-by: Arko Dasgupta <arko@tetrate.io> * lint Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit 2fdf069) Signed-off-by: Guy Daich <guy.daich@sap.com> * bugfix: ignore some unnecessary requests to apiserver. (#4362) Signed-off-by: qicz <qiczzhu@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com> (cherry picked from commit fe1e8bd) Signed-off-by: Guy Daich <guy.daich@sap.com> * fix: rateLimitDeployment ignoring pod labels and annotation merge (#4228) * fix labels and annotation merges for rate limit deployment Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * fix tests and label merge Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * fix annotation merge if prometheus was disabled and annotations were defined Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * renamed labels and annotations to specify they apply to pods only Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * linter Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * fix resource provider tests to new annotation behavior Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * go linter Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * fix gen-check Signed-off-by: Oscar Boher <oscar.boher@scopely.com> * pod labels selector comment Signed-off-by: Oscar Boher <oscar.boher@scopely.com> --------- Signed-off-by: Oscar Boher <oscar.boher@scopely.com> Co-authored-by: zirain <zirain2009@gmail.com> Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com> (cherry picked from commit cf84927) Signed-off-by: Guy Daich <guy.daich@sap.com> * fix dashboard typos (#4422) Signed-off-by: haorenfsa <haorenfsa@gmail.com> Signed-off-by: Guy Daich <guy.daich@sap.com> * fix: ratelimit not working with both headers and cidr matches (#4377) * fix ratelimit descriptors do not respect both headers and cidr match for one rule Signed-off-by: shawnh2 <shawnhxh@outlook.com> * fix gen-check and lint Signed-off-by: shawnh2 <shawnhxh@outlook.com> * fix ratelimit e2e test Signed-off-by: shawnh2 <shawnhxh@outlook.com> * add more comment and update test case Signed-off-by: shawnh2 <shawnhxh@outlook.com> --------- Signed-off-by: shawnh2 <shawnhxh@outlook.com> Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com> (cherry picked from commit 66c0b51) Signed-off-by: Guy Daich <guy.daich@sap.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Guy Daich <guy.daich@sap.com> Signed-off-by: qicz <qiczzhu@gmail.com> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: Oscar Boher <oscar.boher@scopely.com> Signed-off-by: haorenfsa <haorenfsa@gmail.com> Signed-off-by: shawnh2 <shawnhxh@outlook.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Huabing Zhao <zhaohuabing@gmail.com> Co-authored-by: qi <qiczzhu@gmail.com> Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com> Co-authored-by: zirain <zirain2009@gmail.com> Co-authored-by: Oscar Boher <oscar.boher@scopely.com> Co-authored-by: shaoyue <haorenfsa@gmail.com> Co-authored-by: sh2 <shawnhxh@outlook.com>

fix labels and annotation merges for rate limit deployment

84f6958

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

oscarboher closed this Sep 12, 2024

fix tests and label merge

6e5f6a0

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

oscarboher reopened this Sep 12, 2024

oscarboher added 7 commits September 12, 2024 23:33

fix annotation merge if prometheus was disabled and annotations were …

c0f680d

…defined Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

renamed labels and annotations to specify they apply to pods only

6a57aeb

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

Merge branch 'main' into fix/ratelimit-deployment-labels

cd58565

linter

45e0263

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

Merge branch 'fix/ratelimit-deployment-labels' of github.com:oscarboh…

cfe9bab

…er/gateway into fix/ratelimit-deployment-labels

fix resource provider tests to new annotation behavior

2db9a57

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

Merge branch 'main' into fix/ratelimit-deployment-labels

4b86c45

oscarboher marked this pull request as ready for review September 18, 2024 09:41

oscarboher requested a review from a team as a code owner September 18, 2024 09:41

arkodg reviewed Sep 19, 2024

View reviewed changes

internal/infrastructure/kubernetes/ratelimit/resource_provider.go Show resolved Hide resolved

zirain and others added 3 commits September 19, 2024 23:00

Merge branch 'main' into fix/ratelimit-deployment-labels

138ecda

go linter

92a2b13

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

Merge branch 'fix/ratelimit-deployment-labels' of github.com:oscarboh…

1d3f357

…er/gateway into fix/ratelimit-deployment-labels

arkodg reviewed Sep 24, 2024

View reviewed changes

oscarboher and others added 3 commits September 25, 2024 09:47

fix gen-check

d831f49

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

Merge branch 'main' into fix/ratelimit-deployment-labels

e005c19

Merge branch 'main' into fix/ratelimit-deployment-labels

a6b00f3

arkodg previously approved these changes Sep 25, 2024

View reviewed changes

arkodg requested review from a team September 25, 2024 20:57

oscarboher added 2 commits September 26, 2024 08:08

pod labels selector comment

c3220c6

Signed-off-by: Oscar Boher <oscar.boher@scopely.com>

Merge branch 'fix/ratelimit-deployment-labels' of github.com:oscarboh…

2e8a961

…er/gateway into fix/ratelimit-deployment-labels

oscarboher dismissed arkodg’s stale review via 2e8a961 September 26, 2024 06:09

oscarboher added 2 commits September 26, 2024 08:10

Merge branch 'main' into fix/ratelimit-deployment-labels

11e8504

Merge branch 'main' into fix/ratelimit-deployment-labels

1eb1c07

arkodg approved these changes Oct 1, 2024

View reviewed changes

arkodg requested review from a team October 1, 2024 00:08

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 approved these changes Oct 1, 2024

View reviewed changes

zirain merged commit cf84927 into envoyproxy:main Oct 1, 2024

arkodg added the cherrypick/release-v1.1.3 label Oct 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: rateLimitDeployment ignoring pod labels and annotation merge#4228

fix: rateLimitDeployment ignoring pod labels and annotation merge#4228
zirain merged 19 commits intoenvoyproxy:mainfrom
oscarboher:fix/ratelimit-deployment-labels

oscarboher commented Sep 12, 2024

Uh oh!

Uh oh!

codecov bot commented Sep 19, 2024 •

edited

Loading

Uh oh!

arkodg Sep 24, 2024

Uh oh!

oscarboher Sep 25, 2024

Uh oh!

arkodg Sep 25, 2024

Uh oh!

arkodg left a comment

Uh oh!

arkodg left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

oscarboher commented Sep 12, 2024

Uh oh!

Uh oh!

codecov bot commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arkodg Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

oscarboher Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

arkodg Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Sep 19, 2024 •

edited

Loading