Skip to content

Commit

Permalink
data/aws: Consolidated tag handling
Browse files Browse the repository at this point in the history
Centralize extra-tag inclusion on aws/main.tf.  This reduces the
number of places we need to think about what tags should be ;).

Also keep kubernetes.io/cluster/{name} localized in the aws module.
See 8a37f72 (modules/aws/bootstrap: Pull AWS bootstrap setup into a
module, 2018-09-05, openshift#217) for why we need to keep it on the bootstrap
instance.  But the bootstrap resources will be removed after the
bootstrap-complete event comes through, and we don't want Kubernetes
controllers trying to pick them up.

This commit updates the internal Route 53 zone from KubernetesCluster
to kubernetes.io/cluster/{name}: owned, catching it up to
kubernetes/kubernetes@0b5ae539 (AWS: Support shared tag, 2017-02-18,
kubernetes/kubernetes#41695).  That tag originally landed on the zone
back in 75fb49a (platforms/aws: apply tags to internal route53 zone,
2017-05-02, coreos/tectonic-installer#465).

Only the master instances need the clusterid tag, as described in
6c7a5f0 (Tag master machines for adoption by machine controller,
2018-10-17, openshift#479).

A number of VPC resources have moved from "shared" to "owned".  The
shared values are from 45dfc2b (modules/aws,azure: use the new tag
format for k8s 1.6, 2017-05-04, coreos/tectonic-installer#469).  The
commit message doesn't have much to say for motivation, but Brad Ison
said [1]:

  I'm not really sure if anything in Kubernetes actually uses the
  owned vs. shared values at the moment, but in any case, it might
  make more sense to mark subnets as shared.  That was actually one of
  the main use cases for moving to this style of tagging -- being able
  to share subnets between clusters.

But we aren't sharing these resources; see 6f55e67 (terraform/aws:
remove option to use an existing vpc in aws, 2018-11-11, openshift#654).

[1]: coreos/tectonic-installer#469 (comment)
  • Loading branch information
wking committed Jan 11, 2019
1 parent 4617dec commit 476e1f3
Show file tree
Hide file tree
Showing 12 changed files with 54 additions and 92 deletions.
30 changes: 19 additions & 11 deletions data/data/aws/main.tf
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
locals {
private_zone_id = "${aws_route53_zone.int.zone_id}"

tags = "${merge(map(
"openshiftClusterID", "${var.cluster_id}"
), var.aws_extra_tags)}"
}

provider "aws" {
Expand All @@ -20,18 +24,21 @@ module "bootstrap" {

tags = "${merge(map(
"Name", "${var.cluster_name}-bootstrap",
"openshiftClusterID", "${var.cluster_id}"
), var.aws_extra_tags)}"
), local.tags)}"
}

module "masters" {
source = "./master"

base_domain = "${var.base_domain}"
cluster_id = "${var.cluster_id}"
cluster_name = "${var.cluster_name}"
ec2_type = "${var.aws_master_ec2_type}"
extra_tags = "${var.aws_extra_tags}"
base_domain = "${var.base_domain}"
cluster_id = "${var.cluster_id}"
cluster_name = "${var.cluster_name}"
ec2_type = "${var.aws_master_ec2_type}"

tags = "${merge(map(
"kubernetes.io/cluster/${var.cluster_name}", "owned",
), local.tags)}"

instance_count = "${var.master_count}"
master_iam_role = "${var.aws_master_iam_role_name}"
master_sg_ids = "${list(module.vpc.master_sg_id)}"
Expand Down Expand Up @@ -74,7 +81,9 @@ module "vpc" {
cluster_name = "${var.cluster_name}"
region = "${var.aws_region}"

extra_tags = "${var.aws_extra_tags}"
tags = "${merge(map(
"kubernetes.io/cluster/${var.cluster_name}", "owned",
), local.tags)}"
}

resource "aws_route53_record" "etcd_a_nodes" {
Expand Down Expand Up @@ -104,7 +113,6 @@ resource "aws_route53_zone" "int" {

tags = "${merge(map(
"Name", "${var.cluster_name}_int",
"KubernetesCluster", "${var.cluster_name}",
"openshiftClusterID", "${var.cluster_id}"
), var.aws_extra_tags)}"
"kubernetes.io/cluster/${var.cluster_name}", "owned",
), local.tags)}"
}
8 changes: 2 additions & 6 deletions data/data/aws/master/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,8 @@ resource "aws_instance" "master" {

tags = "${merge(map(
"Name", "${var.cluster_name}-master-${count.index}",
"kubernetes.io/cluster/${var.cluster_name}", "owned",
"openshiftClusterID", "${var.cluster_id}",
"clusterid", "${var.cluster_name}"
), var.extra_tags)}"
), var.tags)}"

root_block_device {
volume_type = "${var.root_volume_type}"
Expand All @@ -108,9 +106,7 @@ resource "aws_instance" "master" {

volume_tags = "${merge(map(
"Name", "${var.cluster_name}-master-${count.index}-vol",
"kubernetes.io/cluster/${var.cluster_name}", "owned",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
), var.tags)}"
}

resource "aws_lb_target_group_attachment" "master" {
Expand Down
12 changes: 6 additions & 6 deletions data/data/aws/master/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,6 @@ variable "ec2_type" {
type = "string"
}

variable "extra_tags" {
description = "Extra AWS tags to be applied to created resources."
type = "map"
default = {}
}

variable "ec2_ami" {
type = "string"
default = ""
Expand Down Expand Up @@ -71,6 +65,12 @@ variable "subnet_ids" {
type = "list"
}

variable "tags" {
type = "map"
default = {}
description = "AWS tags to be applied to created resources."
}

variable "target_group_arns" {
type = "list"
default = []
Expand Down
25 changes: 5 additions & 20 deletions data/data/aws/vpc/master-elb.tf
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,7 @@ resource "aws_lb" "api_internal" {
enable_cross_zone_load_balancing = true
idle_timeout = 3600

tags = "${merge(map(
"kubernetes.io/cluster/${var.cluster_name}", "owned",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
tags = "${tags}"
}

resource "aws_lb" "api_external" {
Expand All @@ -20,10 +17,7 @@ resource "aws_lb" "api_external" {
enable_cross_zone_load_balancing = true
idle_timeout = 3600

tags = "${merge(map(
"kubernetes.io/cluster/${var.cluster_name}", "owned",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
tags = "${tags}"
}

resource "aws_lb_target_group" "api_internal" {
Expand All @@ -34,10 +28,7 @@ resource "aws_lb_target_group" "api_internal" {

target_type = "ip"

tags = "${merge(map(
"kubernetes.io/cluster/${var.cluster_name}", "owned",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
tags = "${var.tags}"

health_check {
healthy_threshold = 3
Expand All @@ -57,10 +48,7 @@ resource "aws_lb_target_group" "api_external" {

target_type = "ip"

tags = "${merge(map(
"kubernetes.io/cluster/${var.cluster_name}", "owned",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
tags = "${var.tags}"

health_check {
healthy_threshold = 3
Expand All @@ -80,10 +68,7 @@ resource "aws_lb_target_group" "services" {

target_type = "ip"

tags = "${merge(map(
"kubernetes.io/cluster/${var.cluster_name}", "owned",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
tags = "${var.tags}"

health_check {
healthy_threshold = 3
Expand Down
8 changes: 2 additions & 6 deletions data/data/aws/vpc/sg-elb.tf
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ resource "aws_security_group" "api" {

tags = "${merge(map(
"Name", "${var.cluster_name}_api_sg",
"kubernetes.io/cluster/${var.cluster_name}", "owned",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
), var.tags)}"
}

resource "aws_security_group_rule" "api_egress" {
Expand Down Expand Up @@ -43,9 +41,7 @@ resource "aws_security_group" "console" {

tags = "${merge(map(
"Name", "${var.cluster_name}_console_sg",
"kubernetes.io/cluster/${var.cluster_name}", "owned",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
), var.tags)}"
}

resource "aws_security_group_rule" "console_egress" {
Expand Down
4 changes: 1 addition & 3 deletions data/data/aws/vpc/sg-etcd.tf
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ resource "aws_security_group" "etcd" {

tags = "${merge(map(
"Name", "${var.cluster_name}_etcd_sg",
"kubernetes.io/cluster/${var.cluster_name}", "owned",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
), var.tags)}"
}

resource "aws_security_group_rule" "etcd_egress" {
Expand Down
4 changes: 1 addition & 3 deletions data/data/aws/vpc/sg-master.tf
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ resource "aws_security_group" "master" {

tags = "${merge(map(
"Name", "${var.cluster_name}_master_sg",
"kubernetes.io/cluster/${var.cluster_name}", "owned",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
), var.tags)}"
}

resource "aws_security_group_rule" "master_mcs" {
Expand Down
4 changes: 1 addition & 3 deletions data/data/aws/vpc/sg-worker.tf
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ resource "aws_security_group" "worker" {

tags = "${merge(map(
"Name", "${var.cluster_name}_worker_sg",
"kubernetes.io/cluster/${var.cluster_name}", "owned",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
), var.tags)}"
}

resource "aws_security_group_rule" "worker_egress" {
Expand Down
12 changes: 6 additions & 6 deletions data/data/aws/vpc/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,6 @@ variable "cluster_name" {
type = "string"
}

variable "extra_tags" {
description = "Extra AWS tags to be applied to created resources."
type = "map"
default = {}
}

variable "private_master_endpoints" {
description = "If set to true, private-facing ingress resources are created."
default = true
Expand All @@ -34,3 +28,9 @@ variable "region" {
type = "string"
description = "The target AWS region for the cluster."
}

variable "tags" {
type = "map"
default = {}
description = "AWS tags to be applied to created resources."
}
13 changes: 4 additions & 9 deletions data/data/aws/vpc/vpc-private.tf
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@ resource "aws_route_table" "private_routes" {

tags = "${merge(map(
"Name","${var.cluster_name}-private-${local.new_subnet_azs[count.index]}",
"kubernetes.io/cluster/${var.cluster_name}", "shared",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
), var.tags)}"
}

resource "aws_route" "to_nat_gw" {
Expand All @@ -27,12 +25,9 @@ resource "aws_subnet" "worker_subnet" {
availability_zone = "${local.new_subnet_azs[count.index]}"

tags = "${merge(map(
"Name", "${var.cluster_name}-worker-${local.new_subnet_azs[count.index]}",
"kubernetes.io/cluster/${var.cluster_name}","shared",
"kubernetes.io/role/internal-elb", "",
"openshiftClusterID", "${var.cluster_id}"
),
var.extra_tags)}"
"Name", "${var.cluster_name}-worker-${local.new_subnet_azs[count.index]}",
"kubernetes.io/role/internal-elb", "",
), var.tags)}"
}

resource "aws_route_table_association" "worker_routing" {
Expand Down
22 changes: 6 additions & 16 deletions data/data/aws/vpc/vpc-public.tf
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,15 @@ resource "aws_internet_gateway" "igw" {

tags = "${merge(map(
"Name", "${var.cluster_name}-igw",
"kubernetes.io/cluster/${var.cluster_name}", "shared",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
), var.tags)}"
}

resource "aws_route_table" "default" {
vpc_id = "${data.aws_vpc.cluster_vpc.id}"

tags = "${merge(map(
"Name", "${var.cluster_name}-public",
"kubernetes.io/cluster/${var.cluster_name}", "shared",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
), var.tags)}"
}

resource "aws_main_route_table_association" "main_vpc_routes" {
Expand All @@ -38,10 +34,8 @@ resource "aws_subnet" "master_subnet" {
availability_zone = "${local.new_subnet_azs[count.index]}"

tags = "${merge(map(
"Name", "${var.cluster_name}-master-${local.new_subnet_azs[count.index]}",
"kubernetes.io/cluster/${var.cluster_name}", "shared",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
"Name", "${var.cluster_name}-master-${local.new_subnet_azs[count.index]}",
), var.tags)}"
}

resource "aws_route_table_association" "route_net" {
Expand All @@ -54,9 +48,7 @@ resource "aws_eip" "nat_eip" {
count = "${local.new_az_count}"
vpc = true

tags = "${merge(map(
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
tags = "${var.tags}"

# Terraform does not declare an explicit dependency towards the internet gateway.
# this can cause the internet gateway to be deleted/detached before the EIPs.
Expand All @@ -69,7 +61,5 @@ resource "aws_nat_gateway" "nat_gw" {
allocation_id = "${aws_eip.nat_eip.*.id[count.index]}"
subnet_id = "${aws_subnet.master_subnet.*.id[count.index]}"

tags = "${merge(map(
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
tags = "${var.tags}"
}
4 changes: 1 addition & 3 deletions data/data/aws/vpc/vpc.tf
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ resource "aws_vpc" "new_vpc" {

tags = "${merge(map(
"Name", "${var.cluster_name}.${var.base_domain}",
"kubernetes.io/cluster/${var.cluster_name}", "shared",
"openshiftClusterID", "${var.cluster_id}"
), var.extra_tags)}"
), var.tags)}"
}

resource "aws_vpc_endpoint" "s3" {
Expand Down

0 comments on commit 476e1f3

Please sign in to comment.