-
Notifications
You must be signed in to change notification settings - Fork 43
Add CoPP (Control Plane Policing) modules #139
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
base: main
Are you sure you want to change the base?
Conversation
A `aci-access-spine-copp-policy` module A `aci-copp-interface-policy` module M aci-access-leaf-interface-policy-group M aci-access-leaf-switch-policy-group M aci-access-spine-switch-policy-group M `.pre-commit-config.yaml` M `aci_access_policies.tf` M `defaults.yaml` M `modules.yaml`
|
@juchowan Can we assign to another reviewer? |
| args: ["./modules/terraform-aci-coop-policy"] | ||
| - id: terraform-docs-system | ||
| args: ["./modules/terraform-aci-coop-policy/examples/complete"] | ||
| - id: terraform-docs-system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is duplicated
| dhcp_relay_policies: | ||
| name_suffix: "" | ||
| switch_policies: | ||
| copp_leaf: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaf_copp_policies
| tor_glean_burst: default | ||
| traceroute_rate: default | ||
| traceroute_burst: default | ||
| copp_spine: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spine_copp_policies
| module "aci_access_spine_copp_policy" { | ||
| source = "./modules/terraform-aci-access-spine-copp-policy" | ||
|
|
||
| for_each = { for pol in try(local.access_policies.spine_copp_policies, []) : pol.name => pol if local.modules.aci_access_spine_copp_policy && var.manage_access_policies } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is looping incorrectly, should be over local.access_policies.switch_policies.spine_copp_policies
| module "aci_access_leaf_copp_policy" { | ||
| source = "./modules/terraform-aci-access-leaf-copp-policy" | ||
|
|
||
| for_each = { for pol in try(local.access_policies.leaf_copp_policies, []) : pol.name => pol if local.modules.aci_access_leaf_copp_policy && var.manage_access_policies } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is looping incorrectly, should be over local.access_policies.switch_policies.leaf_copp_policies
| type = var.type | ||
| } | ||
|
|
||
| dynamic "child" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we doing it with dynamic block instead of another resource?
| } | ||
| } | ||
|
|
||
| variable "custom_values" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
each of them should be a separate variable with its own validation or a type object
| } | ||
|
|
||
| resource "aci_rest_managed" "infraRsCoppIfPol" { | ||
| count = var.type != "breakout" ? 1 : 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this resource should be configured only if that policy is configured, to avoid creating empty resources
| } | ||
|
|
||
| resource "aci_rest_managed" "infraRsLeafCoppProfile" { | ||
| dn = "${aci_rest_managed.infraAccNodePGrp.dn}/rsleafCoppProfile" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing condition that would create it only if policy is configured
| } | ||
|
|
||
| resource "aci_rest_managed" "infraRsSpineCoppProfile" { | ||
| dn = "${aci_rest_managed.infraSpineAccNodePGrp.dn}/rsspineCoppProfile" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing condition
A
aci-access-leaf-copp-policymoduleA
aci-access-spine-copp-policymoduleA
aci-copp-interface-policymoduleM aci-access-leaf-interface-policy-group
M aci-access-leaf-switch-policy-group
M aci-access-spine-switch-policy-group
M
.pre-commit-config.yamlM
aci_access_policies.tfM
defaults.yamlM
modules.yaml