Skip to content
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

fix: [[, double effect, empty default #1705

Closed
wants to merge 1 commit into from
Closed

Conversation

SvenSowa
Copy link

Fixed double square brackets "[[parameters".
Fixed double "effect, effect1", removed "effect1". Fixed "defaultValue": "" and removed it, since all parameters are really required.

Overview/Summary

The updated template can be deployed via the REST API without an error. Also, it will show the parameters correctly in the Azure portal.

This PR fixes/adds/changes/removes

  1. Fixes syntax error with "[["
  2. Removed "effect1" since it is defined twice as "effect" and "effect1"
  3. Removed defaultValue:"" since it leads to a faulty deployment, if one ignores to set all parameters, when assigning the initiative

Breaking Changes

  1. No assignment of initiative with default values possible. It would be a broken assignment anyhow.

Testing Evidence

When deployed with "[[parameters" syntax error, the REST API throws this error:
url:
https://learn.microsoft.com/en-us/rest/api/policy/policy-set-definitions/create-or-update?view=rest-policy-2023-04-01&tabs=HTTP&tryIt=true&source=docs#code-try-0
body:
{
"error": {
"code": "UnusedPolicyParameters",
"message": "The policy set '' has defined parameters 'azureFilePrivateDnsZoneId,azureAutomationWebhookPrivateDnsZoneId,azureAutomationDSCHybridPrivateDnsZoneId,azureCosmosSQLPrivateDnsZoneId,azureCosmosMongoPrivateDnsZoneId,azureIotCentralPrivateDnsZoneId,azureStorageTablePrivateDnsZoneId,azureStorageTableSecondaryPrivateDnsZoneId,azureSiteRecoveryBackupPrivateDnsZoneID,azureSiteRecoveryBlobPrivateDnsZoneID,azureSiteRecoveryQueuePrivateDnsZoneID' which are not used in referenced policy definitions. Please either remove these parameters from the definition or ensure that they are used."
}
}

When deployed with "effect" and "effect1", it will show double in the Azure portal:
image

When deployed with "defaultValue": "", it will not show the parameters in the Azure portal, even though they are really required:
image

The fixed version can be deployed trough the REST API, plus it will show the parameters correctly and the effect shows up only once:
image

image

Azure Public

[Deploy To Azure](https://portal.azure.com/#blade/Microsoft_Azure_CreateUIDef/CustomDeploymentBlade/uri/https%3A%2F%2Fraw.githubusercontent.com%2F{YOUR GITHUB ORG/ACCOUNT HERE - Remove Curly Brackets Also}%2FEnterprise-Scale%2F{YOUR GITHUB BRANCH NAME HERE - Remove Curly Brackets Also}%2FeslzArm%2FeslzArm.json/uiFormDefinitionUri/https%3A%2F%2Fraw.githubusercontent.com%2F{YOUR GITHUB ORG/ACCOUNT HERE - Remove Curly Brackets Also}%2FEnterprise-Scale%2F{YOUR GITHUB BRANCH NAME HERE - Remove Curly Brackets Also}%2FeslzArm%2Feslz-portal.json)

Azure US Gov (Fairfax)

[Deploy To Azure](https://portal.azure.us/#blade/Microsoft_Azure_CreateUIDef/CustomDeploymentBlade/uri/https%3A%2F%2Fraw.githubusercontent.com%2F{YOUR GITHUB ORG/ACCOUNT HERE - Remove Curly Brackets Also}%2FEnterprise-Scale%2F{YOUR GITHUB BRANCH NAME HERE - Remove Curly Brackets Also}%2FeslzArm%2FeslzArm.json/uiFormDefinitionUri/https%3A%2F%2Fraw.githubusercontent.com%2F{YOUR GITHUB ORG/ACCOUNT HERE - Remove Curly Brackets Also}%2FEnterprise-Scale%2F{YOUR GITHUB BRANCH NAME HERE - Remove Curly Brackets Also}%2FeslzArm%2Ffairfaxeslz-portal.json)

As part of this Pull Request I have

  • Checked for duplicate Pull Requests
  • Associated it with relevant issues, for tracking and closure.
  • Ensured my code/branch is up-to-date with the latest changes in the main branch
  • Performed testing and provided evidence.
  • Ensured contribution guidance is followed.
  • Updated relevant and associated documentation.
  • Updated the "What's New?" wiki page (located: /docs/wiki/whats-new.md)

Fixed double square brackets "[[parameters".
Fixed double "effect, effect1", removed "effect1".
Fixed "defaultValue": "" and removed it, since all parameters are really required.
@SvenSowa SvenSowa requested a review from a team as a code owner July 11, 2024 09:37
@Springstone
Copy link
Member

Hi @SvenSowa, many thanks for your submission. I delighted to see engagement from our community!
Regrettably though, we cannot merge this due to several reason relating to how Azure Policy and ALZ (this repo) handles policy.

  1. Policies in this repo are not intended to be deployed as is. If you would like to deploy the policy, I would highly recommend looking it up on AzAdvertizer and copying the definition from there into a new policy, or use other tools like Enterprise Policy-as-Code (https://aka.ms/epac). Policies in this repo are authored to support aggregation into a single ARM template (so we deploy one template with all policies), and in order to do this, we need to escape template functions which include parameters hence the double [[. This is required for ALZ purposes, and you will see this in every policy/initiative in this repo.
  2. The Azure Policy engine is case sensitive, which is why we have two versions of the effect parameter, one with upper case DeployIfNotExists and one with lower d deployIfNotExists. These are not evaluated the same and is related to how the built-in policy defined the allowed effect parameter.
  3. For defaultValue="", this is needed, as when you deploy Private DNS Zones, you may not want to deploy all of them (it happens). We need to pass through a value in the event it's not specified, e.g. you only want private DNS for storage - then we need that default value for all the other options.

I hope this makes sense, and feel free to comment on this, I really don't want to discourage you from contributing!
I'll close the PR for now, as we can't proceed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants