Merge pull request #5862 from justinsb/follow_on_5744

Follow on for #5744

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)
 }
 

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 {
+		for _, masterGroup := range masterGroups {
-			suffix := GetGroupSuffix(masterGroupName, 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 ""
-}

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
+	bastionGroups, err := b.GetSecurityGroups(kops.InstanceGroupRoleBastion)
-	{
+	if err != nil {
-		t := &awstasks.SecurityGroup{
+		return err
-			Name:      s(b.SecurityGroupName(kops.InstanceGroupRoleBastion)),
+	}
-			Lifecycle: b.SecurityLifecycle,
+	nodeGroups, err := b.GetSecurityGroups(kops.InstanceGroupRoleNode)
-
+	if err != nil {
-			VPC:              b.LinkToVPC(),
+		return err
-			Description:      s("Security group for bastion"),
+	}
-			RemoveExtraRules: []string{"port=22"},
+	masterGroups, err := b.GetSecurityGroups(kops.InstanceGroupRoleMaster)
-		}
+	if err != nil {
-		t.Tags = b.CloudTags(*t.Name, false)
+		return err
-		c.AddTask(t)
 	}
 
-	// Allow traffic from bastion instances to egress freely
+	// Create security group for bastion instances
-	{
+	for _, bastionGroup := range bastionGroups {
-		t := &awstasks.SecurityGroupRule{
+		bastionGroup.Task.Lifecycle = b.SecurityLifecycle
-			Name:      s("bastion-egress"),
+		c.AddTask(bastionGroup.Task)
-			Lifecycle: b.SecurityLifecycle,
+	}
 
-			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"),
+			Name:          s("ssh-elb-to-bastion" + dest.Suffix),
-			Lifecycle: b.SecurityLifecycle,
+			Lifecycle:     b.SecurityLifecycle,
-
+			SecurityGroup: dest.Task,
-			SecurityGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleBastion),
 			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
-	{
+	for _, src := range bastionGroups {
-		t := &awstasks.SecurityGroupRule{
+		for _, dest := range masterGroups {
-			Name:      s("bastion-to-master-ssh"),
+			t := &awstasks.SecurityGroupRule{
-			Lifecycle: b.SecurityLifecycle,
+				Name:          s("bastion-to-master-ssh" + JoinSuffixes(src, dest)),
-
+				Lifecycle:     b.SecurityLifecycle,
-			SecurityGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleMaster),
+				SecurityGroup: dest.Task,
-			SourceGroup:   b.LinkToSecurityGroup(kops.InstanceGroupRoleBastion),
+				SourceGroup:   src.Task,
-			Protocol:      s("tcp"),
+				Protocol:      s("tcp"),
-			FromPort:      i64(22),
+				FromPort:      i64(22),
-			ToPort:        i64(22),
+				ToPort:        i64(22),
+			}
+			c.AddTask(t)
 		}
-		c.AddTask(t)
 	}
 
 	// Allow bastion nodes to SSH to nodes
-	{
+	for _, src := range bastionGroups {
-		t := &awstasks.SecurityGroupRule{
+		for _, dest := range nodeGroups {
-			Name:      s("bastion-to-node-ssh"),
+			t := &awstasks.SecurityGroupRule{
-			Lifecycle: b.SecurityLifecycle,
+				Name:          s("bastion-to-node-ssh" + JoinSuffixes(src, dest)),
-
+				Lifecycle:     b.SecurityLifecycle,
-			SecurityGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleNode),
+				SecurityGroup: dest.Task,
-			SourceGroup:   b.LinkToSecurityGroup(kops.InstanceGroupRoleBastion),
+				SourceGroup:   src.Task,
-			Protocol:      s("tcp"),
+				Protocol:      s("tcp"),
-			FromPort:      i64(22),
+				FromPort:      i64(22),
-			ToPort:        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

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 {
+			for _, masterGroup := range masterGroups {
-				suffix := GetGroupSuffix(masterGroupName, 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 {
+			for _, nodeGroup := range nodeGroups {
-				suffix := GetGroupSuffix(nodeGroupName, 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 {
+		for _, nodeGroup := range nodeGroups {
-			suffix := GetGroupSuffix(nodeGroupName, 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 {
+			for _, masterGroup := range masterGroups {
-				suffix := GetGroupSuffix(masterGroupName, 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),

pkg/model/firewall.go

@@ -62,21 +62,25 @@ 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)
+	nodeGroups, err := b.GetSecurityGroups(kops.InstanceGroupRoleNode)
 	if err != nil {
 		return nil, err
 	}
 
-	for nodeGroupName, secGroup := range nodeGroups {
+	for _, group := range nodeGroups {
-		suffix := GetGroupSuffix(nodeGroupName, nodeGroups)
+		group.Task.Lifecycle = b.Lifecycle
+		c.AddTask(group.Task)
+	}
+
+	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: secGroup,
+				SecurityGroup: src.Task,
 				Egress:        fi.Bool(true),
 				CIDR:          s("0.0.0.0/0"),
 			}
@@ -84,34 +88,26 @@ func (b *FirewallModelBuilder) buildNodeRules(c *fi.ModelBuilderContext) (map[st
 		}
 
 		// 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: secGroup,
+				SecurityGroup: dest.Task,
-				SourceGroup:   secGroup,
+				SourceGroup:   src.Task,
 			}
 			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", 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
@@ -225,8 +221,9 @@ func (b *FirewallModelBuilder) applyNodeToMasterAllowSpecificPorts(c *fi.ModelBu
 		c.AddTask(t)
 	}
 }
+*/
 
-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 +284,16 @@ func (b *FirewallModelBuilder) applyNodeToMasterBlockSpecificPorts(c *fi.ModelBu
 		}
 	}
 
-	for masterSecGroupName, masterGroup := range masterGroups {
+	for _, masterGroup := range masterGroups {
-		var masterSuffix string
+		for _, nodeGroup := range nodeGroups {
-		var nodeSuffix string
+			suffix := JoinSuffixes(nodeGroup, masterGroup)
-
-		if len(masterGroups) != 1 {
-			masterSuffix = "-" + masterSecGroupName
-		} else {
-			masterSuffix = ""
-		}
-
-		for nodeSecGroupName, nodeGroup := range nodeGroups {
-
-			if len(masterGroups) == 1 && len(nodeGroups) == 1 {
-				nodeSuffix = ""
-			} else {
-				nodeSuffix = fmt.Sprintf("%s-%s", masterSuffix, nodeSecGroupName)
-			}
 
 			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,
+					SecurityGroup: masterGroup.Task,
-					SourceGroup:   nodeGroup,
+					SourceGroup:   nodeGroup.Task,
 					FromPort:      i64(int64(r.From)),
 					ToPort:        i64(int64(r.To)),
 					Protocol:      s("udp"),
@@ -319,10 +302,10 @@ 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,
+					SecurityGroup: masterGroup.Task,
-					SourceGroup:   nodeGroup,
+					SourceGroup:   nodeGroup.Task,
 					FromPort:      i64(int64(r.From)),
 					ToPort:        i64(int64(r.To)),
 					Protocol:      s("tcp"),
@@ -340,32 +323,55 @@ 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,
+					SecurityGroup: masterGroup.Task,
-					SourceGroup:   nodeGroup,
+					SourceGroup:   nodeGroup.Task,
 					Protocol:      s(awsName),
 				}
 				c.AddTask(t)
 			}
 		}
 	}
+
+	// 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 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)
+	masterGroups, err := b.GetSecurityGroups(kops.InstanceGroupRoleMaster)
 	if err != nil {
 		return nil, err
 	}
 
-	for masterSecGroupName, masterGroup := range masterGroups {
+	for _, group := range masterGroups {
-		suffix := GetGroupSuffix(masterSecGroupName, masterGroups)
+		group.Task.Lifecycle = b.Lifecycle
+		c.AddTask(group.Task)
+	}
+
+	for _, src := range masterGroups {
 		// Allow full egress
 		{
 			t := &awstasks.SecurityGroupRule{
-				Name:          s(fmt.Sprintf("master-egress%s", suffix)),
+				Name:          s("master-egress" + src.Suffix),
 				Lifecycle:     b.Lifecycle,
-				SecurityGroup: masterGroup,
+				SecurityGroup: src.Task,
 				Egress:        fi.Bool(true),
 				CIDR:          s("0.0.0.0/0"),
 			}
@@ -373,51 +379,47 @@ 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", suffix)),
+				Name:          s("all-master-to-master" + suffix),
 				Lifecycle:     b.Lifecycle,
-				SecurityGroup: masterGroup,
+				SecurityGroup: dest.Task,
-				SourceGroup:   masterGroup,
+				SourceGroup:   src.Task,
 			}
 			c.AddTask(t)
 		}
-		for nodeSecGroupName, nodeGroup := range nodeGroups {
 
-			if len(masterGroups) == 1 && len(nodeGroups) == 1 {
+		// Masters can talk to nodes
-				nodeSecGroupName = ""
+		for _, dest := range nodeGroups {
-			} else {
+			suffix := JoinSuffixes(src, dest)
-				nodeSecGroupName = fmt.Sprintf("%s-%s", masterSecGroupName, nodeSecGroupName)
-			}
 
-			// Masters can talk to nodes
+			t := &awstasks.SecurityGroupRule{
-			{
+				Name:          s("all-master-to-node" + suffix),
-				t := &awstasks.SecurityGroupRule{
+				Lifecycle:     b.Lifecycle,
-					Name:          s(fmt.Sprintf("all-master-to-node%s", nodeSecGroupName)),
+				SecurityGroup: dest.Task,
-					Lifecycle:     b.Lifecycle,
+				SourceGroup:   src.Task,
-					SecurityGroup: nodeGroup,
-					SourceGroup:   masterGroup,
-				}
-				c.AddTask(t)
 			}
+			c.AddTask(t)
 		}
 	}
 
 	return masterGroups, nil
 }
 
-func (b *KopsModelContext) GetSecurityGroups(role kops.InstanceGroupRole) (map[string]*awstasks.SecurityGroup, error) {
+type SecurityGroupInfo struct {
-	return b.createSecurityGroups(role, nil, nil)
+	Name   string
+	Suffix string
+	Task   *awstasks.SecurityGroup
 }
 
-func (b *KopsModelContext) createSecurityGroups(role kops.InstanceGroupRole, lifecycle *fi.Lifecycle, c *fi.ModelBuilderContext) (map[string]*awstasks.SecurityGroup, error) {
+func (b *KopsModelContext) GetSecurityGroups(role kops.InstanceGroupRole) ([]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,
 			VPC:         b.LinkToVPC(),
 			Description: s("Security group for masters"),
 			RemoveExtraRules: []string{
@@ -434,77 +436,100 @@ 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,
 			VPC:              b.LinkToVPC(),
 			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")
+		name := b.SecurityGroupName(role)
-		/*
+		baseGroup = &awstasks.SecurityGroup{
-			// TODO use this instead of the hardcoded names??
+			Name:             s(name),
-			// b.SecurityGroupName(kops.InstanceGroupRoleBastion))
+			VPC:              b.LinkToVPC(),
-			// TODO implement
+			Description:      s("Security group for bastion"),
-			name := "bastion." + b.ClusterName()
+			RemoveExtraRules: []string{"port=22"},
-			baseGroup = &awstasks.SecurityGroup{
+		}
-				Name:             s(name),
+		baseGroup.Tags = b.CloudTags(name, false)
-				Lifecycle:        lifecycle,
-				VPC:              b.LinkToVPC(),
-				Description:      s("Security group for bastion"),
-				RemoveExtraRules: []string{"port=22"},
-			}
-			baseGroup.Tags = b.CloudTags(*baseGroup.Name, false)
-		*/
 	} else {
 		return nil, fmt.Errorf("not a supported security group type")
 	}
 
-	groups := make(map[string]*awstasks.SecurityGroup)
+	var groups []SecurityGroupInfo
+
+	done := make(map[string]bool)
+
+	// Build groups that specify a SecurityGroupOverride
+	allOverrides := true
 	for _, ig := range b.InstanceGroups {
-		if ig.Spec.SecurityGroupOverride != nil && ig.Spec.Role == role {
+		if ig.Spec.Role != role {
-			name := fi.StringValue(ig.Spec.SecurityGroupOverride)
+			continue
-			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.SecurityGroupOverride == nil {
+			allOverrides = false
+			continue
+		}
+
+		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,
+			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
+
+		groups = append(groups, SecurityGroupInfo{
+			Name:   name,
+			Suffix: suffix,
+			Task:   t,
+		})
 	}
 
-	for name, t := range groups {
+	// Add the default SecurityGroup, if any InstanceGroups are using the default
-		if c != nil {
+	if !allOverrides {
-			glog.V(8).Infof("adding security group: %q", name)
+		groups = append(groups, SecurityGroupInfo{
-			c.AddTask(t)
+			Name: fi.StringValue(baseGroup.Name),
-		}
+			Task: baseGroup,
-	}
+		})
-
-	if len(groups) == 0 {
-		groups[fi.StringValue(baseGroup.Name)] = baseGroup
-		if c != nil {
-			glog.V(8).Infof("adding security group: %q", fi.StringValue(baseGroup.Name))
-			c.AddTask(baseGroup)
-		}
 	}
 
 	return groups, nil
 }
 
-// GetGroupSuffix returns the name of the security groups suffix.
+// JoinSuffixes constructs a suffix for traffic from the src to the dest group
-func GetGroupSuffix(name string, groups map[string]*awstasks.SecurityGroup) string {
+// We have to avoid ambiguity in the case where one has a suffix and the other does not,
-	if len(groups) != 1 {
+// where normally l.Suffix + r.Suffix would equal r.Suffix + l.Suffix
-		glog.V(8).Infof("adding group suffix: %q", name)
+func JoinSuffixes(src SecurityGroupInfo, dest SecurityGroupInfo) string {
-		return "-" + name
+	if src.Suffix == "" && dest.Suffix == "" {
+		return ""
 	}
 
-	return ""
+	s := src.Suffix
+	if s == "" {
+		s = "-default"
+	}
+
+	d := dest.Suffix
+	if d == "" {
+		d = "-default"
+	}
+
+	return s + d
 }

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,8 @@ spec:
   role: Master
   subnets:
   - us-test-1c
-  securityGroupOverride: sg-1234dcba
+  # We don't override the default for this master - testing mixing!
+  #securityGroupOverride: sg-master-1c
 
 ---
 
@@ -130,5 +131,5 @@ spec:
   role: Node
   subnets:
   - us-test-1a
-  securityGroupOverride: sg-1234abcd
+  securityGroupOverride: sg-nodes
 

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    = ["${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}"]
-  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 = ["${aws_security_group.masters-existingsg-example-com.id}", "sg-master-1a", "sg-master-1b"]
 }
 
 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             = ["${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")}"
 
@@ -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,360 @@ 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-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"
+  from_port                = 0
+  to_port                  = 0
+  protocol                 = "-1"
+}
+
+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"
+  from_port                = 0
+  to_port                  = 0
+  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"
+  source_security_group_id = "sg-master-1a"
+  from_port                = 0
+  to_port                  = 0
+  protocol                 = "-1"
+}
+
+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"
+  from_port                = 0
+  to_port                  = 0
+  protocol                 = "-1"
+}
+
+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_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" {
+  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"
+  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" "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" {
+  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" "node-egress-sg-nodes" {
+  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-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                  = 2379
+  protocol                 = "tcp"
+}
+
+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                = 1
+  to_port                  = 2379
+  protocol                 = "tcp"
+}
+
+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                = 1
+  to_port                  = 2379
+  protocol                 = "tcp"
+}
+
+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"
+  from_port                = 2382
+  to_port                  = 4000
+  protocol                 = "tcp"
+}
+
+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                = 2382
+  to_port                  = 4000
+  protocol                 = "tcp"
+}
+
+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                = 2382
+  to_port                  = 4000
+  protocol                 = "tcp"
+}
+
+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"
+  from_port                = 4003
+  to_port                  = 65535
+  protocol                 = "tcp"
+}
+
+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"
+  from_port                = 1
+  to_port                  = 65535
+  protocol                 = "udp"
+}
+
+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"
+  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"
+  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-node-0-0-0-0--0-sg-nodes" {
+  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"