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

fix: convert default sg/acl rule cleaning to provider option #621

Merged
merged 7 commits into from
Sep 13, 2023

Conversation

toddgiguere
Copy link
Contributor

@toddgiguere toddgiguere commented Sep 7, 2023

Description

Use new VPC resource provider option to clean rules from default VPC security group and ACL, retire the workaround scripts.

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

IBM Cloud Terraform provider v1.56.0 has added a new option for the "ibm_is_vpc" resource that will cause the default VPC ACL and Security Group to contain no rules (empty).

This release will retire the usage of existing backend scripts to remove all rules from the VPC default ACL and SG, and instead use this new provider option to accomplish the same feature.

Input variable changes:
The new provider option covers both security group and ACL in one variable, so we will be deprecating the individual "clean_" variables and replace them with a new single boolean to enable the feature.

  • REMOVED: clean_default_security_group and clean_default_acl
  • ADDED: clean_default_sg_acl, if set to "true" will trigger new VPC option to have empty default groups (default is "false")
  • CHANGED: security_group_rules now has a default of empty, instead of a broad default inbound rule that may not be desired
  • Validation has been added to make sure that clean_default_sg_acl has not been set to "true" while having rules specified in the security_group_rules input, which are in direct conflict with each other

Upgrade Notes:
If you have already deployed module with the "clean_" variables not specified, you should see no difference after upgrade.

If you have already deployed module with "clean_" variables set to "true/false", you will get an error after upgrade due to those variables being removed, and you should set the new clean_default_sg_acl variable instead. During a plan phase after upgrading, you may see the following resources marked for DESTROY, this is expected as these are the retired scripts that handled this feature in the past, and have been removed:
module.slz_vpc.null_resource.clean_default_acl[0] will be destroyed
module.slz_vpc.null_resource.clean_default_security_group[0] will be destroyed

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

  • When merging, 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".

@toddgiguere
Copy link
Contributor Author

/run pipeline

@toddgiguere toddgiguere marked this pull request as ready for review September 8, 2023 20:20
@toddgiguere
Copy link
Contributor Author

/run pipeline

@toddgiguere
Copy link
Contributor Author

/run pipeline

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

@toddgiguere What are your thoughts on just removing the old vars all together and including it in the release notes? That way we don't have to come back with a follow up PR to remove them in the future. Either way, this will be a minor version release

@toddgiguere
Copy link
Contributor Author

Latest commit has the deprecated variables removed entirely, the default for security_group_rules has been set to empty array, and a new validation has been added to check if consumer has set the security group options in conflicting way.

@toddgiguere
Copy link
Contributor Author

/run pipeline

@toddgiguere
Copy link
Contributor Author

As anticipated, the UPGRADE test has failed due to the removal of the default security group rule that has in the past been added due to the default value of the input variable security_group_rules. Now that the default value has been removed and replaced with empty array, on upgrade the following resource is marked for destroy:

Resource(s) identified to be destroyed Name: default_vpc_rule Address: module.slz_vpc.ibm_is_security_group_rule.default_vpc_rule["default-sgr"] Actions: [delete]

This is the only resource marked by the upgrade test.

@toddgiguere
Copy link
Contributor Author

/run pipeline

1 similar comment
@toddgiguere
Copy link
Contributor Author

/run pipeline

@ocofaigh ocofaigh merged commit 753b89d into main Sep 13, 2023
2 checks passed
@ocofaigh ocofaigh deleted the update-clean-sg-acl branch September 13, 2023 15:28
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 7.5.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

3 participants