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

are lists not lists but ordered sets? #243

Open
cmohorea opened this issue May 13, 2024 · 9 comments
Open

are lists not lists but ordered sets? #243

cmohorea opened this issue May 13, 2024 · 9 comments
Labels
bug Something isn't working

Comments

@cmohorea
Copy link

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 cmohorea changed the title list order matters! are lists not lists but ordered sets? May 13, 2024
@cmohorea
Copy link
Author

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
Copy link
Member

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.

@danischm danischm added the bug Something isn't working label May 13, 2024
@cmohorea
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Member

Change type of controller_group_list: 6dea6ff

@cmohorea
Copy link
Author

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
Copy link
Author

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants