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

Rework network ACLs (phase 1) #360

Merged
merged 14 commits into from
Feb 28, 2023
Merged

Rework network ACLs (phase 1) #360

merged 14 commits into from
Feb 28, 2023

Conversation

vburckhardt
Copy link
Member

@vburckhardt vburckhardt commented Feb 13, 2023

Description

Initial step in the right direction #357

In this PR:

  • give option to append or prepend ibm rules
  • add a deny all as last element of the list (this is a best practice, and not mandatory as implicit - but absence typically raise questions)
  • add prefix "ibmflow-" to ibm rules
  • refactor: correct naming of some local variable that were referencing kube

Types of changes in this PR

#357

No release required

  • Examples or tests (addition or updates of examples or tests)
  • Documentation update
  • CI-related update (pipeline, etc.)
  • Other changes that don't affect Terraform code

Release required

  • Bug fix (patch release (x.x.X): Change that fixes an issue and is compatible with earlier versions)
  • New feature (minor release (x.X.x): Change that adds functionality and is compatible with earlier versions)
  • Breaking change (major release (X.x.x): Change that is likely incompatible with previous versions)
Release notes content

Replace this text with information that users need to know about the bug fixes, features, and breaking changes. This information helps the merger write the commit message that is published in the release notes for the module.


Checklist for reviewers

  • The PR references a GitHub issue.
  • 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

  • Merge by using "Squash and merge".

  • 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).

@vburckhardt vburckhardt changed the title [DRAFT] Rework network ACLs Rework network ACLs (phase 1) Feb 28, 2023
network_acls.tf Outdated Show resolved Hide resolved
SirSpidey
SirSpidey previously approved these changes Feb 28, 2023
Copy link
Member

@SirSpidey SirSpidey left a comment

Choose a reason for hiding this comment

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

👍 name change looks good to me

Copy link
Member

@rajatagarwal-ibm rajatagarwal-ibm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@SirSpidey SirSpidey left a comment

Choose a reason for hiding this comment

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

👍

@vburckhardt vburckhardt merged commit 6148fc2 into main Feb 28, 2023
@vburckhardt vburckhardt deleted the exp-networkacl branch February 28, 2023 17:16
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@vbontempi
Copy link
Member

@vburckhardt the PR looks correct to me. Just a consideration about its usage: the current logic wouldn't allow a use-case with customer rules preceded by some ibm internal and vpc rules and followed by some other ibm internal and vpc rules.
I am not sure it could be a valid use-case for customers, to allow such a configuration would require to:

  • add append_ibm_rules boolean flag
  • add two different lists of rules, something like to_append_rules and to_prepand_rules (with a better naming)

@vburckhardt
Copy link
Member Author

@vbontempi - Yes that's correct. I kept it simple for now based on discussion with a few consumers. We can always enhance as needed if the requirement arise in future.

gmendel pushed a commit to gmendel/landing-zone-vpc that referenced this pull request Mar 29, 2023
* feat: give option to append or prepend ibm rules
* feat: add a deny all as last element of the list (this is a best practice, and not mandatory as implicit - but absence typically raise questions)
* feat: add prefix "ibmflow-" to ibm rules

BREAKING CHANGE: The interface of the `network_acls` input variable has changed. If your code is setting this variable explicitly, this change requires to add a few extra optional parameters: `add_ibm_cloud_internal_rules`, `add_vpc_connectivity_rules`, `prepend_ibm_rules` . The parameter `add_cluster_rules` has been renamed `add_ibm_cloud_internal_rules`
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