[Core][AWS] Allow specification of IAM roles for resources. #3488

JGSweets · 2024-04-26T17:22:15Z

This PR updates the schema config to allow specification of IAM roles for resources based on the skypilot naming conventions.

Address: #3487

In ~/.sky/config.yaml:

When setting for the controller and all other resources (default)

aws:
  remote_identity:
    sky-serve-controller-*: skypilot-v1-fake
    "*": skypilot-default-fake

When setting for everything by specifying a default

aws:
  remote_identity:
    "*": skypilot-default-fake

When setting for everything by specifying just the remote_identity via string

aws:
  remote_identity: skypilot-default-fake

When first profile doesn't match it chooses the next in line if viable

aws:
  remote_identity:
    does-not-match: skypilot-default-fake
    "*": skypilot-default-fake

Tested (run the relevant ones):

Code formatting: bash format.sh
Any manual or new tests for this PR (please specify below)
- tested running in all 4 configs in AWS
  - Nothing set
  - setting different controller and default values
  - setting default only
  - setting two profile names where the first one doesn't match to ensure it selects the viable candidate in order
All smoke tests: pytest tests/test_smoke.py
Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

JGSweets · 2024-05-02T04:36:09Z

Due note the quotes required around * to meet yaml file specifications: "*".

e.g.

aws:
  remote_identity:
    "*": skypilot-default-fake

sky/backends/backend_utils.py

Michaelvll

Thanks for implementing this @JGSweets! The code looks good to me. Could you help add the new schema in our docs as well: https://skypilot.readthedocs.io/en/latest/reference/config.html

i.e., the following code path:

skypilot/docs/source/reference/config.rst

Line 134 in 2ff95bb

remote_identity: LOCAL_CREDENTIALS

Michaelvll · 2024-05-06T21:01:02Z

sky/backends/backend_utils.py

-    if remote_identity == 'SERVICE_ACCOUNT':
+    if remote_identity is not None and not isinstance(remote_identity, str):
+        for profile in remote_identity:
+            if fnmatch.fnmatchcase(cluster_name_on_cloud, profile):


Would be nice to match cluster_name instead cluster_name_on_cloud as the former is user-facing and should be more intuitive to stay the same as the one specified in ~/.sky/config.yaml.

Suggested change

if fnmatch.fnmatchcase(cluster_name_on_cloud, profile):

if fnmatch.fnmatchcase(cluster_name, profile):

sky/backends/backend_utils.py

Michaelvll · 2024-05-06T21:15:06Z

sky/utils/schemas.py

+        }, {
+            'type': 'object',
+            'required': [],
+            'additionalProperties': {
+                'type': 'string',
+            },
+        }]


Seems we only added the support for AWS at the moment, we should limit this ability to aws cloud only, i.e. in L664, we can only apply this additional support for aws only.

Should be fixed

JGSweets · 2024-05-06T21:56:16Z

docs/source/reference/config.rst

+    # User Specified SERVICE_ACCOUNT (IAM role): The name of the remote identity
+    # to give the launched resouce.


Not sure how we wanted to reference this specifically as an example. Open to suggestion here.

JGSweets · 2024-05-06T21:56:40Z

docs/source/reference/config.rst

+    ### Format 1 ###
+    # A string; the same remote identity is applied to all launched resources.
    remote_identity: LOCAL_CREDENTIALS
+    ### Format 2 ###
+    # A dict mapping wildcard expression of cloud names to the resources to the
+    # resource identity.
+    # NOTE: If not a wildcard expression in the dict mapping does not match a
+    # cloud name for a resouce being deployed, the default remote identity is used.
+    # To specify your own default, utilize "*" as the wildcard expression.
+    remote_identity:
+      sky-serve-controller-*: my-controller-specific-identity
+      "*": SERVICE_ACCOUNT


First attempt at how we represent multiple formats.

Thanks for adding these! I reorganized the comments a little bit in the comment above, can we remove Format 1 and Format 2 here and use the comments above?

sky/backends/backend_utils.py

Michaelvll

Thanks for the quick fix @JGSweets! This PR should be good to go with the comments fixed. : )

docs/source/reference/config.rst

Michaelvll · 2024-05-06T23:39:15Z

docs/source/reference/config.rst

+    ### Format 1 ###
+    # A string; the same remote identity is applied to all launched resources.
    remote_identity: LOCAL_CREDENTIALS
+    ### Format 2 ###
+    # A dict mapping wildcard expression of cloud names to the resources to the
+    # resource identity.
+    # NOTE: If not a wildcard expression in the dict mapping does not match a
+    # cloud name for a resouce being deployed, the default remote identity is used.
+    # To specify your own default, utilize "*" as the wildcard expression.
+    remote_identity:
+      sky-serve-controller-*: my-controller-specific-identity
+      "*": SERVICE_ACCOUNT


Thanks for adding these! I reorganized the comments a little bit in the comment above, can we remove Format 1 and Format 2 here and use the comments above?

docs/source/reference/config.rst

refactor: config description Co-authored-by: Zhanghao Wu <[email protected]>

refactor: config example Co-authored-by: Zhanghao Wu <[email protected]>

JGSweets · 2024-05-07T00:37:54Z

@Michaelvll Fixed as suggested. Thanks for the improvements!

JGSweets changed the title ~~[CORE][AWS] Allow specification of IAM roles for resources.~~ [Core][AWS] Allow specification of IAM roles for resources. Apr 26, 2024

This was referenced Apr 26, 2024

[Core][AWS] Allow the config to set IAM roles for resources being launched by AWS #3487

Closed

[Core][AWS] Allow specification of Security Groups for resources. #3501

Merged

JGSweets force-pushed the add-iam-config branch from f8ac1d6 to 1de4b67 Compare May 2, 2024 04:34

JGSweets commented May 2, 2024

View reviewed changes

sky/backends/backend_utils.py Show resolved Hide resolved

JGSweets added 7 commits May 2, 2024 10:03

feat: add iam capabilities to serve

a83ab9b

fix: do not require default

c9b5f91

fix: pylint

a7d456a

fix: pylint

be4b86c

refactor: use remote_identity instead of iam_instance_profile

4cea7a2

fix: formatting

703cc12

fix: quote delimiter

02353a7

JGSweets force-pushed the add-iam-config branch from 11a9fc4 to 02353a7 Compare May 2, 2024 14:08

Michaelvll reviewed May 6, 2024

View reviewed changes

JGSweets added 5 commits May 6, 2024 16:21

fix: typo

794dd40

refactor: updates config.rst to include aws remote identity

f56cf24

refactor: use cluster_name instead of cluster_name_on_cloud

e6c5956

fix: only use the new remote_identity feature with AWS

7b5861e

fix: config.rst remote identity example

6b34a3f

JGSweets commented May 6, 2024

View reviewed changes

sky/backends/backend_utils.py Show resolved Hide resolved

Michaelvll approved these changes May 6, 2024

View reviewed changes

JGSweets and others added 3 commits May 6, 2024 19:34

Update docs/source/reference/config.rst

f1f7f13

refactor: config description Co-authored-by: Zhanghao Wu <[email protected]>

Update docs/source/reference/config.rst

fe757ff

refactor: config example Co-authored-by: Zhanghao Wu <[email protected]>

refactor: remove redundant example

27a3dfc

Michaelvll merged commit 5a0ecc7 into skypilot-org:master May 7, 2024
20 checks passed

Michaelvll mentioned this pull request May 7, 2024

[AWS] Fix config syntax for IAM profile #3514

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core][AWS] Allow specification of IAM roles for resources. #3488

[Core][AWS] Allow specification of IAM roles for resources. #3488

JGSweets commented Apr 26, 2024 •

edited

Loading

JGSweets commented May 2, 2024 •

edited

Loading

Michaelvll left a comment

Michaelvll May 6, 2024

JGSweets May 6, 2024

Michaelvll May 6, 2024

JGSweets May 6, 2024

JGSweets May 6, 2024

JGSweets May 6, 2024

Michaelvll May 6, 2024

Michaelvll left a comment

Michaelvll May 6, 2024

JGSweets commented May 7, 2024

	if fnmatch.fnmatchcase(cluster_name_on_cloud, profile):
	if fnmatch.fnmatchcase(cluster_name, profile):

		# User Specified SERVICE_ACCOUNT (IAM role): The name of the remote identity
		# to give the launched resouce.

[Core][AWS] Allow specification of IAM roles for resources. #3488

[Core][AWS] Allow specification of IAM roles for resources. #3488

Conversation

JGSweets commented Apr 26, 2024 • edited Loading

JGSweets commented May 2, 2024 • edited Loading

Michaelvll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JGSweets commented May 7, 2024

JGSweets commented Apr 26, 2024 •

edited

Loading

JGSweets commented May 2, 2024 •

edited

Loading