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

fix: Allow sagemaker to have the proper IAM permission to autostop itself #515

Merged
merged 5 commits into from
Jun 7, 2021

Conversation

nguyen102
Copy link
Contributor

@nguyen102 nguyen102 commented Jun 7, 2021

Issue #, if available:

Description of changes:

  • Update permission boundary for Sagemaker instance to allow stopping a Sagemaker instance
  • Change Sagemaker to check whether idle limit has passed, on a minute by minute cadence instead of an hourly cadence
  • Update envStatusPollHandler to run once every minute instead of once every 3 minutes. (More accurate, since users can choose to have Sagemaker stop itself after 1 minute of inactivity)

I tested that Sagemaker was able to stop itself after inactivity. I saw the Sagemaker instance was stopped on the AWS Console and within SWB UI.

Checklist:

  • Have you successfully deployed to an AWS account with your changes?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully tested with your changes locally?
  • If new dependencies have been added, have they been pinned to specific versions?
  • Is this change also required on the AWS Solution version?
  • Have you updated openapi.yaml if you made updates to API definition (including add, delete or update parameter and request data schema)?
  • If you had to run manual tests, have you considered automating those tests by adding them to end-to-end tests?

AS review ticket id:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@nguyen102 nguyen102 changed the base branch from mainline to develop June 7, 2021 13:41
@codecov
Copy link

codecov bot commented Jun 7, 2021

Codecov Report

Merging #515 (13c8d03) into develop (e99ecca) will not change coverage.
The diff coverage is n/a.

❗ Current head 13c8d03 differs from pull request most recent head efcb8d8. Consider uploading reports for the commit efcb8d8 to get more accurate results
Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #515   +/-   ##
========================================
  Coverage    49.04%   49.04%           
========================================
  Files          243      243           
  Lines        12503    12503           
  Branches      2012     2012           
========================================
  Hits          6132     6132           
  Misses        5564     5564           
  Partials       807      807           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e99ecca...efcb8d8. Read the comment docs.

@nguyen102 nguyen102 marked this pull request as ready for review June 7, 2021 16:00
@nguyen102 nguyen102 requested a review from a team as a code owner June 7, 2021 16:00
Action:
- sagemaker:DescribeNotebookInstance
- sagemaker:StopNotebookInstance
Resource: '*'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we restrict the resource here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can the reference be - !Ref BasicNotebookInstance?

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 don't think we can reference it because the notebook is created after this policy is created. More info here. Also since this is a permission boundary I think it's ok to have a broader resource section here. In the actual Sagemaker IAM role, we do scope the permission down to just the notebook.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's what I was trying to understand. If the IAM policy can reference the instance then why can't permission boundary policy do the same.

I think it's ok to have a broader resource section here. In the actual Sagemaker IAM role, we do scope the permission down to just the notebook.

Yeah, that's why I approved the change. But I am still curious if it could have been done just like we did with IAM policy

@nguyen102 nguyen102 merged commit 32007ed into awslabs:develop Jun 7, 2021
ahl27 added a commit that referenced this pull request Jun 11, 2021
* feature: updated UI for AWS accounts page with cards instead of a list box.

* feature: added API calls to update AWS Account, added functionality to check permissions status and update with DB table on backend

* feat: adds filter buttons for accounts as well as code cleanup and general UX improvements.

* fix: fixed budget buttons on account cards to correctly direct to the budget page

* fix: cleaned up code, added unit test, added entry to openapi.yaml

* fix: removed unused file

* fix: made some buttons look better

* fix: added unit test to increase codecov and fixed a minor bug in AwsAccountsStore

* chore: docs dependency fix (#505)

* chore(deps): bump dns-packet from 1.3.1 to 1.3.4 in /docs (#507)

* chore(deps): bump dns-packet from 1.3.1 to 1.3.4 in /docs

Bumps [dns-packet](https://github.com/mafintosh/dns-packet) from 1.3.1 to 1.3.4.
- [Release notes](https://github.com/mafintosh/dns-packet/releases)
- [Changelog](https://github.com/mafintosh/dns-packet/blob/master/CHANGELOG.md)
- [Commits](mafintosh/dns-packet@v1.3.1...v1.3.4)

Signed-off-by: dependabot[bot] <support@github.com>

* fix: trigger build

* feat: Add warning that internal authentication shouldn't be used in production (#506)

* feat: Encrypt s3 buckets for EMR log bucket and CICD Artifact bucket (#508)

* chore: Disable EBS volume for storage gateway (#511)

Co-authored-by: Tim Nguyen <thingut@amazon.com>

* fix: changes suggested by Yanyu in CR

* fix: minor change to openapi.yml

* fix: removed unneccessary script

* fix: removed reliance on undefined value for needsPermissionUpdate

* fix: changed NEW to ONBOARDME for better clarity

* Update settings.json

* Update settings.json

* removed confusing half-implemented function and replaced with placeholder

* chore: Add encryption to CICD SNS topic (#512)

Co-authored-by: Tim Nguyen <thingut@amazon.com>

* fix: Allow sagemaker to have the proper IAM permission to autostop itself (#515)

* chore: Enable access logging for env-type-configs bucket (#520)

* chore: Enable server side encryption on prepare master and edge lambda bucket (#521)

* fix: Corrected Spark defaults to fix read/write functionality from Spark (#526)

Co-authored-by: Yanyu Zheng <yz2690@columbia.edu>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Tim Nguyen <nguyen102@users.noreply.github.com>
Co-authored-by: Tim Nguyen <thingut@amazon.com>
Co-authored-by: Jeet <68876606+jn1119@users.noreply.github.com>
ahl27 pushed a commit to ahl27/service-workbench-on-aws that referenced this pull request Jun 11, 2021
manikandan-thangavelu-rl pushed a commit to RLOpenCatalyst/service-workbench-on-aws that referenced this pull request Jun 14, 2021
manikandan-thangavelu-rl added a commit to RLOpenCatalyst/service-workbench-on-aws that referenced this pull request Jun 14, 2021
@nguyen102 nguyen102 deleted the fix-sagemaker-autostop branch October 26, 2021 14:36
jxuamazon pushed a commit to jxuamazon/service-workbench-on-aws that referenced this pull request Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants