Allow customization of ingress rules in control plane LB security group

[fiunchinho]

What type of PR is this?

/kind feature

What this PR does / why we need it:

We would like to customize the ingress rules that are included in the security group created for the control plane Load Balancer. Currently, capa always adds an ingress rule to allow traffic towards the k8s api from everywhere (0.0.0.0/0). But we would like to have more control over this. We would like to only allow traffic coming from our VPNs.

With the changes in this PR, when no additional ingress rules are defined, we add the ingress rule to allow traffic from everywhere. That way we are backwards compatible. But when additional rules are defined, these will be used instead.

Checklist:

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

Release note:

Allow customization of ingress rules in control plane Load Balancer security group

[k8s-ci-robot]

Welcome @fiunchinho!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @fiunchinho. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[fiunchinho]

Adding @richardcase and @alexander-demicev as we talked about this in Slack.

[alexander-demicev]

/ok-to-test

[alexander-demicev]

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

[alexander-demicev]

The logic itself looks okay to me. However, the API field name AdditionalIngressRules might be confusing to users since the change is not only defining new ingress rules but also overriding already defined rules. We haven't released this API field yet, so it can still be changed to something like CustomIngressRules. What do you think?

Also, what about the IPv6 rules below?

[fiunchinho]

Also, what about the IPv6 rules below?

I've changed the logic so that when no additional rules are defined and we want to use the default rules, we either use IPv6 or IPv4 depending on whether IPv6 is enabled or not.

[fiunchinho]

I pushed a new change to rename the field from additionalIngressRules to simply IngressRules. The name CustomIngressRules seem redundant to me, because we have many fields where we use a default when it's not set, and we don't call it CustomSomething. So IngressRules made sense to me. What do you think?

[fiunchinho]

/retest

[fiunchinho]

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

[alexander-demicev]

Looks good to me, there is also a small piece of documentation that needs to be updated https://cluster-api-aws.sigs.k8s.io/topics/bring-your-own-aws-infrastructure.html?highlight=ingress#control-plane-load-balancer

[AverageMarcus]

/lgtm

Just needs approval from a maintainer :)

/assign @richardcase @Ankitasw @Skarlso

[Skarlso]

Looks okay except for that small refactor. :) Then it should be fine. :)

[fiunchinho]

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

[Skarlso]

E0606 11:34:44.467623       1 controller.go:329] "Reconciler error" err=<
	failed to create one or more IP addresses for NAT gateways: failed to allocate Elastic IP: AddressLimitExceeded: The maximum number of addresses has been reached.
		status code: 400, request id: 162c8357-0949-45bc-b298-c9b3b41b9a17
 > controller="awscluster" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSCluster" AWSCluster="functional-test-ssm-parameter-store-clusterclass-6jt77v/cluster-vetjuj-sc9q9" namespace="functional-test-ssm-parameter-store-clusterclass-6jt77v" name="cluster-vetjuj-sc9q9" reconcileID="5e8ecaea-9c47-492c-86a8-2281666b5bc8"

Pfff...

Now we have to find out if we actually created that many, or there is a problem on the account. I've seen sometimes that when there is an error it kept creating security groups when it eventually reached the limit withing a couple seconds haha.

[fiunchinho]

E0606 11:34:44.467623       1 controller.go:329] "Reconciler error" err=<
	failed to create one or more IP addresses for NAT gateways: failed to allocate Elastic IP: AddressLimitExceeded: The maximum number of addresses has been reached.
		status code: 400, request id: 162c8357-0949-45bc-b298-c9b3b41b9a17
 > controller="awscluster" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSCluster" AWSCluster="functional-test-ssm-parameter-store-clusterclass-6jt77v/cluster-vetjuj-sc9q9" namespace="functional-test-ssm-parameter-store-clusterclass-6jt77v" name="cluster-vetjuj-sc9q9" reconcileID="5e8ecaea-9c47-492c-86a8-2281666b5bc8"

Pfff...

Now we have to find out if we actually created that many, or there is a problem on the account. I've seen sometimes that when there is an error it kept creating security groups when it eventually reached the limit withing a couple seconds haha.

What should I do now? I think the only thing I can do is to re-trigger and hope for the best 😄

[Skarlso]

Nah, don't re-trigger. If the account has a problem we are just adding to it. So let's leave it for a little while and hope it cleans up. 🤷

[Skarlso]

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

[fiunchinho]

All green now 😸

[Skarlso]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Skarlso

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

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

[richardcase]

/milestone v2.2.0