Azure / enterprise-azure-policy-as-code

Enterprise-ready Azure Policy-as-Code (PaC) solution (includes Az DevOps pipeline)
https://azure.github.io/enterprise-azure-policy-as-code/
MIT License
436 stars 244 forks source link

`Convert-PolicyResourcesToDetails` in parallel does not throw if it fails #777

Open o-l-a-v opened 1 month ago

o-l-a-v commented 1 month ago

Describe the bug

Convert-PolicyResourcesToDetails does not throw if it fails when running in parallel.

Snippet from our CI/CD run:

Image

To Reproduce

Have a faulty policyDefinition matching the fail that was thrown in the screenshot?

Expected behavior

If ANY of those parallel jobs have ANY failure the function should throw.

Screenshots

Already added to "Describe the bug".

EPAC Version

10.6.2

o-l-a-v commented 1 month ago

An obeservation: The starter kit does not set $ErrorActionPreference = Stop when doing Build-DeploymentPlans, and neither do we.

Default error action in PowerShell is to continue on error.

This might be related? And shouldn't we expect the plan to fail if Convert-PolicyResourcesToDetails has >= 0 failures?

anwather commented 1 month ago

Possibly it should fail - however that code block shouldn't be possible to reach so I'm curious what did the faulty policy look like? I don't want to go down the path of running tests on all the policy objects before they are deployed at this stage.,

o-l-a-v commented 1 month ago

Here is the faulty policyDefinition:

{
  "$schema": "https://raw.githubusercontent.com/Azure/enterprise-azure-policy-as-code/main/Schemas/policy-definition-schema.json",
  "name": "3b98831c-f754-4ce7-bb79-b88f99ed9f2c",
  "properties": {
    "displayName": "Inherit a tag from the subscription",
    "mode": "All",
    "description": "Adds or replaces the specified tag and value from the containing subscription when any resource is created or updated. Existing resources can be remediated by triggering a remediation task.",
    "metadata": {
      "category": "Tags",
      "version": "1.0.0"
    },
    "version": "1.0.0",
    "parameters": {
      "tagName": {
        "type": "String",
        "metadata": {
          "displayName": "Tag Name",
          "description": "Name of the tag, such as 'environment'"
        }
      }
    },
    "policyRule": {
      "if": {
        "allOf": [
          {
            "field": "[concat('tags[', parameters('tagName'), ']')]",
            "notEquals": "[subscription().tags[parameters('tagName')]]"
          },
          {
            "value": "[subscription().tags[parameters('tagName')]]",
            "notEquals": ""
          }
        ]
      },
      "then": {
        "effect": "[parameters('effect')]",
        "details": {
          "roleDefinitionIds": [
            "/providers/microsoft.authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c"
          ],
          "operations": [
            {
              "operation": "addOrReplace",
              "field": "[concat('tags[', parameters('tagName'), ']')]",
              "value": "[subscription().tags[parameters('tagName')]]"
            }
          ]
        }
      }
    },
    "versions": [
      "1.0.0"
    ]
  },
  "id": "/providers/Microsoft.Authorization/policyDefinitions/b27a0cbd-a167-4dfa-ae64-4337be671140",
  "type": "Microsoft.Authorization/policyDefinitions"
}

Parameter "effect" was wrongfully removed. And id and type at the bottom was not removed.

Seems EPAC does not correctly validate schema either? There is no "additionalProperties": false in the schema:

anwather commented 1 month ago

It may be worth investigating the AzPolicyTest powershell module to perform this testing. The repo can be forked and updated to include tests such as check that parameters specified in the policy rules are also specified in the parameters section. The purpose of the schema in EPAC was simply to provide Intellisense to assist with authoring in VS Code - not to provide strict validation.

Accompanying blog for AzPolicyTest -> https://blog.tyang.org/2024/03/08/azpolicytest-v2-release

o-l-a-v commented 1 month ago

Is there a way to get EPAC to "compile" / build EPAC policyDefinitions and policySetDefinitions to "full fat" ARM templates, which then can be linted / tested by AzPolicyTest?

From initial testing, AzPolicyTest would require a lot of changes. But making EPAC output testable JSON files might be less work?

anwather commented 1 month ago

So I tested using the on a policy definition - it didn't need to be in the full ARM template version for it to work. If it was me looking to this as I said I would fork that repo and expand on the tests - or help contribute to the original project (which I've see you've already done :) )

anwather commented 1 month ago

My testing:

{
  "$schema": "https://raw.githubusercontent.com/Azure/enterprise-azure-policy-as-code/main/Schemas/policy-definition-schema.json",
  "name": "rg-require-tag",
  "properties": {
    "displayName": "Custom - Require a tag on resource groups",
    "description": "Enforces existence of a tag on resource groups.",
    "mode": "All",
    "metadata": {
      "category": "Tags",
      "version": "1.0.0"
    },
    "parameters": {
      "effect": {
        "metadata": {
          "displayName": "Effect",
          "description": "Set to Audit or Deny"
        },
        "allowedValues": [
          "Audit",
          "Deny",
          "Disabled"
        ],
        "type": "String",
        "defaultValue": "Deny"
      },
      "tagName": {
        "metadata": {
          "displayName": "Tag Name",
          "description": "Name of the tag, such as 'environment'"
        },
        "type": "String"
      }
    },
    "policyRule": {
      "if": {
        "allOf": [
          {
            "field": "type",
            "equals": "Microsoft.Resources/subscriptions/resourceGroups"
          },
          {
            "field": "[concat('tags[', parameters('tagName'), ']')]",
            "exists": "false"
          }
        ]
      },
      "then": {
        "effect": "[parameters('effect')]"
      }
    }
  }
}

Output: Image

I didn't adjust the file at all (except rename to .json from .jsonc

o-l-a-v commented 1 month ago

Nice, you're right @anwather. 😊 I made a PR to add JSONC support.

anwather commented 1 month ago

As discussed, this is caused by invalid policy definitions which can be discovered using AzPolicyTest. I'm going to mark this as an enhancement to be completed at a later date as these errors tend to be surfaced during a deploy stage anyway.