-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basic implementation for managed identity for create function app #4213
base: main
Are you sure you want to change the base?
Conversation
@@ -56,7 +57,16 @@ export class FunctionAppCreateStep extends AzureWizardExecuteStep<IFunctionAppWi | |||
context.telemetry.properties.fileLoggingError = parseError(error).message; | |||
} | |||
} | |||
|
|||
const principalId = nonNullProp(nonNullProp(context.site, 'identity'), 'principalId'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit cleaner to do
const principalId = nonNullProp(nonNullProp(context.site, 'identity'), 'principalId'); | |
const principalId = nonNullValueAndProp(context.site.identity, 'principalId'); |
and I think it does the same thing.
package.json
Outdated
@@ -1211,6 +1211,7 @@ | |||
"dependencies": { | |||
"@azure/arm-appinsights": "^5.0.0-alpha.20230530.1", | |||
"@azure/arm-appservice": "^15.0.0", | |||
"@azure/arm-authorization-profile-2020-09-01-hybrid": "^2.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious: Were you forced to use 2020-09-01-hybrid
for some reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just following this documentation. Didn't even really think about how the @azure/arm-authorization-profile
package would exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://learn.microsoft.com/en-us/javascript/api/overview/azure/arm-authorization-readme?view=azure-node-latest
Yeah, not really sure what the difference is
name: `${ConnectionKey.Storage}__accountName`, | ||
value: context.newStorageAccountName ?? context.storageAccount?.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where a lot of our customer scenarios will break since we relied on that setting for a lot, so still plenty of work to do there.
Can you outline which scenarios you expect to break? Have you verified them breaking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Deployment: We rely/enforce having the
AzureWebJobsStorage
setting - Debugging: Same thing as above
Those are the main scenarios. We were discussing maybe having two different settings.json files-- one for remote and one for local settings. I'm not actually sure if the AzureWebJobsStorage__accountName
setting works locally since you would need an app that has an identity with RBAC to the storage account. That's kind of the main push as to why we'd want to have two separate settings.
Partially implements #4206
This is the bare bones implementation of having managed system identities integrated with function apps.
There's still quite a lot to do in terms of checking their settings for debugging/deploying.
This code is very simple because system identity is just a flag you turn on with the
identity
object and the backend completes the rest. The rest is using the role assignment API to give the function app theStorage Blob Data Contributor
role which is the bare minimum requirement for deploying (since it leverages storage blob containers for that).Lastly, we need to change the
AccountWebJobsStorage
toAccountWebJobsStorage__accountName
. This is where a lot of our customer scenarios will break since we relied on that setting for a lot, so still plenty of work to do there.