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

Add --vpc-cidr flag #286

Merged
merged 3 commits into from
Oct 26, 2018
Merged

Add --vpc-cidr flag #286

merged 3 commits into from
Oct 26, 2018

Conversation

errordeveloper
Copy link
Contributor

@errordeveloper errordeveloper commented Oct 26, 2018

Description

User-visible feature: --vpc-cidr flag.

In terms of code, this follows up on #283, and it does two main things:

  • automate splitting of subnets
  • create private subnets as well as public

And a little more refactoring, as most of #283 was theoretical - so here come few practical tweaks.

Fork of kops is temporary, until kubernetes/kops#6004 lands.

Checklist

  • Code compiles correctly (i.e make build)
  • Added tests that cover your change (if possible)
  • All tests passing (i.e. make test)
  • Added/modified documentation as required (such as the README) (will update separately along with other doc updates)

stefanprodan
stefanprodan previously approved these changes Oct 26, 2018
Copy link
Contributor

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM! some small typos to fix.

Network // global CIRD and VPC ID
SecurityGroup string // cluster SG
// subnets are either public or private for use with separate nodegroups
// these are keyed by AZ for conveninece

This comment was marked as abuse.

)

const (
// SubnetTopologyPrivate repesents privately-routed subnets

This comment was marked as abuse.

const (
// SubnetTopologyPrivate repesents privately-routed subnets
SubnetTopologyPrivate SubnetTopology = "Private"
// SubnetTopologyPublic repesents publicly-routed subnets

This comment was marked as abuse.

}
}

// HasSufficientPublicSubnets validates if there is a suffiecent

This comment was marked as abuse.

This comment was marked as abuse.

return len(c.SubnetIDs(SubnetTopologyPublic)) >= 3
}

// HasSufficientPrivateSubnets validates if there is a suffiecent

This comment was marked as abuse.

pkg/eks/vpc.go Outdated

zonesTotal := len(c.Spec.AvailabilityZones)
if 2*zonesTotal > len(zoneCIDRs) {
return fmt.Errorf("insuffience number of subnets (have %d, but need %d) for %d availability zones", len(zoneCIDRs), 2*zonesTotal, zonesTotal)

This comment was marked as abuse.

@errordeveloper
Copy link
Contributor Author

I'm running a manual test and an integration test at the moment, will merge if all goes well.

Copy link
Contributor

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

Looks much more clean compared to before, just found some typos in the comments

type (
// ClusterVPC holds global subnet and all child public/private subnet
ClusterVPC struct {
Network // global CIRD and VPC ID

This comment was marked as abuse.

// subnets are either public or private for use with separate nodegroups
// these are keyed by AZ for convenience
Subnets map[SubnetTopology]map[string]Network
// for additional CIRD associations, e.g. to use with separate CIDR for

This comment was marked as abuse.

}

// HasSufficientPublicSubnets validates if there is a sufficient
// number of subnets available to create a cluster

This comment was marked as abuse.

}

// HasSufficientPrivateSubnets validates if there is a sufficient
// number of subnets available to create a cluster

This comment was marked as abuse.

@errordeveloper
Copy link
Contributor Author

errordeveloper commented Oct 26, 2018

@stefanprodan @gruebel thanks for looking over this! I've added some basic validation of prefix length, as it turned you cannot used anything you like, AWS VPC only allows /16 - /24. Overall, it looks like we are good to go and the integration test was positive.

- validate given CIDR prefix for VPC requirement
- automate splitting of subnets
- create private subnets as well as public
- ensure DNS IP gets set correctly when any 10.0.0.0/8 subnet is used
@errordeveloper errordeveloper merged commit 43771c6 into master Oct 26, 2018
@errordeveloper errordeveloper deleted the vpc-cidr branch October 26, 2018 15:20
torredil pushed a commit to torredil/eksctl that referenced this pull request May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants