Target groups can now also be specified by Name #2655

marcosdiez · 2022-05-23T02:18:03Z

Issue

Partially solves #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

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the docs directory)
Manually tested
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?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

k8s-ci-robot · 2022-05-23T02:18:11Z

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.

codecov-commenter · 2022-05-23T02:24:39Z

Codecov Report

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

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%)`	⬇️
webhooks/elbv2/targetgroupbinding_validator.go	`73.00% <66.66%> (-2.00%)`	⬇️

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

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

marcosdiez · 2022-06-22T13:04:52Z

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

mdiez-modus · 2022-08-22T14:26:36Z

@M00nF1sh don't forget me!

marcosdiez · 2022-11-04T00:03:28Z

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

k8s-triage-robot · 2023-04-12T12:15:49Z

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

marcosdiez · 2023-04-12T12:42:44Z

/remove-lifecycle stale

ivankatliarchuk · 2023-05-12T15:34:44Z

Would it be possible to review this MR?

ivankatliarchuk · 2023-05-15T08:08:10Z

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

johngmyers

I think there needs to be some backstop check for an empty ARN in the main controller, in case the mutating webhook was somehow bypassed.

johngmyers · 2023-07-29T17:07:15Z

docs/guide/targetgroupbinding/targetgroupbinding.md


-## Sample YAML


Was this deletion intended?

Hi! This line wasn't really deleted. It changed to ## Sample YAMLs (plural) and moved about 10 lines above :)

johngmyers · 2023-07-29T17:13:51Z

webhooks/elbv2/targetgroupbinding_validator.go

+		} else if tgb.Spec.TargetGroupName != "" {
+			tgObj, err := v.getTargetGroupsByNameFromAWS(ctx, tgb.Spec.TargetGroupName)
+			if err != nil {
+				return errors.Errorf("Can't locate TargetGroup with name %s", tgb.Spec.TargetGroupName)
+			}
+			tgb.Spec.TargetGroupARN = *tgObj.TargetGroupArn


What is the purpose of this code? The validator can't mutate.

The purpose of this code is to guarantee that the either the ARN of the TargetGroup exists or it's possible to infer the ARN by the name of the TargetGroup (since it's unique).

And even though the validator can't mutate, I added tgb.Spec.TargetGroupARN = *tgObj.TargetGroupArn to guarantee the object is in a consistent state though the rest of the process.

The whole code of aws-load-balancer-controller was written assuming there is an ARN. By changing the object here I guarantee as early as possible that that assumption is true.

A comment to that effect would be helpful.

ivankatliarchuk · 2023-08-26T10:24:45Z

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

johngmyers · 2023-08-29T18:47:25Z

webhooks/elbv2/targetgroupbinding_mutator.go

@@ -37,6 +37,12 @@ func (m *targetGroupBindingMutator) Prototype(_ admission.Request) (runtime.Obje

 func (m *targetGroupBindingMutator) MutateCreate(ctx context.Context, obj runtime.Object) (runtime.Object, error) {
 	tgb := obj.(*elbv2api.TargetGroupBinding)
+	if tgb.Spec.TargetGroupARN == "" && tgb.Spec.TargetGroupName == "" {
+		return nil, errors.Errorf("You must provide either TargetGroupARN or TargetGroupName")


Error messages should not start with upper case.

Suggested change

return nil, errors.Errorf("You must provide either TargetGroupARN or TargetGroupName")

return nil, errors.Errorf("must provide either TargetGroupARN or TargetGroupName")

johngmyers · 2023-08-29T18:49:01Z

webhooks/elbv2/targetgroupbinding_mutator_test.go

+					IPAddressType:   &targetGroupIPAddressTypeIPv4,
+				},
+			},
+		},


Add tests for likely error cases.

johngmyers · 2023-08-29T18:52:14Z

webhooks/elbv2/targetgroupbinding_validator.go

+		} else if tgb.Spec.TargetGroupName != "" {
+			tgObj, err := v.getTargetGroupsByNameFromAWS(ctx, tgb.Spec.TargetGroupName)
+			if err != nil {
+				return errors.Errorf("Can't locate TargetGroup with name %s", tgb.Spec.TargetGroupName)


Suggested change

return errors.Errorf("Can't locate TargetGroup with name %s", tgb.Spec.TargetGroupName)

return fmt.Errorf("searching TargetGroup with name %s: %w", tgb.Spec.TargetGroupName, err)

johngmyers · 2023-08-29T18:54:17Z

webhooks/elbv2/targetgroupbinding_validator.go

+		return nil, err
+	}
+	if len(tgList) != 1 {
+		return nil, errors.Errorf("expecting a single targetGroup with name [%s] but got %v", tgName, len(tgList))


The caller also includes the desired tgName, so consider removing that from this message.

If one needs to read logs, one is usually not having a good day. More info means more helpful logs. I believe we should keep that. It's not sensitive at all.

Duplicating the same information in a single log line is not more helpful.

Sorry, why am I duplicating the same information in a single log line ? tgName != len(tgList)

johngmyers · 2023-08-29T18:55:03Z

webhooks/elbv2/targetgroupbinding_validator.go

+		} else if tgb.Spec.TargetGroupName != "" {
+			tgObj, err := v.getTargetGroupsByNameFromAWS(ctx, tgb.Spec.TargetGroupName)
+			if err != nil {
+				return errors.Errorf("Can't locate TargetGroup with name %s", tgb.Spec.TargetGroupName)
+			}
+			tgb.Spec.TargetGroupARN = *tgObj.TargetGroupArn


A comment to that effect would be helpful.

lukaspj · 2024-01-14T10:00:45Z

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.

…pName has been provided

k8s-ci-robot · 2024-02-07T11:08:45Z

[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:

OWNERS

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

ivankatliarchuk · 2024-03-23T16:15:35Z

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.

k8s-ci-robot · 2024-03-23T16:15:43Z

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-triage-robot · 2024-06-21T16:23:55Z

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

marcosdiez · 2024-06-21T18:08:03Z

/remove-lifecycle stale

k8s-triage-robot · 2024-09-19T18:47:19Z

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

stavvy-cgill · 2024-09-30T14:58:00Z

Any update on this MR? It would make it significantly easier to integrate our GitOps with our IaC. Would love to see it officially supported rather than needing suboptimal workarounds to achieve the same end.

fatmcgav · 2024-10-18T11:05:43Z

I've refreshed this PR in #3903... 🤞 that will gain some traction...

k8s-triage-robot · 2024-11-17T11:25:49Z

The Kubernetes project currently lacks enough active 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 rotten
Close this PR with /close
Offer to help out with Issue Triage

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

/lifecycle rotten

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 23, 2022

k8s-ci-robot requested review from kishorj and M00nF1sh May 23, 2022 02:18

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 23, 2022

marcosdiez mentioned this pull request May 23, 2022

Add support for selecting a Target Group using AWS resource tags #2373

Open

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 23, 2022

marcosdiez mentioned this pull request Jun 3, 2022

Create option to reuse an existing ALB instead of creating a new ALB per Ingress #298

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2023

marcosdiez force-pushed the targetGroupName branch from 505fe46 to 0efe885 Compare January 12, 2023 12:01

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 12, 2023

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 12, 2023

johngmyers suggested changes Jul 29, 2023

View reviewed changes

johngmyers suggested changes Aug 29, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2023

marcosdiez force-pushed the targetGroupName branch from d446894 to 8fda219 Compare October 2, 2023 11:54

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2023

marcosdiez added 7 commits February 7, 2024 08:07

TargetGroupBinding now support targetGroupName

e8061ef

documentation for targetGroupName

a6cb538

unit tests now validate if either either TargetGroupARN or TargetGrou…

debf706

…pName has been provided

more unit tests

f96db24

updated documentation

c57b3ba

Unit Test for MutateCreate

1c36ba4

implemented changes requested by reviewers

d21d942

marcosdiez force-pushed the targetGroupName branch from 8fda219 to d21d942 Compare February 7, 2024 11:08

make crds

59f98d2

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 7, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2024

ivankatliarchuk mentioned this pull request Mar 23, 2024

Extend TargetGroupBinding to allow lookup of target group by name rather than just arn #3586

Open

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 21, 2024

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 21, 2024

MarcusDunn mentioned this pull request Aug 22, 2024

[Feature request] Reference a targetGroup by name in TargetGroupBinding #3819

Open

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 19, 2024

fatmcgav mentioned this pull request Oct 18, 2024

TargetGroupBinding now support targetGroupName - #2655 refresh #3903

Open

6 tasks

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 17, 2024

	return nil, errors.Errorf("You must provide either TargetGroupARN or TargetGroupName")
	return nil, errors.Errorf("must provide either TargetGroupARN or TargetGroupName")

	return errors.Errorf("Can't locate TargetGroup with name %s", tgb.Spec.TargetGroupName)
	return fmt.Errorf("searching TargetGroup with name %s: %w", tgb.Spec.TargetGroupName, err)

Target groups can now also be specified by Name #2655

Are you sure you want to change the base?

Target groups can now also be specified by Name #2655

Conversation

marcosdiez commented May 23, 2022 • edited Loading

Issue

Description

Checklist

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

k8s-ci-robot commented May 23, 2022

codecov-commenter commented May 23, 2022 • edited Loading

Codecov Report

marcosdiez commented Jun 22, 2022

mdiez-modus commented Aug 22, 2022

marcosdiez commented Nov 4, 2022

k8s-triage-robot commented Apr 12, 2023

marcosdiez commented Apr 12, 2023

ivankatliarchuk commented May 12, 2023

ivankatliarchuk commented May 15, 2023

johngmyers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankatliarchuk commented Aug 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaspj commented Jan 14, 2024

k8s-ci-robot commented Feb 7, 2024

ivankatliarchuk commented Mar 23, 2024

k8s-ci-robot commented Mar 23, 2024

k8s-triage-robot commented Jun 21, 2024

marcosdiez commented Jun 21, 2024

k8s-triage-robot commented Sep 19, 2024

stavvy-cgill commented Sep 30, 2024 • edited Loading

fatmcgav commented Oct 18, 2024

k8s-triage-robot commented Nov 17, 2024

marcosdiez commented May 23, 2022 •

edited

Loading

codecov-commenter commented May 23, 2022 •

edited

Loading

ivankatliarchuk commented Aug 26, 2023 •

edited

Loading

stavvy-cgill commented Sep 30, 2024 •

edited

Loading