terraform-provider-netbox Overhaul Custom Fields for Devices to support any underlying type

feat: allow any custom field type in netbox_device

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.

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
	})
}

feat: allow any custom field type in netbox_devices data source

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.

data "netbox_devices" "test" {
	filter {
		name  = "name"
		value = "device1"
	}
}

output "device_field" {
	value = jsondecode(data.netbox_devices.test[0].custom_fields).field1
}

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.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.

These changes may be breaking, however have not fully tested that part. Not sure how best to do so.

Fixes #416 Fixes #409

Aug 01 '23 16:08 tagur87

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.

History

Custom 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". Now, obviously, changing the user-interface of CFs is going to be a major breaking change for many users, so changes should be well thought out and then applied to all resources. A while ago, we had an implmentation PR where someone basically took the solution of another terraform provider (https://github.com/e-breuninger/terraform-provider-netbox/pull/235).

Netbox

Netbox 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. Example: Let F1 be a custom field, then the API will accept {"F1": "foo" } as value of the CF if the type of F1 is "string", but it will respond with an error if F1 is of type "list". So, API-wise, we do not have to (neither CAN we) explicitly provide the type of the CF when sending it to netbox. Let this be Assumption A1. Correct me if I'm wrong here.

Current state of the provider

Currently, 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 provider

Well, 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):

Do we even need to validate the value of CFs in the provider before sending it to netbox? Why?

In that case, the type of of the CF must be known at plan-time.

Can we somehow get the type from the upstream API at runtime?
Do we have the user provide the type in the HCL code as in https://github.com/e-breuninger/terraform-provider-netbox/pull/235?
Do we use a type field with jsonencode?
Do we use a separate block per CF field type? E.g. custom_field_list { name = "foo" value = ["this", "is a terraform", "native list"] } This way, we could use native hcl data structures and do the actual JSON encoding in the provider code instead of HCL
The version implemented here includes lots of raw json on the custom_field attribute level. Is this stable enough? Can we continue using custom_fields as a map fo strings to json-ified strings?

All this in addition to the usual considerations about best experience for the user etc.

Conclusion

Ideally, 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 https://github.com/e-breuninger/terraform-provider-netbox/pull/428 while we discuss this.

Aug 02 '23 11:08 fbreckle

@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? I don't feel we do. Since cannot get the type of the custom field from the API without some major filtering (see point 2), I feel that the validation of the custom field should be on the user to do, based on their specific custom field implementation. The API errors should be able to lead them to format them correctly. But that's just my opinion.

2. Can we somehow get the type from the upstream API at runtime? I'm sure it is possible, but this would mean that we have the query the custom field endpoint for each type that is used before we can even use them. For example, if we wanted to use 4 custom fields in a device resource, then we would have to make 4 queries to the custom field endpoint to get the type for each custom field. This is possible, but adds a lot of extra calls per resource that uses custom fields.

3. Do we have the user provide the type in the HCL code as in Again, is possible, but back to point two, would have to have a way to look this up.

4. Do we use a type field with jsonencode? I am not sure how this could work....the custom fields are just a json structure based on the name of the custom field, which goes back to the underlying type to know if it matches the type properly.

5. Do we use a separate block per CF field type? This could be possible, but I think this could get very complicated. We would have to aggregate them all and then make the update call to the api.

6. Is the jsonencode() usage stable enough? It seems to be. The errors returned by the API are pretty verbose and can help you understand where the errors lie. Also the code as currently implemented handles the unsetting of the fields if a field in the json is removed. Which doesn't happen by default.

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.

Aug 02 '23 16:08 tagur87

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 null as well.

{
    "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
      }]
})
}

Aug 02 '23 16:08 tagur87

Hey @fbreckle any further comments or thoughts on this potential implementation?

Aug 12 '23 13:08 tagur87

@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. Both are exclusive, and if an object type doesn't support a slug only the id is possible implicitely.

Except that is soudns perfect because it covers all the cases with a high flexibility for any future changes in netbox itself. But I think a good data validation and error handling is important to let know the user when something fails and not be in a case where you get an error like ERROR: Invalid custom_fields

Aug 14 '23 12:08 gillg

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.

Aug 14 '23 15:08 tagur87

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.

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. If you think it should go to a next step, sounds good to me.

Aug 16 '23 16:08 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.

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. If you think it should go to a next step, sounds good to me.

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.

Aug 16 '23 18:08 tagur87

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 jsonencode(x) is not x.

I ponder so much on this because I really am not a fan of using a mandatory jsonencode in all custom fields unless absolutely necessary.

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 jsonencode.

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.

Aug 25 '23 21:08 fbreckle

Ok, so one problem with this solution is that it breaks existing state, because we go from TypeMap to TypeString. This would have to be solved by a state migration function, else everyone gets a string expected error at plan time, requiring a manual intervention to the state file.

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 . @tagur87 you mentioned the framework earlier. Did you use the framework already? Would it allow a more elegant solution to custom fields?

Aug 31 '23 15:08 fbreckle

Hi, to be honest - I didn't read through all of the discussion in here and I don't have a clue about how the code works (-:

I just wanted to mention that this is equally an issue with netbox_ip_addresses Telling by the files edited in here so far, it seems it doesn't cover that yet.

Sep 14 '23 08:09 pidreher

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

Sep 14 '23 09:09 fbreckle

Ok, so one problem with this solution is that it breaks existing state, because we go from TypeMap to TypeString. This would have to be solved by a state migration function, else everyone gets a string expected error at plan time, requiring a manual intervention to the state file.

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 . @tagur87 you mentioned the framework earlier. Did you use the framework already? Would it allow a more elegant solution to custom fields?

@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.

Nov 17 '23 21:11 tagur87