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

Fix an issue with referencing of a map in ctl.ImportSubnet #533

Merged
merged 2 commits into from
Feb 12, 2019

Conversation

errordeveloper
Copy link
Contributor

@errordeveloper errordeveloper commented Feb 12, 2019

Description

This fixes #518, which is a bug that got introduced as part of #513.

Checklist

  • Code compiles correctly (i.e make build)
  • All unit tests passing (i.e. make test)
  • All integration tests passing (i.e. make integration-test)

switch topology {
case SubnetTopologyPrivate:
subnets = c.VPC.Subnets.Private
if c.VPC.Subnets.Private == nil {
c.VPC.Subnets.Private = make(map[string]Network)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just trying to understand what was going on here.

The cause of the issue seems to be https://github.com/weaveworks/eksctl/pull/513/files#diff-9cf0956b5581521f18ebe4dedc1f1552L90 - we had been mistakenly throwing away the imported subnets when c.VPC.Subnets.Private or c.VPC.Subnets.Publicwas empty?

Copy link
Contributor Author

@errordeveloper errordeveloper Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for asking! I've looked into this some more, and actually have a better idea of what is going on.

I was assuming that maps are always treated as references, which is indeed the case. However, if you assign a nil map to another nil map, nothing will actually happen. Namely, maps are treated as references (well, sort of), but only after they were initialised. Take a look here: https://play.golang.org/p/OTCUWDoH9HK. I didn't realise this until now, or at least I forgot this.

Copy link
Contributor Author

@errordeveloper errordeveloper Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it's even simpler than this, any nil reference can be assigned to another one, but nothing gets copied really: https://play.golang.org/p/KFefMZEabWQ.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it sort of boils down to i1 = i2 being the same as i1 = nil, so the compiler cannot really tell these apart, hence it has no way of telling me that I'm probably doing something wrong... TIL :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't add nodegroup for existing cluster which created with old eksctl
2 participants