diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go b/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go index c74eef1199f..fbbaa6567d0 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go +++ b/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go @@ -233,6 +233,16 @@ const ServiceAnnotationLoadBalancerEIPAllocations = "service.beta.kubernetes.io/ // For example: "Key1=Val1,Key2=Val2,KeyNoVal1=,KeyNoVal2" const ServiceAnnotationLoadBalancerTargetNodeLabels = "service.beta.kubernetes.io/aws-load-balancer-target-node-labels" +// ServiceAnnotationLoadBalancerSubnets is the annotation used on the service to specify the +// Availability Zone configuration for the load balancer. The values are comma separated list of +// subnetID or subnetName from different AZs +// By default, the controller will auto-discover the subnets. If there are multiple subnets per AZ, auto-discovery +// will break the tie in the following order - +// 1. prefer the subnet with the correct role tag. kubernetes.io/role/elb for public and kubernetes.io/role/internal-elb for private access +// 2. prefer the subnet with the cluster tag kubernetes.io/cluster/ +// 3. prefer the subnet that is first in lexicographic order +const ServiceAnnotationLoadBalancerSubnets = "service.beta.kubernetes.io/aws-load-balancer-subnets" + // Event key when a volume is stuck on attaching state when being attached to a volume const volumeAttachmentStuck = "VolumeAttachmentStuck" @@ -3368,7 +3378,7 @@ func findTag(tags []*ec2.Tag, key string) (string, bool) { return "", false } -// Finds the subnets associated with the cluster, by matching tags. +// Finds the subnets associated with the cluster, by matching cluster tags if present. // For maximal backwards compatibility, if no subnets are tagged, it will fall-back to the current subnet. // However, in future this will likely be treated as an error. func (c *Cloud) findSubnets() ([]*ec2.Subnet, error) { @@ -3384,6 +3394,8 @@ func (c *Cloud) findSubnets() ([]*ec2.Subnet, error) { for _, subnet := range subnets { if c.tagging.hasClusterTag(subnet.Tags) { matches = append(matches, subnet) + } else if c.tagging.hasNoClusterPrefixTag(subnet.Tags) { + matches = append(matches, subnet) } } @@ -3447,7 +3459,7 @@ func (c *Cloud) findELBSubnets(internalELB bool) ([]string, error) { continue } - // Try to break the tie using a tag + // Try to break the tie using the role tag var tagName string if internalELB { tagName = TagNameSubnetInternalELB @@ -3465,8 +3477,17 @@ func (c *Cloud) findELBSubnets(internalELB bool) ([]string, error) { continue } + // Prefer the one with the cluster Tag + existingHasClusterTag := c.tagging.hasClusterTag(existing.Tags) + subnetHasClusterTag := c.tagging.hasClusterTag(subnet.Tags) + if existingHasClusterTag != subnetHasClusterTag { + if subnetHasClusterTag { + subnetsByAZ[az] = subnet + } + continue + } + // If we have two subnets for the same AZ we arbitrarily choose the one that is first lexicographically. - // TODO: Should this be an error. if strings.Compare(*existing.SubnetId, *subnet.SubnetId) > 0 { klog.Warningf("Found multiple subnets in AZ %q; choosing %q between subnets %q and %q", az, *subnet.SubnetId, *existing.SubnetId, *subnet.SubnetId) subnetsByAZ[az] = subnet @@ -3492,6 +3513,90 @@ func (c *Cloud) findELBSubnets(internalELB bool) ([]string, error) { return subnetIDs, nil } +func splitCommaSeparatedString(commaSeparatedString string) []string { + var result []string + parts := strings.Split(commaSeparatedString, ",") + for _, part := range parts { + part = strings.TrimSpace(part) + if len(part) == 0 { + continue + } + result = append(result, part) + } + return result +} + +// parses comma separated values from annotation into string slice, returns true if annotation exists +func parseStringSliceAnnotation(annotations map[string]string, annotation string, value *[]string) bool { + rawValue := "" + if exists := parseStringAnnotation(annotations, annotation, &rawValue); !exists { + return false + } + *value = splitCommaSeparatedString(rawValue) + return true +} + +func (c *Cloud) getLoadBalancerSubnets(service *v1.Service, internalELB bool) ([]string, error) { + var rawSubnetNameOrIDs []string + if exists := parseStringSliceAnnotation(service.Annotations, ServiceAnnotationLoadBalancerSubnets, &rawSubnetNameOrIDs); exists { + return c.resolveSubnetNameOrIDs(rawSubnetNameOrIDs) + } + return c.findELBSubnets(internalELB) +} + +func (c *Cloud) resolveSubnetNameOrIDs(subnetNameOrIDs []string) ([]string, error) { + var subnetIDs []string + var subnetNames []string + if len(subnetNameOrIDs) == 0 { + return []string{}, fmt.Errorf("unable to resolve empty subnet slice") + } + for _, nameOrID := range subnetNameOrIDs { + if strings.HasPrefix(nameOrID, "subnet-") { + subnetIDs = append(subnetIDs, nameOrID) + } else { + subnetNames = append(subnetNames, nameOrID) + } + } + var resolvedSubnets []*ec2.Subnet + if len(subnetIDs) > 0 { + req := &ec2.DescribeSubnetsInput{ + SubnetIds: aws.StringSlice(subnetIDs), + } + subnets, err := c.ec2.DescribeSubnets(req) + if err != nil { + return []string{}, err + } + resolvedSubnets = append(resolvedSubnets, subnets...) + } + if len(subnetNames) > 0 { + req := &ec2.DescribeSubnetsInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("tag:Name"), + Values: aws.StringSlice(subnetNames), + }, + { + Name: aws.String("vpc-id"), + Values: aws.StringSlice([]string{c.vpcID}), + }, + }, + } + subnets, err := c.ec2.DescribeSubnets(req) + if err != nil { + return []string{}, err + } + resolvedSubnets = append(resolvedSubnets, subnets...) + } + if len(resolvedSubnets) != len(subnetNameOrIDs) { + return []string{}, fmt.Errorf("expected to find %v, but found %v subnets", len(subnetNameOrIDs), len(resolvedSubnets)) + } + var subnets []string + for _, subnet := range resolvedSubnets { + subnets = append(subnets, aws.StringValue(subnet.SubnetId)) + } + return subnets, nil +} + func isSubnetPublic(rt []*ec2.RouteTable, subnetID string) (bool, error) { var subnetTable *ec2.RouteTable for _, table := range rt { @@ -3869,7 +3974,7 @@ func (c *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, apiS if isNLB(annotations) { // Find the subnets that the ELB will live in - subnetIDs, err := c.findELBSubnets(internalELB) + subnetIDs, err := c.getLoadBalancerSubnets(apiService, internalELB) if err != nil { klog.Errorf("Error listing subnets in VPC: %q", err) return nil, err @@ -4036,7 +4141,7 @@ func (c *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, apiS } // Find the subnets that the ELB will live in - subnetIDs, err := c.findELBSubnets(internalELB) + subnetIDs, err := c.getLoadBalancerSubnets(apiService, internalELB) if err != nil { klog.Errorf("Error listing subnets in VPC: %q", err) return nil, err diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go b/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go index 56fd43dd12c..cee17b26bf1 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go @@ -20,6 +20,7 @@ package aws import ( "context" + "errors" "fmt" "io" "math/rand" @@ -808,6 +809,356 @@ func constructRouteTable(subnetID string, public bool) *ec2.RouteTable { } } +func Test_findELBSubnets(t *testing.T) { + awsServices := newMockedFakeAWSServices(TestClusterID) + c, err := newAWSCloud(CloudConfig{}, awsServices) + if err != nil { + t.Errorf("Error building aws cloud: %v", err) + return + } + subnetA0000001 := &ec2.Subnet{ + AvailabilityZone: aws.String("us-west-2a"), + SubnetId: aws.String("subnet-a0000001"), + Tags: []*ec2.Tag{ + { + Key: aws.String(TagNameSubnetPublicELB), + Value: aws.String("1"), + }, + }, + } + subnetA0000002 := &ec2.Subnet{ + AvailabilityZone: aws.String("us-west-2a"), + SubnetId: aws.String("subnet-a0000002"), + Tags: []*ec2.Tag{ + { + Key: aws.String(TagNameSubnetPublicELB), + Value: aws.String("1"), + }, + }, + } + subnetA0000003 := &ec2.Subnet{ + AvailabilityZone: aws.String("us-west-2a"), + SubnetId: aws.String("subnet-a0000003"), + Tags: []*ec2.Tag{ + { + Key: aws.String(c.tagging.clusterTagKey()), + Value: aws.String("owned"), + }, + { + Key: aws.String(TagNameSubnetInternalELB), + Value: aws.String("1"), + }, + }, + } + subnetB0000001 := &ec2.Subnet{ + AvailabilityZone: aws.String("us-west-2b"), + SubnetId: aws.String("subnet-b0000001"), + Tags: []*ec2.Tag{ + { + Key: aws.String(c.tagging.clusterTagKey()), + Value: aws.String("owned"), + }, + { + Key: aws.String(TagNameSubnetPublicELB), + Value: aws.String("1"), + }, + }, + } + subnetB0000002 := &ec2.Subnet{ + AvailabilityZone: aws.String("us-west-2b"), + SubnetId: aws.String("subnet-b0000002"), + Tags: []*ec2.Tag{ + { + Key: aws.String(c.tagging.clusterTagKey()), + Value: aws.String("owned"), + }, + { + Key: aws.String(TagNameSubnetInternalELB), + Value: aws.String("1"), + }, + }, + } + subnetC0000001 := &ec2.Subnet{ + AvailabilityZone: aws.String("us-west-2c"), + SubnetId: aws.String("subnet-c0000001"), + Tags: []*ec2.Tag{ + { + Key: aws.String(c.tagging.clusterTagKey()), + Value: aws.String("owned"), + }, + { + Key: aws.String(TagNameSubnetInternalELB), + Value: aws.String("1"), + }, + }, + } + subnetOther := &ec2.Subnet{ + AvailabilityZone: aws.String("us-west-2c"), + SubnetId: aws.String("subnet-other"), + Tags: []*ec2.Tag{ + { + Key: aws.String(TagNameKubernetesClusterPrefix + "clusterid.other"), + Value: aws.String("owned"), + }, + { + Key: aws.String(TagNameSubnetInternalELB), + Value: aws.String("1"), + }, + }, + } + subnetNoTag := &ec2.Subnet{ + AvailabilityZone: aws.String("us-west-2c"), + SubnetId: aws.String("subnet-notag"), + } + + tests := []struct { + name string + subnets []*ec2.Subnet + routeTables map[string]bool + internal bool + want []string + }{ + { + name: "no subnets", + }, + { + name: "single tagged subnet", + subnets: []*ec2.Subnet{ + subnetA0000001, + }, + routeTables: map[string]bool{ + "subnet-a0000001": true, + }, + internal: false, + want: []string{"subnet-a0000001"}, + }, + { + name: "no matching public subnet", + subnets: []*ec2.Subnet{ + subnetA0000002, + }, + routeTables: map[string]bool{ + "subnet-a0000002": false, + }, + want: nil, + }, + { + name: "prefer role over cluster tag", + subnets: []*ec2.Subnet{ + subnetA0000001, + subnetA0000003, + }, + routeTables: map[string]bool{ + "subnet-a0000001": true, + "subnet-a0000003": true, + }, + want: []string{"subnet-a0000001"}, + }, + { + name: "prefer cluster tag", + subnets: []*ec2.Subnet{ + subnetC0000001, + subnetNoTag, + }, + want: []string{"subnet-c0000001"}, + }, + { + name: "include untagged", + subnets: []*ec2.Subnet{ + subnetA0000001, + subnetNoTag, + }, + routeTables: map[string]bool{ + "subnet-a0000001": true, + "subnet-notag": true, + }, + want: []string{"subnet-a0000001", "subnet-notag"}, + }, + { + name: "ignore some other cluster owned subnet", + subnets: []*ec2.Subnet{ + subnetB0000001, + subnetOther, + }, + routeTables: map[string]bool{ + "subnet-b0000001": true, + "subnet-other": true, + }, + want: []string{"subnet-b0000001"}, + }, + { + name: "prefer matching role", + subnets: []*ec2.Subnet{ + subnetB0000001, + subnetB0000002, + }, + routeTables: map[string]bool{ + "subnet-b0000001": false, + "subnet-b0000002": false, + }, + want: []string{"subnet-b0000002"}, + internal: true, + }, + { + name: "choose lexicographic order", + subnets: []*ec2.Subnet{ + subnetA0000001, + subnetA0000002, + }, + routeTables: map[string]bool{ + "subnet-a0000001": true, + "subnet-a0000002": true, + }, + want: []string{"subnet-a0000001"}, + }, + { + name: "everything", + subnets: []*ec2.Subnet{ + subnetA0000001, + subnetA0000002, + subnetB0000001, + subnetB0000002, + subnetC0000001, + subnetNoTag, + subnetOther, + }, + routeTables: map[string]bool{ + "subnet-a0000001": true, + "subnet-a0000002": true, + "subnet-b0000001": true, + "subnet-b0000002": true, + "subnet-c0000001": true, + "subnet-notag": true, + "subnet-other": true, + }, + want: []string{"subnet-a0000001", "subnet-b0000001", "subnet-c0000001"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + awsServices.ec2.RemoveSubnets() + awsServices.ec2.RemoveRouteTables() + for _, subnet := range tt.subnets { + awsServices.ec2.CreateSubnet(subnet) + } + routeTables := constructRouteTables(tt.routeTables) + for _, rt := range routeTables { + awsServices.ec2.CreateRouteTable(rt) + } + got, _ := c.findELBSubnets(tt.internal) + sort.Strings(tt.want) + sort.Strings(got) + assert.Equal(t, tt.want, got) + }) + } +} + +func Test_getLoadBalancerSubnets(t *testing.T) { + awsServices := newMockedFakeAWSServices(TestClusterID) + c, err := newAWSCloud(CloudConfig{}, awsServices) + if err != nil { + t.Errorf("Error building aws cloud: %v", err) + return + } + tests := []struct { + name string + service *v1.Service + subnets []*ec2.Subnet + internalELB bool + want []string + wantErr error + }{ + { + name: "no annotation", + service: &v1.Service{}, + }, + { + name: "annotation with no subnets", + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-subnets": "\t", + }, + }, + }, + wantErr: errors.New("unable to resolve empty subnet slice"), + }, + { + name: "subnet ids", + subnets: []*ec2.Subnet{ + { + AvailabilityZone: aws.String("us-west-2c"), + SubnetId: aws.String("subnet-a000001"), + }, + { + AvailabilityZone: aws.String("us-west-2b"), + SubnetId: aws.String("subnet-a000002"), + }, + }, + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-subnets": "subnet-a000001, subnet-a000002", + }, + }, + }, + want: []string{"subnet-a000001", "subnet-a000002"}, + }, + { + name: "subnet names", + subnets: []*ec2.Subnet{ + { + AvailabilityZone: aws.String("us-west-2c"), + SubnetId: aws.String("subnet-a000001"), + }, + { + AvailabilityZone: aws.String("us-west-2b"), + SubnetId: aws.String("subnet-a000002"), + }, + }, + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-subnets": "My Subnet 1, My Subnet 2 ", + }, + }, + }, + want: []string{"subnet-a000001", "subnet-a000002"}, + }, + { + name: "unable to find all subnets", + subnets: []*ec2.Subnet{ + { + AvailabilityZone: aws.String("us-west-2c"), + SubnetId: aws.String("subnet-a000001"), + }, + }, + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-subnets": "My Subnet 1, My Subnet 2, Test Subnet ", + }, + }, + }, + wantErr: errors.New("expected to find 3, but found 1 subnets"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + awsServices.ec2.RemoveSubnets() + for _, subnet := range tt.subnets { + awsServices.ec2.CreateSubnet(subnet) + } + got, err := c.getLoadBalancerSubnets(tt.service, tt.internalELB) + if tt.wantErr != nil { + assert.EqualError(t, err, tt.wantErr.Error()) + } else { + assert.Equal(t, tt.want, got) + } + }) + } +} + func TestSubnetIDsinVPC(t *testing.T) { awsServices := newMockedFakeAWSServices(TestClusterID) c, err := newAWSCloud(CloudConfig{}, awsServices) @@ -3064,3 +3415,54 @@ func TestCloud_buildNLBHealthCheckConfiguration(t *testing.T) { }) } } + +func Test_parseStringSliceAnnotation(t *testing.T) { + tests := []struct { + name string + annotation string + annotations map[string]string + want []string + wantExist bool + }{ + { + name: "empty annotation", + annotation: "test.annotation", + wantExist: false, + }, + { + name: "empty value", + annotation: "a1", + annotations: map[string]string{ + "a1": "\t, ,,", + }, + want: nil, + wantExist: true, + }, + { + name: "single value", + annotation: "a1", + annotations: map[string]string{ + "a1": " value 1 ", + }, + want: []string{"value 1"}, + wantExist: true, + }, + { + name: "multiple values", + annotation: "a1", + annotations: map[string]string{ + "a1": "subnet-1, subnet-2, My Subnet ", + }, + want: []string{"subnet-1", "subnet-2", "My Subnet"}, + wantExist: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var gotValue []string + gotExist := parseStringSliceAnnotation(tt.annotations, tt.annotation, &gotValue) + assert.Equal(t, tt.wantExist, gotExist) + assert.Equal(t, tt.want, gotValue) + }) + } +} diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/tags.go b/staging/src/k8s.io/legacy-cloud-providers/aws/tags.go index 9ec0cf67e2f..532e697fd2a 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/tags.go +++ b/staging/src/k8s.io/legacy-cloud-providers/aws/tags.go @@ -152,6 +152,15 @@ func (t *awsTagging) hasClusterTag(tags []*ec2.Tag) bool { return false } +func (t *awsTagging) hasNoClusterPrefixTag(tags []*ec2.Tag) bool { + for _, tag := range tags { + if strings.HasPrefix(aws.StringValue(tag.Key), TagNameKubernetesClusterPrefix) { + return false + } + } + return true +} + // Ensure that a resource has the correct tags // If it has no tags, we assume that this was a problem caused by an error in between creation and tagging, // and we add the tags. If it has a different cluster's tags, that is an error. diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/tags_test.go b/staging/src/k8s.io/legacy-cloud-providers/aws/tags_test.go index 66e850341f9..fb124f5d298 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/tags_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/aws/tags_test.go @@ -23,6 +23,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/stretchr/testify/assert" ) func TestFilterTags(t *testing.T) { @@ -182,3 +183,52 @@ func TestHasClusterTag(t *testing.T) { } } } + +func TestHasNoClusterPrefixTag(t *testing.T) { + awsServices := NewFakeAWSServices(TestClusterID) + c, err := newAWSCloud(CloudConfig{}, awsServices) + if err != nil { + t.Errorf("Error building aws cloud: %v", err) + return + } + tests := []struct { + name string + tags []*ec2.Tag + want bool + }{ + { + name: "no tags", + want: true, + }, + { + name: "no cluster tags", + tags: []*ec2.Tag{ + { + Key: aws.String("not a cluster tag"), + Value: aws.String("true"), + }, + }, + want: true, + }, + { + name: "contains cluster tags", + tags: []*ec2.Tag{ + { + Key: aws.String("tag1"), + Value: aws.String("value1"), + }, + { + Key: aws.String("kubernetes.io/cluster/test.cluster"), + Value: aws.String("owned"), + }, + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, c.tagging.hasNoClusterPrefixTag(tt.tags)) + }) + } +}