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.

Sign up for GitHub

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

Jump to bottom

🐛 Fix Target Group's name exceeding 32 characters #4948

Merged
merged 2 commits into from
Apr 24, 2024
Merged

🐛 Fix Target Group's name exceeding 32 characters #4948

merged 2 commits into from
Apr 24, 2024

Conversation

r4f4
Copy link
Contributor

@r4f4 r4f4 commented Apr 23, 2024

What type of PR is this?
/kind bug

What this PR does / why we need it:
Using the LB's name as part of the Target Group's name has the potential of causing failures since both have a limit of 32 characters. For example, using rdossant-installer-04-wn9qs-int as the LB name, will result in the following error

time="2024-04-22T22:54:32Z" level=debug msg="\tfailed to create target group for load balancer: ValidationError: Target group name 'rdossant-installer-04-wn9qs-int-6443' cannot be longer than '32' characters"

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #4947

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • includes emojis
  • adds unit tests
  • adds or updates e2e tests

Release note:

Revert a change where the Target Group's name would use the Load Balancer's name as prefix, possibly causing it to exceed the 32 characters limit

r4f4 added 2 commits April 23, 2024 11:07
@r4f4
🐛: revert to using hardcoded prefix for target groups
e1a95fb 
Using the LB's name as prefix has the potential of exceeding the 32
characters limit.
@r4f4
✨: add validation to TG name field.
077734c
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 23, 2024
@k8s-ci-robot k8s-ci-robot added needs-priority needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 23, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @r4f4. 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.

@r4f4
Copy link
Contributor Author

r4f4 commented Apr 23, 2024

/cc @mtulio
This is one possible solution. Feel free to suggest alternatives.

@k8s-ci-robot
Copy link
Contributor

@r4f4: GitHub didn't allow me to request PR reviews from the following users: mtulio.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @mtulio
This is one possible solution. Feel free to suggest alternatives.

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.

@r4f4 r4f4 changed the title Target group name fix 🐛 Fix Target Group's name exceeding 32 characters Apr 23, 2024
@nrb nrb added this to the v2.5.0 milestone Apr 23, 2024
@nrb
Copy link
Contributor

nrb commented Apr 23, 2024

/ok-to-test

/assign @mtulio

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 23, 2024
@k8s-ci-robot
Copy link
Contributor

@nrb: GitHub didn't allow me to assign the following users: mtulio.

Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/ok-to-test

/assign @mtulio

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 k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 23, 2024
damdo
damdo reviewed Apr 23, 2024
View reviewed changes
Copy link
Member

@damdo damdo left a comment

Choose a reason for hiding this comment

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

/lgtm

pkg/cloud/services/elb/loadbalancer.go
@@ -276,7 +263,7 @@ func (s *Service) getAPIServerLBSpec(elbName string, lbSpec *infrav1.AWSLoadBala
Protocol: infrav1.ELBProtocolTCP,
Port: infrav1.DefaultAPIServerPort,
TargetGroup: infrav1.TargetGroupSpec{
Name: apiTargetGroupName,
Name: fmt.Sprintf("apiserver-target-%d", time.Now().Unix()),
Copy link
Member

Choose a reason for hiding this comment

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

Ok this is currently 30 chars.

>>> len("additional-listener-1257894000")
30

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2024
@mtulio
Copy link
Contributor

mtulio commented Apr 23, 2024

/cc @mtulio This is one possible solution. Feel free to suggest alternatives.

I believe it is fine reverting the subset of #4849 considering the main targets are attributes for the health check. For UX it would be interesting allowing or reusing some custom prefix (like cluster id), or truncating the name in the way that will not have duplicated values.

/lgtm

@k8s-ci-robot
Copy link
Contributor

@mtulio: changing LGTM is restricted to collaborators

In response to this:

/cc @mtulio This is one possible solution. Feel free to suggest alternatives.

I believe it is fine reverting the subset of #4849 considering the main targets are attributes for the health check. For UX it would be interesting allowing or reusing some custom prefix (like cluster id), or truncating the name in the way that will not have duplicated values.

/lgtm

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.

@damdo
Copy link
Member

damdo commented Apr 24, 2024

/assign @richardcase @vincepri @Ankitasw @dlipovetsky
For approval

@mtulio mtulio mentioned this pull request Apr 24, 2024
Merged
2 tasks
@mtulio
Copy link
Contributor

mtulio commented Apr 24, 2024

/tide refresh

@mtulio mtulio mentioned this pull request Apr 24, 2024
Merged
@nrb
Copy link
Contributor

nrb commented Apr 24, 2024

/tide refresh

@richardcase
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

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

The pull request process is described 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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2024
@k8s-ci-robot k8s-ci-robot merged commit 4c5b811 into kubernetes-sigs:main Apr 24, 2024
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damdo damdo damdo left review comments

@vincepri vincepri vincepri approved these changes

@dlipovetsky dlipovetsky Awaiting requested review from dlipovetsky

@fiunchinho fiunchinho Awaiting requested review from fiunchinho

Assignees

@richardcase richardcase

@vincepri vincepri

@dlipovetsky dlipovetsky

@damdo damdo

@Ankitasw Ankitasw

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v2.5.0
Development

Successfully merging this pull request may close these issues.

Do not exceed 32 characters limit for Target Group name
9 participants
@r4f4 @k8s-ci-robot @nrb @mtulio @damdo @richardcase @vincepri @dlipovetsky @Ankitasw