Fix set marshaling. #104
Fix set marshaling. #104
Conversation
|
It appears that this also fixed #99. |
|
Note that I am not 100% confident in this change. I will be testing this locally by building a new AWS provider with these changes and running the |
pgavlin
force-pushed
the
FixSet
branch
2 times, most recently
from
76123f1 to
405e9d5
Compare
|
Great, I was just about to suggest that. I took a look but will review in depth in the AM. 💤 |
|
|
|
The |
|
LGTM. Can you add a basic test for this too? (I'm fine merging first.) |
pkg/tfbridge/schema.go
Outdated
| @@ -391,9 +393,78 @@ func MakeTerraformAttributesFromRPC(res *PulumiResource, m *pbstruct.Struct, | |||
| return MakeTerraformAttributes(res, props, tfs, ps, defaults) | |||
| } | |||
|
|
|||
| func flattenValue(result map[string]string, prefix string, value interface{}) { | |||
There was a problem hiding this comment.
Maybe a quick // flattenValue ... comment here, describing this function's purpose and its logic?
There was a problem hiding this comment.
also a reference to the corresponding implementation in Terraform we're trying to replicate.
|
|
|
👍 |
| if ps != nil { | ||
| eps = ps.Elem | ||
| } | ||
|
|
There was a problem hiding this comment.
We hoisted this because it's invariant, right? I.e. this wasn't a change in behavior?
There was a problem hiding this comment.
Correct. I can undo this if you'd like; it isn't necessary for this fix. It is an artifact of an earlier attempt.
There was a problem hiding this comment.
Let's take it with this PR but split it into a different commit.
With the recent change to use provider diffs in `Update`, we ceased to
produce usable inputs for sets. The break involves the specific
representation of set members in a `terraform.InstanceState` value: each
set member is flattened into a set of key-value pairs under the key
`prefix.<hash-of-member>.*`. When we were constructing the diffs
ourselves, we were able to ensure that each `<hash-of-member>` component
lined up betweemn the old state and the new config (note that this was
itself perhaps subtly wrong, as TF would likely have considered a change
to a set member to have changed its hash and thus its identity; most
changes of this sort in TF appear as a set member removal and a set member
addition). We lost this capability when we moved to TF diffs: when using
TF diffs, the TF state presented to diff must use TF's typical encoding
for set elements.
These changes reimplement old state -> TF state marshaling s.t. set
elements are flattened with the correct prefix. This turned out to be
much more challenging than it ought to be, as TF does not directly
expose the APIs used to compute set member hashes. At first, I attempted
to use a `MapFieldWriter` to construct the flat map, but this particular
type appears to have some problems handling `TypeMap` values (namely, it
expects each field of a `TypeMap` value to be represented as a
`map[string]interface{}`, which is clearly incorrect). Instead, I chose
to use the TF set member hashing functions directly, which required two
irritating adjustments:
- These hash functions expect any set elements to be represented using
whatever types would have been produced by a `FieldReader`. As a
result, the state goes through the following transforms:
grpc.Struct -> pulumi.PropertyMap -> TF inputs -> TF config
The resulting TF config is then fed into a `ConfigFieldReader`. The
top-level values are read out and flattened in order to produce the TF
attributes.
- These has functions do not produce the exact hash as used by TF: if
the value they produce is negative, TF will invert it s.t. it is
always positive. We do the same.
With these changes, I am able to update a CloudFront distribution's
caching rules (which involves set marshaling) without issue.
With the recent change to use provider diffs in
Update, we ceased toproduce usable inputs for sets. The break involves the specific
representation of set members in a
terraform.InstanceStatevalue: eachset member is flattened into a set of key-value pairs under the key
prefix.<hash-of-member>.*. When we were constructing the diffsourselves, we were able to ensure that each
<hash-of-member>componentlined up betweemn the old state and the new config (note that this was
itself perhaps subtly wrong, as TF would likely have considered a change
to a set member to have changed its hash and thus its identity; most
changes of this sort in TF appear as a set member removal and a set member
addition). We lost this capability when we moved to TF diffs: when using
TF diffs, the TF state presented to diff must use TF's typical encoding
for set elements.
These changes reimplement old state -> TF state marshaling s.t. set
elements are flattened with the correct prefix. This turned out to be
much more challenging than it ought to be, as TF does not directly
expose the APIs used to compute set member hashes. At first, I attempted
to use a
MapFieldWriterto construct the flat map, but this particulartype appears to have some problems handling
TypeMapvalues (namely, itexpects each field of a
TypeMapvalue to be represented as amap[string]interface{}, which is clearly incorrect). Instead, I choseto use the TF set member hashing functions directly, which required two
irritating adjustments:
whatever types would have been produced by a
FieldReader. As aresult, the state goes through the following transforms:
grpc.Struct -> pulumi.PropertyMap -> TF inputs -> TF config
The resulting TF config is then fed into a
ConfigFieldReader. Thetop-level values are read out and flattened in order to produce the TF
attributes.
the value they produce is negative, TF will invert it s.t. it is
always positive. We do the same.
With these changes, I am able to update a CloudFront distribution's
caching rules (which involves set marshaling) without issue.