Add support for vpc.controlPlaneOnPrivateSubnets - #8793
Conversation
When eksctl creates a VPC, it passes both the public and the private subnets to the EKS API, so the control plane's cross-account ENIs are placed in public subnets as well. Restricting them to private subnets afterwards requires `eksctl utils update-cluster-vpc-config`, which calls the EKS API directly and leaves the cluster's CloudFormation stack out of sync. Add a `vpc.controlPlaneOnPrivateSubnets` field so that only the private subnets are passed to the EKS API at creation time. Public subnets are still created and used for NAT gateways and internet-facing load balancers, so this does not make the cluster fully private. The field is honoured for eksctl-created VPCs (IPv4 and IPv6, including fully-private clusters) and for pre-existing VPCs. It is rejected when combined with `vpc.controlPlaneSubnetIDs`, and when the configured private subnets do not cover at least two availability zones, which EKS requires. That last check is best-effort: subnets given only by ID have their zone resolved from EC2 after validation runs, so they are allowed through and rejected by the EKS API instead. Outposts are exempt, since the control plane there is already private-only. `eksctl utils update-cluster-vpc-config` rejects the field rather than silently ignoring it, and points at `vpc.controlPlaneSubnetIDs` instead. Control plane subnet selection is now built through a single helper. autoMode is passed into it rather than derived, so that it keeps applying only to VPCs that eksctl creates: the pre-existing VPC path has never restricted the control plane for Auto Mode clusters, and deriving it would have changed that behaviour for configurations that do not use the new field. Issue eksctl-io#8792 Signed-off-by: guessi <guessi@gmail.com>
|
The multi-AZ validation doesn't cover the path the docs advertise. validateControlPlaneOnPrivateSubnets bails out when VPC.Subnets == nil, assuming SetSubnets guarantees one private subnet per requested zone. It doesn't: validateAvailabilityZones only checks count and explicitly permits duplicates, and SetSubnets keys private subnets by zone name, so availabilityZones: [us-west-2a, us-west-2a] collapses to one subnet. I confirmed this passes validation with no complaint. Either count distinct entries in c.AvailabilityZones in that branch (three lines), or drop the docs claim that "eksctl validates this before creating anything." The first is better. The update-cluster-vpc-config test needs to stop calling AWS. newMockCmd drives the real command with no provider mocking, so once the loader passes, doUpdateClusterVPCConfig reaches STS and EKS. It currently only passes because credentials fail to resolve, and its assertion sits inside if err != nil, so it asserts nothing when the call succeeds. Every other case in that file fails before provider construction. Assert against cmdutils.NewUpdateClusterVPCLoader(...).Load() directly. The new hard error in update_cluster_vpc.go directly contradicts the logger.Info on the line above it, which promises other fields "will be ignored." Live, they print back to back. The codebase convention for this situation is logger.Warning (see eksctl update nodegroup in configfile.go:1018), and the strictness is selective anyway since create nodegroup -f and upgrade cluster -f still ignore the field silently. I lean toward a warning, because rejecting breaks the documented one-config-file-per-cluster workflow. |
|
@guessi Thanks for raising this PR, overall looks good. |
- Count distinct availability zones in c.AvailabilityZones when validating controlPlaneOnPrivateSubnets for eksctl-created VPCs, instead of assuming SetSubnets guarantees per-zone coverage. Duplicate zones collapse into a single private subnet in vpc.SetSubnets, and validateAvailabilityZones permits duplicates, so this was previously unchecked. - Rework the update-cluster-vpc-config tests for controlPlaneOnPrivateSubnets to assert against cmdutils.NewUpdateClusterVPCLoader(...).Load() directly instead of executing the full command, which reached AWS via NewProviderForExistingCluster. - Change eksctl utils update-cluster-vpc-config to warn and ignore vpc.controlPlaneOnPrivateSubnets instead of returning a hard error, consistent with how eksctl update nodegroup handles other unsupported fields in a config file. - Update the userdocs claim that eksctl validates AZ coverage before creating anything to note the pre-existing-subnets-by-ID exception, where AZs are resolved from EC2 after validation runs.
|
@gustavodiaz7722 thanks for your time to review PR, I've submit a new commit to address mentioned concerns, please review. |
The new AZ check rejects configs where eksctl selects the zones
apiVersion: eksctl.io/v1alpha5
kind: ClusterConfig
metadata:
name: example
region: us-west-2
vpc:
controlPlaneOnPrivateSubnets: trueSame file against both revisions of this PR, via eksctl had chosen three distinct zones, so that cluster would have created fine. The new Suggested fixGate on a non-empty slice. Auto-selection always yields distinct zones, so there is nothing to if c.VPC.Subnets == nil {
if len(c.AvailabilityZones) > 0 {
if azs := sets.New(c.AvailabilityZones...); azs.Len() < MinRequiredAvailabilityZones {
return fmt.Errorf("vpc.controlPlaneOnPrivateSubnets requires at least %d distinct availability zones, got %d (%v)",
MinRequiredAvailabilityZones, azs.Len(), c.AvailabilityZones)
}
}
return nil
}plus flipping that test to expect success. The |
|
@gustavodiaz7722 Cool, thanks for keeping it at highest standard, will definitely revise my code to get it fixed. Thanks! |
validateControlPlaneOnPrivateSubnets ran before eks.SetAvailabilityZones, so when availabilityZones was left unset it saw an empty c.AvailabilityZones and rejected the config with 'got 0 ([])', even though eksctl always selects distinct zones itself on that path. Only enforce the distinct-zone count when availabilityZones is explicitly set, and flip the test that had locked in the rejection to expect success instead. Document the auto-selection exception in the userdocs alongside the existing pre-existing-subnets-by-ID one, as a nested list since the two exceptions no longer read clearly as a single sentence. Signed-off-by: guessi <guessi@gmail.com>
The loader test only asserted that no error was returned, which passes just as well if the warn-and-ignore behaviour is dropped entirely. Capture logger output and assert the warning text, and assert the field is not translated into controlPlaneSubnetIDs. Verified the assertion fails when the warning is changed. Document that the field only takes effect at cluster creation time and is warned about and ignored by eksctl utils update-cluster-vpc-config, which the code did but the docs did not mention. Signed-off-by: guessi <guessi@gmail.com>
|
@gustavodiaz7722 Thanks, I've adopted your suggested fix in 2d2cbe7. Another commit, 20d7adc, strengthens the loader-test assertion, with the corresponding docs update. |
|
the PR here is based on v0.229.0, I noticed there have tons of fixed introduced by v0.230.0, let me know if I should rebase/squash the commits, thanks! |
Description
Closes #8792.
When eksctl creates a VPC, it currently passes both the public and the private subnets to the EKS API, so the control plane's cross-account ENIs are placed in public subnets as well as private ones. Restricting them to private subnets afterwards requires
eksctl utils update-cluster-vpc-config, which calls the EKS API directly and leaves the cluster's CloudFormation stack out of sync with the actual configuration.This PR adds a
vpc.controlPlaneOnPrivateSubnetsfield so that only the private subnets are passed to the EKS API at cluster creation time. Public subnets are still created and used for NAT gateways and internet-facing load balancers — this does not make the cluster fully private, it only changes which subnets the control plane's ENIs land in.Key implementation points:
vpc.controlPlaneSubnetIDs, since that field already gives explicit control over control plane subnets.availabilityZonesis set explicitly, distinct zones are counted, since duplicates are permitted elsewhere and collapse into a single private subnet. When subnets are pre-existing, their zones are counted directly. The check is best-effort and is skipped in the two cases where the zones are not yet known at validation time:availabilityZonesis left unset, so eksctl selects the zones itself ineks.SetAvailabilityZones, which runs after validation. Auto-selection always yields distinct zones, so there is nothing to check.eksctl utils update-cluster-vpc-confignow warns and ignores the field (consistent with howeksctl update nodegrouphandles other unsupported fields) and points users atvpc.controlPlaneSubnetIDsinstead, since that command talks to the EKS API directly and can't keep the CloudFormation stack in sync.autoModeis passed into it rather than derived internally, so the new field keeps applying only to VPCs that eksctl creates — the pre-existing VPC path has never restricted the control plane for Auto Mode clusters, and derivingautoModethere would have changed that behaviour for configurations not using the new field.See
examples/49-control-plane-on-private-subnets.yamlfor a usage example, and the updateduserdocs/src/usage/cluster-subnets-security-groups.mdfor documentation.Checklist
README.md, or theuserdocsdirectory)area/nodegroup) and kind (e.g.kind/improvement)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯