Update patch set 1

Patch Set 1:

(1 comment)

Patch-set: 1
Attention: {"person_ident":"Gerrit User 1 \u003c1@4a232e18-c5a9-48ee-94c0-e04e7cca6543\u003e","operation":"ADD","reason":"\u003cGERRIT_ACCOUNT_36867\u003e replied on the change"}
Attention: {"person_ident":"Gerrit User 36867 \u003c36867@4a232e18-c5a9-48ee-94c0-e04e7cca6543\u003e","operation":"REMOVE","reason":"\u003cGERRIT_ACCOUNT_36867\u003e replied on the change"}

0db05eba83b08633ea04cbd6ec9598d8cf73ad05

@@ -16,6 +16,24 @@
       "message": "It looks like we really need to get the managed identity by id.  So rather than fetch the list of identities, why don\u0027t we use the pattern we use with images.  So instead of implementing _listManagedIdentities, implement _getManagedIdentity like _getImage.  In a system with, say, 10 identities used by nodepool, it will be a little slower at first because we\u0027ll make 10 api calls.  But there should be no reason to expire them from the cache since the only thing we\u0027re using is the id, and that isn\u0027t going to change.  So once we perform a get for each one, we never have to do another request.  And in a system with 1000 managed identities, that will be much faster.\n\nOR (and this might be even better) you could do no validation of the managed identities at all.  For example, we ask users to provide a subnet id, and we don\u0027t check whether it exists or not, because azure will tell us that.  The same is true here too -- in line 671 we\u0027ll raise an exception if we can\u0027t find a managed ID.  But if we removed all this checking, then Azure will throw an error just a little later in line 684.\n\nAs long as we don\u0027t need any more information than the ID, I would recommend the second approach; it\u0027s much simpler.\n\nRegardless of which approach you take, we should update at least one of the tests to create a node with a user-assigned-identity so that we exercise the new code.",
       "revId": "0db05eba83b08633ea04cbd6ec9598d8cf73ad05",
       "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
+    },
+    {
+      "unresolved": true,
+      "key": {
+        "uuid": "eba8e41a_a334d4c8",
+        "filename": "nodepool/driver/azure/adapter.py",
+        "patchSetId": 1
+      },
+      "lineNbr": 665,
+      "author": {
+        "id": 36867
+      },
+      "writtenOn": "2024-03-08T15:51:07Z",
+      "side": 1,
+      "message": "Thanks for your quick comments James!\n\nI actually started off with your second (simpler) approach but quickly realised that the Compute API forces us to specify the User-assigned Managed Identity as ARM Resource ID: https://learn.microsoft.com/en-us/rest/api/compute/virtual-machines/create-or-update?view\u003drest-compute-2023-10-02\u0026tabs\u003dHTTP#virtualmachineidentity\n\nSo a fundamental limitation is that we need to build the ARM Resource ID of the identities we want to use. The ARM Resource Id of a User-assigned Managed Identity is composed of:\n\n* Subscription Id\n* Resource Group Id\n* User-assigned Managed Identity Name\n\nIt is pretty safe to assume that the subscription will be the same, but what about the Resource Group ... can we assume that all managed identities will be found in providers.[azure].resource-group or should we give the possibility to specify different resource groups as well? Something like this:\n\n```\n            user-assigned-identities:\n              - name: userManagedIdentity1\n                resource-group: rg-non-default1\n              - name: userManagedIdentity2\n```\n\n(Assume providers.[azure].resource-group if resource-group is omitted)",
+      "parentUuid": "2a0643b0_556ecd9c",
+      "revId": "0db05eba83b08633ea04cbd6ec9598d8cf73ad05",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
     }
   ]
 }