Support Azure managed identities - #736
Conversation
Prior to this change, the connection string would override the fall through to default Azure client authentication. We want to preserve the existing behavior while allowing the use of an Azure managed identity, so we add new environment variables to explicitly state we want to use a managed identity.
| const { account, spnAuth, isSpnAuth, useManagedIdentity } = options | ||
| const useManagedIdentityEnabled = useManagedIdentity === true || useManagedIdentity === 'true' | ||
|
|
||
| if (useManagedIdentityEnabled) { |
There was a problem hiding this comment.
I'm trying to understand whether this change is truly needed. The current behaviour seems to be that if one doesn't specify spnAuth nor connectionString config values, the control flow would reach the existing block that uses DefaultAzureCredential.
If my understanding is correct, there's no need to add another config value like useManagedIdentityEnabled. It's sufficient for deployments that wish to use managed identity to set values for the spnAuth and connectionString to none/empty strings.
Alternatively, if we want to explicitly opt into the managed identity approach as suggested here, maybe the existing 'default' block (using default credentials in storageQueueManager) should be removed?
There was a problem hiding this comment.
The problem is in cdConfig.js:
connectionString: config.get('CRAWLER_QUEUE_AZURE_CONNECTION_STRING') || cd_azblob.connection
We have to specify the connection string because it's a different account from cd_azblob, but if we specify the connection string it's going to use that for authentication. Adding another flag seemed like the cleanest way to maintain backwards compatibility with any existing uses of the crawler.
I think you're right that the existing 'default' block in storageQueueManager is not really reachable based on the values in cdConfig.js, but removing that default block would break compatibility if there's anyone calling that code independent of cdConfig.js. Seems very unlikely, but I'm wary of making a change like that to save a small number of easily understood lines of code.
There was a problem hiding this comment.
We have to specify the connection string because it's a different account from cd_azblob
I see, that makes sense.
I'm wary of making a change like that to save a small number of easily understood lines of code
The line savings are small indeed, but it would make the code easier to understand, removing this very doubt that you bring (that someone might actually use that somehow). Less cognitive load is always good. But I'm not 100% against it, and I appreciate the concern, so we can leave it there.
There was a problem hiding this comment.
I doubt anyone uses this code without cdConfig.js but I see why you have done what you have.
ljones140
left a comment
ljones140
left a comment
There was a problem hiding this comment.
I ran integration tests on this and they passed
https://github.com/clearlydefined/operations/actions/runs/23259034268
| const { account, spnAuth, isSpnAuth, useManagedIdentity } = options | ||
| const useManagedIdentityEnabled = useManagedIdentity === true || useManagedIdentity === 'true' | ||
|
|
||
| if (useManagedIdentityEnabled) { |
There was a problem hiding this comment.
I doubt anyone uses this code without cdConfig.js but I see why you have done what you have.
4 participants
Prior to this change, the connection string would override the fall through to default Azure client authentication. We want to preserve the existing behavior while allowing the use of an Azure managed identity, so we add new environment variables to explicitly state we want to use a managed identity.