Merge pull request #610 from mumoshu/fix-efs-per-node-pool

Fix elasticFileSystemId only on node pools

Author: mumoshu

core/controlplane/config/templates/cluster.yaml

@@ -455,6 +455,15 @@ worker:
 #      clusterAutoscalerSupport:
 #        enabled: true
 #
+#      # Mount an EFS only to a specific node pool.
+#      # This is a NFS share that will be available across a node pool through a hostPath volume on the "/efs" mountpoint
+#      #
+#      # If you'd like to configure an EFS mounted to every node, use the top-level `elasticFileSystemId` instead
+#      #
+#      # Beware that currently it doesn't work for a node pool in subnets managed by kube-aws.
+#      # In other words, this node pool must be in existing subnets managed by you by specifying subnets[].id to make this work.
+#      elasticFileSystemId: fs-47a2c22e
+#
 #      # This option has not yet been tested with rkt as container runtime
 #      ephemeralImageStorage:
 #        enabled: true
@@ -1022,16 +1031,26 @@ worker:
 # Use Calico for network policy.
 # useCalico: false
 
-# Create MountTargets for a pre-existing Elastic File System (Amazon EFS). Enter the resource id, eg "fs-47a2c22e"
+# Create MountTargets to subnets managed by kube-aws for a pre-existing Elastic File System (Amazon EFS),
+# and then mount to every node.
+#
+# This is for mounting an EFS to every node.
+# If you'd like to mount an EFS only to a specific node pool, set `worker.nodePools[].elasticFileSystemId` instead.
+#
+# Enter the resource id, eg "fs-47a2c22e"
 # This is a NFS share that will be available across the entire cluster through a hostPath volume on the "/efs" mountpoint
 #
 # You can create a new EFS volume using the CLI:
 # $ aws efs create-file-system --creation-token $(uuidgen)
 #
-# Beware that an EFS file system can not be mounted from mutiple subnets from the same availability zone and
-# therefore this feature won't work like you might have expected when you're deploying your cluster to an existing VPC and
-# wanted to mount an existing EFS to the subnet(s) created by kube-aws
-# See https://github.com/kubernetes-incubator/kube-aws/issues/208 for more information
+# Beware that:
+# * an EFS file system can not be mounted from multiple subnets from the same availability zone and
+#   therefore this feature won't work like you might have expected when you're deploying your cluster to an existing VPC and
+#   wanted to mount an existing EFS to the subnet(s) created by kube-aws
+#   See https://github.com/kubernetes-incubator/kube-aws/issues/208 for more information
+# * kube-aws doesn't create MountTargets for existing subnets(=NOT managed by kube-aws). If you'd like to bring your own subnets
+#   to be used by kube-aws, it is now your responsibility to configure the subnets, including creating MountTargets, accordingly
+#   to your requirements
 #elasticFileSystemId: fs-47a2c22e
 
 # Create shared persistent volume

core/nodepool/config/config.go

@@ -238,6 +238,15 @@ define one or more public subnets in cluster.yaml or explicitly reference privat
 		}
 	}
 
+	anySubnetIsManaged := false
+	for _, s := range c.Subnets {
+		anySubnetIsManaged = anySubnetIsManaged || s.ManageSubnet()
+	}
+
+	if anySubnetIsManaged && main.ElasticFileSystemID == "" && c.ElasticFileSystemID != "" {
+		return fmt.Errorf("elasticFileSystemId cannot be specified for a node pool in managed subnet(s), but was: %s", c.ElasticFileSystemID)
+	}
+
 	c.EtcdNodes = main.EtcdNodes
 	c.KubeResourcesAutosave = main.KubeResourcesAutosave
 

core/nodepool/config/deployment.go

@@ -116,9 +116,13 @@ func (c DeploymentSettings) WithDefaultsFrom(main cfg.DeploymentSettings) Deploy
 	c.Region = main.Region
 	c.ContainerRuntime = main.ContainerRuntime
 	c.KMSKeyARN = main.KMSKeyARN
-	// TODO Allow providing (1) one or more elasticFileSystemId's to be mounted (2) either cluster-wide or per node pool.
-	// It currently supports only one elasticFileSystemId to be mounted cluster-wide
-	c.ElasticFileSystemID = main.ElasticFileSystemID
+
+	// TODO Allow providing one or more elasticFileSystemId's to be mounted both per-node-pool/cluster-wide
+	// TODO Allow providing elasticFileSystemId to a node pool in managed subnets.
+	// Currently, per-node-pool elasticFileSystemId requires existing subnets configured by users to have appropriate MountTargets associated
+	if c.ElasticFileSystemID == "" {
+		c.ElasticFileSystemID = main.ElasticFileSystemID
+	}
 
 	return c
 }

test/integration/maincluster_test.go

@@ -521,6 +521,32 @@ worker:
 				},
 			},
 		},
+		{
+			context: "WithElasticFileSystemIdInSpecificNodePool",
+			configYaml: mainClusterYaml + `
+subnets:
+- name: existing1
+  id: subnet-12345
+  availabilityZone: us-west-1a
+worker:
+  nodePools:
+  - name: pool1
+    subnets:
+    - name: existing1
+    elasticFileSystemId: efs-12345
+  - name: pool2
+`,
+			assertConfig: []ConfigTester{
+				func(c *config.Config, t *testing.T) {
+					if c.NodePools[0].ElasticFileSystemID != "efs-12345" {
+						t.Errorf("Unexpected worker.nodePools[0].elasticFileSystemId: %s", c.NodePools[0].ElasticFileSystemID)
+					}
+					if c.NodePools[1].ElasticFileSystemID != "" {
+						t.Errorf("Unexpected worker.nodePools[1].elasticFileSystemId: %s", c.NodePools[1].ElasticFileSystemID)
+					}
+				},
+			},
+		},
 		{
 			context: "WithEtcdDataVolumeEncrypted",
 			configYaml: minimalValidConfigYaml + `
@@ -3310,6 +3336,23 @@ controller:
 			configYaml:           kubeAwsSettings.withClusterName("my.cluster").minimumValidClusterYaml(),
 			expectedErrorMessage: "clusterName(=my.cluster) is malformed. It must consist only of alphanumeric characters, colons, or hyphens",
 		},
+		{
+			context: "WithElasticFileSystemIdInSpecificNodePoolWithManagedSubnets",
+			configYaml: mainClusterYaml + `
+subnets:
+- name: managed1
+  availabilityZone: us-west-1a
+  instanceCIDR: 10.0.1.0/24
+worker:
+  nodePools:
+  - name: pool1
+    subnets:
+    - name: managed1
+    elasticFileSystemId: efs-12345
+  - name: pool2
+`,
+			expectedErrorMessage: "invalid node pool at index 0: elasticFileSystemId cannot be specified for a node pool in managed subnet(s), but was: efs-12345",
+		},
 		{
 			context: "WithEtcdAutomatedDisasterRecoveryRequiresAutomatedSnapshot",
 			configYaml: minimalValidConfigYaml + `