Skip to content

Commit

Permalink
Remove rawNames from makeObjectTerraformInputs
Browse files Browse the repository at this point in the history 
It never makes sense to pass `rawNames=true` to `makeObjectTerraformInputs` since that
implies we have an object type with properties, but that we should not do property name
translations. This is never the case.

This PR introduces a subtle change in behavior when walking values of unknown
type:

Before this PR, we assumed that an untyped `resource.PropertyMap` was an object (name
translation is performed) but all nested `resource.PropertyMap` values were then assumed
to be a map (name translation is not performed).

After this PR, we treat all untyped `resource.PropertyMap` values as maps. IMO this makes
more sense: the rule is we don't apply name translation unless we know a value is an
object (and can thus generate naming tables for it).

I don't expect that this change will break users since user's should never hit untyped
values. If they do, neither before or after will work consistently.

Addressing the test change:

"object_property_value" was used to test object names but never given a type. In light of
the above change, we needed to make it typed to get the expected name translation.
  • Loading branch information
@iwahbe
iwahbe committed Apr 10, 2024
1 parent 8222ffd commit 2b0bc80
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 51 deletions.
84 changes: 33 additions & 51 deletions pkg/tfbridge/schema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -479,51 +479,40 @@ func (ctx *conversionContext) makeTerraformInput(
oldObject = old.ObjectValue()
}

var input map[string]interface{}
if shimutil.IsOfTypeMap(tfs) {
var tfsElem shim.Schema
if tfs != nil {
if s, ok := tfs.Elem().(shim.Schema); ok {
tfsElem = s
}
}
var psElem *SchemaInfo
if ps != nil {
psElem = ps.Elem
}
var err error
input, err = ctx.makeMapTerraformInputs(oldObject, v.ObjectValue(),
tfsElem, psElem)
if err != nil {
return nil, err
}
} else {
var tfflds shim.SchemaMap
if tfs != nil {
if res, isres := tfs.Elem().(shim.Resource); isres {
tfflds = res.Schema()
}
var tfflds shim.SchemaMap

// We cannot use [shimutil.CastToTypeObject] because we have machinery that constructs invalid
// resource objects, such as [elemSchemas].
if tfs != nil {
if r, ok := tfs.Elem().(shim.Resource); ok {
tfflds = r.Schema()
}
}

if tfflds != nil {
var psflds map[string]*SchemaInfo
if ps != nil {
psflds = ps.Fields
}
var err error
input, err = ctx.makeObjectTerraformInputs(oldObject, v.ObjectValue(),
tfflds, psflds, rawNames)
obj, err := ctx.makeObjectTerraformInputs(oldObject, v.ObjectValue(), tfflds, psflds)
if err != nil {
return nil, err
}
}

// If we have schema information that indicates that this value is being presented to a map-typed field whose
// Elem is a shim.Resource, wrap the value in an array in order to work around a bug in Terraform.
if tfs != nil && tfs.Type() == shim.TypeMap {
if _, hasResourceElem := tfs.Elem().(shim.Resource); hasResourceElem {
return []interface{}{input}, nil
if tfs.Type() == shim.TypeMap {
// If we have schema information that indicates that this value is being
// presented to a map-typed field whose Elem is a shim.Resource, wrap the
// value in an array in order to work around a bug in Terraform.
//
// TODO[pulumi/pulumi-terraform-bridge#1828]: This almost certainly stems from
// a bug in the bridge's implementation and not terraform's implementation.
return []interface{}{obj}, nil
}
return obj, nil
}
return input, nil

etfs, eps := elemSchemas(tfs, ps)
return ctx.makeMapTerraformInputs(oldObject, v.ObjectValue(), etfs, eps)
case v.IsComputed() || v.IsOutput():
// If any variables are unknown, we need to mark them in the inputs so the config map treats it right. This
// requires the use of the special UnknownVariableValue sentinel in Terraform, which is how it internally stores
Expand All @@ -533,7 +522,6 @@ func (ctx *conversionContext) makeTerraformInput(
contract.Failf("Unexpected value marshaled: %v", v)
return nil, nil
}

}

// makeTerraformInputs takes a property map plus custom schema info and does whatever is necessary
Expand All @@ -545,7 +533,7 @@ func (ctx *conversionContext) makeTerraformInputs(
tfs shim.SchemaMap,
ps map[string]*SchemaInfo,
) (map[string]interface{}, error) {
return ctx.makeObjectTerraformInputs(olds, news, tfs, ps, false /*rawNames*/)
return ctx.makeObjectTerraformInputs(olds, news, tfs, ps)
}

// Should only be called from inside makeTerraformInputs. Variation for makeTerraformInputs used
Expand Down Expand Up @@ -591,14 +579,14 @@ func (ctx *conversionContext) makeMapTerraformInputs(
return result, nil
}

// Should only be called from inside makeTerraformInputs. This variation should only be handling the
// case when an object type is expected, or else the schema is lost and the translation is not sure
// of the expected type. The case when map types are expected is handled by makeMapTerraformInputs.
// Should only be called from inside makeTerraformInputs. This variation should only be
// handling the case when an object type is expected. The case when map types are expected
// or the schema is lost and the translation is not sure of the expected type is handled
// by makeMapTerraformInputs.
func (ctx *conversionContext) makeObjectTerraformInputs(
olds, news resource.PropertyMap,
tfs shim.SchemaMap,
ps map[string]*SchemaInfo,
rawNames bool,
) (map[string]interface{}, error) {
result := make(map[string]interface{})
tfAttributesToPulumiProperties := make(map[string]string)
Expand All @@ -612,14 +600,8 @@ func (ctx *conversionContext) makeObjectTerraformInputs(
}

// First translate the Pulumi property name to a Terraform name.
name, tfi, psi := getInfoFromPulumiName(key, tfs, ps, rawNames)

// rawNames=true indicate that we are processing a map[string,X], not an object type, and therefore
// should not be renaming terraform_style names to pulumiStyle names or assuming that the map keys are
// non-empty.
if !rawNames {
contract.Assertf(name != "", `name != ""`)
}
name, tfi, psi := getInfoFromPulumiName(key, tfs, ps, false)
contract.Assertf(name != "", `Object properties cannot be empty`)

if _, duplicate := result[name]; duplicate {
// If multiple Pulumi `key`s map to the same Terraform attribute `name`, then
Expand All @@ -644,7 +626,7 @@ func (ctx *conversionContext) makeObjectTerraformInputs(
}

// And then translate the property value.
v, err := ctx.makeTerraformInput(name, old, value, tfi, psi, rawNames)
v, err := ctx.makeTerraformInput(name, old, value, tfi, psi, false)
if err != nil {
return nil, err
}
Expand All @@ -653,15 +635,15 @@ func (ctx *conversionContext) makeObjectTerraformInputs(
}

// Now enumerate and propagate defaults if the corresponding values are still missing.
if err := ctx.applyDefaults(result, olds, news, tfs, ps, rawNames); err != nil {
if err := ctx.applyDefaults(result, olds, news, tfs, ps, false); err != nil {
return nil, err
}

if tfs != nil && ctx.ApplyMaxItemsOneDefaults {
// Iterate over the TF schema and add an empty array for each nil MaxItemsOne property.
tfs.Range(func(key string, value shim.Schema) bool {
// First do a lookup of the name/info.
_, tfi, psi := getInfoFromTerraformName(key, tfs, ps, rawNames)
_, tfi, psi := getInfoFromTerraformName(key, tfs, ps, false)
if IsMaxItemsOne(tfi, psi) && result[key] == nil {
result[key] = []interface{}{}
}
Expand Down
8 changes: 8 additions & 0 deletions pkg/tfbridge/schema_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,14 @@ func TestTerraformInputs(t *testing.T) {
}),
}).Shim(),
},
"object_property_value": {
Elem: (&schema.Resource{
Schema: schemaMap(map[string]*schema.Schema{
"property_a": {Type: shim.TypeString, Required: true},
"property_b": {Type: shim.TypeBool, Optional: true},
}),
}).Shim(),
},
"map_property_value": {Type: shim.TypeMap},
"nested_resource": {
Type: shim.TypeList,
Expand Down

0 comments on commit 2b0bc80

Please sign in to comment.