apply schema marks to returned instance values #34567
Conversation
The original sensitivity handling implementation applied the marks from a resource schema only when decoding values for evaluation. This appeared to work in most cases, since the resource value could only be used elsewhere in the configuration by references that would be returned from the evaluator. However, this approach left holes in the handling of the instance values where some marks may have been inserted from its own references to sensitive value, but the instance's own sensitive attributes were left unmarked. This was most noticeable in the state storage, where only sensitive references were listed in `sensitive_attributes`, leaving the actual sensitive attributes unmarked. The same problems would arise in the plan artifacts too, with the change `After` values missing attributes that should be sensitive, and the correction to the sensitive attributes depending on being decoded again during evaluation. Rather than depending on a value to be round-tripped through an encoding/decoding cycle via evaluation in order to be correctly marked, we need to apply the schema marks to all new values returned by a provider. This is added to the `ReadResource`, and `PlanResourceChange` paths to ensure values are consistently marked. We can then remove the re-insertion of the schema marks from the evaluator, because the value it sees should be correctly marked in all cases. Notably we don't need to do this for `Import`, because that value is never stored or referenced, -- an `ImportResource` call always consists of a followup `ReadResource` call, and there is no need to strip and re-apply the same marks. We also don't need to separately apply the schema marks after `ApplyResourceChange`, because they are the same marks we already stored in the plan `After` value (which should always be correct now that we insert them after `PlanResourceChange`).
|
This seems to fix #34323 in my local testing. |
There was a problem hiding this comment.
This seems plausible to me!
I'm also pleased to see that this explained what was going on with one of the tests that could only pass while written "incorrectly". I remember encountering that and being pretty concerned that it was masking a bug, and here it is!
|
Oops, meant to make this a draft while until I chased down all the other tests that compare marks in state and plans. :D Thanks @alisdair! I knew that issue was somewhere, but couldn't find it! |
Add the missing sensitive_attributes to the test state, and update the output to reflect the new `"checks"` attribute name.
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
The original sensitivity handling implementation applied the marks from a resource schema only when decoding values for evaluation. This appeared to work in most cases, since the resource value could only be used elsewhere in the configuration by references that would be returned from the evaluator. However, this approach left holes in the handling of the instance values where some marks may have been inserted from its own references to sensitive value, but the instance's own sensitive attributes were left unmarked. This was most noticeable in the state storage, where only sensitive references were listed in
sensitive_attributes, leaving the actual sensitive attributes unmarked.The same problems would arise in the plan artifacts too, with the change
Aftervalues missing attributes that should be sensitive, and the correction to those sensitive attributes depending on being decoded again during evaluation. This would appear to users as unexpected updates in a plan with no changes, usually when the provider added a sensitive attribute, or the instance was recently imported.Rather than depending on a value to be round-tripped through an encoding/decoding cycle via evaluation in order to be correctly marked, we need to apply the schema marks to all new values returned by a provider. This is added to the
ReadResource, andPlanResourceChangepaths to ensure values are consistently marked. We can then remove the re-insertion of the schema marks from the evaluator, because the value it sees should be correctly marked in all cases.Notably we don't need to do this for
Import, because that value is never stored or referenced, -- anImportResourcecall always consists of a followupReadResourcecall, and there is no need to strip and re-apply the same marks. We also don't need to separately apply the schema marks afterApplyResourceChange, because they are the same marks we already stored in the planAftervalue (which should always be correct now that we insert them afterPlanResourceChange).Finally, even though we rely on the stored state to be correctly marked, this should not affect normal usage when the user has a state missing the
sensitive_attributes, because the plan will compare the newly refreshed state with the new planned state, which will both be correctly marked. There will however be an empty update to resources when using-refresh=falseor-refresh-onlywhich should be called out in the upgrade notes. If that is deemed too intrusive, we could insert an extra addition of marks to state decoding, but I opted not to yet as that is apt to hide any other mis-handling of marks.There were a number of tests written to match the inconsistent marks handling which needed to be updated, but that provided some good locations to double check the new behavior using those same tests.
Fixes: #34144
Fixes: #34323