Policy key already exists

[BernieWhite]

Description of the issue

When policy definitions are assigned multiple times (which may have different parameters) the subsequent occurrences fail.

Expected behaviour

We need to:

• Ignore adding cases were the same policy definition is assigned with the same parameters to prevent duplicates.
• Generate a unique name and ref for each policy rule. We should be able to do this via a hash of the condition/ pre-conditions.

For example:

• Ref: Policy.e749c2d003da

Error output

Export-AzPolicyAssignmentRuleData: An item with the same key has already been added. Key: /providers/Microsoft.Authorization/policyDefinitions/26a828e1-e88f-464e-bbb3-c134a282b9de

Module in use and version:

• Module: PSRule.Rules.Azure
• Version: 1.20.0-B0028

Captured output from $PSVersionTable:

Name                           Value
----                           -----
PSVersion                      7.2.6
PSEdition                      Core
GitCommitId                    7.2.6
OS                             Microsoft Windows 10.0.22000
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

Additional context

Related to #181

[BernieWhite]

@fbinotto I think this issue will be dealing with the dictionaries defined here when new items are added.

[ArmaanMcleod]

@BernieWhite I can work on this one.

Not sure how we would generate a unique name for the policy rules, since the current implementation just uses the definition name. Could we keep the same name but use a different ref?

Also, not sure how we'd handle cases where the same policy definition is assigned but different variations of parameters are used. Would we include all rules and generate unique ref for each different variation? For assignments with the same parameters, we can skip those.

Also thinking for the dictionary key collision issue, should include the ref as part of the key? instead of just the definition id which results in key conflicts for duplicate definitions.

[BernieWhite]

@ArmaanMcleod name and ref both need to be unique.

Yes. I think the error is a dictionary collision, however I think we are still going to need to resolve it more broadly.

---

Consider an initiative assignment that might be used for tagging. To enforce two or more tags it's highly likely and recommended that a customer would reference the same policy definition twice such as the built-in Require a tag on resource groups or Inherit a tag from the resource group.

In both cases the customer would just use the tagName parameter to supply the actual tag.

---

I was thinking a hash could be generated of the Condition property here:

PSRule.Rules.Azure/src/PSRule.Rules.Azure/Data/Policy/Models.cs

Line 105 in 7be571c

public JObject Condition { get; set; }

Although we would probably also need to think about any pre-conditions that would come out of #1708 at some point.

If we generate a hash based on definitionId + condition + pre-conditions this should give us something unique that can be used to avoid collisions and deterministic.

[ArmaanMcleod]

@BernieWhite I'm assuming your tags example would like:

"policyDefinitions": [
      {
        "policyDefinitionReferenceId": "Inherit a tag from the subscription_1",
        "policyDefinitionId": "/providers/Microsoft.Authorization/policyDefinitions/b27a0cbd-a167-4dfa-ae64-4337be671140",
        "parameters": {
          "tagName": {
            "value": "costCentre"
          }
        },
        "groupNames": []
      },
      {
        "policyDefinitionReferenceId": "Inherit a tag from the subscription_2",
        "policyDefinitionId": "/providers/Microsoft.Authorization/policyDefinitions/b27a0cbd-a167-4dfa-ae64-4337be671140",
        "parameters": {
          "tagName": {
            "value": "department"
          }
        },
        "groupNames": []
      }
    ]

Where the same definition is used in the initiative twice, but different tagName parameters.

The problem I see here is Export-AzPolicyAssignmentData won't include these tagName parameter values, as it only includes initiative level parameters, so it becomes difficult to detect if the same parameters have been used for both definitions.

I think this can be fixed by tweaking ExpandPolicyAssignment to include parameters from definitions like this. We also would need to consider if policyDefinitionReferenceId needs to be included as well.

What do you think?

[BernieWhite]

@ArmaanMcleod That's one variation, but definition parameter can be fed from initiative assignment parameters as well.

policyDefinitionReferenceId is only unique within an initiative nor specifically between initiatives, not sure on what the value including it yet would be.

Using the Condition has advantages because it is already processed by the export process it is going to help deduplicate rules. You could do this via the unprocessed policy definitions but it would be likely more work and more complex.

[ArmaanMcleod]

@BernieWhite Yeah fully agree, I think the issue I'm seeing is how the visitor is processing parameters before expanding the policy rule: https://github.com/Azure/PSRule.Rules.Azure/blob/main/src/PSRule.Rules.Azure/Data/Policy/PolicyAssignmentVisitor.cs#L171-L182

In order for it to look up and expand parameters, the parameters from assignment level are set beforehand. This means for the condition approach to be reliable, it must be processed and expanded first, otherwise policy definitions will be duplicated since the conditions will remain the same.

Gets challenging to do definition duplication detection with condition hash before the condition is expanded 😄.

[BernieWhite]

@ArmaanMcleod I've refactored for a fair bit for #1708. I think it should be easier to implement this with the latest updates.

If you get stuck, let me know.

[ArmaanMcleod]

@BernieWhite What do you think of this for unique hashes for ref and name:

"name": "Azure.Policy.f5388351250d",
"ref": "Azure.Policy.f5388351250d",

I kept them the same for now as they both use the hash(hexidecimal with length 6) of the definitionId + condition + pre-condition.

[BernieWhite]

@ArmaanMcleod That should be fine. In terms of Azure. I was thinking that customers may want to customize this prefix and it may make sense if customers have multiple scopes or environments that want to run tests for, but it could default to Azure..

Configuration would be by ps-rule.yaml or command-line parameter. Something like AZURE_POLICY_RULE_PREFIX and -RulePrefix.

Maybe let's allow for this. And either tackle this in this or a separate issue.

[ArmaanMcleod]

@BernieWhite That sounds good. Should we create something like a PolicyRuleOption to encapsulate these specific configurations for policy rules?

Something like this:

configuration:
  AZURE_POLICY_RULE:
    prefix: 'something'
    otherConfig: ...

Or would you rather just keep it simpler

configuration:
  AZURE_POLICY_RULE_PREFIX: 'something'

I was thinking if we should use the first approach in case we need to configure more stuff together like the other options.

[BernieWhite]

@ArmaanMcleod Let's go with option 2. It adds complexity to manage merging nested objects. If we don't need to better to keep it simple.