search

RaJiska / terraform-aws-fck-nat

Terraform module for fck-nat
https://registry.terraform.io/modules/RaJiska/fck-nat/aws/latest
MIT License
74 stars 38 forks source link

Added support for SSM connections #10

Closed kieranbrown closed 8 months ago

kieranbrown commented 10 months ago

Unsure if you want this configurable by a variable? Considering SSM may become the only method to connect by default (https://github.com/AndrewGuenther/fck-nat/pull/48) I figured there was no harm in attaching the SSM policy by default.

For now, this will only be relevant for users building their own AMI based on your PR to add SSM support.

RaJiska commented 9 months ago

Thanks for the PR @kieranbrown . Indeed, adding SSM permissions would be necessary once this becomes the default in the upstream fck-nat project. However I am a bit uncomfortable with using the AWS' managed policy for this project as the policy has too many permissions.

When SSM becomes the default, I think it would be better either to make a trimmed policy containing only the IAM actions for SSM-messages required for remote shell, or alternatively at the very least exposing a variable controlling the policy ARN to let the user chose a policy that suit their needs.

kieranbrown commented 9 months ago

@RaJiska I've updated the PR to use least privilege and added a variable to disable for those who don't want it.

There are a few approaches we can take for those who have more complex requirements, personally I'm leaning towards outputting a role_id or role_name and allowing users to attach managed policies or inline policies themselves outside of the module - I know the role name is just var.name but it saves people needing to use depends_on to wait the for role creation if they can reference it directly.

Curious to hear your thoughts on this, I'm happy to work on it in a separate PR as I don't think it's absolutely necessary for this to be merged.

RaJiska commented 8 months ago

LGTM, thanks for the contribution @kieranbrown .

I think outputting the role name and ARN is definitely a good idea regardless of whether the user wishes to add policies or not as the end user may make alternative use of it (e.g: storing in parameter store).

Though I'm more for adopting a minimalist approach in regards of the number of variables exposed to the user as in my opinion, the goal of this project is to mimic NAT Gateway, and make this as dumb as possible, therefore removing as much complexity as possible. In the future I may wish to remove the attach_ssm_policy variable and just have it enabled by default and make SSH off by default.