Skip to content

Commit

Permalink
Dont apply TF defaults before validating resource input (#1583)
Browse files Browse the repository at this point in the history
This PR fixes an inconsistency in how we treat defaults versus TF.
Previously we would have applied defaults before validating input. TF
does the reverse.

Should address
#1546.

Tested in AWS here: pulumi/pulumi-aws#3157


Note that this PR technically introduces a breaking change. An example
of how it'd manifest is in
[TestDefaultsAndRequiredWithValidationInteraction/CheckMissingRequiredWith](https://github.com/pulumi/pulumi-terraform-bridge/blob/5ef834fd75b7c259dacc8fb71a0cc6edfd2472bc/pkg/tfbridge/provider_test.go#L2514).

Previously we'd apply the defaults before passing to the TF provider for
validation, so this test case would have yielded no failures. This is
not what TF does - TF would fail validation on that input. We will now
match the TF behaviour and fail this input. This should be fine as the
input likely does not make sense anyway.
  • Loading branch information
VenelinMartinov authored Dec 28, 2023
1 parent 7f7af28 commit 2bb6194
Show file tree
Hide file tree
Showing 8 changed files with 456 additions and 8 deletions.
128 changes: 128 additions & 0 deletions internal/testprovider/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -929,3 +929,131 @@ func MaxItemsOneProvider() *schemav2.Provider {
},
}
}

func ConflictsWithValidationProvider() *schemav2.Provider {
return &schemav2.Provider{
Schema: map[string]*schemav2.Schema{},
ResourcesMap: map[string]*schemav2.Resource{
"default_value_res": {
Schema: map[string]*schemav2.Schema{
"conflicting_property": {
Type: schemav2.TypeString,
Optional: true,
// Some TF providers use this to mean not specified
// https://github.com/pulumi/pulumi-azuredevops/issues/38
Default: "",
ConflictsWith: []string{"conflicting_property2"},
},
"conflicting_property2": {
Type: schemav2.TypeString,
Optional: true,
Default: "",
ConflictsWith: []string{"conflicting_property"},
},

"conflicting_required_property": {
Type: schemav2.TypeString,
Required: true,
Default: "",
ConflictsWith: []string{"conflicting_nonrequired_property2"},
},
"conflicting_nonrequired_property2": {
Type: schemav2.TypeString,
Optional: true,
Default: "",
ConflictsWith: []string{"conflicting_required_property"},
},
},
},
},
}
}

func ExactlyOneOfValidationProvider() *schemav2.Provider {
return &schemav2.Provider{
Schema: map[string]*schemav2.Schema{},
ResourcesMap: map[string]*schemav2.Resource{
"default_value_res": {
Schema: map[string]*schemav2.Schema{
"exactly_one_of_property": {
Type: schemav2.TypeString,
Optional: true,
Default: "",
ExactlyOneOf: []string{"exactly_one_of_property", "exactly_one_of_property2"},
},
"exactly_one_of_property2": {
Type: schemav2.TypeString,
Optional: true,
Default: "",
ExactlyOneOf: []string{"exactly_one_of_property", "exactly_one_of_property2"},
},

// This doesn't really make much sense as a schema
// but it should still behave as expected.
"exactly_one_of_required_property": {
Type: schemav2.TypeString,
Required: true,
Default: "",
ExactlyOneOf: []string{"exactly_one_of_required_property", "exactly_one_of_nonrequired_property2"},
},
"exactly_one_of_nonrequired_property2": {
Type: schemav2.TypeString,
Optional: true,
Default: "",
ExactlyOneOf: []string{"exactly_one_of_required_property", "exactly_one_of_nonrequired_property2"},
},
},
},
},
}
}

func RequiredWithValidationProvider() *schemav2.Provider {
return &schemav2.Provider{
Schema: map[string]*schemav2.Schema{},
ResourcesMap: map[string]*schemav2.Resource{
"default_value_res": {
Schema: map[string]*schemav2.Schema{
"required_with_property": {
Type: schemav2.TypeString,
Optional: true,
Default: "",
RequiredWith: []string{"required_with_property2"},
},
"required_with_property2": {
Type: schemav2.TypeString,
Optional: true,
Default: "",
RequiredWith: []string{"required_with_property"},
},

"required_with_required_property": {
Type: schemav2.TypeString,
Required: true,
Default: "",
RequiredWith: []string{"required_with_nonrequired_property"},
},
"required_with_nonrequired_property": {
Type: schemav2.TypeString,
Optional: true,
Default: "",
RequiredWith: []string{"required_with_required_property"},
},

"required_with_required_property2": {
Type: schemav2.TypeString,
Required: true,
Default: "",
RequiredWith: []string{"required_with_required_property3"},
},
"required_with_required_property3": {
Type: schemav2.TypeString,
Required: true,
Default: "",
RequiredWith: []string{"required_with_required_property2"},
},
},
},
},
}
}
10 changes: 9 additions & 1 deletion pkg/tfbridge/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -664,7 +664,7 @@ func (p *Provider) Check(ctx context.Context, req *pulumirpc.CheckRequest) (*pul
// Now fetch the default values so that (a) we can return them to the caller and (b) so that validation
// includes the default values. Otherwise, the provider wouldn't be presented with its own defaults.
tfname := res.TFName
inputs, assets, err := MakeTerraformInputs(ctx,
inputs, _, err := makeTerraformInputsWithoutTFDefaults(ctx,
&PulumiResource{URN: urn, Properties: news, Seed: req.RandomSeed},
p.configValues, olds, news, res.TF.Schema(), res.Schema.Fields)
if err != nil {
Expand All @@ -683,6 +683,14 @@ func (p *Provider) Check(ctx context.Context, req *pulumirpc.CheckRequest) (*pul
// Now produce CheckFalures for any properties that failed verification.
failures := p.adaptCheckFailures(ctx, urn, false /*isProvider*/, res.TF.Schema(), res.Schema.GetFields(), errs)

// Now re-generate the inputs WITH the TF defaults
inputs, assets, err := MakeTerraformInputs(ctx,
&PulumiResource{URN: urn, Properties: news, Seed: req.RandomSeed},
p.configValues, olds, news, res.TF.Schema(), res.Schema.Fields)
if err != nil {
return nil, err
}

// After all is said and done, we need to go back and return only what got populated as a diff from the origin.
pinputs := MakeTerraformOutputs(p.tf, inputs, res.TF.Schema(), res.Schema.Fields, assets, false, p.supportsSecrets)

Expand Down
Loading

0 comments on commit 2bb6194

Please sign in to comment.