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(trafficrouting): Fix setHeaderRoute behavior to affect only related ingress rules #3727

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ArenSH
Copy link

@ArenSH ArenSH commented Jul 17, 2024

This PR fixes ALB::SetHeaderRoute so it only affects Ingress rules that contain stableService as backend.

For example, if we have some ingress:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: some-ingress
  annotations:
    alb.ingress.kubernetes.io/actions.redirect-to-app: 'some action'
    # ...
spec:
  defaultBackend:
    service:
      name: svc-stable
      port:
        name: some-port
  rules:
  - host: host1.example.local
    http:
      paths:
      - backend:
          service:
            name: redirect-to-app
            port:
              name: use-annotation
        path: /
        pathType: ImplementationSpecific
  - host: host2.example.local
    http:
      paths:
      - backend:
          service:
            name: redirect-to-app
            port:
              name: use-annotation
        path: /
        pathType: ImplementationSpecific
  - http:
      paths:
      - backend:
          service:
            name: svc-stable
            port:
              name: use-annotation
        path: /*
        pathType: ImplementationSpecific

And rollout with the following step:

#...
  strategy:
    canary:
      stableService: svc-stable
      canaryService: svc-canary
      steps:
      - setWeight: 10
      - setHeaderRoute:
          match:
          - headerName: x-user-agent
            headerValue:
              exact: test-framework
          name: route-test-requests
# ...

Then, without this fix, the new route-test-requests path will be added to every rule in Ingress. Documentation leaves impression that only rules related to rollout services will be affected.

I'm not sure this fix is consistent with desired argo-rollouts behavior, sou would be happy to receive feedback and adjust the code.

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.

@ArenSH ArenSH force-pushed the fix-set-header-route branch 2 times, most recently from dec22ae to 7bb4a1c Compare July 17, 2024 16:07
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.89%. Comparing base (b8a9bf5) to head (41cfaeb).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3727      +/-   ##
==========================================
+ Coverage   83.88%   83.89%   +0.01%     
==========================================
  Files         163      163              
  Lines       18560    18563       +3     
==========================================
+ Hits        15569    15574       +5     
+ Misses       2119     2118       -1     
+ Partials      872      871       -1     

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

@zachaller zachaller self-assigned this Jul 23, 2024
@argoproj argoproj deleted a comment from github-actions bot Jul 30, 2024
@argoproj argoproj deleted a comment from github-actions bot Jul 30, 2024
@argoproj argoproj deleted a comment from github-actions bot Jul 30, 2024
Copy link
Contributor

github-actions bot commented Jul 30, 2024

Published E2E Test Results

  4 files    4 suites   3h 13m 53s ⏱️
113 tests 104 ✅  7 💤 2 ❌
456 runs  424 ✅ 28 💤 4 ❌

For more details on these failures, see this check.

Results for commit 41cfaeb.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Jul 30, 2024

Published Unit Test Results

2 276 tests   2 276 ✅  2m 59s ⏱️
  128 suites      0 💤
    1 files        0 ❌

Results for commit 41cfaeb.

♻️ This comment has been updated with latest results.

…ss rules related to rollout

Signed-off-by: Armen Shakhbazian <armen.shakhbazian@gmail.com>
Copy link

sonarcloud bot commented Sep 17, 2024

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.

2 participants