-
Notifications
You must be signed in to change notification settings - Fork 60
leverage ServiceConnector to setup connection to keyvault and DB #1
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,13 @@ param appSettings object = {} | |
| param keyVaultName string | ||
| param serviceName string = 'api' | ||
|
|
||
| // Target DB properties | ||
| param connectionStringKey string = '' | ||
| param targetResourceId string = '' | ||
| param appUser string = '' | ||
| @secure() | ||
| param appUserPassword string | ||
|
|
||
| module api '../core/host/appservice.bicep' = { | ||
| name: '${name}-app-module' | ||
| params: { | ||
|
|
@@ -25,6 +32,10 @@ module api '../core/host/appservice.bicep' = { | |
| runtimeName: 'dotnetcore' | ||
| runtimeVersion: '6.0' | ||
| scmDoBuildDuringDeployment: false | ||
| targetResourceId: targetResourceId | ||
| appUser: appUser | ||
| appUserPassword: appUserPassword | ||
|
Comment on lines
+36
to
+37
Member
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. Why does appservice need to know about appUser? We don't want to couple the two at the appservice level.
Author
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. appservice doesn't know appUser, it's a param passed in from main, user knows their app, and their db's username/password. |
||
| connectionStringKey: connectionStringKey | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,13 @@ param numberOfWorkers int = -1 | |
| param scmDoBuildDuringDeployment bool = false | ||
| param use32BitWorkerProcess bool = false | ||
|
|
||
| // Target DB properties | ||
| param connectionStringKey string = 'AZURE-SQL-CONNECTION-STRING' | ||
|
Member
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. We wouldn't update /core/host/appservice to use SQL because the app service itself can be used with any DB and it shouldn't default to anything.
Author
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. update default to empty |
||
| param targetResourceId string = '' | ||
| param appUser string = '' | ||
| @secure() | ||
| param appUserPassword string = '' | ||
|
|
||
|
Comment on lines
+39
to
+42
Member
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. Why do we need appUser for app service?
Author
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. when setting up connection between the app service and target db, user need to provide target db's connection info including appUser, appPassword. |
||
| resource appService 'Microsoft.Web/sites@2022-03-01' = { | ||
| name: name | ||
| location: location | ||
|
|
@@ -66,8 +73,7 @@ resource appService 'Microsoft.Web/sites@2022-03-01' = { | |
| SCM_DO_BUILD_DURING_DEPLOYMENT: string(scmDoBuildDuringDeployment) | ||
| ENABLE_ORYX_BUILD: string(enableOryxBuild) | ||
| }, | ||
| !empty(applicationInsightsName) ? { APPLICATIONINSIGHTS_CONNECTION_STRING: applicationInsights.properties.ConnectionString } : {}, | ||
| !empty(keyVaultName) ? { AZURE_KEY_VAULT_ENDPOINT: keyVault.properties.vaultUri } : {}) | ||
| !empty(applicationInsightsName) ? { APPLICATIONINSIGHTS_CONNECTION_STRING: applicationInsights.properties.ConnectionString } : {}) | ||
| } | ||
|
|
||
| resource configLogs 'config' = { | ||
|
|
@@ -92,6 +98,61 @@ resource applicationInsights 'Microsoft.Insights/components@2020-02-02' existing | |
| name: applicationInsightsName | ||
| } | ||
|
|
||
| // if !empty(keyVaultName), create connection to keyvault so that db credentials could be saved into keyvault, | ||
| // and app service could retrieve secrets from keyvault using managed identity | ||
| resource connectionToKeyVault 'Microsoft.ServiceLinker/linkers@2022-11-01-preview' = if(!empty(keyVaultName)) { | ||
| name: 'conn_kv' | ||
| scope: appService | ||
| properties: { | ||
| targetService: { | ||
| id: keyVault.id | ||
| type: 'AzureResource' | ||
| } | ||
| clientType: 'none' | ||
| authInfo: { | ||
| authType: 'systemAssignedIdentity' | ||
| roles: [ | ||
| '4633458b-17de-408a-b874-0445c86b69e6' | ||
| ] | ||
| } | ||
| configurationInfo: { | ||
| customizedKeys: { | ||
| 'AZURE_KEYVAULT_RESOURCEENDPOINT': 'AZURE_KEY_VAULT_ENDPOINT' | ||
| } | ||
| } | ||
|
Comment on lines
+118
to
+122
Member
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. How set are you on using that key? Ideally azd and service connector use the same so we don't have to overide by default.
Author
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. azd's default is |
||
| } | ||
| } | ||
|
|
||
| // if !empty(targetResourceId), create connection to target database, including: | ||
| // - add db connectionstr (from keyvault if applicable) in webapp appsettings or connectionString(for dotnetcore convention) | ||
| // - allow webapp firewall at target database if applicable (target allows firewall instead of public access) | ||
| resource connectionToTargetDB 'Microsoft.ServiceLinker/linkers@2022-11-01-preview' = if (!empty(targetResourceId)) { | ||
| name: 'conn_db' | ||
| scope: appService | ||
| properties: { | ||
| targetService: { | ||
| id: targetResourceId | ||
| type: 'AzureResource' | ||
| } | ||
| secretStore: { | ||
| keyVaultId: !empty(keyVaultName) ? keyVault.id : '' | ||
| keyVaultSecretName: !empty(keyVaultName) ? connectionStringKey : '' | ||
| } | ||
|
Comment on lines
+137
to
+140
Member
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. Do we want to not created the resource if the keyVult isn't passed in?
Author
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. no, no need to create keyvault conn is keyvault is empty |
||
| authInfo: { | ||
| authType: 'secret' | ||
| name: appUser | ||
| secretInfo: { | ||
| secretType: 'rawValue' | ||
| value: appUserPassword | ||
| } | ||
| } | ||
| clientType: 'dotnet' | ||
| } | ||
| dependsOn: [ | ||
| connectionToKeyVault | ||
| ] | ||
| } | ||
|
|
||
|
Comment on lines
+129
to
+155
Member
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. This (being in appservice) means that app service can only be linked to one db. I think we need to consider a scenario where all of this is abstracted and included in main.bicep as a module with very few inputs. |
||
| output identityPrincipalId string = managedIdentity ? appService.identity.principalId : '' | ||
| output name string = appService.name | ||
| output uri string = 'https://${appService.properties.defaultHostName}' | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ param sqlServerName string = '' | |
| param sqlDatabaseName string = '' | ||
| param webServiceName string = '' | ||
| param apimServiceName string = '' | ||
| param appUser string = 'appUser' | ||
|
|
||
| @description('Flag to use Azure API Management to mediate the calls between the Web frontend and the backend API') | ||
| param useAPIM bool = false | ||
|
|
@@ -78,16 +79,10 @@ module api './app/api.bicep' = { | |
| appSettings: { | ||
| AZURE_SQL_CONNECTION_STRING_KEY: sqlServer.outputs.connectionStringKey | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Give the API access to KeyVault | ||
| module apiKeyVaultAccess './core/security/keyvault-access.bicep' = { | ||
| name: 'api-keyvault-access' | ||
| scope: rg | ||
| params: { | ||
| keyVaultName: keyVault.outputs.name | ||
| principalId: api.outputs.SERVICE_API_IDENTITY_PRINCIPAL_ID | ||
| targetResourceId: '${sqlServer.outputs.id}/databases/${sqlServer.outputs.databaseName}' | ||
|
Member
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. I'd add this string as an output of db.bicep |
||
| appUser: appUser | ||
| appUserPassword: appUserPassword | ||
| connectionStringKey: sqlServer.outputs.connectionStringKey | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
To me, targetResourceId name isn't enough to know what it is or used for.