Problem with a Rest API call using Get-AzureRMResource cmdlet when dealing with '#' symbols

marckean commented 5 years ago

20190313 AzureRM, Az issue.docx

Description

Steps to reproduce

See attached doc

Module versions

ModuleType Version    Name                                ExportedCommands
---------- -------    ----                                ----------------
Script     6.12.0     AzureRM
Script     0.6.14     AzureRM.AnalysisServices            {Resume-AzureRmAnalysisServicesServer, Suspend-AzureRmAnalysisServicesServer, Get-AzureRmAnalysisServicesServer, Remove-AzureRmAnalysisSe...
Script     6.1.5      AzureRM.ApiManagement               {Add-AzureRmApiManagementRegion, Get-AzureRmApiManagementSsoToken, New-AzureRmApiManagementHostnameConfiguration, New-AzureRmApiManagemen...
Script     0.1.8      AzureRM.ApplicationInsights         {Get-AzureRmApplicationInsights, New-AzureRmApplicationInsights, Remove-AzureRmApplicationInsights, Set-AzureRmApplicationInsightsPricing...
Script     5.1.2      AzureRM.Automation                  {Get-AzureRMAutomationHybridWorkerGroup, Get-AzureRmAutomationJobOutputRecord, Import-AzureRmAutomationDscNodeConfiguration, Export-Azure...
Script     4.0.11     AzureRM.Backup                      {Backup-AzureRmBackupItem, Enable-AzureRmBackupContainerReregistration, Get-AzureRmBackupContainer, Register-AzureRmBackupContainer...}
Script     4.1.5      AzureRM.Batch                       {Remove-AzureRmBatchAccount, Get-AzureRmBatchAccount, Get-AzureRmBatchAccountKeys, New-AzureRmBatchAccount...}
Script     0.14.6     AzureRM.Billing                     {Get-AzureRmBillingInvoice, Get-AzureRmBillingPeriod, Get-AzureRmEnrollmentAccount}
Script     5.0.6      AzureRM.Cdn                         {Get-AzureRmCdnProfile, Get-AzureRmCdnProfileSsoUrl, New-AzureRmCdnProfile, Remove-AzureRmCdnProfile...}
Script     0.9.12     AzureRM.CognitiveServices           {Get-AzureRmCognitiveServicesAccount, Get-AzureRmCognitiveServicesAccountKey, Get-AzureRmCognitiveServicesAccountSkus, Get-AzureRmCogniti...
Script     5.8.0      AzureRM.Compute                     {Remove-AzureRmAvailabilitySet, Get-AzureRmAvailabilitySet, New-AzureRmAvailabilitySet, Update-AzureRmAvailabilitySet...}
Script     0.3.7      AzureRM.Consumption                 {Get-AzureRmConsumptionBudget, Get-AzureRmConsumptionMarketplace, Get-AzureRmConsumptionPriceSheet, Get-AzureRmConsumptionReservationDeta...
Script     0.2.10     AzureRM.ContainerInstance           {New-AzureRmContainerGroup, Get-AzureRmContainerGroup, Remove-AzureRmContainerGroup, Get-AzureRmContainerInstanceLog}
Script     1.0.10     AzureRM.ContainerRegistry           {New-AzureRmContainerRegistry, Get-AzureRmContainerRegistry, Update-AzureRmContainerRegistry, Remove-AzureRmContainerRegistry...}
Script     5.0.3      AzureRM.DataFactories               {Remove-AzureRmDataFactory, Get-AzureRmDataFactoryRun, Get-AzureRmDataFactorySlice, Save-AzureRmDataFactoryLog...}
Script     0.5.11     AzureRM.DataFactoryV2               {Set-AzureRmDataFactoryV2, Update-AzureRmDataFactoryV2, Get-AzureRmDataFactoryV2, Remove-AzureRmDataFactoryV2...}
Script     5.1.4      AzureRM.DataLakeAnalytics           {Get-AzureRmDataLakeAnalyticsDataSource, New-AzureRmDataLakeAnalyticsCatalogCredential, Remove-AzureRmDataLakeAnalyticsCatalogCredential,...
Script     6.2.1      AzureRM.DataLakeStore               {Get-AzureRmDataLakeStoreTrustedIdProvider, Remove-AzureRmDataLakeStoreTrustedIdProvider, Remove-AzureRmDataLakeStoreFirewallRule, Set-Az...
Script     4.0.9      AzureRM.DevTestLabs                 {Get-AzureRmDtlAllowedVMSizesPolicy, Get-AzureRmDtlAutoShutdownPolicy, Get-AzureRmDtlAutoStartPolicy, Get-AzureRmDtlVMsPerLabPolicy...}
Script     5.1.0      AzureRM.Dns                         {Get-AzureRmDnsRecordSet, New-AzureRmDnsRecordConfig, Remove-AzureRmDnsRecordSet, Set-AzureRmDnsRecordSet...}
Script     0.3.7      AzureRM.EventGrid                   {New-AzureRmEventGridTopic, Get-AzureRmEventGridTopic, Set-AzureRmEventGridTopic, New-AzureRmEventGridTopicKey...}
Script     0.7.0      AzureRM.EventHub                    {New-AzureRmEventHubNamespace, Get-AzureRmEventHubNamespace, Set-AzureRmEventHubNamespace, Remove-AzureRmEventHubNamespace...}
Script     4.1.8      AzureRM.HDInsight                   {Get-AzureRmHDInsightJob, New-AzureRmHDInsightSqoopJobDefinition, Wait-AzureRmHDInsightJob, New-AzureRmHDInsightStreamingMapReduceJobDefi...
Script     5.1.5      AzureRM.Insights                    {Get-AzureRmMetricDefinition, Get-AzureRmMetric, Remove-AzureRmLogProfile, Get-AzureRmLogProfile...}
Script     3.1.8      AzureRM.IotHub                      {Add-AzureRmIotHubKey, Get-AzureRmIotHubEventHubConsumerGroup, Get-AzureRmIotHubConnectionString, Get-AzureRmIotHubJob...}
Script     5.2.1      AzureRM.KeyVault                    {Add-AzureKeyVaultCertificate, Update-AzureKeyVaultCertificate, Stop-AzureKeyVaultCertificateOperation, Get-AzureKeyVaultCertificateOpera...
Script     4.1.4      AzureRM.LogicApp                    {Get-AzureRmIntegrationAccountAgreement, Get-AzureRmIntegrationAccountCallbackUrl, Get-AzureRmIntegrationAccountCertificate, Get-AzureRmI...
Script     0.18.5     AzureRM.MachineLearning             {Move-AzureRmMlCommitmentAssociation, Get-AzureRmMlCommitmentAssociation, Get-AzureRmMlCommitmentPlanUsageHistory, Remove-AzureRmMlCommit...
Script     0.4.8      AzureRM.MachineLearningCompute      {Get-AzureRmMlOpCluster, Get-AzureRmMlOpClusterKey, Test-AzureRmMlOpClusterSystemServicesUpdateAvailability, Update-AzureRmMlOpClusterSys...
Script     0.2.5      AzureRM.MarketplaceOrdering         {Get-AzureRmMarketplaceTerms, Set-AzureRmMarketplaceTerms}
Script     0.10.4     AzureRM.Media                       {Sync-AzureRmMediaServiceStorageKeys, Set-AzureRmMediaServiceKey, Get-AzureRmMediaServiceKeys, Get-AzureRmMediaServiceNameAvailability...}
Script     6.10.0     AzureRM.Network                     {Add-AzureRmApplicationGatewayAuthenticationCertificate, Get-AzureRmApplicationGatewayAuthenticationCertificate, New-AzureRmApplicationGa...
Script     5.0.3      AzureRM.NotificationHubs            {Get-AzureRmNotificationHub, Get-AzureRmNotificationHubAuthorizationRules, Get-AzureRmNotificationHubListKeys, Get-AzureRmNotificationHub...
Script     5.0.6      AzureRM.OperationalInsights         {New-AzureRmOperationalInsightsAzureActivityLogDataSource, New-AzureRmOperationalInsightsCustomLogDataSource, Disable-AzureRmOperationalI...
Script     1.1.0      AzureRM.PolicyInsights              {Get-AzureRmPolicyEvent, Get-AzureRmPolicyState, Get-AzureRmPolicyStateSummary, Get-AzureRmPolicyRemediation...}
Script     4.1.10     AzureRM.PowerBIEmbedded             {Remove-AzureRmPowerBIWorkspaceCollection, Get-AzureRmPowerBIWorkspaceCollection, Get-AzureRmPowerBIWorkspaceCollectionAccessKeys, Get-Az...
Script     5.8.0      AzureRM.Profile                     {Disable-AzureRmDataCollection, Disable-AzureRmContextAutosave, Enable-AzureRmDataCollection, Enable-AzureRmContextAutosave...}
Script     4.1.8      AzureRM.RecoveryServices            {Get-AzureRmRecoveryServicesBackupProperty, Get-AzureRmRecoveryServicesVault, Get-AzureRmRecoveryServicesVaultSettingsFile, New-AzureRmRe...
Script     4.5.0      AzureRM.RecoveryServices.Backup     {Backup-AzureRmRecoveryServicesBackupItem, Get-AzureRmRecoveryServicesBackupManagementServer, Get-AzureRmRecoveryServicesBackupContainer,...
Script     0.2.10     AzureRM.RecoveryServices.SiteRec... {Edit-AzureRmRecoveryServicesAsrRecoveryPlan, Get-AzureRmRecoveryServicesAsrAlertSetting, Get-AzureRmRecoveryServicesAsrEvent, Get-AzureR...
Script     5.1.0      AzureRM.RedisCache                  {Remove-AzureRmRedisCachePatchSchedule, New-AzureRmRedisCacheScheduleEntry, Get-AzureRmRedisCachePatchSchedule, New-AzureRmRedisCachePatc...
Script     0.3.10     AzureRM.Relay                       {New-AzureRmRelayNamespace, Get-AzureRmRelayNamespace, Set-AzureRmRelayNamespace, Remove-AzureRmRelayNamespace...}
Script     6.7.1      AzureRM.Resources                   {Get-AzureRmProviderOperation, Remove-AzureRmRoleAssignment, Get-AzureRmRoleAssignment, New-AzureRmRoleAssignment...}
Script     0.16.10    AzureRM.Scheduler                   {Disable-AzureRmSchedulerJobCollection, Enable-AzureRmSchedulerJobCollection, Get-AzureRmSchedulerJobCollection, Get-AzureRmSchedulerJob...}
Script     0.6.13     AzureRM.ServiceBus                  {New-AzureRmServiceBusNamespace, Get-AzureRmServiceBusNamespace, Set-AzureRmServiceBusNamespace, Remove-AzureRmServiceBusNamespace...}
Script     0.3.13     AzureRM.ServiceFabric               {Add-AzureRmServiceFabricApplicationCertificate, Add-AzureRmServiceFabricClientCertificate, Add-AzureRmServiceFabricClusterCertificate, A...
Script     1.0.0      AzureRM.SignalR                     {New-AzureRmSignalR, Get-AzureRmSignalR, Get-AzureRmSignalRKey, New-AzureRmSignalRKey...}
Script     4.11.5     AzureRM.Sql                         {Get-AzureRmSqlDatabaseTransparentDataEncryption, Get-AzureRmSqlDatabaseTransparentDataEncryptionActivity, Set-AzureRmSqlDatabaseTranspar...
Script     5.2.0      AzureRM.Storage                     {Get-AzureRmStorageAccount, Get-AzureRmStorageAccountKey, New-AzureRmStorageAccount, New-AzureRmStorageAccountKey...}
Script     4.0.10     AzureRM.StreamAnalytics             {Get-AzureRmStreamAnalyticsFunction, Get-AzureRmStreamAnalyticsDefaultFunctionDefinition, New-AzureRmStreamAnalyticsFunction, Remove-Azur...
Script     4.0.5      AzureRM.Tags                        {Remove-AzureRmTag, Get-AzureRmTag, New-AzureRmTag}
Script     4.1.1      AzureRM.TrafficManager              {Add-AzureRmTrafficManagerCustomHeaderToEndpoint, Remove-AzureRmTrafficManagerCustomHeaderFromEndpoint, Add-AzureRmTrafficManagerCustomHe...
Script     4.0.5      AzureRM.UsageAggregates             Get-UsageAggregates
Script     5.2.0      AzureRM.Websites                    {Get-AzureRmAppServicePlan, Set-AzureRmAppServicePlan, New-AzureRmAppServicePlan, Remove-AzureRmAppServicePlan...}

Debug output

See attached doc

Error output

See attached doc
[20190313 AzureRM, Az issue.docx](https://github.com/Azure/azure-powershell/files/2959242/20190313.AzureRM.Az.issue.docx)

vladimir-shcherbakov commented 5 years ago

@ravbhatnagar Could you please take a look?

marckean commented 5 years ago

Any update on this yet?

ScottHolden commented 5 years ago

Issue seems to be in the underlying .NET SDK, but does not originate there: https://github.com/Azure/azure-libraries-for-net/blob/master/src/ResourceManagement/ResourceManager/Generated/ResourcesOperations.cs#L1398 ResoucreID is simply replaced into the URL with no data escaping.

This is in turn used by the powershell commandlets via 'ResourceManagementSDKClient.GetById': https://github.com/Azure/azure-powershell/blob/master/src/Resources/ResourceManager/SdkClient/ResourceManagerSdkClient.cs#L1138

It seems that AutoRest obeys the 'x-ms-skip-url-encoding' property within the rest api spec, and in this case, resourceID is set to ignore url encoding: https://github.com/Azure/azure-rest-api-specs/blob/master/specification/resources/resource-manager/Microsoft.Resources/stable/2019-03-01/resources.json#L1805

Which is generated via: https://github.com/Azure/autorest.csharp/blame/master/src/azure/Model/MethodCsa.cs#L321

The issue at hand is a little bit deeper than just enabling url encoding for the resourceID property, as System.Uri.EscapeDataString will also escape slash characters, breaking the format of the resourceID.

Eg: System.Uri.EscapeDataString("Abc/dev#123") is Abc%2Fdev%23123

There nearly needs to be an 'escape all but slashes' call to handle non-alphanumeric resource naming while preserving structure:

private static string EscapeResourceIdString(string resourceId) 
{
    string[] parts = resourceId.Split('/');
    for(int i=0; i<parts.Length; i++)
        parts[i] = Uri.EscapeDataString(parts[i]);
    return string.Join("/", parts);
}

But this would have to happen at an AutoRest level, and possibly a swagger change to support it. It would also be interesting to see if other AutoRest generated clients show the same behaviour. I've checked the nodejs client and it also url encodes '/' characters.

markcowl commented 5 years ago

It is generally the case that Urlencoding is destructive when performed over an entire url ('#' could also technically occur as part of the Url, specifying a document fragment, which makes this a really odd pattern for a service to use when automatically naming resources), and also non-idempotent.

The correct thing for SDKs to do may be to expect the caller to url encode, but the correct thing for callers (including PowerShell) is definitely to ensure the input string is url encoded.

The swagger for this is breaking a bunch of rules for good swagger in the cause of generation rather than description of the api (you generally cannot have a path parameter that contains path segments, for example).

markcowl commented 5 years ago

Description

Generic Resource cmdlets are not correctly url encoding passed-in resopurce ids

[ ] Create generic common code that will correctly check input ResourceId for correctness, and encode them piecemeal
[ ] Apply to generic resource manager cmdlets (Get, Set, Remove, etc.)
[ ] Add tests usiing
- [ ] Pre-encoded Resource Ids
- [ ] Unencoded Resource Ids with characters that need to be escaped

Azure / azure-powershell