Relax required permissions for custom VPC (GCP) (#2944)

* Update TODO to show that Custom VPC can be used * Add update for GCS support * A way to allow more restrictive service account permissions * Clarification * Sky check respect less restrictive custom VPC permissions * Revert * Move to constant definition * Accidental add * Add to new, duplicate, constants file * Run format.sh * Update sky/provision/gcp/constants.py Co-authored-by: Zhanghao Wu <[email protected]> * Update docs/source/cloud-setup/cloud-permissions/gcp.rst Co-authored-by: Zhanghao Wu <[email protected]> --------- Co-authored-by: Zhanghao Wu <[email protected]>
skypilot-org · Jan 5, 2024 · d4e0f5b · d4e0f5b
1 parent b13e9c6
commit d4e0f5b
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 7 deletions.
diff --git a/docs/source/cloud-setup/cloud-permissions/gcp.rst b/docs/source/cloud-setup/cloud-permissions/gcp.rst
@@ -94,6 +94,10 @@ User
     resourcemanager.projects.get
     resourcemanager.projects.getIamPolicy
 
+.. note::
+
+    For custom VPC users (with :code:`gcp.vpc_name` specified in :code:`~/.sky/config.yaml`, check `here <#_gcp-bring-your-vpc>`_),  :code:`compute.firewalls.create` and :code:`compute.firewalls.delete` are not necessary unless opening ports is needed via `resources.ports` in task yaml.
+
 4. **Optional**: If the user needs to access GCS buckets, you can additionally add the following permissions:
 
 .. code-block:: text
@@ -102,6 +106,7 @@ User
     storage.buckets.get
     storage.buckets.delete
     storage.objects.create
+    storage.objects.update
     storage.objects.delete
     storage.objects.get
     storage.objects.list

diff --git a/sky/provision/gcp/constants.py b/sky/provision/gcp/constants.py
@@ -1,4 +1,5 @@
 """Constants used by the GCP provisioner."""
+from sky import skypilot_config
 
 VERSION = 'v1'
 # Using v2 according to
@@ -180,6 +181,13 @@
     'resourcemanager.projects.get',
     'resourcemanager.projects.getIamPolicy',
 ]
+# If specifying custom VPC, permissions to modify network are not necessary
+# unless opening ports (e.g., via `resources.ports`).
+if skypilot_config.get_nested(('gcp', 'vpc_name'), ''):
+    remove = ('compute.firewalls.create', 'compute.firewalls.delete')
+    VM_MINIMAL_PERMISSIONS = [
+        p for p in VM_MINIMAL_PERMISSIONS if p not in remove
+    ]
 
 TPU_MINIMAL_PERMISSIONS = [
     'tpu.nodes.create',

diff --git a/sky/skylet/providers/gcp/constants.py b/sky/skylet/providers/gcp/constants.py
@@ -1,3 +1,5 @@
+from sky import skypilot_config
+
 SKYPILOT_VPC_NAME = "skypilot-vpc"
 
 # Below parameters are from the default VPC on GCP.
@@ -30,8 +32,9 @@
                 "ports": ["22"],
             }
         ],
-        # TODO(skypilot): some users reported that this should be relaxed (e.g.,
-        # allowlisting only certain IPs to have ssh access).
+        # Some users have reported that this conflicts with their network
+        # security policy. A custom VPC can be specified in ~/.sky/config.yaml
+        # allowing for restriction of source ranges bypassing this requirement.
         "sourceRanges": ["0.0.0.0/0"],
     },
 ]
@@ -90,11 +93,6 @@
 VM_MINIMAL_PERMISSIONS = [
     "compute.disks.create",
     "compute.disks.list",
-    # TODO(skypilot): some users reported that firewalls changes
-    # (create/delete/update) should be removed if VPC/firewalls are separately
-    # set up. It is undesirable for a normal account to have these permissions.
-    # Note that if these permissions are removed, opening ports (e.g., via
-    # `resources.ports`) would fail.
     "compute.firewalls.create",
     "compute.firewalls.delete",
     "compute.firewalls.get",
@@ -124,6 +122,11 @@
     "resourcemanager.projects.get",
     "resourcemanager.projects.getIamPolicy",
 ]
+# If specifying custom VPC, permissions to modify network are not necessary
+# unless opening ports (e.g., via `resources.ports`).
+if skypilot_config.get_nested(("gcp", "vpc_name"), ""):
+    remove = ("compute.firewalls.create", "compute.firewalls.delete")
+    VM_MINIMAL_PERMISSIONS = [p for p in VM_MINIMAL_PERMISSIONS if p not in remove]
 
 TPU_MINIMAL_PERMISSIONS = [
     "tpu.nodes.create",