This repository was archived by the owner on Feb 5, 2020. It is now read-only.
AWS: VPC subnets with custom CIDRs and AZs per workers / masters #267
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
s-urbaniak
reviewed
Apr 20, 2017
Contributor
s-urbaniak
left a comment
s-urbaniak
left a comment
There was a problem hiding this comment.
I think those interpolations in their current state are not that bad, I would add some documentation around them.
modules/aws/vpc/vpc-private.tf
Outdated
|
|
||
| cidr_block = "${lookup(var.worker_subnets, | ||
| "${element(concat(keys(var.worker_subnets), list("padding")), count.index)}", | ||
| "${cidrsubnet(data.aws_vpc.cluster_vpc.cidr_block, 4, count.index + var.worker_az_count)}") |
Contributor
There was a problem hiding this comment.
The above interpolation (and the following ones) is pretty complex. I suggest we add a comment explaining what this does, especially the list("padding") workaround which makes the element function happy in case worker_subnets is empty and the cidrsubnet logic.
Contributor
Author
There was a problem hiding this comment.
My plan is to move these expressions on the module inputs.
It would spread out the verbosity and make the resources read more cleanly.
I will then add documentation on the module inputs, which would all be in one place.
modules/aws/vpc/vpc-private.tf
Outdated
|
|
||
| availability_zone = "${ "${length(keys(var.worker_subnets))}" > 0 ? | ||
| "${element(concat(keys(var.worker_subnets), list("padding")), count.index)}" : | ||
| "${data.aws_availability_zones.azs.names[count.index]}" |
Contributor
There was a problem hiding this comment.
The same applies here, I suggest we add a doc comment above.
alexsomesan
force-pushed
the
custom-az-cidrs
branch
from
e22bc79 to
23539b9
Compare
s-urbaniak
approved these changes
Apr 20, 2017
Contributor
s-urbaniak
left a comment
s-urbaniak
left a comment
There was a problem hiding this comment.
terraform wizardry, but well documented, LGTM, thanks!
Contributor
Author
|
I renamed the top-level subnet input variables to explicitly mention "custom". |
wking
added a commit
to wking/openshift-installer
that referenced
this pull request
Nov 18, 2018
This functionality was originally from 8324c21 (AWS: VPC subnets with custom CIDRs and AZs per workers / masters, 2017-04-20, coreos/tectonic-installer#267), but we haven't exposed it in openshift-install (which has never used the parsers removed by f7a4e68, pkg/types/config: Drop ParseConfig and other Parse* methods, 2018-10-02, openshift#403). Currently we are unable to scale masters post-install, because auto-scaling etcd is difficult. Depending on how long that takes us to get working, we may need to re-enable this for masters later. Workers are already managable via the cluster API and MachineSets, so folks who need custom worker subnets can create a cluster without workers and then launch their worker machine-sets directly as a day-2 operation. The cluster-API type chain is: * MachineSet.Spec [1] * MachineSetSpec.Template [2] * MachineTemplateSpec.Spec [3] * MachineSpec.ProviderConfig [4] * ProviderConfig.Value [5] * RawExtension which is nice and generic, but a dead-end for structured configuration ;). Jumping over to the OpenShift AWS provider, there is an AWSMachineProviderConfig.Subnet [6]. I don't see code for auto-creating those subnets, but an admin could manually create the subnet wherever they wanted and then use the cluster API to launch new workers into that subnet. And maybe there will be generic tooling to automate that subnet creation (setting up routing, etc.) to make that less tedious/error-prone. Also in this space, see [7,8] [1]: https://github.com/kubernetes-sigs/cluster-api/blob/0734939e05aeb64ab198e3feeee8b4e90ee5cbb2/pkg/apis/cluster/v1alpha1/machineset_types.go#L42 [2]: https://github.com/kubernetes-sigs/cluster-api/blob/0734939e05aeb64ab198e3feeee8b4e90ee5cbb2/pkg/apis/cluster/v1alpha1/machineset_types.go#L68-L71 [3]: https://github.com/kubernetes-sigs/cluster-api/blob/0734939e05aeb64ab198e3feeee8b4e90ee5cbb2/pkg/apis/cluster/v1alpha1/machineset_types.go#L84-L87 [4]: https://github.com/kubernetes-sigs/cluster-api/blob/0734939e05aeb64ab198e3feeee8b4e90ee5cbb2/pkg/apis/cluster/v1alpha1/machine_types.go#L62-L64 [5]: https://github.com/kubernetes-sigs/cluster-api/blob/0734939e05aeb64ab198e3feeee8b4e90ee5cbb2/pkg/apis/cluster/v1alpha1/common_types.go#L29-L34 [6]: https://github.com/openshift/cluster-api-provider-aws/blob/e6986093d1fbac2084c50b04fe2f78125ffca582/pkg/apis/awsproviderconfig/v1alpha1/awsmachineproviderconfig_types.go#L130-L131 [7]: kubernetes/kops#1333 [8]: https://github.com/kubernetes-sigs/cluster-api/blob/0734939e05aeb64ab198e3feeee8b4e90ee5cbb2/pkg/apis/cluster/v1alpha1/cluster_types.go#L62-L82
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change allows the VPC module to create subnets in arbitrary subsets of availability zones and using per-subnet custom CIDR ranges.
Resolves #215 and #214
cc: @s-urbaniak @Quentin-M @sym3tri