Skip to content

fix: update subnet naming to resource_name to allow safe prefix updates#1098

Closed
Khuzaima05 wants to merge 17 commits intomainfrom
issue-16771
Closed

fix: update subnet naming to resource_name to allow safe prefix updates#1098
Khuzaima05 wants to merge 17 commits intomainfrom
issue-16771

Conversation

@Khuzaima05
Copy link
Copy Markdown
Member

@Khuzaima05 Khuzaima05 commented Dec 10, 2025

Description

  1. Updates subnet and address-prefix resource naming to use a stable resource_name instead of names derived from the VPC prefix.
  2. This prevents unintended resource destruction/recreation when the prefix value is changed.
  3. Ensures Terraform state remains consistent and safe for prefix updates on existing VPCs.
  4. Includes migration support to map existing state to the new naming scheme.

issue: https://github.ibm.com/GoldenEye/issues/issues/16771

Release required?

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

This release introduces resource-based subnet naming, enabling safe prefix changes without forcing subnet recreation.

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.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@Khuzaima05 Khuzaima05 marked this pull request as ready for review December 11, 2025 17:42
@Khuzaima05 Khuzaima05 changed the title fix: fix subnet naming to resource_name to allow safe prefix updates fix: update subnet naming to resource_name to allow safe prefix updates Dec 11, 2025
@Khuzaima05
Copy link
Copy Markdown
Member Author

/run pipeline

@Khuzaima05
Copy link
Copy Markdown
Member Author

/run pipeline

@Khuzaima05 Khuzaima05 marked this pull request as draft December 12, 2025 08:57
@Khuzaima05 Khuzaima05 marked this pull request as ready for review January 7, 2026 10:44
@Khuzaima05
Copy link
Copy Markdown
Member Author

/run pipeline

@Khuzaima05
Copy link
Copy Markdown
Member Author

Testing logs:

I tested the script using the following steps:

  1. Ran terraform apply on the main branch with the prefix set to testvpc.
  2. Updated the prefix to testvpc1 and ran terraform plan on the same infrastructure. The plan indicated that some resources would be destroyed.
  3. Switched to my branch and executed the migration script, which updated the existing infrastructure to align with the latest changes.
  4. Ran terraform apply again with the new prefix (testvpc1). This time, no resources were marked for destruction.

VPC keys migration script logs.txt

@Khuzaima05 Khuzaima05 self-assigned this Jan 13, 2026
@Khuzaima05
Copy link
Copy Markdown
Member Author

Khuzaima05 commented Jan 13, 2026

Before

  • Resource keys were indirectly tied to var.prefix
  • Changing prefix → Terraform saw:
    - old keys removed
    - new keys added
  • Result: destroy & recreate resources

After

  • for_each uses a map keyed by resource_name (e.g. 1-subnet-a)
  • prefix is used only in the actual IBM Cloud resource name, not the Terraform key
  • Changing prefix now causes:
    - in-place name update
    - or no change at all
  • Terraform state keys remain unchanged → no destroy

Migration

  • A migration script renames existing Terraform state entries:
    <old-prefix>-vpc-subnet-a → 1-subnet-a
  • This updates state without touching real infrastructure
  • After migration, plans become clean even when prefix changes

@Khuzaima05
Copy link
Copy Markdown
Member Author

/run pipeline

@Khuzaima05
Copy link
Copy Markdown
Member Author

/run pipeline

@Khuzaima05
Copy link
Copy Markdown
Member Author

/run pipeline

@Khuzaima05
Copy link
Copy Markdown
Member Author

/run pipeline

1 similar comment
@Khuzaima05
Copy link
Copy Markdown
Member Author

/run pipeline

@Khuzaima05
Copy link
Copy Markdown
Member Author

Skipping the upgrade test because this change updates the Terraform keys used for subnets and address prefixes . it will not find the existing resources. Upgrade will fail.

Screenshot 2026-01-20 at 10 25 51 AM

@Khuzaima05
Copy link
Copy Markdown
Member Author

/run pipeline

@Khuzaima05
Copy link
Copy Markdown
Member Author

/run pipeline

@ocofaigh
Copy link
Copy Markdown
Contributor

@Khuzaima05 Hang on - we can't just skip the upgrade test. Isn't it flagging a valid breaking change? How are we going to handle this. Do we have migration?

@ocofaigh
Copy link
Copy Markdown
Contributor

ocofaigh commented Jan 20, 2026

@Khuzaima05 The scripts you added are for module consumers, but what about the DA? We have several DAs that use this module. We need to full understand the impact here. DA users are not going to know about the scripts you added. Plus we have several other external consumes like PowerVS, HPC etc etc. We need to find a clean way to do this.
In the past we had to use a pre-deploy ansible script for DAs. but its a lot of effort. I want to fully understand why we are making this breaking change after many years of using this code. Please add to the deep dive here so we can discuss.

@ocofaigh
Copy link
Copy Markdown
Contributor

Please also be aware that if this PR does proceed as a major version update, the VPN code needs to be removed from it since in #1041 we say it will be removed in v9 of the module

@Khuzaima05
Copy link
Copy Markdown
Member Author

@Khuzaima05 The scripts you added are for module consumers, but what about the DA? We have several DAs that use this module. We need to full understand the impact here. DA users are not going to know about the scripts you added. Plus we have several other external consumes like PowerVS, HPC etc etc. We need to find a clean way to do this. In the past we had to use a pre-deploy ansible script for DAs. but its a lot of effort. I want to fully understand why we are making this breaking change after many years of using this code. Please add to the deep dive here so we can discuss.

@ocofaigh , sure I will add this topic in tomorrow’s deep dive.

@Khuzaima05
Copy link
Copy Markdown
Member Author

As discussed in the deep dive, fixing this issue now would take a lot of effort and coordination because the VPC module is used by many DAs and other consumers. The impact of a breaking change would be too large.

Next steps:

  • Find all repositories and DAs that use the VPC module and make sure this limitation is clearly mentioned.

  • Clearly document this limitation in the prefix variable description so users know what to expect.

@Khuzaima05
Copy link
Copy Markdown
Member Author

Closing this PR, as per deep dive discussion.

@Khuzaima05 Khuzaima05 closed this Jan 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants