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: retain non-nginx canary annotations. Fixes: #1070 #3806

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

jahvon
Copy link
Contributor

@jahvon jahvon commented Aug 26, 2024

The current patch applied for nginx canaries does not play nicely with other controller that may be modifying Ingress annotations. In our case, the Rancher field.cattle.io/publicEndpoints is repeatedly added/removed, resulting in a significant amount of updates to Ingress resources in the cluster.

With this fix, before building and applying the canary ingress patch, all annotations from the existing canary ingress will be copied over to the desired Ingress.

fixes: #1070

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

Signed-off-by: Jahvon Dockery <jdockery@cargurus.com>
@jahvon jahvon force-pushed the maintain-ingress-annotations branch from 5e58402 to e7f029d Compare August 26, 2024 20:39
Copy link

@jahvon jahvon marked this pull request as ready for review August 26, 2024 20:49
@jahvon
Copy link
Contributor Author

jahvon commented Aug 26, 2024

I've also tested this in a RKE cluster. Here is a screenshot of the PATCH|POST|PUT|APPLY API Server request for Ingresses (before and after)

Screenshot 2024-08-26 at 4 19 40 PM

Copy link
Contributor

Published E2E Test Results

  4 files    4 suites   3h 13m 39s ⏱️
113 tests 106 ✅  7 💤 0 ❌
452 runs  424 ✅ 28 💤 0 ❌

Results for commit e7f029d.

Copy link
Contributor

Published Unit Test Results

2 265 tests   2 265 ✅  2m 58s ⏱️
  128 suites      0 💤
    1 files        0 ❌

Results for commit e7f029d.

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.83%. Comparing base (23e186e) to head (e7f029d).
Report is 56 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3806      +/-   ##
==========================================
- Coverage   83.87%   83.83%   -0.04%     
==========================================
  Files         162      162              
  Lines       18524    18531       +7     
==========================================
- Hits        15537    15536       -1     
- Misses       2119     2123       +4     
- Partials      868      872       +4     

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


🚨 Try these New Features:

@jahvon jahvon changed the title fix: retain nginx canary annotations fix: retain non-nginx canary annotations. Fixes: #1070 Aug 26, 2024
@zachaller zachaller merged commit f9b62a8 into argoproj:master Aug 27, 2024
28 checks passed
@mozemke
Copy link

mozemke commented Oct 7, 2024

hi @zachaller,

i`m so glad this got merged! :)
do you have any idea when the next argo rollouts release with these changes might be coming out?

meeech pushed a commit to CircleCI-Public/argo-rollouts that referenced this pull request Feb 10, 2025
…roj#3806)

fix: retain nginx canary annotations

Signed-off-by: Jahvon Dockery <jdockery@cargurus.com>
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.

Infinite Ingress recreation when using rancher managed cluster
3 participants