github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2025-08-29 05:22:37 +00:00
Code Issues Projects Releases Wiki Activity

Merge pull request #58767 from 2rs2ts/tag-elb-sgs

Browse Source 
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Tag Security Group created for AWS ELB with same additional tags as ELB

/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-tags` annotation. 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

```release-note
AWS Security Groups created for ELBs will now be tagged with the same additional tags as the ELB (i.e. the tags specified by the "service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags" annotation.)
```
This commit is contained in:
Kubernetes Submit Queue 2018-02-25 11:59:53 -08:00 committed by GitHub
commit 62c5f21d5d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 20 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
pkg/cloudprovider/providers/aws/aws.go
View File

@ -2845,8 +2845,9 @@ 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.
// Additional tags can be specified
// 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, additionalTags map[string]string) (string, error) {
groupID := ""
attempt := 0
for {
@ -2912,7 +2913,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, additionalTags)
if err != nil {
// If we retry, ensureClusterTags will recover from this - it
// will add the missing tags. We could delete the security
@ -3126,9 +3127,10 @@ func getPortSets(annotation string) (ports *portSets) {
// buildELBSecurityGroupList returns list of SecurityGroups which should be
// attached to ELB created by a service. List always consist of at least
// 1 member which is an SG created for this service or a SG from the Global config. Extra groups can be
// specified via annotation
func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, loadBalancerName, annotation string) ([]string, error) {
// 1 member which is an SG created for this service or a SG from the Global config.
// Extra groups can be specified via annotation, as can extra tags for any
// new groups.
func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, loadBalancerName string, annotations map[string]string) ([]string, error) {
var err error
var securityGroupID string
@ -3138,7 +3140,7 @@ func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, load
// Create a security group for the load balancer
sgName := "k8s-elb-" + loadBalancerName
sgDescription := fmt.Sprintf("Security group for Kubernetes ELB %s (%v)", loadBalancerName, serviceName)
securityGroupID, err = c.ensureSecurityGroup(sgName, sgDescription)
securityGroupID, err = c.ensureSecurityGroup(sgName, sgDescription, getLoadBalancerAdditionalTags(annotations))
if err != nil {
glog.Errorf("Error creating load balancer security group: %q", err)
return nil, err
@ -3146,7 +3148,7 @@ func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, load
}
sgList := []string{securityGroupID}
for _, extraSG := range strings.Split(annotation, ",") {
for _, extraSG := range strings.Split(annotations[ServiceAnnotationLoadBalancerExtraSecurityGroups], ",") {
extraSG = strings.TrimSpace(extraSG)
if len(extraSG) > 0 {
sgList = append(sgList, extraSG)
@ -3444,7 +3446,7 @@ func (c *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, apiS
loadBalancerName := cloudprovider.GetLoadBalancerName(apiService)
serviceName := types.NamespacedName{Namespace: apiService.Namespace, Name: apiService.Name}
securityGroupIDs, err := c.buildELBSecurityGroupList(serviceName, loadBalancerName, annotations[ServiceAnnotationLoadBalancerExtraSecurityGroups])
securityGroupIDs, err := c.buildELBSecurityGroupList(serviceName, loadBalancerName, annotations)
if err != nil {
return nil, err
}

19
pkg/cloudprovider/providers/aws/aws_test.go
View File

@ -1132,19 +1132,20 @@ func TestLBExtraSecurityGroupsAnnotation(t *testing.T) {
awsServices := newMockedFakeAWSServices(TestClusterId)
c, _ := newAWSCloud(CloudConfig{}, awsServices)
sg1 := "sg-000001"
sg2 := "sg-000002"
sg1 := map[string]string{ServiceAnnotationLoadBalancerExtraSecurityGroups: "sg-000001"}
sg2 := map[string]string{ServiceAnnotationLoadBalancerExtraSecurityGroups: "sg-000002"}
sg3 := map[string]string{ServiceAnnotationLoadBalancerExtraSecurityGroups: "sg-000001, sg-000002"}
tests := []struct {
name string
extraSGsAnnotation string
expectedSGs []string
annotations map[string]string
expectedSGs []string
}{
{"No extra SG annotation", "", []string{}},
{"Empty extra SGs specified", ", ,,", []string{}},
{"SG specified", sg1, []string{sg1}},
{"Multiple SGs specified", fmt.Sprintf("%s, %s", sg1, sg2), []string{sg1, sg2}},
{"No extra SG annotation", map[string]string{}, []string{}},
{"Empty extra SGs specified", map[string]string{ServiceAnnotationLoadBalancerExtraSecurityGroups: ", ,,"}, []string{}},
{"SG specified", sg1, []string{sg1[ServiceAnnotationLoadBalancerExtraSecurityGroups]}},
{"Multiple SGs specified", sg3, []string{sg1[ServiceAnnotationLoadBalancerExtraSecurityGroups], sg2[ServiceAnnotationLoadBalancerExtraSecurityGroups]}},
}
awsServices.ec2.(*MockedFakeEC2).expectDescribeSecurityGroups(TestClusterId, "k8s-elb-aid", "cluster.test")
@ -1153,7 +1154,7 @@ func TestLBExtraSecurityGroupsAnnotation(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
serviceName := types.NamespacedName{Namespace: "default", Name: "myservice"}
sgList, err := c.buildELBSecurityGroupList(serviceName, "aid", test.extraSGsAnnotation)
sgList, err := c.buildELBSecurityGroupList(serviceName, "aid", test.annotations)
assert.NoError(t, err, "buildELBSecurityGroupList failed")
extraSGs := sgList[1:]
assert.True(t, sets.NewString(test.expectedSGs...).Equal(sets.NewString(extraSGs...)),