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

Add name overrides for Internal and External Load Balancers #2420

Merged

Conversation

cvanwijck-hub24
Copy link
Contributor

When developing IaC for existing infrastructure and attempting to achieve parity between the two, the current naming assignments of multiple modules in CFF causes issues.

For example in net-lb-ext the name of the load balancer added additional items to the name if a forwarding_rules_config was passed.

name = (
  each.key == "" ? var.name : "${var.name}-${each.key}"
)

This can cause issues where a specific name is preferred, such as when bringing existing infrastructure under the control of IaC. Changing the name of a load balancer, backend service or health check would cause them to be replaced by Terraform.

This is also relevant to #2419 where peerings have a name set by combining the local and peering network names with a prefix. A similar override_name variable would resolve this issue.

Adding an optional override_name variable allows the user to specify a name if the generated name is not helpful for their use case. For users who would like to use the module as before and have names generated as before, leaving the override_name as null will use the default name assignment.


Checklist

I acknowledge that I have:

  • Read the contributing guide
  • Ran terraform fmt on all modified files
  • Regenerated the relevant README.md files using tools/tfdoc.py
  • Made sure all relevant tests pass

Copy link

google-cla bot commented Jul 11, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

modules/net-lb-ext/main.tf Outdated Show resolved Hide resolved
@ludoo
Copy link
Collaborator

ludoo commented Jul 11, 2024

It's probably easier if you run checks locally without waiting for the workflow to kick in. Details are in the contributing guide in the root of the repo. :)

Copy link
Collaborator

@wiktorn wiktorn left a comment

Choose a reason for hiding this comment

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

I think that to be fully comprehensive, this requires allowing additional overrides in net-lb-ext for names of:

  • google_compute_region_backend_service
  • google_compute_service_attachment

modules/net-lb-int/variables.tf Outdated Show resolved Hide resolved
@cvanwijck-hub24
Copy link
Contributor Author

I think that to be fully comprehensive, this requires allowing additional overrides in net-lb-ext for names of:

* google_compute_region_backend_service

* google_compute_service_attachment

If "fully comprehensive" is the goal, this could be expanded to adding override names across multiple other modules. This would also partially resolve #2419. We have encountered this across many areas of our deployment in taking existing click-ops configured infrastructure under control of Terraform but trying to use CFF to accelerate development. Overrides would be useful for many brownfield deployments.

modules/net-vpc-peering/variables.tf Outdated Show resolved Hide resolved
modules/net-vpc-peering/main.tf Outdated Show resolved Hide resolved
modules/net-vpc-peering/main.tf Outdated Show resolved Hide resolved
@juliocc
Copy link
Collaborator

juliocc commented Jul 31, 2024

This PR is getting messy. I extracted the changes to the peering module to #2459.

@juliocc
Copy link
Collaborator

juliocc commented Aug 6, 2024

@cvanwijck-hub24, just checking in to see if you plan on finishing this PR. If not, we'll go ahead and close it

@coopervw
Copy link

coopervw commented Aug 6, 2024

@cvanwijck-hub24, just checking in to see if you plan on finishing this PR. If not, we'll go ahead and close it

Yes I will resolve the outstanding issues this week if possible.

Copy link
Contributor Author

@cvanwijck-hub24 cvanwijck-hub24 left a comment

Choose a reason for hiding this comment

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

All use coalesce and changed override_name to use just name for clarity

modules/net-lb-ext/README.md Outdated Show resolved Hide resolved
modules/net-lb-ext/variables.tf Outdated Show resolved Hide resolved
modules/net-vpn-ha/variables.tf Outdated Show resolved Hide resolved
Copy link
Collaborator

@juliocc juliocc left a comment

Choose a reason for hiding this comment

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

lgtm now. Thanks for your patience @cvanwijck-hub24

@juliocc juliocc merged commit 3cf8889 into GoogleCloudPlatform:master Aug 16, 2024
14 checks passed
@cvanwijck-hub24 cvanwijck-hub24 deleted the cvanwijck-hub24/override_name branch December 17, 2024 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants