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

implement support for Partner Interconnect #345

Conversation

gaspar-chilingarov
Copy link
Contributor

  • add 3-networks/modules/partner_interconnect module
  • update documentation
  • provide partner_interconnect.tf.example modules for each environment,
    including hub_and_spoke mode.

- add 3-networks/modules/partner_interconnect module
- update documentation
- provide partner_interconnect.tf.example modules for each environment,
  including hub_and_spoke mode.
@@ -0,0 +1,4 @@

enable_partner_interconnect = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these be default values for the variables, rather than something that needs to be tfvars?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

default is false :P so no Partnet IC by default :) just renaming parnet_interconnect.tf.example is not enough, as we need value changes in primary main.tf.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok I understand better now. Why not just set these values when you call the partner interconnect module? (in example files) - the less tfvars files that need to be managed, the better IMHO and you can customise these per environment in the actual code if you want.

Also FYI, for the other tfvars files they have been sym linked to this folder so you only need to update them once. https://github.com/gaspar-chilingarov/terraform-example-foundation/tree/feature/add-partner-interconnect-example/3-networks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not just set these values when you call the partner interconnect module? (in example files)

Because I need to feed different ASN value in the main.tf depending if this needs to support patner interconnect or not. I cannot set variables from the .tf file. At least no non-hacky ways I'm aware of.

other tfvars files they have been sym linked to this folder
Well, interconnect configuration (or even presence/absense) cound be different per environment.

Also there is a reason why we don't do it with .example files - as the target of the symlink should be always in place. (so user must rename file in the top directory and then also symlink in all environments)

so straightforward solution is to decide not to use .example as we do now, move all of them into normal .tf files and add count = enabled_{interconnect|partner|vpn} condition. In that way we have don't have variance in TF code which is executed, just some modules/resources will not be instantiated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I think we can get rid of the example files and just have a condition now, because we will be on terraform 0.13+ - so I agree that is a good idea.

Would you like to do this in a follow up PR?

@bharathkkb FYI

Copy link
Member

Choose a reason for hiding this comment

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

sgtm

add count = enabled_{interconnect|partner|vpn} condition

maybe instead of bool flags we can condense them into a single string input and switch base on string content interconnect|partner|vpn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, if we want to integrate VPN/Dedicated (need rework anyway, as configurations are wrong)/Partner - sure, we can do that in separate PR

Copy link
Collaborator

@rjerrems rjerrems left a comment

Choose a reason for hiding this comment

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

LGTM - Only one minor nit about tfvars files.

@rjerrems
Copy link
Collaborator

rjerrems commented Mar 2, 2021

/gcbrun

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.

None yet

3 participants