aws: Support for ELB tagging by users #45932

lpabon · 2017-05-17T03:33:54Z

This PR provides support for tagging AWS ELBs using information in an
annotation and provided as a list of comma separated key-value pairs.

Closes kubernetes/community#404

k8s-ci-robot · 2017-05-17T03:34:01Z

Hi @lpabon. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot 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.

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. I understand the commands that are listed here.

cblecker · 2017-05-17T03:39:41Z

/sig aws

lpabon · 2017-05-17T03:42:07Z

/assign @justinsb

ericchiang

Seems reasonable, particularly since there's already tags being included by this cloud provider plugin.

ericchiang · 2017-05-17T20:47:00Z

pkg/cloudprovider/providers/aws/aws.go

@@ -130,6 +130,11 @@ const ServiceAnnotationLoadBalancerSSLPorts = "service.beta.kubernetes.io/aws-lo
 // a HTTP listener is used.
 const ServiceAnnotationLoadBalancerBEProtocol = "service.beta.kubernetes.io/aws-load-balancer-backend-protocol"

+// ServiceAnnotationLoadBalancerAdditionalTagsi is the annotation used on the service
+// to specify a comma-separated list of key-value pairs which will be recorded as


Can you document an example of the key value pairs? e.g. key1=foo,key2=bar

looks like we have this (now?) on line 136

ericchiang · 2017-05-17T20:55:14Z

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

+
+		// Break up "Key=Val"
+		for _, tagSet := range tagList {
+			tag := strings.Split(strings.TrimSpace(tagSet), "=")


nit: What about values with "=" in them or keys with zero length? Maybe:

i := strings.Index(tagSet, "=") if i <= 0 || i+1 >= len(tagSet) { continue } additionalTags[tagSet[:i]] = tagSet[i+1:]

Will check if there's already something to implement this parsing.

I adjusted the unit tests to check for these types of keys and added the fix. Thanks for the note

justinsb

Some suggestions around value-less tags but then will look good to me

justinsb · 2017-05-19T16:06:45Z

pkg/cloudprovider/providers/aws/aws.go

@@ -130,6 +130,12 @@ const ServiceAnnotationLoadBalancerSSLPorts = "service.beta.kubernetes.io/aws-lo
 // a HTTP listener is used.
 const ServiceAnnotationLoadBalancerBEProtocol = "service.beta.kubernetes.io/aws-load-balancer-backend-protocol"

+// ServiceAnnotationLoadBalancerAdditionalTagsi is the annotation used on the service


Nit: typo (extra i)

justinsb · 2017-05-19T16:45:39Z

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

+			tag := strings.Split(strings.TrimSpace(tagSet), "=")
+			if len(tag) >= 2 && len(tag[0]) != 0 && len(tag[1]) != 0 {
+				// There is a key and a value, so save it
+				additionalTags[tag[0]] = tag[1]


We can have an empty Value, and I think we should support that. Either Key= or Key syntax (or both) would work for me

I realized someone wrote a pretty cool one for kops - we can just use that if you want:
https://github.com/kubernetes/kops/blob/master/cmd/kops/create_cluster.go#L969-L994

Good point, I'll make the change.

This PR provides support for tagging AWS ELBs using information in an annotation and provided as a list of comma separated key-value pairs. Closes kubernetes/community#404

lpabon · 2017-05-23T20:21:57Z

Hi, @justinsb @zmerlynn Any other comments or reviews needed?

justinsb · 2017-05-24T14:24:52Z

@k8s-bot ok to test

/lgtm

k8s-github-robot · 2017-05-24T14:24:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, lpabon

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cloudprovider/providers/aws/OWNERS~~ [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

lpabon · 2017-05-24T20:53:54Z

@k8s-bot unit test this issue: #46374

lpabon · 2017-05-24T20:56:17Z

@k8s-bot e2e test this issue: #43520

lpabon · 2017-05-25T05:11:02Z

@k8s-bot unit test this issue: #46365
@k8s-bot unit test this issue: #46383

justinsb · 2017-05-25T14:27:47Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

k8s-github-robot · 2017-05-25T18:46:04Z

Automatic merge from submit-queue (batch tested with PRs 45518, 46127, 46146, 45932, 45003)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 17, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 17, 2017

k8s-github-robot assigned zmerlynn and justinsb May 17, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels May 17, 2017

k8s-ci-robot added the sig/aws label May 17, 2017

ericchiang reviewed May 17, 2017

View reviewed changes

lpabon force-pushed the elbtag_pr branch from 8c95595 to 299aa04 Compare May 18, 2017 04:31

k8s-github-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 18, 2017

lpabon force-pushed the elbtag_pr branch from 299aa04 to 81db95b Compare May 18, 2017 15:15

justinsb reviewed May 19, 2017

View reviewed changes

lpabon force-pushed the elbtag_pr branch from 81db95b to 906dce5 Compare May 19, 2017 20:32

aws: Support for ELB tagging by users

67d2697

This PR provides support for tagging AWS ELBs using information in an annotation and provided as a list of comma separated key-value pairs. Closes kubernetes/community#404

lpabon force-pushed the elbtag_pr branch from 906dce5 to 67d2697 Compare May 19, 2017 20:34

philips mentioned this pull request May 23, 2017

Feature: Allow user to add extra tags to any AWS resource created by the cluster #46257

Closed

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 24, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels May 24, 2017

justinsb added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels May 24, 2017

ericchiang mentioned this pull request May 24, 2017

[flake] certificates controller unit test flake #46374

Closed

k8s-github-robot merged commit 29b3bb4 into kubernetes:master May 25, 2017

lpabon deleted the elbtag_pr branch May 25, 2017 19:26

ericchiang mentioned this pull request May 31, 2017

gce-multizone test has been broken since may 25 #46737

Closed

lpabon mentioned this pull request Sep 7, 2017

AWS CreateVolume should include tags #50898

Closed

wking mentioned this pull request Oct 16, 2018

kubernetes.io/cluster/CLUSTER_NAME tag missing from EIPs, NAT GWs, S3 buckets and R53 zones openshift/installer#458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aws: Support for ELB tagging by users #45932

aws: Support for ELB tagging by users #45932

lpabon commented May 17, 2017

k8s-ci-robot commented May 17, 2017

cblecker commented May 17, 2017

lpabon commented May 17, 2017

ericchiang left a comment

ericchiang May 17, 2017

bigkraig May 19, 2017

ericchiang May 17, 2017

lpabon May 18, 2017

justinsb left a comment

justinsb May 19, 2017

justinsb May 19, 2017

justinsb May 19, 2017

lpabon May 19, 2017

lpabon commented May 23, 2017

justinsb commented May 24, 2017

k8s-github-robot commented May 24, 2017

lpabon commented May 24, 2017

lpabon commented May 24, 2017

lpabon commented May 25, 2017

justinsb commented May 25, 2017

k8s-github-robot commented May 25, 2017

aws: Support for ELB tagging by users #45932

aws: Support for ELB tagging by users #45932

Conversation

lpabon commented May 17, 2017

k8s-ci-robot commented May 17, 2017

cblecker commented May 17, 2017

lpabon commented May 17, 2017

ericchiang 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

justinsb 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

lpabon commented May 23, 2017

justinsb commented May 24, 2017

k8s-github-robot commented May 24, 2017

lpabon commented May 24, 2017

lpabon commented May 24, 2017

lpabon commented May 25, 2017

justinsb commented May 25, 2017

k8s-github-robot commented May 25, 2017