From ad8f248375d8e7c06d23b9a132a976c741f826d5 Mon Sep 17 00:00:00 2001 From: Stuart Konen Date: Tue, 10 Nov 2020 22:25:22 -0800 Subject: [PATCH 1/4] Template Specs: Fixing issue where template specs from subscriptions not in context could not be deployed --- .../ResourceWithParameterCmdletBase.cs | 51 +++++++++---------- src/Resources/Resources/ChangeLog.md | 1 + 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/Resources/ResourceManager/Implementation/CmdletBase/ResourceWithParameterCmdletBase.cs b/src/Resources/ResourceManager/Implementation/CmdletBase/ResourceWithParameterCmdletBase.cs index 53c8e5dbf77e..522aa8a1b8a2 100644 --- a/src/Resources/ResourceManager/Implementation/CmdletBase/ResourceWithParameterCmdletBase.cs +++ b/src/Resources/ResourceManager/Implementation/CmdletBase/ResourceWithParameterCmdletBase.cs @@ -17,11 +17,11 @@ using System.Collections.Generic; using System.Linq; using System.Management.Automation; +using Microsoft.Azure.Commands.Common.Authentication; using Microsoft.Azure.Commands.Common.Authentication.Abstractions; using Microsoft.Azure.Commands.ResourceManager.Cmdlets.Components; -using Microsoft.Azure.Commands.ResourceManager.Cmdlets.SdkClient; -using Microsoft.Azure.Commands.ResourceManager.Cmdlets.SdkModels; using Microsoft.Azure.Commands.ResourceManager.Cmdlets.Utilities; +using Microsoft.Azure.Management.ResourceManager; using Microsoft.WindowsAzure.Commands.Utilities.Common; using Newtonsoft.Json.Linq; @@ -142,26 +142,6 @@ protected ResourceWithParameterCmdletBase() "to provide a better error message in the case where not all required parameters are satisfied.")] public SwitchParameter SkipTemplateParameterPrompt { get; set; } - private TemplateSpecsSdkClient templateSpecsSdkClient; - - /// - /// Gets or sets the Template Specs Azure sdk client wrapper - /// - public TemplateSpecsSdkClient TemplateSpecsSdkClient - { - get - { - if (this.templateSpecsSdkClient == null) - { - this.templateSpecsSdkClient = new TemplateSpecsSdkClient(DefaultContext); - } - - return this.templateSpecsSdkClient; - } - - set { this.templateSpecsSdkClient = value; } - } - public object GetDynamicParameters() { if (!this.IsParameterBound(c => c.SkipTemplateParameterPrompt)) @@ -238,12 +218,31 @@ public object GetDynamicParameters() throw new PSArgumentException("No version found in Resource ID"); } - var templateSpecVersion = TemplateSpecsSdkClient.GetTemplateSpec( - ResourceIdUtility.GetResourceName(templateSpecId).Split('/')[0], + ITemplateSpecsClient templateSpecsClient = + AzureSession.Instance.ClientFactory.CreateArmClient( + DefaultContext, + AzureEnvironment.Endpoint.ResourceManager + ); + + if (!string.IsNullOrEmpty(resourceIdentifier.Subscription) && + templateSpecsClient.SubscriptionId != resourceIdentifier.Subscription) + { + // The template spec is in a different subscription than our default + // context. Force the client to use that subscription: + templateSpecsClient.SubscriptionId = resourceIdentifier.Subscription; + } + + var templateSpecVersion = templateSpecsClient.TemplateSpecVersions.Get( ResourceIdUtility.GetResourceGroupName(templateSpecId), - resourceIdentifier.ResourceName).Versions.Single(); + ResourceIdUtility.GetResourceName(templateSpecId).Split('/')[0], + resourceIdentifier.ResourceName); + + if (!(templateSpecVersion.Template is JObject)) + { + throw new InvalidOperationException("Unexpected type."); // Sanity check + } - var templateObj = JObject.Parse(templateSpecVersion.Template); + JObject templateObj = (JObject)templateSpecVersion.Template; if (string.IsNullOrEmpty(TemplateParameterUri)) { diff --git a/src/Resources/Resources/ChangeLog.md b/src/Resources/Resources/ChangeLog.md index c3fd3dbe8414..67753356fe4d 100644 --- a/src/Resources/Resources/ChangeLog.md +++ b/src/Resources/Resources/ChangeLog.md @@ -21,6 +21,7 @@ * Fixed an issue where What-If shows two resource group scopes with different casing * Updated `Export-AzResourceGroup` to use the SDK. * Added culture info to parse methods +* Fixed issue where attempts to deploy template specs from a subscription outside of the current subscription context would fail ## Version 3.0.0 * Fixed parsing bug From 2a4c3fac78e656644e5a062492a1c5152b3e7e7f Mon Sep 17 00:00:00 2001 From: Stuart Konen Date: Mon, 23 Nov 2020 00:23:44 -0800 Subject: [PATCH 2/4] Adding mock test for covering recent cross-sub template spec deployment bug involving dynamic parameters. --- .../ResourceWithParameterCmdletBase.cs | 89 +++++++++++++++---- ...zureResourceGroupDeploymentCommandTests.cs | 82 ++++++++++++++++- 2 files changed, 153 insertions(+), 18 deletions(-) diff --git a/src/Resources/ResourceManager/Implementation/CmdletBase/ResourceWithParameterCmdletBase.cs b/src/Resources/ResourceManager/Implementation/CmdletBase/ResourceWithParameterCmdletBase.cs index 522aa8a1b8a2..b703d8655a9e 100644 --- a/src/Resources/ResourceManager/Implementation/CmdletBase/ResourceWithParameterCmdletBase.cs +++ b/src/Resources/ResourceManager/Implementation/CmdletBase/ResourceWithParameterCmdletBase.cs @@ -59,6 +59,8 @@ public abstract class ResourceWithParameterCmdletBase : ResourceManagerCmdletBas private string templateSpecId; + private ITemplateSpecsClient templateSpecsClient; + protected ResourceWithParameterCmdletBase() { dynamicParameters = new RuntimeDefinedParameterDictionary(); @@ -142,10 +144,35 @@ protected ResourceWithParameterCmdletBase() "to provide a better error message in the case where not all required parameters are satisfied.")] public SwitchParameter SkipTemplateParameterPrompt { get; set; } + /// + /// Gets or sets the Template Specs Azure SDK client + /// + public ITemplateSpecsClient TemplateSpecsClient + { + get + { + if (this.templateSpecsClient == null) + { + this.templateSpecsClient = + AzureSession.Instance.ClientFactory.CreateArmClient( + DefaultContext, + AzureEnvironment.Endpoint.ResourceManager + ); + } + + return this.templateSpecsClient; + } + + set { this.templateSpecsClient = value; } + } + public object GetDynamicParameters() { if (!this.IsParameterBound(c => c.SkipTemplateParameterPrompt)) { + // Resolve the static parameter names for this cmdlet: + string[] staticParameterNames = this.GetStaticParameterNames(); + if (TemplateObject != null && TemplateObject != templateObject) { templateObject = TemplateObject; @@ -155,7 +182,7 @@ public object GetDynamicParameters() TemplateObject, TemplateParameterObject, this.ResolvePath(TemplateParameterFile), - MyInvocation.MyCommand.Parameters.Keys.ToArray()); + staticParameterNames); } else { @@ -163,7 +190,7 @@ public object GetDynamicParameters() TemplateObject, TemplateParameterObject, TemplateParameterUri, - MyInvocation.MyCommand.Parameters.Keys.ToArray()); + staticParameterNames); } } else if (!string.IsNullOrEmpty(TemplateFile) && @@ -176,7 +203,7 @@ public object GetDynamicParameters() this.ResolvePath(TemplateFile), TemplateParameterObject, this.ResolvePath(TemplateParameterFile), - MyInvocation.MyCommand.Parameters.Keys.ToArray()); + staticParameterNames); } else { @@ -184,7 +211,7 @@ public object GetDynamicParameters() this.ResolvePath(TemplateFile), TemplateParameterObject, TemplateParameterUri, - MyInvocation.MyCommand.Parameters.Keys.ToArray()); + staticParameterNames); } } else if (!string.IsNullOrEmpty(TemplateUri) && @@ -197,7 +224,7 @@ public object GetDynamicParameters() TemplateUri, TemplateParameterObject, this.ResolvePath(TemplateParameterFile), - MyInvocation.MyCommand.Parameters.Keys.ToArray()); + staticParameterNames); } else { @@ -205,7 +232,7 @@ public object GetDynamicParameters() TemplateUri, TemplateParameterObject, TemplateParameterUri, - MyInvocation.MyCommand.Parameters.Keys.ToArray()); + staticParameterNames); } } else if (!string.IsNullOrEmpty(TemplateSpecId) && @@ -218,21 +245,15 @@ public object GetDynamicParameters() throw new PSArgumentException("No version found in Resource ID"); } - ITemplateSpecsClient templateSpecsClient = - AzureSession.Instance.ClientFactory.CreateArmClient( - DefaultContext, - AzureEnvironment.Endpoint.ResourceManager - ); - if (!string.IsNullOrEmpty(resourceIdentifier.Subscription) && - templateSpecsClient.SubscriptionId != resourceIdentifier.Subscription) + TemplateSpecsClient.SubscriptionId != resourceIdentifier.Subscription) { // The template spec is in a different subscription than our default // context. Force the client to use that subscription: - templateSpecsClient.SubscriptionId = resourceIdentifier.Subscription; + TemplateSpecsClient.SubscriptionId = resourceIdentifier.Subscription; } - var templateSpecVersion = templateSpecsClient.TemplateSpecVersions.Get( + var templateSpecVersion = TemplateSpecsClient.TemplateSpecVersions.Get( ResourceIdUtility.GetResourceGroupName(templateSpecId), ResourceIdUtility.GetResourceName(templateSpecId).Split('/')[0], resourceIdentifier.ResourceName); @@ -250,7 +271,7 @@ public object GetDynamicParameters() templateObj, TemplateParameterObject, this.ResolvePath(TemplateParameterFile), - MyInvocation.MyCommand.Parameters.Keys.ToArray()); + staticParameterNames); } else { @@ -258,7 +279,7 @@ public object GetDynamicParameters() templateObj, TemplateParameterObject, TemplateParameterUri, - MyInvocation.MyCommand.Parameters.Keys.ToArray()); + staticParameterNames); } } } @@ -344,5 +365,39 @@ protected string GetDeploymentDebugLogLevel(string deploymentDebugLogLevel) return debugSetting; } + + /// + /// Gets the names of the static parameters defined for this cmdlet. + /// + protected string[] GetStaticParameterNames() + { + if (MyInvocation.MyCommand != null) + { + // We're running inside the shell... parameter information will already + // be resolved for us: + return MyInvocation.MyCommand.Parameters.Keys.ToArray(); + } + + // This invocation is internal (e.g: through a unit test), fallback to + // collecting the command/parameter info explicitly from our current type: + + CmdletAttribute cmdletAttribute = (CmdletAttribute)this.GetType() + .GetCustomAttributes(typeof(CmdletAttribute), true) + .FirstOrDefault(); + + if (cmdletAttribute == null) + { + throw new InvalidOperationException( + $"Expected type '{this.GetType().Name}' to have CmdletAttribute." + ); + } + + // The command name we provide for the temporary CmdletInfo isn't consumed + // anywhere other than instantiation, but let's resolve it anyway: + string commandName = $"{cmdletAttribute.VerbName}-{cmdletAttribute.NounName}"; + + CmdletInfo cmdletInfo = new CmdletInfo(commandName, this.GetType()); + return cmdletInfo.Parameters.Keys.ToArray(); + } } } diff --git a/src/Resources/Resources.Test/ResourceGroupDeployments/NewAzureResourceGroupDeploymentCommandTests.cs b/src/Resources/Resources.Test/ResourceGroupDeployments/NewAzureResourceGroupDeploymentCommandTests.cs index 71d669129276..9423b19dcc61 100644 --- a/src/Resources/Resources.Test/ResourceGroupDeployments/NewAzureResourceGroupDeploymentCommandTests.cs +++ b/src/Resources/Resources.Test/ResourceGroupDeployments/NewAzureResourceGroupDeploymentCommandTests.cs @@ -30,6 +30,12 @@ using System.Collections; using FluentAssertions; using ProvisioningState = Microsoft.Azure.Commands.ResourceManager.Cmdlets.Entities.ProvisioningState; +using Microsoft.Azure.Management.ResourceManager; +using Newtonsoft.Json.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Rest.Azure; +using Newtonsoft.Json; namespace Microsoft.Azure.Commands.Resources.Test { @@ -39,6 +45,10 @@ public class NewAzureResourceGroupDeploymentCommandTests : RMTestBase private Mock resourcesClientMock; + private Mock templateSpecsClientMock; + + private Mock templateSpecsVersionOperationsMock; + private Mock commandRuntimeMock; private string resourceGroupName = "myResourceGroup"; @@ -54,13 +64,20 @@ public class NewAzureResourceGroupDeploymentCommandTests : RMTestBase public NewAzureResourceGroupDeploymentCommandTests(ITestOutputHelper output) { resourcesClientMock = new Mock(); + + templateSpecsClientMock = new Mock(); + templateSpecsClientMock.SetupAllProperties(); + templateSpecsVersionOperationsMock = new Mock(); + templateSpecsClientMock.Setup(m => m.TemplateSpecVersions).Returns(templateSpecsVersionOperationsMock.Object); + XunitTracingInterceptor.AddToContext(new XunitTracingInterceptor(output)); commandRuntimeMock = new Mock(); SetupConfirmation(commandRuntimeMock); cmdlet = new NewAzureResourceGroupDeploymentCmdlet() { CommandRuntime = commandRuntimeMock.Object, - ResourceManagerSdkClient = resourcesClientMock.Object + ResourceManagerSdkClient = resourcesClientMock.Object, + TemplateSpecsClient = templateSpecsClientMock.Object }; } @@ -271,5 +288,68 @@ public void CreatesNewPSResourceGroupDeploymentWithUserTemplateEmptyRollback() commandRuntimeMock.Verify(f => f.WriteObject(expected), Times.Once()); } + + /// + /// When deployments are created using a template spec, the dynamic parameters are + /// resolved by reading the parameters from the template spec version's template body. + /// Previously a bug was present that prevented successful dynamic parameter resolution + /// if the template spec existed in a subscription outside the current subscription + /// context. This test validates dynamic parameter resolution works for deployments using + /// cross-subscription template specs. + /// + + [Fact] + [Trait(Category.AcceptanceType, Category.CheckIn)] + public void ResolvesDynamicParametersWithCrossSubTemplateSpec() + { + const string templateSpecSubscriptionId = "10000000-0000-0000-0000-000000000000"; + const string templateSpecRGName = "someRG"; + const string templateSpecName = "myTemplateSpec"; + const string templateSpecVersion = "v1"; + + string templateSpecId = $"/subscriptions/{templateSpecSubscriptionId}/" + + $"resourceGroups/{templateSpecRGName}/providers/Microsoft.Resources/" + + $"templateSpecs/{templateSpecName }/versions/{templateSpecVersion}"; + + var templateContentForTest = File.ReadAllText(templateFile); + var template = JsonConvert.DeserializeObject(templateContentForTest); + + templateSpecsVersionOperationsMock.Setup(f => f.GetWithHttpMessagesAsync( + templateSpecRGName, + templateSpecName, + templateSpecVersion, + null, + new CancellationToken())) + .Returns(() => { + + // We should only be getting this template spec from the expected subscription: + Assert.Equal(templateSpecSubscriptionId, templateSpecsClientMock.Object.SubscriptionId); + + var versionToReturn = new TemplateSpecVersion( + location: "westus2", + id: templateSpecId, + name: templateSpecVersion, + type: "Microsoft.Resources/templateSpecs/versions", + template: JObject.Parse(templateContentForTest) + ); + + return Task.Factory.StartNew(() => + new AzureOperationResponse() + { + Body = versionToReturn + } + ); + }); + + cmdlet.ResourceGroupName = resourceGroupName; + cmdlet.Name = deploymentName; + cmdlet.TemplateSpecId = templateSpecId; + + var dynamicParams = cmdlet.GetDynamicParameters() as RuntimeDefinedParameterDictionary; + + dynamicParams.Should().NotBeNull(); + dynamicParams.Count().Should().Be(template.Parameters.Count); + dynamicParams.Keys.Should().Contain(template.Parameters.Keys); + } } } From 94249db716792e3d9697fe33ce2261f7a931f3be Mon Sep 17 00:00:00 2001 From: Stuart Konen Date: Sun, 29 Nov 2020 19:13:26 -0800 Subject: [PATCH 3/4] Template Specs: Fixing issue with test on Unix based systems --- .../NewAzureResourceGroupDeploymentCommandTests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Resources/Resources.Test/ResourceGroupDeployments/NewAzureResourceGroupDeploymentCommandTests.cs b/src/Resources/Resources.Test/ResourceGroupDeployments/NewAzureResourceGroupDeploymentCommandTests.cs index 9423b19dcc61..3e8fd136e786 100644 --- a/src/Resources/Resources.Test/ResourceGroupDeployments/NewAzureResourceGroupDeploymentCommandTests.cs +++ b/src/Resources/Resources.Test/ResourceGroupDeployments/NewAzureResourceGroupDeploymentCommandTests.cs @@ -311,7 +311,8 @@ public void ResolvesDynamicParametersWithCrossSubTemplateSpec() $"resourceGroups/{templateSpecRGName}/providers/Microsoft.Resources/" + $"templateSpecs/{templateSpecName }/versions/{templateSpecVersion}"; - var templateContentForTest = File.ReadAllText(templateFile); + // Note: We use GetFullPath below to normalize paths for Unix based systems... + var templateContentForTest = File.ReadAllText(Path.GetFullPath(templateFile)); var template = JsonConvert.DeserializeObject(templateContentForTest); templateSpecsVersionOperationsMock.Setup(f => f.GetWithHttpMessagesAsync( From b11ece7cb993ecb78272060b323edc7244f3e760 Mon Sep 17 00:00:00 2001 From: Stuart Konen Date: Sun, 29 Nov 2020 20:10:39 -0800 Subject: [PATCH 4/4] Fix for test failure --- .../NewAzureResourceGroupDeploymentCommandTests.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Resources/Resources.Test/ResourceGroupDeployments/NewAzureResourceGroupDeploymentCommandTests.cs b/src/Resources/Resources.Test/ResourceGroupDeployments/NewAzureResourceGroupDeploymentCommandTests.cs index 3e8fd136e786..4be4a82ba0c0 100644 --- a/src/Resources/Resources.Test/ResourceGroupDeployments/NewAzureResourceGroupDeploymentCommandTests.cs +++ b/src/Resources/Resources.Test/ResourceGroupDeployments/NewAzureResourceGroupDeploymentCommandTests.cs @@ -311,8 +311,12 @@ public void ResolvesDynamicParametersWithCrossSubTemplateSpec() $"resourceGroups/{templateSpecRGName}/providers/Microsoft.Resources/" + $"templateSpecs/{templateSpecName }/versions/{templateSpecVersion}"; - // Note: We use GetFullPath below to normalize paths for Unix based systems... - var templateContentForTest = File.ReadAllText(Path.GetFullPath(templateFile)); + // Ensure our template file path is normalized for the current system: + var normalizedTemplateFilePath = (Path.DirectorySeparatorChar != '\\') + ? templateFile.Replace('\\', Path.DirectorySeparatorChar) // Other/Unix based + : templateFile; // Windows based (already valid) + + var templateContentForTest = File.ReadAllText(normalizedTemplateFilePath); var template = JsonConvert.DeserializeObject(templateContentForTest); templateSpecsVersionOperationsMock.Setup(f => f.GetWithHttpMessagesAsync(