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
Closed
Labels

Comments

@errordeveloper
Copy link
Contributor

errordeveloper 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
Copy link
Contributor Author

errordeveloper commented Oct 31, 2018

The code is here:

https://github.com/weaveworks/eksctl/blob/fa8e2496c223f7462f80510ba8cc886e67d8b942/pkg/cfn/builder/cluster.go#L109-L116

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.

https://github.com/weaveworks/eksctl/blob/fa8e2496c223f7462f80510ba8cc886e67d8b942/pkg/cfn/builder/nodegroup.go#L110-L122

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.

@errordeveloper errordeveloper mentioned this issue Nov 8, 2018
3 tasks
torredil pushed a commit to torredil/eksctl that referenced this issue May 20, 2022
Return correct error when restoring non-existent snapshot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant