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/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/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/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..41e23f7917 100644 --- a/pkg/model/firewall.go +++ b/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 { - suffix := GetGroupSuffix(nodeGroupName, nodeGroups) + for _, group := range 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, - SourceGroup: secGroup, + SecurityGroup: dest.Task, + 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 { - var masterSuffix string - var nodeSuffix string - - 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 _, masterGroup := range masterGroups { + for _, nodeGroup := range nodeGroups { + 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, - SourceGroup: nodeGroup, + SecurityGroup: masterGroup.Task, + 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, - SourceGroup: nodeGroup, + SecurityGroup: masterGroup.Task, + 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, - SourceGroup: nodeGroup, + SecurityGroup: masterGroup.Task, + 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) { - masterGroups, err := b.createSecurityGroups(kops.InstanceGroupRoleMaster, b.Lifecycle, c) +func (b *FirewallModelBuilder) buildMasterRules(c *fi.ModelBuilderContext, nodeGroups []SecurityGroupInfo) ([]SecurityGroupInfo, error) { + masterGroups, err := b.GetSecurityGroups(kops.InstanceGroupRoleMaster) if err != nil { return nil, err } - for masterSecGroupName, masterGroup := range masterGroups { - suffix := GetGroupSuffix(masterSecGroupName, masterGroups) + for _, group := range 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, - SourceGroup: masterGroup, + SecurityGroup: dest.Task, + SourceGroup: src.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) - } + // Masters can talk to nodes + for _, dest := range nodeGroups { + suffix := JoinSuffixes(src, dest) - // Masters can talk to nodes - { - t := &awstasks.SecurityGroupRule{ - Name: s(fmt.Sprintf("all-master-to-node%s", nodeSecGroupName)), - Lifecycle: b.Lifecycle, - SecurityGroup: nodeGroup, - SourceGroup: masterGroup, - } - c.AddTask(t) + t := &awstasks.SecurityGroupRule{ + Name: s("all-master-to-node" + suffix), + Lifecycle: b.Lifecycle, + SecurityGroup: dest.Task, + SourceGroup: src.Task, } + c.AddTask(t) } } return masterGroups, nil } -func (b *KopsModelContext) GetSecurityGroups(role kops.InstanceGroupRole) (map[string]*awstasks.SecurityGroup, 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) (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") - /* - // 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), + 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") } - 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 { - 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) + + // 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 { - if c != nil { - glog.V(8).Infof("adding security group: %q", name) - c.AddTask(t) - } - } - - 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) - } + // Add the default SecurityGroup, if any InstanceGroups are using the default + if !allOverrides { + groups = append(groups, SecurityGroupInfo{ + Name: fi.StringValue(baseGroup.Name), + Task: baseGroup, + }) } 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 +// 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 "" } - 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/in-v1alpha2.yaml b/tests/integration/update_cluster/existing_sg/in-v1alpha2.yaml index 5a019aff5d..e670497777 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,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 diff --git a/tests/integration/update_cluster/existing_sg/kubernetes.tf b/tests/integration/update_cluster/existing_sg/kubernetes.tf index 291a944c5d..c63adc5ebe 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 = ["${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"