Fixes an issue when changing between different target-type annotations

Author: bigkraig

docs/ingress-resources.md

@@ -97,7 +97,7 @@ Optional annotations are:
 
 - **listen-ports**: Defines the ports the ALB will expose. It defaults to `[{"HTTP": 80}]` unless a certificate ARN is defined, then it is `[{"HTTPS": 443}]`. Uses a format as follows '[{"HTTP":8080,"HTTPS": 443}]'.
 
-- **target-type**: Defines if the EC2 instance ID or the pod IP is added to the Target Groups. Defaults to `instance`. Valid options are `instance` and `pod`. With `instance` the Target Group targets are `<ec2 instance id>:<node port>`, for `pod` the targets are `<pod ip>:<pod port>`. When using the pod IP, it will route from all availabilty zones. `pod` is to be used when the pod network is routable and can be reached by the ALB.
+- **target-type**: Defines if the EC2 instance ID or the pod IP are used in the managed Target Groups. Defaults to `instance`. Valid options are `instance` and `pod`. With `instance` the Target Group targets are `<ec2 instance id>:<node port>`, for `pod` the targets are `<pod ip>:<pod port>`. When using the pod IP, it will route from all availabilty zones. `pod` is to be used when the pod network is routable and can be reached by the ALB.
 
 - **security-groups**: [Security groups](http://docs.aws.amazon.com/AmazonVPC/latest/UserGuide/VPC_SecurityGroups.html) that should be applied to the ALB instance. These can be referenced by security group IDs or the name tag associated with each security group. Example ID values are `sg-723a380a,sg-a6181ede,sg-a5181edd`. Example tag values are `appSG, webSG`. When the annotation is not present, the controller will create a security group with appropriate ports allowing access to `0.0.0.0/0` and attached to the ALB. It will also create a security group for instances that allows all TCP traffic when the source is the security group created for the ALB.
 

pkg/alb/rs/rule.go

@@ -111,9 +111,11 @@ func (r *Rule) Reconcile(rOpts *ReconcileOptions) error {
 			log.Prettify(r.rs.current.Priority),
 			log.Prettify(r.rs.current.Conditions))
 
-	// case *r.rs.desired.IsDefault: // rule is default (attached to listener), do nothing
-	// 	r.logger.Debugf("Found desired rule that is a default and is already created with its respective listener. Rule: %s", log.Prettify(r.rs.desired))
-	// 	r.rs.current = r.rs.desired
+	case *r.rs.desired.IsDefault:
+		// rule is default (attached to listener), do nothing
+		// Seems to happen automatically, if we try to change it we get an error:
+		// OperationNotPermitted: Default rule '<arn>' cannot be modified
+		r.rs.current = r.rs.desired
 
 	case r.rs.current == nil: // rule doesn't exist and should be created
 		r.logger.Infof("Start Rule creation.")
@@ -220,6 +222,9 @@ func (r *Rule) needsModification() bool {
 	case crs == nil:
 		r.logger.Debugf("Current is nil")
 		return true
+	case !util.DeepEqual(crs.Actions, drs.Actions):
+		r.logger.Debugf("Actions needs to be changed (%v != %v)", log.Prettify(crs.Actions), log.Prettify(drs.Actions))
+		return true
 	case !conditionsEqual(crs.Conditions, drs.Conditions):
 		r.logger.Debugf("Conditions needs to be changed (%v != %v)", log.Prettify(crs.Conditions), log.Prettify(drs.Conditions))
 		return true

pkg/alb/rs/rule_test.go

@@ -382,6 +382,7 @@ func genTG(arn, svcname string) *tg.TargetGroup {
 			Protocol:        aws.String("HTTP"),
 		},
 	})
+	tg.CopyCurrentToDesired(t)
 	return t
 }
 func TestTargetGroupArn(t *testing.T) {

pkg/alb/rs/rules.go

@@ -33,7 +33,7 @@ func NewCurrentRules(o *NewCurrentRulesOptions) (Rules, error) {
 		// TODO LOOKUP svcName based on TG
 		i, tg := o.TargetGroups.FindCurrentByARN(*r.Actions[0].TargetGroupArn)
 		if i < 0 {
-			return nil, fmt.Errorf("failed to find a target group associated with a rule. This should not be possible. Rule: %s", awsutil.Prettify(r.RuleArn))
+			return nil, fmt.Errorf("failed to find a target group associated with a rule. This should not be possible. Rule: %s, ARN: %s", awsutil.Prettify(r.RuleArn), *r.Actions[0].TargetGroupArn)
 		}
 
 		newRule := NewCurrentRule(&NewCurrentRuleOptions{

pkg/alb/tg/targetgroup.go

@@ -38,13 +38,14 @@ type NewDesiredTargetGroupOptions struct {
 func NewDesiredTargetGroup(o *NewDesiredTargetGroupOptions) *TargetGroup {
 	hasher := md5.New()
 	hasher.Write([]byte(o.LoadBalancerID))
-	name := hex.EncodeToString(hasher.Sum(nil))
 
 	targetType := aws.String("instance")
 	if *o.Annotations.TargetType == "pod" {
 		targetType = aws.String("ip")
+		hasher.Write([]byte(*targetType))
 	}
 
+	name := hex.EncodeToString(hasher.Sum(nil))
 	id := fmt.Sprintf("%.12s-%.5d-%.5s-%.7s", o.ALBNamePrefix, o.Port, *o.Annotations.BackendProtocol, name)
 
 	// TODO: Quick fix as we can't have the loadbalancer and target groups share pointers to the same

pkg/alb/tg/targetgroups.go

@@ -20,7 +20,7 @@ func (t TargetGroups) LookupBySvc(svc string, port int32) int {
 		if v == nil {
 			continue
 		}
-		if v.SvcName == svc && (v.SvcPort == port || v.SvcPort == 0) {
+		if v.SvcName == svc && (v.SvcPort == port || v.SvcPort == 0) && v.tg.desired != nil {
 			return p
 		}
 	}

pkg/alb/tg/types.go

@@ -60,3 +60,12 @@ const (
 	tagsModified
 	attributesModified
 )
+
+// CopyCurrentToDesired is used for testing other packages against tg
+func CopyCurrentToDesired(a *TargetGroup) {
+	if a != nil {
+		a.tg.desired = a.tg.current
+		a.tags.desired = a.tags.current
+		a.targets.desired = a.targets.current
+	}
+}

pkg/annotations/annotations.go

@@ -402,7 +402,7 @@ func (a *Annotations) setTargetType(annotations map[string]string) error {
 		a.TargetType = aws.String("instance")
 		return nil
 	case annotations[targetTypeKey] != "instance" && annotations[targetTypeKey] != "pod":
-		return fmt.Errorf("ALB Routing Type [%v] must be either `instance` or `pod`", annotations[targetTypeKey])
+		return fmt.Errorf("ALB Target Type [%v] must be either `instance` or `pod`", annotations[targetTypeKey])
 	}
 	a.TargetType = aws.String(annotations[targetTypeKey])
 	return nil

pkg/aws/albelbv2/elbv2.go

@@ -249,7 +249,7 @@ func (e *ELBV2) RemoveTargetGroup(arn *string) error {
 	for i := 0; i < deleteTargetGroupReattemptMax; i++ {
 		_, err := e.DeleteTargetGroup(in)
 		if err == nil {
-			break
+			return nil
 		}
 
 		if aerr, ok := err.(awserr.Error); ok {