Tag Security Group created for AWS ELB with same additional tags as ELB #58767
Conversation
Reuse the service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags annotation for these tags. We think that this is a simpler and less intrusive approach than adding a new annotation, and most users will want the same set of tags applied. Signed-off-by: Brett Kochendorfer <brett.kochendorfer@gmail.com>
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
cncf-cla: yes
|
/assign @zmerlynn |
| @@ -2815,7 +2815,7 @@ func (c *Cloud) removeSecurityGroupIngress(securityGroupID string, removePermiss | |||
| // Makes sure the security group exists. | |||
| // For multi-cluster isolation, name must be globally unique, for example derived from the service UUID. | |||
| // Returns the security group id or error | |||
| func (c *Cloud) ensureSecurityGroup(name string, description string) (string, error) { | |||
| func (c *Cloud) ensureSecurityGroup(name string, description string, annotations map[string]string) (string, error) { | |||
There was a problem hiding this comment.
Nit: let's call them "tags" not "annotations"? Maybe "extraTags" or "additionalTags" ?
It's would also be nice to apply the tags where we didn't previously create them. I think there's an additionalTags argument to readRepairClusterTags which will do the right thing. If it's complicated we can address this later, but I think it's simple (?)
There was a problem hiding this comment.
Because we are passing in the annotations on the Service and then getting the additional-tags annotation from that map of annotations, that's why we call this argument annotations.
I see in your other comment that you are thinking of passing in that annotation instead though, which I think makes sense. It's been a hot minute since I wrote this with my colleague but I think we wrote it like this in case other annotations might get used someday. If that seems like unnecessary futureproofing, I can just pass the tags annotation.
| @@ -2881,7 +2881,7 @@ func (c *Cloud) ensureSecurityGroup(name string, description string) (string, er | |||
| return "", fmt.Errorf("created security group, but id was not returned: %s", name) | |||
| } | |||
|
|
|||
| err := c.tagging.createTags(c.ec2, groupID, ResourceLifecycleOwned, nil) | |||
| err := c.tagging.createTags(c.ec2, groupID, ResourceLifecycleOwned, getLoadBalancerAdditionalTags(annotations)) | |||
There was a problem hiding this comment.
Instead of assuming that this is a ELB security group, how about if we pass in getLoadBalancerAdditionalTags(annotations) to ensureSecurityGroup. I suspect we don't call the function anywhere else, but it makes the method less surprising.
|
/assign /ok-to-test Seems like the right thing to do, and a good implementation. I had few suggestions - see if you agree with them :-) |
k8s-ci-robot
removed
the
needs-ok-to-test
|
/test pull-kubernetes-e2e-kops-aws |
|
I do not understand this CI failure; I can't even see the reason why the build failed |
|
/lgtm Thanks @2rs2ts @bkochendorfer |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2rs2ts, justinsb 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 |
k8s-ci-robot
added
the
approved
|
/test pull-kubernetes-unit Failure seems unrelated |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
|
@2rs2ts Thanks for doing this but it appears if I add tags (annotations) to an ELB after it's creation... the ELB tags update but the underlying SG don't. Is there anyway to update the tags? |
|
@cdenneen we'd probably need to add something to |
|
@ucodevault I am not sure, I feel like there was but I can't find an issue in my subscriptions about it. Feel free to open an issue about it. |
|
Ah, I was thinking of #95247, which isn't related. I think you should definitely open a new issue. |
Thanks for the info. @2rs2ts |
/sig aws
(I worked on this with @bkochendorfer)
Tags the SG created for the ELB with the same additional tags the ELB gets from the
service.beta.kubernetes.io/aws-load-balancer-additional-resource-tagsannotation. This is useful for identifying orphaned resources.We think that reusing the annotation is a simpler and less intrusive approach than adding a new annotation, and most users will want the same set of tags applied.
We weren't sure how to write a test for this because it looks like the fake EC2 code doesn't store the state of the security groups. If new tests are a requirement for merging, we'll need help writing them.
Fixes #53489