Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure resource private data is carried through the entire resource lifecycle #21611

Merged
merged 6 commits into from
Jun 5, 2019

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Jun 5, 2019

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 ReadResource call. While not currently used, it makes the use of the data more consistent, and would allow for more flexibility with future providers.

Fixes #21528

jbardin added 2 commits June 3, 2019 18:01
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.
@jbardin jbardin requested a review from a team June 5, 2019 15:16

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 link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just A Comment: only 20m 😭

@@ -181,14 +184,17 @@ func (obj *ResourceInstanceObject) DeepCopy() *ResourceInstanceObject {

var private []byte
if obj.Private != nil {
private := make([]byte, len(obj.Private))
private = make([]byte, len(obj.Private))
copy(private, obj.Private)
}

// Some addrs.Referencable implementations are technically mutable, but
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo fix opportunity? addrs.Referenceable (if we apply the "while we're in the neighborhood" rule)

Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jbardin force-pushed the jbardin/private-data-read branch 2 times, most recently from 2315561 to 750e7e5 Compare June 5, 2019 18:20
@jbardin
Copy link
Member Author

jbardin commented Jun 5, 2019

@pselle,

Private here isn't meant to be secure, it's provider data that is "private" from terraform core, meaning that core will not attempt to interpret or alter the data. The contract here is that if a provider returns something in Private, it expects to get that same data back in the next api call concerning the same resource.

@@ -266,7 +267,7 @@ func testState() *states.State {
Type: "test",
}.Absolute(addrs.RootModuleInstance),
)
})
}).DeepCopy()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jbardin force-pushed the jbardin/private-data-read branch from 750e7e5 to 5b1dcd1 Compare June 5, 2019 23:21
jbardin added 4 commits June 5, 2019 19:22
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 jbardin force-pushed the jbardin/private-data-read branch from 5b1dcd1 to 4cb6ebe Compare June 5, 2019 23:22
@jbardin jbardin merged commit e71e3d8 into master Jun 5, 2019
@jbardin jbardin deleted the jbardin/private-data-read branch June 5, 2019 23:36
@ghost
Copy link

ghost commented Jul 25, 2019

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 ghost locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v0.12: Resource timeouts being ignored on destroy operations
3 participants