From 0cd07b2224399a920a27bfe88bf440bd0eecea24 Mon Sep 17 00:00:00 2001 From: staebler Date: Wed, 23 Jan 2019 14:30:12 -0500 Subject: [PATCH 1/2] data/aws: use azs for master set in manifests These changes pass the availability zones to use for the masters set in 99_openshift-cluster-api_master-machines.yaml through to terraform. Prior to these changes the masters were always placed in the first 3 availability zones. There is no validation done on the availability zones to verify that they are valid for the region. Fix for https://bugzilla.redhat.com/show_bug.cgi?id=1662119. --- data/data/aws/main.tf | 3 ++- data/data/aws/master/main.tf | 2 +- data/data/aws/master/outputs.tf | 4 ---- data/data/aws/master/variables.tf | 14 ++++++++++---- data/data/aws/variables-aws.tf | 5 +++++ data/data/aws/vpc/common.tf | 9 +++++---- data/data/aws/vpc/outputs.tf | 4 ++++ pkg/asset/cluster/tfvars.go | 8 +++++--- pkg/tfvars/aws/aws.go | 17 +++++++++++++---- 9 files changed, 45 insertions(+), 21 deletions(-) diff --git a/data/data/aws/main.tf b/data/data/aws/main.tf index b204687bfd9..36bf53bc9d0 100644 --- a/data/data/aws/main.tf +++ b/data/data/aws/main.tf @@ -32,12 +32,13 @@ module "masters" { tags = "${local.tags}" + availability_zones = "${var.aws_master_availability_zones}" + az_to_subnet_id = "${module.vpc.az_to_private_subnet_id}" instance_count = "${var.master_count}" master_sg_ids = "${list(module.vpc.master_sg_id)}" root_volume_iops = "${var.aws_master_root_volume_iops}" root_volume_size = "${var.aws_master_root_volume_size}" root_volume_type = "${var.aws_master_root_volume_type}" - subnet_ids = "${module.vpc.private_subnet_ids}" target_group_arns = "${module.vpc.aws_lb_target_group_arns}" target_group_arns_length = "${module.vpc.aws_lb_target_group_arns_length}" ec2_ami = "${aws_ami_copy.main.id}" diff --git a/data/data/aws/master/main.tf b/data/data/aws/master/main.tf index 175a2c1510f..1c1a46063ee 100644 --- a/data/data/aws/master/main.tf +++ b/data/data/aws/master/main.tf @@ -74,7 +74,7 @@ resource "aws_instance" "master" { iam_instance_profile = "${aws_iam_instance_profile.master.name}" instance_type = "${var.instance_type}" - subnet_id = "${element(var.subnet_ids, count.index)}" + subnet_id = "${var.az_to_subnet_id[var.availability_zones[count.index]]}" user_data = "${var.user_data_ign}" vpc_security_group_ids = ["${var.master_sg_ids}"] diff --git a/data/data/aws/master/outputs.tf b/data/data/aws/master/outputs.tf index 6fdc625ddba..305dd696ccf 100644 --- a/data/data/aws/master/outputs.tf +++ b/data/data/aws/master/outputs.tf @@ -1,7 +1,3 @@ -output "subnet_ids" { - value = "${var.subnet_ids}" -} - output "cluster_id" { value = "${var.cluster_id}" } diff --git a/data/data/aws/master/variables.tf b/data/data/aws/master/variables.tf index 34302ba014c..a03e0cc6042 100644 --- a/data/data/aws/master/variables.tf +++ b/data/data/aws/master/variables.tf @@ -1,3 +1,13 @@ +variable "availability_zones" { + type = "list" + description = "List of the availability zones in which to create the masters. The length of this list must match instance_count." +} + +variable "az_to_subnet_id" { + type = "map" + description = "Map from availability zone name to the ID of the subnet in that availability zone" +} + variable "cluster_id" { type = "string" } @@ -40,10 +50,6 @@ variable "root_volume_type" { description = "The type of volume for the root block device." } -variable "subnet_ids" { - type = "list" -} - variable "tags" { type = "map" default = {} diff --git a/data/data/aws/variables-aws.tf b/data/data/aws/variables-aws.tf index 3f0f5902763..41f04cb7770 100644 --- a/data/data/aws/variables-aws.tf +++ b/data/data/aws/variables-aws.tf @@ -57,3 +57,8 @@ variable "aws_region" { type = "string" description = "The target AWS region for the cluster." } + +variable "aws_master_availability_zones" { + type = "list" + description = "The availability zones in which to create the masters. The length of this list must match master_count." +} diff --git a/data/data/aws/vpc/common.tf b/data/data/aws/vpc/common.tf index f1585f2a1a7..dc3114cc55a 100644 --- a/data/data/aws/vpc/common.tf +++ b/data/data/aws/vpc/common.tf @@ -17,10 +17,11 @@ locals { vpc_id = "${aws_vpc.new_vpc.id}" // When referencing the _ids arrays or data source arrays via count = , always use the *_count variable rather than taking the length of the list - private_subnet_ids = "${aws_subnet.private_subnet.*.id}" - public_subnet_ids = "${aws_subnet.public_subnet.*.id}" - private_subnet_count = "${local.new_az_count}" - public_subnet_count = "${local.new_az_count}" + private_subnet_ids = "${aws_subnet.private_subnet.*.id}" + public_subnet_ids = "${aws_subnet.public_subnet.*.id}" + private_subnet_count = "${local.new_az_count}" + public_subnet_count = "${local.new_az_count}" + az_to_private_subnet_id = "${zipmap(local.new_subnet_azs, local.private_subnet_ids)}" } # all data sources should be input variable-agnostic and used as canonical source for querying "state of resources" and building outputs diff --git a/data/data/aws/vpc/outputs.tf b/data/data/aws/vpc/outputs.tf index b175fca82da..5308e8f91f7 100644 --- a/data/data/aws/vpc/outputs.tf +++ b/data/data/aws/vpc/outputs.tf @@ -2,6 +2,10 @@ output "vpc_id" { value = "${data.aws_vpc.cluster_vpc.id}" } +output "az_to_private_subnet_id" { + value = "${local.az_to_private_subnet_id}" +} + output "public_subnet_ids" { value = "${local.public_subnet_ids}" } diff --git a/pkg/asset/cluster/tfvars.go b/pkg/asset/cluster/tfvars.go index 01faf2e02db..cec3b7f6198 100644 --- a/pkg/asset/cluster/tfvars.go +++ b/pkg/asset/cluster/tfvars.go @@ -107,9 +107,11 @@ func (t *TerraformVariables) Generate(parents asset.Parents) error { if err != nil { return err } - data, err = awstfvars.TFVars( - masters[0].Spec.ProviderSpec.Value.Object.(*awsprovider.AWSMachineProviderConfig), - ) + masterConfigs := make([]*awsprovider.AWSMachineProviderConfig, len(masters)) + for i, m := range masters { + masterConfigs[i] = m.Spec.ProviderSpec.Value.Object.(*awsprovider.AWSMachineProviderConfig) + } + data, err := awstfvars.TFVars(masterConfigs) if err != nil { return errors.Wrapf(err, "failed to get %s Terraform variables", platform) } diff --git a/pkg/tfvars/aws/aws.go b/pkg/tfvars/aws/aws.go index a30fb0301e6..456eba934e1 100644 --- a/pkg/tfvars/aws/aws.go +++ b/pkg/tfvars/aws/aws.go @@ -15,6 +15,7 @@ type config struct { ExtraTags map[string]string `json:"aws_extra_tags,omitempty"` BootstrapInstanceType string `json:"aws_bootstrap_instance_type,omitempty"` MasterInstanceType string `json:"aws_master_instance_type,omitempty"` + AvailabilityZones []string `json:"aws_master_availability_zones"` IOPS int64 `json:"aws_master_root_volume_iops"` Size int64 `json:"aws_master_root_volume_size,omitempty"` Type string `json:"aws_master_root_volume_type,omitempty"` @@ -22,12 +23,19 @@ type config struct { } // TFVars generates AWS-specific Terraform variables launching the cluster. -func TFVars(masterConfig *v1beta1.AWSMachineProviderConfig) ([]byte, error) { +func TFVars(masterConfigs []*v1beta1.AWSMachineProviderConfig) ([]byte, error) { + masterConfig := masterConfigs[0] + tags := make(map[string]string, len(masterConfig.Tags)) for _, tag := range masterConfig.Tags { tags[tag.Name] = tag.Value } + availabilityZones := make([]string, len(masterConfigs)) + for i, c := range masterConfigs { + availabilityZones[i] = c.Placement.AvailabilityZone + } + if len(masterConfig.BlockDevices) == 0 { return nil, errors.New("block device slice cannot be empty") } @@ -52,9 +60,10 @@ func TFVars(masterConfig *v1beta1.AWSMachineProviderConfig) ([]byte, error) { instanceClass := defaults.InstanceClass(masterConfig.Placement.Region) cfg := &config{ - Region: masterConfig.Placement.Region, - ExtraTags: tags, - AMI: *masterConfig.AMI.ID, + Region: masterConfig.Placement.Region, + ExtraTags: tags, + AMI: *masterConfig.AMI.ID, + AvailabilityZones: availabilityZones, BootstrapInstanceType: fmt.Sprintf("%s.large", instanceClass), MasterInstanceType: masterConfig.InstanceType, Size: *rootVolume.EBS.VolumeSize, From afa0b599b76163b08ea393bc36e94e6248042a8e Mon Sep 17 00:00:00 2001 From: staebler Date: Mon, 25 Feb 2019 15:36:56 -0500 Subject: [PATCH 2/2] data/aws: place bootstrap node in same az as first master The bootstrap node was being placed in the first availability zone in the region. Now, place the bootstrap node in the same availability zone as the first master. Remove the local az_to_private_subnet_id variable from the vpc module as it is only used as an output from the module. The output value is now calculated at the place where the output value is defined. Remove the cluster_id output value from the vpc module as it is unused. --- data/data/aws/main.tf | 2 +- data/data/aws/master/outputs.tf | 4 ---- data/data/aws/vpc/common.tf | 9 ++++----- data/data/aws/vpc/outputs.tf | 2 +- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/data/data/aws/main.tf b/data/data/aws/main.tf index 36bf53bc9d0..a8d84d98818 100644 --- a/data/data/aws/main.tf +++ b/data/data/aws/main.tf @@ -15,7 +15,7 @@ module "bootstrap" { instance_type = "${var.aws_bootstrap_instance_type}" cluster_id = "${var.cluster_id}" ignition = "${var.ignition_bootstrap}" - subnet_id = "${module.vpc.public_subnet_ids[0]}" + subnet_id = "${module.vpc.az_to_private_subnet_id[var.aws_master_availability_zones[0]]}" target_group_arns = "${module.vpc.aws_lb_target_group_arns}" target_group_arns_length = "${module.vpc.aws_lb_target_group_arns_length}" vpc_id = "${module.vpc.vpc_id}" diff --git a/data/data/aws/master/outputs.tf b/data/data/aws/master/outputs.tf index 305dd696ccf..30844fb4306 100644 --- a/data/data/aws/master/outputs.tf +++ b/data/data/aws/master/outputs.tf @@ -1,7 +1,3 @@ -output "cluster_id" { - value = "${var.cluster_id}" -} - output "ip_addresses" { value = "${aws_instance.master.*.private_ip}" } diff --git a/data/data/aws/vpc/common.tf b/data/data/aws/vpc/common.tf index dc3114cc55a..f1585f2a1a7 100644 --- a/data/data/aws/vpc/common.tf +++ b/data/data/aws/vpc/common.tf @@ -17,11 +17,10 @@ locals { vpc_id = "${aws_vpc.new_vpc.id}" // When referencing the _ids arrays or data source arrays via count = , always use the *_count variable rather than taking the length of the list - private_subnet_ids = "${aws_subnet.private_subnet.*.id}" - public_subnet_ids = "${aws_subnet.public_subnet.*.id}" - private_subnet_count = "${local.new_az_count}" - public_subnet_count = "${local.new_az_count}" - az_to_private_subnet_id = "${zipmap(local.new_subnet_azs, local.private_subnet_ids)}" + private_subnet_ids = "${aws_subnet.private_subnet.*.id}" + public_subnet_ids = "${aws_subnet.public_subnet.*.id}" + private_subnet_count = "${local.new_az_count}" + public_subnet_count = "${local.new_az_count}" } # all data sources should be input variable-agnostic and used as canonical source for querying "state of resources" and building outputs diff --git a/data/data/aws/vpc/outputs.tf b/data/data/aws/vpc/outputs.tf index 5308e8f91f7..d41c15f5df3 100644 --- a/data/data/aws/vpc/outputs.tf +++ b/data/data/aws/vpc/outputs.tf @@ -3,7 +3,7 @@ output "vpc_id" { } output "az_to_private_subnet_id" { - value = "${local.az_to_private_subnet_id}" + value = "${zipmap(local.new_subnet_azs, local.private_subnet_ids)}" } output "public_subnet_ids" {