-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Doc: Add note for rename behavior of IngressGroup #3283
Conversation
Welcome @yubingjiaocn! |
Hi @yubingjiaocn. 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 Once the patch is verified, the new status will be reflected by the 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. |
|
||
!!!warning "Rename behavior" | ||
ALB is uniquely identified by it's tag `ingress.k8s.aws/stack`, whose value is the name of IngressGroup. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- i think we can remove this "There is no way to rename an existing IngressGroup while keeping ALB", since
- the following text already clarified the recreation behavior.
- technically there is a way, e.g. by manually change the AWS Tags on the existing provisioned ALB. But i don't want to call this hacky solution out in this document.
When the
groupNameof IngressGroup for Ingress is changed, the Ingress will be moved to new IngressGroup and supported by ALB for the new IngressGroup. If ALB for the new IngressGroup doesn't exist, a new ALB will be created. If the old IngressGroup no longer contain any Ingresses, ALB for old IngressGroup will be deleted, and deletion protection of ALB will be ignored
.- we can remove the "If an IngressGroup has only one Ingress (or IngressGroup is not explicit specified), changing name of IngressGroup will cause recreation of Load Balancer. If an IngressGroup has no Ingress, the corrsponding ALB will be removed. In both cases, deletion protection of ALB will be ignored." given the Refactor josh #2 clarified it.
docs/guide/ingress/annotations.md
Outdated
@@ -79,6 +79,13 @@ By default, Ingresses don't belong to any IngressGroup, and we treat it as a "im | |||
other Kubernetes users may create/modify their Ingresses to belong to the same IngressGroup, and can thus add more rules or overwrite existing rules with higher priority to the ALB for your Ingress. | |||
|
|||
We'll add more fine-grained access-control in future versions. | |||
|
|||
!!!warning "Rename behavior" | |||
ALB is uniquely identified by it's tag `ingress.k8s.aws/stack`, whose value is the name of IngressGroup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALB is uniquely identified by it's tag `ingress.k8s.aws/stack`, whose value is the name of IngressGroup. | |
The ALB for an IngressGroup is found by searching for an AWS tag `ingress.k8s.aws/stack` tag with the name of the IngressGroup as its value. |
docs/guide/ingress/annotations.md
Outdated
|
||
There is no way to rename an existing IngressGroup while keeping ALB. When the `groupName` of IngressGroup of Ingress is changed, the Ingress will be moved to corresponding IngressGroup and ALB. If target IngressGroup doesn't exist, a new ALB will be created. | ||
|
||
If an IngressGroup has only one Ingress (or IngressGroup is not explicit specified), changing name of IngressGroup will cause recreation of Load Balancer. If an IngressGroup has no Ingress, the corrsponding ALB will be removed. In both cases, deletion protection of ALB will be ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an IngressGroup has only one Ingress (or IngressGroup is not explicit specified), changing name of IngressGroup will cause recreation of Load Balancer. If an IngressGroup has no Ingress, the corrsponding ALB will be removed. In both cases, deletion protection of ALB will be ignored. | |
If an IngressGroup has only one Ingress (or IngressGroup is not explicitly specified), changing the IngressGroup will cause recreation of its Load Balancer. If an IngressGroup has no Ingress, the corresponding ALB will be deleted. In both cases, the deletion protection of the ALB will be ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the logic of this is not right. Changing the name of an IngressGroup will cause the ALB to be recreated regardless of the number of Ingresses using it. Separately, if an IngressGroup goes to zero Ingresses using it, the ALB will be deleted.
@johngmyers @M00nF1sh Thank you for your suggestion, and I have change some wording to clarify the logic. Please take a look if you got time. Thx. |
Co-authored-by: John Gardiner Myers <jgmyers@proofpoint.com>
/ok-to-test |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: M00nF1sh, yubingjiaocn 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 |
Hi, thank you for merging this PR. Since this PR documents current behavior, is it possible to cherry-pick it to |
…to 2.6.0 Merge in DEL/aws-load-balancer-controller-fork from merge-up to main * commit '195e896b0efbd467694bb9a19de7c5a12c5dde8c': (71 commits) check the canary test result and exit if it failed Apply suggestions from code review Update docs/guide/service/annotations.md Addressing the comment Remove dependency on aws-sdk-go-v2 (kubernetes-sigs#3320) Update live docs for NLB-SG feature release cut v2.6.0 release refactor targetGroupBinding network builder Add support for NLB security groups Allow TLS 1.2 with restricted ciphers for webhooks Update the RSA filter for Cert discovery Doc: Add note for rename behavior of IngressGroup (kubernetes-sigs#3283) Make Ingress validating webhook ignore ingresses not managed by AWS LBC (kubernetes-sigs#3272) add oliviassss as reviewer fix the race condition in pod cache and endpoint resolver Bump github.com/onsi/ginkgo/v2 from 2.6.0 to 2.11.0 Bump github.com/aws/aws-sdk-go from 1.44.184 to 1.44.294 (kubernetes-sigs#3271) Provide better explanation of failure to find a subnet (kubernetes-sigs#3292) test/framework: replace deprecated ioutil.ReadAll (kubernetes-sigs#3256) Add warning in doc for ServiceMutatorWebhook (kubernetes-sigs#3180) ...
* Add warning for rename behavior of IngressGroup * Change wording per suggestion from code review * Apply grammar suggestions from code review Co-authored-by: John Gardiner Myers <jgmyers@proofpoint.com> --------- Co-authored-by: John Gardiner Myers <jgmyers@proofpoint.com>
Issue
#2271
#3034
Description
Add a warning to describe rename behavior of IngressGroup.
When rename a IngressGroup with only an Ingress, ALB will be recreated. Deletion protection will be ignored during recreate. However, this behavior is not documented and caused confuse and interruption of customer usage.
By adding description about ALB to IngressGroup relationship and rename behavior, customer will have a clearer view on how IngressGroup run under the hood.
Checklist
README.md
, or thedocs
directory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯