Network troubleshooter - support for managed identity connections and key vault references #38
Conversation
…port setting a summary and details.
…I found while reviewing the messages.
…s and moed two files out of Exceptions folder and namespace
… logic simple and compact
…es in error messages.
|
|
|
These changes were made in a forked branch and validated for both new scenarios and regression. I have reviewed the changes in the forked branch so requesting that Jeff and Puneet review this final merge to the main repo. |
sidkri
changed the title
DiagnosticsExtension/Controllers/ConnectionStringValidationController.cs
Show resolved
Hide resolved
| var result = await Validate(requestBody.ConnectionString, requestBody.Type); | ||
| return result; | ||
| } | ||
|
|
||
| [HttpGet] | ||
| [Route("validateappsetting")] | ||
| public async Task<HttpResponseMessage> ValidateAppSetting(string appSettingName, string type) | ||
| public async Task<HttpResponseMessage> ValidateAppSetting(string appSettingName, string type, string entityName = null) |
There was a problem hiding this comment.
This function is very specialized for function apps, because if I want to validate connection strings stored in AppSettings for WebApp, I won't be able to use this function (otherwise it will treat a non-existed appsetting as MSI enabled, and the managed identity naming convention only exists in functions). Maybe we can change the function name to ValidateAppSettingForFunctionApp or something like that?
There was a problem hiding this comment.
Can we add some comment to the function and briefly describe what is the entityName in the parameter
There was a problem hiding this comment.
I assume you would also want the route to change from "validateappsetting" to something more functions specific?
| { | ||
| public static class ManagedIdentityCredentialTokenValidator | ||
| { | ||
| public static ManagedIdentityCredential GetValidatedCredential(string clientId, string appSettingName) |
There was a problem hiding this comment.
I wonder if appSettingName parameter is needed here. Can we catch the exception at the callsite of this function and add appSettingName into error message there? So that this class can be a more general purpose one and no need to stick to appSettingName
There was a problem hiding this comment.
This function specifically deals with clientid appsetting set in Azure functions only.
ClientIdInvalidTokenGeneratedSummary - This variable has documentation link related to Azure functions only.
Can we create a new method, which can be more generic?
There was a problem hiding this comment.
I don't think clientId is something specific to Function App, so I believe it can be used for webapp as well without any modification. But I just noticed that the scope was set to https://storage.azure.com/.default, so is this function exclusively for storage account? If so, can you update the class name to reflect this?
| public ResultStatus? Status; | ||
| [Newtonsoft.Json.JsonIgnore] | ||
| public string IdentityType; | ||
| [JsonProperty("Summary")] |
There was a problem hiding this comment.
Can we simply change these properties name instead of overriding them?
…ajibillik/DaaS into sidkri/527_Networkvalidator_V3
…' into sidkri/527_Networkvalidator_V3
This PR adds Functions specific support for end-to-end connectivity testing of triggers/bindings configured for access managed identity v/s connection strings (Blob, Queue, Service Bus and Event Hubs are currently supported).
It also supports validation of key vault references.
Key callouts:
BlobStorageValidator, QueueStorageValidator, FileShareStorageValidator.ConnectionStringResponseUtilityandManagedIdentityConnectionResponseUtility.Constants