cloudposse / terraform-aws-ec2-client-vpn

https://cloudposse.com/accelerate
Apache License 2.0
45 stars 28 forks source link

Self Signed Certs all have hardcoded names, not allowing for multiple Client VPNs in a region #24

Closed garrinmf closed 2 years ago

garrinmf commented 2 years ago

what

The certificate names are all hardcoded, not allowing modification via context.

why

In order to have multiple Client VPNs in the same region, the keys stored in SSM need to be unique, the hardcoded approach doesn't allow this.

I really don't know best practices around backwards compatibility or naming in general, just that the way it is doesn't allow for specifying unique names for the stored SSM keys. let me know if a different approach would work better.

references

nitrocode commented 2 years ago

/test all

nitrocode commented 2 years ago

@garrinmf please review the terratest failure if you get a chance

garrinmf commented 2 years ago

@nitrocode It doesn't seem like the self signed cert module, https://github.com/cloudposse/terraform-aws-ssm-tls-self-signed-cert, knows how to look to the attributes. So all the keys are writing to the same SSM parameter name, in the test case it's example.pem as example is the module.this.name. I pulled it and manually tried it locally and if I don't specify context (with a name) the keyname is actually just .pem. If I do specify context with a name they're named the same, not taking into account attributes. Not sure if this needs fixed downstream or a different approach in this module with_context default ?

garrinmf commented 2 years ago

@nitrocode Is there another approach, besides the attributes, that works better than my original attempt but the downstream packages work with?

josh-onchain commented 2 years ago

+1

nitrocode commented 2 years ago

/test test/terratest

mergify[bot] commented 2 years ago

This pull request is now in conflict. Could you fix it @garrinmf? 🙏

nitrocode commented 2 years ago

/test all

nitrocode commented 2 years ago

/test all