Repository: google/cloud-forensics-utils

Issue:
Add support for specifying resources by ID as well as name
Add support for accessing resources by ID as well as name, for example instance ID for GetInstance.
Instance names are treated as project-wide unique identifiers
## Issue Description
Instance names are only unique within a zone, not project wide.

For example [libcloudforensics.providers.gcp.internal.compute.ListInstances](https://github.com/google/cloud-forensics-utils/blob/9a0a583c12e3d17d677d396f985bdbe019c1ff26/libcloudforensics/providers/gcp/internal/compute.py#L124) generates a dictionary of all instances in a project keyed on instance name. If there are more than one instance in the project with the same name only one will be included in the returned dict.

## Possible Fixes:
1. Use instance IDs instead of instance names - a less human-friendly value but helper functions could be added to still access instances by name etc.
2. Use zone:instance_name to refer to instances.

Discussion:
codecov-commenter: # [Codecov](https://codecov.io/gh/google/cloud-forensics-utils/pull/440?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) Report
> :exclamation: No coverage uploaded for pull request base (`main@9a0a583`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google#section-missing-base-commit).
> The diff coverage is `n/a`.

> :exclamation: Current head b458b92 differs from pull request most recent head 4901fbb. Consider uploading reports for the commit 4901fbb to get more accurate results

```diff
@@           Coverage Diff           @@
##             main     #440   +/-   ##
=======================================
  Coverage        ?   59.11%           
=======================================
  Files           ?       49           
  Lines           ?     4041           
  Branches        ?        0           
=======================================
  Hits            ?     2389           
  Misses          ?     1652           
  Partials        ?        0           
```

| Flag | Coverage Δ | |
|---|---|---|
| nosetests | `59.11% <0.00%> (?)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google#carryforward-flags-in-the-pull-request-comment) to find out more.


------

[Continue to review full report at Codecov](https://codecov.io/gh/google/cloud-forensics-utils/pull/440?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/cloud-forensics-utils/pull/440?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google). Last update [9a0a583...4901fbb](https://codecov.io/gh/google/cloud-forensics-utils/pull/440?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google).


Patch:
diff --git a/libcloudforensics/errors.py b/libcloudforensics/errors.py
index 38714e8..9256e8f 100644
--- a/libcloudforensics/errors.py
+++ b/libcloudforensics/errors.py
@@ -89,5 +89,10 @@ class TransferCreationError(LCFError):
 class TransferExecutionError(LCFError):
   """Error when an issue with running a transfer job is encountered."""
 
+
 class ResourceAlreadyExistsError(LCFError):
   """Error when trying to create a resource with existing name."""
+
+
+class AmbiguousIdentifierError(LCFError):
+  """Error when an identifier could refer to more than one resource."""
diff --git a/libcloudforensics/providers/gcp/internal/compute.py b/libcloudforensics/providers/gcp/internal/compute.py
index 0e63d65..c5e690c 100644
--- a/libcloudforensics/providers/gcp/internal/compute.py
+++ b/libcloudforensics/providers/gcp/internal/compute.py
@@ -14,12 +14,13 @@
 # limitations under the License.
 """Google Compute Engine functionalities."""
 
+# pylint: disable=line-too-long
 import os
 import re
 import subprocess
 import time
 from collections import defaultdict
-from typing import Dict, Tuple, List, TYPE_CHECKING, Union, Optional, Any
+from typing import Any, Dict, List, Optional, Tuple, TypeVar, TYPE_CHECKING, Union
 
 from googleapiclient.errors import HttpError
 
@@ -32,6 +33,7 @@ from libcloudforensics import errors
 
 if TYPE_CHECKING:
   import googleapiclient
+# pylint: enable=line-too-long
 
 logging_utils.SetUpLogger(__name__)
 logger = logging_utils.GetLogger(__name__)
@@ -48,7 +50,10 @@ NON_HIERARCHICAL_FW_POLICY_LEVEL = 999
 
 # Will only matches IDs as names can't start with a number
 # https://cloud.google.com/compute/docs/naming-resources#resource-name-format
-RESOURCE_ID_REGEX = r'^\d{19}$'
+RESOURCE_ID_REGEX = r'^\d+$'
+
+ComputeResource = TypeVar(
+  'ComputeResource', bound='compute_base_resource.GoogleComputeBaseResource')
 
 
 class GoogleCloudCompute(common.GoogleCloudComputeClient):
@@ -77,6 +82,62 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
     self._region_disks = {}  # type: Dict[str, GoogleRegionComputeDisk]
     super().__init__(self.project_id)
 
+  def _FindResourceByName(
+      self,
+      resources: Dict[str, ComputeResource],
+      name: str,
+      zone: Optional[str] = None,
+      region: Optional[str] = None
+  ) -> ComputeResource:
+    """A helper function for finding compute resources by name.
+
+    The resources can be zonal or regional.
+
+    Args:
+      resources: A dict of resources with resource IDs as keys and resource
+        objects as values.
+      name: The name of the resource to find.
+      zone: The zone containing the resource.
+      region: The region containing the resource.
+
+    Returns:
+      A subclass of 'compute_base_resource.GoogleComputeBaseResource' depending
+        on the resource type.
+
+    Raises:
+      ValueError: If both zone and region is provided.
+      ResourceNotFoundError: If the resource name is not found in resources.
+      AmbiguousIdentifierError: If name matches multiple resources.
+    """
+    if zone and region:
+      raise ValueError(
+          'The resource must be either zonal or regional: '
+          '_FindResourceByName is called with a zone and a region.')
+
+    if zone:
+      matches = [resource for resource in resources.values()
+                if resource.name == name and resource.zone == zone]
+    elif region:
+      matches = [resource for resource in resources.values()
+                if resource.name == name and resource.region == region]
+    else:
+      matches = [resource for resource in resources.values()
+                 if resource.name == name]
+
+    if not matches:
+      raise errors.ResourceNotFoundError(
+        f'Resource {name} was not found in project {self.project_id}',
+        __name__)
+    if len(matches) > 1:
+      location : List [str] = [
+          resource.zone or resource.region or '_' for resource in matches]
+      raise errors.AmbiguousIdentifierError(
+          f'Multiple resources found matching {name} in zones/regions '
+          f'{", ".join(location)} in project {self.project_id}. Either '
+          f'provide a resource ID or a zone argument.', __name__)
+
+    return matches.pop()
+
   def Instances(self,
                 refresh: bool = True) -> Dict[str, 'GoogleComputeInstance']:
     """Get all instances in the project.
@@ -85,7 +146,7 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
       refresh (boolean): Optional. Returns refreshed result if True.
 
     Returns:
-      Dict[str, GoogleComputeInstance]: Dictionary mapping instance names
+      Dict[str, GoogleComputeInstance]: Dictionary mapping instance IDs
           (str) to their respective GoogleComputeInstance object.
     """
     if not refresh and self._instances:
@@ -100,7 +161,7 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
       refresh (boolean): Optional. Returns refreshed result if True.
 
     Returns:
-      Dict[str, GoogleComputeDisk]: Dictionary mapping disk names (str) to
+      Dict[str, GoogleComputeDisk]: Dictionary mapping disk IDs (str) to
           their respective GoogleComputeDisk object.
     """
     if not refresh and self._disks:
@@ -117,7 +178,7 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
       refresh: Returns refreshed result if True.
 
     Returns:
-      Dictionary mapping disk names (str) to their respective
+      Dictionary mapping disk IDs (str) to their respective
         GoogleRegionComputeDisk object.
     """
     if not refresh and self._region_disks:
@@ -129,7 +190,7 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
     """List instances in project.
 
     Returns:
-      Dict[str, GoogleComputeInstance]: Dictionary mapping instance names (str)
+      Dict[str, GoogleComputeInstance]: Dictionary mapping instance IDs (str)
           to their respective GoogleComputeInstance object.
     """
 
@@ -146,7 +207,7 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
             name = instance['name']
             resource_id = instance['id']
             deletion_protection = instance.get('deletionProtection', False)
-            instances[name] = GoogleComputeInstance(
+            instances[resource_id] = GoogleComputeInstance(
                 self.project_id,
                 zone,
                 name,
@@ -252,16 +313,17 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
         gce_disk_client, 'aggregatedList', {'project': self.project_id})
 
     for response in responses:
-      for zone in response['items']:
-        try:
-          for disk in response['items'][zone]['disks']:
-            _, zone = disk['zone'].rsplit('/', 1)
-            name = disk['name']
-            disks[name] = GoogleComputeDisk(
-                self.project_id, zone, name, labels=disk.get('labels'))
-        except KeyError:
-          pass
-
+      # Disks.aggregatedList returns both zones and regions in items.
+      for location in response['items']:
+        for disk in response['items'][location].get('disks', []):
+          # Skip if regional disk, i.e. has no zone.
+          if not disk.get('zone'):
+            continue
+          name = disk['name']
+          resource_id = disk['id']
+          _, zone = disk['zone'].rsplit('/', 1)
+          disks[resource_id] = GoogleComputeDisk(
+              self.project_id, zone, name, labels=disk.get('labels'))
     return disks
 
   def ListComputeRegions(self) -> List[str]:
@@ -299,17 +361,22 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
         disks = response.get('items', [])
         for disk in disks:
           name = disk['name']
-          region_disks[name] = GoogleRegionComputeDisk(
+          resource_id = disk['id']
+          region_disks[resource_id] = GoogleRegionComputeDisk(
               self.project_id, region, name, labels=disk.get('labels'))
     return region_disks
 
-  def GetRegionDisk(self, disk_name: str) -> 'GoogleRegionComputeDisk':
+  def GetRegionDisk(
+      self,
+      disk_name: str,
+      region: Optional[str] = None) -> 'GoogleRegionComputeDisk':
     """Get regional disk in project.
 
     Regional disks API resource: https://cloud.google.com/compute/docs/reference/rest/v1/regionDisks#resource:-disk  # pylint: disable=line-too-long
 
     Args:
-      disk_name: name of the regional disk to get.
+      disk_name: Name or ID of the regional disk to get.
+      region: Compute region.
 
     Returns:
       Regional disk object.
@@ -317,21 +384,27 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
     Raises:
       ResourceNotFoundError: When the specified disk cannot be found in project.
     """
-    region_disks = self.RegionDisks()
-    try:
-      return region_disks[disk_name]
-    except KeyError as e:
+    disks = self.RegionDisks()
+    if re.match(RESOURCE_ID_REGEX, disk_name):
+      disk = disks.get(disk_name)
+    else:
+      disk = self._FindResourceByName(disks, disk_name, region=region)
+    if not disk:
       raise errors.ResourceNotFoundError(
-          'Regional disk {0:s} was not found in project {1:s}'.format(
-              disk_name, self.project_id),
-          __name__) from e
+          f'Disk {disk_name} was not found in project '
+          f'{self.project_id}', __name__)
+    return disk
 
-  def GetInstance(self, instance_name: str) -> 'GoogleComputeInstance':
+  def GetInstance(
+      self,
+      instance_name: str,
+      zone: Optional[str] = None) -> 'GoogleComputeInstance':
     """Get instance from project.
 
     Args:
-      instance_name (str): The instance identifier, can be either an instance
+      instance_name: The instance identifier, can be either an instance
         name or ID.
+      zone: Compute zone.
 
     Returns:
       GoogleComputeInstance: A Google Compute Instance object.
@@ -343,24 +416,25 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
     instances = self.Instances()
 
     if re.match(RESOURCE_ID_REGEX, instance_name):
-      id_match = [instance for instance in instances.values()
-                  if instance.resource_id == instance_name]
-      instance = id_match.pop() if id_match else None
-    else:
       instance = instances.get(instance_name)
+    else:
+      instance = self._FindResourceByName(instances, instance_name, zone)
 
     if not instance:
       raise errors.ResourceNotFoundError(
-          'Instance {0:s} was not found in project {1:s}'.format(
-              instance_name, self.project_id),
-          __name__)
+          f'Instance {instance_name} was not found in project '
+          f'{self.project_id}', __name__)
     return instance
 
-  def GetDisk(self, disk_name: str) -> 'GoogleComputeDisk':
+  def GetDisk(
+      self,
+      disk_name: str,
+      zone: Optional[str] = None) -> 'GoogleComputeDisk':
     """Get a GCP disk object.
 
     Args:
-      disk_name (str): Name of the disk.
+      disk_name: The disk identifier, can be either a disk name or ID.
+      zone: Compute zone.
 
     Returns:
       GoogleComputeDisk: Disk object.
@@ -370,13 +444,15 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
     """
 
     disks = self.Disks()
-    try:
-      return disks[disk_name]
-    except KeyError as e:
+    if re.match(RESOURCE_ID_REGEX, disk_name):
+      disk = disks.get(disk_name)
+    else:
+      disk = self._FindResourceByName(disks, disk_name, zone)
+    if not disk:
       raise errors.ResourceNotFoundError(
-          'Disk {0:s} was not found in project {1:s}'.format(
-              disk_name, self.project_id),
-          __name__) from e
+          f'Disk {disk_name} was not found in project '
+          f'{self.project_id}', __name__)
+    return disk
 
   def CreateDiskFromSnapshot(
       self,
diff --git a/tools/gcp_cli.py b/tools/gcp_cli.py
index 55a0cc5..4585293 100644
--- a/tools/gcp_cli.py
+++ b/tools/gcp_cli.py
@@ -62,11 +62,11 @@ def ListInstances(args: 'argparse.Namespace') -> None:
   instances = project.compute.ListInstances()
 
   logger.info('Instances found:')
-  for instance_name, instance in instances.items():
+  for instance in instances.values():
     bootdisk = instance.GetBootDisk()
     if bootdisk:
       logger.info('Name: {0:s}, Bootdisk: {1:s}'.format(
-          instance_name, bootdisk.name))
+          instance.name, bootdisk.name))
 
 
 def ListDisks(args: 'argparse.Namespace') -> None:
@@ -85,8 +85,8 @@ def ListDisks(args: 'argparse.Namespace') -> None:
   project = gcp_project.GoogleCloudProject(args.project)
   disks = project.compute.ListDisks()
   logger.info('Disks found:')
-  for disk_name, disk in disks.items():
-    logger.info('Name: {0:s}, Zone: {1:s}'.format(disk_name, disk.zone))
+  for disk in disks.values():
+    logger.info('Name: {0:s}, Zone: {1:s}'.format(disk.name, disk.zone))
 
 
 def CreateDiskCopy(args: 'argparse.Namespace') -> None:


Test patch:
diff --git a/tests/providers/gcp/gcp_mocks.py b/tests/providers/gcp/gcp_mocks.py
index 912c5fb..ad7b6c6 100644
--- a/tests/providers/gcp/gcp_mocks.py
+++ b/tests/providers/gcp/gcp_mocks.py
@@ -47,6 +47,10 @@ FAKE_INSTANCE = compute.GoogleComputeInstance(
     FAKE_SOURCE_PROJECT.project_id, 'fake-zone', 'fake-instance',
     resource_id='0123456789012345678')
 
+FAKE_INSTANCE_NAME_DUP = compute.GoogleComputeInstance(
+    FAKE_SOURCE_PROJECT.project_id, 'fake-zone2', 'fake-instance',
+    resource_id='1234567890123456789')
+
 FAKE_DISK = compute.GoogleComputeDisk(
     FAKE_SOURCE_PROJECT.project_id, 'fake-zone', 'fake-disk')
 
@@ -136,23 +140,25 @@ MOCK_DISKS_AGGREGATED = {
         0: {
             'disks': [{
                 'name': FAKE_BOOT_DISK.name,
-                'zone': '/' + FAKE_BOOT_DISK.zone
+                'zone': '/' + FAKE_BOOT_DISK.zone,
+		'id': '01234567890123456789'
             }]
         },
         1: {
             'disks': [{
                 'name': FAKE_DISK.name,
-                'zone': '/' + FAKE_DISK.zone
+                'zone': '/' + FAKE_DISK.zone,
+		'id': '0123456789012345678'
             }]
         }
     }
 }
 
-MOCK_LIST_INSTANCES = {FAKE_INSTANCE.name: FAKE_INSTANCE}
+MOCK_LIST_INSTANCES = {'0123456789012345678': FAKE_INSTANCE}
 
 MOCK_LIST_DISKS = {
-    FAKE_DISK.name: FAKE_DISK,
-    FAKE_BOOT_DISK.name: FAKE_BOOT_DISK
+    '0123456789012345678': FAKE_DISK,
+    '01234567890123456789': FAKE_BOOT_DISK
 }
 
 MOCK_GCE_OPERATION_INSTANCES_LABELS_SUCCESS = {
diff --git a/tests/providers/gcp/internal/test_compute.py b/tests/providers/gcp/internal/test_compute.py
index b9dbd82..5de80e0 100644
--- a/tests/providers/gcp/internal/test_compute.py
+++ b/tests/providers/gcp/internal/test_compute.py
@@ -39,6 +39,30 @@ class GoogleCloudComputeTest(unittest.TestCase):
     self.assertIsInstance(formatted_msg, six.string_types)
     self.assertEqual('project:fake-source-project Test message', formatted_msg)
 
+  @typing.no_type_check
+  def testFindResourceByName(self):
+    """Test _findResourceByName handles error cases."""
+    test_compute = gcp_mocks.FAKE_SOURCE_PROJECT.compute
+    test_resources = {
+      gcp_mocks.FAKE_INSTANCE.resource_id: gcp_mocks.FAKE_INSTANCE
+    }
+    test_resources_dup_name = {
+      gcp_mocks.FAKE_INSTANCE.resource_id: gcp_mocks.FAKE_INSTANCE,
+      gcp_mocks.FAKE_INSTANCE_NAME_DUP.resource_id: gcp_mocks.FAKE_INSTANCE_NAME_DUP
+    }
+    # pylint: disable=protected-access
+    self.assertEqual(
+        test_compute._FindResourceByName(test_resources, 'fake-instance'),
+        gcp_mocks.FAKE_INSTANCE)
+    self.assertEqual(
+        test_compute._FindResourceByName(test_resources_dup_name, 'fake-instance', zone='fake-zone'),
+        gcp_mocks.FAKE_INSTANCE)
+    with self.assertRaises(errors.ResourceNotFoundError):
+      test_compute._FindResourceByName(test_resources, 'not-found')
+    with self.assertRaises(errors.AmbiguousIdentifierError):
+      test_compute._FindResourceByName(test_resources_dup_name, 'fake-instance')
+    # pylint: enable=protected-access
+
   @typing.no_type_check
   @mock.patch('libcloudforensics.providers.gcp.internal.common.GoogleCloudComputeClient.GceApi')
   def testListInstances(self, mock_gce_api):
@@ -47,8 +71,8 @@ class GoogleCloudComputeTest(unittest.TestCase):
     instances.return_value.execute.return_value = gcp_mocks.MOCK_INSTANCES_AGGREGATED
     list_instances = gcp_mocks.FAKE_ANALYSIS_PROJECT.compute.ListInstances()
     self.assertEqual(1, len(list_instances))
-    self.assertEqual('fake-instance', list_instances['fake-instance'].name)
-    self.assertEqual('fake-zone', list_instances['fake-instance'].zone)
+    self.assertEqual('fake-instance', list_instances['0123456789012345678'].name)
+    self.assertEqual('fake-zone', list_instances['0123456789012345678'].zone)
 
   @typing.no_type_check
   @mock.patch('libcloudforensics.providers.gcp.internal.common.GoogleCloudComputeClient.BlockOperation')
@@ -66,15 +90,15 @@ class GoogleCloudComputeTest(unittest.TestCase):
   @typing.no_type_check
   @mock.patch('libcloudforensics.providers.gcp.internal.common.GoogleCloudComputeClient.GceApi')
   def testListDisks(self, mock_gce_api):
-    """Test that disks of instances are correctly listed."""
+    """Test that disks are correctly listed."""
     disks = mock_gce_api.return_value.disks.return_value.aggregatedList
     disks.return_value.execute.return_value = gcp_mocks.MOCK_DISKS_AGGREGATED
     list_disks = gcp_mocks.FAKE_ANALYSIS_PROJECT.compute.ListDisks()
     self.assertEqual(2, len(list_disks))
-    self.assertEqual('fake-disk', list_disks['fake-disk'].name)
-    self.assertEqual('fake-boot-disk', list_disks['fake-boot-disk'].name)
-    self.assertEqual('fake-zone', list_disks['fake-disk'].zone)
-    self.assertEqual('fake-zone', list_disks['fake-boot-disk'].zone)
+    self.assertEqual('fake-disk', list_disks['0123456789012345678'].name)
+    self.assertEqual('fake-boot-disk', list_disks['01234567890123456789'].name)
+    self.assertEqual('fake-zone', list_disks['0123456789012345678'].zone)
+    self.assertEqual('fake-zone', list_disks['01234567890123456789'].zone)
 
   @typing.no_type_check
   @mock.patch('libcloudforensics.providers.gcp.internal.compute.GoogleCloudCompute.ListInstances')
diff --git a/tests/providers/gcp/test_forensics.py b/tests/providers/gcp/test_forensics.py
index 5aa3a4a..af0ce86 100644
--- a/tests/providers/gcp/test_forensics.py
+++ b/tests/providers/gcp/test_forensics.py
@@ -112,8 +112,8 @@ class GCPForensicsTest(unittest.TestCase):
   def testCreateDiskCopy3(self, mock_list_disks, mock_list_instances):
     """Test that a disk from a remote project is duplicated and attached to
     an analysis project. """
-    mock_list_disks.return_value = gcp_mocks.MOCK_DISKS_AGGREGATED
-    mock_list_instances.return_value = gcp_mocks.MOCK_INSTANCES_AGGREGATED
+    mock_list_disks.return_value = gcp_mocks.MOCK_LIST_DISKS
+    mock_list_instances.return_value = gcp_mocks.MOCK_LIST_INSTANCES
 
     # create_disk_copy(
     #     src_proj,