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

**BREAKING CHANGE** to CT resource schema: Change primitives from set to map #998

Conversation

chrismarget-j
Copy link
Collaborator

@chrismarget-j chrismarget-j commented Dec 17, 2024

Problem Statement

Connectivity Templates are implemented as trees of ep-endpoint-policy nodes. Our previous implementation of CT resources persisted the ID of the root node (the specific ep-endpoint-policy node which represents the whole CT) through edits, but did not persist the IDs of branch and leaf nodes within the tree. Edits to an existing CT modified IDs deep inside the tree. These changes had adverse effects to applied CTs which required follow-on modifications like configuring IP addresses on point-to-point logical links.

Schema Change

CT primitives used to be stored as sets, each with a name attribute. Now they are maps, keyed by the primitive name.

Old Style

resource "apstra_datacenter_connectivity_template_loopback" "example" {
  blueprint_id = local.blueprint_id
  name         = "old style"
  bgp_peering_ip_endpoints = [
    {
      name         = "neighbor_a" # <==== primitive name is an attribute
      neighbor_asn = null
      bfd_enabled  = true
      ipv4_address = "192.168.10.10"
    },
  ]
}

New Style

resource "apstra_datacenter_connectivity_template_loopback" "example" {
  blueprint_id = local.blueprint_id
  name         = "new style"
  bgp_peering_ip_endpoints = {
    neighbor_a = { # <=================== primitive name is the map key
      neighbor_asn = null
      bfd_enabled  = true
      ipv4_address = "192.168.10.10"
    },
  }
}

Details

Preventing ID churn deep within the tree of CT primitives requires us to keep track of the IDs of those primitives. In the old style config, primitives were members of sets, and those sets could nest a few layers deep.

Because set elements are dereferenced by their value, we can't put Computed attributes (the IDs) into an object which represents a set element. Adding the ID would change the value, destroying the old element, and creating a new one. Terraform would complain.

The new scheme solves that problem by keeping the primitives as (nested) maps. Maps elements are referenced by their key, so Computed values do not cause a problem.

We now keep the pipeline (upstream), policy, and batch (downstream) IDs for each primitive, and re-use these values during Update().

Summary of impacted files

It's a big PR. 31 files are modified here. They are:

  • apstra/resource_connectivity_template_<type>.go - 4 files - these are the top-level resource files. The only change here is that Request() and Update() now invoke plan.LoadPrimitiveIds(), which is a new method on the resource model struct. We do this because (a) IDs are generated locally on the client side (a quirk of the CT API), and (b) they're nested deep within the request struct, so teasing them out warranted a dedicated method.
  • apstra/resource_connectivity_template_<type>_integration_test.go - 4 files - these are the top-level resource test files. The changes here revolve around generating acceptance test checks and the change from set to map.
  • apstra/blueprint/connectivity_templates/connectivity_template_<type>.go - 4 files - These are the resource model struct files. The changes here revolve around the set/map distinction and the introduction of the LoadPrimitiveIds() method.
  • apstra/blueprint/connectivity_templates/primitves/<type>.go - 10 files - These files represent the 10 primitive types which make up the branches and leaves of the CT node tree. The changes in here were driven by the map/set schema change, along with the introduction of 3 IDs to the schema for each primitive. Those IDs represent the primitive data, the pipeline (upstream) node, and the batch (downstream) node. Each file has a new function LoadIDsInto<type>Map(), which is called by the parent LoadPrimitiveIds() method.
  • examples/ and docs/ - 8 files - pretty clear cut
  • apstra/resource_datacenter_connectivity_template_primitives_test.go - 1 file - This file contains test helper methods for each of the 10 primitives. These helpers are used to generate random configurations during acceptance testing. This thing is probably the biggest diff, because so much stuff got restructured.

@chrismarget-j chrismarget-j linked an issue Dec 17, 2024 that may be closed by this pull request
@chrismarget-j chrismarget-j changed the title WIP: 993 changing ct primitive elements has unintended downstream effects **BREAKING CHANGE** CT resource schema - change primitives from set to map Dec 19, 2024
@chrismarget-j chrismarget-j changed the title **BREAKING CHANGE** CT resource schema - change primitives from set to map **BREAKING CHANGE** to CT resource schema: Change primitives from set to map Dec 19, 2024
@chrismarget-j
Copy link
Collaborator Author

Closes #977

@chrismarget-j chrismarget-j merged commit 0c973ca into main Dec 21, 2024
1 check passed
@chrismarget-j chrismarget-j deleted the 993-changing-ct-primitive-elements-has-unintended-downstream-effects branch December 21, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing CT primitive elements has unintended downstream effects
2 participants