Fix: Handle 403 Forbidden in get_reservation_details for shared reservation (#5117)

Author: kvenkatachala333

community/modules/compute/schedmd-slurm-gcp-v6-nodeset/README.md

@@ -94,9 +94,9 @@ The format of the `reservation_name` input determines the type of reservation us
   * `RESERVATION_NAME` is the name assigned to the specific reservation.
   * `BLOCK_ID` (Optional) is the identifier for a specific reservation block, if the reservation is composed of multiple blocks.
 
-> **_NOTE:_** Using a shared reservation requires the 'compute.reservations.get'
-> permission for the node service account in the host project (HOST_PROJECT_ID).
-> Ensure this permission is granted before deploying.
+> **_NOTE:_** Using a shared reservation ideally requires the 'compute.reservations.get'
+> permission for the node service account in the host project (HOST_PROJECT_ID) to fetch full reservation details.
+> However, if this permission is missing, the system will fall back to minimal reservation settings and proceed.
 
 ### No Reservation
 
@@ -240,7 +240,7 @@ modules. For support with the underlying modules, see the instructions in the
 | <a name="input_preemptible"></a> [preemptible](#input\_preemptible) | Should use preemptibles to burst. | `bool` | `false` | no |
 | <a name="input_project_id"></a> [project\_id](#input\_project\_id) | Project ID to create resources in. | `string` | n/a | yes |
 | <a name="input_region"></a> [region](#input\_region) | The default region for Cloud resources. | `string` | n/a | yes |
-| <a name="input_reservation_name"></a> [reservation\_name](#input\_reservation\_name) | Name or path of the reservation to use for VM resources. Must be a "SPECIFIC" reservation.<br/>Set to an empty string if using no reservation or automatically-consumed reservations.<br/><br/>Formats:<br/>- Local Reservation: For reservations in the same project as the cluster (var.project\_id), the name is sufficient:<br/>  RESERVATION\_NAME[/reservationBlocks/BLOCK\_ID]<br/>- Shared Reservation: For reservations shared from a different project, the full resource path is required:<br/>  projects/HOST\_PROJECT\_ID/reservations/RESERVATION\_NAME[/reservationBlocks/BLOCK\_ID]<br/><br/>Where:<br/>- HOST\_PROJECT\_ID: Project ID where the shared reservation was created.<br/>- RESERVATION\_NAME: The name assigned to the specific reservation.<br/>- BLOCK\_ID (Optional): The identifier for a specific reservation block, if the reservation is composed of multiple blocks.<br/><br/>Note: Using a shared reservation requires the 'compute.reservations.get' permission for the node service account in the host project. | `string` | `""` | no |
+| <a name="input_reservation_name"></a> [reservation\_name](#input\_reservation\_name) | Name or path of the reservation to use for VM resources. Must be a "SPECIFIC" reservation.<br/>Set to an empty string if using no reservation or automatically-consumed reservations.<br/><br/>Formats:<br/>- Local Reservation: For reservations in the same project as the cluster (var.project\_id), the name is sufficient:<br/>  RESERVATION\_NAME[/reservationBlocks/BLOCK\_ID]<br/>- Shared Reservation: For reservations shared from a different project, the full resource path is required:<br/>  projects/HOST\_PROJECT\_ID/reservations/RESERVATION\_NAME[/reservationBlocks/BLOCK\_ID]<br/><br/>Where:<br/>- HOST\_PROJECT\_ID: Project ID where the shared reservation was created.<br/>- RESERVATION\_NAME: The name assigned to the specific reservation.<br/>- BLOCK\_ID (Optional): The identifier for a specific reservation block, if the reservation is composed of multiple blocks.<br/><br/>Note: Using a shared reservation ideally requires the 'compute.reservations.get' permission for the node service account in the host project; without it, full details cannot be fetched, but deployment will still proceed with defaults. | `string` | `""` | no |
 | <a name="input_resource_manager_tags"></a> [resource\_manager\_tags](#input\_resource\_manager\_tags) | (Optional) A set of key/value resource manager tag pairs to bind to the instances. Keys must be in the format tagKeys/{tag\_key\_id}, and values are in the format tagValues/456. | `map(string)` | `{}` | no |
 | <a name="input_service_account"></a> [service\_account](#input\_service\_account) | DEPRECATED: Use `service_account_email` and `service_account_scopes` instead. | <pre>object({<br/>    email  = string<br/>    scopes = set(string)<br/>  })</pre> | `null` | no |
 | <a name="input_service_account_email"></a> [service\_account\_email](#input\_service\_account\_email) | Service account e-mail address to attach to the compute instances. | `string` | `null` | no |

community/modules/compute/schedmd-slurm-gcp-v6-nodeset/variables.tf

@@ -524,7 +524,7 @@ variable "reservation_name" {
     - RESERVATION_NAME: The name assigned to the specific reservation.
     - BLOCK_ID (Optional): The identifier for a specific reservation block, if the reservation is composed of multiple blocks.
 
-    Note: Using a shared reservation requires the 'compute.reservations.get' permission for the node service account in the host project.
+    Note: Using a shared reservation ideally requires the 'compute.reservations.get' permission for the node service account in the host project; without it, full details cannot be fetched, but deployment will still proceed with defaults.
   EOD
   type        = string
   default     = ""

community/modules/scheduler/schedmd-slurm-gcp-v6-controller/modules/slurm_files/scripts/tests/test_util.py

@@ -23,6 +23,7 @@
 import util
 from util import NodeState, MachineType, AcceleratorInfo, UpcomingMaintenance, InstanceResourceStatus, FutureReservation, ReservationDetails
 from google.api_core.client_options import ClientOptions  # noqa: E402
+from googleapiclient.errors import HttpError # type: ignore
 from addict import Dict as NSDict # type: ignore
 
 # Note: need to install pytest-mock
@@ -710,3 +711,50 @@ def test_slurm_version(stdout_data, exception_to_raise, expected_version, mocker
 
     assert version == expected_version
     mock_run.assert_called_once()
+
+
+def test_get_reservation_details_403(mocker):
+    lkp = util.Lookup(TstCfg())
+    
+    # Mock HttpError 403
+    mock_resp = Mock()
+    mock_resp.status = 403
+    error = HttpError(mock_resp, b'Forbidden')
+    
+    lkp._get_reservation = Mock(side_effect=error)
+    
+    details = lkp.get_reservation_details(
+        project="my-project",
+        zone="us-central1-a",
+        name="my-reservation",
+        bulk_insert_name="projects/my-project/reservations/my-reservation"
+    )
+    
+    assert details.policies == []
+    assert details.deployment_type is None
+    assert details.reservation_mode is None
+    assert details.assured_count == 0
+    assert details.bulk_insert_name == "projects/my-project/reservations/my-reservation"
+    
+    lkp._get_reservation.assert_called_once_with("my-project", "us-central1-a", "my-reservation")
+
+
+def test_get_reservation_details_error(mocker):
+    lkp = util.Lookup(TstCfg())
+    
+    # Mock HttpError 500
+    mock_resp = Mock()
+    mock_resp.status = 500
+    error = HttpError(mock_resp, b'Internal Server Error')
+    
+    lkp._get_reservation = Mock(side_effect=error)
+    
+    with pytest.raises(HttpError):
+        lkp.get_reservation_details(
+            project="my-project",
+            zone="us-central1-a",
+            name="my-reservation",
+            bulk_insert_name="projects/my-project/reservations/my-reservation"
+        )
+    
+    lkp._get_reservation.assert_called_once_with("my-project", "us-central1-a", "my-reservation")

community/modules/scheduler/schedmd-slurm-gcp-v6-controller/modules/slurm_files/scripts/util.py

@@ -51,6 +51,7 @@
 import googleapiclient.discovery # type: ignore
 import google_auth_httplib2 # type: ignore
 from googleapiclient.http import set_user_agent # type: ignore
+from googleapiclient.errors import HttpError # type: ignore
 from google.api_core.client_options import ClientOptions
 import httplib2
 
@@ -1887,7 +1888,22 @@ def _get_future_reservation(self, project:str, zone:str, name: str) -> Any:
         return self.compute.futureReservations().get(project=project, zone=zone, futureReservation=name).execute()
 
     def get_reservation_details(self, project:str, zone:str, name:str, bulk_insert_name:str) -> ReservationDetails:
-        reservation = self._get_reservation(project, zone, name)
+        try:
+            reservation = self._get_reservation(project, zone, name)
+        except HttpError as e:
+            if e.resp.status == 403:
+                log.warning(f"Could not fetch reservation details for {project}/{zone}/{name}: {e}. Proceeding with minimal reservation settings.")
+                return ReservationDetails(
+                    project=project,
+                    zone=zone,
+                    name=name,
+                    policies=[],
+                    bulk_insert_name=bulk_insert_name,
+                    deployment_type=None,
+                    reservation_mode=None,
+                    assured_count=0,
+                    delete_at_time=None)
+            raise e
 
         # Converts policy URLs to names, e.g.:
         # projects/111111/regions/us-central1/resourcePolicies/zebra -> zebra