[GCP] Support private IPs for GCP #2819

Michaelvll · 2023-11-24T07:52:03Z

Use Internal IP

For security reason, users may only want to use internal IPs for SkyPilot instances.
To do so, you can use SkyPilot's global config file ~/.sky/config.yaml to specify the gcp.use_internal_ips and gcp.ssh_proxy_command fields (to see the detailed syntax, see :ref:config-yaml):

    gcp:
      use_internal_ips: true
      # VPC with NAT setup, see below
      vpc_name: my-vpc-name
      ssh_proxy_command: ssh -W %h:%p -o StrictHostKeyChecking=no myself@my.proxy

The gcp.ssh_proxy_command field is optional. If SkyPilot is run on a machine that can directly access the internal IPs of the instances, it can be omitted. Otherwise, it should be set to a command that can be used to proxy SSH connections to the internal IPs of the instances.

Cloud NAT Setup

Instances created with internal IPs only on GCP cannot access public internet by default. To make sure SkyPilot can install the dependencies correctly on the instances,
cloud NAT needs to be setup for the VPC.

Cloud NAT is a regional resource, so it will need to be created in each region that SkyPilot will be used in.

To limit SkyPilot to use some specific regions only, you can specify the gcp.ssh_proxy_command to be a dict mapping from region to the SSH proxy command for that region (see :ref:config-yaml for details):

    gcp:
      use_internal_ips: true
      vpc_name: my-vpc-name
      ssh_proxy_command:
        us-west1: ssh -W %h:%p -o StrictHostKeyChecking=no myself@my.us-west1.proxy
        us-east1: ssh -W %h:%p -o StrictHostKeyChecking=no myself@my.us-west2.proxy

If proxy is not needed, but the regions need to be limited, you can set the gcp.ssh_proxy_command to be a dict mapping from region to null:

    gcp:
      use_internal_ips: true
      vpc_name: my-vpc-name
      ssh_proxy_command:
        us-west1: null
        us-east1: null

With the setup above, we should be able to create clusters with internal IPs only using SkyPilot and the ~/.sky/config.yaml above.
sky launch -c test-private --cloud gcp --cpus 2

Tested (run the relevant ones):

concretevitamin

Awesome @Michaelvll! Finished a pass except for docs/. Some comments.

sky/utils/cluster_yaml_utils.py

sky/utils/controller_utils.py

concretevitamin · 2023-11-28T04:20:19Z

sky/skypilot_config.py

@@ -119,6 +119,19 @@ def set_nested(keys: Iterable[str], value: Any) -> Dict[str, Any]:
    return to_return


+def overwrite_config_file(config: dict) -> None:


Is it safe? It feels hard to reason about. Any ways to avoid this?

When the controller is launching a task, it should use the task-specific skypilot_config decided by SKYPILOT_CONFIG env var, and we have to update the proxy command setting in that config.
Added a docstr to for calling this with cautious.

If needed we can save the new config in a new config_file and reset the os.environ['SKYPILOT_CONFIG'] to the new config_file and set _dict to the new config.

I don't think this will make much difference than directly overwrite the original config file. Wdyt?

What's the reason for replacing the previous way? I.e., directly write out the properly modified config to a local temp file and upload it. It seems like local client does know where the controller is/is going to be launched?

If there's a strong reason to do it this way, how about renaming this to unsafe_overwrite_config_file_on_controller or something like that?

What's the reason for replacing the previous way? I.e., directly write out the properly modified config to a local temp file and upload it. It seems like local client does know where the controller is/is going to be launched?

The previous way was done before the controller is actually UP (failover may happen), which means we don't know which cloud the controller will use when we run sky.launch for the controller. The current way replaces the config on the controller, so it has the latest information about the controller itself.

If there's a strong reason to do it this way, how about renaming this to unsafe_overwrite_config_file_on_controller or something like that?

Yes, renaming it sounds good to me.

sky/backends/cloud_vm_ray_backend.py

concretevitamin · 2023-11-28T04:41:55Z

sky/utils/controller_utils.py

+            config_dict = skypilot_config.set_nested(proxy_command_key,
+                                                     ssh_proxy_command)
+
+        skypilot_config.overwrite_config_file(config_dict)


See comment in skypilot_config.

concretevitamin

Thanks, a question about overwriting the config file. Other parts LGTM

sky/backends/cloud_vm_ray_backend.py

concretevitamin · 2023-11-28T17:16:18Z

sky/skypilot_config.py

@@ -119,6 +119,19 @@ def set_nested(keys: Iterable[str], value: Any) -> Dict[str, Any]:
    return to_return


+def overwrite_config_file(config: dict) -> None:


What's the reason for replacing the previous way? I.e., directly write out the properly modified config to a local temp file and upload it. It seems like local client does know where the controller is/is going to be launched?

If there's a strong reason to do it this way, how about renaming this to unsafe_overwrite_config_file_on_controller or something like that?

docs/source/reference/config.rst

docs/source/cloud-setup/cloud-permissions/gcp.rst

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

Michaelvll · 2023-11-29T05:57:27Z

According to the request, I refactored the way we reset the config file being uploaded to the controller, so we don't have to overwrite the config file on the remote cluster. @concretevitamin PTAL. : )

Tested:

AWS spot controller, and check the config file uploaded to the controller:

spot:
  controller:
    resources:
      cloud: aws

gcp:
  use_internal_ips: true
  vpc_name: skypilot-vpc
  ssh_proxy_command: 
    us-west1: ssh -W %h:%p -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o IdentitiesOnly=yes gcpuser@xx.xxx.xxx.xx -i ~/.ssh/sky-key

aws:
  use_internal_ips: true
  vpc_name: skypilot-test-vpc
  ssh_proxy_command:
    us-east-1: ssh -W %h:%p -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o IdentitiesOnly=yes -i ~/.ssh/sky-key ec2-user@xx.xxx.xxx.xxx

Launch spot job on GCP: sky spot launch --cloud gcp
Launch spot job on AWS: sky spot launch --cloud aws

GCP spot controller, and check the config file uploaded to the controller:

spot:
  controller:
    resources:
      cloud: gcp

gcp:
  use_internal_ips: true
  vpc_name: skypilot-vpc
  ssh_proxy_command: 
    us-west1: ssh -W %h:%p -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o IdentitiesOnly=yes gcpuser@xx.xxx.xxx.xx -i ~/.ssh/sky-key

aws:
  use_internal_ips: true
  vpc_name: skypilot-test-vpc
  ssh_proxy_command:
    us-east-1: ssh -W %h:%p -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o IdentitiesOnly=yes -i ~/.ssh/sky-key ec2-user@xx.xxx.xxx.xxx

pytest tests/test_smoke.py --gcp (except docker and test_gcp_http_server_with_custom_ports as they don't support ssh_proxy_command)

…ternal-ips

docs/source/cloud-setup/cloud-permissions/gcp.rst

concretevitamin · 2023-11-29T22:37:19Z

sky/backends/cloud_vm_ray_backend.py

@@ -3215,6 +3218,8 @@ def _sync_file_mounts(
        storage_mounts: Optional[Dict[Path, storage_lib.Storage]],
    ) -> None:
        """Mounts all user files to the remote nodes."""
+        controller_utils.replace_skypilot_config_path_in_file_mounts(


It doesn't look quite clean to do a controller_utils call here.

Can we move this invocation to controller_utils.get_controller_resources(), perhaps renaming it back to controller_utils.skypilot_config_setup()?

This way, we can also remove the hard-coded magic constant skypilot:local_skypilot_config_path. The two controllers j2 files can just take a {{local config path}} like before, and we can pass in a tmp file there.

No, when we call control_utils.get_controller_resources(), it is not possible to know where the controller will be launched, i.e. not be able to get the correct skypilot config. We have to call the function after the controller is provisioned, i.e. after the _provision step is finished and the cloud is decided for the controller.

Basically, we have to decide the content of the skypilot config to be uploaded after the controller actually goes through failover and provisioned. I think the current way is clean if we move this replace_skypilot_config_path_in_file_mounts from controller_utils to backend_utils and rename it to replace_predefined_placeholder_in_file_mounts, as it can be used for filling other predefined file mounts values that needs to be decided after the VM is provisioned.
It can also be useful in the future when we want to decide the location of the mounted bucket based on the cloud a normal VM is launched (currently we just default to the first cloud that is enabled).

sky/utils/controller_utils.py

concretevitamin · 2023-11-29T22:38:26Z

sky/task.py

-                        os.path.abspath(os.path.expanduser(source))):
+                if (not os.path.exists(
+                        os.path.abspath(os.path.expanduser(source))) and
+                        not source.startswith('skypilot:')):


concretevitamin · 2023-11-29T22:40:08Z

sky/utils/controller_utils.py



-def setup_proxy_command_on_controller():
+def _get_skypilot_config_for_controller_task(


setup_proxy_command_on_controller() seems more accurate?

After discussion, this comment is still relevant? Also, maybe arg rename: cloud -> controller_launched_cloud?

concretevitamin

LGTM, thanks @Michaelvll for shipping this!

concretevitamin · 2023-11-30T04:18:18Z

sky/utils/controller_utils.py



-def setup_proxy_command_on_controller():
+def _get_skypilot_config_for_controller_task(
+        cloud: 'clouds.Cloud') -> Dict[str, Any]:
    """Sets up proxy command on the controller.

    This function should be called on the controller (remote cluster), which


Seems outdated now.

concretevitamin · 2023-11-30T04:44:49Z

sky/utils/controller_utils.py



-def setup_proxy_command_on_controller():
+def _get_skypilot_config_for_controller_task(


After discussion, this comment is still relevant? Also, maybe arg rename: cloud -> controller_launched_cloud?

* Add internal IPs and proxy command * Fix schemas * Add config * vpc_name refactor * format * remove remnant * fix API * filter by proxy command * fix region field * Fix tpu pod * Fix internal IP fetching for TPU VM * remove k80 from the resources unordered tests * Address comments * add comment * Add comment * format * Update docs/source/cloud-setup/cloud-permissions/gcp.rst Co-authored-by: Zongheng Yang <zongheng.y@gmail.com> * Address comments * improve docs * refactor skypilot_config upload * fix merge issue * fix * Adopt test fixes from skypilot-org#2681 * fix test for quota * longer timeout * Address comments * format --------- Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>

Michaelvll added 6 commits November 24, 2023 02:12

Add internal IPs and proxy command

557d859

Fix schemas

82f8e35

Add config

4851a9f

vpc_name refactor

5a18820

format

217ed84

remove remnant

e07a67c

Michaelvll requested a review from concretevitamin November 24, 2023 08:23

Michaelvll added 6 commits November 24, 2023 08:31

fix API

25b7a64

filter by proxy command

3d7d182

fix region field

c94ba58

Fix tpu pod

ad8ffad

Fix internal IP fetching for TPU VM

23e0493

remove k80 from the resources unordered tests

3a0fe91

concretevitamin reviewed Nov 28, 2023

View reviewed changes

Michaelvll added 4 commits November 28, 2023 09:04

Address comments

1885529

add comment

40d913c

Add comment

3fdbdc9

format

80c3de9

concretevitamin reviewed Nov 28, 2023

View reviewed changes

Michaelvll and others added 4 commits November 29, 2023 05:22

Update docs/source/cloud-setup/cloud-permissions/gcp.rst

08c7213

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

Address comments

3350a1b

improve docs

569101f

refactor skypilot_config upload

ee727be

Michaelvll added 6 commits November 29, 2023 06:01

Merge branch 'master' of github.com:skypilot-org/skypilot into gcp-in…

6c28e8f

…ternal-ips

fix merge issue

423b83c

fix

c0731e2

Adopt test fixes from #2681

9675edd

fix test for quota

b90df17

longer timeout

304fc9e

Michaelvll requested a review from concretevitamin November 29, 2023 12:13

concretevitamin reviewed Nov 29, 2023

View reviewed changes

concretevitamin approved these changes Nov 30, 2023

View reviewed changes

Michaelvll added 2 commits November 30, 2023 22:34

Address comments

b8cd23b

format

51ad80f

Michaelvll merged commit a1b0bd3 into master Dec 1, 2023
19 checks passed

Michaelvll deleted the gcp-internal-ips branch December 1, 2023 03:29

Michaelvll mentioned this pull request Dec 16, 2023

[VPC] Fix config yaml env var for spot/serve controller #2874

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GCP] Support private IPs for GCP #2819

[GCP] Support private IPs for GCP #2819

Michaelvll commented Nov 24, 2023 •

edited

concretevitamin left a comment

concretevitamin Nov 28, 2023

Michaelvll Nov 28, 2023

concretevitamin Nov 28, 2023 •

edited

Michaelvll Nov 28, 2023

concretevitamin Nov 28, 2023

concretevitamin left a comment

concretevitamin Nov 28, 2023 •

edited

Michaelvll commented Nov 29, 2023 •

edited

concretevitamin Nov 29, 2023

Michaelvll Nov 29, 2023 •

edited

concretevitamin Nov 29, 2023

concretevitamin Nov 29, 2023

concretevitamin Nov 30, 2023

concretevitamin left a comment

concretevitamin Nov 30, 2023

concretevitamin Nov 30, 2023

		@@ -119,6 +119,19 @@ def set_nested(keys: Iterable[str], value: Any) -> Dict[str, Any]:
		return to_return


		def overwrite_config_file(config: dict) -> None:



		def setup_proxy_command_on_controller():
		def _get_skypilot_config_for_controller_task(

[GCP] Support private IPs for GCP #2819

[GCP] Support private IPs for GCP #2819

Conversation

Michaelvll commented Nov 24, 2023 • edited

Use Internal IP

Cloud NAT Setup

concretevitamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concretevitamin Nov 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concretevitamin left a comment

Choose a reason for hiding this comment

concretevitamin Nov 28, 2023 • edited

Choose a reason for hiding this comment

Michaelvll commented Nov 29, 2023 • edited

Choose a reason for hiding this comment

Michaelvll Nov 29, 2023 • edited

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

concretevitamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll commented Nov 24, 2023 •

edited

concretevitamin Nov 28, 2023 •

edited

concretevitamin Nov 28, 2023 •

edited

Michaelvll commented Nov 29, 2023 •

edited

Michaelvll Nov 29, 2023 •

edited