Skip to content

Policy parameters support in New-AzureRmPolicyDefinition and New-AzureRmPolicyAssignment #3476

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

Merged
merged 18 commits into from
Feb 16, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public static class Constants
/// <summary>
/// The default policy API version.
/// </summary>
public static readonly string PolicyApiVersion = "2016-04-01";
public static readonly string PolicyApiVersion = "2016-12-01";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious as to why the new API version date is set to December instead of a more recent date?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the resource schema seen on Azure Portal, the latest version for policy is 2016-12-01:

        {
            "resourceType": "policyDefinitions",
            "locations": [],
            "apiVersions": [
                "2016-12-01",
                "2016-04-01",
                "2015-10-01-preview"
            ]
        },
        {
            "resourceType": "policyAssignments",
            "locations": [],
            "apiVersions": [
                "2016-12-01",
                "2016-04-01",
                "2015-10-01-preview"
            ]
        },


/// <summary>
/// The default providers API version.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
namespace Microsoft.Azure.Commands.ResourceManager.Cmdlets.Entities.Policy
{
using Newtonsoft.Json;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it has 3 commits. We should squash changes to a single commit, so that it is easy to revert the entire change if required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a 1-click when merging on this page, though I don't have permission to do that now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok please squash before merging then.

using Newtonsoft.Json.Linq;

/// <summary>
/// The policy assignment properties.
Expand All @@ -38,5 +39,11 @@ public class PolicyAssignmentProperties
/// </summary>
[JsonProperty(Required = Required.Always)]
public string PolicyDefinitionId { get; set; }

/// <summary>
/// The parameter values.
/// </summary>
[JsonProperty(Required = Required.Default)]
public JObject Parameters { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can rename this to Parameter to be consistent with the PolicyDefinitionProperties class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this property cannot be renamed because that should match the REST API. (I also only realized this when I caught an error after trying to keep consistency). Of course we can match names using JsonProperty attribute, but no need.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,5 +39,11 @@ public class PolicyDefinitionProperties
/// </summary>
[JsonProperty(Required = Required.Always)]
public JObject PolicyRule { get; set; }

/// <summary>
/// The parameters declaration.
/// </summary>
[JsonProperty(Required = Required.Default)]
public JObject Parameters { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please confirm that there are no changes required for other policy cmdlets? i.e.,

  1. Get/set/remove policy definition works with and without parameters
  2. Get/set/remove policy assignment works with and without parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, no impact at this time.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ namespace Microsoft.Azure.Commands.ResourceManager.Cmdlets.Extensions
using Newtonsoft.Json.Converters;
using Newtonsoft.Json.Linq;
using System;
using System.Collections;
using System.Collections.Generic;
using System.Globalization;
using System.IO;
Expand Down Expand Up @@ -212,5 +213,31 @@ public static bool TryConvertTo<TType>(this JToken jobject, out TType result)
result = default(TType);
return false;
}

/// <summary>
/// Converts an <see cref="IDictionary"/> object to <see cref="JObject"/> with an extra level labeled as "value".
/// </summary>
/// <param name="dict">The <see cref="IDictionary"/> object.</param>
/// <param name="keys">The key set to extract from the <see cref="IDictionary"/> object. Default is all keys.</param>
/// <returns>The conversion result.</returns>
public static JObject ToJObjectWithValue(this IDictionary dict, IEnumerable keys = null)
{
if (dict == null)
{
return null;
}
if (keys == null)
{
keys = dict.Keys;
}
var parameterObject = new JObject();
foreach (string paramKey in keys)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use var instead of string as is the convention.

Copy link
Contributor Author

@luanshixia luanshixia Feb 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here "keys" is of type IEnumerable not IEnumerable<string> so paramKey will not be inferred as string unless declared specific.

{
var valueObject = new JObject();
valueObject.Add("value", dict[paramKey].ToJToken());
parameterObject.Add(paramKey, valueObject);
}
return parameterObject;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -133,5 +133,32 @@ private static JToken ToJToken(object value)

return new JValue(value.ToString());
}

/// <summary>
/// Gets nested property values from a <see cref="PSObject"/> easily using property path.
/// </summary>
/// <param name="propertyObject">The <see cref="PSObject"/> to get property value from.</param>
/// <param name="propertyPath">The nested property path, e.g. "metadata.description".</param>
/// <returns>The value of the specified property.</returns>
internal static object GetPSObjectProperty(this PSObject propertyObject, string propertyPath)
{
var propertyNames = propertyPath.Split('.');
var tmpObject = propertyObject;
foreach (var propertyName in propertyNames)
{
var propertyInfo = tmpObject.Properties[propertyName];
if (propertyInfo != null)
{
if (propertyInfo.Value is PSObject)
{
tmpObject = propertyInfo.Value as PSObject;
continue;
}
return propertyInfo.Value;
}
return null;
}
return tmpObject;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,26 @@ namespace Microsoft.Azure.Commands.ResourceManager.Cmdlets.Implementation
using Microsoft.Azure.Commands.ResourceManager.Cmdlets.Components;
using Microsoft.Azure.Commands.ResourceManager.Cmdlets.Entities.Policy;
using Microsoft.Azure.Commands.ResourceManager.Cmdlets.Extensions;
using Microsoft.Azure.Commands.Common.Authentication;
using Microsoft.WindowsAzure.Commands.Utilities.Common;
using Newtonsoft.Json.Linq;
using System.Management.Automation;
using System;
using System.Linq;
using System.Collections;

/// <summary>
/// Creates a policy assignment.
/// </summary>
[Cmdlet(VerbsCommon.New, "AzureRmPolicyAssignment"), OutputType(typeof(PSObject))]
public class NewAzurePolicyAssignmentCmdlet : PolicyAssignmentCmdletBase
[Cmdlet(VerbsCommon.New, "AzureRmPolicyAssignment", DefaultParameterSetName = ParameterlessPolicyParameterSetName), OutputType(typeof(PSObject))]
public class NewAzurePolicyAssignmentCmdlet : PolicyAssignmentCmdletBase, IDynamicParameters
{
protected RuntimeDefinedParameterDictionary dynamicParameters = new RuntimeDefinedParameterDictionary();

protected const string PolicyParameterObjectParameterSetName = "Policy assignment with parameters via policy parameter object";
protected const string PolicyParameterStringParameterSetName = "Policy assignment with parameters via policy parameter string";
protected const string ParameterlessPolicyParameterSetName = "Policy assignment without parameters";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luanshixia the name of this parameter set is misleading - Name and Scope (and PolicyDefinition based on its implementation) parameters are globally required parameters, so you are always going to need to provide a value for them, meaning you'll never have a parameter set that is "parameterless"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cormacpayne No, "parameter" here refers to the policy parameters, not the cmdlet parameters.. This is similar to what is done for template parameters in NewAzureRmResourceGroupDeployment.


/// <summary>
/// Gets or sets the policy assignment name parameter.
/// </summary>
Expand All @@ -50,9 +61,30 @@ public class NewAzurePolicyAssignmentCmdlet : PolicyAssignmentCmdletBase
/// <summary>
/// Gets or sets the policy assignment policy definition parameter.
/// </summary>
[Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The pollicy definition object.")]
[Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The policy definition object.")]
Copy link
Contributor

@maphad maphad Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the parameter set to work correctly all OTHER parameters should belong to both parameter sets PolicyParameterObjectParameterSetName and PolicyParameterStringParameterSetName. You can see example in FindAzureResource Cmdlet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed, I fixed this according to NewAzureRmRGDeployment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So @cormacpayne is working on a fix for BreakingChangeAnalyzer which prevents this PR from passing the build (the reason: the analyzer compares the parameter set with the previous build. In previous build this property belongs to the default parameter set, so it enforces this for later builds). This line is a workaround which can be removed after his fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luanshixia if this parameter is required in every single parameter set, why make this change? Previously it was a globally required parameter, so it was required in any parameter set you used. The same applies now with the changes you are trying to make here.

Copy link
Contributor Author

@luanshixia luanshixia Feb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cormacpayne So there are 3 usages:

  • PolicyDefinition only
  • PolicyDefinition + PolicyParameterObject
  • PolicyDefinition + PolicyParameter

We don't want users to set both PolicyParameterObject and PolicyParameter, so we use two different sets for them. But we also need to allow users to set none of PolicyParameterObject and PolicyParameter, which comes the third set. Note we have to set Mandatory=true for PolicyParameterObject and PolicyParameter or we will get parameter set resolving conflict.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luanshixia I am fine with the three different parameter sets, but there is no need to change PolicyDefinition from a global parameter to a parameter that's in each of the three parameter sets (that is what is implied by being a global parameter).

[Parameter(ParameterSetName = ParameterlessPolicyParameterSetName,
Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The policy definition object.")]
[Parameter(ParameterSetName = PolicyParameterObjectParameterSetName,
Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The policy definition object.")]
[Parameter(ParameterSetName = PolicyParameterStringParameterSetName,
Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The policy definition object.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luanshixia how will users use this ValueFromPipelineByPropertyName attribute for this parameter? Will there be an object they can pass through the pipeline to this cmdlet that contains the PolicyDefinition property? Will it also include the below parameters you are trying to add since they also have the ValueFromPipelineByPropertyName attribute as well?

Copy link
Contributor Author

@luanshixia luanshixia Feb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cormacpayne I checked the existing code and found pipeline usage is supported across all Resources cmdlets. So the user can do like: @{"PolicyDefinition": @{...} ...} | New-AzureRmPolicyAssignment

public PSObject PolicyDefinition { get; set; }

/// <summary>
/// Gets or sets the policy assignment policy parameter object.
/// </summary>
[Parameter(ParameterSetName = PolicyParameterObjectParameterSetName,
Mandatory = true, ValueFromPipelineByPropertyName = false, HelpMessage = "The policy parameter object.")]
public Hashtable PolicyParameterObject { get; set; }

/// <summary>
/// Gets or sets the policy assignment policy parameter file path or policy parameter string.
/// </summary>
[Parameter(ParameterSetName = PolicyParameterStringParameterSetName,
Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The policy parameter file path or policy parameter string.")]
[ValidateNotNullOrEmpty]
public string PolicyParameter { get; set; }

/// <summary>
/// Executes the cmdlet.
/// </summary>
Expand Down Expand Up @@ -112,11 +144,72 @@ private JToken GetResource()
{
DisplayName = this.DisplayName ?? null,
PolicyDefinitionId = this.PolicyDefinition.Properties["policyDefinitionId"].Value.ToString(),
Scope = this.Scope
Scope = this.Scope,
Parameters = this.GetParameters()
}
};

return policyassignmentObject.ToJToken();
}

object IDynamicParameters.GetDynamicParameters()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

who calls this method and can u point me to how dynamic parameters work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface method is called by the PowerShell engine (thus a callback), so PowerShell can know what parameters are defined dynamically in the cmdlet and allow users to provide values for them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look like it belongs here. Can we move this out to a common/util file?

Copy link
Contributor Author

@luanshixia luanshixia Feb 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the implementation of the interface IDynamicParameters, which the class conforms to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luanshixia why do you need to add dynamic parameters to this cmdlet? We try to avoid using them as much as possible since they cannot be picked up by our StaticAnalysis tooling and don't allow us to easily check any public interface changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cormacpayne Because we want to allow users to specify the policy parameters through cmdlet parameters, so we have to dynamically parse the policy definition and extract a list of defined parameters. That is from the requirements. In the future, we also want to add IntelliSense for the dynamic parameters, just like what we did for template deployment.

{
if (this.PolicyDefinition != null)
{
var parameters = this.PolicyDefinition.GetPSObjectProperty("Properties.parameters") as PSObject;
if (parameters != null)
{
foreach (var param in parameters.Properties)
{
var type = (param.Value as PSObject).Properties["type"];
var typeString = type != null ? type.Value.ToString() : string.Empty;
var description = (param.Value as PSObject).GetPSObjectProperty("metadata.description");
var helpString = description != null ? description.ToString() : string.Format("The {0} policy parameter.", param.Name);
var dp = new RuntimeDefinedParameter
{
Name = param.Name,
ParameterType = typeString.Equals("array", StringComparison.OrdinalIgnoreCase) ? typeof(string[]) : typeof(string)
};
dp.Attributes.Add(new ParameterAttribute
{
ParameterSetName = ParameterlessPolicyParameterSetName,
Mandatory = true,
ValueFromPipelineByPropertyName = false,
HelpMessage = helpString
});
this.dynamicParameters.Add(param.Name, dp);
}
}
}

return this.dynamicParameters;
}

private JObject GetParameters()
{
// Load parameters from local file or literal
if (this.PolicyParameter != null)
{
string policyParameterFilePath = this.TryResolvePath(this.PolicyParameter);
return FileUtilities.DataStore.FileExists(policyParameterFilePath)
? JObject.Parse(FileUtilities.DataStore.ReadFileAsText(policyParameterFilePath))
: JObject.Parse(this.PolicyParameter);
}

// Load from PS object
if (this.PolicyParameterObject != null)
{
return this.PolicyParameterObject.ToJObjectWithValue();
}

// Load dynamic parameters
var parameters = PowerShellUtilities.GetUsedDynamicParameters(dynamicParameters, MyInvocation);
if (parameters.Count() > 0)
{
return MyInvocation.BoundParameters.ToJObjectWithValue(parameters.Select(p => p.Name));
}

return null;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,19 @@ public class NewAzurePolicyDefinitionCmdlet : PolicyDefinitionCmdletBase
public string Description { get; set; }

/// <summary>
/// Gets or sets the policy parameter
/// Gets or sets the policy definition policy rule parameter
/// </summary>
[Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The rule for policy definition. This can either be a path to a file name containing the rule, or the rule as string.")]
[ValidateNotNullOrEmpty]
public string Policy { get; set; }

/// <summary>
/// Gets or sets the policy definition parameters parameter
/// </summary>
[Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = "The parameters declaration for policy definition. This can either be a path to a file name containing the parameters declaration, or the parameters declaration as string.")]
[ValidateNotNullOrEmpty]
public string Parameter { get; set; }

/// <summary>
/// Executes the cmdlet.
/// </summary>
Expand Down Expand Up @@ -113,7 +120,8 @@ private JToken GetResource()
{
Description = this.Description ?? null,
DisplayName = this.DisplayName ?? null,
PolicyRule = JObject.Parse(GetPolicyRuleObject().ToString())
PolicyRule = JObject.Parse(this.GetPolicyRuleObject().ToString()),
Parameters = this.Parameter == null ? null : JObject.Parse(this.GetParametersObject().ToString())
}
};

Expand All @@ -131,5 +139,17 @@ private JToken GetPolicyRuleObject()
? JToken.FromObject(FileUtilities.DataStore.ReadFileAsText(policyFilePath))
: JToken.FromObject(this.Policy);
}

/// <summary>
/// Gets the parameters object
/// </summary>
private JToken GetParametersObject()
{
string parametersFilePath = this.TryResolvePath(this.Parameter);

return File.Exists(parametersFilePath)
? JToken.FromObject(FileUtilities.DataStore.ReadFileAsText(parametersFilePath))
: JToken.FromObject(this.Parameter);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -145,4 +145,4 @@ protected string GetResourceId()
extensionResourceName: this.Name);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,15 @@
<None Include="keyVaultTemplateParams.json">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</None>
<None Include="SamplePolicyAssignmentParameters.json">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</None>
<None Include="SamplePolicyDefinitionParameters.json">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</None>
<None Include="SamplePolicyDefinitionWithParameters.json">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</None>
<None Include="SamplePolicyDefinition.json">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</None>
Expand Down Expand Up @@ -395,8 +404,14 @@
<None Include="SessionRecords\Microsoft.Azure.Commands.Resources.Test.ScenarioTests.PolicyTests\TestPolicyAssignmentCRUD.json">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</None>
<None Include="SessionRecords\Microsoft.Azure.Commands.Resources.Test.ScenarioTests.PolicyTests\TestPolicyAssignmentWithParameters.json">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</None>
<None Include="SessionRecords\Microsoft.Azure.Commands.Resources.Test.ScenarioTests.PolicyTests\TestPolicyDefinitionCRUD.json">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</None>
<None Include="SessionRecords\Microsoft.Azure.Commands.Resources.Test.ScenarioTests.PolicyTests\TestPolicyDefinitionWithParameters.json">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</None>
<None Include="SessionRecords\Microsoft.Azure.Commands.Resources.Test.ScenarioTests.ProviderFeatureTests\TestAzureProviderFeature.json">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"listOfAllowedLocations": {
"value": [
"West US",
"West US 2"
]
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"listOfAllowedLocations": {
"type": "array",
"metadata": {
"description": "An array of permitted locations for resources.",
"strongType": "location",
"displayName": "List of locations"
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"if": {
"not": {
"field": "location",
"in": "[parameters('listOfAllowedLocations')]"
}
},
"then": {
"effect": "deny"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,19 @@ public void TestPolicyAssignmentCRUD()
{
ResourcesController.NewInstance.RunPsTest("Test-PolicyAssignmentCRUD");
}

[Fact]
[Trait(Category.AcceptanceType, Category.CheckIn)]
public void TestPolicyDefinitionWithParameters()
{
ResourcesController.NewInstance.RunPsTest("Test-PolicyDefinitionWithParameters");
}

[Fact]
[Trait(Category.AcceptanceType, Category.CheckIn)]
public void TestPolicyAssignmentWithParameters()
{
ResourcesController.NewInstance.RunPsTest("Test-PolicyAssignmentWithParameters");
}
}
}
Loading