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

…` version Before version `0.12.17` loadbalancer target group target type was not specified and defaulted to `instance` in CloudFormation, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-elasticloadbalancingv2-targetgroup.html#cfn-elasticloadbalancingv2-targetgroup-targettype PR #461 introduced AWS CNI mode and configured target group target type either to `ip` or `instance`. Changing target type from unset to `instance` in Cloudformation triggers target group re-creation which makes it impossible to upgrade from pre `0.12.17` without downtime. This change: - makes `target-access-mode` flag required to force users to choose proper value - renames `HostPort` option of `target-access-mode` into `InstanceFilter` - introduces a new `Legacy` option that does not set target type and thus enables upgrade from pre `0.12.17`. Fixes #507 Signed-off-by: Alexander Yastrebov <[email protected]>
zalando-incubator · Nov 3, 2022 · 8a5eef4 · 8a5eef4
1 parent cc59821
commit 8a5eef4
Show file tree

Hide file tree

Showing 8 changed files with 99 additions and 36 deletions.
diff --git a/README.md b/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=InstanceFilter` or `--target-access-mode=AWSCNI`.
+
+To upgrade from `<v0.12.17` use `--target-access-mode=Legacy` - it is the same as `InstanceFilter` but does not set target type and
+relies on CloudFormation to use `instance` as a default value.
+
+Note that using `--target-access-mode=InstanceFilter` or 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 (i.e. default) or set to `HostPort` use `--target-access-mode=InstanceFilter`.
+
 ### <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,22 +659,27 @@ 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=InstanceFilter`) 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                                   |
+| `InstanceFilter`  |   `true`    |  `true`  | target group updated by ASG                          |
 | `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               |

diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-v0.13
+v0.14
diff --git a/aws/adapter.go b/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"
+
+	TargetAccessModeInstanceFilter = "InstanceFilter"
+	TargetAccessModeAWSCNI         = "AWSCNI"
+	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 TargetAccessModeInstanceFilter:
+		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,

diff --git a/aws/adapter_test.go b/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 InstanceFilter", func(t *testing.T) {
+		a := &Adapter{TargetCNI: &TargetCNIconfig{Enabled: true}}
+		a = a.WithTargetAccessMode("InstanceFilter")
+
+		assert.Equal(t, elbv2.TargetTypeEnumInstance, a.targetType)
+		assert.False(t, a.TargetCNI.Enabled)
 	})
-	t.Run("WithTargetAccessMode disables AWS CNI mode", func(t *testing.T) {
+	t.Run("WithTargetAccessMode Legacy", func(t *testing.T) {
 		a := &Adapter{TargetCNI: &TargetCNIconfig{Enabled: true}}
-		a = a.WithTargetAccessMode("HostPort")
-		require.False(t, a.TargetCNI.Enabled)
+		a = a.WithTargetAccessMode("Legacy")
+
+		assert.Equal(t, "", a.targetType)
+		assert.False(t, a.TargetCNI.Enabled)
 	})
 }
diff --git a/aws/cf.go b/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 {

diff --git a/aws/cf_template.go b/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(),
 	}
 

diff --git a/aws/cf_template_test.go b/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)
 			},
 		},
 	} {

diff --git a/controller.go b/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. "+
+		"InstanceFilter 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 InstanceFilter but does not set target type and relies on CloudFormation to use 'instance' as a default value. "+
+		"Changing value from 'Legacy' to 'InstanceFilter' will change target type in CloudFormation and trigger target group recreation and downtime.").
+		Required().EnumVar(&targetAccessMode, aws.TargetAccessModeInstanceFilter, 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)