Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nodegroup stack builder should not assume subnet/AZ mapping based on the order #293

Closed
errordeveloper opened this issue Oct 31, 2018 · 1 comment · Fixed by #310

Comments

@errordeveloper
Copy link
Member

commented Oct 31, 2018

At the moment we assume order of subnets we get in a list is the same as the order of AZs. This is indeed the case most of the time with eksctl create cluster, as the lists are the same.
However, if we want to create a nodegroup separately, that assumption is most likely to break.

I believe we have to options here:

  • let cluster stack export the list of AZs in the same order as subnets, which is bound to ordering assumption at the time of creation (which is probably fine)
  • when importing subnets from cluster stack, check which AZs they belong to (probably via an API lookup on eksctl side, doing it in CloudFormation would be harder)

Broadly, the boundary is currently unclean between how we use stack exports and how we maintain state on eksctl side, we have a bit of mix going on at the moment and it's most likely to stay that way, as we do what's convenient most of the time, especially because some of CloudFormation facilities are harder to use then others and we only utilise some of them.

This area needs to be reviewed.

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2018

The code is here:

// TODO: shouldn't assume the order - https://github.com/weaveworks/eksctl/issues/293
for i, subnet := range c.outputs.SubnetsPrivate {
c.spec.ImportSubnet(api.SubnetTopologyPrivate, c.spec.AvailabilityZones[i], subnet)
}
for i, subnet := range c.outputs.SubnetsPublic {
c.spec.ImportSubnet(api.SubnetTopologyPublic, c.spec.AvailabilityZones[i], subnet)
}

This is arguably okay for most of current uses actually, this is not called in isolation from cluster stack creation process. Calling the API actually will require mocking in tests. I maybe easier to export list of zone, as it's probably safe to assume CloudFormation won't re-order the list in any way.

subnets := n.clusterSpec.VPC.Subnets[n.spec.SubnetTopology()]
errorDesc := fmt.Sprintf("(subnets=%#v AZs=%#v)", subnets, n.spec.AvailabilityZones)
if len(subnets) < numNodeGroupsAZs {
return fmt.Errorf("VPC doesn't have enough subnets for nodegroup AZs %s", errorDesc)
}
vpcZoneIdentifier = make([]interface{}, numNodeGroupsAZs)
for i, az := range n.spec.AvailabilityZones {
subnet, ok := subnets[az]
if !ok {
return fmt.Errorf("VPC doesn't have subnets in %s %s", az, errorDesc)
}
vpcZoneIdentifier.([]interface{})[i] = subnet.ID
}

However, there is a nodegroup stack part, that currently depends on cfg.VPC.Subnets to be fully populated, so we might just need to satisfy the needs of nodegroup creation here, for which exporting a list may suffice just fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.