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

Improve subnet CIDR calculation #23

Closed
wants to merge 8 commits into from
Closed

Improve subnet CIDR calculation #23

wants to merge 8 commits into from

Conversation

balazsgaspar
Copy link
Contributor

Currently, the prereq modules configure the vpc/cnet submodules in a way that the total VPC/VNet CIDR range is split up equally between the number of subnets created.

While this is a sound approach, it results in a networking layout that does not align with our recommended network settings (and the current "Create new VPC" feature of CDP) and may be a source of confusion. See also https://docs.cloudera.com/cdp-public-cloud/cloud/requirements-aws/topics/mc-aws-req-vpc.html

We should implement a logic where the user can select the size of the public/private (internal/gateway) subnets and the module calculates each subnet's CIDR range accordingly.

Defaults subnet sizes will be set according to the linked documentation (/19 for internal/private and /24 for public/CDP subnets)

Copy link
Contributor

@jimright jimright left a comment

Choose a reason for hiding this comment

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

@balazsgaspar - thanks for taking care of this.

Just a small change required to add the new variables to the top-level pre-req module so that they are exposed to the user.

Also can you run terraform fmt -recursive to format all Terraform source files. The GitHub actions are failing on this.

@balazsgaspar
Copy link
Contributor Author

terraform fmt -recursive

Done!

@jimright jimright self-requested a review August 15, 2023 20:04
Copy link
Contributor

@jimright jimright left a comment

Choose a reason for hiding this comment

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

Tested CDP deployment with these changes to public and private subnets. All deployment patterns work well. Also confirmed that the change has no impact on the BYO-VPC/VNet configuration.

Approved.

@jimright
Copy link
Contributor

@balazsgaspar - this PR is all tested and ready to be merged.

However the merge is blocking as we need DCO signoff is needed on all commits. Details of how to do this are at: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

@wmudge wmudge changed the title Merge CDPAUTO-102 Improve subnet CIDR calculation Aug 29, 2023
@wmudge wmudge added the enhancement New feature or request label Aug 29, 2023
@jimright
Copy link
Contributor

jimright commented Sep 7, 2023

Closing without merging as functionality was added as part of #29

@jimright jimright closed this Sep 7, 2023
@jimright jimright deleted the CDPAUTO-102 branch September 26, 2023 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants