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 two targetGroup related bug #1635

Merged
merged 3 commits into from
Nov 11, 2020
Merged

Conversation

M00nF1sh
Copy link
Collaborator

@M00nF1sh M00nF1sh commented Nov 10, 2020

Changes done

  1. remove TargetGroup's port from TargetGroup replacement criteria. so that TargetGroups can be reused if only port differs.
    • this allow us to migrate from AWSALBIngressController/AWSLoadBalancerController(v2.0.0) with 0 downtime.
  2. change the port for instance TargetGroup to be nodePort for better UX. (the port don't matter)
  3. fix an edge that when same service referenced by both name and port, only a single TargetGroup are created. Now two different targetGroup will be created.
    Details:
    1. in both AWSALBIngressController and AWSLoadBalancerController(v2.0.0), we used the backendPort specified as resourceID(so it can be either port name or port number). Ideally different resourceID should result in different TargetGroup.
    2. in v2.0.0, even the model contains two targetGroup, only a single targetGroup will be created since the TG name is same, and due to ELBV2's API's idempotency. This works fine, however, it relies on ELBv2 API's implementation details and hacky.
    3. an alternative design is to always use the numeric port as resourceID. and to be backwards-compatible for backend port referenced by name, we can add a logic in targetGroup's cleanup logic to ignore TargetGroups's arn been used. we decided not to take this approach as it's hacky. (also, in the future, we might have extra logic to compress TargetGroup usage across Ingresses, so no need to compress for this edge case this time).

Test done

Pre steps:

  1. Install AWSALBIngressController with 0 replica
  2. Install AWSLoadBalancerController with 1 replica

compatibility test with AWSALBIngressController:

  1. scale up AWSALBIngressController to 1 replica
  2. launch Ingress with 4 path: echo: http, echo: 80, echo-ip: http, echo-ip:80. (echo is instance TargetType, echo-ip is ip TargetType, both http and 80 refers to same servicePort).
  3. verify that traffic works, and 4 target group created.
  4. scale down AWSALBIngressController to 0 replica
  5. scale up AWSLoadBalancerController to 1 replica
  6. verify the controller added elbv2.k8s.aws/cluster":"m00nf1sh-dev tags to resources, and kept old resources like LoadBalancer/TargetGroups.

compatibility test with AWSLoadBalancerController:

  1. change to AWSLoadBalancerController v2.0.0 and scale up AWSLoadBalancerController to 1 replica
  2. launch Ingress with 4 path: echo: http, echo: 80, echo-ip: http, echo-ip:80. (echo is instance TargetType, echo-ip is ip TargetType, both http and 80 refers to same servicePort).
  3. verify that traffic works, and only two target group created.
  4. change to AWSLoadBalancerController to image built with this change and scale up AWSLoadBalancerController to 1 replica
  5. verify that the controller added two new targetGroup and configure rules properly.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 10, 2020
pkg/ingress/model_build_target_group.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fawadkhaliq fawadkhaliq left a comment

Choose a reason for hiding this comment

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

Let's pls add a comment why port=1 has no impact on the datapath as we override with a target

/lgtm otherwise

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fawadkhaliq, kishorj, M00nF1sh
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@M00nF1sh M00nF1sh merged commit 8ac0f32 into kubernetes-sigs:main Nov 11, 2020
Copy link
Contributor

@fawadkhaliq fawadkhaliq left a comment

Choose a reason for hiding this comment

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

/lgtm

alfredkrohmer added a commit to alfredkrohmer/aws-alb-ingress-controller that referenced this pull request Nov 19, 2020
M00nF1sh pushed a commit that referenced this pull request Nov 19, 2020
Timothy-Dougherty pushed a commit to adammw/aws-load-balancer-controller that referenced this pull request Nov 9, 2023
* fix two targetGroup related bug

* address PR comments

* address PR comments
Timothy-Dougherty pushed a commit to adammw/aws-load-balancer-controller that referenced this pull request Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants