-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Changes from all commits
c5ceee2
1c9e87b
cfbca37
d4537e9
b487ba2
ef53917
bf9aec6
81f9384
5cdfdc7
fb1d10e
6fc4b9e
0b2b101
26a48e9
0bf829f
e80e719
5dfaed9
7b5de51
5cf1478
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 |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
namespace Microsoft.Azure.Commands.ResourceManager.Cmdlets.Entities.Policy | ||
{ | ||
using Newtonsoft.Json; | ||
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 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. 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 can be a 1-click when merging on this page, though I don't have permission to do that now. 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. ok please squash before merging then. |
||
using Newtonsoft.Json.Linq; | ||
|
||
/// <summary> | ||
/// The policy assignment properties. | ||
|
@@ -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; } | ||
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. You can rename this to Parameter to be consistent with the PolicyDefinitionProperties class 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. 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 |
---|---|---|
|
@@ -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; } | ||
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. Can you please confirm that there are no changes required for other policy cmdlets? i.e.,
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. AFAIK, no impact at this time. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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) | ||
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. you can use var instead of string as is the convention. 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. 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 |
---|---|---|
|
@@ -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"; | ||
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. @luanshixia the name of this parameter set is misleading - 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. @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> | ||
|
@@ -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.")] | ||
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. 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 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. As we discussed, I fixed this according to NewAzureRmRGDeployment. 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. Is this line required? 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. 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. 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. @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. 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. @cormacpayne So there are 3 usages:
We don't want users to set both 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. @luanshixia I am fine with the three different parameter sets, but there is no need to change |
||
[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.")] | ||
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. @luanshixia how will users use this 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. @cormacpayne I checked the existing code and found pipeline usage is supported across all Resources cmdlets. So the user can do like: |
||
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> | ||
|
@@ -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() | ||
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. who calls this method and can u point me to how dynamic parameters work? 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 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. 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 doesn't look like it belongs here. Can we move this out to a common/util file? 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 is the implementation of the interface IDynamicParameters, which the class conforms to. 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. @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 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. @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 |
---|---|---|
|
@@ -145,4 +145,4 @@ protected string GetResourceId() | |
extensionResourceName: this.Name); | ||
} | ||
} | ||
} | ||
} |
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" | ||
} | ||
} |
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.
just curious as to why the new API version date is set to December instead of a more recent date?
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.
Per the resource schema seen on Azure Portal, the latest version for policy is 2016-12-01: