Add a warning/error option to schema validation

[dak1n1]

Add new schema options to configure the warning/error level of validation functions.
This allows the following functions to be used as a warning-level diag instead of a fatal error:
"ConflictsWith", "AtLeastOneOf", "ExactlyOneOf", "RequiredWith".

The ConditionsMode allows provider developers to toggle between "warning" and "error" level messages.
The ConditionsMessage is an optional additional message to be displayed to users at run time.

Fixes #727.

[dak1n1]

Here's some output from testing, in case it helps. I copied the schema.go file into the Kubernetes provider's vendor directory so I could play with it locally using real provider code.

My only issue with it is that it repeats the messages quite a lot. But I wasn't able to find the cause of this redundancy.

$ cat main.tf
terraform {
  required_providers {
    kubernetes = {
      source = "localhost/test/kubernetes"
      version = "9.9.9"
    }
  }
}

# These two provider configuration options conflict with each other
provider "kubernetes" {
  config_path  = "/home/dakini/work/20210420/conflictswithwarning/aks/kubeconfig"
  host          = "invalidhost"
}

resource "kubernetes_namespace" "test" {
  metadata {
    name = "test"
  }
}

resource "kubernetes_namespace" "test2" {
  metadata {
    name = "test2"
  }
}

$ terraform plan

Plan: 2 to add, 0 to change, 0 to destroy.
╷
│ Warning: ConflictsWith
│ 
│ Specifying more than one authentication method can lead to unpredictable behavior. This option will be removed in a future release. Please
│ update your configuration. "host": conflicts with config_path
╵
╷
│ Warning: ConflictsWith
│ 
│ Specifying more than one authentication method can lead to unpredictable behavior. This option will be removed in a future release. Please
│ update your configuration. "config_path": conflicts with host
╵
╷
│ Warning: ConflictsWith
│ 
│ Specifying more than one authentication method can lead to unpredictable behavior. This option will be removed in a future release. Please
│ update your configuration. "host": conflicts with config_path
╵
╷
│ Warning: ConflictsWith
│ 
│ Specifying more than one authentication method can lead to unpredictable behavior. This option will be removed in a future release. Please
│ update your configuration. "config_path": conflicts with host
╵

Here's the full provider schema where I'm using ConditionsMode and ConditionsMessage: https://github.com/hashicorp/terraform-provider-kubernetes/blob/415a860b9c9d6afc8c30c6061512c805f3eba542/kubernetes/provider.go

[paddycarver]

Is there any chance we could make this an enum with constants, rather than a string?

E.g.:

type SchemaConditionsMode uint8

const (
  SchemaConditionsModeError SchemaConditionsMode = 0
  SchemaConditionsModeWarning SchemaConditionsMode = 1
)

Then in this Schema struct it would be:

type Schema struct {
  // everything else is the same, omitting
  ConditionsMode SchemaConditionsMode
  // everything else is the same, omitting
}

and in when using it, you'd do:

mySchema := &schema.Schema{
  // everything else is the same, omitting
  ConditionsMode: schema.SchemaConditionsModeWarning,
  // everything else is the same, omitting
}

This has the neat benefit that we can define the default behavior (unset/0) as SchemaConditionsModeError, meaning we don't need to detect when it's unset, the default is just an error.

[paddycarver]

Alternatively, we could make ConditionsMode accept a diag.Severity, but I think I like that less.

[dak1n1]

Wow, that is really neat! Sure, I'll try this.

[paddycarver]

nitpick: can we use "allowlist" and "denylist" or something similar?

[paddycarver]

(I'm sure you're just copying code that exists, but I'd like to leave the campsite better than we found it and not carry that pattern into the future. :) )

[dak1n1]

I would prefer this too! Would it be ok if I replace this in other tests too? Or should I limit the scope of my changes for this PR? (For reference, there are 144 instances of "whitelist" in this file).

[paddycarver]

As long as the tests pass, I would be happy to accept that change as part of this PR, in the spirit of leaving the area of code we're working on better than we found it, as we're unlikely to prioritize a PR that changed only those things. :)

[dak1n1]

I rebased, added the enum type, and updated the tests. Tests are passing.

$ make test
==> Checking that code complies with gofmt requirements...
go generate ./...
go test ./...
?       github.com/hashicorp/terraform-plugin-sdk/v2/diag       [no test files]
ok      github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest     0.064s
ok      github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff  0.031s
?       github.com/hashicorp/terraform-plugin-sdk/v2/helper/logging     [no test files]
ok      github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource    2.879s
ok      github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema      0.047s
ok      github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure   0.026s
ok      github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation  0.011s
?       github.com/hashicorp/terraform-plugin-sdk/v2/internal/addrs     [no test files]
ok      github.com/hashicorp/terraform-plugin-sdk/v2/internal/configs/configschema      0.022s
ok      github.com/hashicorp/terraform-plugin-sdk/v2/internal/configs/hcl2shim  0.068s
?       github.com/hashicorp/terraform-plugin-sdk/v2/internal/diagutils [no test files]
ok      github.com/hashicorp/terraform-plugin-sdk/v2/internal/helper/hashcode   0.025s
ok      github.com/hashicorp/terraform-plugin-sdk/v2/internal/plans/objchange   0.011s
ok      github.com/hashicorp/terraform-plugin-sdk/v2/internal/plugin/convert    0.044s
?       github.com/hashicorp/terraform-plugin-sdk/v2/internal/plugintest        [no test files]
?       github.com/hashicorp/terraform-plugin-sdk/v2/internal/providers [no test files]
ok      github.com/hashicorp/terraform-plugin-sdk/v2/internal/tfdiags   0.038s
ok      github.com/hashicorp/terraform-plugin-sdk/v2/internal/vault/sdk/helper/compressutil     0.013s
ok      github.com/hashicorp/terraform-plugin-sdk/v2/internal/vault/sdk/helper/jsonutil 0.003s
?       github.com/hashicorp/terraform-plugin-sdk/v2/meta       [no test files]
?       github.com/hashicorp/terraform-plugin-sdk/v2/plugin     [no test files]
ok      github.com/hashicorp/terraform-plugin-sdk/v2/terraform  0.009s

I also made the required changes in the Kubernetes provider PR, to test the new type there. It worked when I tested it in minikube.

[paddycarver]

Sorry to nitpick, but I'd feel much better about explicitly converting these rather than type casting, because if either value ever changes, I think the results would be surprising.

Maybe we could have a SchemaConditionsMode.AsDiagSeverity method that returned the diag.Severity for that mode? The method could be as simple as:

func (s SchemaConditionsMode) AsDiagSeverity() diag.Severity {
  switch s {
    case SchemaConditionsModeWarning:
      return diag.Warning
    default:
      return diag.Error
  }
}

for example.

[hashicorp-cla]

All committers have signed the CLA.