Removing unused/deprecated security groups and ports. Updated AWS doc

[cuppett]

Summary:

• Console security group unused, replaced along the way by Router ELB
• API security group unused, no longer in use after switch to NLB
• Ports 80/443 not required, console using service port and Router ELB
• Master port 22623 only targeted from inside the VPC
• API server only on 6443 (6444 & 6445 carryover from Tectonic)
• SSH port only targeted from inside the VPC with move to private
• Heapster (Port 4194) no longer used
• Master port 10255 (insecure) deprecated and removed in OpenShift
• Renamed 10250 to secure, because it is the secure kubelet port

[cuppett]

/assign @wking

[cuppett]

cc @crawford @eparis @abhinavdahiya

I noticed some of this debt when going through everything for UPI PoCs. It cleans up our AWS story quite a bit.

[abhinavdahiya]

/approve

[wking]

Final consumers for this and api_sg_id were removed in ec2cfa5 (coreos/tectonic-installer#2177). Nice catch :)

[wking]

nit: drop "three"? You already count them in the previous line, and the list is short and numbered. Personally, if we're going to maintain an enumerated list, I'd rather drop "Each cluster creates {number} distinct security groups." as well.

[wking]

$ git grep '^resource "aws_security_group"' origin/pr/1306 | cat
origin/pr/1306:data/data/aws/bootstrap/main.tf:resource "aws_security_group" "bootstrap" {
origin/pr/1306:data/data/aws/vpc/sg-master.tf:resource "aws_security_group" "master" {
origin/pr/1306:data/data/aws/vpc/sg-worker.tf:resource "aws_security_group" "worker" {

So I'm not sure whether the VPC default counts or not (I'd guess it does), but this entry should probably be "Bootstrap", and we might want to list it before the master entry.

[cuppett]

6443 is the API server. 12251 and 12252 are the other two. 12250 is kubelet secure. 6444 and 6445 are no longer needed.

…

On Tue, Feb 26, 2019 at 4:55 PM Abhinav Dahiya ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In data/data/aws/vpc/sg-master.tf <#1306 (comment)>: > - - protocol = "tcp" - from_port = 4194 - to_port = 4194 - self = true -} - -resource "aws_security_group_rule" "master_ingress_heapster_from_worker" { - type = "ingress" - security_group_id = "${aws_security_group.master.id}" - source_security_group_id = "${aws_security_group.worker.id}" - - protocol = "tcp" - from_port = 4194 - to_port = 4194 + to_port = 6443 What i remember from the spike was, we were kube-apiserver on 6443 <https://github.com/coreos-inc/tectonic-operators/blob/e2121a3daab332cbbb800802d566093f8f395f9d/operator/kube-core/spec/spec.go#L32>, openshift-apiserver on 6444 <https://github.com/coreos-inc/tectonic-operators/blob/e2121a3daab332cbbb800802d566093f8f395f9d/operator/kube-core/spec/spec.go#L44> and openshift-controller-manager on 6445 <https://github.com/coreos-inc/tectonic-operators/blob/e2121a3daab332cbbb800802d566093f8f395f9d/operator/kube-core/spec/spec.go#L56> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1306 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOpIAley47tbP6ajiXDH3Y_mkNnNNVLks5vRa1kgaJpZM4bOSdx> .

-- Stephen Cuppett OpenShift Engineering Red Hat <https://www.redhat.com> <https://red.ht/sig>

[cuppett]

VPC default is just an artifact of VPC creation. The bootstrap security group is removed when that terraform is torn down at the end. It's replaced by the Router one when that operator comes up. So the installer creates "three" or "four" depending on how you count them and then you are left with "four" total at the end.

…

On Tue, Feb 26, 2019 at 4:44 PM W. Trevor King ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In docs/user/aws/limits.md <#1306 (comment)>: > @@ -53,8 +53,13 @@ the new VPC. If you intend to create more than 20 clusters, you will need to req ## Security Groups -Each cluster creates 10 distinct security groups. The default limit of 2,500 for new accounts allows for many clusters -to be created. +Each cluster creates 4 distinct security groups. The default limit of 2,500 for new accounts allows for many clusters +to be created. The three security groups created by the default install are: + + 1. VPC default + 1. Master + 1. Worker + 1. Router/Ingress $ git grep '^resource "aws_security_group"' origin/pr/1306 | cat origin/pr/1306:data/data/aws/bootstrap/main.tf:resource "aws_security_group" "bootstrap" { origin/pr/1306:data/data/aws/vpc/sg-master.tf:resource "aws_security_group" "master" { origin/pr/1306:data/data/aws/vpc/sg-worker.tf:resource "aws_security_group" "worker" { So I'm not sure whether the VPC default counts or not (I'd guess it does), but *this* entry should probably be "Bootstrap", and we might want to list it before the master entry. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1306 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOpILocyHScxD1elFpV-04TJt3FR2B1ks5vRaqugaJpZM4bOSdx> .

-- Stephen Cuppett OpenShift Engineering Red Hat <https://www.redhat.com> <https://red.ht/sig>

[cuppett]

I'll add a commit to clean this up.

…

On Tue, Feb 26, 2019 at 4:42 PM W. Trevor King ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In docs/user/aws/limits.md <#1306 (comment)>: > @@ -53,8 +53,13 @@ the new VPC. If you intend to create more than 20 clusters, you will need to req ## Security Groups -Each cluster creates 10 distinct security groups. The default limit of 2,500 for new accounts allows for many clusters -to be created. +Each cluster creates 4 distinct security groups. The default limit of 2,500 for new accounts allows for many clusters +to be created. The three security groups created by the default install are: nit: drop "three"? You already count them in the previous line, and the list is short and numbered. Personally, if we're going to maintain an enumerated list, I'd rather drop "Each cluster creates {number} distinct security groups." as well. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1306 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOpIOUrKSV2KdkvEDZ5RexUFQxTkCR6ks5vRaosgaJpZM4bOSdx> .

-- Stephen Cuppett OpenShift Engineering Red Hat <https://www.redhat.com> <https://red.ht/sig>

[cuppett]

See my other reply, We don't need 6444 or 6445.

…

On Tue, Feb 26, 2019 at 4:38 PM W. Trevor King ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In data/data/aws/vpc/sg-master.tf <#1306 (comment)>: > - - protocol = "tcp" - from_port = 4194 - to_port = 4194 - self = true -} - -resource "aws_security_group_rule" "master_ingress_heapster_from_worker" { - type = "ingress" - security_group_id = "${aws_security_group.master.id}" - source_security_group_id = "${aws_security_group.worker.id}" - - protocol = "tcp" - from_port = 4194 - to_port = 4194 + to_port = 6443 This unwinds part of ff00e12 <ff00e12> (coreos/tectonic-installer#3270 <coreos/tectonic-installer#3270>). I see a question <coreos/tectonic-installer#3270 (comment)> about 6445 there, but no further explanation. Does anyone know what this was about? If 6445 was broken (I'd have expected the 6443 you return to here) but clusters functioned anyway, then do we actually need this rule at all? Or is this rule pre-positioning for kubernetes/kubernetes#65618 <kubernetes/kubernetes#65618> or something? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1306 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOpID3IIfyoxlHfUVlBasbaWQfPlkgGks5vRak7gaJpZM4bOSdx> .

-- Stephen Cuppett OpenShift Engineering Red Hat <https://www.redhat.com> <https://red.ht/sig>

[wking]

The bootstrap security group is removed when that terraform is torn down at the end. It's replaced by the Router one when that operator comes up.

Are we comfortable leaving the bootstrap group unlisted? For example, we list the bootstrap machine. I expect the router/ingress group is created after the bootstrap resources have been torn down, but I dunno if that's reliable (or even true). Similarly for the bootstrap machine, I expect it to usually be torn down around when or before the compute nodes come up. Do we have a reason to list the transitive bootstrap instance but not its security group?

[cuppett]

The transitive state may not be material to the overall limit doc at this spot. Can add it to the PR, but it opens us up for explaining the pivot states, etc. in the user guide sections here. Am okay either way.

…

On Tue, Feb 26, 2019 at 6:08 PM W. Trevor King ***@***.***> wrote: The bootstrap security group is removed when that terraform is torn down at the end. It's replaced by the Router one when that operator comes up. Are we comfortable leaving the bootstrap group unlisted? For example, we list the bootstrap machine <https://github.com/openshift/installer/blob/8811e63e3f70196f088d6bbf3993ca9043ac3909/docs/user/aws/limits.md#instance-limits>. I expect the router/ingress group is created after the bootstrap resources have been torn down, but I dunno if that's reliable (or even true). Similarly for the bootstrap machine, I expect it to usually be torn down around when or before the compute nodes come up. Do we have a reason to list the transitive bootstrap instance but not its security group? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1306 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOpIHHJnD3lOhEoIJB4TSAPbxOoFpkLks5vRb6EgaJpZM4bOSdx> .

-- Stephen Cuppett OpenShift Engineering Red Hat <https://www.redhat.com> <https://red.ht/sig>

[crawford]

/approve

[abhinavdahiya]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, crawford, cuppett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [abhinavdahiya,crawford]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[wking]

The transitive state may not be material to the overall limit doc at this spot. Can add it to the PR, but it opens us up for explaining...

I'm also fine either way, but would rather we are internally consistent, listing both or neither of the bootstrap instance and security group. Currently we only list the former.