hashicorp / terraform-aws-hashicorp-vault-eks-addon

HashiCorp Vault Add-on for AWS EKS
https://www.hashicorp.com/blog/vault-and-aws-partner-to-enhance-kubernetes-security
Apache License 2.0
23 stars 14 forks source link

feat: Update example to match current EKS blueprints, remove experimental feature to allow users to use Terraform 1.3+ #8

Closed bryantbiggs closed 2 years ago

bryantbiggs commented 2 years ago
bryantbiggs commented 2 years ago

@ksatirli would you mind taking a look when you get a chance so we can resolve the upstream issue related to Terraform v1.3+

ksatirli commented 2 years ago

@bryantbiggs these look great on first look, I'll take an in-depth look in the morning.

Excited about the changes you're proposing (with vault_namespace being the only thing that immediately stood out as a case of unfortunate naming)

bryantbiggs commented 2 years ago

Thanks @ksatirli - I've updated to change vault_namespace to simply namespace in the example. I also removed the version pin due to a cyclical conflict between external modules and the Blueprints repository/version

ksatirli commented 2 years ago

Thanks @ksatirli - I've updated to change vault_namespace to simply namespace in the example. I also removed the version pin due to a cyclical conflict between external modules and the Blueprints repository/version

Thanks - These changes look great!

I vaguely remember there being a reason to not pin the Blueprints module (which is - as you point out - an anti-pattern), but I can't find the actual why.

bryantbiggs commented 2 years ago

Yes, I think the way that we have both the module definition for the helm_addon and the module implementation in the same repository coupled with the fact that we have external addons like Vault that also point to this addon and are referenced from within the same project, it creates a nasty cyclical loop. I will have a chat with the team to see if we can do something about moving the helm_addon to its own module repo to break this loop and then we can utilize versions better

ksatirli commented 2 years ago

@bryantbiggs I've tested this locally with 1.3.1 and found it working.

I've enabled tflint via #3, which caused a small merge conflict in blueprints/getting-started/main.tf - happy to resolve this or let you be the sole committer of this PR.

ksatirli commented 2 years ago

@bryantbiggs how do you feel about adding a note with some upstream information on why the modules aren't pinned to a specific version?

And simultaneously disabling tflint for that section (through # tflint-ignore: terraform_module_pinned_source)

ksatirli commented 2 years ago

Code looks great @bryantbiggs - the terraform-docs Action will be fixed in an upcoming PR.

Unless you need any more changes, I'm good with merging this.

bryantbiggs commented 2 years ago

Thanks @ksatirli - all good from my end so please feel free when you're ready

We do have a plan to fix this cyclical loop - once we have that fleshed out I'll be back to update so we can show pinned versions for better stability and user experience. stay tuned! Thank you again!

ksatirli commented 2 years ago

Time to :shipit:.

Thank you once again, @bryantbiggs