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

feat: Insights/scheduledQueryRule - Added missing properties #4718

Draft
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

AlexanderSehr
Copy link
Contributor

Description

  • Added missing actions properties
  • Added test case for actions
  • Updated to use avm-common-types

Closes #4708

Pipeline Reference

Pipeline
avm.res.insights.scheduled-query-rule

Type of Change

  • Update to CI Environment or utilities (Non-module affecting changes)
  • Azure Verified Module updates:
    • Bugfix containing backwards-compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in version.json:
      • Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description.
      • The bug was found by the module author, and no one has opened an issue to report it yet.
    • Feature update backwards compatible feature updates, and I have bumped the MINOR version in version.json.
    • Breaking changes and I have bumped the MAJOR version in version.json.
    • Update to documentation

Checklist

  • I'm sure there are no other open Pull Requests for the same update/change
  • I have run Set-AVMModule locally to generate the supporting module files.
  • My corresponding pipelines / checks run clean and green without any errors or warnings

@AlexanderSehr AlexanderSehr self-assigned this Mar 12, 2025
@AlexanderSehr AlexanderSehr added Needs: Core Team 🧞 This item needs the AVM Core Team to review it Status: Module Orphaned 👀 The module has no owner and is therefore orphaned at this time labels Mar 12, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Triage 🔍 Maintainers need to triage still Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue labels Mar 12, 2025
@description('Optional. Mute actions for the chosen period of time (in ISO 8601 duration format) after the alert is fired. If set, autoMitigate must be disabled.Relevant only for rules of the kind LogAlert.')
param suppressForMinutes string = ''
@description('Optional. Mute actions for the chosen period of time (in ISO 8601 duration format) after the alert is fired. If set, autoMitigate must be disabled. Relevant only for rules of the kind LogAlert.')
param suppressForMinutes string?

Copy link
Contributor

@ahmadabdalla ahmadabdalla Mar 18, 2025

Choose a reason for hiding this comment

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

I would like to suggest adding support for managed identity as well. I've implemented this for a customer recently:

var formattedUserAssignedIdentities = reduce(
  map((managedIdentities.?userAssignedResourceIds ?? []), (id) => { '${id}': {} }),
  {},
  (cur, next) => union(cur, next)
) //

var identity = !empty(managedIdentities)
  ? {
      type: (managedIdentities.?systemAssigned ?? false)
        ? (!empty(managedIdentities.?userAssignedResourceIds ?? {}) ? 'UserAssigned' : 'SystemAssigned')
        : (!empty(managedIdentities.?userAssignedResourceIds ?? {}) ? 'UserAssigned' : 'None')
      userAssignedIdentities: !empty(formattedUserAssignedIdentities) ? formattedUserAssignedIdentities : null
    }
  : null

Reference: https://learn.microsoft.com/en-us/azure/templates/microsoft.insights/scheduledqueryrules?pivots=deployment-language-bicep

Additional properties locks, roleAssignments

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @ahmadabdalla,
sounds good, will do :) I am however struggling adding a test case for the original changes of this PR (referencing an action group with parameters) which is currently blocking me a bit in finishing it (ref) 😄
In other words - it may take a moment to merge the change as I feel like I'm searching for a solution in the dark.

Copy link
Contributor Author

@AlexanderSehr AlexanderSehr Mar 18, 2025

Choose a reason for hiding this comment

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

Alright, added & tested. 💪 I used the fail() function to ensure a user knows they can only either define a system-assigned or user-assigned identity. Seemed appropriate at the time. 😏

If you configure both, the deployment would fail with the error message: 14:56:19 - The deployment 'a-r-i-sqr-max-t1-20250318T1403155517Z' failed with error(s). Showing 1 out of 1 error(s). Status Message: Deployment template validation failed: 'The template variable 'identity' is not valid: You can only configure either a system-assigned or user-assigned identities, not both.. Please see https://aka.ms/arm-functions for usage details.'. (Code:InvalidTemplate)

Learned about this fancy feature in a post by @sebassem today 🚀

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I missed these messages! I am glad it's working! Thank you @AlexanderSehr

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm I do remember seeing something like this. Can you try deploying a new test LAW just for testing sake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @ahmadabdalla, thanks for the suggestion. Just tested it (so far without luck). The web has also not yet yielded any insights. But I'll continue testing options

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @AlexanderSehr couple of things I did from a testing perspective before:

  • Set the max test case to use system identity.
  • Created a separate test case for Azure Resource Graph queries with user identities using the following properties for main.test.bicep:
module nestedDependencies 'dependencies.bicep' = {
  scope: resourceGroup
  name: '${uniqueString(deployment().name, resourceLocation)}-nestedDependencies'
  params: {
    managedIdentityName: 'dep-${namePrefix}-msi-${serviceShort}'
    location: resourceLocation
  }
}

resource managedIdentityRoleAssignment 'Microsoft.Authorization/roleAssignments@2022-04-01' = {
  name: guid('Custom seed ${namePrefix}${serviceShort}')
  properties: {
    roleDefinitionId: subscriptionResourceId(
      'Microsoft.Authorization/roleDefinitions',
      'acdd72a7-3385-48ef-bd42-f606fba81ae7' // 'Reader' role
    )
    principalId: nestedDependencies.outputs.managedIdentityPrincipalId
    principalType: 'ServicePrincipal'
  }
}

// ============== //
// Test Execution //
// ============== //

@batchSize(1)
module testDeployment '../../../main.bicep' = [
  for iteration in ['init', 'idem']: {
    scope: resourceGroup
    name: '${uniqueString(deployment().name, resourceLocation)}-test-${serviceShort}-${iteration}'
    params: {
      name: '${namePrefix}${serviceShort}001'
      location: resourceLocation
      managedIdentities: {
        userAssignedResourceIds: [
          nestedDependencies.outputs.managedIdentityResourceId
        ]
      }
      alertDescription: 'My sample Alert'
      autoMitigate: false
      criterias: {
        allOf: [
          {
            query: 'arg("").PolicyResources\n| where type =~ \'Microsoft.PolicyInsights/PolicyStates\'\n'
            timeAggregation: 'Count'
            dimensions: []
            operator: 'GreaterThan'
            threshold: json('10')
            failingPeriods: {
              numberOfEvaluationPeriods: 1
              minFailingPeriodsToAlert: 1
            }
          }
        ]
      }
      evaluationFrequency: 'PT5M'
      queryTimeRange: 'PT5M'
      alertDisplayName: '${uniqueString(deployment().name, resourceLocation)}-displayNameTest-${serviceShort}-${iteration}'
      scopes: [
        subscription().id
      ]
      suppressForMinutes: 'PT5M'
      windowSize: 'PT5M'
    }
    dependsOn: [
      managedIdentityRoleAssignment
    ]
  }
]

I noticed from your test case you aren't granting permissions for the user identity to the workspace.

Based on the doco:

"If the query is accessing a Log Analytics workspace, the identity must be assigned a reader role for all workspaces that the query accesses. If you're creating resource-centric log search alerts, the alert rule might access multiple workspaces, and the identity must have a reader role on all of them."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll be damned, you're of course right @ahmadabdalla. After you created this nice little trap with the added support for MSIs you also solved it for me 😄.
Had a hunch that it might be because of the LAW permissions, but it failed so fast that it seemed even more fundamental.

Copy link
Contributor

Choose a reason for hiding this comment

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

Most importantly.. did it work :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Core Team 🧞 This item needs the AVM Core Team to review it Needs: Triage 🔍 Maintainers need to triage still Status: Module Orphaned 👀 The module has no owner and is therefore orphaned at this time Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AVM Module Issue]: Add customProperties to avm/res/insights/scheduled-query-rule
2 participants