Merge pull request #523 from zalando-incubator/legacy-target-access-mode

Add `Legacy` `target-access-mode` to enable upgrade from pre `0.12.17` version

README.md

@@ -36,18 +36,33 @@ This information is used to manage AWS resources for each ingress objects of the
 
 ## Upgrade
 
+### <v0.14.0 to >=v0.14.0
+
+Version `v0.14.0` makes `target-access-mode` flag required to make upgrading users aware of the [issue](https://github.com/zalando-incubator/kube-ingress-aws-controller/issues/507).
+
+New deployment of the controller should use `--target-access-mode=HostPort` or `--target-access-mode=AWSCNI`.
+
+To upgrade from `<v0.12.17` use `--target-access-mode=Legacy` - it is the same as `HostPort` but does not set target type and
+relies on CloudFormation to use `instance` as a default value.
+
+Note that changing later from `--target-access-mode=Legacy` will change target type in CloudFormation and trigger target group recreation and downtime.
+
+To upgrade from `>=v0.12.17` when `--target-access-mode` is not set use explicit `--target-access-mode=HostPort`.
+
 ### <v0.13.0 to >=0.13.0
 
 Version `v0.13.0` use Ingress version v1 as default. You can downgrade
 ingress version to earlier versions via flag. You will also need to
 allow the access via RBAC, see more information in [<v0.11.0 to >=0.11.0](#v0110-to-0110) below.
 
-### <v0.12.17 to >=v0.12.17
+### <v0.12.17 to <v0.14.0
 
 Please see [release note](https://github.com/zalando-incubator/kube-ingress-aws-controller/releases/tag/v0.12.17)
 and [issue](https://github.com/zalando-incubator/kube-ingress-aws-controller/issues/507)
 this update can cause 30s downtime, if you don't use AWS CNI mode.
 
+Please upgrade to `>=v0.14.0`.
+
 ### <v0.12.0 to <=0.12.16
 
 Version `v0.12.0` changes Network Load Balancer type handling if Application Load Balancer type feature is requested. See [Load Balancers types](#load-balancers-types) notes for details.
@@ -644,25 +659,30 @@ Those ports are now configured individually. If you relied on this behavior, ple
 
 ## AWS CNI Mode (experimental)
 
-The default operation mode of the controller (`target-access-mode=HostPort`) is to link the target groups to the autoscaling group. The target group type is `instance`, requiring the ingress pod to be accessible through a `HostNetwork` and `HostPort`.
+The common operation mode of the controller (`--target-access-mode=HostPort`) is to link the target groups to the autoscaling group.
+The target group type is `instance`, requiring the ingress pod to be accessible through a `HostNetwork` and `HostPort`.
 
-In *AWS CNI Mode* (`target-access-mode=AWSCNI`) the controller actively manages the target group members. Since AWS EKS cluster running AWS VPC CNI have their pods as first class members in the VPCs, they can receive the traffic directly, being managed through a target group type is `ip`, which means there is no necessity for the HostPort indirection.
+In *AWS CNI Mode* (`--target-access-mode=AWSCNI`) the controller actively manages the target group members.
+Since AWS EKS cluster running AWS VPC CNI have their pods as first class members in the VPCs, they can receive the traffic directly,
+being managed through a target group type is `ip`, which means there is no necessity for the HostPort indirection.
 
 ### Notes
 
 - For security reasons the HostPort requirement might be of concern
-- Direct management of the target group members is significantly faster compared to the AWS linked mode, but it requires a running controller for updates. As of now, the controller is not prepared for high availability replicated setup.
-- The registration and deregistration is synced with the pod lifecycle, hence a pod in terminating phase is deregistered from the target group before shut down.
+- Direct management of the target group members is significantly faster compared to the AWS linked mode, but it requires
+  a running controller for updates. As of now, the controller is not prepared for high availability replicated setup.
+- The registration and deregistration is synced with the pod lifecycle, hence a pod in terminating phase is deregistered
+  from the target group before shut down.
 - Ingress pods are not bound to nodes in CNI mode and the deployment can scale independently.
 
 ### Configuration options
 
-| access mode | HostNetwork | HostPort |                      Notes                      |
-| :---------: | :---------: | :------: | :---------------------------------------------: |
-| `HostPort`  |   `true`    |  `true`  | default setup                                   |
-| `AWSCNI`    |   `true`    |  `true`  | PodIP == HostIP: limited scaling and host bound |
-| `AWSCNI`    |   `false`   |  `true`  | PodIP != HostIP: limited scaling and host bound |
-| `AWSCNI`    |   `false`   | `false`  | free scaling, pod VPC CNI IP used               |
+| access mode | HostNetwork | HostPort |                      Notes                             |
+| :---------: | :---------: | :------: | :----------------------------------------------------: |
+| `HostPort`  |   `true`    |  `true`  | target group updated by ASG, see v0.14.0 release notes |
+| `AWSCNI`    |   `true`    |  `true`  | PodIP == HostIP: limited scaling and host bound        |
+| `AWSCNI`    |   `false`   |  `true`  | PodIP != HostIP: limited scaling and host bound        |
+| `AWSCNI`    |   `false`   | `false`  | free scaling, pod VPC CNI IP used                      |
 
 ## Trying it out
 

VERSION

@@ -1 +1 @@
-v0.13
+v0.14

aws/adapter.go

@@ -46,6 +46,7 @@ type Adapter struct {
 	albHealthyThresholdCount    uint
 	albUnhealthyThresholdCount  uint
 	nlbHealthyThresholdCount    uint
+	targetType                  string
 	targetPort                  uint
 	albHTTPTargetPort           uint
 	nlbHTTPTargetPort           uint
@@ -133,8 +134,10 @@ const (
 	LoadBalancerTypeNetwork     = "network"
 	IPAddressTypeIPV4           = "ipv4"
 	IPAddressTypeDualstack      = "dualstack"
-	TargetAccessModeAWSCNI      = "AWSCNI"
-	TargetAccessModeHostPort    = "HostPort"
+
+	TargetAccessModeAWSCNI   = "AWSCNI"
+	TargetAccessModeHostPort = "HostPort"
+	TargetAccessModeLegacy   = "Legacy"
 )
 
 var (
@@ -443,8 +446,17 @@ func (a *Adapter) WithInternalDomains(domains []string) *Adapter {
 }
 
 // WithTargetAccessMode returns the receiver adapter after defining the target access mode
-func (a *Adapter) WithTargetAccessMode(t string) *Adapter {
-	a.TargetCNI.Enabled = t == TargetAccessModeAWSCNI
+func (a *Adapter) WithTargetAccessMode(mode string) *Adapter {
+	a.TargetCNI.Enabled = mode == TargetAccessModeAWSCNI
+
+	switch mode {
+	case TargetAccessModeHostPort:
+		a.targetType = elbv2.TargetTypeEnumInstance
+	case TargetAccessModeAWSCNI:
+		a.targetType = elbv2.TargetTypeEnumIp
+	case TargetAccessModeLegacy:
+		a.targetType = ""
+	}
 	return a
 }
 
@@ -669,6 +681,7 @@ func (a *Adapter) CreateStack(certificateARNs []string, scheme, securityGroup, o
 		albHealthyThresholdCount:          a.albHealthyThresholdCount,
 		albUnhealthyThresholdCount:        a.albUnhealthyThresholdCount,
 		nlbHealthyThresholdCount:          a.nlbHealthyThresholdCount,
+		targetType:                        a.targetType,
 		targetPort:                        a.targetPort,
 		targetHTTPS:                       a.targetHTTPS,
 		httpDisabled:                      a.httpDisabled(loadBalancerType),
@@ -690,7 +703,6 @@ func (a *Adapter) CreateStack(certificateARNs []string, scheme, securityGroup, o
 		http2:                             http2,
 		tags:                              a.stackTags,
 		internalDomains:                   a.internalDomains,
-		targetAccessModeCNI:               a.TargetCNI.Enabled,
 		denyInternalDomains:               a.denyInternalDomains,
 		denyInternalDomainsResponse: denyResp{
 			body:        a.denyInternalRespBody,
@@ -725,6 +737,7 @@ func (a *Adapter) UpdateStack(stackName string, certificateARNs map[string]time.
 		albHealthyThresholdCount:          a.albHealthyThresholdCount,
 		albUnhealthyThresholdCount:        a.albUnhealthyThresholdCount,
 		nlbHealthyThresholdCount:          a.nlbHealthyThresholdCount,
+		targetType:                        a.targetType,
 		targetPort:                        a.targetPort,
 		targetHTTPS:                       a.targetHTTPS,
 		httpDisabled:                      a.httpDisabled(loadBalancerType),
@@ -746,7 +759,6 @@ func (a *Adapter) UpdateStack(stackName string, certificateARNs map[string]time.
 		http2:                             http2,
 		tags:                              a.stackTags,
 		internalDomains:                   a.internalDomains,
-		targetAccessModeCNI:               a.TargetCNI.Enabled,
 		denyInternalDomains:               a.denyInternalDomains,
 		denyInternalDomainsResponse: denyResp{
 			body:        a.denyInternalRespBody,

aws/adapter_test.go

@@ -9,6 +9,8 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/elbv2"
+
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -1011,14 +1013,25 @@ func TestAdapter_SetTargetsOnCNITargetGroups(t *testing.T) {
 }
 
 func TestWithTargetAccessMode(t *testing.T) {
-	t.Run("WithTargetAccessMode enables AWS CNI mode", func(t *testing.T) {
+	t.Run("WithTargetAccessMode AWSCNI", func(t *testing.T) {
 		a := &Adapter{TargetCNI: &TargetCNIconfig{Enabled: false}}
 		a = a.WithTargetAccessMode("AWSCNI")
-		require.True(t, a.TargetCNI.Enabled)
+
+		assert.Equal(t, elbv2.TargetTypeEnumIp, a.targetType)
+		assert.True(t, a.TargetCNI.Enabled)
 	})
-	t.Run("WithTargetAccessMode disables AWS CNI mode", func(t *testing.T) {
+	t.Run("WithTargetAccessMode HostPort", func(t *testing.T) {
 		a := &Adapter{TargetCNI: &TargetCNIconfig{Enabled: true}}
 		a = a.WithTargetAccessMode("HostPort")
-		require.False(t, a.TargetCNI.Enabled)
+
+		assert.Equal(t, elbv2.TargetTypeEnumInstance, a.targetType)
+		assert.False(t, a.TargetCNI.Enabled)
+	})
+	t.Run("WithTargetAccessMode Legacy", func(t *testing.T) {
+		a := &Adapter{TargetCNI: &TargetCNIconfig{Enabled: true}}
+		a = a.WithTargetAccessMode("Legacy")
+
+		assert.Equal(t, "", a.targetType)
+		assert.False(t, a.TargetCNI.Enabled)
 	})
 }

aws/cf.go

@@ -163,6 +163,7 @@ type stackSpec struct {
 	albHealthyThresholdCount          uint
 	albUnhealthyThresholdCount        uint
 	nlbHealthyThresholdCount          uint
+	targetType                        string
 	targetPort                        uint
 	targetHTTPS                       bool
 	httpDisabled                      bool
@@ -186,7 +187,6 @@ type stackSpec struct {
 	denyInternalDomainsResponse       denyResp
 	internalDomains                   []string
 	tags                              map[string]string
-	targetAccessModeCNI               bool
 }
 
 type healthCheck struct {

aws/cf_template.go

@@ -8,7 +8,6 @@ import (
 	"crypto/sha256"
 	"sort"
 
-	"github.com/aws/aws-sdk-go/service/elbv2"
 	cloudformation "github.com/mweagle/go-cloudformation"
 )
 
@@ -445,10 +444,11 @@ func generateDenyInternalTrafficRule(listenerName string, rulePriority int64, in
 }
 
 func newTargetGroup(spec *stackSpec, targetPortParameter string) *cloudformation.ElasticLoadBalancingV2TargetGroup {
-	targetType := elbv2.TargetTypeEnumInstance
-	if spec.targetAccessModeCNI {
-		targetType = elbv2.TargetTypeEnumIp
+	var targetType *cloudformation.StringExpr
+	if spec.targetType != "" {
+		targetType = cloudformation.String(spec.targetType)
 	}
+
 	protocol := "HTTP"
 	healthCheckProtocol := "HTTP"
 	healthyThresholdCount, unhealthyThresholdCount := spec.albHealthyThresholdCount, spec.albUnhealthyThresholdCount
@@ -477,7 +477,7 @@ func newTargetGroup(spec *stackSpec, targetPortParameter string) *cloudformation
 		UnhealthyThresholdCount:    cloudformation.Integer(int64(unhealthyThresholdCount)),
 		Port:                       cloudformation.Ref(targetPortParameter).Integer(),
 		Protocol:                   cloudformation.String(protocol),
-		TargetType:                 cloudformation.String(targetType),
+		TargetType:                 targetType,
 		VPCID:                      cloudformation.Ref(parameterTargetGroupVPCIDParameter).String(),
 	}
 

aws/cf_template_test.go

@@ -615,28 +615,42 @@ func TestGenerateTemplate(t *testing.T) {
 			},
 		},
 		{
-			name: "Default TG type is Instance",
+			name: "Default TG type is not set",
 			spec: &stackSpec{
 				loadbalancerType: LoadBalancerTypeApplication,
+				targetType:       "",
 			},
 			validate: func(t *testing.T, template *cloudformation.Template) {
 				require.NotNil(t, template.Resources, "TG")
-				tg, ok := template.Resources["TG"].Properties.(*cloudformation.ElasticLoadBalancingV2TargetGroup)
-				require.True(t, ok, "couldn't convert resource to ElasticLoadBalancingV2TargetGroup")
-				require.Equal(t, cloudformation.String("instance"), tg.TargetType)
+				tg := template.Resources["TG"].Properties.(*cloudformation.ElasticLoadBalancingV2TargetGroup)
+
+				assert.Nil(t, tg.TargetType)
 			},
 		},
 		{
-			name: "TG type is IP in in mode AWS CNI",
+			name: "Sets 'instance' TG type",
 			spec: &stackSpec{
-				loadbalancerType:    LoadBalancerTypeApplication,
-				targetAccessModeCNI: true,
+				loadbalancerType: LoadBalancerTypeApplication,
+				targetType:       "instance",
 			},
 			validate: func(t *testing.T, template *cloudformation.Template) {
 				require.NotNil(t, template.Resources, "TG")
-				tg, ok := template.Resources["TG"].Properties.(*cloudformation.ElasticLoadBalancingV2TargetGroup)
-				require.True(t, ok, "couldn't convert resource to ElasticLoadBalancingV2TargetGroup")
-				require.Equal(t, cloudformation.String("ip"), tg.TargetType)
+				tg := template.Resources["TG"].Properties.(*cloudformation.ElasticLoadBalancingV2TargetGroup)
+
+				assert.Equal(t, cloudformation.String("instance"), tg.TargetType)
+			},
+		},
+		{
+			name: "Sets 'ip' TG type",
+			spec: &stackSpec{
+				loadbalancerType: LoadBalancerTypeApplication,
+				targetType:       "ip",
+			},
+			validate: func(t *testing.T, template *cloudformation.Template) {
+				require.NotNil(t, template.Resources, "TG")
+				tg := template.Resources["TG"].Properties.(*cloudformation.ElasticLoadBalancingV2TargetGroup)
+
+				assert.Equal(t, cloudformation.String("ip"), tg.TargetType)
 			},
 		},
 	} {

controller.go

@@ -187,8 +187,12 @@ func loadSettings() error {
 		Default("text/plain").StringVar(&denyInternalRespContentType)
 	kingpin.Flag("deny-internal-domains-response-status-code", "Defines the response status code for a request identified as to an internal domain when -deny-internal-domains is set.").
 		Default("401").IntVar(&denyInternalRespStatusCode)
-	kingpin.Flag("target-access-mode", "Target group accessing Ingress via HostPort or AWS VPC CNI. Set to ASG for HostPort access or CNI for pod direct IP access.").
-		Default(aws.TargetAccessModeHostPort).EnumVar(&targetAccessMode, aws.TargetAccessModeHostPort, aws.TargetAccessModeAWSCNI)
+	kingpin.Flag("target-access-mode", "Defines target type of the target groups in CloudFormation and how loadbalancer targets are discovered. "+
+		"HostPort sets target type to 'instance' and discovers EC2 instances using AWS API and instance filters. "+
+		"AWSCNI sets target type to 'ip' and discovers target IPs using Kubernetes API and Pod label selector. "+
+		"Legacy is the same as HostPort but does not set target type and relies on CloudFormation to use 'instance' as a default value. "+
+		"Changing value from 'Legacy' to 'HostPort' will change target type in CloudFormation and trigger target group recreation and downtime.").
+		Required().EnumVar(&targetAccessMode, aws.TargetAccessModeHostPort, aws.TargetAccessModeAWSCNI, aws.TargetAccessModeLegacy)
 	kingpin.Flag("target-cni-namespace", "AWS VPC CNI only. Defines the namespace for ingress pods that should be linked to target group.").StringVar(&targetCNINamespace)
 	// LabelSelector semantics https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors
 	kingpin.Flag("target-cni-pod-labelselector", "AWS VPC CNI only. Defines the labelselector for ingress pods that should be linked to target group. Supports simple equality and multi value form (a=x,b=y) as well as complex forms (a IN (x,y,z).").StringVar(&targetCNIPodLabelSelector)