Remove mapPublicIPs and routeTableId in the top-level of cluster.…

…yaml

Resolves kubernetes-retired#929

core/controlplane/cluster/cluster_test.go

@@ -176,8 +176,11 @@ func TestExistingVPCValidation(t *testing.T) {
 		`
 vpcCIDR: 10.5.0.0/16
 vpcId: vpc-xxx1
-routeTableId: rtb-xxxxxx
-instanceCIDR: 10.5.11.0/24
+subnets:
+- name: Subnet0
+  routeTable:
+    id: rtb-xxxxxx
+  instanceCIDR: 10.5.11.0/24
 `, `
 vpcCIDR: 192.168.1.0/24
 vpcId: vpc-xxx2
@@ -198,30 +201,37 @@ subnets:
 		`
 vpcCIDR: 10.0.0.0/16
 vpcId: vpc-xxx3 #vpc does not exist
-instanceCIDR: 10.0.0.0/24
-routeTableId: rtb-xxxxxx
+subnets:
+- name: Subnet0
+  routeTable:
+    id: rtb-xxxxxx
+  instanceCIDR: 10.0.0.0/24
 `, `
 vpcCIDR: 10.10.0.0/16 #vpc cidr does match existing vpc-xxx1
 vpcId: vpc-xxx1
-instanceCIDR: 10.10.0.0/24
-routeTableId: rtb-xxxxxx
+subnets:
+- name: Subnet0
+  routeTable:
+    id: rtb-xxxxxx
+  instanceCIDR: 10.10.0.0/24
 `, `
 vpcCIDR: 10.5.0.0/16
 instanceCIDR: 10.5.2.0/28 #instance cidr conflicts with existing subnet
 vpcId: vpc-xxx1
-routeTableId: rtb-xxxxxx
 `, `
 vpcCIDR: 192.168.1.0/24
 instanceCIDR: 192.168.1.100/26 #instance cidr conflicts with existing subnet
 vpcId: vpc-xxx2
-routeTableId: rtb-xxxxxx
 `, `
 vpcCIDR: 192.168.1.0/24
 vpcId: vpc-xxx2
-routeTableId: rtb-xxxxxx
 subnets:
   - instanceCIDR: 192.168.1.100/26  #instance cidr conflicts with existing subnet
+    routeTable:
+      id: rtb-xxxxxx
   - instanceCIDR: 192.168.1.0/26
+    routeTable:
+      id: rtb-xxxxxx
 `,
 	}
 

core/controlplane/config/config.go

@@ -109,7 +109,6 @@ func NewDefaultCluster() *Cluster {
 			ContainerRuntime:   "docker",
 			Subnets:            []model.Subnet{},
 			EIPAllocationIDs:   []string{},
-			MapPublicIPs:       true,
 			Experimental:       experimental,
 			ManageCertificates: true,
 			AmazonSsmAgent: AmazonSsmAgent{
@@ -246,7 +245,9 @@ func (c *Cluster) Load() error {
 		return fmt.Errorf("invalid cluster: %v", err)
 	}
 
-	c.SetDefaults()
+	if err := c.SetDefaults(); err != nil {
+		return fmt.Errorf("invalid cluster: %v", err)
+	}
 
 	if c.ExternalDNSName != "" {
 		// TODO: Deprecate externalDNSName?
@@ -286,47 +287,18 @@ func (c *Cluster) ConsumeDeprecatedKeys() {
 	}
 }
 
-func (c *Cluster) SetDefaults() {
+func (c *Cluster) SetDefaults() error {
 	// For backward-compatibility
 	if len(c.Subnets) == 0 {
 		c.Subnets = []model.Subnet{
 			model.NewPublicSubnet(c.AvailabilityZone, c.InstanceCIDR),
 		}
 	}
 
-	privateTopologyImplied := c.RouteTableID != "" && !c.MapPublicIPs
-	publicTopologyImplied := c.RouteTableID != "" && c.MapPublicIPs
-
 	for i, s := range c.Subnets {
 		if s.Name == "" {
 			c.Subnets[i].Name = fmt.Sprintf("Subnet%d", i)
 		}
-
-		// DEPRECATED AND REMOVED IN THE FUTURE
-		// See https://github.com/kubernetes-incubator/kube-aws/pull/284#issuecomment-275998862
-		//
-		// This implies a deployment to an existing VPC with a route table with a preconfigured Internet Gateway
-		// and all the subnets created by kube-aws are public
-		if publicTopologyImplied {
-			c.Subnets[i].RouteTable.ID = c.RouteTableID
-			if s.Private {
-				panic(fmt.Sprintf("mapPublicIPs(=%v) and subnets[%d].private(=%v) conflicts: %+v", c.MapPublicIPs, i, s.Private, s))
-			}
-			c.Subnets[i].Private = false
-		}
-
-		// DEPRECATED AND REMOVED IN THE FUTURE
-		// See https://github.com/kubernetes-incubator/kube-aws/pull/284#issuecomment-275998862
-		//
-		// This implies a deployment to an existing VPC with a route table with a preconfigured NAT Gateway
-		// and all the subnets created by kube-aws are private
-		if privateTopologyImplied {
-			c.Subnets[i].RouteTable.ID = c.RouteTableID
-			if s.Private {
-				panic(fmt.Sprintf("mapPublicIPs(=%v) and subnets[%d].private(=%v) conflicts. You don't need to set true to both of them. If you want to make all the subnets private, make mapPublicIPs false. If you want to make only part of subnets private, make subnets[].private true accordingly: %+v", c.MapPublicIPs, i, s.Private, s))
-			}
-			c.Subnets[i].Private = true
-		}
 	}
 
 	for i, s := range c.Controller.Subnets {
@@ -345,15 +317,17 @@ func (c *Cluster) SetDefaults() {
 	}
 
 	if len(c.Controller.Subnets) == 0 {
-		if privateTopologyImplied {
-			c.Controller.Subnets = c.PrivateSubnets()
-		} else {
-			c.Controller.Subnets = c.PublicSubnets()
+		c.Controller.Subnets = c.PublicSubnets()
+
+		if len(c.Controller.Subnets) == 0 {
+			return errors.New("`controller.subnets` in cluster.yaml defaults to include only public subnets defined under `subnets`. However, there was no public subnet for that. Please define one or more public subnets under `subnets` or set `controller.subnets`.")
 		}
+	} else if c.Controller.Subnets.ContainsBothPrivateAndPublic() {
+		return errors.New("You can not mix private and public subnets for controller nodes. Please explicitly configure controller.subnets[] to contain either public or private subnets only")
 	}
 
 	if len(c.Controller.LoadBalancer.Subnets) == 0 {
-		if c.Controller.LoadBalancer.Private || privateTopologyImplied {
+		if c.Controller.LoadBalancer.Private {
 			c.Controller.LoadBalancer.Subnets = c.PrivateSubnets()
 			c.Controller.LoadBalancer.Private = true
 		} else {
@@ -362,12 +336,16 @@ func (c *Cluster) SetDefaults() {
 	}
 
 	if len(c.Etcd.Subnets) == 0 {
-		if privateTopologyImplied {
-			c.Etcd.Subnets = c.PrivateSubnets()
-		} else {
-			c.Etcd.Subnets = c.PublicSubnets()
+		c.Etcd.Subnets = c.PublicSubnets()
+
+		if len(c.Etcd.Subnets) == 0 {
+			return errors.New("`etcd.subnets` in cluster.yaml defaults to include only public subnets defined under `subnets`. However, there was no public subnet for that. Please define one or more public subnets under `subnets` or set `etcd.subnets`.")
 		}
+	} else if c.Etcd.Subnets.ContainsBothPrivateAndPublic() {
+		return fmt.Errorf("You can not mix private and public subnets for etcd nodes. Please explicitly configure etcd.subnets[] to contain either public or private subnets only")
 	}
+
+	return nil
 }
 
 func ClusterFromBytesWithEncryptService(data []byte, encryptService EncryptService) (*Cluster, error) {
@@ -417,17 +395,15 @@ type DeploymentSettings struct {
 	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"`
 	K8sVer                  string            `yaml:"kubernetesVersion,omitempty"`
 	ContainerRuntime        string            `yaml:"containerRuntime,omitempty"`
 	KMSKeyARN               string            `yaml:"kmsKeyArn,omitempty"`
 	StackTags               map[string]string `yaml:"stackTags,omitempty"`
-	Subnets                 []model.Subnet    `yaml:"subnets,omitempty"`
+	Subnets                 model.Subnets     `yaml:"subnets,omitempty"`
 	EIPAllocationIDs        []string          `yaml:"eipAllocationIDs,omitempty"`
-	MapPublicIPs            bool              `yaml:"mapPublicIPs,omitempty"`
 	ElasticFileSystemID     string            `yaml:"elasticFileSystemId,omitempty"`
 	SharedPersistentVolume  bool              `yaml:"sharedPersistentVolume,omitempty"`
 	SSHAuthorizedKeys       []string          `yaml:"sshAuthorizedKeys,omitempty"`
@@ -1122,10 +1098,6 @@ func (c DeploymentSettings) Validate() (*DeploymentValidationResult, error) {
 		return nil, errors.New("kmsKeyArn must be set")
 	}
 
-	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() {
 		return nil, errors.New("region must be set")
 	}
@@ -1191,31 +1163,27 @@ func (c DeploymentSettings) Validate() (*DeploymentValidationResult, error) {
 				)
 			}
 
-			if subnet.RouteTableID() != "" && c.RouteTableID != "" {
-				return nil, fmt.Errorf("either subnets[].routeTable.id(%s) or routeTableId(%s) but not both can be specified", subnet.RouteTableID(), c.RouteTableID)
+			if !c.VPC.HasIdentifier() && (subnet.RouteTable.HasIdentifier() || c.InternetGateway.HasIdentifier()) {
+				return nil, errors.New("vpcId must be specified if subnets[].routeTable.id or internetGateway.id are specified")
 			}
 
-			if subnet.ManageSubnet() && (subnet.Public() && c.MapPublicIPs) && c.VPC.HasIdentifier() && (subnet.ManageRouteTable() && c.RouteTableID == "") && !c.InternetGateway.HasIdentifier() {
+			if subnet.ManageSubnet() && subnet.Public() && c.VPC.HasIdentifier() && subnet.ManageRouteTable() && !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.InternetGateway.HasIdentifier() {
+		if allPublic && 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.InternetGateway.HasIdentifier() {
+		if allPrivate && 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 {
-			return nil, fmt.Errorf("network topology including both private and public subnets specified while the single route table(%s) is also specified. You must differentiate the route table at least between private and public subnets. Use subets[].routeTable.id instead of routeTableId for that.", c.RouteTableID)
-		}
-
 		for i, a := range instanceCIDRs {
 			for j := i + 1; j < len(instanceCIDRs); j++ {
 				b := instanceCIDRs[j]
@@ -1243,7 +1211,7 @@ func (c DeploymentSettings) AssetsEncryptionEnabled() bool {
 	return c.ManageCertificates && c.Region.SupportsKMS()
 }
 
-func (s DeploymentSettings) AllSubnets() []model.Subnet {
+func (s DeploymentSettings) AllSubnets() model.Subnets {
 	subnets := s.Subnets
 	return subnets
 }
@@ -1261,7 +1229,7 @@ func (c DeploymentSettings) FindSubnetMatching(condition model.Subnet) model.Sub
 	panic(fmt.Errorf("No subnet matching %v found in %s", condition, out))
 }
 
-func (c DeploymentSettings) PrivateSubnets() []model.Subnet {
+func (c DeploymentSettings) PrivateSubnets() model.Subnets {
 	result := []model.Subnet{}
 	for _, s := range c.Subnets {
 		if s.Private {
@@ -1271,7 +1239,7 @@ func (c DeploymentSettings) PrivateSubnets() []model.Subnet {
 	return result
 }
 
-func (c DeploymentSettings) PublicSubnets() []model.Subnet {
+func (c DeploymentSettings) PublicSubnets() model.Subnets {
 	result := []model.Subnet{}
 	for _, s := range c.Subnets {
 		if !s.Private {

core/controlplane/config/config_test.go

@@ -102,7 +102,13 @@ podCIDR: 172.4.0.0/16
 serviceCIDR: 172.5.0.0/16
 dnsServiceIP: 172.6.100.101 #dnsServiceIP not in service CIDR
 `, `
-routeTableId: rtb-xxxxxx # routeTableId specified without vpcId
+
+subnets:
+- name: Subnet0
+  instanceCIDR: "10.0.0.0/24"
+  availabilityZone: us-west-1c
+  routeTable:
+    id: rtb-xxxxxx # routeTable.id specified without vpcId
 `, `
 # invalid TTL
 recordSetTTL: 0
@@ -324,7 +330,7 @@ func TestMultipleSubnets(t *testing.T) {
 
 	validConfigs := []struct {
 		conf    string
-		subnets []model.Subnet
+		subnets model.Subnets
 	}{
 		{
 			conf: `
@@ -336,7 +342,7 @@ subnets:
   - availabilityZone: ap-northeast-1c
     instanceCIDR: 10.4.4.0/24
 `,
-			subnets: []model.Subnet{
+			subnets: model.Subnets{
 				{
 					InstanceCIDR:     "10.4.3.0/24",
 					AvailabilityZone: "ap-northeast-1a",
@@ -356,7 +362,7 @@ vpcCIDR: 10.4.3.0/16
 availabilityZone: ap-northeast-1a
 instanceCIDR: 10.4.3.0/24
 `,
-			subnets: []model.Subnet{
+			subnets: model.Subnets{
 				{
 					AvailabilityZone: "ap-northeast-1a",
 					InstanceCIDR:     "10.4.3.0/24",
@@ -372,7 +378,7 @@ availabilityZone: ap-northeast-1a
 instanceCIDR: 10.4.3.0/24
 subnets: []
 `,
-			subnets: []model.Subnet{
+			subnets: model.Subnets{
 				{
 					AvailabilityZone: "ap-northeast-1a",
 					InstanceCIDR:     "10.4.3.0/24",
@@ -386,7 +392,7 @@ subnets: []
 availabilityZone: "ap-northeast-1a"
 subnets: []
 `,
-			subnets: []model.Subnet{
+			subnets: model.Subnets{
 				{
 					AvailabilityZone: "ap-northeast-1a",
 					InstanceCIDR:     "10.0.0.0/24",
@@ -399,7 +405,7 @@ subnets: []
 # Missing subnets field fall-backs to the single subnet with the default az/cidr.
 availabilityZone: "ap-northeast-1a"
 `,
-			subnets: []model.Subnet{
+			subnets: model.Subnets{
 				{
 					AvailabilityZone: "ap-northeast-1a",
 					InstanceCIDR:     "10.0.0.0/24",
@@ -939,7 +945,7 @@ func TestValidateExistingVPC(t *testing.T) {
 	cluster := NewDefaultCluster()
 
 	cluster.VPCCIDR = "10.0.0.0/16"
-	cluster.Subnets = []model.Subnet{
+	cluster.Subnets = model.Subnets{
 		model.NewPublicSubnet("ap-northeast-1a", "10.0.1.0/24"),
 		model.NewPublicSubnet("ap-northeast-1a", "10.0.2.0/24"),
 	}

core/nodepool/config/deployment.go

@@ -23,18 +23,12 @@ func (c DeploymentSettings) ValidateInputs() error {
 	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)
-	}
 	if c.VPCCIDR != "" {
 		return fmt.Errorf("although you can't customize `vpcCIDR` per node pool but you did specify \"%s\" in your cluster.yaml", c.VPCCIDR)
 	}
 	if c.InstanceCIDR != "" {
 		return fmt.Errorf("although you can't customize `instanceCIDR` per node pool but you did specify \"%s\" in your cluster.yaml", c.InstanceCIDR)
 	}
-	if c.MapPublicIPs {
-		return fmt.Errorf("although you can't customize `mapPublicIPs` per node pool but you did specify %v in your cluster.yaml", c.MapPublicIPs)
-	}
 	if c.ElasticFileSystemID != "" {
 		return fmt.Errorf("although you can't customize `elasticFileSystemId` per node pool but you did specify \"%s\" in your cluster.yaml", c.ElasticFileSystemID)
 	}

model/controller.go

@@ -13,7 +13,7 @@ type Controller struct {
 	LoadBalancer       ControllerElb       `yaml:"loadBalancer,omitempty"`
 	IAMConfig          IAMConfig           `yaml:"iam,omitempty"`
 	SecurityGroupIds   []string            `yaml:"securityGroupIds"`
-	Subnets            []Subnet            `yaml:"subnets,omitempty"`
+	Subnets            Subnets             `yaml:"subnets,omitempty"`
 	CustomFiles        []CustomFile        `yaml:"customFiles,omitempty"`
 	CustomSystemdUnits []CustomSystemdUnit `yaml:"customSystemdUnits,omitempty"`
 	NodeSettings       `yaml:",inline"`
@@ -79,5 +79,5 @@ func (c Controller) Validate() error {
 
 type ControllerElb struct {
 	Private bool
-	Subnets []Subnet
+	Subnets Subnets
 }

model/derived/api_endpoint_lb.go

@@ -13,7 +13,7 @@ type APIEndpointLB struct {
 	// APIEndpoint is inherited to configure this load balancer
 	model.APIEndpoint
 	// Subnets contains all the subnets assigned to this load-balancer. Specified only when this load balancer is not reused but managed one
-	Subnets []model.Subnet
+	Subnets model.Subnets
 }
 
 // DNSNameRef returns a CloudFormation ref for the Amazon-provided DNS name of this load balancer, which is typically used

model/etcd.go

@@ -17,7 +17,7 @@ type Etcd struct {
 	Nodes              []EtcdNode   `yaml:"nodes,omitempty"`
 	SecurityGroupIds   []string     `yaml:"securityGroupIds"`
 	Snapshot           EtcdSnapshot `yaml:"snapshot,omitempty"`
-	Subnets            []Subnet     `yaml:"subnets,omitempty"`
+	Subnets            Subnets      `yaml:"subnets,omitempty"`
 	UnknownKeys        `yaml:",inline"`
 }