'fp.monitoring' fixed and expanded (Issue623) by jmiguelhp · Pull Request #267 · netascode/terraform-aci-nac-aci

jmiguelhp · 2025-07-17T04:24:09Z

Current Fabric Monitoring Data Model only accept the configuration of name. Support for destination_group was added and support of user-defined fabric monitoring policies.

These are non-breaking changes.

Data tested:

---
apic:
  fabric_policies:
    monitoring:
      snmp_traps:
        - name: trap1
          destination_group: mon_snmp1_dst_grp
        - name: trap2
      syslogs:
        - name: syslog1
          description: desc1
          session: true
          minimum_severity: debugging
          admin_state: false
          format: nxos
          show_millisecond: false
          show_timezone: false
          local_admin_state: false
          local_severity: emergencies
          console_admin_state: false
          console_severity: alerts
          destination_group: mon_syslog1_dst_grp
        - name: syslog2
      policies:
        - name: Custom_Policy1
          description: This is a custom policy for policy monitoring.
          fault_severity_policies:
            - class: snmpClient
              faults:
                - fault_id: F1368
                  description: "Fault 1368 nice description"
                  initial_severity: critical
                  target_severity: inherit
            - class: snmpConfIssues
            - class: snmpTrapDest
              faults:
                - fault_id: F1450
                  description: "Fault 1450 cool description"
                - fault_id: F1449
                  description: "Fault 1449 superb description"
                  initial_severity: minor
                  target_severity: critical
          snmp_traps:
            - name: policy_trap1
              destination_group: pol_snmp_dst_grp
            - name: policy_trap2
          syslogs:
            - name: policy_syslog1
              description: desc1
              session: true
              minimum_severity: debugging
              admin_state: false
              format: nxos
              show_millisecond: false
              show_timezone: false
              local_admin_state: false
              local_severity: emergencies
              console_admin_state: false
              console_severity: alerts
              destination_group: pol_syslog_dst_grp
            - name: policy_syslog2
        - name: Custom_Policy2

ogorczow · 2025-07-23T12:14:59Z

I guess this example data is vague?

          syslogs:
            - name: policy_syslog1
              description: desc1
              session: true
              minimum_severity: debugging
              admin_state: false
              format: nxos
              show_millisecond: false
              show_timezone: false
              local_admin_state: false
              local_severity: emergencies
              console_admin_state: false
              console_severity: alerts
              destination_group: pol_syslog_dst_grp

ogorczow · 2025-07-23T11:56:26Z

@@ -854,15 +854,19 @@ module "aci_syslog_policy" {
 module "aci_monitoring_policy" {


maybe lets call it aci_monitoring_policy_common and 2nd module just aci_monitoring_policy?

and lets add there fault severity policies (for common object), to keep all modules supporting the same

by the way, i guess there is not an option to configure new syslog destination or snmp destination without adding it to common object?

maybe lets call it aci_monitoring_policy_common and 2nd module just aci_monitoring_policy?

We can check this with Justyna. I tried to keep it the way it is in order to ensure backwards compatibility.

and lets add there fault severity policies (for common object), to keep all modules supporting the same

As per my tests, common policy does not allow changes in Fault severities.

by the way, i guess there is not an option to configure new syslog destination or snmp destination without adding it to common object?

Not sure about this one. Hope you can elaborate it

ogorczow · 2025-07-25T09:57:53Z

If we analyze common policy, i guess that there was 1 more broken thing:

---
apic:
  fabric_policies:
    monitoring:
      snmp_traps:
        - name: trap1
          destination_group: mon_snmp1_dst_grp

If we consider that we create an SNMP destination named "trap1" and also it automatically creates under monitoring common policy reference, in our case it will generate reference to 'mon_snmp1_dst_group` which doesn't exists.

Wouldn't it be there that we need to keep destination_group and name always the same to make it working? I guess maybe we should force users to keep it the same?

destination_group would have to include name_suffix as well

jmiguelhp · 2025-07-25T15:43:10Z

If we analyze common policy, i guess that there was 1 more broken thing:
---
apic:
  fabric_policies:
    monitoring:
      snmp_traps:
        - name: trap1
          destination_group: mon_snmp1_dst_grp
If we consider that we create an SNMP destination named "trap1" and also it automatically creates under monitoring common policy reference, in our case it will generate reference to 'mon_snmp1_dst_group` which doesn't exists.

Wouldn't it be there that we need to keep destination_group and name always the same to make it working? I guess maybe we should force users to keep it the same?

destination_group would have to include name_suffix as well

User should be in charge of make sure references are pointing to valid objects (e.g. when you define a BD under EPG, tool does not validate if the BD exists and no error is popped up). Also, an internal issue was raised where the engineers are not using the same name for both objetcs, instead, they create the SNMP destinations independently.

Let's meet and discuss this further.

Thanks!

ogorczow · 2025-07-28T10:44:37Z

+    faults            = try(policy.faults, local.defaults.apic.fabric_policies.monitoring.syslogs.faults)
+    session           = try(policy.session, local.defaults.apic.fabric_policies.monitoring.syslogs.session)
+    minimum_severity  = try(policy.minimum_severity, local.defaults.apic.fabric_policies.monitoring.syslogs.minimum_severity)
+    destination_group = try("${policy.destination_group}${local.defaults.apic.fabric_policies.monitoring.syslogs.destination_group_suffix}", "")


this should read from:

--- defaults: apic: fabric_policies: monitoring: syslogs: name_suffix: ""

ogorczow · 2025-07-28T10:45:10Z

+  count = local.modules.aci_monitoring_policy == true && var.manage_fabric_policies ? 1 : 0
+  snmp_trap_policies = [for policy in try(local.fabric_policies.monitoring.snmp_traps, []) : {
+    name              = "${policy.name}${local.defaults.apic.fabric_policies.monitoring.snmp_traps.name_suffix}"
+    destination_group = try("${policy.destination_group}${local.defaults.apic.fabric_policies.monitoring.snmp_traps.destination_group_suffix}", "")


this should read from:

--- defaults: apic: fabric_policies: monitoring: snmp_traps: name_suffix: ""

there is no need to create new destination_group_suffix

jmiguelhp · 2025-11-14T15:16:44Z

This PR has been updated to be fully backwards compatible: current data models and current terraform modules remain exactly the same

It proposes a new and independent Terraform module to handle User defined Fabric monitoring policies that does not affect current assets in any way.

New module was tested in a whole and isolated way.

ogorczow

all ok.
The only thing to discuss is if we want to keep module name this way "aci_monitoring_policy_user_defined"

ogorczow

1 major change
2 minor changes

ogorczow · 2026-01-23T13:34:43Z

-  aci_monitoring_policy: true
+  aci_monitoring_policy_common: true
+  aci_monitoring_policy_custom: true
+  aci_access_monitoring_policy: true


duplicated aci_access_monitoring_policy: true in code:

│ Call to function "provider::utils::yaml_merge" failed: Error reading YAML string: yaml: unmarshal │ errors: │ line 120: mapping key "aci_access_monitoring_policy" already defined at line 12.

ogorczow · 2026-01-23T13:48:27Z

+  }
+}
+
+resource "aci_rest_managed" "snmpRsDestGroup" {


if you try to configure custom policy without destination_group, then terraform tries to create resource with empty tDn snmpgroup name:

# module.aci.module.aci_monitoring_policy_custom["NEW_POLICY"].aci_rest_managed.snmpRsDestGroup["trap2"] will be created + resource "aci_rest_managed" "snmpRsDestGroup" { + annotation = "orchestrator:terraform" + class_name = "snmpRsDestGroup" + content = { + "tDn" = "uni/fabric/snmpgroup-" } + dn = "uni/fabric/monfab-NEW_POLICY/snmpsrc-trap2/rsdestGroup" + escape_html = true + id = (known after apply) }

common one works fine

policies: - name: NEW_POLICY description: This is a new policy snmp_traps: - name: trap2

ogorczow · 2026-01-23T13:50:25Z

+}
+
+resource "aci_rest_managed" "syslogRsDestGroup" {
+  for_each   = { for s in var.syslog_policies : s.name => s if s.destination_group != null }


if you try to configure custom policy without destination_group, then terraform tries to create resource with empty tDn slgroup name:

# module.aci.module.aci_monitoring_policy_custom["NEW_POLICY"].aci_rest_managed.syslogRsDestGroup["syslogX"] will be created + resource "aci_rest_managed" "syslogRsDestGroup" { + annotation = "orchestrator:terraform" + class_name = "syslogRsDestGroup" + content = { + "tDn" = "uni/fabric/slgroup-" } + dn = "uni/fabric/monfab-NEW_POLICY/slsrc-syslogX/rsdestGroup" + escape_html = true + id = (known after apply) }

common one works fine

policies: - name: NEW_POLICY description: This is a new policy syslogs: - name: syslogX

jmiguelhp added 3 commits July 16, 2025 21:00

'fp.moniroring' fixed and expanded (Issue623)

1606bce

Fix typos

3d59937

Fix typos

7fca57a

ogorczow self-requested a review July 23, 2025 11:32

ogorczow requested changes Jul 23, 2025

View reviewed changes

jmiguelhp added 2 commits July 24, 2025 12:44

Updates as per Oskar comments.

d605f46

pre-commit run

cbc5f51

SNMP/Syslog destination suffix added.

bda1c13

ogorczow reviewed Jul 28, 2025

View reviewed changes

Merge branch 'main' into Issue623

c04265c

jmiguelhp marked this pull request as draft November 13, 2025 22:50

Revert changes in current modules for backwards compatibility

f6b7d78

jmiguelhp marked this pull request as ready for review November 14, 2025 15:16

jmiguelhp requested a review from ogorczow November 14, 2025 15:16

ogorczow approved these changes Nov 19, 2025

View reviewed changes

jmiguelhp and others added 3 commits December 2, 2025 08:46

Merge branch 'netascode:main' into Issue623

12264ea

updates

8c02888

Merge branch 'main' into pr/267

331502c

ogorczow self-requested a review January 23, 2026 13:04

ogorczow requested changes Jan 23, 2026

View reviewed changes

ogorczow mentioned this pull request Jan 26, 2026

Allow SNMP/Syslog source and dest group names to be different #342

Closed

updates

b34fbac

juchowan merged commit 9ea868d into netascode:main Mar 10, 2026
1 check passed

		@@ -854,15 +854,19 @@ module "aci_syslog_policy" {
		module "aci_monitoring_policy" {

Uh oh!

Conversation

jmiguelhp commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogorczow commented Jul 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ogorczow commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmiguelhp commented Jul 25, 2025

Uh oh!

ogorczow Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogorczow Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmiguelhp commented Nov 14, 2025

Uh oh!

ogorczow left a comment

Choose a reason for hiding this comment

Uh oh!

ogorczow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmiguelhp commented Jul 17, 2025 •

edited

Loading

ogorczow commented Jul 25, 2025 •

edited

Loading

ogorczow Jul 28, 2025 •

edited

Loading

ogorczow Jul 28, 2025 •

edited

Loading