Instrument DecodePropertyMap failures to assist root-causing refresh issues in Plugin Framework #1920
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1920 +/- ##
==========================================
+ Coverage 60.24% 60.30% +0.05%
==========================================
Files 328 328
Lines 44342 44335 -7
==========================================
+ Hits 26715 26736 +21
+ Misses 16131 16100 -31
- Partials 1496 1499 +3 ☔ View full report in Codecov by Sentry. |
| @@ -141,22 +141,17 @@ func (p *provider) readResource( | |||
| return plugin.ReadResult{}, nil | |||
| } | |||
|
|
|||
| // TF interpretes a null new state as an indication that the resource does not | |||
| // exist in the cloud provider. | |||
| newStateNull, err := resp.NewState.IsNull() | |||
There was a problem hiding this comment.
Previously IsNull check was done on the DynamicValue domain, and after the change it's done on the tftypes.Value domain. There's a very slim chance the issue was there, namely that there were values that return IsNull() = false on DynamicValue domain that then translate to Null values on tftypes.Value domain.
| readState, err := parseResourceStateFromTF(ctx, rh, resp.NewState, resp.Private) | ||
| if err != nil { | ||
| return plugin.ReadResult{}, fmt.Errorf("parsing resource state: %w", err) | ||
| } | ||
|
|
||
| readStateMap, err := readState.ToPropertyMap(rh) | ||
| // TF interprets a null new state as an indication that the resource does not exist in the cloud provider. |
There was a problem hiding this comment.
I still believe this is how TF works.
| Private: private, | ||
| }}, nil | ||
| } | ||
| if state == nil { |
There was a problem hiding this comment.
Tightening to also handle physical nulls, probably unnecessary but just in case.
| pv, err := dec.toPropertyValue(v) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if !pv.IsObject() { | ||
| return nil, fmt.Errorf("Expected an Object, got: %v", pv.String()) | ||
| details := fmt.Sprintf(`DecodePropertyMap expected the decoder to return an Object |
There was a problem hiding this comment.
Emitting a little more information in the debug log. Not returning it as an err in case it is printed on stdout since the values may be sensitive.
t0yv0
changed the title
Investigating #1919
DecodePropertyMap called from Read fails because it decoded something to a null but a PropertyMap is expected. It appears it should never happen because Read will not call DecodePropertyMap with a tftypes.Value=nil but instead short-circuit the "resource not found path".
resp.State.RemoveResource(ctx)likewise seems to work correctly.Testing this further, still unable to reproduce the scenario in test. Instead, opting for adding some more debugging instrumentation around the line that emits the error, and doing some conservative code tightening around this code path.