Use kubernetes.io/cluster/<cluster-id> tag for discovery (#96)

* Use kubernetes.io/cluster/<cluster-id> tag for discovery

Uses the 'official' `kubernetes.io/cluster/<cluster-id>` tag for
discovery to not depend on custom tags.

Fix #77

* fix dependency on CF created tags in order to be useable by kops created clusters
this introduced a fallback to switch from <v0.4.0 to v0.4.0 as documented

* fix readme Security Group section

* Add info about the instance clusterID tag to the upgrade guide

* Update README.md

use before instead of <

* use const

* fix requirements to match the new tag specs

README.md

@@ -21,6 +21,23 @@ This information is used to manage AWS resources for each ingress objects of the
 - Automatic forwarding of requests to all Worker Nodes, even with auto scaling
 - Automatic cleanup of unnecessary managed resources
 
+## Upgrade
+
+### <v0.4.0 to >=v0.4.0
+
+In versions before v.04.0 we used AWS Tags that were set by CloudFormation automatically to find
+some AWS resources.
+This behavior has been changed to use custom non cloudformation tags.
+
+In order to update to v0.4.0, you have to add the following tags to your AWs Loadbalancer
+SecurityGroup before updating:
+- `kubernetes:application=kube-ingress-aws-controller`
+- `kubernetes.io/cluster/<cluster-id>=owned`
+
+Additionally you must ensure that the instance where the ingress-controller is
+running has the clusterID tag `kubernetes.io/cluster/<cluster-id>=owned` set
+(was `ClusterID=<cluster-id>` before v0.4.0).
+
 ## Development Status
 
 This controller is a work in progress, under active development. It aims to be out-of-the-box useful for anyone
@@ -64,8 +81,8 @@ On startup, the controller discovers the AWS resources required for the controll
 
 2. The Security Group
 
-    Lookup of the "Name" tag of the Security Group matching the stack for the controller node and the
-    tag `aws:cloudformation:logical-id` matching the value `IngressLoadBalancerSecurityGroup`
+    Lookup of the `kubernetes.io/cluster/<cluster-id>` tag of the Security Group matching the clusterID for the controller node and `kubernetes:application` matching the value `kube-ingress-aws-controller` or as fallback for <v0.4.0
+    tag `aws:cloudformation:logical-id` matching the value `IngressLoadBalancerSecurityGroup` (only clusters created by CF).
 
 ### Creating Load Balancers
 
@@ -143,7 +160,7 @@ from other load balancers. The tag looks like this:
 
     `kubernetes:application` = `kube-ingress-aws-controller`
 
-They also share the `ClusterID` tag with other resources from the same CloudFormation stack.
+They also share the `kubernetes.io/cluster/<cluster-id>` tag with other resources from the cluster where it belongs.
 
 ### Deleting load balancers
 

aws/adapter.go

@@ -63,12 +63,9 @@ const (
 	DefaultCreationTimeout           = 5 * time.Minute
 	DefaultStackTTL                  = 5 * time.Minute
 
-	clusterIDTag = "ClusterID"
-	nameTag      = "Name"
+	nameTag = "Name"
 
-	kubernetesCreatorTag   = "kubernetes:application"
-	kubernetesCreatorValue = "kube-ingress-aws-controller"
-	certificateARNTag      = "ingress:certificate-arn"
+	certificateARNTag = "ingress:certificate-arn"
 )
 
 var (
@@ -327,9 +324,9 @@ func buildManifest(awsAdapter *Adapter) (*manifest, error) {
 		return nil, err
 	}
 
-	stackName := instanceDetails.name()
+	clusterID := instanceDetails.clusterID()
 
-	securityGroupDetails, err := findSecurityGroupWithNameTag(awsAdapter.ec2, stackName)
+	securityGroupDetails, err := findSecurityGroupWithClusterID(awsAdapter.ec2, clusterID)
 	if err != nil {
 		return nil, err
 	}

aws/cf.go

@@ -153,7 +153,7 @@ func createStack(svc cloudformationiface.CloudFormationAPI, spec *stackSpec) (st
 		},
 		Tags: []*cloudformation.Tag{
 			cfTag(kubernetesCreatorTag, kubernetesCreatorValue),
-			cfTag(clusterIDTag, spec.clusterID),
+			cfTag(clusterIDTagPrefix+spec.clusterID, resourceLifecycleOwned),
 		},
 		TemplateBody:     aws.String(template),
 		TimeoutInMinutes: aws.Int64(int64(spec.timeoutInMinutes)),
@@ -192,7 +192,7 @@ func updateStack(svc cloudformationiface.CloudFormationAPI, spec *stackSpec) (st
 		},
 		Tags: []*cloudformation.Tag{
 			cfTag(kubernetesCreatorTag, kubernetesCreatorValue),
-			cfTag(clusterIDTag, spec.clusterID),
+			cfTag(clusterIDTagPrefix+spec.clusterID, resourceLifecycleOwned),
 		},
 		TemplateBody: aws.String(template),
 	}
@@ -338,10 +338,8 @@ func isManagedStack(cfTags []*cloudformation.Tag, clusterID string) bool {
 	if tags[kubernetesCreatorTag] != kubernetesCreatorValue {
 		return false
 	}
-	if tags[clusterIDTag] != clusterID {
-		return false
-	}
-	return true
+	// TODO(sszuecs): remove 2nd condition, only for migration
+	return tags[clusterIDTagPrefix+clusterID] == resourceLifecycleOwned || tags[clusterIDTag] == clusterID
 }
 
 func convertCloudFormationTags(tags []*cloudformation.Tag) map[string]string {

aws/cf_test.go

@@ -123,21 +123,21 @@ func TestManagementAssertion(t *testing.T) {
 	}{
 		{"managed", []*cloudformation.Tag{
 			cfTag(kubernetesCreatorTag, kubernetesCreatorValue),
-			cfTag(clusterIDTag, "test-cluster"),
+			cfTag(clusterIDTagPrefix+"test-cluster", resourceLifecycleOwned),
 			cfTag("foo", "bar"),
 		}, true},
 		{"missing-cluster-tag", []*cloudformation.Tag{
 			cfTag(kubernetesCreatorTag, kubernetesCreatorValue),
 		}, false},
 		{"missing-kube-mgmt-tag", []*cloudformation.Tag{
-			cfTag(clusterIDTag, "test-cluster"),
+			cfTag(clusterIDTagPrefix+"test-cluster", resourceLifecycleOwned),
 		}, false},
 		{"missing-all-mgmt-tag", []*cloudformation.Tag{
 			cfTag("foo", "bar"),
 		}, false},
 		{"mismatch-cluster-tag", []*cloudformation.Tag{
 			cfTag(kubernetesCreatorTag, kubernetesCreatorValue),
-			cfTag(clusterIDTag, "other-cluster"),
+			cfTag(clusterIDTagPrefix+"other-cluster", resourceLifecycleOwned),
 			cfTag("foo", "bar"),
 		}, false},
 	} {
@@ -187,7 +187,7 @@ func TestFindingManagedStacks(t *testing.T) {
 							StackName: aws.String("managed-stack-not-ready"),
 							Tags: []*cloudformation.Tag{
 								cfTag(kubernetesCreatorTag, kubernetesCreatorValue),
-								cfTag(clusterIDTag, "test-cluster"),
+								cfTag(clusterIDTagPrefix+"test-cluster", resourceLifecycleOwned),
 								cfTag(certificateARNTag, "cert-arn"),
 							},
 							Outputs: []*cloudformation.Output{
@@ -200,7 +200,7 @@ func TestFindingManagedStacks(t *testing.T) {
 							StackStatus: aws.String(cloudformation.StackStatusCreateComplete),
 							Tags: []*cloudformation.Tag{
 								cfTag(kubernetesCreatorTag, kubernetesCreatorValue),
-								cfTag(clusterIDTag, "test-cluster"),
+								cfTag(clusterIDTagPrefix+"test-cluster", resourceLifecycleOwned),
 								cfTag(certificateARNTag, "cert-arn"),
 							},
 							Outputs: []*cloudformation.Output{
@@ -212,13 +212,13 @@ func TestFindingManagedStacks(t *testing.T) {
 							StackName: aws.String("managed-stack-not-ready"),
 							Tags: []*cloudformation.Tag{
 								cfTag(kubernetesCreatorTag, kubernetesCreatorValue),
-								cfTag(clusterIDTag, "test-cluster"),
+								cfTag(clusterIDTagPrefix+"test-cluster", resourceLifecycleOwned),
 							},
 						},
 						{
 							StackName: aws.String("unmanaged-stack"),
 							Tags: []*cloudformation.Tag{
-								cfTag(clusterIDTag, "test-cluster"),
+								cfTag(clusterIDTagPrefix+"test-cluster", resourceLifecycleOwned),
 							},
 						},
 						{
@@ -231,7 +231,7 @@ func TestFindingManagedStacks(t *testing.T) {
 							StackName: aws.String("belongs-to-other-cluster"),
 							Tags: []*cloudformation.Tag{
 								cfTag(kubernetesCreatorTag, kubernetesCreatorValue),
-								cfTag(clusterIDTag, "other-cluster"),
+								cfTag(clusterIDTagPrefix+"other-cluster", resourceLifecycleOwned),
 							},
 						},
 					},
@@ -244,9 +244,9 @@ func TestFindingManagedStacks(t *testing.T) {
 					certificateARN: "cert-arn",
 					targetGroupARN: "tg-arn",
 					tags: map[string]string{
-						kubernetesCreatorTag: kubernetesCreatorValue,
-						clusterIDTag:         "test-cluster",
-						certificateARNTag:    "cert-arn",
+						kubernetesCreatorTag:                kubernetesCreatorValue,
+						clusterIDTagPrefix + "test-cluster": resourceLifecycleOwned,
+						certificateARNTag:                   "cert-arn",
 					},
 				},
 			},
@@ -263,7 +263,7 @@ func TestFindingManagedStacks(t *testing.T) {
 							StackStatus: aws.String(cloudformation.StackStatusReviewInProgress),
 							Tags: []*cloudformation.Tag{
 								cfTag(kubernetesCreatorTag, kubernetesCreatorValue),
-								cfTag(clusterIDTag, "test-cluster"),
+								cfTag(clusterIDTagPrefix+"test-cluster", resourceLifecycleOwned),
 							},
 							Outputs: []*cloudformation.Output{
 								{OutputKey: aws.String(outputLoadBalancerDNSName), OutputValue: aws.String("example-notready.com")},
@@ -275,7 +275,7 @@ func TestFindingManagedStacks(t *testing.T) {
 							StackStatus: aws.String(cloudformation.StackStatusRollbackComplete),
 							Tags: []*cloudformation.Tag{
 								cfTag(kubernetesCreatorTag, kubernetesCreatorValue),
-								cfTag(clusterIDTag, "test-cluster"),
+								cfTag(clusterIDTagPrefix+"test-cluster", resourceLifecycleOwned),
 							},
 							Outputs: []*cloudformation.Output{
 								{OutputKey: aws.String(outputLoadBalancerDNSName), OutputValue: aws.String("example.com")},

aws/ec2.go

@@ -10,8 +10,12 @@ import (
 )
 
 const (
-	defaultInstanceName     = "unknown-instance"
 	defaultClusterID        = "unknown-cluster"
+	clusterIDTag            = "ClusterID" // TODO(sszuecs): deprecated fallback cleanup
+	clusterIDTagPrefix      = "kubernetes.io/cluster/"
+	resourceLifecycleOwned  = "owned"
+	kubernetesCreatorTag    = "kubernetes:application"
+	kubernetesCreatorValue  = "kube-ingress-aws-controller"
 	autoScalingGroupNameTag = "aws:autoscaling:groupName"
 	runningState            = 16 // See https://github.com/aws/aws-sdk-go/blob/master/service/ec2/api.go, type InstanceState
 )
@@ -27,16 +31,11 @@ type instanceDetails struct {
 	tags  map[string]string
 }
 
-func (id *instanceDetails) name() string {
-	if n, err := getNameTag(id.tags); err == nil {
-		return n
-	}
-	return defaultInstanceName
-}
-
 func (id *instanceDetails) clusterID() string {
-	if clusterID, err := getTag(id.tags, clusterIDTag); err == nil {
-		return clusterID
+	for name, value := range id.tags {
+		if strings.HasPrefix(name, clusterIDTagPrefix) && value == resourceLifecycleOwned {
+			return strings.TrimPrefix(name, clusterIDTagPrefix)
+		}
 	}
 	return defaultClusterID
 }
@@ -224,31 +223,31 @@ func isSubnetPublic(rt []*ec2.RouteTable, subnetID string) (bool, error) {
 	return false, nil
 }
 
-func findSecurityGroupWithNameTag(svc ec2iface.EC2API, nameTag string) (*securityGroupDetails, error) {
+func findSecurityGroupWithClusterID(svc ec2iface.EC2API, clusterID string) (*securityGroupDetails, error) {
 	params := &ec2.DescribeSecurityGroupsInput{
 		Filters: []*ec2.Filter{
 			{
 				Name: aws.String("tag-key"),
 				Values: []*string{
-					aws.String("Name"),
+					aws.String(clusterIDTagPrefix + clusterID),
 				},
 			},
 			{
 				Name: aws.String("tag-value"),
 				Values: []*string{
-					aws.String(nameTag),
+					aws.String(resourceLifecycleOwned),
 				},
 			},
 			{
 				Name: aws.String("tag-key"),
 				Values: []*string{
-					aws.String("aws:cloudformation:logical-id"),
+					aws.String(kubernetesCreatorTag),
 				},
 			},
 			{
 				Name: aws.String("tag-value"),
 				Values: []*string{
-					aws.String("IngressLoadBalancerSecurityGroup"),
+					aws.String(kubernetesCreatorValue),
 				},
 			},
 		},

aws/ec2_test.go

@@ -51,7 +51,7 @@ func TestFindingSecurityGroup(t *testing.T) {
 	} {
 		t.Run(fmt.Sprintf("%v", test.name), func(t *testing.T) {
 			ec2 := &mockEc2Client{outputs: test.responses}
-			got, err := findSecurityGroupWithNameTag(ec2, "foo")
+			got, err := findSecurityGroupWithClusterID(ec2, "foo")
 			assertResultAndError(t, test.want, got, test.wantError, err)
 		})
 	}
@@ -60,46 +60,37 @@ func TestFindingSecurityGroup(t *testing.T) {
 func TestInstanceDetails(t *testing.T) {
 	for _, test := range []struct {
 		given         instanceDetails
-		wantName      string
 		wantClusterID string
 	}{
 		{
 			given: instanceDetails{id: "this-should-be-fine", vpcID: "bar", tags: map[string]string{
-				nameTag:      "baz",
-				clusterIDTag: "zbr",
+				nameTag:                    "baz",
+				clusterIDTagPrefix + "zbr": resourceLifecycleOwned,
 			}},
-			wantName:      "baz",
 			wantClusterID: "zbr",
 		},
 		{
 			given: instanceDetails{id: "missing-name-tag", vpcID: "bar", tags: map[string]string{
-				clusterIDTag: "zbr",
+				clusterIDTagPrefix + "zbr": resourceLifecycleOwned,
 			}},
-			wantName:      defaultInstanceName,
 			wantClusterID: "zbr",
 		},
 		{
 			given: instanceDetails{id: "missing-cluster-id-tag", vpcID: "bar", tags: map[string]string{
 				nameTag: "baz",
 			}},
-			wantName:      "baz",
 			wantClusterID: defaultClusterID,
 		},
 		{
 			given:         instanceDetails{id: "missing-mgmt-tags", vpcID: "bar", tags: map[string]string{}},
-			wantName:      defaultInstanceName,
 			wantClusterID: defaultClusterID,
 		},
 		{
 			given:         instanceDetails{id: "nil-mgmt-tags", vpcID: "bar"},
-			wantName:      defaultInstanceName,
 			wantClusterID: defaultClusterID,
 		},
 	} {
 		t.Run(fmt.Sprintf("%v", test.given.id), func(t *testing.T) {
-			if test.given.name() != test.wantName {
-				t.Errorf("unexpected instance name. wanted %q, got %q", test.wantName, test.given.name())
-			}
 			if test.given.clusterID() != test.wantClusterID {
 				t.Errorf("unexpected cluster ID. wanted %q, got %q", test.wantClusterID, test.given.clusterID())
 			}

deploy/requirements.md

@@ -3,7 +3,9 @@
 This document describes the prerequisites for deploying the Kubernetes Ingress Controller on AWS.
 The following is needed:
 
-- an additional security group to allow traffic from the internet to the load balancers. This can be done by using the following cloud formation stack which needs to have the same name of the autoscaling group used for the nodes:
+- an additional security group to allow traffic from the internet to
+the load balancers. This can be done by using the following cloud
+formation stack which needs to have the same ClusterID as the EC2 instances:
 ```
 Resources:
   IngressLoadBalancerSecurityGroup:
@@ -15,11 +17,13 @@ Resources:
       - {CidrIp: 0.0.0.0/0, FromPort: 443, IpProtocol: tcp, ToPort: 443}
       VpcId: "$VPC_ID"
       Tags:
-        - Key: "ClusterID"
-          Value: "$CLUSTER_ID"
+        - Key: "kubernetes.io/cluster/$ClusterID"
+          Value: "owned"
+        - Key: "kubernetes:application"
+          Value: "kube-ingress-aws-controller"
 ```
 
-- Make the port used by `skipper` (in this example, port `9999`) accessible from the IP range of the VPC to allow health checking from the AWS Application Load Balancer (ALB).
+- Make the port used by your chosen ingress proxy (in this example `skipper`, port `9999`) accessible from the IP range of the VPC to allow health checking from the AWS Application Load Balancer (ALB).
 
 Please also note that the worker nodes will need the right permission to describe autoscaling groups, create load balancers and so on. The full list of required AWS IAM roles is the following: