[AWS SSO] Use service account to access EC2 and S3 on head and worker nodes #1489

Michaelvll · 2022-12-05T09:35:45Z

This partially fixes #1488

For aws, we now set the worker nodes to have the same IAM role as the head node, so that they can correctly access s3 buckets without the need to upload the user's credential files (TBD: using the same role will allow the worker to access EC2 as well, which may not be necessary, and we can fix it by creating a new s3-access role for that, such as skypilot-service-worker-v1).

Having the IAM:PassRole for the IAM role is required, as the head node has to pass the role skypilot-service-v1 to the worker node.

It also adds mypy check for sky/clouds/*.py

TODO:

Backward compatibility test, as the service account name has been changed.
Without uploading the credential files to the VM, the spot controller launched on other clouds will not be able to create instance on AWS. [We added back credential uploads, as the VMs on other clouds needs that to access AWS]

For an SSO account, we will show the following hints:

Tested:

sky launch -c multi-node -t t3.micro ./examples/using_file_mounts.yaml with a private s3 bucket.
sky launch -c multi-node-sso -t t3.micro ./examples/using_file_mounts.yaml with a private s3 bucket, using an account set up with SSO and AdministratorAccess
tests/run_smoke_tests.sh
tests/backward_comaptibility_tests.sh
sky launch -t t2.micro -i 0; sky status -r correctly autostopped with SSO account.
According to our user, the current PR works with their SSO account.

… access aws

…nto ray-autoscaler-role-for-all

Michaelvll · 2022-12-12T04:11:24Z

Important problem: Seems we have a bug with the tests/backward_comaptibility_tests.sh, that we did not capture the following backward compatibility issue:

git checkout master
sky launch -c min
git checkout ray-autoscaler-role-for-all
sky launch -c min # It stops the original min cluster and launched a new one.

Update: Fixed the compatibility issue by having a new version of the AWSNodeProvider instead of overwriting the original one, and tested with the tests/backward_compatitiblity.sh (added instruction to that test and double check the test works.)

…patibility

Michaelvll · 2022-12-20T08:17:24Z

Tested (24e22c9):

AWS_PROFILE=admin-user sky launch -c sso -t t3.micro -i 1
AWS_PROFILE=admin-user sky launch -c sso-down -t t3.micro -i 1 --down
sky status -r
AWS_PROFILE=admin-user sky status -r
AWS_PROFILE=admin-user skt down sso
sky launch -c multi-node -t t3.xlarge -i 1 ./examples/using_file_mounts.yaml with a private s3 bucket.
AWS_PROFILE=admin-user sky launch -c multi-node-sso -t t3.xlarge ./examples/using_file_mounts.yaml with a private s3 bucket, using an account set up with SSO and AdministratorAccess
AWS_PROFILE=admin-user sky start multi-node-sso

…xperiments into ray-autoscaler-role-for-all

…nto ray-autoscaler-role-for-all

concretevitamin

This looks great @Michaelvll; did a pass.

sky/backends/backend_utils.py

sky/clouds/azure.py

sky/clouds/cloud.py

sky/skylet/providers/aws/node_provider.py

sky/clouds/aws.py

concretevitamin · 2022-12-27T01:48:57Z

sky/clouds/aws.py

                # TODO(zhwu): after we publish sky to PyPI,
                # change this to `pip install sky[aws]`
-                '\n     $ pip install .[aws]'
-                '\n   Credentials may also need to be set.' + help_str)
+                f'\n{prefix}  $ pip install .[aws]'


I think we should make PyPI users see the correct message, since users installing from master can be assumed as more "hands-on"/advanced, so they should be able to change the command accordingly?

sky/clouds/aws.py

concretevitamin · 2022-12-27T01:49:35Z

sky/clouds/aws.py

@@ -368,9 +365,28 @@ def check_credentials(self) -> Tuple[bool, Optional[str]]:
        except exceptions.CloudUserIdentityError as e:
            return False, str(e)

+        hints = None
+        # `aws configure list` does not guarantee this file exists.


Seems removable?

Added the credential file checking back for multiple clouds case.

sky/clouds/aws.py

Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>

Michaelvll · 2022-12-27T06:46:37Z

concretevitamin

LGTM. Thanks @Michaelvll!

sky/skylet/providers/aws/config.py

sky/clouds/aws.py

Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>

Michaelvll added 6 commits December 5, 2022 01:29

Create SkyPilot service account with neccessary permission

1181cc1

legacy key name

996eba2

align quote

470557c

lint

abc224b

Hints for not working with other clouds

d90b851

Add back credential file upload, since VMs on other clouds need it to…

e18b91e

… access aws

Michaelvll marked this pull request as ready for review December 5, 2022 22:39

Michaelvll added 9 commits December 5, 2022 20:32

Add get instance profile and pin awscli

92bcb32

rename policy

3fe931e

Fix hints

6e28d9a

align

7b7447d

Not upload AWScredentials when SSO

1673bd1

format

8c326bf

mark costly smoke tests

ba4f041

Use ubuntu-20.04 for test

7e3987c

format

4efebf1

Michaelvll mentioned this pull request Dec 7, 2022

[Spot] Add option for specifying controller instance type #1493

Closed

1 task

Michaelvll added 2 commits December 6, 2022 21:43

rename tests

1719e2e

fix

fe23509

Michaelvll force-pushed the ray-autoscaler-role-for-all branch from 5baf406 to fe23509 Compare December 7, 2022 05:46

support option for run_smoke_tests.sh

f1fd9ce

Michaelvll mentioned this pull request Dec 7, 2022

[Tests] Fix github CI #1495

Merged

Michaelvll added 2 commits December 7, 2022 00:54

Merge branch 'master' of github.com:concretevitamin/sky-experiments i…

d6c5e91

…nto ray-autoscaler-role-for-all

rename variable

21d1a23

Michaelvll changed the title ~~[AWS] Use service account to access EC2 and S3 on head and worker nodes~~ [AWS SSO] Use service account to access EC2 and S3 on head and worker nodes Dec 11, 2022

Michaelvll mentioned this pull request Dec 12, 2022

Add user identity to cluster status to avoid leakage when switching account #1513

Merged

6 tasks

Michaelvll added 3 commits December 11, 2022 20:33

Use version instead of inplace change for aws config for backward com…

355088e

…patibility

fix

705b86e

Add comments

1430d74

Michaelvll added 5 commits December 20, 2022 00:55

Fix identity check

78aada9

lint

2eac8b6

remove spaces

c86b69d

Merge branch 'fix-identity-check' of github.com:concretevitamin/sky-e…

f74f604

…xperiments into ray-autoscaler-role-for-all

Merge branch 'master' of github.com:concretevitamin/sky-experiments i…

24e22c9

…nto ray-autoscaler-role-for-all

Michaelvll requested review from concretevitamin and suquark December 26, 2022 23:29

concretevitamin reviewed Dec 27, 2022

View reviewed changes

Michaelvll and others added 7 commits December 26, 2022 22:24

address comments

a1df005

Update sky/clouds/aws.py

62378bd

Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>

Update sky/clouds/aws.py

80911dc

Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>

address comments

ba54fe7

zone to be None

6723190

add comment

bfc5f56

apply changes to help str

d150cd9

concretevitamin approved these changes Dec 27, 2022

View reviewed changes

sky/skylet/providers/aws/config.py Outdated Show resolved Hide resolved

sky/skylet/providers/aws/config.py Outdated Show resolved Hide resolved

sky/clouds/aws.py Outdated Show resolved Hide resolved

Michaelvll and others added 6 commits December 26, 2022 23:25

Enable fail over when the sso is expired

e99853a

Update sky/skylet/providers/aws/config.py

1b85317

Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>

address comments

75b3424

avoid unbounded get_project_id exceptions

0e9819d

format

6546bb1

print out status refresh result

1c2c549

Michaelvll force-pushed the ray-autoscaler-role-for-all branch from 1f313e2 to 1c2c549 Compare December 27, 2022 18:44

Michaelvll added 3 commits December 27, 2022 10:52

retry for azure identity fetching

3ef278b

wait longer for autostop

3df29ea

more retry for azure

47ac24e

Michaelvll merged commit 9709e71 into master Dec 28, 2022

Michaelvll deleted the ray-autoscaler-role-for-all branch December 28, 2022 04:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AWS SSO] Use service account to access EC2 and S3 on head and worker nodes #1489

[AWS SSO] Use service account to access EC2 and S3 on head and worker nodes #1489

Michaelvll commented Dec 5, 2022 •

edited

Loading

Michaelvll commented Dec 12, 2022 •

edited

Loading

Michaelvll commented Dec 20, 2022 •

edited

Loading

concretevitamin left a comment

concretevitamin Dec 27, 2022

concretevitamin Dec 27, 2022

Michaelvll Dec 27, 2022

Michaelvll commented Dec 27, 2022 •

edited

Loading

concretevitamin left a comment

[AWS SSO] Use service account to access EC2 and S3 on head and worker nodes #1489

[AWS SSO] Use service account to access EC2 and S3 on head and worker nodes #1489

Conversation

Michaelvll commented Dec 5, 2022 • edited Loading

Michaelvll commented Dec 12, 2022 • edited Loading

Michaelvll commented Dec 20, 2022 • edited Loading

concretevitamin left a comment

Choose a reason for hiding this comment

concretevitamin Dec 27, 2022

Choose a reason for hiding this comment

concretevitamin Dec 27, 2022

Choose a reason for hiding this comment

Michaelvll Dec 27, 2022

Choose a reason for hiding this comment

Michaelvll commented Dec 27, 2022 • edited Loading

concretevitamin left a comment

Choose a reason for hiding this comment

Michaelvll commented Dec 5, 2022 •

edited

Loading

Michaelvll commented Dec 12, 2022 •

edited

Loading

Michaelvll commented Dec 20, 2022 •

edited

Loading

Michaelvll commented Dec 27, 2022 •

edited

Loading