Allow to update subnets and security group IDs (#119)

Issue #, if available:
aws-controllers-k8s/community#1855

Description of changes:
Subnets or security group IDs can (now) be updated with the api but not at the same time as the public or private accesses.
This patch split the api calls for theses changes into two separate methods.

This also fix aws-controllers-k8s/community#1855 as the changes in subnets or security group IDs triggers an update but these field are removed before the call to the api. The api then response with an error because there is no changes to apply.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Author: vflaux

pkg/resource/cluster/hook.go

@@ -220,8 +220,27 @@ func (rm *resourceManager) customUpdate(
 		}
 		return returnClusterUpdating(updatedRes)
 	}
-	if delta.DifferentAt("Spec.ResourcesVPCConfig") {
-		if err := rm.updateConfigResourcesVPCConfig(ctx, desired); err != nil {
+	if delta.DifferentAt("Spec.ResourcesVPCConfig.EndpointPrivateAccess") ||
+		delta.DifferentAt("Spec.ResourcesVPCConfig.EndpointPublicAccess") ||
+		delta.DifferentAt("Spec.ResourcesVPCConfig.PublicAccessCIDRs") {
+		if err := rm.updateConfigResourcesVPCConfigPublicAndPrivateAccess(ctx, desired); err != nil {
+			awserr, ok := ackerr.AWSError(err)
+
+			// Check to see if we've raced an async update call and need to
+			// requeue
+			if ok && awserr.Code() == "ResourceInUseException" {
+				return nil, requeueAfterAsyncUpdate()
+			}
+
+			return nil, err
+		}
+		return returnClusterUpdating(updatedRes)
+	}
+	if delta.DifferentAt("Spec.ResourcesVPCConfig.SecurityGroupIDs") ||
+		delta.DifferentAt("Spec.ResourcesVPCConfig.SecurityGroupRefs") ||
+		delta.DifferentAt("Spec.ResourcesVPCConfig.SubnetIDs") ||
+		delta.DifferentAt("Spec.ResourcesVPCConfig.SubnetRefs") {
+		if err := rm.updateConfigResourcesVPCConfigSubnetsAndSecurityGroups(ctx, desired); err != nil {
 			awserr, ok := ackerr.AWSError(err)
 
 			// Check to see if we've raced an async update call and need to
@@ -406,21 +425,20 @@ func (rm *resourceManager) updateAccessConfig(
 	return nil
 }
 
-func (rm *resourceManager) updateConfigResourcesVPCConfig(
+func (rm *resourceManager) updateConfigResourcesVPCConfigPublicAndPrivateAccess(
 	ctx context.Context,
 	r *resource,
 ) (err error) {
 	rlog := ackrtlog.FromContext(ctx)
-	exit := rlog.Trace("rm.updateConfigResourcesVPCConfig")
+	exit := rlog.Trace("rm.updateConfigResourcesVPCConfigPublicAndPrivateAccess")
 	defer exit(err)
 	input := &svcsdk.UpdateClusterConfigInput{
 		Name:               r.ko.Spec.Name,
 		ResourcesVpcConfig: rm.newVpcConfigRequest(r),
 	}
 
-	// From the EKS documentation:
-	// "You can't update the subnets or security group IDs for an existing
-	// cluster."
+	// We only want to update endpointPrivateAccess, endpointPublicAccess and
+	// publicAccessCidrs
 	input.ResourcesVpcConfig.SetSubnetIds(nil)
 	input.ResourcesVpcConfig.SetSecurityGroupIds(nil)
 
@@ -433,6 +451,32 @@ func (rm *resourceManager) updateConfigResourcesVPCConfig(
 	return nil
 }
 
+func (rm *resourceManager) updateConfigResourcesVPCConfigSubnetsAndSecurityGroups(
+	ctx context.Context,
+	r *resource,
+) (err error) {
+	rlog := ackrtlog.FromContext(ctx)
+	exit := rlog.Trace("rm.updateConfigResourcesVPCConfigSubnetsAndSecurityGroups")
+	defer exit(err)
+	input := &svcsdk.UpdateClusterConfigInput{
+		Name:               r.ko.Spec.Name,
+		ResourcesVpcConfig: rm.newVpcConfigRequest(r),
+	}
+
+	// We only want to update securityGroupIds and subnetIds
+	input.ResourcesVpcConfig.EndpointPublicAccess = nil
+	input.ResourcesVpcConfig.EndpointPrivateAccess = nil
+	input.ResourcesVpcConfig.SetPublicAccessCidrs(nil)
+
+	_, err = rm.sdkapi.UpdateClusterConfigWithContext(ctx, input)
+	rm.metrics.RecordAPICall("UPDATE", "UpdateClusterConfig", err)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
 func (rm *resourceManager) listNodegroups(
 	ctx context.Context,
 	r *resource,

test/e2e/service_bootstrap.py

@@ -14,15 +14,26 @@
 """
 import logging
 
+import boto3
+from acktest.aws.identity import get_region
 from acktest.bootstrapping import Resources, BootstrapFailureException
 from acktest.bootstrapping.iam import Role
 from acktest.bootstrapping.vpc import VPC
 from e2e import bootstrap_directory
 from e2e.bootstrap_resources import BootstrapResources
 
+def get_availability_zone_names():
+    ec2_client = boto3.client("ec2", region_name=get_region())
+    zones = ec2_client.describe_availability_zones()
+    return list(map(lambda x: x['ZoneName'], zones['AvailabilityZones']))
+
 def service_bootstrap() -> Resources:
     logging.getLogger().setLevel(logging.INFO)
 
+    zones = get_availability_zone_names()
+    # We create one subnet more than the number of AZs in order to have the last subnet in the same AZ as the first one
+    num_public_subnet=len(zones) + 1
+
     resources = BootstrapResources(
         ClusterRole=Role("cluster-role", "eks.amazonaws.com", managed_policies=["arn:aws:iam::aws:policy/AmazonEKSClusterPolicy"]),
         FargatePodRole=Role("fargate-pod-role", "eks-fargate-pods.amazonaws.com", managed_policies=["arn:aws:iam::aws:policy/AmazonEKSFargatePodExecutionRolePolicy"]),
@@ -42,7 +53,7 @@ def service_bootstrap() -> Resources:
             "ack-access-entry-principal-role",
             "eks.amazonaws.com",
         ),
-        ClusterVPC=VPC(name_prefix="cluster-vpc", num_public_subnet=2, num_private_subnet=2)
+        ClusterVPC=VPC(name_prefix="cluster-vpc", num_public_subnet=num_public_subnet, num_private_subnet=2),
     )
 
     try:
@@ -55,4 +66,4 @@ def service_bootstrap() -> Resources:
 if __name__ == "__main__":
     config = service_bootstrap()
     # Write config to current directory by default
-    config.serialize(bootstrap_directory)
+    config.serialize(bootstrap_directory)

test/e2e/tests/test_cluster.py

@@ -17,14 +17,14 @@
 import boto3
 import logging
 import time
-from typing import Dict
 
 import pytest
 
 from acktest.k8s import resource as k8s
 from acktest.k8s import condition
 from acktest.resources import random_suffix_name
 from e2e import service_marker, CRD_GROUP, CRD_VERSION, load_eks_resource
+from e2e.bootstrap_resources import get_bootstrap_resources
 from e2e.common.types import CLUSTER_RESOURCE_PLURAL
 from e2e.common.waiter import wait_until_deleted
 from e2e.replacement_values import REPLACEMENT_VALUES
@@ -156,7 +156,7 @@ def test_create_update_delete_cluster(self, eks_client, simple_cluster):
 
         wait_for_cluster_active(eks_client, cluster_name)
 
-        # Update the logging and VPC config fields
+        # Update VPC endpoint public access config field
         updates = {
             "spec": {
                 "resourcesVPCConfig": {
@@ -180,6 +180,28 @@ def test_create_update_delete_cluster(self, eks_client, simple_cluster):
         aws_res = eks_client.describe_cluster(name=cluster_name)
         assert aws_res["cluster"]["resourcesVpcConfig"]["endpointPublicAccess"] == False
 
+        # Update the VPC subnets config field
+        vpc_subnets_ids = get_bootstrap_resources().ClusterVPC.public_subnets.subnet_ids
+        # We substitute the first subnet with the last one which is in the same AZ
+        subnets_ids = [vpc_subnets_ids[len(vpc_subnets_ids)-1], vpc_subnets_ids[1]]
+        
+        updates = {
+            "spec": {
+                "resourcesVPCConfig": {
+                    "subnetIDs": subnets_ids,
+                }
+            }
+        }
+
+        k8s.patch_custom_resource(ref, updates)
+        time.sleep(MODIFY_WAIT_AFTER_SECONDS)
+
+        wait_for_cluster_active(eks_client, cluster_name)
+
+        aws_res = eks_client.describe_cluster(name=cluster_name)
+        assert sorted(aws_res["cluster"]["resourcesVpcConfig"]["subnetIds"]) == sorted(subnets_ids)
+
+        # Update the logging fields
         updates = {
             "spec": {
                 "logging": {
@@ -245,7 +267,7 @@ def test_update_cluster_version(self, eks_client, simple_cluster_version_minus_2
 
         wait_for_cluster_active(eks_client, cluster_name)
 
-       # Bump two minor versions 1.27 -> 1.29
+        # Bump two minor versions 1.27 -> 1.29
         updates = {
             "spec": {
                 "version": "1.29"