-
Notifications
You must be signed in to change notification settings - Fork 133
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?
Changes from 2 commits
d83d2ff
945dd5f
96c1e18
fa85b6c
07c8afb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -4,6 +4,7 @@ | |||||
*--------------------------------------------------------------------------------------------*/ | ||||||
|
||||||
import { type NameValuePair, type Site, type SiteConfig, type WebSiteManagementClient } from '@azure/arm-appservice'; | ||||||
import { AuthorizationManagementClient } from '@azure/arm-authorization-profile-2020-09-01-hybrid'; | ||||||
import { BlobServiceClient } from '@azure/storage-blob'; | ||||||
import { ParsedSite, WebsiteOS, type CustomLocation, type IAppServiceWizardContext } from '@microsoft/vscode-azext-azureappservice'; | ||||||
import { LocationListStep } from '@microsoft/vscode-azext-azureutils'; | ||||||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit cleaner to do
Suggested change
and I think it does the same thing. |
||||||
// this is the same apiVersion being used by the portal | ||||||
const apiVersion = '2020-06-01'; | ||||||
const amClient = new AuthorizationManagementClient(context.credentials, context.subscriptionId, { apiVersion }); | ||||||
|
||||||
const scope = nonNullProp(nonNullProp(context, 'storageAccount'), 'id'); | ||||||
const guid = crypto.randomUUID(); | ||||||
// this roleDefintionId cooresponds to the "Storage Blob Data Contributor" role | ||||||
const roleDefinitionId = '/providers/Microsoft.Authorization/roleDefinitions/ba92f5b4-2d11-453d-a403-e96b0029c9fe'; | ||||||
await amClient.roleAssignments.create(scope, guid, { properties: { roleDefinitionId, principalId } }); | ||||||
showSiteCreated(site, context); | ||||||
} | ||||||
|
||||||
|
@@ -73,7 +83,8 @@ export class FunctionAppCreateStep extends AzureWizardExecuteStep<IFunctionAppWi | |||||
serverFarmId: context.plan?.id, | ||||||
clientAffinityEnabled: false, | ||||||
siteConfig: await this.getNewSiteConfig(context, stack), | ||||||
reserved: context.newSiteOS === WebsiteOS.linux // The secret property - must be set to true to make it a Linux plan. Confirmed by the team who owns this API. | ||||||
reserved: context.newSiteOS === WebsiteOS.linux, // The secret property - must be set to true to make it a Linux plan. Confirmed by the team who owns this API. | ||||||
identity: { type: 'SystemAssigned' } | ||||||
}; | ||||||
|
||||||
if (context.customLocation) { | ||||||
|
@@ -142,8 +153,8 @@ export class FunctionAppCreateStep extends AzureWizardExecuteStep<IFunctionAppWi | |||||
const storageConnectionString: string = (await getStorageConnectionString(context)).connectionString; | ||||||
let appSettings: NameValuePair[] = [ | ||||||
{ | ||||||
name: ConnectionKey.Storage, | ||||||
value: storageConnectionString | ||||||
name: `${ConnectionKey.Storage}__accountName`, | ||||||
value: context.newStorageAccountName ?? context.storageAccount?.name | ||||||
Comment on lines
+156
to
+157
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe reason will be displayed to describe this comment to others. Learn more.
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 |
||||||
} | ||||||
]; | ||||||
|
||||||
|
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.https://learn.microsoft.com/en-us/javascript/api/overview/azure/arm-authorization-profile-2020-09-01-hybrid-readme?view=azure-node-latest
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