⚠️ ELB uses separate security group

[aaroniscode]

What this PR does / why we need it:
Currently the ELB provisioned for the Kubernetes API server shares the security group with the control plane nodes. This leverages the existing LB security group that was created but never used. It also exposes the external port as set in Clusters's spec.clusterNetwork.apiServerPort

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1452

[aaroniscode]

Took a first stab at this in response to #1452. Interested in feedback if this is the preferred approach.

Also added Path MTU Discovery. This is already done for each new ELB that the AWS cloud provider adds for services of type LoadBalancer.

See: kubernetes/kubernetes#24254
Kops has it as well: kubernetes/kops#6297

[detiber]

@aaroniscode we'll need an additional new Security Group for this change, the existing LB that you are re-purposing here is meant for use by the AWS cloud-provider integration (hence the different ownership tag used).

[aaroniscode]

@aaroniscode we'll need an additional new Security Group for this change, the existing LB that you are re-purposing here is meant for use by the AWS cloud-provider integration (hence the different ownership tag used).

@detiber I noticed the comment that it was meant for use by the AWS cloud-provider integration but it didn't appear to be used. When testing the AWS cloud-provider it created new security groups for each load balancer. So I can learn, can you point to me where I missed how this is used by the AWS cloud provider?

And I can look at creating a new k8s API load balancer security group.

[detiber]

@detiber I noticed the comment that it was meant for use by the AWS cloud-provider integration but it didn't appear to be used. When testing the AWS cloud-provider it created new security groups for each load balancer. So I can learn, can you point to me where I missed how this is used by the AWS cloud provider?

It was introduced in #706, there might be additional context as to why in the comments on that PR

[aaroniscode]

@detiber I updated the PR based on your feedback. I left the LB that we create for the AWS cloud-provider integration alone and created a new LB just for the K8s API Server.

[aaroniscode]

I believe all review comments have been resolved. Thanks all! Removed the Path MTU for now as it probably belongs in more than just the k8s LB security group and wanted to keep this PR focused. Will open a separate PR for Path MTU shortly.

Also tested both Internet-facing and internal ELB.

[ncdc]

@aaroniscode what port did you test for the ELBs?

[aaroniscode]

@aaroniscode what port did you test for the ELBs?

6443

[ncdc]

Ok. Think there'd be any issue with 443?

[aaroniscode]

Ok. Think there'd be any issue with 443?

I tested with 443 a week or so back by setting in Cluster object:

spec:
  clusterNetwork:
    apiServerPort: 443

[ncdc]

/lgtm
/assign @vincepri

[randomvariable]

/lgtm too

[vincepri]

Note for later: given that we're now adding a new security group for the ELB, we should probably consider changing this line https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/1456/files#diff-b0ffc0997fd2bffcad9bd812a56adccbR382 that allows everyone to access the control plane port, regardless of network. We can probably make sure that the control plane security group is allowed to talk to the hardcoded port 6443, and the new ELB security group.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaroniscode, vincepri

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 [vincepri]

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

[ncdc]

@vincepri we'll never come back to your note unless we file an issue - will you do that?

[vincepri]

Will do yeah