feat: allow for specifying subnet type for az

[davidspek]

What type of PR is this?
/kind feature

What this PR does / why we need it:

Currently when specifying the availability zone(s) for a (managed) machine pool, all the subnets in the zone(s) will be used. By default CAPI creates both a public and a private subnet in each availability zone. So when a machine pool specifies an availability zone, both the public and private subnets for the zone are used for the machine pool. It seems as though the private subnet is preferred in the ASG, however, I haven't been able to find any documentation regarding which subnet an instance gets launched in when an ASG contains both a private and public subnet. This means it's nondeterministic in which subnet an instance of the ASG will be launched in.

To solve for this, a new optional parameter AvailabilityZoneSubnetType is added to the AWSManagedMachinePool and AWSMachinePool specs allowing administrators to choose if the machine pool should use public or private subnets in the requested availability zones. If the parameter isn't specified the current behavior is kept where both public and private subnets in the availability zone(s) are used. This is also related to #2191 where the preferred subnet type is private when no options are specified, but doing this currently isn't possible when specifying availability zone(s) for a machine pool.

Which issue(s) this PR fixes:
Fixes #2991

Special notes for your reviewer:

Checklist:

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

Release note:

Allow for specifying if private or public subnets should be used when availability zones are defined in `AWSManagedMachinePool` and `AWSMachinePool` specs.

[k8s-ci-robot]

Hi @davidspek. 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.

[AndiDog]

Likely, no new feature is needed. Please try this:

apiVersion: infrastructure.cluster.x-k8s.io/v1beta2
kind: AWSMachinePool
spec:
  subnets:
  - filters:
      - name: tag:kubernetes.io/cluster/mycluster
        values:
          - shared
          - owned
      - name: tag:sigs.k8s.io/cluster-api-provider-aws/role
        values:
          - private
# [...]

[davidspek]

Likely, no new feature is needed. Please try this:

apiVersion: infrastructure.cluster.x-k8s.io/v1beta2
kind: AWSMachinePool
spec:
  subnets:
  - filters:
      - name: tag:kubernetes.io/cluster/mycluster
        values:
          - shared
          - owned
      - name: tag:sigs.k8s.io/cluster-api-provider-aws/role
        values:
          - private
# [...]

We are using AWSManagedMachinePool resources which don't allow for filtering the subnets in that manor.

[davidspek]

@AndiDog Also, I don't think the subnet filter works alongside the specified availability zones. We need to be able to specify the type of subnet (private or public) for the specified availability zone(s).

[AndiDog]

Code-wise, for AWSManagedMachinePool, it's func (p *defaultSubnetPlacementStrategy) Place(input *placementInput) ([]string, error) which decides the subnets to use. That's also the place you found and improved.

I'd love if EKS types became more consistent with non-EKS. At best, we would implement AWSManagedMachinePool.Spec.Subnets and deprecate AWSManagedMachinePool.Spec.SubnetIDs, while still keeping backward compatibility for a while.

You said in chat

After looking at implementing the Subnets field for the AWSManagedMachinePool I noticed this doesn't actually work alongside the specified availability zones.

Can you explain what that means? Did you hit a roadblock implementing that?

[davidspek]

The Subnets field allows you to filter subnets in the VPC that should be used by a node pool. However, it doesn't take the availability zone(s) set in the spec.availabilityZones into account. Even with implementing Subnets in the machine pool spec, you can't limit a machine pool to a specific availability zone and subnet type (private or public).

So in short Subnets can make it easy to allow you specify creating node pool(s) into all public or private subnets in the VPC.
We want to be able to specify public or private subnets for a specific availability zone.

The spec.availabilityZones is used to make it easier for people to place node pools into specific AZs, but all it actually does is subnet selection. The field is somewhat useless if you can't tell it in which type of subnet you wouldn't want to place it (since you probably don't want public unless you need it for some very specific reason in which case you wouldn't want it to use private). If you can also filter for the AZ in the subnet filter that could be a solution, but then spec.availabilityZones might as well be removed since it doesn't fully work as a helper field.

[richardcase]

/ok-to-test

[davidspek]

/retest

[davidspek]

@richardcase The tests pass now.

[AndiDog]

The spec.availabilityZones is used to make it easier for people to place node pools into specific AZs, but all it actually does is subnet selection. The field is somewhat useless if you can't tell it in which type of subnet you wouldn't want to place it (since you probably don't want public unless you need it for some very specific reason in which case you wouldn't want it to use private). If you can also filter for the AZ in the subnet filter that could be a solution, but then spec.availabilityZones might as well be removed since it doesn't fully work as a helper field.

All true! I think on top of the tag sigs.k8s.io/cluster-api-provider-aws/role={public,private}, there should be one for the AZ, such that the Subnets[*].Filter can use them.

[AndiDog]

@richardcase The above ideas (not what the PR currently implements) would be breaking changes and probably we can consider that on top of the AWSNetwork CRD idea in order to improve networking.

[richardcase]

Should we also add the existing situation as well explicitly (i.e. all or publicandprivate)?

[richardcase]

So that people can explicitly choose that.

[davidspek]

I can definitely add that. Currently not setting the value does that, but we could add an explicit toggle for it as well in case the default logic changes in the future.

[davidspek]

@richardcase I've rebased and added an option for all along with updated unit tests.

[richardcase]

Thanks @davidspek ...i'll take a look

[davidspek]

Thanks!

[davidspek]

@richardcase Have you been able to take another look at this?

[davidspek]

@AndiDog Maybe you can also have a look here since you've just become a member.

[AndiDog]

/lgtm

Looks good. Did you also test this successfully for your use case?

[davidspek]

@AndiDog Yes, I've built an image for the controller and been able to successfully deploy AWSManagedMachinePool resources that specify the AZs as well as the new AvailabilityZoneSubnetType and those node groups are only bound to the private subnet(s).

[davidspek]

@AndiDog @richardcase What's left before this can be merged?

[davidspek]

Friendly ping @richardcase @AndiDog

[Ankitasw]

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

[Ankitasw]

/approve
/hold

until tests passes. @davidspek feel free to unhold once tests passes

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ankitasw

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

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

[davidspek]

/unhold