Make sure resource private data is carried through the entire resource lifecycle#21611
Make sure resource private data is carried through the entire resource lifecycle#21611
Conversation
Private data was previously created during Plan, and sent back to the provider during Apply. This data also needs to be persisteded accross Read calls, but rather than rely on core for that we can send the data to the provider during Read to allow for more flexibilty.
Send Private data blob through ReadResource as well. This will allow for extra flexibility for future providers that may want to pass data out of band through to their resource Read functions.
|
|
||
| func TestResourceTimeout_delete(t *testing.T) { | ||
| // If the delete timeout isn't saved until destroy, the cleanup here will | ||
| // fail because the default is only 20m. |
| copy(private, obj.Private) | ||
| } | ||
|
|
||
| // Some addrs.Referencable implementations are technically mutable, but |
There was a problem hiding this comment.
Typo fix opportunity? addrs.Referenceable (if we apply the "while we're in the neighborhood" rule)
pselle
left a comment
pselle
left a comment
There was a problem hiding this comment.
Comment-y comments, I think I would need a talk-through to properly understand intent here (and changes to private data raise a security?? brain flag for me)
jbardin
force-pushed
the
jbardin/private-data-read
branch
2 times, most recently
from
2315561 to
750e7e5
Compare
|
|
| }.Absolute(addrs.RootModuleInstance), | ||
| ) | ||
| }) | ||
| }).DeepCopy() |
There was a problem hiding this comment.
This DeepCopy made me 🤔 a bunch because there's no other variable holding a reference to the state we built here and thus the copying seems superfluous.
However, reading down to see the other changes below I assume the goal here is to make sure that all of the data in the state makes it into the copy, and that makes sense. For the benefit of a future maintainer who won't have the benefit of seeing this line in the context of this PR diff, perhaps a short comment explaining that this is here to implicitly test DeepCopy would be helpful so it doesn't get cleaned up.
jbardin
force-pushed
the
jbardin/private-data-read
branch
from
750e7e5 to
5b1dcd1
Compare
Fix the scope of the private data copy in DeepCopy. Make sure Dependencies matches nil vs empty so that Equal compares correctly between copied states
The timeout value is still not being persisted. While it doesn't fix this issue, make sure to always return Private during Plan.
Makre sure private data is maintained all the way to destroy. This slipped through, since private data isn't used much for current providers, except for timeouts.
The private->meta data was lost in the test harness.
jbardin
force-pushed
the
jbardin/private-data-read
branch
from
5b1dcd1 to
4cb6ebe
Compare
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
3 participants
The Private data was lost in a couple places, and not detected due to the Private data not being used heavily by the current SDK.
This also adds the Private data to the
ReadResourcecall. While not currently used, it makes the use of the data more consistent, and would allow for more flexibility with future providers.Fixes #21528