Merge pull request #760 from mumoshu/vpc-igw-x-stack-ref

Support cross-stack references of VPC, IGW

Author: mumoshu

core/controlplane/cluster/cluster.go

@@ -54,24 +54,32 @@ type ec2Service interface {
 }
 
 func (c *ClusterRef) validateExistingVPCState(ec2Svc ec2Service) error {
-	if c.VPCID == "" {
+	if !c.VPC.HasIdentifier() {
 		//The VPC will be created. No existing state to validate
 		return nil
 	}
 
+	// TODO kube-aws should de-reference the vpc id from the stack output and continue validating with it
+	if c.VPC.IDFromStackOutput != "" {
+		fmt.Printf("kube-aws doesn't support validating the vpc referenced by the stack output `%s`. Skipped validation of exsiting vpc state. The cluster creation may fail afterwards if the VPC isn't configured properly.", c.VPC.IDFromStackOutput)
+		return nil
+	}
+
+	vpcId := c.VPC.ID
+
 	describeVpcsInput := ec2.DescribeVpcsInput{
-		VpcIds: []*string{aws.String(c.VPCID)},
+		VpcIds: []*string{aws.String(vpcId)},
 	}
 	vpcOutput, err := ec2Svc.DescribeVpcs(&describeVpcsInput)
 	if err != nil {
 		return fmt.Errorf("error describing existing vpc: %v", err)
 	}
 	if len(vpcOutput.Vpcs) == 0 {
-		return fmt.Errorf("could not find vpc %s in region %s", c.VPCID, c.Region)
+		return fmt.Errorf("could not find vpc %s in region %s", vpcId, c.Region)
 	}
 	if len(vpcOutput.Vpcs) > 1 {
 		//Theoretically this should never happen. If it does, we probably want to know.
-		return fmt.Errorf("found more than one vpc with id %s. this is NOT NORMAL", c.VPCID)
+		return fmt.Errorf("found more than one vpc with id %s. this is NOT NORMAL", vpcId)
 	}
 
 	existingVPC := vpcOutput.Vpcs[0]

core/controlplane/config/config.go

@@ -228,6 +228,8 @@ func (c *Cluster) Load() error {
 
 	c.HostedZoneID = withHostedZoneIDPrefix(c.HostedZoneID)
 
+	c.ConsumeDeprecatedKeys()
+
 	if err := c.valid(); err != nil {
 		return fmt.Errorf("invalid cluster: %v", err)
 	}
@@ -259,6 +261,19 @@ func (c *Cluster) Load() error {
 	return nil
 }
 
+func (c *Cluster) ConsumeDeprecatedKeys() {
+	// TODO Remove in v0.9.9-rc.1
+	if c.DeprecatedVPCID != "" {
+		fmt.Println("WARN: vpcId is deprecated and will be removed in v0.9.9. Please use vpc.id instead")
+		c.VPC.ID = c.DeprecatedVPCID
+	}
+
+	if c.DeprecatedInternetGatewayID != "" {
+		fmt.Println("WARN: internetGatewayId is deprecated and will be removed in v0.9.9. Please use internetGateway.id instead")
+		c.InternetGateway.ID = c.DeprecatedInternetGatewayID
+	}
+}
+
 func (c *Cluster) SetDefaults() {
 	// For backward-compatibility
 	if len(c.Subnets) == 0 {
@@ -380,15 +395,17 @@ type ComputedDeploymentSettings struct {
 // Though it is highly configurable, it's basically users' responsibility to provide `correct` values if they're going beyond the defaults.
 type DeploymentSettings struct {
 	ComputedDeploymentSettings
-	ClusterName       string       `yaml:"clusterName,omitempty"`
-	KeyName           string       `yaml:"keyName,omitempty"`
-	Region            model.Region `yaml:",inline"`
-	AvailabilityZone  string       `yaml:"availabilityZone,omitempty"`
-	ReleaseChannel    string       `yaml:"releaseChannel,omitempty"`
-	AmiId             string       `yaml:"amiId,omitempty"`
-	VPCID             string       `yaml:"vpcId,omitempty"`
-	InternetGatewayID string       `yaml:"internetGatewayId,omitempty"`
-	RouteTableID      string       `yaml:"routeTableId,omitempty"`
+	ClusterName                 string                `yaml:"clusterName,omitempty"`
+	KeyName                     string                `yaml:"keyName,omitempty"`
+	Region                      model.Region          `yaml:",inline"`
+	AvailabilityZone            string                `yaml:"availabilityZone,omitempty"`
+	ReleaseChannel              string                `yaml:"releaseChannel,omitempty"`
+	AmiId                       string                `yaml:"amiId,omitempty"`
+	DeprecatedVPCID             string                `yaml:"vpcId,omitempty"`
+	VPC                         model.VPC             `yaml:"vpc,omitempty"`
+	DeprecatedInternetGatewayID string                `yaml:"internetGatewayId,omitempty"`
+	InternetGateway             model.InternetGateway `yaml:"internetGateway,omitempty"`
+	RouteTableID                string                `yaml:"routeTableId,omitempty"`
 	// Required for validations like e.g. if instance cidr is contained in vpc cidr
 	VPCCIDR                string            `yaml:"vpcCIDR,omitempty"`
 	InstanceCIDR           string            `yaml:"instanceCIDR,omitempty"`
@@ -811,28 +828,35 @@ func (c Cluster) EtcdIndexEnvVarName() string {
 	return "KUBE_AWS_ETCD_INDEX"
 }
 
-func (c Config) VPCLogicalName() string {
-	return vpcLogicalName
+func (c Config) VPCLogicalName() (string, error) {
+	if c.VPC.HasIdentifier() {
+		return "", fmt.Errorf("[BUG] .VPCLogicalName should not be called in stack template when vpc id is specified")
+	}
+	return vpcLogicalName, nil
 }
 
-func (c Config) VPCRef() string {
-	if c.VPCID != "" {
-		return fmt.Sprintf("%q", c.VPCID)
-	} else {
-		return fmt.Sprintf(`{ "Ref" : %q }`, c.VPCLogicalName())
+func (c Config) VPCID() (string, error) {
+	fmt.Println("WARN: .VPCID in stack template is deprecated and will be removed in v0.9.9. Please use .VPC.ID instead")
+	if !c.VPC.HasIdentifier() {
+		return "", fmt.Errorf("[BUG] .VPCID should not be called in stack template when vpc.id(FromStackOutput) is specified. Use .VPCManaged instead.")
 	}
+	return c.VPC.ID, nil
+}
+
+func (c Config) VPCManaged() bool {
+	return !c.VPC.HasIdentifier()
+}
+
+func (c Config) VPCRef() (string, error) {
+	return c.VPC.RefOrError(c.VPCLogicalName)
 }
 
 func (c Config) InternetGatewayLogicalName() string {
 	return internetGatewayLogicalName
 }
 
 func (c Config) InternetGatewayRef() string {
-	if c.InternetGatewayID != "" {
-		return fmt.Sprintf("%q", c.InternetGatewayID)
-	} else {
-		return fmt.Sprintf(`{ "Ref" : %q }`, c.InternetGatewayLogicalName())
-	}
+	return c.InternetGateway.Ref(c.InternetGatewayLogicalName)
 }
 
 // ExternalDNSNames returns all the DNS names of Kubernetes API endpoints should be covered in the TLS cert for k8s API
@@ -1025,8 +1049,8 @@ func (c DeploymentSettings) Valid() (*DeploymentValidationResult, error) {
 		return nil, errors.New("kmsKeyArn must be set")
 	}
 
-	if c.VPCID == "" && (c.RouteTableID != "" || c.InternetGatewayID != "") {
-		return nil, errors.New("vpcId must be specified if routeTableId or internetGatewayId are specified")
+	if !c.VPC.HasIdentifier() && (c.RouteTableID != "" || c.InternetGateway.HasIdentifier()) {
+		return nil, errors.New("vpc id must be specified if route table id or internet gateway id are specified")
 	}
 
 	if c.Region.IsEmpty() {
@@ -1098,21 +1122,21 @@ func (c DeploymentSettings) Valid() (*DeploymentValidationResult, error) {
 				return nil, fmt.Errorf("either subnets[].routeTable.id(%s) or routeTableId(%s) but not both can be specified", subnet.RouteTableID(), c.RouteTableID)
 			}
 
-			if subnet.ManageSubnet() && (subnet.Public() && c.MapPublicIPs) && c.VPCID != "" && (subnet.ManageRouteTable() && c.RouteTableID == "") && c.InternetGatewayID == "" {
-				return nil, errors.New("internetGatewayId can't be omitted when there're one or more managed public subnets in an existing VPC")
+			if subnet.ManageSubnet() && (subnet.Public() && c.MapPublicIPs) && c.VPC.HasIdentifier() && (subnet.ManageRouteTable() && c.RouteTableID == "") && !c.InternetGateway.HasIdentifier() {
+				return nil, errors.New("internet gateway id can't be omitted when there're one or more managed public subnets in an existing VPC")
 			}
 		}
 
 		// All the subnets are explicitly/implicitly(they're public by default) configured to be "public".
 		// They're also configured to reuse existing route table(s).
 		// However, the IGW, which won't be applied to anywhere, is specified
-		if (allPublic && c.MapPublicIPs) && (c.RouteTableID != "" || allExistingRouteTable) && c.InternetGatewayID != "" {
-			return nil, errors.New("internetGatewayId can't be specified when all the public subnets have existing route tables associated. kube-aws doesn't try to modify an exisinting route table to include a route to the internet gateway")
+		if (allPublic && c.MapPublicIPs) && (c.RouteTableID != "" || allExistingRouteTable) && c.InternetGateway.HasIdentifier() {
+			return nil, errors.New("internet gateway id can't be specified when all the public subnets have existing route tables associated. kube-aws doesn't try to modify an exisinting route table to include a route to the internet gateway")
 		}
 
 		// All the subnets are explicitly configured to be "private" but the IGW, which won't be applied anywhere, is specified
-		if (allPrivate || !c.MapPublicIPs) && c.InternetGatewayID != "" {
-			return nil, errors.New("internetGatewayId can't be spcified when all the subnets are existing private subnets")
+		if (allPrivate || !c.MapPublicIPs) && c.InternetGateway.HasIdentifier() {
+			return nil, errors.New("internet gateway id can't be specified when all the subnets are existing private subnets")
 		}
 
 		if c.RouteTableID != "" && !allPublic && !allPrivate {

core/controlplane/config/templates/cluster.yaml

@@ -676,12 +676,26 @@ worker:
 
 ## Networking config
 
-# ID of existing VPC to create subnet in. Leave blank to create a new VPC
+# CAUTION: Deprecated and will be removed in v0.9.9. Please use vpc.id instead
 # vpcId:
 
-# ID of existing Internet Gateway to associate subnet with. Leave blank to create a new Internet Gateway
+#vpc:
+#  # ID of existing VPC to create subnet in. Leave blank to create a new VPC
+#  id:
+#  # Exported output's name from another stack
+#  # Only specify either id or idFromStackOutput but not both
+#  #idFromStackOutput: myinfra-Vpc
+
+# CAUTION: Deprecated and will be removed in v0.9.9. Please use internetGateway.id instead
 # internetGatewayId:
 
+#internetGateway:
+#  # ID of existing Internet Gateway to associate subnet with. Leave blank to create a new Internet Gateway
+#  id:
+#  # Exported output's name from another stack
+#  # Only specify either id or idFromStackOutput but not both
+#  #idFromStackOutput: myinfra-igw
+
 # Advanced: ID of existing route table in existing VPC to attach subnet to.
 # Leave blank to use the VPC's main route table.
 # This should be specified if and only if vpcId is specified.

core/controlplane/config/templates/stack-template.json

@@ -1484,7 +1484,7 @@
     {{end}}
     {{end}}
 
-    {{if not .VPCID}}
+    {{if .VPCManaged}}
     ,
     "{{.InternetGatewayLogicalName}}": {
       "Properties": {
@@ -1528,7 +1528,7 @@
   },
 
   "Outputs": {
-    {{if not .VPCID}}
+    {{if .VPCManaged}}
     "VPC" : {
       "Description" : "The VPC managed by this stack",
       "Value" :  { "Ref" : "{{.VPCLogicalName}}" },

core/nodepool/config/config.go

@@ -349,12 +349,16 @@ func (c ProvidedConfig) StackNameEnvVarName() string {
 	return "KUBE_AWS_STACK_NAME"
 }
 
-func (c ProvidedConfig) VPCRef() string {
-	//This means this VPC already exists, and we can reference it directly by ID
-	if c.VPCID != "" {
-		return fmt.Sprintf("%q", c.VPCID)
-	}
-	return `{"Fn::ImportValue" : {"Fn::Sub" : "${ControlPlaneStackName}-VPC"}}`
+func (c ProvidedConfig) VPCRef() (string, error) {
+	igw := c.InternetGateway
+	// When HasIdentifier returns true, it means the VPC already exists, and we can reference it directly by ID
+	if !c.VPC.HasIdentifier() {
+		// Otherwise import the VPC ID from the control-plane stack
+		igw.IDFromStackOutput = `{"Fn::Sub" : "${ControlPlaneStackName}-VPC"}`
+	}
+	return igw.RefOrError(func() (string, error) {
+		return "", fmt.Errorf("[BUG] Tried to reference VPC by its logical name")
+	})
 }
 
 func (c ProvidedConfig) SecurityGroupRefs() []string {

core/nodepool/config/deployment.go

@@ -10,18 +10,18 @@ func (c DeploymentSettings) ValidateInputs() error {
 	// By design, kube-aws doesn't allow customizing the following settings among node pools.
 	//
 	// Every node pool imports subnets from the main stack and therefore there's no need for setting:
-	// * VPCID
-	// * InternetGatewayID
+	// * VPC.ID(FromStackOutput)
+	// * InternetGateway.ID(FromStackOutput)
 	// * RouteTableID
 	// * VPCCIDR
 	// * InstanceCIDR
 	// * MapPublicIPs
 	// * ElasticFileSystemID
-	if c.VPCID != "" {
-		return fmt.Errorf("although you can't customize `vpcId` per node pool but you did specify \"%s\" in your cluster.yaml", c.VPCID)
+	if c.VPC.HasIdentifier() {
+		return fmt.Errorf("although you can't customize VPC per node pool but you did specify \"%v\" in your cluster.yaml", c.VPC)
 	}
-	if c.InternetGatewayID != "" {
-		return fmt.Errorf("although you can't customize `internetGatewayId` per node pool but you did specify \"%s\" in your cluster.yaml", c.InternetGatewayID)
+	if c.InternetGateway.HasIdentifier() {
+		return fmt.Errorf("although you can't customize internet gateway per node pool but you did specify \"%v\" in your cluster.yaml", c.InternetGateway)
 	}
 	if c.RouteTableID != "" {
 		return fmt.Errorf("although you can't customize `routeTableId` per node pool but you did specify \"%s\" in your cluster.yaml", c.RouteTableID)

e2e/run

@@ -149,8 +149,8 @@ customize_cluster_yaml() {
   echo Writing to $(pwd)/cluster.yaml
 
   if [ "${KUBE_AWS_DEPLOY_TO_EXISTING_VPC}" != "" ]; then
-    echo "vpcId: $(testinfra_vpc)" >> cluster.yaml
-    echo "routeTableId: $(testinfra_public_routetable)" >> cluster.yaml
+    echo -e "vpc:\n  id: $(testinfra_vpc)" >> cluster.yaml
+    echo -e "routeTableId: $(testinfra_public_routetable)" >> cluster.yaml
     echo -e "workerSecurityGroupIds:\n- $(testinfra_glue_sg)" >> cluster.yaml
   fi
 

model/vpc.go

@@ -0,0 +1,12 @@
+package model
+
+// kube-aws manages at most one VPC per cluster
+// If ID or IDFromStackOutput is non-zero, kube-aws doesn't manage the VPC but its users' responsibility to
+// provide properly configured one to be reused by kube-aws.
+// More concretely:
+// * If an user is going to reuse an existing VPC, it must have an internet gateway attached and
+// * A valid internet gateway ID must be provided via `internetGateway.id` or `internetGateway.idFromStackOutput`.
+//   In other words, kube-aws doesn't create an internet gateway in an existing VPC.
+type VPC struct {
+	Identifier `yaml:",inline"`
+}