(feature request) Validate monitor during plan with datadog api

[daisy1754]

This is a feature request same as https://github.com/terraform-providers/terraform-provider-datadog/issues/66 (closed) - datadog now provides /validate endpoint which I believe can be used by terraform-provider-datadog.

Terraform Version

0.12.19

Affected Resource(s)

• datadog_monitor

Terraform Configuration Files

Note query field is invalid (no closing }). Other fields are not relevant with issue.

resource "datadog_monitor" "my_monitor" {
  name    = "example"
  type    = "query alert"
  message = <<EOF
example
EOF
  query   = "sum(last_15m):sum:my_metrics.foo.bar{env:production.as_count() < 15"

  thresholds = {
    critical = 15.0
  }

  timeout_h           = 0
  no_data_timeframe   = 0
  renotify_interval   = 0
  notify_no_data      = false
  require_full_window = false
  notify_audit        = false
  include_tags        = true

  lifecycle {
    ignore_changes = [silenced]
  }

  tags = [
    "example"
  ]
}

Expected Behavior

terraform plan fails because this is invalid monitor definition

Actual Behavior

terraform plan succeeds, however when you run terraform apply you will get error

Steps to Reproduce

1. copy above definition
2. terraform plan -> no error
3. terraform apply -> error

References

https://github.com/terraform-providers/terraform-provider-datadog/issues/66 was closed due to philosophy that we shouldn't duplicate validation logic on provider side. However datadog provides /validate endpoint that could be used https://docs.datadoghq.com/api/?lang=bash#validate-a-monitor

[Towerthousand]

This is a big problem for me as well. Master breaks when people merge monitors because it gets applied and the queries are invalid. Adding the validate API check would be great.

[therve]

I had a crack at this, it's not trivial. First, terraform doesn't have a general validate hook for resources. The best next thing I could think of was to use CustomizeDiff for that purpose.

It seems to work OK, but it has a big caveat: at that point, we don't necessarily have all data (for example, if fields depend on non-created resources). It manifested in the tests with composite monitors. The trick is to skip validation in that particular case, but I'm a bit worried to miss another case, and that would block people from doing anything (plan fails, and you're stuck).

Any input appreciated, thanks.

[phillip-dd]

Would writing a custom ValidateFunc work? Ref: https://www.terraform.io/docs/extend/schemas/schema-behaviors.html#validatefunc. That might also not be fully resolved data though.

[therve]

ValidateFunc only works on single fields. We could just validate query, is that what you mean? That could work, we need to fake the rest a bit. AFAICT it doesn't work, because we need at least the type to do query validation.

[phillip-dd]

we need at least the type to do query validation.

Good point!

[mikelococo]

Some links to upstream issues and other discussions, for posterity:

• Here's the relevant github issue for the terraform-plugin-sdk which includes some links to real-world CustomizeDiff hacks: Multi-attribute validation hashicorp/terraform-plugin-sdk#233
• It has a linked issue where a hashicorp employee validates that CustomizeDiff is indeed the most sensible workaround: ValidateFunc should support validation of two fields dependent on each other that are defined in resource schema hashicorp/terraform-plugin-sdk#354 (comment)
• And another linked issue where another hashicorp employee, who I believe is a maintainer for terraform-provider-aws, validates that the CustomizeDiff approach is hairy enough that they don't want to touch it in at least some cases (though it looks like from the linked examples in the issue description they do it in other cases): [WIP] db_instance: allow_major_version_upgrade conflicts replicate_source_db hashicorp/terraform-provider-aws#7013 (comment)

It seems there may be no easy solutions on the horizon here. I'll just reiterate the value of having SOME validation of the query string though. The vast majority of the complexity (and apply-time errors) associated with the monitor resource come from that string, which is currently completely opaque to terraform. I know teams that are building their own incomplete string-builders to try to reduce apply-time errors. Having something at plan time that weeds out obvious invalid query-strings would speed up the debugging cycle for slow applies, even if it has false negatives (like allowing a query-string that would be valid for a different monitor type... which isn't a common real-world error and allowing it through is no worse than the behavior today).

[martinb3]

TBH, just validating the query would be huge. That's 99% of my validation failures.