netascode / terraform-aci-nac-aci

Terraform Cisco ACI Nexus-as-Code Module
https://registry.terraform.io/modules/netascode/nac-aci/aci
Apache License 2.0
21 stars 25 forks source link

Need enhancement for configure Fabric L2 MTU / Port MTU size #34

Closed guilinyan closed 11 months ago

guilinyan commented 1 year ago

Hello,

When testing netascode/nac-aci/aci v0.8.0 with below yaml file as input on APIC v4.2, simulator, default Fabric L2 MTU Policy / Port MTU size(bytes) will be updated between 9000 and 9216 every time after running terraform apply with the same input yaml file. That's because below yaml input will be applied by two modules -- terraform-aci-fabric-l2-mtu and terraform-aci-l2-mtu-policy and the two modules are all applied on default Fabric L2 MTU Policy / Port MTU size(bytes), but with different values.

apic:
  fabric_policies:
    l2_port_mtu: 9000
    l2_mtu_policies:
      - name: default
        port_mtu_size: 9216

If module aci_l2_mtu_policy is designed for customized L2 MTU Policy, how about excluding default as showed below?

Line 150 https://github.com/netascode/terraform-aci-nac-aci/blob/eda11599284526188037bfaecb6db17beeec7eca/aci_fabric_policies.tf from for_each = { for policy in try(local.fabric_policies.l2_mtu_policies, []) : policy.name => policy if local.modules.aci_l2_mtu_policy && var.manage_fabric_policies } to for_each = { for policy in try(local.fabric_policies.l2_mtu_policies, []) : policy.name => policy if local.modules.aci_l2_mtu_policy && var.manage_fabric_policies && policy.name != "default" }

image

image

danischm commented 1 year ago

I understand that this can be confusing, but I am not convinced it would be less confusing if we would silently ignore explicit configuration of the default policy using l2_mtu_policies. If one adheres to the general best practice of not modifying default policies it should not be an issue. We could add a semantic pre-change validation rule (iac-validate) to flag such an overlapping configuration.

guilinyan commented 1 year ago

Hi Daniel, thank you very much for your quick response and clarification. It's a good suggestion to add a semantic pre-change validation rule(iac-validate). I will try to add this validation rule. Module terraform-aci-fabric-l2-mtu only can be used to configure the default Fabric L2 MTU, whereas terraform-aci-l2-mtu-policy module can be used to configure both default and customized Fabric L2 MTU. The examples on https://netascode.cisco.com/data_model/apic/fabric_policies/l2_mtu is perfect. I don't know if we could add more description for module terraform-aci-fabric-l2-mtu.

andbyrne commented 1 year ago

Either l2_port_mtu in the schema and the terraform-aci-fabric-l2-mtu module could be deprecated or the terraform-aci-l2-mtu-policy module could have a validation rule on the name variable to disallow a value of default.

guilinyan commented 1 year ago

Hi Andrew, adding a validation rule on the name variable to disallow a value of default to terraform-aci-l2-mtu-policy module sounds great to me. thank you for the suggestions.