Allow securing api LB, only allowing traffic from required sources

[fiunchinho]

What type of PR is this?
/kind bug

What this PR does / why we need it:

On this PR #4304 we introduced the possibility of passing custom ingress rules that would be applied to the api Load Balancer security group, so that users could specify more granular rules (for example, allowing traffic only from company VPNs) instead of allowing traffic from everywhere.
But the api Load Balancer needs to always allow traffic coming from the kubelet, or the cluster won't work.

On this PR, I'm changing the logic so that the custom ingress rules are appended to a set of basic rules that will always be added to make sure the kubelet can talk to the api Load Balancer. This set of basic rules will be different depending on whether the Load Balancer is a internet-facing or internal one.

• When using a internet-facing LB, we allow the public IPs of the Nat Gateways.
• When using an internal LB, we allow the workload cluster VPC CIDR.

Special notes for your reviewer:

Checklist:

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

Release note:

Fix control plane LB ingress rules so that kubelet can access the API

[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.

[richardcase]

/ok-to-test

[fiunchinho]

/retest

[fiunchinho]

/retest

[fiunchinho]

/retest

[fiunchinho]

/retest

[fiunchinho]

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

[fiunchinho]

/retest

[fiunchinho]

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

[Skarlso]

Uh, why did the APIDiff thing start to fail?

[fiunchinho]

Uh, why did the APIDiff thing start to fail?

I don't know :/ The job output is not very helpful.

[Skarlso]

@fiunchinho What was your latest commit? It's squashed so I don't know. :) It didn't fail before.

[fiunchinho]

I'd say it's this.

But what is the job complaining about? what's the change in the api?

[Skarlso]

Ah, try running make clean && make generate please. See if anything changes.

[fiunchinho]

Ah, try running make clean && make generate please. See if anything changes.

Nothing changes.

[Skarlso]

Usually that api-diff says that there is a backwards incompatible change between two API versions. I suggest reverting that commit, or try looking in the other packages for that same commit and change it also there.

[fiunchinho]

Usually that api-diff says that there is a backwards incompatible change between two API versions. I suggest reverting that commit, or try looking in the other packages for that same commit and change it also there.

Looking at the output of the job, I don't think there is a backwards incompatible change

sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope
  Incompatible changes:
  - NetworkScope.GetNatGatewaysIPs: added
  - NetworkScope.SetNatGatewaysIPs: added
  - SGScope.GetNatGatewaysIPs: added
  - SGScope.SetNatGatewaysIPs: added
  Compatible changes:
  - (*ClusterScope).GetNatGatewaysIPs: added
  - (*ClusterScope).SetNatGatewaysIPs: added
  - (*ManagedControlPlaneScope).GetNatGatewaysIPs: added
  - (*ManagedControlPlaneScope).SetNatGatewaysIPs: added

I think it should be ok to merge.

[Skarlso]

These are the incompatible changes:

  Incompatible changes:
  - NetworkScope.GetNatGatewaysIPs: added
  - NetworkScope.SetNatGatewaysIPs: added
  - SGScope.GetNatGatewaysIPs: added
  - SGScope.SetNatGatewaysIPs: added
  - ```

[fiunchinho]

These are the incompatible changes:

  Incompatible changes:
  - NetworkScope.GetNatGatewaysIPs: added
  - NetworkScope.SetNatGatewaysIPs: added
  - SGScope.GetNatGatewaysIPs: added
  - SGScope.SetNatGatewaysIPs: added
  - ```

But that's not the API, isn't it?. Isn't the job supposed to look for incompatible changes in the facing API, like changes inm AWSClusterSpec and so on? Also, how is adding functions an incompatible change?

[Skarlso]

No. You modified the interface by adding two new functions. Which means that this change is now backwards incompatible since people requiring that interface will have a broken code.

I'm not sure how we usually deal with changes like these. @richardcase @Ankitasw ?

[Ankitasw]

No. You modified the interface by adding two new functions. Which means that this change is now backwards incompatible since people requiring that interface will have a broken code.

I'm not sure how we usually deal with changes like these. @richardcase @Ankitasw ?

Generally we don't add API changes to the existing API version, we wait for the API version bump, as these APIs are exposed and people who are already using CAPA latest version, their clusters would break if they are using these exposed APIs. I would suggest we wait for v1beta3 API for this change, but I would also defer to @richardcase

[richardcase]

There are 2 types of API changes going on here, the k8s CRD change and changes to exported Go packages.

The new NatGatewaysIPs field is optional, because of this, it's a compatible change as it doesn't require the user to do anything for normal behaviour of CAPA. If the field was required or if the change was changing the behaviour of an existing field, then it would be different. So, even though the apidiff is marking it as a compatible change, it's always worth checking the nuance of the field change.

With the other changes to the scopes that apidiff is complaining about. Historically, even though we have apidiff we haven't guaranteed not breaking the Go API for people that have imported the packages directly (we should probably revisit this decision and also the use of internal in the office hours soon). Also, as we are just adding functions and any consumers of the Go API can choose not to bump their imports, so i think we can override this.

/override pull-cluster-api-provider-aws-apidiff-main

[k8s-ci-robot]

@richardcase: Overrode contexts on behalf of richardcase: pull-cluster-api-provider-aws-apidiff-main

In response to this:

There are 2 types of API changes going on here, the k8s CRD change and changes to exported Go packages.

The new NatGatewaysIPs field is optional, because of this, it's a compatible change as it doesn't require the user to do anything for normal behaviour of CAPA. If the field was required or if the change was changing the behaviour of an existing field, then it would be different. So, even though the apidiff is marking it as a compatible change, it's always worth checking the nuance of the field change.

With the other changes to the scopes that apidiff is complaining about. Historically, even though we have apidiff we haven't guaranteed not breaking the Go API for people that have imported the packages directly (we should probably revisit this decision and also the use of internal in the office hours soon). Also, as we are just adding functions and any consumers of the Go API can choose not to bump their imports, so i think we can override this.

/override pull-cluster-api-provider-aws-apidiff-main

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.

[Skarlso]

Sounds good. Thanks, Richard!
/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

[Ankitasw]

/cherry-pick release-2.2

[k8s-infra-cherrypick-robot]

@Ankitasw: new pull request created: #4496

In response to this:

/cherry-pick release-2.2

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.