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

feat: Enable creating network acls for disjoint ip address spaces #542

Merged
merged 12 commits into from
Jun 14, 2023

Conversation

Aashiq-J
Copy link
Member

@Aashiq-J Aashiq-J commented May 23, 2023

Description

case where the address space is disjoint (eg: multiple vpcs and on-prem direct link connected to same transit gateway) - it is not possible to have one single cidr covering all address space.
Loop through the list of all the network_cidr and create acl rules.

Types of changes in this PR

Issue : #536

Changes that affect the core Terraform module or submodules

  • Bug fix
  • New feature
  • Dependency update

Changes that don't affect the core Terraform module or submodules

  • Examples or tests (addition or updates of examples or tests)
  • Documentation update
  • CI-related update (pipeline, etc.)
  • Other

Release required?

Identify the type of release. For information about the changes in a semantic versioning release, see Release versioning.

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content
  • Change for variable name network_cidr to network_cidrs.
  • Ability to pass multiple network_cidrs as a list.
  • Generation of acl rules based on the list of network_cidrs.

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

Merge actions for mergers

  • Use a relevant conventional commit message that is based on the PR contents and any release notes provided by the PR author. The commit message determines whether a new version of the module is needed, and if so, which semver increment to use (major, minor, or patch).
  • Merge by using "Squash and merge".

@Aashiq-J Aashiq-J marked this pull request as ready for review May 24, 2023 10:21
network_acls.tf Outdated Show resolved Hide resolved
@Aashiq-J Aashiq-J requested a review from akocbek May 29, 2023 18:15
@Aashiq-J
Copy link
Member Author

Aashiq-J commented Jun 7, 2023

/run pipeline

@vburckhardt
Copy link
Member

This looks good. Thanks for the PR. Could you just put a screenshot of the network acl from the example where there are 2 values in the network_cidrs input.

@vburckhardt
Copy link
Member

/run pipeline

@Aashiq-J
Copy link
Member Author

@vburckhardt
image
image

@Aashiq-J
Copy link
Member Author

/run pipeline

@jojustin jojustin merged commit 6718c01 into main Jun 14, 2023
@jojustin jojustin deleted the disjoint branch June 14, 2023 05:01
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 7.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@vburckhardt
Copy link
Member

vburckhardt commented Jun 14, 2023

@Aashiq-J @jojustin - The allow on any IPs are not expected here. This would opened up too broadly the acls. Is this with the default configuration?

The network acls should be set as follows:

inbound:
each of the network_cidr -> each of the vpc address prefix

outbound:
each of the vpc address prefix -> each of the network_cidr

@jojustin
Copy link
Member

Tx @vburckhardt for taking a look at it. I merged as it was approved. I should have checked the changes too. Will take care of it going forward.

@Aashiq-J can you please take a look at the above and fix it.

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

Successfully merging this pull request may close these issues.

None yet

6 participants