DataDog / terraform-provider-datadog

Terraform Datadog provider
https://www.terraform.io/docs/providers/datadog/
Mozilla Public License 2.0
399 stars 375 forks source link

[datadog_dashboard] Add support for text_formats in query table widget requests #2587

Open hyungl opened 1 week ago

hyungl commented 1 week ago

Motivation

Customer has asked for Terraform support of an existing feature: Query Table text formats. Corresponding api-spec pr.

Existing dashboard schema containing text_formats:

{
    "id": "",
    "title": "",
    "description": "Created using the Datadog provider in Terraform",
    ...
    "widgets": [
        {
            "definition": {
                "requests": [
                    {
                        "aggregator": "max",
                        "alias": "cpu user",
                        "cell_display_mode": [
                            "number"
                        ],
                        "limit": 25,
                        "order": "desc",
                        "q": "avg:system.cpu.user{account:prod} by {service, team}",
                        "text_formats": [
                            [
                                {
                                    "match": {
                                        "type": "is",
                                        "value": "test"
                                    },
                                    "palette": "black_on_light_yellow",
                                    "replace": {
                                        "type": "all",
                                        "with": "test"
                                    }
                                }
                            ],
                            [
                                {
                                    "match": {
                                        "type": "is",
                                        "value": "versus"
                                    }
                                }
                            ]
                        ]
                    }
                ],
                ...
            },
            "id": 
        }
    ],
   ...
}

Fields

match is an object containing fields type, value. replace is an object containing fields type, with, and substring. palette is a dropdown of color options. custom_bg_color and custom_fg_color are strings.

text_formats is a nested array of objects

Each column of values in a query table accounts for each inner array of text_formats. Within each inner array, each object is a separate rule that governs the column of data.

In the provider, we have to define text_formats as a nested array

In resource_datadog_dashboard.go, we have to define the schema like this:

"text_formats": {
            Type:     schema.TypeList,
            Optional: true,
            Elem: &schema.Schema{
                Type: schema.TypeList,
                Elem: &schema.Resource{
                    Schema: map[string]*schema.Schema{
                        "match": {
                                                  ...

This seems to work, at least in terms of using terraform to manage dashboards

This example dashboard:

resource "datadog_dashboard" "query_table_dashboard" {
    title         = "{{uniq}}"
    description   = "Created using the Datadog provider in Terraform"
    layout_type   = "ordered"

    widget {
        query_table_definition {
            title_size = "16"
            title = "this is hyung"
            title_align = "right"
            live_span = "1d"
            request {
                aggregator = "max"
                q = "avg:system.cpu.user{account:prod} by {service, team}"
                alias = "cpu user"
                limit = 25
                order = "desc"
                cell_display_mode = ["number"]
                text_formats = [
                    [{
                        match = {
                            type = "is"
                            value = "test"
                        }
                        palette = "black_on_light_yellow"
                        replace = {
                            type = "all"
                            with = "test"
                        },
                        custom_bg_color = null 
                        custom_fg_color = null 
                    }],
                    [{
                        match = {
                            type = "is"
                            value = "versus"
                        }
                        palette = null
                        replace = null 
                        custom_bg_color = null
                        custom_fg_color = null
                    },
                    {
                        match = {
                            type = "is"
                            value = "today"
                        }
                        palette = null
                        replace = null 
                        custom_bg_color = null
                        custom_fg_color = null
                    }]
                ]
            }
            has_search_bar = "auto"
        }
    }
}

saves a dashboard like this:

{
    "id": "8px-pfi-msw",
    "title": "{{uniq}}",
    "description": "Created using the Datadog provider in Terraform",
    "author_handle": "hyung.lee@datadoghq.com",
    "author_name": "Hyung Lee",
    "layout_type": "ordered",
    "url": "/dashboard/8px-pfi-msw/uniq",
    "is_read_only": false,
    "template_variables": [],
    "widgets": [
        {
            "definition": {
                "has_search_bar": "auto",
                "requests": [
                    {
                        "aggregator": "max",
                        "alias": "cpu user",
                        "cell_display_mode": [
                            "number"
                        ],
                        "limit": 25,
                        "order": "desc",
                        "q": "avg:system.cpu.user{account:prod} by {service, team}",
                        "text_formats": [
                            [
                                {
                                    "match": {
                                        "type": "is",
                                        "value": "test"
                                    },
                                    "palette": "black_on_light_yellow",
                                    "replace": {
                                        "type": "all",
                                        "with": "test"
                                    }
                                }
                            ],
                            [
                                {
                                    "match": {
                                        "type": "is",
                                        "value": "versus"
                                    }
                                },
                                {
                                    "match": {
                                        "type": "is",
                                        "value": "today"
                                    }
                                }
                            ]
                        ]
                    }
                ],
                "time": {
                    "live_span": "1d"
                },
                "title": "this is hyung",
                "title_align": "right",
                "title_size": "16",
                "type": "query_table"
            },
            "id": 1229419269467444
        }
    ],
    "notify_list": [],
    "created_at": "2024-09-23T03:39:48.265448+00:00",
    "modified_at": "2024-09-23T04:18:29.538357+00:00",
    "experience_type": "default",
    "template_variable_presets": [],
    "tags": [],
    "restricted_roles": []
}

Terraform doesn't respect that fields are defined as optional. All fields need to be specified, and given null values if unused

If any of the fields are left out in the terraform config, we see during testing ( Testing command: RECORD=true TESTARGS="-run TestAccDatadogDashboardQueryTableWithTextFormats" op run -- make testall, which runs this terraform command: terraform refresh -no-color -input=false -lock-timeout=0s -lock=true ) this error:

Error: Incorrect attribute value type
...
Inappropriate value for attribute "text_formats": element 0: element 0:
attributes "custom_bg_color", "custom_fg_color", and "replace" are required.

This is happening when the test is trying to do terraform refresh of the config.

Possible causes

Using logs, confirmed that the Provider function in this repo does specify the new optional fields as optional.

Example logging

log.Println(fmt.Sprintf("custom_fg_color is optional %v", provider.ResourcesMap["datadog_dashboard"].SchemaFunc()["widget"].Elem.(*schema.Resource).Schema["query_table_definition"].Elem.(*schema.Resource).Schema["request"].Elem.(*schema.Resource).Schema["text_formats"].Elem.(*schema.Schema).Elem.(*schema.Resource).Schema["custom_fg_color"].Optional))

To see why terraform was complaining, I had to build terraform from source and set logs in the dependency hcl library to see what's happening when the terraform config and schema are used.

hcldec.AttrSpec instead of hcldec.BlockListSpec

The nodes of the terraform schema are handled as specs. Setting logs here confirmed that text_formats are handled as *hcldec.AttrSpec, not *hcldec.BlockListSpec. Maybe this is why the optionals aren't working correctly? Another field in QueryTable.Request is conditional_formats which is only one level of array nesting. That one is evaluated as a BlockListSpec and seems to handle Optional values correctly.

Invalid Plan that is ignored

[WARN]  Provider "registry.terraform.io/datadog/datadog" produced an invalid plan for datadog_dashboard.query_table_dashboard, but we are tolerating it because it is using the legacy plugin SDK.
    The following problems may be the cause of any confusing errors from downstream operations:
      - .notify_list: planned value cty.SetValEmpty(cty.String) for a non-computed attribute
      - .reflow_type: planned value cty.StringVal("") for a non-computed attribute
      - .is_read_only: planned value cty.False for a non-computed attribute
      - .restricted_roles: planned value cty.SetValEmpty(cty.String) for a non-computed attribute
      - .tags: planned value cty.ListValEmpty(cty.String) for a non-computed attribute
      - .widget[0].query_table_definition[0].request[0].text_formats: planned value cty.ListVal([]cty.Value{cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"custom_bg_color":cty.StringVal(""), "custom_fg_color":cty.StringVal(""), "match":cty.MapVal(map[string]cty.Value{"type":cty.StringVal("is"), "value":cty.StringVal("test")}), "palette":cty.NullVal(cty.String), "replace":cty.MapValEmpty(cty.String)})})}) does not match config value cty.ListVal([]cty.Value{cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"custom_bg_color":cty.NullVal(cty.String), "custom_fg_color":cty.NullVal(cty.String), "match":cty.MapVal(map[string]cty.Value{"type":cty.StringVal("is"), "value":cty.StringVal("test")}), "palette":cty.NullVal(cty.String), "replace":cty.NullVal(cty.Map(cty.String))})})}) nor prior value cty.ListVal([]cty.Value{cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"custom_bg_color":cty.StringVal(""), "custom_fg_color":cty.StringVal(""), "match":cty.MapVal(map[string]cty.Value{"type":cty.StringVal("is"), "value":cty.StringVal("test")}), "palette":cty.StringVal("black_on_light_yellow"), "replace":cty.MapVal(map[string]cty.Value{"type":cty.StringVal("all"), "with":cty.StringVal("test")})})}), cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"custom_bg_color":cty.StringVal(""), "custom_fg_color":cty.StringVal(""), "match":cty.MapVal(map[string]cty.Value{"type":cty.StringVal("is"), "value":cty.StringVal("versus")}), "palette":cty.StringVal(""), "replace":cty.MapValEmpty(cty.String)}), cty.ObjectVal(map[string]cty.Value{"custom_bg_color":cty.StringVal(""), "custom_fg_color":cty.StringVal(""), "match":cty.MapVal(map[string]cty.Value{"type":cty.StringVal("is"), "value":cty.StringVal("today")}), "palette":cty.StringVal(""), "replace":cty.MapValEmpty(cty.String)})})})

Seems terraform is being lenient with the difference between null and "". Maybe this is causing issues downstream?

Inner fields not Optional after decoding?

Printing out the spec that terraform hcl is using at this part of the code:

&{Name:text_formats Type:{typeImpl:{typeImplSigil:{} ElementTypeT:{typeImpl:{typeImplSigil:{} ElementTypeT:{typeImpl:{typeImplSigil:{} AttrTypes:map[custom_bg_color:{typeImpl:{typeImplSigil:{} Kind:83}} custom_fg_color:{typeImpl:{typeImplSigil:{} Kind:83}} match:{typeImpl:{typeImplSigil:{} ElementTypeT:{typeImpl:{typeImplSigil:{} Kind:83}}}} palette:{typeImpl:{typeImplSigil:{} Kind:83}} replace:{typeImpl:{typeImplSigil:{} ElementTypeT:{typeImpl:{typeImplSigil:{} Kind:83}}}}] AttrOptional:map[]}}}}}} Required:false}

Maybe it's not good that Required: false appears at the level of the entire object, but not for each nested field that is optional.

Appendix - notes on testing

Since terraform-provider-datadog depends on terraform-plugin-testing which depends on terraform-exec, I replicated the test scenario by building terraform locally:

  1. git clone git@github.com:hashicorp/terraform.git
  2. Add logging
  3. go build -ldflags "-w -s -X 'github.com/hashicorp/terraform/version.dev=no'" -o bin/ .
  4. In terraform's go.mod:
    
    replace github.com/hashicorp/hcl/v2 => /Users/hyung.lee/go/src/github.com/DataDog/hcl

replace github.com/zclconf/go-cty v1.15.0 => /Users/hyung.lee/go/src/github.com/DataDog/go-cty

5. I put a `dashboard.tf` file containing the config I wanted to test within a folder
6. `$DATADOG_ROOT/terraform/bin/terraform refresh -no-color -input=false -lock-timeout=0s -lock=true`

Terraform seems to scan the list of list of objects correctly. [A section of code](https://github.com/zclconf/go-cty/blob/v1.13.0/cty/convert/public.go#L42) translates the `text_formats` config from tuple to list of list of object:

in: {{{{} [{{{} [{{{} map[custom_bg_color:{{{}}} custom_fg_color:{{{}}} match:{{{} map[type:{{{} 83}} value:{{{} 83}}] map[]}} palette:{{{}}} replace:{{{}}}] map[]}}]}}]}} [[map[custom_bg_color: custom_fg_color: match:map[type:is value:test] palette: replace:]]]} in type name: tuple want type name: list of list of object

brtu commented 5 days ago

What would the consequences if we change text_formats from a list of lists to just a list? I suspect that may be causing the issue with requiring those explicitly set null fields

drichards-87 commented 2 days ago

Created a Jira ticket for Docs Team review.