Overhaul Custom Fields for Devices to support any underlying type #443
Conversation
|
Judging by the branch name, you have already noticed that this is a major overhaul of a part of this provider. I will counter this PR with some thoughts about the current state of custom fields (CFs) in this provider. Note that since I do not use custom fields personally, my insights in this topic might be somewhat limited. HistoryCustom fields quickly became a "I have to tackle this eventually" topic in this provider, as shown by the number of issues and PRs about this topic. Since it is also a somewhat complex topic, I historically prioritized "lower hanging fruit" like new resources/data-sources and small fixes and the occasional support for a new netbox version over "properly handle custom fields". NetboxNetbox supports custom fields of various types, including non-basic types like "multiple selection". I did not look at the API implementation of it yet, but I got the impression that it basically takes any object and checks at runtime whether the value of a given custom field matches the type defined in netbox. Current state of the providerCurrently, we assume that the custom field is a string, which seems to work somewhat okay for string types (obviously) and seems to be wonky at best, broken at worst with other types of CFs. Desired state of the providerWell, that is a good question. Generally, all types of custom fields should be supported by the provider. Over the years, I thought about this here and there and these were the questions I wanted to answer (given A1 above):
In that case, the type of of the CF must be known at plan-time.
All this in addition to the usual considerations about best experience for the user etc. ConclusionIdeally, I want to handle the custom fields problem exactly once, in a global, sustainable way, not in small steps. I will mark this as draft, similar to #428 while we discuss this. |
|
@fbreckle - thanks for taking the time to review this and provide such a clear response of the current state of CFs along with questions you have. I figured this was a major breaking change from the users perspective, since it's changing the behavior of the field. I thought doing it incrementally might be easier to handle, but i understand your perspective of doing a global change. I will do my best to answer your questions to the best of my current understanding. First of all, your assumption (A1) of how Netbox handles custom fields is correct. Netbox will check the types of the custom fields, and return an error from the API if the type for the specific custom field is incorrect. 1. Do we need to validate the CFs before sending? 2. Can we somehow get the type from the upstream API at runtime? 3. Do we have the user provide the type in the HCL code as in 4. Do we use a type field with jsonencode? 5. Do we use a separate block per CF field type? 6. Is the jsonencode() usage stable enough? If this provider was using the terraform-plugin-framework, there may be some other options in the schema to handle some of these complex types better. But with the SDKv2, I think our options are pretty limited. |
|
Also, for reference, here is the structure of every single custom field type in json form, for reference. All of their values when empty use {
"custom_fields": {
"boolean": true,
"date": "2023-08-02",
"decimal": 1.2,
"integer": 1,
"json": {
"a": "1"
},
"multiple_object": [
{
"id": 63,
"url": "http://localhost:8001/api/virtualization/clusters/63/",
"display": "cluster1",
"name": "cluster1"
},
{
"id": 64,
"url": "http://localhost:8001/api/virtualization/clusters/64/",
"display": "cluster2",
"name": "cluster2"
}
],
"multiple_selection": [
"choice1",
"choice2"
],
"object": {
"id": 95,
"url": "http://localhost:8001/api/dcim/sites/95/",
"display": "site1",
"name": "site1",
"slug": "site1"
},
"selection": "choice1",
"text": "this is a text string",
"text_long": "this is a really long text string\r\nwith \r\nenters",
"url": "http://test.com"
}
}The implementation in this PR should handle this full structure. But the user would have to understand the structure to use it appropriately. For example resource "netbox_device" "test" {
name = "test"
custom_fields = jsonencode({
boolean = true
text = "this is text"
url = "http://test.com"
multiple_selection = ["choice1", "choice2"]
object = {
id = 95
}
multiple_object = [{
id = 63
},
{
id = 64
}]
})
} |
|
Hey @fbreckle any further comments or thoughts on this potential implementation? |
|
@tagur87 I would have an opinion about your suggested exemple. Idealy if the custom_fields json could be smart and handle id OR slug for the objects that could be nice. Except that is soudns perfect because it covers all the cases with a high flexibility for any future changes in netbox itself. |
|
Hi @gillg Not sure what ID or slug you are referring to? Custom fields do not have those that I am aware of. It is an arbitrary map of values with an unknown structure depending on the specific configuration. Can you clarify what you mean? Sounds like you are in agreement with this option? Not sure what added validation we can provide...the api already does a pretty good job at responding with an error message that indicates if the custom field is not valid or improperly formatted. |
I mean it would be bery nice to have a way to do that: resource "netbox_device" "test" {
name = "test"
custom_fields = jsonencode({
boolean = true
text = "this is text"
url = "http://test.com"
multiple_selection = ["choice1", "choice2"]
object = {
id = 95 # Let's say this object doesn't have a secondary key like "slug" or we use a datasource to get the ID
}
multiple_object = [{
slug = "key1" # Let's say this object type is a tenant with a slug which is a valid unique key
},
{
slug = "key42"
}]
})
}You can't define slug and id in the same map, but having the choice seems nice. |
I see what you’re saying. The custom field “schema” is just a json blob that gets sent to the api. You can format it in whatever way the api will allow. If the custom field supports setting via a slug, it will work. Whatever you can do with a POST or PUT in the api will work with this field. |
|
Hi again, finally revisiting this now, since I have some time the upcoming week. I hope we can get this feature across the finish line in that time. This time, I want to take a look at it from the HCL side of things: Can we somehow move from this (your code): resource "netbox_device" "test" {
name = "test"
custom_fields = jsonencode({
boolean = true
text = "this is text"
url = "http://test.com"
multiple_selection = ["choice1", "choice2"]
object = {
id = 95
}
multiple_object = [
{
id = 63
},
{
id = 64
}
]
})
}to this (jsonencode where necessary) resource "netbox_device" "test" {
name = "test"
custom_fields = {
boolean = true
text = "this is text"
url = "http://test.com" # might need jsonencode
multiple_selection = jsonencode(["choice1", "choice2"])
object = jsonencode({
id = 95
})
multiple_object = jsonencode([
{
id = 63
},
{
id = 64
}
])
}
}Can this work? It might be wonky for primitives where I ponder so much on this because I really am not a fan of using a mandatory I played around a bit and the more I am testing, the more I think that your implementation is the most stable one. When dealing with strings as values, the only way to know if the provided value is intended to be json is by encoding ALL values with Just rambling and updating on the state of this PR. I will also have to check the behavior in the tests and at plan time manually. |
|
Ok, so one problem with this solution is that it breaks existing state, because we go from I'm considering biting the bullet and migrating this provider to the framework because of https://developer.hashicorp.com/terraform/plugin/framework/migrating/benefits#custom-attribute-types . |
|
Hi, I just wanted to mention that this is equally an issue with netbox_ip_addresses |
|
yes. tl;dr: custom fields need to be completely revamped, but other things keep getting in the way. like netbox breaking changes with 3.6.0 again, netbox making openapi3 spec only, hashicorp changing the recommended framework for tf providers and regular features this PR is a stand-in for the problem as a whole and has one solution implemented for a single resource |
Create functions for overahauling custom fields to support multiple underlying types. Create function for the schema with using TypeString instead of TypeMap. This allows for complex json types using jsonencode/jsondecode. Create handleCustomFieldUpdate function and tests for it. This function takes in the results of a `d.GetChange` funtion `old` & `new`, which allows us to compare old and new values, and update the `custom_fields` field in the api. Add unit test to validate functionality. handleCustomFieldRead and tests were added to handle the reading of custom fields and setting the field to "" if all the custom fields are nil. Adds tests to validate the functionality.
Update the data source `netbox_devices` to use the string based
schema for the `custom_fields` attribute.
This allows for any underlying type to be accessed by using the
`jsondecode()` terraform function.
For example, to access custom fields, the following code can be used.
```terraform
data "netbox_devices" "test" {
filter {
name = "name"
value = "device1"
}
}
output "device_field" {
value = jsondecode(data.netbox_devices.test[0].custom_fields).field1
}
```
Updates the `netbox_device` resource to allow for any custom field
type to be set. This is done by using a string field with the
`jsonencode()` and `jsondecode()` terraform functions.
This allows for any complex custom fields or data types that are needed,
since the underlying types don't matter and are unmarshalled using
map[string]interface{}.
To set custom fields using this new method, use the following syntax.
```terraform
resource "netbox_device" "test" {
name = "device1"
tenant_id = netbox_tenant.test.id
role_id = netbox_device_role.test.id
device_type_id = netbox_device_type.test.id
site_id = netbox_site.test.id
custom_fields = jsonencode({
"text_field" = "81"
"bool_field" = true
})
}
```
NOTE: This may be a breaking change for some users.
@fbreckle - sorry for the long delay on this, been wrapped up in other projects. I understand the concern on the state issues. A state migration would be the best way forward probably. I have worked on some framework projects, but not a ton. I have a rough understanding of it. Are you thinking we should still wait till a move toward framework is started/worked on? I know there are lots of other things on your plate, let me know what you think. |
feat: allow any custom field type in netbox_device
Updates the
netbox_deviceresource to allow for any custom fieldtype to be set. This is done by using a string field with the
jsonencode()andjsondecode()terraform functions.This allows for any complex custom fields or data types that are needed,
since the underlying types don't matter and are unmarshalled using
map[string]interface{}.
To set custom fields using this new method, use the following syntax.
feat: allow any custom field type in netbox_devices data source
Update the data source
netbox_devicesto use the string basedschema for the
custom_fieldsattribute.This allows for any underlying type to be accessed by using the
jsondecode()terraform function.For example, to access custom fields, the following code can be used.
feat: Add funcs for custom fields of any type
Create functions for overahauling custom fields to support multiple
underlying types.
Create function for the schema with using TypeString instead of
TypeMap. This allows for complex json types using jsonencode/jsondecode.
Create handleCustomFieldUpdate function and tests for it. This function
takes in the results of a
d.GetChangefuntionold&new, whichallows us to compare old and new values, and update the
custom_fieldsfield in the api. Add unit test to validate functionality.
handleCustomFieldRead and tests were added to handle the reading of
custom fields and setting the field to "" if all the custom fields
are nil. Adds tests to validate the functionality.
These changes may be breaking, however have not fully tested that part. Not sure how best to do so.
Fixes #416
Fixes #409