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

feat: Support for encrypted root disk in node_groups #1428

Merged

Conversation

alzabo
Copy link
Contributor

@alzabo alzabo commented Jun 4, 2021

PR o'clock

Description

Add encryption config to node_group launch_group ebs config block.

I'm open to any thoughts about what the keys should be named. I chose to preface with disk_ for consistency with the other related keys.

Checklist

@jmorgan415
Copy link

+1 for the need for this feature. For some reason, the managed node group launch templates are lacking quite a few options compared to the self-managed worker launch templates, with disk encryption and metadata options being the most glaring. I'll submit a PR today for the metadata options as this PR covers the disk encryption quite nicely (I tested successfully on a fork of this repo).

@dracut5
Copy link

dracut5 commented Jul 5, 2021

Would be nice to get this functionality, in other case launch template need to be created separately.

Comment on lines 26 to 27
| disk\_encrypted | Whether the root disk will be encrypyted. Require `create_launch_template` to be `true` and require `disk_kms_key_id` to be set | bool | false |
| disk\_kms\_key\_id | KMS Key used to encrypt the root disk. Require both `create_launch_template` and `disk_encrypted` to be `true` | string | "" |

Choose a reason for hiding this comment

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

Suggested change
| disk\_encrypted | Whether the root disk will be encrypyted. Require `create_launch_template` to be `true` and require `disk_kms_key_id` to be set | bool | false |
| disk\_kms\_key\_id | KMS Key used to encrypt the root disk. Require both `create_launch_template` and `disk_encrypted` to be `true` | string | "" |
| disk\_encrypted | Whether the root disk will be encrypyted. Requires `create_launch_template` to be `true` and require `disk_kms_key_id` to be set | bool | false |
| disk\_kms\_key\_id | KMS Key used to encrypt the root disk. Requires both `create_launch_template` and `disk_encrypted` to be `true` | string | "" |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can certainly change this. I elected to use the singular as it was the existing style in the other README entries for the sake of consistency, though I agree it's a little stilted.

Copy link

@johngmyers johngmyers Jul 20, 2021

Choose a reason for hiding this comment

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

It's more than just stilted, it's grammatically incorrect. The verb needs to match in number with the implied subject.

Just noticed there's a second "require" on line 26 that also has incorrect number. It should probably be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay. I updated the language in the README

@alzabo alzabo force-pushed the node_group_encryption branch 2 times, most recently from 94ecce8 to 777f29d Compare August 8, 2021 16:59
@alzabo alzabo force-pushed the node_group_encryption branch from 777f29d to 68fbcdb Compare August 8, 2021 17:01
Copy link
Member

@bryantbiggs bryantbiggs left a comment

Choose a reason for hiding this comment

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

@antonbabenko / @barryib 👍🏼

@jweigand
Copy link

Looks like this has one approval, any guess when it might be merged/released? Urgently needed for a project we're working on (which already uses the module).

@antonbabenko
Copy link
Member

@jweigand Please see my other comment - #1459 (comment) . You will hear about this PR soon.

@antonbabenko antonbabenko merged commit 6067290 into terraform-aws-modules:master Aug 25, 2021
@antonbabenko
Copy link
Member

Here we go! 🎉

v17.2.0 has been just released.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2022
@alzabo alzabo deleted the node_group_encryption branch November 19, 2022 14:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants