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

Add profile name parameter to diag policies #1059

Merged
merged 11 commits into from
Oct 25, 2022
Merged

Conversation

anwather
Copy link
Contributor

@anwather anwather commented Sep 28, 2022

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

  • 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.
  • Updated relevant and associated documentation.
  • Updated the "What's New?" wiki page (located: /docs/wiki/whats-new.md)

Copy link
Contributor

@jfaurskov jfaurskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution @anwather, great work! Just a couple of minor details if you don't mind.

  1. Please assign a default value (SetByPolicy) for the profileName parameter or this will be a breaking change.
  2. Please remember to update the version in policy meta data. We are following this for versioning
  3. Please also update the associated policy set definition (Deploy-Diagnostics-LogAnalytics.) with the appropriate parameter and default parameters, including parameter assignment for all child policies

@krowlandson
Copy link
Contributor

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.

@cloudchristoph
Copy link

Are you sure about the double opening square brackets?

@jtracey93
Copy link
Collaborator

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 👍

@krowlandson
Copy link
Contributor

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?

@anwather
Copy link
Contributor Author

@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
Copy link
Contributor Author

anwather commented Oct 11, 2022

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?

@jtracey93
Copy link
Collaborator

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. :)

@jfaurskov
Copy link
Contributor

@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?

@jtracey93
Copy link
Collaborator

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

@anwather
Copy link
Contributor Author

anwather commented Oct 11, 2022 via email

jfaurskov
jfaurskov previously approved these changes Oct 21, 2022
Copy link
Contributor

@jfaurskov jfaurskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions bot requested a review from a team as a code owner October 21, 2022 08:00
@krowlandson
Copy link
Contributor

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 krowlandson marked this pull request as draft October 21, 2022 08:28
@krowlandson krowlandson marked this pull request as ready for review October 21, 2022 08:28
@krowlandson krowlandson temporarily deployed to csu-rw October 21, 2022 08:28 Inactive
Copy link
Contributor

@jfaurskov jfaurskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

@anwather
Copy link
Contributor Author

@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

@jfaurskov jfaurskov self-requested a review October 25, 2022 12:19
Copy link
Contributor

@jfaurskov jfaurskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's good to merge

Copy link
Contributor

@jfaurskov jfaurskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jfaurskov jfaurskov marked this pull request as draft October 25, 2022 12:28
@jfaurskov jfaurskov marked this pull request as ready for review October 25, 2022 12:31
@jfaurskov jfaurskov temporarily deployed to csu-rw October 25, 2022 12:31 Inactive
@jtracey93 jtracey93 added policy and removed Needs: Triage 🔍 Needs triaging by the team labels Oct 25, 2022
@jfaurskov jfaurskov merged commit 460b21e into Azure:main Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Policy bug when changing initiative profilename values
5 participants