Repository: skypilot-org/skypilot
Pull request: Fix the hardcoded gcp project id
Closes #317. This PR fixes the hardcoded GCP project id in our template file, using the same method in cloud storage for fetching the project id.

Tested:
- [x] `sky launch -c fm-gcp-test --cloud gcp examples/using_file_mounts.yaml` with the `sudo apt update; sudo apt install tree` commented out (due to #629).

Problem statement:
Consider allowing users to configure cloud templates / changing GCP's project_id
(To discuss) Motivations for configurability of cloud templates
- (GCP) users may want to change GCP's project id
- (Azure) similarly, subscriptions etc.
- Users may want to add disks

Downsides of exposing the templates
- Too much control; e.g.,
  - users can mess with the setup commands
  - users can add heterogeneous type clusters, but sky itself doesn't have this notion (yet), which may lead to bugs
We could "mitigate" by saying modify at your own risk.
-----------

Main motivation: some pilot users use GCP.  For this specific use case we can do away with configurability: the system can grab the local default project_id and fill it into the cluster template:
```
gcloud config get-value project 2>/dev/null
```

Hints from issue discussion:
IIUC, this is now solved by `sky check`. Closing for now.
gcp-ray.yml.j2 currently hardcodes the `project_id` field - I think this issue is still open.
> gcp-ray.yml.j2 currently hardcodes the `project_id` field - I think this issue is still open.

Ahh, good catch. Our cloud storage got the project_id from the GCP credential, but not for the VM.

Commit message: Move azure subscription id to auth

Patch to review:
diff --git a/sky/authentication.py b/sky/authentication.py
index 838854b8daf..0894b2c6419 100644
--- a/sky/authentication.py
+++ b/sky/authentication.py
@@ -12,6 +12,7 @@
 from cryptography.hazmat.backends import default_backend
 from Crypto.PublicKey import RSA
 
+from sky import clouds
 from sky.adaptors import aws, gcp
 
 # TODO: Should tolerate if gcloud is not installed. Also,
@@ -81,7 +82,8 @@ def get_or_generate_keys(private_key_path: str, public_key_path: str):
 # Snippets of code inspired from
 # https://github.com/ray-project/ray/blob/master/python/ray/autoscaler/_private/aws/config.py
 # Takes in config, a yaml dict and outputs a postprocessed dict
-def setup_aws_authentication(config):
+def setup_aws_authentication(config, dryrun: bool = False):
+    del dryrun
     config = copy.deepcopy(config)
     private_key_path = config['auth'].get('ssh_private_key', None)
     if private_key_path is None:
@@ -140,7 +142,7 @@ def insert_char_every_n_chars(string, char='\n', every=2):
 # Snippets of code inspired from
 # https://github.com/ray-project/ray/blob/master/python/ray/autoscaler/_private/aws/config.py
 # Takes in config, a yaml dict and outputs a postprocessed dict
-def setup_gcp_authentication(config):
+def setup_gcp_authentication(config, dryrun: bool = False):
     config = copy.deepcopy(config)
     private_key_path = config['auth'].get('ssh_private_key', None)
     if private_key_path is None:
@@ -151,7 +153,8 @@ def setup_gcp_authentication(config):
     public_key_path = get_public_key_path(private_key_path)
     config = copy.deepcopy(config)
 
-    project_id = config['provider']['project_id']
+    project_id = clouds.GCP.get_project_id(dryrun=dryrun)
+    config['provider']['project_id'] = project_id
     compute = gcp.build('compute',
                         'v1',
                         credentials=None,
@@ -208,7 +211,7 @@ def _unexpand_user(path):
 
 
 # Takes in config, a yaml dict and outputs a postprocessed dict
-def setup_azure_authentication(config):
+def setup_azure_authentication(config, dryrun: bool = False):
     # Doesn't need special library calls!
     config = copy.deepcopy(config)
     private_key_path = config['auth'].get('ssh_private_key', None)
@@ -218,6 +221,8 @@ def setup_azure_authentication(config):
 
     private_key_path = os.path.expanduser(private_key_path)
     public_key_path = get_public_key_path(private_key_path)
+    config['provider']['subscription_id'] = clouds.Azure.get_subscription_id(
+        dryrun)
 
     # Generating ssh key if it does not exist
     _, _ = get_or_generate_keys(private_key_path, public_key_path)
diff --git a/sky/backends/backend_utils.py b/sky/backends/backend_utils.py
index e5495808a6d..6359eb05f1e 100644
--- a/sky/backends/backend_utils.py
+++ b/sky/backends/backend_utils.py
@@ -30,7 +30,6 @@
 from sky import global_user_state
 from sky import exceptions
 from sky import sky_logging
-from sky.adaptors import azure
 from sky.skylet import log_lib
 
 if typing.TYPE_CHECKING:
@@ -536,27 +535,6 @@ def write_cluster_config(to_provision: 'resources.Resources',
         instance_type = resources_vars['instance_type']
         aws_default_ami = cloud.get_default_ami(region, instance_type)
 
-    azure_subscription_id = None
-    if isinstance(cloud, clouds.Azure):
-        if dryrun:
-            azure_subscription_id = 'ffffffff-ffff-ffff-ffff-ffffffffffff'
-        else:
-            try:
-                azure_subscription_id = azure.get_subscription_id()
-                if not azure_subscription_id:
-                    raise ValueError  # The error message will be replaced.
-            except ModuleNotFoundError as e:
-                raise ModuleNotFoundError('Unable to import azure python '
-                                          'module. Is azure-cli python package '
-                                          'installed? Try pip install '
-                                          '.[azure] in the sky repo.') from e
-            except Exception as e:
-                raise RuntimeError(
-                    'Failed to get subscription id from azure cli. '
-                    'Make sure you have logged in and run this Azure '
-                    'cli command: "az account set -s <subscription_id>".'
-                ) from e
-
     assert cluster_name is not None
 
     credentials = sky_check.get_cloud_credential_file_mounts()
@@ -575,7 +553,6 @@ def write_cluster_config(to_provision: 'resources.Resources',
                 # AWS only.
                 'aws_default_ami': aws_default_ami,
                 # Azure only.
-                'azure_subscription_id': azure_subscription_id,
                 'resource_group': f'{cluster_name}-{region}',
                 # Ray version.
                 'ray_version': SKY_REMOTE_RAY_VERSION,
@@ -590,7 +567,7 @@ def write_cluster_config(to_provision: 'resources.Resources',
     config_dict['ray'] = yaml_path
     if dryrun:
         return config_dict
-    _add_ssh_to_cluster_config(cloud, yaml_path)
+    _add_auth_to_cluster_config(cloud, yaml_path, dryrun)
     if resources_vars.get('tpu_type') is not None:
         tpu_name = resources_vars.get('tpu_name')
         if tpu_name is None:
@@ -617,7 +594,7 @@ def write_cluster_config(to_provision: 'resources.Resources',
     return config_dict
 
 
-def _add_ssh_to_cluster_config(cloud_type, cluster_config_file):
+def _add_auth_to_cluster_config(cloud_type, cluster_config_file, dryrun: bool):
     """Adds SSH key info to the cluster config.
 
     This function's output removes comments included in the jinja2 template.
@@ -626,11 +603,11 @@ def _add_ssh_to_cluster_config(cloud_type, cluster_config_file):
         config = yaml.safe_load(f)
     cloud_type = str(cloud_type)
     if cloud_type == 'AWS':
-        config = auth.setup_aws_authentication(config)
+        config = auth.setup_aws_authentication(config, dryrun)
     elif cloud_type == 'GCP':
-        config = auth.setup_gcp_authentication(config)
+        config = auth.setup_gcp_authentication(config, dryrun)
     elif cloud_type == 'Azure':
-        config = auth.setup_azure_authentication(config)
+        config = auth.setup_azure_authentication(config, dryrun)
     else:
         raise ValueError('Cloud type not supported, must be [AWS, GCP, Azure]')
     dump_yaml(cluster_config_file, config)
diff --git a/sky/clouds/azure.py b/sky/clouds/azure.py
index a5f6de2e0ac..60dd02c7a60 100644
--- a/sky/clouds/azure.py
+++ b/sky/clouds/azure.py
@@ -6,6 +6,7 @@
 from typing import Dict, Iterator, List, Optional, Tuple
 
 from sky import clouds
+from sky.adaptors import azure
 from sky.clouds import service_catalog
 
 
@@ -240,3 +241,22 @@ def check_credentials(self) -> Tuple[bool, Optional[str]]:
 
     def get_credential_file_mounts(self) -> Tuple[Dict[str, str], List[str]]:
         return {'~/.azure': '~/.azure'}, []
+
+    @classmethod
+    def get_project_id(cls, dryrun: bool = False) -> str:
+        if dryrun:
+            return 'dryrun-project-id'
+        try:
+            azure_subscription_id = azure.get_subscription_id()
+            if not azure_subscription_id:
+                raise ValueError  # The error message will be replaced.
+        except ModuleNotFoundError as e:
+            raise ModuleNotFoundError('Unable to import azure python '
+                                      'module. Is azure-cli python package '
+                                      'installed? Try pip install '
+                                      '.[azure] in the sky repo.') from e
+        except Exception as e:
+            raise RuntimeError(
+                'Failed to get subscription id from azure cli. '
+                'Make sure you have logged in and run this Azure '
+                'cli command: "az account set -s <subscription_id>".') from e
diff --git a/sky/clouds/gcp.py b/sky/clouds/gcp.py
index 6d6c4ae7758..12de197844f 100644
--- a/sky/clouds/gcp.py
+++ b/sky/clouds/gcp.py
@@ -10,6 +10,10 @@
 from sky import clouds
 from sky.clouds import service_catalog
 
+DEFAULT_GCP_APPLICATION_CREDENTIAL_PATH = os.path.expanduser(
+    '~/.config/gcloud/'
+    'application_default_credentials.json')
+
 
 def _run_output(cmd):
     proc = subprocess.run(cmd,
@@ -258,13 +262,15 @@ def check_credentials(self) -> Tuple[bool, Optional[str]]:
                     '~/.config/gcloud/credentials.db'
             ]:
                 assert os.path.isfile(os.path.expanduser(file))
+            # Check if application default credentials are set.
+            self.get_project_id()
             # Calling `auth.default()` ensures the GCP client library works,
             # which is used by Ray Autoscaler to launch VMs.
             auth.default()
             # Check the installation of google-cloud-sdk.
             _run_output('gcloud --version')
         except (AssertionError, auth.exceptions.DefaultCredentialsError,
-                subprocess.CalledProcessError):
+                subprocess.CalledProcessError, FileNotFoundError, KeyError):
             # See also: https://stackoverflow.com/a/53307505/1165051
             return False, (
                 'GCP tools are not installed or credentials are not set. '
@@ -290,3 +296,24 @@ def get_credential_file_mounts(self) -> Tuple[Dict[str, str], List[str]]:
         # TODO(zhwu): rsync_exclude here is unsafe as it may exclude the folder
         # from other file_mounts as well in ray yaml.
         return {'~/.config/gcloud': '~/.config/gcloud'}, ['virtenv']
+
+    @classmethod
+    def get_project_id(cls, dryrun: bool = False) -> str:
+        if dryrun:
+            return 'dryrun-project-id'
+        if 'GOOGLE_APPLICATION_CREDENTIALS' in os.environ:
+            gcp_credential_path = os.environ['GOOGLE_APPLICATION_CREDENTIALS']
+        else:
+            gcp_credential_path = DEFAULT_GCP_APPLICATION_CREDENTIAL_PATH
+        if not os.path.exists(gcp_credential_path):
+            raise FileNotFoundError(f'No GCP credentials found at '
+                                    f'{gcp_credential_path}. Please set the '
+                                    f'GOOGLE_APPLICATION_CREDENTIALS '
+                                    f'environment variable to point to '
+                                    f'the path of your credentials file.')
+
+        with open(gcp_credential_path, 'r') as fp:
+            gcp_credentials = json.load(fp)
+        project_id = gcp_credentials.get('quota_project_id',
+                                         None) or gcp_credentials['project_id']
+        return project_id
diff --git a/sky/data/data_transfer.py b/sky/data/data_transfer.py
index 96f1d27235b..4ef92824c30 100644
--- a/sky/data/data_transfer.py
+++ b/sky/data/data_transfer.py
@@ -20,6 +20,7 @@
 import os
 from typing import Any
 
+from sky import clouds
 from sky import sky_logging
 from sky.adaptors import aws, gcp
 
@@ -28,10 +29,6 @@
 S3Store = Any
 GcsStore = Any
 
-DEFAULT_GCS_CREDENTIALS_PATH = os.path.expanduser(
-    '~/.config/gcloud/'
-    'application_default_credentials.json')
-
 
 def s3_to_gcs(s3_bucket_name: str, gs_bucket_name: str) -> None:
     """Creates a one-time transfer from Amazon S3 to Google Cloud Storage.
@@ -51,21 +48,7 @@ def s3_to_gcs(s3_bucket_name: str, gs_bucket_name: str) -> None:
     session = aws.session()
     aws_credentials = session.get_credentials().get_frozen_credentials()
 
-    if 'GOOGLE_APPLICATION_CREDENTIALS' in os.environ:
-        gcp_credential_path = os.environ['GOOGLE_APPLICATION_CREDENTIALS']
-    else:
-        gcp_credential_path = DEFAULT_GCS_CREDENTIALS_PATH
-    if not os.path.exists(gcp_credential_path):
-        raise FileNotFoundError(f'No GCP credentials found at '
-                                f'{gcp_credential_path}. Please set the '
-                                f'GOOGLE_APPLICATION_CREDENTIALS '
-                                f'environment variable to point to '
-                                f'the path of your credentials file.')
-
-    with open(gcp_credential_path, 'r') as fp:
-        gcp_credentials = json.load(fp)
-    project_id = gcp_credentials.get('quota_project_id',
-                                     None) or gcp_credentials['project_id']
+    project_id = clouds.GCP.get_project_id()
 
     # Update cloud bucket IAM role to allow for data transfer
     storage_account = storagetransfer.googleServiceAccounts().get(
diff --git a/sky/templates/azure-ray.yml.j2 b/sky/templates/azure-ray.yml.j2
index d6631c228c4..9bbd82c897e 100644
--- a/sky/templates/azure-ray.yml.j2
+++ b/sky/templates/azure-ray.yml.j2
@@ -17,8 +17,8 @@ provider:
     # Keep (otherwise cannot reuse when re-provisioning).
     # teardown(terminate=True) will override this.
     cache_stopped_nodes: True
-    # subscription id of the azure user
-    subscription_id: {{azure_subscription_id}}
+    # subscription id of the azure user will be filled out by sky from Azure credential.
+    # subscription_id: <your-subscription-id>
 
 auth:
     ssh_user: azureuser
diff --git a/sky/templates/gcp-ray.yml.j2 b/sky/templates/gcp-ray.yml.j2
index c0fc7b84ce0..147ef235d9f 100644
--- a/sky/templates/gcp-ray.yml.j2
+++ b/sky/templates/gcp-ray.yml.j2
@@ -14,10 +14,11 @@ provider:
   module: sky.skylet.providers.gcp.GCPNodeProvider
   region: {{region}}
   availability_zone: {{zones}}
-  project_id: intercloud-320520  # Globally unique project id
   # Keep (otherwise cannot reuse when re-provisioning).
   # teardown(terminate=True) will override this.
   cache_stopped_nodes: True
+  # The GCP project ID, will be fill out by sky automatically from gcp credential.
+  # project_id: <your-project-id>
 
 auth:
   ssh_user: gcpuser


Code review comments:
diff_hunk: @@ -14,10 +14,11 @@ provider:
   module: sky.skylet.providers.gcp.GCPNodeProvider
   region: {{region}}
   availability_zone: {{zones}}
-  project_id: intercloud-320520  # Globally unique project id
   # Keep (otherwise cannot reuse when re-provisioning).
   # teardown(terminate=True) will override this.
   cache_stopped_nodes: True
+  # The GCP project ID, will be fill out by sky automatically from gcp credential.
path: sky/templates/gcp-ray.yml.j2
text: @user1:
Do we need `automatically from gcp credential.`?