Enterprise-Scale icon indicating copy to clipboard operation
Enterprise-Scale copied to clipboard

Add profile name parameter to diag policies

Open anwather opened this issue 2 years ago • 10 comments

Overview/Summary

Replace this with a brief description of what this Pull Request fixes, changes, etc.

This PR fixes/adds/changes/removes

  1. Updated diagnostic policies where "setByPolicy" is hardcoded as the extension resource in the DINE effect.

IHAC where they have deployed the diag policies with a different profile name as a parameter and several resources are reporting as non-compliant because it is looking for a diagnosticSettings/setByPolicy resource.

Remediation works successfully however policy continues to show as non-compliant.

Breaking Changes

None

Testing Evidence

Non compliant resource diagnostics became compliant after this fix.

As part of this Pull Request I have

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

anwather avatar Sep 28 '22 07:09 anwather

Thank you for your contribution @anwather

When you created this PR did you leave the permissions enabled for Allow edits from maintainers?

Just trying to work out why the Update Portal Experience action failed with a permissions error.

Putting the feedback from @jfaurskov aside, we will also need this to run successfully before we can merge the PR.

krowlandson avatar Oct 07 '22 15:10 krowlandson

Are you sure about the double opening square brackets?

cloudchristoph avatar Oct 10 '22 13:10 cloudchristoph

Are you sure about the double opening square brackets?

100% 🤓

This is due to the way the policies get wrapped in variables and then looped over in the resulting compiled policies.json file.

If you werent wrapping them, then yes the escaping isnt the same 👍

jtracey93 avatar Oct 10 '22 14:10 jtracey93

Are you sure about the double opening square brackets?

@jtracey93 is it worth us running a spike to see whether we can remove these without introducing too much additional complexity into the Bicep template?

We might be able to find a better balance between how I built this vs. the way you handle this in the ALZ/Bicep implementation?

krowlandson avatar Oct 11 '22 07:10 krowlandson

@jfaurskov - all those policies have a default value for profileName parameter - I have bumped all the versions up. Unless there is something I have missed?

anwather avatar Oct 11 '22 08:10 anwather

Thank you for your contribution @anwather

When you created this PR did you leave the permissions enabled for Allow edits from maintainers?

Just trying to work out why the Update Portal Experience action failed with a permissions error.

Putting the feedback from @jfaurskov aside, we will also need this to run successfully before we can merge the PR.

@krowlandson - Permission is definitely enabled on there

==> Commit changes...
[anwather/main 0dbf403] Auto-update Portal experience [anwather/748c6e8c]
 1 file changed, 52 insertions(+), 52 deletions(-)
==> Push changes...
Pushing changes to: anwather/Enterprise-Scale
To https://github.com/anwather/Enterprise-Scale.git
 ! [remote rejected] anwather/main -> anwather/main (permission denied)
error: failed to push some refs to 'https://github.com/anwather/Enterprise-Scale.git'

Why is it trying to push back to my fork?

anwather avatar Oct 11 '22 08:10 anwather

Are you sure about the double opening square brackets?

@jtracey93 is it worth us running a spike to see whether we can remove these without introducing too much additional complexity into the Bicep template?

We might be able to find a better balance between how I built this vs. the way you handle this in the ALZ/Bicep implementation?

lets open another item and discuss this there to not confuse this thread. But yes we can, but will need prioritisation conversation etc. :)

jtracey93 avatar Oct 11 '22 08:10 jtracey93

@jfaurskov - all those policies have a default value for profileName parameter - I have bumped all the versions up. Unless there is something I have missed?

@anwather looking good to me. We just need to get the update portal experience to work now. @krowlandson any ideas?

jfaurskov avatar Oct 11 '22 09:10 jfaurskov

@jfaurskov & @anwather do we need to update the Policy Set Definition/Initiative with the new parameter and version etc?

jtracey93 avatar Oct 11 '22 09:10 jtracey93

I didn't change the version as there was no update to the initiative - there is already a default value for profile name in the initiative as well

anwather avatar Oct 11 '22 09:10 anwather

Why is it trying to push back to my fork?

@anwather we are using automation to re-build the policies.json file from the src folder content, so when you update an individual policy file, it will trigger this process and write back into the PR.

I've fixed the git push issue but it looks like there's a merge conflict, so I'm just going to manually resolve that and we should be good to go on this one.

krowlandson avatar Oct 21 '22 08:10 krowlandson

@anwather could you please update versioning in the following files, then we should be set to merge: src/resources/Microsoft.Authorization/policyDefinitions/Deploy-Diagnostics-WVDAppGroup.json src/resources/Microsoft.Authorization/policyDefinitions/Deploy-Diagnostics-WVDHostPools.json src/resources/Microsoft.Authorization/policyDefinitions/Deploy-Diagnostics-WVDWorkspace.json

Updated

anwather avatar Oct 24 '22 04:10 anwather