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

Address Book Terraform Plan Output Improvement #316

Closed
tagur87 opened this issue Dec 1, 2021 · 4 comments · Fixed by #319
Closed

Address Book Terraform Plan Output Improvement #316

tagur87 opened this issue Dec 1, 2021 · 4 comments · Fixed by #319
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@tagur87
Copy link
Contributor

tagur87 commented Dec 1, 2021

We have been experiencing issues with large plan diffs, especially when adding a new address to the global address book. We have found that the address book operates more list an ordred list, when in all actuality, it should operate more as an unordered set.

Terraform Version: v1.0.11
Junos Device: vSRX

Error/Issue:

After applying the configuration once, then moving the order of the objects yields the following plan diff.

   # junos_security_address_book.global will be updated in-place
   ~ resource "junos_security_address_book" "global" {
         id              = "global"
         name        = "global"
         # (1 unchanged attribute hidden)
 
       ~ network_address {
           ~ name  = "host.local.firewall_primary" -> "host.local.firewall_cluster"
           ~ value = "192.168.1.2/32" -> "10.1.1.1/32"
         }
       ~ network_address {
           ~ name  = "host.local.firewall_primary-v6" -> "host.local.firewall_cluster-v6"
           ~ value = "1028:f123::2/128" -> "2601:fff2::1/128"
         }
       ~ network_address {
           ~ name  = "host.local.firewall_primary_deployment" -> "host.local.firewall_cluster_deployment"
           ~ value = "192.168.1.3/32" -> "192.168.1.1/32"
         }
       ~ network_address {
           ~ name  = "host.local.firewall_primary_deployment-v6" -> "host.local.firewall_cluster_deployment-v6"
           ~ value = "1028:f123::3/128" -> "1028:f123::1/128"
         }
       ~ network_address {
           ~ name  = "host.local.firewall_secondary" -> "host.local.firewall_primary"
           ~ value = "192.168.1.4/32" -> "192.168.1.2/32"
         }
       ~ network_address {
           ~ name  = "host.local.firewall_secondary-v6" -> "host.local.firewall_primary-v6"
           ~ value = "1028:f123::4/128" -> "1028:f123::2/128"
         }
       ~ network_address {
           ~ name  = "host.local.firewall_cluster" -> "host.local.firewall_primary_deployment"
           ~ value = "10.1.1.1/32" -> "192.168.1.3/32"
         }
       ~ network_address {
           ~ name  = "host.local.firewall_cluster-v6" -> "host.local.firewall_primary_deployment-v6"
           ~ value = "2601:fff2::1/128" -> "1028:f123::3/128"
         }
       ~ network_address {
           ~ name  = "host.local.firewall_cluster_deployment" -> "host.local.firewall_secondary"
           ~ value = "192.168.1.1/32" -> "192.168.1.4/32"
         }
       ~ network_address {
           ~ name  = "host.local.firewall_cluster_deployment-v6" -> "host.local.firewall_secondary-v6"
           ~ value = "1028:f123::1/128" -> "1028:f123::4/128"
         }
     }

I have tested using TypeSet instead of TypeList for the network_address, and it appears to work well as a drop in replacement that resolves this issue.

I propose changing these types of objects to sets, which will help make the plan output more accurate.

@tagur87
Copy link
Contributor Author

tagur87 commented Dec 1, 2021

I can work on this if you agree to this change.

@tagur87
Copy link
Contributor Author

tagur87 commented Dec 1, 2021

After further investigation, it seems this isn't quite a drop in replacement, I will continue to investigate other solutions.

@jeremmfr
Copy link
Owner

jeremmfr commented Dec 6, 2021

This problem had already been discussed in issue #169 for the address book of zones.
Ordering addresses is unnecessary for Junos, but the update plan output with "Block Set" (unordered) is not better in most cases.

For example, if I active blue address and add it in collorall in this resource :

resource "junos_security_address_book" "testman_secglobpolicy" {
  network_address {
    name  = "red"
    value = "192.0.2.3/32"
  }
  network_address {
    name  = "green"
    value = "192.0.2.3/32"
  }
  # network_address {
  #  name  = "blue"
  #  value = "192.0.2.1/32"
  # }
  address_set {
    name    = "colorall"
    address = ["green", "red"]
    # address = ["green", "red", "blue"]
  }
}

With "Block List", the output is :

  # junos_security_address_book.testman_secglobpolicy will be updated in-place
  ~ resource "junos_security_address_book" "testman_secglobpolicy" {
        id          = "global"
        name        = "global"
        # (1 unchanged attribute hidden)

      ~ address_set {
          ~ address     = [
              + "blue",
                # (2 unchanged elements hidden)
            ]
            name        = "colorall"
            # (1 unchanged attribute hidden)
        }

      + network_address {
          + name  = "blue"
          + value = "192.0.2.1/32"
        }
        # (2 unchanged blocks hidden)
    }

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

But with "Block Set", the output is :

  # junos_security_address_book.testman_secglobpolicy will be updated in-place
  ~ resource "junos_security_address_book" "testman_secglobpolicy" {
        id          = "global"
        name        = "global"
        # (1 unchanged attribute hidden)

      + address_set {
          + address     = [
              + "blue",
              + "green",
              + "red",
            ]
          + address_set = []
          + name        = "colorall"
        }
      - address_set {
          - address     = [
              - "green",
              - "red",
            ] -> null
          - address_set = [] -> null
          - name        = "colorall" -> null
        }

      + network_address {
          + name  = "blue"
          + value = "192.0.2.1/32"
        }
      - network_address {
          - name  = "green" -> null
          - value = "192.0.2.3/32" -> null
        }
      - network_address {
          - name  = "red" -> null
          - value = "192.0.2.3/32" -> null
        }
      + network_address {
          + name  = "green"
          + value = "192.0.2.3/32"
        }
      + network_address {
          + name  = "red"
          + value = "192.0.2.3/32"
        }
    }

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

After investigation, I think there is a bug in Terraform (core or sdk) that detects a change between empty values in the state and undefined optional arguments in the configuration.

Customize hash of block with SchemaSetFunc doesn't work to avoid this bug. We will probably have to wait for the new plugin terraform-plugin-framework to resolve this bug.

But if I define empty value on all optional arguments not used of blocks in the configuration, unchanged blocks are correctly hidden.

For example, I add the optional argument description on all network_address :

resource "junos_security_address_book" "testman_secglobpolicy" {
  network_address {
    name        = "red"
    value       = "192.0.2.3/32"
    description = ""
  }
  network_address {
    name        = "green"
    value       = "192.0.2.3/32"
    description = ""
  }
  network_address {
    name        = "blue"
    value       = "192.0.2.1/32"
    description = ""
  }
  address_set {
    name = "colorall"
    #address = ["green", "red"]
    address = ["green", "red", "blue"]
  }
}

I have this better output :

  # junos_security_address_book.testman_secglobpolicy will be updated in-place
  ~ resource "junos_security_address_book" "testman_secglobpolicy" {
        id          = "global"
        name        = "global"
        # (1 unchanged attribute hidden)

      + address_set {
          + address     = [
              + "blue",
              + "green",
              + "red",
            ]
          + address_set = []
          + name        = "colorall"
        }
      - address_set {
          - address     = [
              - "green",
              - "red",
            ] -> null
          - address_set = [] -> null
          - name        = "colorall" -> null
        }

      + network_address {
          + name  = "blue"
          + value = "192.0.2.1/32"
        }
        # (2 unchanged blocks hidden)
    }

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

So finally I will change my mind, I'll take care of switching blocks to unordered (Block Set) for address-book global and in zones, and add a note in documentation to encourage to set empty values on optional unused arguments to avoid large update plan output with unchanged blocks not correctly hidden.

@jeremmfr jeremmfr self-assigned this Dec 6, 2021
@jeremmfr jeremmfr added the enhancement New feature or request label Dec 6, 2021
jeremmfr added a commit that referenced this issue Dec 6, 2021
The outputs of the update plan will be different, see the note in the documentation to prevent unchanged blocks not being correctly hidden.
Fix #316
@jeremmfr
Copy link
Owner

jeremmfr commented Dec 7, 2021

Finally, after some tests, I found another solution to avoid large outputs on the update plan.
If I directly define a default empty value for optional primitives arguments like description in provider, the output is correct and there is no more need to add the optional arguments in the hcl code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants