Repository: google/cloud-forensics-utils

Issue:
Consider adding deleteInstance/deleteDisks 
This would be nice to have when implementing CI tests, and we want a way to cleanup stuff

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/google/cloud-forensics-utils/pull/266?src=pr&el=h1) Report
> Merging [#266](https://codecov.io/gh/google/cloud-forensics-utils/pull/266?src=pr&el=desc) into [master](https://codecov.io/gh/google/cloud-forensics-utils/commit/e4448c60002eee03c344ec2803276ff5d91936be?el=desc) will **increase** coverage by `2.01%`.
> The diff coverage is `69.85%`.

[![Impacted file tree graph](https://codecov.io/gh/google/cloud-forensics-utils/pull/266/graphs/tree.svg?width=650&height=150&src=pr&token=zZkPno3ah0)](https://codecov.io/gh/google/cloud-forensics-utils/pull/266?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
+ Coverage   65.39%   67.40%   +2.01%     
==========================================
  Files          14       29      +15     
  Lines         968     2019    +1051     
==========================================
+ Hits          633     1361     +728     
- Misses        335      658     +323     
```

| Flag | Coverage Δ | |
|---|---|---|
| #nosetests | `67.40% <69.85%> (+2.01%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/google/cloud-forensics-utils/pull/266?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...cloudforensics/providers/azure/internal/network.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/266/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2F6dXJlL2ludGVybmFsL25ldHdvcmsucHk=) | `24.52% <24.52%> (ø)` | |
| [libcloudforensics/providers/aws/internal/kms.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/266/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2F3cy9pbnRlcm5hbC9rbXMucHk=) | `40.00% <40.00%> (ø)` | |
| [libcloudforensics/providers/gcp/forensics.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/266/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2djcC9mb3JlbnNpY3MucHk=) | `54.54% <48.00%> (-10.46%)` | :arrow_down: |
| [...ibcloudforensics/providers/gcp/internal/compute.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/266/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2djcC9pbnRlcm5hbC9jb21wdXRlLnB5) | `53.15% <48.44%> (-9.20%)` | :arrow_down: |
| [libcloudforensics/providers/aws/forensics.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/266/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2F3cy9mb3JlbnNpY3MucHk=) | `56.16% <53.06%> (-13.61%)` | :arrow_down: |
| [...cs/providers/gcp/internal/compute\_base\_resource.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/266/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2djcC9pbnRlcm5hbC9jb21wdXRlX2Jhc2VfcmVzb3VyY2UucHk=) | `37.33% <54.28%> (+0.82%)` | :arrow_up: |
| [libcloudforensics/providers/aws/internal/log.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/266/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2F3cy9pbnRlcm5hbC9sb2cucHk=) | `73.07% <55.55%> (-4.20%)` | :arrow_down: |
| [...bcloudforensics/providers/gcp/internal/function.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/266/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2djcC9pbnRlcm5hbC9mdW5jdGlvbi5weQ==) | `40.54% <62.50%> (+8.28%)` | :arrow_up: |
| [...loudforensics/providers/azure/internal/resource.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/266/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2F6dXJlL2ludGVybmFsL3Jlc291cmNlLnB5) | `65.38% <65.38%> (ø)` | |
| [...ibcloudforensics/providers/gcp/internal/project.py](https://codecov.io/gh/google/cloud-forensics-utils/pull/266/diff?src=pr&el=tree#diff-bGliY2xvdWRmb3JlbnNpY3MvcHJvdmlkZXJzL2djcC9pbnRlcm5hbC9wcm9qZWN0LnB5) | `61.90% <66.66%> (-3.81%)` | :arrow_down: |
| ... and [35 more](https://codecov.io/gh/google/cloud-forensics-utils/pull/266/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/cloud-forensics-utils/pull/266?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/cloud-forensics-utils/pull/266?src=pr&el=footer). Last update [375fb7c...2196cad](https://codecov.io/gh/google/cloud-forensics-utils/pull/266?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

sa3eed3ed: @rgayon can you please add unit-tests or open a new issue and add them in a subsequent PR?
rgayon: > @rgayon can you please add unit-tests or open a new issue and add them in a subsequent PR?

So I actually wonder what I should be testing for, if I'm mock'ing gce_image_client.delete() & gce_disk_client.delete() as I'm not using their result whatsoever.
This is similar to how gce_image_client.delete() & gce_snapshot_client.delete() don't have unit tests either
sa3eed3ed: > > @rgayon can you please add unit-tests or open a new issue and add them in a subsequent PR?
> 
> So I actually wonder what I should be testing for, if I'm mock'ing gce_image_client.delete() & gce_disk_client.delete() as I'm not using their result whatsoever.
> This is similar to how gce_image_client.delete() & gce_snapshot_client.delete() don't have unit tests either

yes thats true, I think what can be tested here is the delete_disks flag to make sure that disks_to_delete is what is expected to get (from the mock with disks.source fields ).
rgayon: > > > @rgayon can you please add unit-tests or open a new issue and add them in a subsequent PR?
> > 
> > 
> > So I actually wonder what I should be testing for, if I'm mock'ing gce_image_client.delete() & gce_disk_client.delete() as I'm not using their result whatsoever.
> > This is similar to how gce_image_client.delete() & gce_snapshot_client.delete() don't have unit tests either
> 
> yes thats true, I think what can be tested here is the delete_disks flag to make sure that disks_to_delete is what is expected to get (from the mock with disks.source fields ).

Added a test to check that the disks().delete() method is called

rgayon: > little things

ack
hacktobeer: sa3eed3ed did a solid review, lgtm from me.

Patch:
diff --git a/libcloudforensics/errors.py b/libcloudforensics/errors.py
index d6201c4..2edb5cb 100644
--- a/libcloudforensics/errors.py
+++ b/libcloudforensics/errors.py
@@ -60,5 +60,7 @@ class ResourceNotFoundError(LCFError):
 
 class ResourceCreationError(LCFError):
   """Error when an issue with creating a new resource is encountered."""
+
+
 class ResourceDeletionError(LCFError):
   """Error when an issue with deleting a resource is encountered."""
diff --git a/libcloudforensics/providers/gcp/internal/compute.py b/libcloudforensics/providers/gcp/internal/compute.py
index 2b9071a..59f2a1c 100644
--- a/libcloudforensics/providers/gcp/internal/compute.py
+++ b/libcloudforensics/providers/gcp/internal/compute.py
@@ -547,7 +547,7 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
       gcs_uri = os.path.join(common.STORAGE_LINK_URL, gcs_uri)
     image_body = {
         'name': name,
-        "rawDisk": {
+        'rawDisk': {
             'source': gcs_uri
         }
     }
@@ -687,7 +687,7 @@ class GoogleCloudCompute(common.GoogleCloudComputeClient):
             'env': ['BUILD_ID=$BUILD_ID']
         }],
         'timeout': '86400s',
-        'tags': ["gce-daisy", "gce-daisy-image-import"]
+        'tags': ['gce-daisy', 'gce-daisy-image-import']
     }
     cloud_build = build.GoogleCloudBuild(self.project_id)
     response = cloud_build.CreateBuild(build_body)
@@ -854,6 +854,48 @@ class GoogleComputeInstance(compute_base_resource.GoogleComputeBaseResource):
     response = request.execute()
     self.BlockOperation(response, zone=self.zone)
 
+  def Delete(self, delete_disks: bool = False) -> None:
+    """Delete an Instance.
+
+    Args:
+      delete_disks (bool): force delete all attached disks (ignores the 'Keep
+        when instance is deleted' bit).
+    """
+    disks_to_delete = []
+    if delete_disks:
+      disks_to_delete = [
+          disk['source'].split('/')[-1] for disk in self.GetValue('disks')]
+
+    gce_instance_client = self.GceApi().instances()
+    logger.info(
+        self.FormatLogMessage('Deleting Instance: {0:s}'.format(self.name)))
+    try:
+      request = gce_instance_client.delete(
+          project=self.project_id, instance=self.name, zone=self.zone)
+      response = request.execute()
+    except HttpError as exception:
+      if exception.resp.status == 404:
+        logger.warning(
+            ('Can not find resource {0:s}, it might be already '
+             'deleted. API call resulted in the following error: '
+             '{1:s}').format(self.name, str(exception)))
+      else:
+        logger.error((
+            'While deleting GCE instance {0:s} the following error occurred: '
+            '{1:s}').format(self.name, str(exception)))
+        raise errors.ResourceDeletionError
+
+    self.BlockOperation(response, zone=self.zone)
+
+    for disk_name in disks_to_delete:
+      try:
+        disk = GoogleCloudCompute(self.project_id).GetDisk(disk_name=disk_name)
+        disk.Delete()
+      except RuntimeError:
+        logger.info(
+            self.FormatLogMessage(
+                'Could not find disk: {0:s}, skipping'.format(disk_name)))
+
 
 class GoogleComputeDisk(compute_base_resource.GoogleComputeBaseResource):
   """Class representing a Compute Engine disk."""
@@ -915,6 +957,28 @@ class GoogleComputeDisk(compute_base_resource.GoogleComputeBaseResource):
     self.BlockOperation(response, zone=self.zone)
     return GoogleComputeSnapshot(disk=self, name=snapshot_name)
 
+  def Delete(self) -> None:
+    """Delete a Disk."""
+
+    gce_disk_client = self.GceApi().disks()
+    try:
+      request = gce_disk_client.delete(
+          project=self.project_id, disk=self.name, zone=self.zone)
+      request.execute()
+    except HttpError as exception:
+      if exception.resp.status == 404:
+        logger.warning(
+            ('Can not find resource {0:s}, it might be already '
+             'deleted. API call resulted in the following error: '
+             '{1:s}').format(self.name, str(exception)))
+      else:
+        logger.error((
+            'While deleting GCE disk {0:s} the following error occurred: '
+            '{1:s}').format(self.name, str(exception)))
+        raise errors.ResourceDeletionError
+    logger.info(
+        self.FormatLogMessage('Deleted Disk: {0:s}'.format(self.name)))
+
   def GetDiskType(self) -> str:
     """Return the disk type.
 
@@ -1033,8 +1097,7 @@ class GoogleComputeImage(compute_base_resource.GoogleComputeBaseResource):
         'Image {0:s} exported to {1:s}.'.format(self.name, full_path))
 
   def Delete(self) -> None:
-    """Delete Compute Disk Image from a project.
-    """
+    """Delete Compute Disk Image from a project."""
 
     gce_image_client = self.GceApi().images()
     request = gce_image_client.delete(project=self.project_id, image=self.name)
diff --git a/tools/cli.py b/tools/cli.py
index 1d3144b..5eefa75 100644
--- a/tools/cli.py
+++ b/tools/cli.py
@@ -42,6 +42,7 @@ PROVIDER_TO_FUNC = {
     'gcp': {
         'copydisk': gcp_cli.CreateDiskCopy,
         'creatediskgcs': gcp_cli.CreateDiskFromGCSImage,
+        'deleteinstance': gcp_cli.DeleteInstance,
         'listdisks': gcp_cli.ListDisks,
         'listinstances': gcp_cli.ListInstances,
         'listlogs': gcp_cli.ListLogs,
@@ -309,6 +310,13 @@ def Main() -> None:
                 ('--attach_disks', 'Comma separated list of disk names '
                                    'to attach.', None)
             ])
+  AddParser('gcp', gcp_subparsers, 'deleteinstance', 'Delete a GCE instance.',
+            args=[
+                ('instance_name', 'Name of the GCE instance to delete.', ''),
+                ('--delete_all_disks',
+                 'Force delete disks marked as "Keep when deleting".',
+                 False),
+            ])
   AddParser('gcp', gcp_subparsers, 'querylogs', 'Query GCP logs.',
             args=[
                 ('--filter', 'Query filter', None),
diff --git a/tools/gcp_cli.py b/tools/gcp_cli.py
index 4aa2255..76e5f57 100644
--- a/tools/gcp_cli.py
+++ b/tools/gcp_cli.py
@@ -20,6 +20,7 @@ import sys
 from typing import TYPE_CHECKING
 
 # pylint: disable=line-too-long
+from libcloudforensics.providers.gcp.internal import compute as gcp_compute
 from libcloudforensics.providers.gcp.internal import log as gcp_log
 from libcloudforensics.providers.gcp.internal import monitoring as gcp_monitoring
 from libcloudforensics.providers.gcp.internal import project as gcp_project
@@ -85,6 +86,20 @@ def CreateDiskCopy(args: 'argparse.Namespace') -> None:
   logger.info('Name: {0:s}'.format(disk.name))
 
 
+def DeleteInstance(args: 'argparse.Namespace') -> None:
+  """Deletes a GCE instance.
+
+  Args:
+    args (argparse.Namespace): Arguments from ArgumentParser.
+  """
+
+  compute_client = gcp_compute.GoogleCloudCompute(args.project)
+  instance = compute_client.GetInstance(instance_name=args.instance_name)
+  instance.Delete(delete_disks=args.delete_disks)
+
+  print('Instance deleted.')
+
+
 def ListLogs(args: 'argparse.Namespace') -> None:
   """List GCP logs for a project.
 


Test patch:
diff --git a/tests/providers/gcp/internal/test_compute.py b/tests/providers/gcp/internal/test_compute.py
index d4a7e7c..14bcc0e 100644
--- a/tests/providers/gcp/internal/test_compute.py
+++ b/tests/providers/gcp/internal/test_compute.py
@@ -270,7 +270,7 @@ class GoogleCloudComputeTest(unittest.TestCase):
     self.assertIn('fake-image', image_object.name)
     fake_image_body = {
         'name': 'fake-image',
-        "rawDisk": {
+        'rawDisk': {
             'source': 'https://storage.cloud.google.com/fake-bucket/fake-folder/image.tar.gz'
         }
     }
@@ -344,7 +344,7 @@ class GoogleComputeInstanceTest(unittest.TestCase):
   @mock.patch('libcloudforensics.providers.gcp.internal.compute.GoogleCloudCompute.ListDisks')
   @mock.patch('libcloudforensics.providers.gcp.internal.compute.GoogleComputeInstance.GetOperation')
   def testListDisks(self, mock_get_operation, mock_list_disks):
-    """Test that a all disks of an instance are correctly retrieved."""
+    """Test that all disks of an instance are correctly retrieved."""
     mock_get_operation.return_value = gcp_mocks.MOCK_GCE_OPERATION_INSTANCES_GET
     mock_list_disks.return_value = gcp_mocks.MOCK_LIST_DISKS
 
@@ -352,6 +352,31 @@ class GoogleComputeInstanceTest(unittest.TestCase):
     self.assertEqual(2, len(disks))
     self.assertEqual(['fake-boot-disk', 'fake-disk'], list(disks.keys()))
 
+  @typing.no_type_check
+  @mock.patch('libcloudforensics.providers.gcp.internal.compute.GoogleCloudCompute.ListDisks')
+  @mock.patch('libcloudforensics.providers.gcp.internal.compute.GoogleComputeInstance.GetOperation')
+  @mock.patch('libcloudforensics.providers.gcp.internal.common.GoogleCloudComputeClient.GceApi')
+  @mock.patch('libcloudforensics.providers.gcp.internal.common.GoogleCloudComputeClient.BlockOperation')
+  def testDelete(
+      self, mock_block_operation, mock_gce_api, mock_get_operation,
+      mock_list_disks):
+    """Test that all disks of an instance are correctly deleted."""
+    mock_block_operation.return_value = None
+    mock_get_operation.return_value = gcp_mocks.MOCK_GCE_OPERATION_INSTANCES_GET
+    mock_list_disks.return_value = gcp_mocks.MOCK_LIST_DISKS
+
+    mock_disk_delete = mock_gce_api.return_value.disks.return_value.delete
+    mock_disk_delete.return_value.execute.return_value = None
+
+    gcp_mocks.FAKE_INSTANCE.Delete(delete_disks=True)
+    calls = [
+        mock.call(project='fake-source-project', disk='fake-boot-disk', zone='fake-zone'),
+        mock.call().execute(),
+        mock.call(project='fake-source-project', disk='fake-disk', zone='fake-zone'),
+        mock.call().execute(),
+    ]
+    mock_disk_delete.assert_has_calls(calls)
+
 
 class GoogleComputeDiskTest(unittest.TestCase):
   """Test Google Cloud Compute Disk class."""
@@ -380,3 +405,7 @@ class GoogleComputeDiskTest(unittest.TestCase):
     # Snapshot(snapshot_name='Non-compliant-name'). Should raise a ValueError
     with self.assertRaises(errors.InvalidNameError):
       gcp_mocks.FAKE_DISK.Snapshot('Non-compliant-name')
+
+
+if __name__ == '__main__':
+  unittest.main()