feat: Bring your own control plane ELB

[dlipovetsky]

What type of PR is this?
/kind feature

What this PR does / why we need it:
Lets the user define the control plane ELB name, so that the AWSCluster controller can use an existing ELB.

If the ELB exists and does not have a tag that says it is owned by the cluster, then the controller treats the ELB as unmanaged: it does not reconcile the ELB's attributes, tags, or attached subnets.

If the ELB does not exist, the controller behaves as has in the past: It creates the ELB, marks the ELB as owned by the cluster, and treats it as managed: it reconciles the ELB's attributes, tags, and attached subnets.

If the user does not provide a name, the controller generates a name from the cluster name and writes it to the AWSCluster Spec.

Something else to note: If the user defines the name, but the ELB does not exist, then the controller creates an ELB with the user-defined name, and treats it as managed.

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 #2774

Special notes for your reviewer:
I have tried to keep changes to a minimum, but despite that, the PR is large. I find it easier to review the commits one-by-one.

Checklist:

• squashed commits
• includes documentation
• adds unit tests
• adds or updates e2e tests

Release note:

Adds support for using an existing Classic ELB as the control plane load balancer. This is an advanced feature.

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[dlipovetsky]

/ok-to-test

[dlipovetsky]

/retest

[dlipovetsky]

/retest

[richardcase]

Looking good! A few minor comments

[dlipovetsky]

Another point of discussion:

CAPA has to be able to update the ELB's instances. I think it's also ok the let CAPA make other updates to the ELB.

However, as with other bring-your-own infrastructure, CAPA should not delete the ELB.

[dlipovetsky]

In light of @richardcase's comments, I'd like to change the approach here: The AWSCluster controller should generate the ELB name and store it in the Spec. I considered this earlier, but found it awkward to implement with the existing ReconcileLoadbalancers abstraction.

[richardcase]

In light of @richardcase's comments, I'd like to change the approach here: The AWSCluster controller should generate the ELB name and store it in the Spec. I considered this earlier, but found it awkward to implement with the existing ReconcileLoadbalancers abstraction.

This would be a similar approach to what we have taken elsewhere, i think this would be good.

[dlipovetsky]

I've identified a new problem: The AWSCluster controller deletes the control plane ELB when the it deletes the cluster resources. It should not delete a user-created control-plane ELB. The controller needs to know whether the ELB is user-created or CAPA-created.

If the new Name field is defined if and only if the ELB is user-created, the controller can infer that the ELB is user-created from the field. If, to allow future changes to the way we generate the name, we write the generated name to this field, then the controller can no longer infer whether the ELB is user-created.

I think we'll need to store the generated name and user-defined name separately, or add a boolean that indicates the name is generated. Maybe I can repeat the approach we used with the VPC: store the ID, but determine whether it's CAPA-created by reading the tags.

[richardcase]

The controller needs to know whether the ELB is user-created or CAPA-create

I think we should be able to use the owned tag?

[richardcase]

Good job on fixing the e2e failures so quick 🥇

I have no further comments on this change and the e2e are passing (here and here), so from my part:

/lgtm

[dlipovetsky]

Unfortunately, I just got another merge conflict, I think with #2900. It's a bit silly, because it appears to be just whitespace.

I'm rebasing on main and force-pushing.

[dlipovetsky]

Re-running because I resolved the merge conflict and force-pushed:

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

[randomvariable]

/hold
for when the tests pass

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: randomvariable

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

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

[k8s-ci-robot]

@dlipovetsky: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-aws-e2e	138bcac	link	false	/test pull-cluster-api-provider-aws-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[randomvariable]

GPU test is flaking #2905

[randomvariable]

/unhold