CiscoDevNet / terraform-provider-iosxr

Terraform Cisco IOS-XR Provider
https://registry.terraform.io/providers/CiscoDevNet/iosxr
Mozilla Public License 2.0
5 stars 9 forks source link

State matching faulty for ACLv4 #185

Closed summed closed 1 year ago

summed commented 1 year ago

When the "permit_destination_address" is set for a ACL (https://registry.terraform.io/providers/CiscoDevNet/iosxr/latest/docs/resources/ipv4_access_list), it will always try and update it with a "new" value. Seems like a minor bug with string matching proposed-config/change versus state.

danischm commented 1 year ago

Thanks for opening the issue! Would you mind sharing an example value of permit_destination_address that would trigger the mentioned behavior?

summed commented 1 year ago

No problem. Although this isn't the exact same thing as the issue originally mention, I would believe they are related.

I'll investigate a little more to see if I can find a pattern.

the example below always asks to set the 'permit_protocol: "ipv4"'

resource "iosxr_ipv4_access_list" "aclv4-issue" {
    access_list_name = "issue-185-test"
    sequences = [
        {
            permit_protocol: "ipv4",
            sequence_number: 10
        }
    ]
}
summed commented 1 year ago

These rules

resource "iosxr_ipv4_access_list" "aclv4-issue" {
    access_list_name = "issue-185-test"
    sequences = [
        {
            permit_protocol: "ipv4",
            sequence_number: 10
        }
    ]
}

resource "iosxr_ipv4_access_list" "aclv4-issue-2" {
    access_list_name = "issue-185-test-2"
    sequences = [
        {
            sequence_number: 10
            permit_destination_any: true
            permit_protocol: "ipv4"
            permit_source_host: "1.1.1.1"
        },
        {
            sequence_number: 15
            permit_destination_any: true
            permit_protocol: "ipv4"
            permit_source_host: "2.2.2.2"
        },
        {
            sequence_number: 20
            deny_protocol: "ipv4"
            deny_source_any: true
            permit_destination_any: true
        }
    ]
}

result in

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # module.poi-lon.module.router.iosxr_ipv4_access_list.aclv4-issue-2 will be updated in-place
  ~ resource "iosxr_ipv4_access_list" "aclv4-issue-2" {
        id               = "Cisco-IOS-XR-um-ipv4-access-list-cfg:/ipv4/access-lists/access-list[access-list-name=issue-185-test-2]"
      ~ sequences        = [
          ~ {
              ~ permit_destination_any = false -> true
                # (3 unchanged attributes hidden)
            },
            # (2 unchanged elements hidden)
        ]
        # (1 unchanged attribute hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

There seems to be some conflict happening if I have other rules, and the acl updates become more erratic.

danischm commented 1 year ago

You cannot have permit and deny statements in the same entry (sequence).

summed commented 1 year ago

Hi Daniel. Sorry for the delay response on this. You are ofcourse completly correct.

Could/would it be a good idea of organising the resource input objects in a way that catches/disallows this?

for example

resource "iosxr_ipv4_access_list" "aclv4-issue" {
  access_list_name = "issue-185-test-rule"
  sequences = [
    {
       seq: 10
       deny: {
          protocol: "ipv4"
          ...
       }
    }, ...
  ]
}

or...

resource "iosxr_ipv4_access_list" "aclv4-issue" {
  access_list_name = "issue-185-test-rule"
  sequences = {
    10: {              # <-- (unique) Sequence number
       action: deny
       protocol: "ipv4"
       ...
    }, ...
  }
}

I'll do a little cleanup in our rules and see if that doesnt fix the problems mention above...

danischm commented 1 year ago

The structure of the resources is aligned with the unified YANG models of XR and we follow a convention of flattening nested containers/dictionaries. This is consistent across all resources and allows us to auto-generate all the code, tests and documentation. Therefore I would like to avoid deviating from these conventions, but you can of course create a Terraform module on top, that implements your own preferred structure/model.

summed commented 1 year ago

Yeah, I had a hunch something like that was the case. Thanks for the info.

I'll try and follow up on this ticket once get a little further with testing of our resources.

danischm commented 1 year ago

Can we close this one?

summed commented 1 year ago

Yes, it seems to be working well. I'll let you know if I run into anything. Thanks a bunch!