CiscoDevNet / terraform-provider-sdwan

Terraform Cisco SD-WAN Provider
https://registry.terraform.io/providers/CiscoDevNet/sdwan
Mozilla Public License 2.0
17 stars 11 forks source link

are lists not lists but ordered sets? #243

Closed cmohorea closed 3 weeks ago

cmohorea commented 2 months ago

Symptom: in sdwan_cisco_system_feature_template resource, change

  controller_group_list = [1,2]
  controller_group_list = [2,1]

and the reported outcome is "No changes. Your infrastructure matches the configuration."

However, the order is important as router walks them sequentially, so it's a different configuration

cmohorea commented 2 months ago

Some additional digging: controller_group_list = [4,3,2,1], controller_group_list = [1,2,3,4], controller_group_list = [1,3,2,4,3,4,2,1,2,3] are all translated into the same "1,2,3,4" order in the GUI. This is completely wrong, as, when configuring vSmart affinity, you list all the groups (for the redundancy) but indicate preference with the order.

I think I saw similar behavior when I was doing import of a feature template where same device model was listed twice (possible when playing with APIs!) and terraform complained that it was an incorrect set (while it was a list)

danischm commented 2 months ago

controller_group_list is indeed modelled as a "Set" which is also shown in the documentation (https://registry.terraform.io/providers/CiscoDevNet/sdwan/latest/docs/resources/cisco_system_feature_template#controller_group_list). This is incorrect as you pointed out and needs to be fixed in the provider. Unfortunately, the schema does not provide any hint if a "List" or "Set" is the appropriate type, therefore we use a "Set" by default in case of primitive values.

cmohorea commented 2 months ago

I might be making a very global statement... but based on YANG model, I don't believe that anything in schema is a "set". I guess that maybe in some cases where certain restrictions exists, data may be better modelled with sets, but generally I'd say that default approach should be to use "list" type, otherwise you'll be running into similar cases in the future .

I'd be curious to see an example where SD-WAN data structure is actually is a set, not a list (maybe I am wrong? :) )

danischm commented 2 months ago

Unfortunately, there are no YANG models available for this. I was referring to this schema for example: https://github.com/CiscoDevNet/terraform-provider-sdwan/blob/main/gen/models/feature_templates/cisco_system.json

The device (type) list for example is a "set" as the order of device types is not relevant. In general, having a "list" where the order is not significant can cause issues as well, like for example if the GUI "reorders" the elements TF will try to reconcile it, whereas it is semantically the same. So it needs to be determined on a case by case basis. Anyway, if you find other instances where it should be a "list", please let us know and we will fix it.

cmohorea commented 2 months ago

I'm glad you used the 'The device (type) list for example is a "set"' example, as it is definitely not! vManage gladly accepts a template definition (created via API) with same device listed several times: image It even has nothing against this when you go to edit it in GUI (although it did remove dups when it saved it, 20.12 code): image

And while vManage is able to handle it, this is what's happening in terraform:

$ terraform import sdwan_cli_template_feature_template.CLI_TEST 30be6016-554a-4c3b-87e3-602928514dbd
sdwan_cli_template_feature_template.CLI_TEST: Importing from ID "30be6016-554a-4c3b-87e3-602928514dbd"...
sdwan_cli_template_feature_template.CLI_TEST: Import prepared!
  Prepared sdwan_cli_template_feature_template for import
sdwan_cli_template_feature_template.CLI_TEST: Refreshing state... [id=30be6016-554a-4c3b-87e3-602928514dbd]
╷
│ Error: Duplicate Set Element
│
│ This attribute contains duplicate values of: tftypes.String<"vedge-C8000V">
╵
danischm commented 2 months ago

It is not necessarily about duplicates, which even though the API accepts them, they do not make sense in the context of device types. The important part is, the order of elements is of no significance, therefore if the backend/UI changes the order of elements and we would be using a "list" type, TF would continuously detect this as a change, trying to reorder them, even though it is semantically the same configuration.

danischm commented 2 months ago

Change type of controller_group_list: https://github.com/CiscoDevNet/terraform-provider-sdwan/commit/6dea6ff65feb4806b9a6afad584cda0a2d4eeb35

cmohorea commented 2 months ago

I understand your approach but, again, it leads to mismatches (like above, TF fails with error where vManage is handling it fine). I'm not going to run in circles arguing about this :) But my prediction is that you'll run into this again :)

cmohorea commented 2 months ago

In terms of other examples, right away, [affinity_group_preference] under sdwan_cisco_system_feature_template is also a "Set" while it should be an ordered list (order matter)

danischm commented 1 month ago

affinity_group_preference attribute fix: https://github.com/CiscoDevNet/terraform-provider-sdwan/commit/79ac2635775db824628897758625a694d3cd6573

danischm commented 3 weeks ago

Mentioned types are fixed in v0.3.10. If there are any other inconsistencies please raise a new issue. Thanks!