add discovery and SWIM functionality #18

jabielecki · 2024-02-09T16:37:16Z

Changes brought:

Add catalystcenter_discovery resource and data source
Add catalystcenter_network_devices data source
Add catalystcenter_device_detail resource and data source
Add catalystcenter_image resource
Add catalystcenter_image_distribution resource
Add catalystcenter_image_activation resource

This pull request is quite large, and I would welcome a suggestion what would be ideal PR size from reviewer's perspective.

Use exclude_example for most discovery attribs Why the "timeout" is write_only: the POST needs "timeout", but GET annoyingly returns "timeOut".

The HTTP DELETE does not use the usual dna/intent/* prefix. Reason is that dna/intent/* returns 404 for DELETE, but without the prefix it works as expected on our lab.

Don't copy large file to log. First timeout: the POST itself, until CC receives the upload bytes. Second timeout on top of the first: the CC task execution.

Since API has no concept of unique id for an activation, we can compose one. Just take all attributes that define uniqueness, and concatenate them.

This reverts commit 0fa569d.

I've only included the attributes that are non-null on a couple of my test devices. The main motivation for this is to be able to filter devices based on their location (area/building/floor). There are basically two documented APIs for that: v1/device-detail and v1/membership. The reason for preferring the former here in this commit is that the latter is exceptionally slow. The v1/network-device, contrary to its documentation, does not send location or locationName data. It always sends nulls for these.

Reason: there is no harm in trying to adjust an empty subcategory, whether the golang code was auto-generated or manually specified.

It is confusing to users to show them any `id` attribute because this resource always selects all devices. In order to avoid gen/generator.go from adding `id` back I put the ds under manual maintenace. So now the only remaining contribution from gen/generator.go is partial templating of model.go, just because it will be useful for any future attribute work.

After the first apply, the CC API returns some of the strings with letter case changed. E.g. my POST contains type "RANGE" but the next GET returns "Range" instead. It means that every subsequent `terraform apply` would attempt to change the object. This fails the TF_ACC test and is also user-hostile.

Reason: The API PUT uses the same attribute specs as GET, which are both different than POST. While it can be implemented manually some day, it seems too quirky for the current gen/templates. Lack of PUT in practice requires both `no_update` and `skip_minimal_test`, otherwise the tests would always fail. Reason for no_import: the generator for now mistakenly uses name attribute instead of numerical id attribute.

gen/generator.go

danischm · 2024-02-09T17:19:52Z

gen/definitions/network_devices.yaml

+name: Network Devices
+rest_endpoint: /dna/intent/api/v1/network-device
+get_no_id: true
+no_data_source: true


I think we could still auto-generate the data source code and just replace specific sections of the generated code. Each rendered .go file has a number of sections (//template:begin and //template:end), where if you want to change a certain section you would make the necessary changes, then remove the begin/end template markers, which will prevent this specific section of code from being overwritten the next time you would run go generate.

In this particular place I had additional problem, which I couldn't solve with deleting //template:begin and //template:end. By default, the generated docs and examples of data source always tells users to use id attribute:

data "catalystcenter_network_devices" "example" { id = "76d24097-41c4-4558-a4d0-a8c07ac08470" }

But in this particular case, the id doesn't fit well. It is confusing to users to even show them an id attribute because this source always returns all devices, whereas any user would probably confuse that id with the device's UUID.

The no_data_source is probably the least invasive way to prevent gen/generator.go from adding id back to the docs and examples.

So now the only remaining contribution from gen/generator.go is partial templating of model.go, and I agree it will be useful for any future attribute additions.

A singleton data source implementation is potentially something we might require for more data sources in the future, so we could also consider enhancing the generator to support this. For now I am fine with keeping it like this and then revisiting it once we have more data sources requiring this.

Acknowledged. The phrase "singleton data source" brings back a bit of GoF memories :)

gen/definitions/image_activation.yaml

danischm · 2024-02-09T22:22:03Z

gen/definitions/device_detail.yaml

+no_delete: true
+no_import: true
+no_read: true
+doc_category: Devices


We already have a category Inventory, would it make sense to add it there?

done, thanks

danischm · 2024-02-09T22:23:22Z

gen/definitions/device_detail.yaml

+get_extra_query_params: '&identifier=uuid'
+ds_description: This data source fetches device's details, including the assigned physical location.
+no_resource: true
+no_update: true


If we are not adding a resource I think we can remove no_update, no_delete, no_import and no_read.

done, thanks

danischm · 2024-02-09T22:28:24Z

gen/definitions/discovery.yaml

+    requires_replace: true
+  - model_name: enablePasswordList
+    type: StringList
+    # description: Enable Password of the devices to be discovered.


Is there a reason the description is commented out?

In fact I wanted to comment out the entire attribute. I'm not sure about the semantics of these 3 lists. Should it be in fact a single list with nested attribs? Also I don't have a liberty to touch device credentials at all in my environment, so I cannot really play and see myself what is going on with these.

Suggestions?

According to the API documentation it is a list of strings, so this would be the right type to use. I would suggest to leave it in and uncomment the descriptions. We will test this with an internal instance.

done, I've added descriptions

danischm · 2024-02-09T22:28:39Z

gen/definitions/discovery.yaml

+    requires_replace: true
+  - model_name: passwordList
+    type: StringList
+    # description: Password of the devices to be discovered.


Is there a reason the description is commented out?

done per above

danischm · 2024-02-09T22:28:45Z

gen/definitions/discovery.yaml

+    requires_replace: true
+  - model_name: userNameList
+    type: StringList
+    # description: Username for the devices to be discovered.


Is there a reason the description is commented out?

done per above

danischm · 2024-02-09T22:46:46Z

gen/definitions/network_devices.yaml

+name: Network Devices
+rest_endpoint: /dna/intent/api/v1/network-device
+get_no_id: true
+no_data_source: true


A singleton data source implementation is potentially something we might require for more data sources in the future, so we could also consider enhancing the generator to support this. For now I am fine with keeping it like this and then revisiting it once we have more data sources requiring this.

gen/templates/resource.go

danischm · 2024-02-09T22:53:14Z

gen/templates/resource.go

@@ -410,7 +410,7 @@ func (r *{{camelCase .Name}}Resource) Create(ctx context.Context, req resource.C
 	{{- if .PutCreate}}
 	res, err := r.client.Put(plan.getPath() + params, body)


Let's maybe also add support for the max_async_wait_time parameter here.

done, thanks

danischm · 2024-02-09T22:53:28Z

gen/templates/resource.go

@@ -535,7 +535,7 @@ func (r *{{camelCase .Name}}Resource) Update(ctx context.Context, req resource.U
 	params += "?{{.PutIdQueryParam}}=" + plan.Id.ValueString()
 	{{- end}}
 	{{- if .PostUpdate}}
-	res, err := r.client.Post(plan.getPath() + params, body)
+	res, err := r.client.Post(plan.getPath() + params, body {{- if .MaxAsyncWaitTime }}, func(r *cc.Req) { r.MaxAsyncWaitTime={{.MaxAsyncWaitTime}} }{{end}})
 	{{- else if .PutCreate}}
 	res, err := r.client.Put(plan.getPath() + params, body)


done, thanks

danischm · 2024-02-09T23:01:24Z

internal/provider/data_source_catalystcenter_network_devices_test.go

+}
+
+func testAccDataSourceCcNetworkDevicesConfig() string {
+	config := `resource "catalystcenter_network_devices" "test" {` + "\n"


This would need to be updated as this resource does not exist. We would either need to add test_prerequisites to add a device first or assume a certain device already exists. We can keep it like this for now and I would then update it later to work with our CI environment.

With test_prerequisites, the test becomes quite weak. It amounts to a single value response.0.management_ip_address... All the other attributes need to be real (meaning I don't see how to POST/PUT them).

In addition to the catalystcenter_network_devices, the more acute case is catalystcenter_device_detail. The latter only seems to be working at all if the device is in managed state.

Suggestions?

Do I lock these tests behind os.Getenv("INVENTORY") as currently used in catalystcenter_device_role? Do they belong approximately to the same boat?

Yes, let's add a test tag for now and we will see how we can update the test to work in our ci environment.

done - tag INVENTORY

danischm · 2024-02-09T23:10:31Z

This pull request is quite large, and I would welcome a suggestion what would be ideal PR size from reviewer's perspective.

First of all, thank you for the contribution and awesome work! In general I would say one resource and/or data source per PR, though this could create merge conflicts in some of the "shared" files if we work on multiple PRs in parallel. If the resources and/or data sources are somehow connected and/or have interdependencies, I am fine with more than one in a single PR as well.

danischm · 2024-02-09T23:13:26Z

I really like the more comprehensive TF example you have added and I am wondering if it would be more "visible" if we would add it with some explanations to the "Guides" section in the documentation.

Also, remove redundant YAML entries. If we are not adding a resource we can remove no_update, no_delete, no_import and no_read.

danischm · 2024-02-13T10:30:38Z

gen/definitions/discovery.yaml

+    requires_replace: true
+  - model_name: enablePasswordList
+    type: StringList
+    # description: Enable Password of the devices to be discovered.


According to the API documentation it is a list of strings, so this would be the right type to use. I would suggest to leave it in and uncomment the descriptions. We will test this with an internal instance.

danischm · 2024-02-13T10:33:05Z

internal/provider/data_source_catalystcenter_network_devices_test.go

+}
+
+func testAccDataSourceCcNetworkDevicesConfig() string {
+	config := `resource "catalystcenter_network_devices" "test" {` + "\n"


Yes, let's add a test tag for now and we will see how we can update the test to work in our ci environment.

Where the POST requests are slow the same would be probably true for the PUT requests (more often than not).

I'm not sure about the semantics of these 3 lists. Should it be in fact a single list with 3 nested attribs? For now going with raw API specs, but re-structuring would be probably welcome here.

Both network_devices and device_detail need their attributes to come from a real device. I don't see how to POST/PUT them. Similarly IMAGE_ACTIVATION and IMAGE_DISTRIBUTION, these tests require specific real devices and matching image binary files.

jabielecki and others added 30 commits February 7, 2024 09:27

add to .gitignore

95805e9

add Discovery

0023bf1

Use exclude_example for most discovery attribs Why the "timeout" is write_only: the POST needs "timeout", but GET annoyingly returns "timeOut".

fix schema comment

7fcc0f6

add data network_devices

019a82a

add RequiresReplace

fc69369

dirty MaxAsyncWaitTime for discovery operation

2fda4ee

add SWIM image_distribution

0679a82

add SWIM image

2230df4

The HTTP DELETE does not use the usual dna/intent/* prefix. Reason is that dna/intent/* returns 404 for DELETE, but without the prefix it works as expected on our lab.

image: manual changes to generated code

c0fa05f

Don't copy large file to log. First timeout: the POST itself, until CC receives the upload bytes. Second timeout on top of the first: the CC task execution.

calculated id for image_distribution, no_read

1e1a2c4

image_distribution no_data_source

e4fe515

add image_distribution requires_replace

dbfe2ce

image_distribution add no_update

1eff1bc

add image_activation

9c4bf02

calculate composite ID for image_activation

a1aa5fa

Since API has no concept of unique id for an activation, we can compose one. Just take all attributes that define uniqueness, and concatenate them.

Revert "dirty MaxAsyncWaitTime for discovery operation"

9bdb7b9

This reverts commit 0fa569d.

MaxAsyncWaitTime - manual change

6acb49b

MaxAsyncWaitTime - auto-generate change

82b0072

network_devices: rm resource and import

b23b1b3

docs: always attempt to adjust subcategory

ced4e98

Reason: there is no harm in trying to adjust an empty subcategory, whether the golang code was auto-generated or manually specified.

network_devices: docs

1326190

examples/basic provider.tf

4ff35b7

doc image_distribution

fca7270

network_devices: add management_state

27209cf

add examples/basic/upgrade_multiple

4314cb4

activation can take >39 minutes in practice, doubling

254b71b

jabielecki added 2 commits February 9, 2024 16:44

discovery requires_replace

1ded29a

add max_async_wait_time to schema.yaml

9a63071

danischm reviewed Feb 9, 2024

View reviewed changes

gen/generator.go Show resolved Hide resolved

danischm requested changes Feb 9, 2024

View reviewed changes

jabielecki force-pushed the swim branch from 6a0252c to f850d4e Compare February 9, 2024 19:28

danischm requested changes Feb 9, 2024

View reviewed changes

jabielecki added 2 commits February 12, 2024 17:06

format as string: data_path '0'

407ae90

doc_category: Inventory instead of Devices

ac35c88

Also, remove redundant YAML entries. If we are not adding a resource we can remove no_update, no_delete, no_import and no_read.

jabielecki force-pushed the swim branch from f850d4e to 0a4d387 Compare February 12, 2024 16:58

danischm requested changes Feb 13, 2024

View reviewed changes

jabielecki added 4 commits February 13, 2024 13:58

max_async_wait_time for HTTP PUT requests

3e231d7

Where the POST requests are slow the same would be probably true for the PUT requests (more often than not).

add 3 list descriptions

247169f

I'm not sure about the semantics of these 3 lists. Should it be in fact a single list with 3 nested attribs? For now going with raw API specs, but re-structuring would be probably welcome here.

generator: handle NoImport in the test.go

78e2061

jabielecki force-pushed the swim branch from 0a4d387 to 23b9a63 Compare February 13, 2024 16:43

jabielecki added 2 commits February 13, 2024 20:10

network_devices: return devices instead of response

4be47cf

requires_replace for nested attributes

bba415a

jabielecki force-pushed the swim branch from 23b9a63 to bba415a Compare February 13, 2024 22:11

danischm approved these changes Feb 14, 2024

View reviewed changes

danischm merged commit f7f125b into CiscoDevNet:main Feb 14, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add discovery and SWIM functionality #18

add discovery and SWIM functionality #18

jabielecki commented Feb 9, 2024

danischm Feb 9, 2024

jabielecki Feb 9, 2024

danischm Feb 9, 2024

jabielecki Feb 12, 2024

danischm Feb 9, 2024

jabielecki Feb 12, 2024

danischm Feb 9, 2024

jabielecki Feb 12, 2024

danischm Feb 9, 2024

jabielecki Feb 12, 2024

danischm Feb 13, 2024

jabielecki Feb 13, 2024

danischm Feb 9, 2024

jabielecki Feb 13, 2024

danischm Feb 9, 2024

jabielecki Feb 13, 2024

danischm Feb 9, 2024

danischm Feb 9, 2024

jabielecki Feb 12, 2024

danischm Feb 9, 2024

jabielecki Feb 12, 2024

danischm Feb 9, 2024

jabielecki Feb 12, 2024

danischm Feb 13, 2024

jabielecki Feb 13, 2024

danischm commented Feb 9, 2024

danischm commented Feb 9, 2024

danischm Feb 13, 2024

danischm Feb 13, 2024

		@@ -410,7 +410,7 @@ func (r *{{camelCase .Name}}Resource) Create(ctx context.Context, req resource.C
		{{- if .PutCreate}}
		res, err := r.client.Put(plan.getPath() + params, body)

add discovery and SWIM functionality #18

add discovery and SWIM functionality #18

Conversation

jabielecki commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danischm commented Feb 9, 2024

danischm commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment