From 076742f52875393fb20ab6b31e496e124c651ebb Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Tue, 2 Oct 2018 10:31:19 -0700 Subject: [PATCH 1/8] Still materialize terraform output in tests If we skip it, we can't test it. We do expect that most users will use a lifecycle that only warns though. --- cmd/kops/integration_test.go | 1 + .../existing_sg/in-v1alpha2.yaml | 10 +- .../update_cluster/existing_sg/kubernetes.tf | 306 +++++++++++++++++- 3 files changed, 303 insertions(+), 14 deletions(-) diff --git a/cmd/kops/integration_test.go b/cmd/kops/integration_test.go index 545c00e458..8b03b3cf4e 100644 --- a/cmd/kops/integration_test.go +++ b/cmd/kops/integration_test.go @@ -88,6 +88,7 @@ func TestExistingIAMCloudformation(t *testing.T) { // TestExistingSG runs the test with existing Security Group, similar to kops create cluster minimal.example.com --zones us-west-1a func TestExistingSG(t *testing.T) { lifecycleOverrides := []string{"SecurityGroup=ExistsAndWarnIfChanges", "SecurityGroupRule=ExistsAndWarnIfChanges"} + lifecycleOverrides = nil runTestAWS(t, "existingsg.example.com", "existing_sg", "v1alpha2", false, 3, true, lifecycleOverrides) } diff --git a/tests/integration/update_cluster/existing_sg/in-v1alpha2.yaml b/tests/integration/update_cluster/existing_sg/in-v1alpha2.yaml index 5a019aff5d..02d959a86f 100644 --- a/tests/integration/update_cluster/existing_sg/in-v1alpha2.yaml +++ b/tests/integration/update_cluster/existing_sg/in-v1alpha2.yaml @@ -6,7 +6,7 @@ metadata: spec: api: loadBalancer: - securityGroupOverride: sg-abcd1234 + securityGroupOverride: sg-elb # Not a valid name format on AWS, but doesn't matter for our tests kubernetesApiAccess: - 0.0.0.0/0 channel: stable @@ -72,7 +72,7 @@ spec: role: Master subnets: - us-test-1a - securityGroupOverride: sg-1234dcba + securityGroupOverride: sg-master-1a --- @@ -91,7 +91,7 @@ spec: role: Master subnets: - us-test-1b - securityGroupOverride: sg-1234dcba + securityGroupOverride: sg-master-1b --- @@ -110,7 +110,7 @@ spec: role: Master subnets: - us-test-1c - securityGroupOverride: sg-1234dcba + securityGroupOverride: sg-master-1c --- @@ -130,5 +130,5 @@ spec: role: Node subnets: - us-test-1a - securityGroupOverride: sg-1234abcd + securityGroupOverride: sg-nodes diff --git a/tests/integration/update_cluster/existing_sg/kubernetes.tf b/tests/integration/update_cluster/existing_sg/kubernetes.tf index 291a944c5d..5a09d5eb79 100644 --- a/tests/integration/update_cluster/existing_sg/kubernetes.tf +++ b/tests/integration/update_cluster/existing_sg/kubernetes.tf @@ -1,11 +1,11 @@ locals = { cluster_name = "existingsg.example.com" master_autoscaling_group_ids = ["${aws_autoscaling_group.master-us-test-1a-masters-existingsg-example-com.id}", "${aws_autoscaling_group.master-us-test-1b-masters-existingsg-example-com.id}", "${aws_autoscaling_group.master-us-test-1c-masters-existingsg-example-com.id}"] - master_security_group_ids = ["sg-1234dcba"] + master_security_group_ids = ["sg-master-1a", "sg-master-1b", "sg-master-1c"] masters_role_arn = "${aws_iam_role.masters-existingsg-example-com.arn}" masters_role_name = "${aws_iam_role.masters-existingsg-example-com.name}" node_autoscaling_group_ids = ["${aws_autoscaling_group.nodes-existingsg-example-com.id}"] - node_security_group_ids = ["sg-1234abcd"] + node_security_group_ids = ["sg-nodes"] node_subnet_ids = ["${aws_subnet.us-test-1a-existingsg-example-com.id}"] nodes_role_arn = "${aws_iam_role.nodes-existingsg-example-com.arn}" nodes_role_name = "${aws_iam_role.nodes-existingsg-example-com.name}" @@ -27,7 +27,7 @@ output "master_autoscaling_group_ids" { } output "master_security_group_ids" { - value = ["sg-1234dcba"] + value = ["sg-master-1a", "sg-master-1b", "sg-master-1c"] } output "masters_role_arn" { @@ -43,7 +43,7 @@ output "node_autoscaling_group_ids" { } output "node_security_group_ids" { - value = ["sg-1234abcd"] + value = ["sg-nodes"] } output "node_subnet_ids" { @@ -321,7 +321,7 @@ resource "aws_elb" "api-existingsg-example-com" { lb_protocol = "TCP" } - security_groups = ["sg-abcd1234"] + security_groups = ["sg-elb"] subnets = ["${aws_subnet.us-test-1a-existingsg-example-com.id}", "${aws_subnet.us-test-1b-existingsg-example-com.id}", "${aws_subnet.us-test-1c-existingsg-example-com.id}"] health_check = { @@ -393,7 +393,7 @@ resource "aws_launch_configuration" "master-us-test-1a-masters-existingsg-exampl instance_type = "m3.medium" key_name = "${aws_key_pair.kubernetes-existingsg-example-com-c4a6ed9aa889b9e2c39cd663eb9c7157.id}" iam_instance_profile = "${aws_iam_instance_profile.masters-existingsg-example-com.id}" - security_groups = ["sg-1234dcba"] + security_groups = ["sg-master-1a"] associate_public_ip_address = true user_data = "${file("${path.module}/data/aws_launch_configuration_master-us-test-1a.masters.existingsg.example.com_user_data")}" @@ -421,7 +421,7 @@ resource "aws_launch_configuration" "master-us-test-1b-masters-existingsg-exampl instance_type = "m3.medium" key_name = "${aws_key_pair.kubernetes-existingsg-example-com-c4a6ed9aa889b9e2c39cd663eb9c7157.id}" iam_instance_profile = "${aws_iam_instance_profile.masters-existingsg-example-com.id}" - security_groups = ["sg-1234dcba"] + security_groups = ["sg-master-1b"] associate_public_ip_address = true user_data = "${file("${path.module}/data/aws_launch_configuration_master-us-test-1b.masters.existingsg.example.com_user_data")}" @@ -449,7 +449,7 @@ resource "aws_launch_configuration" "master-us-test-1c-masters-existingsg-exampl instance_type = "m3.medium" key_name = "${aws_key_pair.kubernetes-existingsg-example-com-c4a6ed9aa889b9e2c39cd663eb9c7157.id}" iam_instance_profile = "${aws_iam_instance_profile.masters-existingsg-example-com.id}" - security_groups = ["sg-1234dcba"] + security_groups = ["sg-master-1c"] associate_public_ip_address = true user_data = "${file("${path.module}/data/aws_launch_configuration_master-us-test-1c.masters.existingsg.example.com_user_data")}" @@ -477,7 +477,7 @@ resource "aws_launch_configuration" "nodes-existingsg-example-com" { instance_type = "t2.medium" key_name = "${aws_key_pair.kubernetes-existingsg-example-com-c4a6ed9aa889b9e2c39cd663eb9c7157.id}" iam_instance_profile = "${aws_iam_instance_profile.nodes-existingsg-example-com.id}" - security_groups = ["sg-1234abcd"] + security_groups = ["sg-nodes"] associate_public_ip_address = true user_data = "${file("${path.module}/data/aws_launch_configuration_nodes.existingsg.example.com_user_data")}" @@ -539,6 +539,294 @@ resource "aws_route_table_association" "us-test-1c-existingsg-example-com" { route_table_id = "${aws_route_table.existingsg-example-com.id}" } +resource "aws_security_group_rule" "all-master-to-master-sg-master-1a" { + type = "ingress" + security_group_id = "sg-master-1a" + source_security_group_id = "sg-master-1a" + from_port = 0 + to_port = 0 + protocol = "-1" +} + +resource "aws_security_group_rule" "all-master-to-master-sg-master-1b" { + type = "ingress" + security_group_id = "sg-master-1b" + source_security_group_id = "sg-master-1b" + from_port = 0 + to_port = 0 + protocol = "-1" +} + +resource "aws_security_group_rule" "all-master-to-master-sg-master-1c" { + type = "ingress" + security_group_id = "sg-master-1c" + source_security_group_id = "sg-master-1c" + from_port = 0 + to_port = 0 + protocol = "-1" +} + +resource "aws_security_group_rule" "all-master-to-nodesg-master-1a-sg-nodes" { + type = "ingress" + security_group_id = "sg-nodes" + source_security_group_id = "sg-master-1a" + from_port = 0 + to_port = 0 + protocol = "-1" +} + +resource "aws_security_group_rule" "all-master-to-nodesg-master-1b-sg-nodes" { + type = "ingress" + security_group_id = "sg-nodes" + source_security_group_id = "sg-master-1b" + from_port = 0 + to_port = 0 + protocol = "-1" +} + +resource "aws_security_group_rule" "all-master-to-nodesg-master-1c-sg-nodes" { + type = "ingress" + security_group_id = "sg-nodes" + source_security_group_id = "sg-master-1c" + from_port = 0 + to_port = 0 + protocol = "-1" +} + +resource "aws_security_group_rule" "all-node-to-node" { + type = "ingress" + security_group_id = "sg-nodes" + source_security_group_id = "sg-nodes" + from_port = 0 + to_port = 0 + protocol = "-1" +} + +resource "aws_security_group_rule" "api-elb-egress" { + type = "egress" + security_group_id = "sg-elb" + from_port = 0 + to_port = 0 + protocol = "-1" + cidr_blocks = ["0.0.0.0/0"] +} + +resource "aws_security_group_rule" "https-api-elb-0-0-0-0--0" { + type = "ingress" + security_group_id = "sg-elb" + from_port = 443 + to_port = 443 + protocol = "tcp" + cidr_blocks = ["0.0.0.0/0"] +} + +resource "aws_security_group_rule" "https-elb-to-master-sg-master-1a" { + type = "ingress" + security_group_id = "sg-master-1a" + source_security_group_id = "sg-elb" + from_port = 443 + to_port = 443 + protocol = "tcp" +} + +resource "aws_security_group_rule" "https-elb-to-master-sg-master-1b" { + type = "ingress" + security_group_id = "sg-master-1b" + source_security_group_id = "sg-elb" + from_port = 443 + to_port = 443 + protocol = "tcp" +} + +resource "aws_security_group_rule" "https-elb-to-master-sg-master-1c" { + type = "ingress" + security_group_id = "sg-master-1c" + source_security_group_id = "sg-elb" + from_port = 443 + to_port = 443 + protocol = "tcp" +} + +resource "aws_security_group_rule" "master-egress-sg-master-1a" { + type = "egress" + security_group_id = "sg-master-1a" + from_port = 0 + to_port = 0 + protocol = "-1" + cidr_blocks = ["0.0.0.0/0"] +} + +resource "aws_security_group_rule" "master-egress-sg-master-1b" { + type = "egress" + security_group_id = "sg-master-1b" + from_port = 0 + to_port = 0 + protocol = "-1" + cidr_blocks = ["0.0.0.0/0"] +} + +resource "aws_security_group_rule" "master-egress-sg-master-1c" { + type = "egress" + security_group_id = "sg-master-1c" + from_port = 0 + to_port = 0 + protocol = "-1" + cidr_blocks = ["0.0.0.0/0"] +} + +resource "aws_security_group_rule" "node-egress" { + type = "egress" + security_group_id = "sg-nodes" + from_port = 0 + to_port = 0 + protocol = "-1" + cidr_blocks = ["0.0.0.0/0"] +} + +resource "aws_security_group_rule" "node-to-master-tcp-1-2379-sg-master-1a-sg-nodes" { + type = "ingress" + security_group_id = "sg-master-1a" + source_security_group_id = "sg-nodes" + from_port = 1 + to_port = 2379 + protocol = "tcp" +} + +resource "aws_security_group_rule" "node-to-master-tcp-1-2379-sg-master-1b-sg-nodes" { + type = "ingress" + security_group_id = "sg-master-1b" + source_security_group_id = "sg-nodes" + from_port = 1 + to_port = 2379 + protocol = "tcp" +} + +resource "aws_security_group_rule" "node-to-master-tcp-1-2379-sg-master-1c-sg-nodes" { + type = "ingress" + security_group_id = "sg-master-1c" + source_security_group_id = "sg-nodes" + from_port = 1 + to_port = 2379 + protocol = "tcp" +} + +resource "aws_security_group_rule" "node-to-master-tcp-2382-4000-sg-master-1a-sg-nodes" { + type = "ingress" + security_group_id = "sg-master-1a" + source_security_group_id = "sg-nodes" + from_port = 2382 + to_port = 4000 + protocol = "tcp" +} + +resource "aws_security_group_rule" "node-to-master-tcp-2382-4000-sg-master-1b-sg-nodes" { + type = "ingress" + security_group_id = "sg-master-1b" + source_security_group_id = "sg-nodes" + from_port = 2382 + to_port = 4000 + protocol = "tcp" +} + +resource "aws_security_group_rule" "node-to-master-tcp-2382-4000-sg-master-1c-sg-nodes" { + type = "ingress" + security_group_id = "sg-master-1c" + source_security_group_id = "sg-nodes" + from_port = 2382 + to_port = 4000 + protocol = "tcp" +} + +resource "aws_security_group_rule" "node-to-master-tcp-4003-65535-sg-master-1a-sg-nodes" { + type = "ingress" + security_group_id = "sg-master-1a" + source_security_group_id = "sg-nodes" + from_port = 4003 + to_port = 65535 + protocol = "tcp" +} + +resource "aws_security_group_rule" "node-to-master-tcp-4003-65535-sg-master-1b-sg-nodes" { + type = "ingress" + security_group_id = "sg-master-1b" + source_security_group_id = "sg-nodes" + from_port = 4003 + to_port = 65535 + protocol = "tcp" +} + +resource "aws_security_group_rule" "node-to-master-tcp-4003-65535-sg-master-1c-sg-nodes" { + type = "ingress" + security_group_id = "sg-master-1c" + source_security_group_id = "sg-nodes" + from_port = 4003 + to_port = 65535 + protocol = "tcp" +} + +resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-master-1a-sg-nodes" { + type = "ingress" + security_group_id = "sg-master-1a" + source_security_group_id = "sg-nodes" + from_port = 1 + to_port = 65535 + protocol = "udp" +} + +resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-master-1b-sg-nodes" { + type = "ingress" + security_group_id = "sg-master-1b" + source_security_group_id = "sg-nodes" + from_port = 1 + to_port = 65535 + protocol = "udp" +} + +resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-master-1c-sg-nodes" { + type = "ingress" + security_group_id = "sg-master-1c" + source_security_group_id = "sg-nodes" + from_port = 1 + to_port = 65535 + protocol = "udp" +} + +resource "aws_security_group_rule" "ssh-external-to-master-0-0-0-0--0-sg-master-1a" { + type = "ingress" + security_group_id = "sg-master-1a" + from_port = 22 + to_port = 22 + protocol = "tcp" + cidr_blocks = ["0.0.0.0/0"] +} + +resource "aws_security_group_rule" "ssh-external-to-master-0-0-0-0--0-sg-master-1b" { + type = "ingress" + security_group_id = "sg-master-1b" + from_port = 22 + to_port = 22 + protocol = "tcp" + cidr_blocks = ["0.0.0.0/0"] +} + +resource "aws_security_group_rule" "ssh-external-to-master-0-0-0-0--0-sg-master-1c" { + type = "ingress" + security_group_id = "sg-master-1c" + from_port = 22 + to_port = 22 + protocol = "tcp" + cidr_blocks = ["0.0.0.0/0"] +} + +resource "aws_security_group_rule" "ssh-external-to-node-0-0-0-0--0" { + type = "ingress" + security_group_id = "sg-nodes" + from_port = 22 + to_port = 22 + protocol = "tcp" + cidr_blocks = ["0.0.0.0/0"] +} + resource "aws_subnet" "us-test-1a-existingsg-example-com" { vpc_id = "${aws_vpc.existingsg-example-com.id}" cidr_block = "172.20.32.0/19" From bfb54935ff51a9ca3789175814800e658cd46c35 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Tue, 2 Oct 2018 10:47:47 -0700 Subject: [PATCH 2/8] Build security groups along with suffixes Fixes the case where we mix use of specified & default SGs. --- pkg/model/awsmodel/api_loadbalancer.go | 16 +- pkg/model/external_access.go | 26 +-- pkg/model/firewall.go | 151 +++++++++--------- .../update_cluster/existing_sg/kubernetes.tf | 12 +- 4 files changed, 98 insertions(+), 107 deletions(-) diff --git a/pkg/model/awsmodel/api_loadbalancer.go b/pkg/model/awsmodel/api_loadbalancer.go index 3731b19162..ffdd2ecce1 100644 --- a/pkg/model/awsmodel/api_loadbalancer.go +++ b/pkg/model/awsmodel/api_loadbalancer.go @@ -236,13 +236,13 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { // Allow HTTPS to the master instances from the ELB { - for masterGroupName, masterGroup := range masterGroups { - suffix := GetGroupSuffix(masterGroupName, masterGroups) + for _, masterGroup := range masterGroups { + suffix := masterGroup.Suffix t := &awstasks.SecurityGroupRule{ Name: s(fmt.Sprintf("https-elb-to-master%s", suffix)), Lifecycle: b.SecurityLifecycle, - SecurityGroup: masterGroup, + SecurityGroup: masterGroup.Task, SourceGroup: lbSG, FromPort: i64(443), ToPort: i64(443), @@ -343,13 +343,3 @@ func (b *APILoadBalancerBuilder) chooseBestSubnetForELB(zone string, subnets []* return scoredSubnets[0].subnet } - -// GetGroupSuffix returns the name of the security groups suffix. -func GetGroupSuffix(name string, groups map[string]*awstasks.SecurityGroup) string { - if len(groups) != 1 { - glog.V(8).Infof("adding group suffix: %q", name) - return "-" + name - } - - return "" -} diff --git a/pkg/model/external_access.go b/pkg/model/external_access.go index ded0f3ec5a..4394ab0dec 100644 --- a/pkg/model/external_access.go +++ b/pkg/model/external_access.go @@ -61,12 +61,12 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error { } else { for _, sshAccess := range b.Cluster.Spec.SSHAccess { - for masterGroupName, masterGroup := range masterGroups { - suffix := GetGroupSuffix(masterGroupName, masterGroups) + for _, masterGroup := range masterGroups { + suffix := masterGroup.Suffix t := &awstasks.SecurityGroupRule{ Name: s(fmt.Sprintf("ssh-external-to-master-%s%s", sshAccess, suffix)), Lifecycle: b.Lifecycle, - SecurityGroup: masterGroup, + SecurityGroup: masterGroup.Task, Protocol: s("tcp"), FromPort: i64(22), ToPort: i64(22), @@ -75,12 +75,12 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error { c.AddTask(t) } - for nodeGroupName, nodeGroup := range nodeGroups { - suffix := GetGroupSuffix(nodeGroupName, nodeGroups) + for _, nodeGroup := range nodeGroups { + suffix := nodeGroup.Suffix t := &awstasks.SecurityGroupRule{ Name: s(fmt.Sprintf("ssh-external-to-node-%s%s", sshAccess, suffix)), Lifecycle: b.Lifecycle, - SecurityGroup: nodeGroup, + SecurityGroup: nodeGroup.Task, Protocol: s("tcp"), FromPort: i64(22), ToPort: i64(22), @@ -97,12 +97,12 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error { return err } - for nodeGroupName, nodeGroup := range nodeGroups { - suffix := GetGroupSuffix(nodeGroupName, nodeGroups) + for _, nodeGroup := range nodeGroups { + suffix := nodeGroup.Suffix t1 := &awstasks.SecurityGroupRule{ Name: s(fmt.Sprintf("nodeport-tcp-external-to-node-%s%s", nodePortAccess, suffix)), Lifecycle: b.Lifecycle, - SecurityGroup: nodeGroup, + SecurityGroup: nodeGroup.Task, Protocol: s("tcp"), FromPort: i64(int64(nodePortRange.Base)), ToPort: i64(int64(nodePortRange.Base + nodePortRange.Size - 1)), @@ -113,7 +113,7 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error { t2 := &awstasks.SecurityGroupRule{ Name: s(fmt.Sprintf("nodeport-udp-external-to-node-%s%s", nodePortAccess, suffix)), Lifecycle: b.Lifecycle, - SecurityGroup: nodeGroup, + SecurityGroup: nodeGroup.Task, Protocol: s("udp"), FromPort: i64(int64(nodePortRange.Base)), ToPort: i64(int64(nodePortRange.Base + nodePortRange.Size - 1)), @@ -130,12 +130,12 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error { // HTTPS to the master is allowed (for API access) for _, apiAccess := range b.Cluster.Spec.KubernetesAPIAccess { - for masterGroupName, masterGroup := range masterGroups { - suffix := GetGroupSuffix(masterGroupName, masterGroups) + for _, masterGroup := range masterGroups { + suffix := masterGroup.Suffix t := &awstasks.SecurityGroupRule{ Name: s(fmt.Sprintf("https-external-to-master-%s%s", apiAccess, suffix)), Lifecycle: b.Lifecycle, - SecurityGroup: masterGroup, + SecurityGroup: masterGroup.Task, Protocol: s("tcp"), FromPort: i64(443), ToPort: i64(443), diff --git a/pkg/model/firewall.go b/pkg/model/firewall.go index 7a4859611a..42eb44d2ee 100644 --- a/pkg/model/firewall.go +++ b/pkg/model/firewall.go @@ -62,21 +62,21 @@ func (b *FirewallModelBuilder) Build(c *fi.ModelBuilderContext) error { return nil } -func (b *FirewallModelBuilder) buildNodeRules(c *fi.ModelBuilderContext) (map[string]*awstasks.SecurityGroup, error) { +func (b *FirewallModelBuilder) buildNodeRules(c *fi.ModelBuilderContext) ([]SecurityGroupInfo, error) { nodeGroups, err := b.createSecurityGroups(kops.InstanceGroupRoleNode, b.Lifecycle, c) if err != nil { return nil, err } - for nodeGroupName, secGroup := range nodeGroups { - suffix := GetGroupSuffix(nodeGroupName, nodeGroups) + for _, nodeGroup := range nodeGroups { + suffix := nodeGroup.Suffix // Allow full egress { t := &awstasks.SecurityGroupRule{ Name: s(fmt.Sprintf("node-egress%s", suffix)), Lifecycle: b.Lifecycle, - SecurityGroup: secGroup, + SecurityGroup: nodeGroup.Task, Egress: fi.Bool(true), CIDR: s("0.0.0.0/0"), } @@ -88,8 +88,8 @@ func (b *FirewallModelBuilder) buildNodeRules(c *fi.ModelBuilderContext) (map[st t := &awstasks.SecurityGroupRule{ Name: s(fmt.Sprintf("all-node-to-node%s", suffix)), Lifecycle: b.Lifecycle, - SecurityGroup: secGroup, - SourceGroup: secGroup, + SecurityGroup: nodeGroup.Task, + SourceGroup: nodeGroup.Task, } c.AddTask(t) } @@ -226,7 +226,7 @@ func (b *FirewallModelBuilder) applyNodeToMasterAllowSpecificPorts(c *fi.ModelBu } } -func (b *FirewallModelBuilder) applyNodeToMasterBlockSpecificPorts(c *fi.ModelBuilderContext, nodeGroups map[string]*awstasks.SecurityGroup, masterGroups map[string]*awstasks.SecurityGroup) { +func (b *FirewallModelBuilder) applyNodeToMasterBlockSpecificPorts(c *fi.ModelBuilderContext, nodeGroups []SecurityGroupInfo, masterGroups []SecurityGroupInfo) { type portRange struct { From int To int @@ -287,30 +287,24 @@ func (b *FirewallModelBuilder) applyNodeToMasterBlockSpecificPorts(c *fi.ModelBu } } - for masterSecGroupName, masterGroup := range masterGroups { - var masterSuffix string + for _, masterGroup := range masterGroups { + masterSuffix := masterGroup.Suffix var nodeSuffix string - if len(masterGroups) != 1 { - masterSuffix = "-" + masterSecGroupName - } else { - masterSuffix = "" - } + for _, nodeGroup := range nodeGroups { - for nodeSecGroupName, nodeGroup := range nodeGroups { - - if len(masterGroups) == 1 && len(nodeGroups) == 1 { + if masterSuffix == "" && nodeGroup.Suffix == "" { nodeSuffix = "" } else { - nodeSuffix = fmt.Sprintf("%s-%s", masterSuffix, nodeSecGroupName) + nodeSuffix = fmt.Sprintf("%s-%s", masterSuffix, nodeGroup.Name) } for _, r := range udpRanges { t := &awstasks.SecurityGroupRule{ Name: s(fmt.Sprintf("node-to-master-udp-%d-%d%s", r.From, r.To, nodeSuffix)), Lifecycle: b.Lifecycle, - SecurityGroup: masterGroup, - SourceGroup: nodeGroup, + SecurityGroup: masterGroup.Task, + SourceGroup: nodeGroup.Task, FromPort: i64(int64(r.From)), ToPort: i64(int64(r.To)), Protocol: s("udp"), @@ -321,8 +315,8 @@ func (b *FirewallModelBuilder) applyNodeToMasterBlockSpecificPorts(c *fi.ModelBu t := &awstasks.SecurityGroupRule{ Name: s(fmt.Sprintf("node-to-master-tcp-%d-%d%s", r.From, r.To, nodeSuffix)), Lifecycle: b.Lifecycle, - SecurityGroup: masterGroup, - SourceGroup: nodeGroup, + SecurityGroup: masterGroup.Task, + SourceGroup: nodeGroup.Task, FromPort: i64(int64(r.From)), ToPort: i64(int64(r.To)), Protocol: s("tcp"), @@ -342,8 +336,8 @@ func (b *FirewallModelBuilder) applyNodeToMasterBlockSpecificPorts(c *fi.ModelBu t := &awstasks.SecurityGroupRule{ Name: s(fmt.Sprintf("node-to-master-protocol-%s%s", name, nodeSuffix)), Lifecycle: b.Lifecycle, - SecurityGroup: masterGroup, - SourceGroup: nodeGroup, + SecurityGroup: masterGroup.Task, + SourceGroup: nodeGroup.Task, Protocol: s(awsName), } c.AddTask(t) @@ -352,20 +346,20 @@ func (b *FirewallModelBuilder) applyNodeToMasterBlockSpecificPorts(c *fi.ModelBu } } -func (b *FirewallModelBuilder) buildMasterRules(c *fi.ModelBuilderContext, nodeGroups map[string]*awstasks.SecurityGroup) (map[string]*awstasks.SecurityGroup, error) { +func (b *FirewallModelBuilder) buildMasterRules(c *fi.ModelBuilderContext, nodeGroups []SecurityGroupInfo) ([]SecurityGroupInfo, error) { masterGroups, err := b.createSecurityGroups(kops.InstanceGroupRoleMaster, b.Lifecycle, c) if err != nil { return nil, err } - for masterSecGroupName, masterGroup := range masterGroups { - suffix := GetGroupSuffix(masterSecGroupName, masterGroups) + for _, masterGroup := range masterGroups { + masterSuffix := masterGroup.Suffix // Allow full egress { t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("master-egress%s", suffix)), + Name: s(fmt.Sprintf("master-egress%s", masterSuffix)), Lifecycle: b.Lifecycle, - SecurityGroup: masterGroup, + SecurityGroup: masterGroup.Task, Egress: fi.Bool(true), CIDR: s("0.0.0.0/0"), } @@ -375,28 +369,23 @@ func (b *FirewallModelBuilder) buildMasterRules(c *fi.ModelBuilderContext, nodeG // Masters can talk to masters { t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("all-master-to-master%s", suffix)), + Name: s(fmt.Sprintf("all-master-to-master%s", masterSuffix)), Lifecycle: b.Lifecycle, - SecurityGroup: masterGroup, - SourceGroup: masterGroup, + SecurityGroup: masterGroup.Task, + SourceGroup: masterGroup.Task, } c.AddTask(t) } - for nodeSecGroupName, nodeGroup := range nodeGroups { - - if len(masterGroups) == 1 && len(nodeGroups) == 1 { - nodeSecGroupName = "" - } else { - nodeSecGroupName = fmt.Sprintf("%s-%s", masterSecGroupName, nodeSecGroupName) - } + for _, nodeGroup := range nodeGroups { + nodeSuffix := masterSuffix + nodeGroup.Suffix // Masters can talk to nodes { t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("all-master-to-node%s", nodeSecGroupName)), + Name: s(fmt.Sprintf("all-master-to-node%s", nodeSuffix)), Lifecycle: b.Lifecycle, - SecurityGroup: nodeGroup, - SourceGroup: masterGroup, + SecurityGroup: nodeGroup.Task, + SourceGroup: masterGroup.Task, } c.AddTask(t) } @@ -406,12 +395,17 @@ func (b *FirewallModelBuilder) buildMasterRules(c *fi.ModelBuilderContext, nodeG return masterGroups, nil } -func (b *KopsModelContext) GetSecurityGroups(role kops.InstanceGroupRole) (map[string]*awstasks.SecurityGroup, error) { +func (b *KopsModelContext) GetSecurityGroups(role kops.InstanceGroupRole) ([]SecurityGroupInfo, error) { return b.createSecurityGroups(role, nil, nil) } -func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lifecycle *fi.Lifecycle, c *fi.ModelBuilderContext) (map[string]*awstasks.SecurityGroup, error) { +type SecurityGroupInfo struct { + Name string + Suffix string + Task *awstasks.SecurityGroup +} +func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lifecycle *fi.Lifecycle, c *fi.ModelBuilderContext) ([]SecurityGroupInfo, error) { var baseGroup *awstasks.SecurityGroup if role == kops.InstanceGroupRoleMaster { name := "masters." + b.ClusterName() @@ -465,46 +459,53 @@ func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lif return nil, fmt.Errorf("not a supported security group type") } - groups := make(map[string]*awstasks.SecurityGroup) + var groups []SecurityGroupInfo + + // Build groups that specify a SecurityGroupOverride + allOverrides := true for _, ig := range b.InstanceGroups { - if ig.Spec.SecurityGroupOverride != nil && ig.Spec.Role == role { - name := fi.StringValue(ig.Spec.SecurityGroupOverride) - t := &awstasks.SecurityGroup{ - Name: ig.Spec.SecurityGroupOverride, - ID: ig.Spec.SecurityGroupOverride, - Lifecycle: lifecycle, - VPC: b.LinkToVPC(), - Shared: fi.Bool(true), - Description: baseGroup.Description, - } - groups[name] = t + if ig.Spec.Role != role { + continue } + + if ig.Spec.SecurityGroupOverride == nil { + allOverrides = false + continue + } + + name := fi.StringValue(ig.Spec.SecurityGroupOverride) + t := &awstasks.SecurityGroup{ + Name: ig.Spec.SecurityGroupOverride, + ID: ig.Spec.SecurityGroupOverride, + Lifecycle: lifecycle, + VPC: b.LinkToVPC(), + Shared: fi.Bool(true), + Description: baseGroup.Description, + } + + suffix := "-" + name + + groups = append(groups, SecurityGroupInfo{ + Name: name, + Suffix: suffix, + Task: t, + }) + } - for name, t := range groups { - if c != nil { - glog.V(8).Infof("adding security group: %q", name) - c.AddTask(t) - } + if !allOverrides { + groups = append(groups, SecurityGroupInfo{ + Name: fi.StringValue(baseGroup.Name), + Task: baseGroup, + }) } - if len(groups) == 0 { - groups[fi.StringValue(baseGroup.Name)] = baseGroup + for _, group := range groups { if c != nil { - glog.V(8).Infof("adding security group: %q", fi.StringValue(baseGroup.Name)) - c.AddTask(baseGroup) + glog.V(8).Infof("adding security group: %q", group.Name) + c.AddTask(group.Task) } } return groups, nil } - -// GetGroupSuffix returns the name of the security groups suffix. -func GetGroupSuffix(name string, groups map[string]*awstasks.SecurityGroup) string { - if len(groups) != 1 { - glog.V(8).Infof("adding group suffix: %q", name) - return "-" + name - } - - return "" -} diff --git a/tests/integration/update_cluster/existing_sg/kubernetes.tf b/tests/integration/update_cluster/existing_sg/kubernetes.tf index 5a09d5eb79..57ed4c9533 100644 --- a/tests/integration/update_cluster/existing_sg/kubernetes.tf +++ b/tests/integration/update_cluster/existing_sg/kubernetes.tf @@ -566,7 +566,7 @@ resource "aws_security_group_rule" "all-master-to-master-sg-master-1c" { protocol = "-1" } -resource "aws_security_group_rule" "all-master-to-nodesg-master-1a-sg-nodes" { +resource "aws_security_group_rule" "all-master-to-node-sg-master-1a-sg-nodes" { type = "ingress" security_group_id = "sg-nodes" source_security_group_id = "sg-master-1a" @@ -575,7 +575,7 @@ resource "aws_security_group_rule" "all-master-to-nodesg-master-1a-sg-nodes" { protocol = "-1" } -resource "aws_security_group_rule" "all-master-to-nodesg-master-1b-sg-nodes" { +resource "aws_security_group_rule" "all-master-to-node-sg-master-1b-sg-nodes" { type = "ingress" security_group_id = "sg-nodes" source_security_group_id = "sg-master-1b" @@ -584,7 +584,7 @@ resource "aws_security_group_rule" "all-master-to-nodesg-master-1b-sg-nodes" { protocol = "-1" } -resource "aws_security_group_rule" "all-master-to-nodesg-master-1c-sg-nodes" { +resource "aws_security_group_rule" "all-master-to-node-sg-master-1c-sg-nodes" { type = "ingress" security_group_id = "sg-nodes" source_security_group_id = "sg-master-1c" @@ -593,7 +593,7 @@ resource "aws_security_group_rule" "all-master-to-nodesg-master-1c-sg-nodes" { protocol = "-1" } -resource "aws_security_group_rule" "all-node-to-node" { +resource "aws_security_group_rule" "all-node-to-node-sg-nodes" { type = "ingress" security_group_id = "sg-nodes" source_security_group_id = "sg-nodes" @@ -674,7 +674,7 @@ resource "aws_security_group_rule" "master-egress-sg-master-1c" { cidr_blocks = ["0.0.0.0/0"] } -resource "aws_security_group_rule" "node-egress" { +resource "aws_security_group_rule" "node-egress-sg-nodes" { type = "egress" security_group_id = "sg-nodes" from_port = 0 @@ -818,7 +818,7 @@ resource "aws_security_group_rule" "ssh-external-to-master-0-0-0-0--0-sg-master- cidr_blocks = ["0.0.0.0/0"] } -resource "aws_security_group_rule" "ssh-external-to-node-0-0-0-0--0" { +resource "aws_security_group_rule" "ssh-external-to-node-0-0-0-0--0-sg-nodes" { type = "ingress" security_group_id = "sg-nodes" from_port = 22 From 1f2a8042b5d99f677eb27f1e56be52c90f481451 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Tue, 2 Oct 2018 10:50:04 -0700 Subject: [PATCH 3/8] Test case where we mix override & default SGs --- .../existing_sg/in-v1alpha2.yaml | 3 +- .../update_cluster/existing_sg/kubernetes.tf | 106 ++++++++++-------- 2 files changed, 61 insertions(+), 48 deletions(-) diff --git a/tests/integration/update_cluster/existing_sg/in-v1alpha2.yaml b/tests/integration/update_cluster/existing_sg/in-v1alpha2.yaml index 02d959a86f..e670497777 100644 --- a/tests/integration/update_cluster/existing_sg/in-v1alpha2.yaml +++ b/tests/integration/update_cluster/existing_sg/in-v1alpha2.yaml @@ -110,7 +110,8 @@ spec: role: Master subnets: - us-test-1c - securityGroupOverride: sg-master-1c + # We don't override the default for this master - testing mixing! + #securityGroupOverride: sg-master-1c --- diff --git a/tests/integration/update_cluster/existing_sg/kubernetes.tf b/tests/integration/update_cluster/existing_sg/kubernetes.tf index 57ed4c9533..f452c6c720 100644 --- a/tests/integration/update_cluster/existing_sg/kubernetes.tf +++ b/tests/integration/update_cluster/existing_sg/kubernetes.tf @@ -1,7 +1,7 @@ locals = { cluster_name = "existingsg.example.com" master_autoscaling_group_ids = ["${aws_autoscaling_group.master-us-test-1a-masters-existingsg-example-com.id}", "${aws_autoscaling_group.master-us-test-1b-masters-existingsg-example-com.id}", "${aws_autoscaling_group.master-us-test-1c-masters-existingsg-example-com.id}"] - master_security_group_ids = ["sg-master-1a", "sg-master-1b", "sg-master-1c"] + master_security_group_ids = ["${aws_security_group.masters-existingsg-example-com.id}", "sg-master-1a", "sg-master-1b"] masters_role_arn = "${aws_iam_role.masters-existingsg-example-com.arn}" masters_role_name = "${aws_iam_role.masters-existingsg-example-com.name}" node_autoscaling_group_ids = ["${aws_autoscaling_group.nodes-existingsg-example-com.id}"] @@ -27,7 +27,7 @@ output "master_autoscaling_group_ids" { } output "master_security_group_ids" { - value = ["sg-master-1a", "sg-master-1b", "sg-master-1c"] + value = ["${aws_security_group.masters-existingsg-example-com.id}", "sg-master-1a", "sg-master-1b"] } output "masters_role_arn" { @@ -449,7 +449,7 @@ resource "aws_launch_configuration" "master-us-test-1c-masters-existingsg-exampl instance_type = "m3.medium" key_name = "${aws_key_pair.kubernetes-existingsg-example-com-c4a6ed9aa889b9e2c39cd663eb9c7157.id}" iam_instance_profile = "${aws_iam_instance_profile.masters-existingsg-example-com.id}" - security_groups = ["sg-master-1c"] + security_groups = ["${aws_security_group.masters-existingsg-example-com.id}"] associate_public_ip_address = true user_data = "${file("${path.module}/data/aws_launch_configuration_master-us-test-1c.masters.existingsg.example.com_user_data")}" @@ -539,6 +539,27 @@ resource "aws_route_table_association" "us-test-1c-existingsg-example-com" { route_table_id = "${aws_route_table.existingsg-example-com.id}" } +resource "aws_security_group" "masters-existingsg-example-com" { + name = "masters.existingsg.example.com" + vpc_id = "${aws_vpc.existingsg-example-com.id}" + description = "Security group for masters" + + tags = { + KubernetesCluster = "existingsg.example.com" + Name = "masters.existingsg.example.com" + "kubernetes.io/cluster/existingsg.example.com" = "owned" + } +} + +resource "aws_security_group_rule" "all-master-to-master" { + type = "ingress" + security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" + source_security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" + from_port = 0 + to_port = 0 + protocol = "-1" +} + resource "aws_security_group_rule" "all-master-to-master-sg-master-1a" { type = "ingress" security_group_id = "sg-master-1a" @@ -557,15 +578,6 @@ resource "aws_security_group_rule" "all-master-to-master-sg-master-1b" { protocol = "-1" } -resource "aws_security_group_rule" "all-master-to-master-sg-master-1c" { - type = "ingress" - security_group_id = "sg-master-1c" - source_security_group_id = "sg-master-1c" - from_port = 0 - to_port = 0 - protocol = "-1" -} - resource "aws_security_group_rule" "all-master-to-node-sg-master-1a-sg-nodes" { type = "ingress" security_group_id = "sg-nodes" @@ -584,10 +596,10 @@ resource "aws_security_group_rule" "all-master-to-node-sg-master-1b-sg-nodes" { protocol = "-1" } -resource "aws_security_group_rule" "all-master-to-node-sg-master-1c-sg-nodes" { +resource "aws_security_group_rule" "all-master-to-node-sg-nodes" { type = "ingress" security_group_id = "sg-nodes" - source_security_group_id = "sg-master-1c" + source_security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" from_port = 0 to_port = 0 protocol = "-1" @@ -620,6 +632,15 @@ resource "aws_security_group_rule" "https-api-elb-0-0-0-0--0" { cidr_blocks = ["0.0.0.0/0"] } +resource "aws_security_group_rule" "https-elb-to-master" { + type = "ingress" + security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" + source_security_group_id = "sg-elb" + from_port = 443 + to_port = 443 + protocol = "tcp" +} + resource "aws_security_group_rule" "https-elb-to-master-sg-master-1a" { type = "ingress" security_group_id = "sg-master-1a" @@ -638,13 +659,13 @@ resource "aws_security_group_rule" "https-elb-to-master-sg-master-1b" { protocol = "tcp" } -resource "aws_security_group_rule" "https-elb-to-master-sg-master-1c" { - type = "ingress" - security_group_id = "sg-master-1c" - source_security_group_id = "sg-elb" - from_port = 443 - to_port = 443 - protocol = "tcp" +resource "aws_security_group_rule" "master-egress" { + type = "egress" + security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" + from_port = 0 + to_port = 0 + protocol = "-1" + cidr_blocks = ["0.0.0.0/0"] } resource "aws_security_group_rule" "master-egress-sg-master-1a" { @@ -665,15 +686,6 @@ resource "aws_security_group_rule" "master-egress-sg-master-1b" { cidr_blocks = ["0.0.0.0/0"] } -resource "aws_security_group_rule" "master-egress-sg-master-1c" { - type = "egress" - security_group_id = "sg-master-1c" - from_port = 0 - to_port = 0 - protocol = "-1" - cidr_blocks = ["0.0.0.0/0"] -} - resource "aws_security_group_rule" "node-egress-sg-nodes" { type = "egress" security_group_id = "sg-nodes" @@ -701,9 +713,9 @@ resource "aws_security_group_rule" "node-to-master-tcp-1-2379-sg-master-1b-sg-no protocol = "tcp" } -resource "aws_security_group_rule" "node-to-master-tcp-1-2379-sg-master-1c-sg-nodes" { +resource "aws_security_group_rule" "node-to-master-tcp-1-2379-sg-nodes" { type = "ingress" - security_group_id = "sg-master-1c" + security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" source_security_group_id = "sg-nodes" from_port = 1 to_port = 2379 @@ -728,9 +740,9 @@ resource "aws_security_group_rule" "node-to-master-tcp-2382-4000-sg-master-1b-sg protocol = "tcp" } -resource "aws_security_group_rule" "node-to-master-tcp-2382-4000-sg-master-1c-sg-nodes" { +resource "aws_security_group_rule" "node-to-master-tcp-2382-4000-sg-nodes" { type = "ingress" - security_group_id = "sg-master-1c" + security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" source_security_group_id = "sg-nodes" from_port = 2382 to_port = 4000 @@ -755,9 +767,9 @@ resource "aws_security_group_rule" "node-to-master-tcp-4003-65535-sg-master-1b-s protocol = "tcp" } -resource "aws_security_group_rule" "node-to-master-tcp-4003-65535-sg-master-1c-sg-nodes" { +resource "aws_security_group_rule" "node-to-master-tcp-4003-65535-sg-nodes" { type = "ingress" - security_group_id = "sg-master-1c" + security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" source_security_group_id = "sg-nodes" from_port = 4003 to_port = 65535 @@ -782,15 +794,24 @@ resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-master-1b-sg-n protocol = "udp" } -resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-master-1c-sg-nodes" { +resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-nodes" { type = "ingress" - security_group_id = "sg-master-1c" + security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" source_security_group_id = "sg-nodes" from_port = 1 to_port = 65535 protocol = "udp" } +resource "aws_security_group_rule" "ssh-external-to-master-0-0-0-0--0" { + type = "ingress" + security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" + from_port = 22 + to_port = 22 + protocol = "tcp" + cidr_blocks = ["0.0.0.0/0"] +} + resource "aws_security_group_rule" "ssh-external-to-master-0-0-0-0--0-sg-master-1a" { type = "ingress" security_group_id = "sg-master-1a" @@ -809,15 +830,6 @@ resource "aws_security_group_rule" "ssh-external-to-master-0-0-0-0--0-sg-master- cidr_blocks = ["0.0.0.0/0"] } -resource "aws_security_group_rule" "ssh-external-to-master-0-0-0-0--0-sg-master-1c" { - type = "ingress" - security_group_id = "sg-master-1c" - from_port = 22 - to_port = 22 - protocol = "tcp" - cidr_blocks = ["0.0.0.0/0"] -} - resource "aws_security_group_rule" "ssh-external-to-node-0-0-0-0--0-sg-nodes" { type = "ingress" security_group_id = "sg-nodes" From 1906bcdf5d65ee24c8c8463f9d24235370515eca Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Tue, 2 Oct 2018 11:07:53 -0700 Subject: [PATCH 4/8] We need to create the cross-product of rules for SG overrides e.g. each master SGs need to be configured to talk to each master SG --- pkg/model/firewall.go | 78 ++++++++++++------- .../update_cluster/existing_sg/kubernetes.tf | 78 ++++++++++++++++--- 2 files changed, 116 insertions(+), 40 deletions(-) diff --git a/pkg/model/firewall.go b/pkg/model/firewall.go index 42eb44d2ee..3c984c17f4 100644 --- a/pkg/model/firewall.go +++ b/pkg/model/firewall.go @@ -69,14 +69,13 @@ func (b *FirewallModelBuilder) buildNodeRules(c *fi.ModelBuilderContext) ([]Secu return nil, err } - for _, nodeGroup := range nodeGroups { - suffix := nodeGroup.Suffix + for _, src := range nodeGroups { // Allow full egress { t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("node-egress%s", suffix)), + Name: s("node-egress" + src.Suffix), Lifecycle: b.Lifecycle, - SecurityGroup: nodeGroup.Task, + SecurityGroup: src.Task, Egress: fi.Bool(true), CIDR: s("0.0.0.0/0"), } @@ -84,12 +83,14 @@ func (b *FirewallModelBuilder) buildNodeRules(c *fi.ModelBuilderContext) ([]Secu } // Nodes can talk to nodes - { + for _, dest := range nodeGroups { + suffix := JoinSuffixes(src, dest) + t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("all-node-to-node%s", suffix)), + Name: s("all-node-to-node" + suffix), Lifecycle: b.Lifecycle, - SecurityGroup: nodeGroup.Task, - SourceGroup: nodeGroup.Task, + SecurityGroup: dest.Task, + SourceGroup: src.Task, } c.AddTask(t) } @@ -99,7 +100,7 @@ func (b *FirewallModelBuilder) buildNodeRules(c *fi.ModelBuilderContext) ([]Secu // Nodes can talk to masters { t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("all-nodes-to-master%s", suffix)), + Name: s(fmt.Sprintf("all-nodes-to-master%s", src.Suffix)), Lifecycle: b.Lifecycle, SecurityGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleMaster), SourceGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleNode), @@ -352,14 +353,13 @@ func (b *FirewallModelBuilder) buildMasterRules(c *fi.ModelBuilderContext, nodeG return nil, err } - for _, masterGroup := range masterGroups { - masterSuffix := masterGroup.Suffix + for _, src := range masterGroups { // Allow full egress { t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("master-egress%s", masterSuffix)), + Name: s("master-egress" + src.Suffix), Lifecycle: b.Lifecycle, - SecurityGroup: masterGroup.Task, + SecurityGroup: src.Task, Egress: fi.Bool(true), CIDR: s("0.0.0.0/0"), } @@ -367,28 +367,29 @@ func (b *FirewallModelBuilder) buildMasterRules(c *fi.ModelBuilderContext, nodeG } // Masters can talk to masters - { + for _, dest := range masterGroups { + suffix := JoinSuffixes(src, dest) + t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("all-master-to-master%s", masterSuffix)), + Name: s("all-master-to-master" + suffix), Lifecycle: b.Lifecycle, - SecurityGroup: masterGroup.Task, - SourceGroup: masterGroup.Task, + SecurityGroup: dest.Task, + SourceGroup: src.Task, } c.AddTask(t) } - for _, nodeGroup := range nodeGroups { - nodeSuffix := masterSuffix + nodeGroup.Suffix - // Masters can talk to nodes - { - t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("all-master-to-node%s", nodeSuffix)), - Lifecycle: b.Lifecycle, - SecurityGroup: nodeGroup.Task, - SourceGroup: masterGroup.Task, - } - c.AddTask(t) + // Masters can talk to nodes + for _, dest := range nodeGroups { + suffix := JoinSuffixes(src, dest) + + t := &awstasks.SecurityGroupRule{ + Name: s("all-master-to-node" + suffix), + Lifecycle: b.Lifecycle, + SecurityGroup: dest.Task, + SourceGroup: src.Task, } + c.AddTask(t) } } @@ -509,3 +510,24 @@ func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lif return groups, nil } + +// JoinSuffixes constructs a suffix for traffic from the src to the dest group +// We have to avoid ambiguity in the case where one has a suffix and the other does not, +// where normally l.Suffix + r.Suffix would equal r.Suffix + l.Suffix +func JoinSuffixes(src SecurityGroupInfo, dest SecurityGroupInfo) string { + if src.Suffix == "" && dest.Suffix == "" { + return "" + } + + s := src.Suffix + if s == "" { + s = "-default" + } + + d := dest.Suffix + if d == "" { + d = "-default" + } + + return s + d +} diff --git a/tests/integration/update_cluster/existing_sg/kubernetes.tf b/tests/integration/update_cluster/existing_sg/kubernetes.tf index f452c6c720..69ab79f3d5 100644 --- a/tests/integration/update_cluster/existing_sg/kubernetes.tf +++ b/tests/integration/update_cluster/existing_sg/kubernetes.tf @@ -560,7 +560,34 @@ resource "aws_security_group_rule" "all-master-to-master" { protocol = "-1" } -resource "aws_security_group_rule" "all-master-to-master-sg-master-1a" { +resource "aws_security_group_rule" "all-master-to-master-default-sg-master-1a" { + type = "ingress" + security_group_id = "sg-master-1a" + source_security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" + from_port = 0 + to_port = 0 + protocol = "-1" +} + +resource "aws_security_group_rule" "all-master-to-master-default-sg-master-1b" { + type = "ingress" + security_group_id = "sg-master-1b" + source_security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" + from_port = 0 + to_port = 0 + protocol = "-1" +} + +resource "aws_security_group_rule" "all-master-to-master-sg-master-1a-default" { + type = "ingress" + security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" + source_security_group_id = "sg-master-1a" + from_port = 0 + to_port = 0 + protocol = "-1" +} + +resource "aws_security_group_rule" "all-master-to-master-sg-master-1a-sg-master-1a" { type = "ingress" security_group_id = "sg-master-1a" source_security_group_id = "sg-master-1a" @@ -569,7 +596,34 @@ resource "aws_security_group_rule" "all-master-to-master-sg-master-1a" { protocol = "-1" } -resource "aws_security_group_rule" "all-master-to-master-sg-master-1b" { +resource "aws_security_group_rule" "all-master-to-master-sg-master-1a-sg-master-1b" { + type = "ingress" + security_group_id = "sg-master-1b" + source_security_group_id = "sg-master-1a" + from_port = 0 + to_port = 0 + protocol = "-1" +} + +resource "aws_security_group_rule" "all-master-to-master-sg-master-1b-default" { + type = "ingress" + security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" + source_security_group_id = "sg-master-1b" + from_port = 0 + to_port = 0 + protocol = "-1" +} + +resource "aws_security_group_rule" "all-master-to-master-sg-master-1b-sg-master-1a" { + type = "ingress" + security_group_id = "sg-master-1a" + source_security_group_id = "sg-master-1b" + from_port = 0 + to_port = 0 + protocol = "-1" +} + +resource "aws_security_group_rule" "all-master-to-master-sg-master-1b-sg-master-1b" { type = "ingress" security_group_id = "sg-master-1b" source_security_group_id = "sg-master-1b" @@ -578,6 +632,15 @@ resource "aws_security_group_rule" "all-master-to-master-sg-master-1b" { protocol = "-1" } +resource "aws_security_group_rule" "all-master-to-node-default-sg-nodes" { + type = "ingress" + security_group_id = "sg-nodes" + source_security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" + from_port = 0 + to_port = 0 + protocol = "-1" +} + resource "aws_security_group_rule" "all-master-to-node-sg-master-1a-sg-nodes" { type = "ingress" security_group_id = "sg-nodes" @@ -596,16 +659,7 @@ resource "aws_security_group_rule" "all-master-to-node-sg-master-1b-sg-nodes" { protocol = "-1" } -resource "aws_security_group_rule" "all-master-to-node-sg-nodes" { - type = "ingress" - security_group_id = "sg-nodes" - source_security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" - from_port = 0 - to_port = 0 - protocol = "-1" -} - -resource "aws_security_group_rule" "all-node-to-node-sg-nodes" { +resource "aws_security_group_rule" "all-node-to-node-sg-nodes-sg-nodes" { type = "ingress" security_group_id = "sg-nodes" source_security_group_id = "sg-nodes" From 1e2a62992b91b0329ab4c383a19d9c9b9e2fc30e Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Tue, 2 Oct 2018 11:16:19 -0700 Subject: [PATCH 5/8] Use JoinSuffixes for node->master traffic, also fix AmazonVPC rule This ensures we are consistently naming our rules --- pkg/model/firewall.go | 52 +++++------ .../update_cluster/existing_sg/kubernetes.tf | 88 +++++++++---------- 2 files changed, 71 insertions(+), 69 deletions(-) diff --git a/pkg/model/firewall.go b/pkg/model/firewall.go index 3c984c17f4..7baa1e6cf5 100644 --- a/pkg/model/firewall.go +++ b/pkg/model/firewall.go @@ -95,24 +95,14 @@ func (b *FirewallModelBuilder) buildNodeRules(c *fi.ModelBuilderContext) ([]Secu c.AddTask(t) } - // Pods running in Nodes could need to reach pods in master/s - if b.Cluster.Spec.Networking != nil && b.Cluster.Spec.Networking.AmazonVPC != nil { - // Nodes can talk to masters - { - t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("all-nodes-to-master%s", src.Suffix)), - Lifecycle: b.Lifecycle, - SecurityGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleMaster), - SourceGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleNode), - } - c.AddTask(t) - } - } } return nodeGroups, nil } +/* +This is dead code, but hopefully one day we can open specific ports only, for better security + func (b *FirewallModelBuilder) applyNodeToMasterAllowSpecificPorts(c *fi.ModelBuilderContext) { // TODO: We need to remove the ALL rule //W1229 12:32:22.300132 9003 executor.go:109] error running task "SecurityGroupRule/node-to-master-443" (9m58s remaining to succeed): error creating SecurityGroupIngress: InvalidPermission.Duplicate: the specified rule "peer: sg-f6b1a68b, ALL, ALLOW" already exists @@ -226,6 +216,7 @@ func (b *FirewallModelBuilder) applyNodeToMasterAllowSpecificPorts(c *fi.ModelBu c.AddTask(t) } } +*/ func (b *FirewallModelBuilder) applyNodeToMasterBlockSpecificPorts(c *fi.ModelBuilderContext, nodeGroups []SecurityGroupInfo, masterGroups []SecurityGroupInfo) { type portRange struct { @@ -289,20 +280,12 @@ func (b *FirewallModelBuilder) applyNodeToMasterBlockSpecificPorts(c *fi.ModelBu } for _, masterGroup := range masterGroups { - masterSuffix := masterGroup.Suffix - var nodeSuffix string - for _, nodeGroup := range nodeGroups { - - if masterSuffix == "" && nodeGroup.Suffix == "" { - nodeSuffix = "" - } else { - nodeSuffix = fmt.Sprintf("%s-%s", masterSuffix, nodeGroup.Name) - } + suffix := JoinSuffixes(nodeGroup, masterGroup) for _, r := range udpRanges { t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("node-to-master-udp-%d-%d%s", r.From, r.To, nodeSuffix)), + Name: s(fmt.Sprintf("node-to-master-udp-%d-%d%s", r.From, r.To, suffix)), Lifecycle: b.Lifecycle, SecurityGroup: masterGroup.Task, SourceGroup: nodeGroup.Task, @@ -314,7 +297,7 @@ func (b *FirewallModelBuilder) applyNodeToMasterBlockSpecificPorts(c *fi.ModelBu } for _, r := range tcpRanges { t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("node-to-master-tcp-%d-%d%s", r.From, r.To, nodeSuffix)), + Name: s(fmt.Sprintf("node-to-master-tcp-%d-%d%s", r.From, r.To, suffix)), Lifecycle: b.Lifecycle, SecurityGroup: masterGroup.Task, SourceGroup: nodeGroup.Task, @@ -335,7 +318,7 @@ func (b *FirewallModelBuilder) applyNodeToMasterBlockSpecificPorts(c *fi.ModelBu } t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("node-to-master-protocol-%s%s", name, nodeSuffix)), + Name: s(fmt.Sprintf("node-to-master-protocol-%s%s", name, suffix)), Lifecycle: b.Lifecycle, SecurityGroup: masterGroup.Task, SourceGroup: nodeGroup.Task, @@ -345,6 +328,25 @@ func (b *FirewallModelBuilder) applyNodeToMasterBlockSpecificPorts(c *fi.ModelBu } } } + + // For AmazonVPC networking, pods running in Nodes could need to reach pods in master/s + if b.Cluster.Spec.Networking != nil && b.Cluster.Spec.Networking.AmazonVPC != nil { + // Nodes can talk to masters + for _, src := range nodeGroups { + for _, dest := range masterGroups { + suffix := JoinSuffixes(src, dest) + + t := &awstasks.SecurityGroupRule{ + Name: s("all-nodes-to-master" + suffix), + Lifecycle: b.Lifecycle, + SecurityGroup: dest.Task, + SourceGroup: src.Task, + } + c.AddTask(t) + } + } + } + } func (b *FirewallModelBuilder) buildMasterRules(c *fi.ModelBuilderContext, nodeGroups []SecurityGroupInfo) ([]SecurityGroupInfo, error) { diff --git a/tests/integration/update_cluster/existing_sg/kubernetes.tf b/tests/integration/update_cluster/existing_sg/kubernetes.tf index 69ab79f3d5..c63adc5ebe 100644 --- a/tests/integration/update_cluster/existing_sg/kubernetes.tf +++ b/tests/integration/update_cluster/existing_sg/kubernetes.tf @@ -749,25 +749,7 @@ resource "aws_security_group_rule" "node-egress-sg-nodes" { cidr_blocks = ["0.0.0.0/0"] } -resource "aws_security_group_rule" "node-to-master-tcp-1-2379-sg-master-1a-sg-nodes" { - type = "ingress" - security_group_id = "sg-master-1a" - source_security_group_id = "sg-nodes" - from_port = 1 - to_port = 2379 - protocol = "tcp" -} - -resource "aws_security_group_rule" "node-to-master-tcp-1-2379-sg-master-1b-sg-nodes" { - type = "ingress" - security_group_id = "sg-master-1b" - source_security_group_id = "sg-nodes" - from_port = 1 - to_port = 2379 - protocol = "tcp" -} - -resource "aws_security_group_rule" "node-to-master-tcp-1-2379-sg-nodes" { +resource "aws_security_group_rule" "node-to-master-tcp-1-2379-sg-nodes-default" { type = "ingress" security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" source_security_group_id = "sg-nodes" @@ -776,25 +758,25 @@ resource "aws_security_group_rule" "node-to-master-tcp-1-2379-sg-nodes" { protocol = "tcp" } -resource "aws_security_group_rule" "node-to-master-tcp-2382-4000-sg-master-1a-sg-nodes" { +resource "aws_security_group_rule" "node-to-master-tcp-1-2379-sg-nodes-sg-master-1a" { type = "ingress" security_group_id = "sg-master-1a" source_security_group_id = "sg-nodes" - from_port = 2382 - to_port = 4000 + from_port = 1 + to_port = 2379 protocol = "tcp" } -resource "aws_security_group_rule" "node-to-master-tcp-2382-4000-sg-master-1b-sg-nodes" { +resource "aws_security_group_rule" "node-to-master-tcp-1-2379-sg-nodes-sg-master-1b" { type = "ingress" security_group_id = "sg-master-1b" source_security_group_id = "sg-nodes" - from_port = 2382 - to_port = 4000 + from_port = 1 + to_port = 2379 protocol = "tcp" } -resource "aws_security_group_rule" "node-to-master-tcp-2382-4000-sg-nodes" { +resource "aws_security_group_rule" "node-to-master-tcp-2382-4000-sg-nodes-default" { type = "ingress" security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" source_security_group_id = "sg-nodes" @@ -803,25 +785,25 @@ resource "aws_security_group_rule" "node-to-master-tcp-2382-4000-sg-nodes" { protocol = "tcp" } -resource "aws_security_group_rule" "node-to-master-tcp-4003-65535-sg-master-1a-sg-nodes" { +resource "aws_security_group_rule" "node-to-master-tcp-2382-4000-sg-nodes-sg-master-1a" { type = "ingress" security_group_id = "sg-master-1a" source_security_group_id = "sg-nodes" - from_port = 4003 - to_port = 65535 + from_port = 2382 + to_port = 4000 protocol = "tcp" } -resource "aws_security_group_rule" "node-to-master-tcp-4003-65535-sg-master-1b-sg-nodes" { +resource "aws_security_group_rule" "node-to-master-tcp-2382-4000-sg-nodes-sg-master-1b" { type = "ingress" security_group_id = "sg-master-1b" source_security_group_id = "sg-nodes" - from_port = 4003 - to_port = 65535 + from_port = 2382 + to_port = 4000 protocol = "tcp" } -resource "aws_security_group_rule" "node-to-master-tcp-4003-65535-sg-nodes" { +resource "aws_security_group_rule" "node-to-master-tcp-4003-65535-sg-nodes-default" { type = "ingress" security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" source_security_group_id = "sg-nodes" @@ -830,7 +812,34 @@ resource "aws_security_group_rule" "node-to-master-tcp-4003-65535-sg-nodes" { protocol = "tcp" } -resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-master-1a-sg-nodes" { +resource "aws_security_group_rule" "node-to-master-tcp-4003-65535-sg-nodes-sg-master-1a" { + type = "ingress" + security_group_id = "sg-master-1a" + source_security_group_id = "sg-nodes" + from_port = 4003 + to_port = 65535 + protocol = "tcp" +} + +resource "aws_security_group_rule" "node-to-master-tcp-4003-65535-sg-nodes-sg-master-1b" { + type = "ingress" + security_group_id = "sg-master-1b" + source_security_group_id = "sg-nodes" + from_port = 4003 + to_port = 65535 + protocol = "tcp" +} + +resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-nodes-default" { + type = "ingress" + security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" + source_security_group_id = "sg-nodes" + from_port = 1 + to_port = 65535 + protocol = "udp" +} + +resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-nodes-sg-master-1a" { type = "ingress" security_group_id = "sg-master-1a" source_security_group_id = "sg-nodes" @@ -839,7 +848,7 @@ resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-master-1a-sg-n protocol = "udp" } -resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-master-1b-sg-nodes" { +resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-nodes-sg-master-1b" { type = "ingress" security_group_id = "sg-master-1b" source_security_group_id = "sg-nodes" @@ -848,15 +857,6 @@ resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-master-1b-sg-n protocol = "udp" } -resource "aws_security_group_rule" "node-to-master-udp-1-65535-sg-nodes" { - type = "ingress" - security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" - source_security_group_id = "sg-nodes" - from_port = 1 - to_port = 65535 - protocol = "udp" -} - resource "aws_security_group_rule" "ssh-external-to-master-0-0-0-0--0" { type = "ingress" security_group_id = "${aws_security_group.masters-existingsg-example-com.id}" From 9a6653421c232eb3bb60d1d5d45d8ae2d2e8a5f7 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Tue, 2 Oct 2018 11:28:28 -0700 Subject: [PATCH 6/8] Support override security groups with bastion --- pkg/model/bastion.go | 105 ++++++++++++++++++++++-------------------- pkg/model/firewall.go | 32 ++++++------- 2 files changed, 68 insertions(+), 69 deletions(-) diff --git a/pkg/model/bastion.go b/pkg/model/bastion.go index f5a0b86f1a..47f75305b3 100644 --- a/pkg/model/bastion.go +++ b/pkg/model/bastion.go @@ -43,38 +43,42 @@ type BastionModelBuilder struct { var _ fi.ModelBuilder = &BastionModelBuilder{} func (b *BastionModelBuilder) Build(c *fi.ModelBuilderContext) error { - var bastionGroups []*kops.InstanceGroup + var bastionInstanceGroups []*kops.InstanceGroup for _, ig := range b.InstanceGroups { if ig.Spec.Role == kops.InstanceGroupRoleBastion { - bastionGroups = append(bastionGroups, ig) + bastionInstanceGroups = append(bastionInstanceGroups, ig) } } - if len(bastionGroups) == 0 { + if len(bastionInstanceGroups) == 0 { return nil } - // Create security group for bastion instances - { - t := &awstasks.SecurityGroup{ - Name: s(b.SecurityGroupName(kops.InstanceGroupRoleBastion)), - Lifecycle: b.SecurityLifecycle, - - VPC: b.LinkToVPC(), - Description: s("Security group for bastion"), - RemoveExtraRules: []string{"port=22"}, - } - t.Tags = b.CloudTags(*t.Name, false) - c.AddTask(t) + bastionGroups, err := b.GetSecurityGroups(kops.InstanceGroupRoleBastion) + if err != nil { + return err + } + nodeGroups, err := b.GetSecurityGroups(kops.InstanceGroupRoleNode) + if err != nil { + return err + } + masterGroups, err := b.GetSecurityGroups(kops.InstanceGroupRoleMaster) + if err != nil { + return err } - // Allow traffic from bastion instances to egress freely - { - t := &awstasks.SecurityGroupRule{ - Name: s("bastion-egress"), - Lifecycle: b.SecurityLifecycle, + // Create security group for bastion instances + for _, bastionGroup := range bastionGroups { + bastionGroup.Task.Lifecycle = b.SecurityLifecycle + c.AddTask(bastionGroup.Task) + } - SecurityGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleBastion), + for _, src := range bastionGroups { + // Allow traffic from bastion instances to egress freely + t := &awstasks.SecurityGroupRule{ + Name: s("bastion-egress" + src.Suffix), + Lifecycle: b.SecurityLifecycle, + SecurityGroup: src.Task, Egress: fi.Bool(true), CIDR: s("0.0.0.0/0"), } @@ -83,12 +87,11 @@ func (b *BastionModelBuilder) Build(c *fi.ModelBuilderContext) error { // Allow incoming SSH traffic to bastions, through the ELB // TODO: Could we get away without an ELB here? Tricky to fix if dns-controller breaks though... - { + for _, dest := range bastionGroups { t := &awstasks.SecurityGroupRule{ - Name: s("ssh-elb-to-bastion"), - Lifecycle: b.SecurityLifecycle, - - SecurityGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleBastion), + Name: s("ssh-elb-to-bastion" + dest.Suffix), + Lifecycle: b.SecurityLifecycle, + SecurityGroup: dest.Task, SourceGroup: b.LinkToELBSecurityGroup(BastionELBSecurityGroupPrefix), Protocol: s("tcp"), FromPort: i64(22), @@ -98,33 +101,35 @@ func (b *BastionModelBuilder) Build(c *fi.ModelBuilderContext) error { } // Allow bastion nodes to SSH to masters - { - t := &awstasks.SecurityGroupRule{ - Name: s("bastion-to-master-ssh"), - Lifecycle: b.SecurityLifecycle, - - SecurityGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleMaster), - SourceGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleBastion), - Protocol: s("tcp"), - FromPort: i64(22), - ToPort: i64(22), + for _, src := range bastionGroups { + for _, dest := range masterGroups { + t := &awstasks.SecurityGroupRule{ + Name: s("bastion-to-master-ssh" + JoinSuffixes(src, dest)), + Lifecycle: b.SecurityLifecycle, + SecurityGroup: dest.Task, + SourceGroup: src.Task, + Protocol: s("tcp"), + FromPort: i64(22), + ToPort: i64(22), + } + c.AddTask(t) } - c.AddTask(t) } // Allow bastion nodes to SSH to nodes - { - t := &awstasks.SecurityGroupRule{ - Name: s("bastion-to-node-ssh"), - Lifecycle: b.SecurityLifecycle, - - SecurityGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleNode), - SourceGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleBastion), - Protocol: s("tcp"), - FromPort: i64(22), - ToPort: i64(22), + for _, src := range bastionGroups { + for _, dest := range nodeGroups { + t := &awstasks.SecurityGroupRule{ + Name: s("bastion-to-node-ssh" + JoinSuffixes(src, dest)), + Lifecycle: b.SecurityLifecycle, + SecurityGroup: dest.Task, + SourceGroup: src.Task, + Protocol: s("tcp"), + FromPort: i64(22), + ToPort: i64(22), + } + c.AddTask(t) } - c.AddTask(t) } // Create security group for bastion ELB @@ -173,7 +178,7 @@ func (b *BastionModelBuilder) Build(c *fi.ModelBuilderContext) error { var elbSubnets []*awstasks.Subnet { zones := sets.NewString() - for _, ig := range bastionGroups { + for _, ig := range bastionInstanceGroups { subnets, err := b.GatherSubnets(ig) if err != nil { return err @@ -231,7 +236,7 @@ func (b *BastionModelBuilder) Build(c *fi.ModelBuilderContext) error { c.AddTask(elb) } - for _, ig := range bastionGroups { + for _, ig := range bastionInstanceGroups { // We build the ASG when we iterate over the instance groups // Attach the ELB to the ASG diff --git a/pkg/model/firewall.go b/pkg/model/firewall.go index 7baa1e6cf5..608c86587d 100644 --- a/pkg/model/firewall.go +++ b/pkg/model/firewall.go @@ -411,7 +411,7 @@ type SecurityGroupInfo struct { func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lifecycle *fi.Lifecycle, c *fi.ModelBuilderContext) ([]SecurityGroupInfo, error) { var baseGroup *awstasks.SecurityGroup if role == kops.InstanceGroupRoleMaster { - name := "masters." + b.ClusterName() + name := b.SecurityGroupName(role) baseGroup = &awstasks.SecurityGroup{ Name: s(name), Lifecycle: lifecycle, @@ -431,9 +431,9 @@ func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lif // TODO: Protocol 4 for calico }, } - baseGroup.Tags = b.CloudTags(*baseGroup.Name, false) + baseGroup.Tags = b.CloudTags(name, false) } else if role == kops.InstanceGroupRoleNode { - name := "nodes." + b.ClusterName() + name := b.SecurityGroupName(role) baseGroup = &awstasks.SecurityGroup{ Name: s(name), Lifecycle: lifecycle, @@ -441,23 +441,17 @@ func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lif Description: s("Security group for nodes"), RemoveExtraRules: []string{"port=22"}, } - baseGroup.Tags = b.CloudTags(*baseGroup.Name, false) + baseGroup.Tags = b.CloudTags(name, false) } else if role == kops.InstanceGroupRoleBastion { - return nil, fmt.Errorf("bastion are not supported yet with instancegroup securitygroup") - /* - // TODO use this instead of the hardcoded names?? - // b.SecurityGroupName(kops.InstanceGroupRoleBastion)) - // TODO implement - name := "bastion." + b.ClusterName() - baseGroup = &awstasks.SecurityGroup{ - Name: s(name), - Lifecycle: lifecycle, - VPC: b.LinkToVPC(), - Description: s("Security group for bastion"), - RemoveExtraRules: []string{"port=22"}, - } - baseGroup.Tags = b.CloudTags(*baseGroup.Name, false) - */ + name := b.SecurityGroupName(role) + baseGroup = &awstasks.SecurityGroup{ + Name: s(name), + Lifecycle: lifecycle, + VPC: b.LinkToVPC(), + Description: s("Security group for bastion"), + RemoveExtraRules: []string{"port=22"}, + } + baseGroup.Tags = b.CloudTags(name, false) } else { return nil, fmt.Errorf("not a supported security group type") } From 81cadec4cafda03971c180f078e560514e68e03a Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Tue, 2 Oct 2018 11:42:27 -0700 Subject: [PATCH 7/8] Simplify building of security groups Also add comments about why we don't set e.g. RemoveExtraRules --- pkg/model/firewall.go | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/pkg/model/firewall.go b/pkg/model/firewall.go index 608c86587d..8995dfcecf 100644 --- a/pkg/model/firewall.go +++ b/pkg/model/firewall.go @@ -64,11 +64,16 @@ func (b *FirewallModelBuilder) Build(c *fi.ModelBuilderContext) error { func (b *FirewallModelBuilder) buildNodeRules(c *fi.ModelBuilderContext) ([]SecurityGroupInfo, error) { - nodeGroups, err := b.createSecurityGroups(kops.InstanceGroupRoleNode, b.Lifecycle, c) + nodeGroups, err := b.GetSecurityGroups(kops.InstanceGroupRoleNode) if err != nil { return nil, err } + for _, group := range nodeGroups { + group.Task.Lifecycle = b.Lifecycle + c.AddTask(group.Task) + } + for _, src := range nodeGroups { // Allow full egress { @@ -350,11 +355,16 @@ func (b *FirewallModelBuilder) applyNodeToMasterBlockSpecificPorts(c *fi.ModelBu } func (b *FirewallModelBuilder) buildMasterRules(c *fi.ModelBuilderContext, nodeGroups []SecurityGroupInfo) ([]SecurityGroupInfo, error) { - masterGroups, err := b.createSecurityGroups(kops.InstanceGroupRoleMaster, b.Lifecycle, c) + masterGroups, err := b.GetSecurityGroups(kops.InstanceGroupRoleMaster) if err != nil { return nil, err } + for _, group := range masterGroups { + group.Task.Lifecycle = b.Lifecycle + c.AddTask(group.Task) + } + for _, src := range masterGroups { // Allow full egress { @@ -398,23 +408,18 @@ func (b *FirewallModelBuilder) buildMasterRules(c *fi.ModelBuilderContext, nodeG return masterGroups, nil } -func (b *KopsModelContext) GetSecurityGroups(role kops.InstanceGroupRole) ([]SecurityGroupInfo, error) { - return b.createSecurityGroups(role, nil, nil) -} - type SecurityGroupInfo struct { Name string Suffix string Task *awstasks.SecurityGroup } -func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lifecycle *fi.Lifecycle, c *fi.ModelBuilderContext) ([]SecurityGroupInfo, error) { +func (b *KopsModelContext) GetSecurityGroups(role kops.InstanceGroupRole) ([]SecurityGroupInfo, error) { var baseGroup *awstasks.SecurityGroup if role == kops.InstanceGroupRoleMaster { name := b.SecurityGroupName(role) baseGroup = &awstasks.SecurityGroup{ Name: s(name), - Lifecycle: lifecycle, VPC: b.LinkToVPC(), Description: s("Security group for masters"), RemoveExtraRules: []string{ @@ -436,7 +441,6 @@ func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lif name := b.SecurityGroupName(role) baseGroup = &awstasks.SecurityGroup{ Name: s(name), - Lifecycle: lifecycle, VPC: b.LinkToVPC(), Description: s("Security group for nodes"), RemoveExtraRules: []string{"port=22"}, @@ -446,7 +450,6 @@ func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lif name := b.SecurityGroupName(role) baseGroup = &awstasks.SecurityGroup{ Name: s(name), - Lifecycle: lifecycle, VPC: b.LinkToVPC(), Description: s("Security group for bastion"), RemoveExtraRules: []string{"port=22"}, @@ -474,11 +477,12 @@ func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lif t := &awstasks.SecurityGroup{ Name: ig.Spec.SecurityGroupOverride, ID: ig.Spec.SecurityGroupOverride, - Lifecycle: lifecycle, VPC: b.LinkToVPC(), Shared: fi.Bool(true), Description: baseGroup.Description, } + // Because the SecurityGroup is shared, we don't set RemoveExtraRules + // This does mean we don't check them. We might want to revisit this in future. suffix := "-" + name @@ -487,9 +491,9 @@ func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lif Suffix: suffix, Task: t, }) - } + // Add the default SecurityGroup, if any InstanceGroups are using the default if !allOverrides { groups = append(groups, SecurityGroupInfo{ Name: fi.StringValue(baseGroup.Name), @@ -497,13 +501,6 @@ func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lif }) } - for _, group := range groups { - if c != nil { - glog.V(8).Infof("adding security group: %q", group.Name) - c.AddTask(group.Task) - } - } - return groups, nil } From 789b7c9f28451eb0bdadf55a88c5edf61e3fa28f Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Tue, 2 Oct 2018 12:46:55 -0700 Subject: [PATCH 8/8] Remove duplicate security-group overrides --- pkg/model/firewall.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/model/firewall.go b/pkg/model/firewall.go index 8995dfcecf..41e23f7917 100644 --- a/pkg/model/firewall.go +++ b/pkg/model/firewall.go @@ -461,6 +461,8 @@ func (b *KopsModelContext) GetSecurityGroups(role kops.InstanceGroupRole) ([]Sec var groups []SecurityGroupInfo + done := make(map[string]bool) + // Build groups that specify a SecurityGroupOverride allOverrides := true for _, ig := range b.InstanceGroups { @@ -474,6 +476,13 @@ func (b *KopsModelContext) GetSecurityGroups(role kops.InstanceGroupRole) ([]Sec } name := fi.StringValue(ig.Spec.SecurityGroupOverride) + + // De-duplicate security groups + if done[name] { + continue + } + done[name] = true + t := &awstasks.SecurityGroup{ Name: ig.Spec.SecurityGroupOverride, ID: ig.Spec.SecurityGroupOverride,