-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
aws: Create subnets for additional network CIDRs #15805
aws: Create subnets for additional network CIDRs #15805
Conversation
5b8e669
to
f63bdaa
Compare
/cc @justinsb |
/assign @justinsb |
@@ -300,7 +299,7 @@ func NewCmdCreateCluster(f *util.Factory, out io.Writer) *cobra.Command { | |||
cmd.RegisterFlagCompletionFunc("subnets", completeSubnetID(options)) | |||
cmd.Flags().StringSliceVar(&options.UtilitySubnetIDs, "utility-subnets", options.UtilitySubnetIDs, "Shared utility subnets to use") | |||
cmd.RegisterFlagCompletionFunc("utility-subnets", completeSubnetID(options)) | |||
cmd.Flags().StringVar(&options.NetworkCIDR, "network-cidr", options.NetworkCIDR, "Network CIDR to use") | |||
cmd.Flags().StringSliceVar(&options.NetworkCIDRs, "network-cidr", options.NetworkCIDRs, "Network CIDR(s) to use") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to make this change here as well - https://github.com/kubernetes/kops/blob/master/tests/e2e/kubetest2-kops/deployer/up.go#L112?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any mention of network-cidr
there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, kubetest2 uses that interface which I linked earlier and we would need to provide this flag there as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can pass them via the "passthrough" CreateArgs:
kops/tests/e2e/kubetest2-kops/deployer/up.go
Lines 126 to 135 in 9e0db3d
if d.CreateArgs != "" { | |
if strings.Contains(d.CreateArgs, "arm64") { | |
isArm = true | |
} | |
createArgs, err := shlex.Split(d.CreateArgs) | |
if err != nil { | |
return err | |
} | |
args = append(args, createArgs...) | |
} |
@@ -300,7 +299,7 @@ func NewCmdCreateCluster(f *util.Factory, out io.Writer) *cobra.Command { | |||
cmd.RegisterFlagCompletionFunc("subnets", completeSubnetID(options)) | |||
cmd.Flags().StringSliceVar(&options.UtilitySubnetIDs, "utility-subnets", options.UtilitySubnetIDs, "Shared utility subnets to use") | |||
cmd.RegisterFlagCompletionFunc("utility-subnets", completeSubnetID(options)) | |||
cmd.Flags().StringVar(&options.NetworkCIDR, "network-cidr", options.NetworkCIDR, "Network CIDR to use") | |||
cmd.Flags().StringSliceVar(&options.NetworkCIDRs, "network-cidr", options.NetworkCIDRs, "Network CIDR(s) to use") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion would be to add a new flag called additional-network-cidrs
to be backward compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is already backwards compatible. Nothing changes from user point of view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, might be missing something here but won't going from StringVar
to StringSliceVar
require users to make a change to pass a slice of strings now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can pass a slice as multiple --network-cidr
flags or comma separated.
For example, with this change, one can just do:
--network-cidr=10.0.0.0/16 --network-cidr=10.1.0.0/16 --network-cidr=10.2.0.0/16 --network-cidr=10.3.0.0/16 --network-cidr=10.4.0.0/16 --network-cidr=10.5.0.0/16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it's a nice feature of the flags library!
if len(opt.NetworkCIDRs) > 1 { | ||
for i, cidr := range opt.NetworkCIDRs[1:] { | ||
zoneName := opt.Zones[i%len(opt.Zones)] | ||
subnetName := fmt.Sprintf("%s-%d", zoneName, i+1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is pretty specific to AWS. Also the way we round-robin around the zones is convenient, but might be surprising (along with the numbering).
Should we feature-flag this feature (and/or restrict to AWS only at first)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional network CIDRs are already restricted to AWS in validation, so it should not be a problem.
Round-robin may be surprising, but could not think of any other behaviour when adding multiple network cirds and multiple zones. In any case, I doubt many people will create clusters with multiple network cidrs. It's quite an edge case. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One possibility would be to put the -%d
suffix on the first subnet as well when len(opt.NetworkCIDRs) > 1
but on AWS the primary CIDR of a VPC does have slightly different semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about the slightly different semantics. This is why I preferred to have different names for additional network CIDRs.
- cidr: 10.1.0.0/16 | ||
name: us-test-1a-1 | ||
type: Public | ||
zone: us-test-1a | ||
- cidr: 10.2.0.0/16 | ||
name: us-test-1a-2 | ||
type: Public | ||
zone: us-test-1a | ||
- cidr: 10.3.0.0/16 | ||
name: us-test-1a-3 | ||
type: Public | ||
zone: us-test-1a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking this test should have a second zone. And we should have a private_complex test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, done.
/lgtm Holding off on approval to give opportunity to add those additional test cases. I would be okay with this going in as-is. |
60c656d
to
f41e93f
Compare
f41e93f
to
35e7bba
Compare
Thanks @hakman We discussed in office hours, this is good to go I think, my concern was whether we should have a feature flag. @johngmyers agreed with you that we didn't, so... /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This will be needed for scale tests, to be able to use clusters with more than 500 nodes.