From 37da625ee9693a0933f15a33e4d8629ede4a53bd Mon Sep 17 00:00:00 2001
From: Martin Atkins <mart@degeneration.co.uk>
Date: Mon, 26 Nov 2018 15:29:59 -0800
Subject: [PATCH] helper/schema: Tell Core attribute is optional if set
 conditionally

The SDK has a mechanism that effectively makes it possible to declare an
attribute as being _conditionally_ required, which is not a concept that
Terraform Core is aware of.

Since this mechanism is in practice only used for a small UX improvement
in prompting for these values interactively when the environment variable
is not set, we avoid here introducing all of this complexity into the
plugin protocol by just having the provider selectively modify its schema
if it detects that such an attribute might be set dynamically.

This then prevents Terraform Core from validating the presence of the
argument or prompting for a new value for it, allowing the null value to
pass through into the provider so that the default value can be generated
again dynamically.

This is a kinda-kludgey solution which we're accepting here because the
alternative would be a much-more-complex two-pass decode operation within
Core itself, and that doesn't seem worth it.

This fixes #19139.
---
 helper/schema/core_schema.go      | 33 +++++++++++++++-
 helper/schema/core_schema_test.go | 65 +++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/helper/schema/core_schema.go b/helper/schema/core_schema.go
index 85e8b8c5b..9578baaf5 100644
--- a/helper/schema/core_schema.go
+++ b/helper/schema/core_schema.go
@@ -58,10 +58,39 @@ func (m schemaMap) CoreConfigSchema() *configschema.Block {
 // Elem is an instance of Schema. Use coreConfigSchemaBlock for collections
 // whose elem is a whole resource.
 func (s *Schema) coreConfigSchemaAttribute() *configschema.Attribute {
+	// The Schema.DefaultFunc capability adds some extra weirdness here since
+	// it can be combined with "Required: true" to create a sitution where
+	// required-ness is conditional. Terraform Core doesn't share this concept,
+	// so we must sniff for this possibility here and conditionally turn
+	// off the "Required" flag if it looks like the DefaultFunc is going
+	// to provide a value.
+	// This is not 100% true to the original interface of DefaultFunc but
+	// works well enough for the EnvDefaultFunc and MultiEnvDefaultFunc
+	// situations, which are the main cases we care about.
+	//
+	// Note that this also has a consequence for commands that return schema
+	// information for documentation purposes: running those for certain
+	// providers will produce different results depending on which environment
+	// variables are set. We accept that weirdness in order to keep this
+	// interface to core otherwise simple.
+	reqd := s.Required
+	opt := s.Optional
+	if reqd && s.DefaultFunc != nil {
+		v, err := s.DefaultFunc()
+		// We can't report errors from here, so we'll instead just force
+		// "Required" to false and let the provider try calling its
+		// DefaultFunc again during the validate step, where it can then
+		// return the error.
+		if err != nil || (err == nil && v != nil) {
+			reqd = false
+			opt = true
+		}
+	}
+
 	return &configschema.Attribute{
 		Type:        s.coreConfigSchemaType(),
-		Optional:    s.Optional,
-		Required:    s.Required,
+		Optional:    opt,
+		Required:    reqd,
 		Computed:    s.Computed,
 		Sensitive:   s.Sensitive,
 		Description: s.Description,
diff --git a/helper/schema/core_schema_test.go b/helper/schema/core_schema_test.go
index c9b0513aa..44dc9a8e0 100644
--- a/helper/schema/core_schema_test.go
+++ b/helper/schema/core_schema_test.go
@@ -1,6 +1,7 @@
 package schema
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -294,6 +295,70 @@ func TestSchemaMapCoreConfigSchema(t *testing.T) {
 				BlockTypes: map[string]*configschema.NestedBlock{},
 			}),
 		},
+		"conditionally required on": {
+			map[string]*Schema{
+				"string": {
+					Type:     TypeString,
+					Required: true,
+					DefaultFunc: func() (interface{}, error) {
+						return nil, nil
+					},
+				},
+			},
+			testResource(&configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"string": {
+						Type:     cty.String,
+						Required: true,
+					},
+				},
+				BlockTypes: map[string]*configschema.NestedBlock{},
+			}),
+		},
+		"conditionally required off": {
+			map[string]*Schema{
+				"string": {
+					Type:     TypeString,
+					Required: true,
+					DefaultFunc: func() (interface{}, error) {
+						// If we return a non-nil default then this overrides
+						// the "Required: true" for the purpose of building
+						// the core schema, so that core will ignore it not
+						// being set and let the provider handle it.
+						return "boop", nil
+					},
+				},
+			},
+			testResource(&configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"string": {
+						Type:     cty.String,
+						Optional: true,
+					},
+				},
+				BlockTypes: map[string]*configschema.NestedBlock{},
+			}),
+		},
+		"conditionally required error": {
+			map[string]*Schema{
+				"string": {
+					Type:     TypeString,
+					Required: true,
+					DefaultFunc: func() (interface{}, error) {
+						return nil, fmt.Errorf("placeholder error")
+					},
+				},
+			},
+			testResource(&configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"string": {
+						Type:     cty.String,
+						Optional: true, // Just so we can progress to provider-driven validation and return the error there
+					},
+				},
+				BlockTypes: map[string]*configschema.NestedBlock{},
+			}),
+		},
 	}
 
 	for name, test := range tests {