aws-load-balancer-controller icon indicating copy to clipboard operation
aws-load-balancer-controller copied to clipboard

Target groups can now also be specified by Name

Open marcosdiez opened this issue 2 years ago • 19 comments

Issue

Partially solves https://github.com/kubernetes-sigs/aws-load-balancer-controller/issues/2373

Description

On TargetGroupBinding one can now choose a target group by it's name, using the targetGroupName field.

Example

---
apiVersion: elbv2.k8s.aws/v1beta1
kind: TargetGroupBinding
metadata:
  name: mdiezalbtest2
  namespace: default
spec:
  ipAddressType: ipv4
  serviceRef:
    name: mdiezalbtest
    port: 8080
  targetGroupName: mdiezalbtest-tg

Implementation detail:

All I did was to make the field targetGroupName valid, make targetGroupARN non mandatory and intercept MutateCreate and checkRequiredFields, so that if targetGroupARN is empty and targetGroupName is provided, AWS is queried for the ARN.

As a future PR, I could from time to time check if a new target group with targetGroupName was created and later bind it.

One can currently test this using the following container: marcosdiez/aws-alb-ingress-controller:20220524-1006

Checklist

  • [X] Added tests that cover your change (if possible)
  • [X] Added/modified documentation as required (such as the README.md, or the docs directory)
  • [X] Manually tested
  • [X] Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! :exploding_head:

  • [ ] Backfilled missing tests for code in same general area :tada:
  • [ ] Refactored something and made the world a better place :star2:

marcosdiez avatar May 23 '22 02:05 marcosdiez

Hi @marcosdiez. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot avatar May 23 '22 02:05 k8s-ci-robot

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: marcosdiez To complete the pull request process, please assign kishorj after the PR has been reviewed. You can assign the PR to them by writing /assign @kishorj 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

k8s-ci-robot avatar May 23 '22 02:05 k8s-ci-robot

Codecov Report

Base: 54.07% // Head: 54.07% // Decreases project coverage by -0.00% :warning:

Coverage data is based on head (0efe885) compared to base (a92e689). Patch coverage: 55.55% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2655      +/-   ##
==========================================
- Coverage   54.07%   54.07%   -0.01%     
==========================================
  Files         144      144              
  Lines        8301     8335      +34     
==========================================
+ Hits         4489     4507      +18     
- Misses       3484     3492       +8     
- Partials      328      336       +8     
Impacted Files Coverage Δ
webhooks/elbv2/targetgroupbinding_mutator.go 66.23% <44.44%> (-6.65%) :arrow_down:
webhooks/elbv2/targetgroupbinding_validator.go 73.00% <66.66%> (-2.00%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov-commenter avatar May 23 '22 02:05 codecov-commenter

Hello @M00nF1sh ! Could you please take a look at this PR ?

marcosdiez avatar Jun 22 '22 13:06 marcosdiez

@M00nF1sh don't forget me!

mdiez-modus avatar Aug 22 '22 14:08 mdiez-modus

@kishorj could you please take a look at this PR ?

marcosdiez avatar Nov 04 '22 00:11 marcosdiez

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Apr 12 '23 12:04 k8s-triage-robot

/remove-lifecycle stale

marcosdiez avatar Apr 12 '23 12:04 marcosdiez

Would it be possible to review this MR?

ivankatliarchuk avatar May 12 '23 15:05 ivankatliarchuk

@kishorj when you have a capacity, would you be able to review this MR and share your feedback please?

ivankatliarchuk avatar May 15 '23 08:05 ivankatliarchuk

Hi, @marcosdiez do you have the capacity to add suggested changes and update MR?

ivankatliarchuk avatar Aug 26 '23 10:08 ivankatliarchuk

Hi, What is missing on this end?

We would very much like not having to depend on the full Target Group ARN because it is not deterministic so it requires a certain amount of handover from our Terraform-based IaC to the in-cluster reconciliation.

If this was implemented, we can make assumptions on how to identify the Target Group, helping us to decouple the two reconciliation engines.

lukaspj avatar Jan 14 '24 10:01 lukaspj

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: marcosdiez Once this PR has been reviewed and has the lgtm label, please assign johngmyers for approval. For more information see the Kubernetes Code Review Process.

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

k8s-ci-robot avatar Feb 07 '24 11:02 k8s-ci-robot

Any chance to release it as experimental feature. Buy that time the community could provide feedback. And for sure we could fix the bugs if any. This mr is already 2 years old.

ivankatliarchuk avatar Mar 23 '24 16:03 ivankatliarchuk

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot avatar Mar 23 '24 16:03 k8s-ci-robot