Skip to content

Default to not attching AmazonSSMManagedInstanceCore to instances #143

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 25, 2020
Merged

Default to not attching AmazonSSMManagedInstanceCore to instances #143

merged 2 commits into from
Aug 25, 2020

Conversation

HenryNguyen5
Copy link
Contributor

@HenryNguyen5 HenryNguyen5 commented Aug 17, 2020 •
edited
Loading

Disables AmazonSSMManagedInstanceCore policy attachment to runner instances unless the
debug_instances variable is set.

Related issue:
https://github.com/philips-labs/terraform-aws-github-runner/issues/114

@HenryNguyen5 HenryNguyen5 force-pushed the feature/lock_down_ssm_perms branch from 930452d to 1c2b7fe Compare August 17, 2020 19:41
npalm
npalm requested changes Aug 18, 2020
View reviewed changes
@HenryNguyen5 HenryNguyen5 force-pushed the feature/lock_down_ssm_perms branch from 1c2b7fe to a48a6d8 Compare August 18, 2020 21:47
@HenryNguyen5 HenryNguyen5 requested a review from npalm August 18, 2020 21:47
npalm
npalm requested changes Aug 19, 2020
View reviewed changes
Copy link
Member

@npalm npalm left a comment

Choose a reason for hiding this comment

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

@HenryNguyen5 please check the build, terraform format is failing. The repo has configured https://pre-commit.com/ run terraform format on commint and update the variables in the readme.

@HenryNguyen5 HenryNguyen5 force-pushed the feature/lock_down_ssm_perms branch from a48a6d8 to 2c236b3 Compare August 19, 2020 15:29
@HenryNguyen5 HenryNguyen5 requested a review from npalm August 19, 2020 15:31
@HenryNguyen5
Copy link
Contributor Author

@npalm should be good now!

@HenryNguyen5 HenryNguyen5 force-pushed the feature/lock_down_ssm_perms branch from 2c236b3 to 2bf5e1a Compare August 19, 2020 16:59
@HenryNguyen5
Copy link
Contributor Author

Rebased

@npalm
Copy link
Member

npalm commented Aug 21, 2020

@HenryNguyen5 thanks for updating, just checked the PR. Seems the permissions are set twice in the runner module. To disable the SSM we need to remove a bit more. Please can you remove resource "aws_iam_role_policy" "runner_session_manager_policy" https://github.com/philips-labs/terraform-aws-github-runner/blob/39a4b8753e5b0a6f0c3463c4aa631d0fd3ef5a5b/modules/runners/policies-runner.tf#L17 and policy file olicies/instance-session-manager-policy.json Permissions set in this file are also set via AmazonSSMManagedInstanceCore

@HenryNguyen5
Copy link
Contributor Author

@npalm removed!

@HenryNguyen5
Copy link
Contributor Author

@npalm anything I should do to push this along?

@npalm
Copy link
Member

npalm commented Aug 25, 2020

@npalm anything I should do to push this along?

Thank, looks all good. We will merge asap

@npalm npalm merged commit 214f0c7 into github-aws-runners:develop Aug 25, 2020
npalm pushed a commit that referenced this pull request Aug 25, 2020
@HenryNguyen5 @npalm
feat: Default to not attching AmazonSSMManagedInstanceCore to instanc…
f7792d1 
…es (#143)

* Default to not attching AmazonSSMManagedInstanceCore to instances

* Remove instance_runner_session_manager_policy
@gertjanmaas gertjanmaas mentioned this pull request Aug 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@npalm npalm npalm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@HenryNguyen5 @npalm