Skip to content

Commit fdeb465

Browse files
mitchellhmitchellh
committed
terraform: deep copy shadow arguments to avoid state modifications
The arguments passed into Apply, Refresh, Diff could be modified which caused the shadow comparison later to cause errors. Also, the result should be deep copied so that it isn't modified.
1 parent 4de8036 commit fdeb465

4 files changed

Lines changed: 138 additions & 13 deletions

File tree

‎terraform/diff.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,16 @@ func (d *InstanceDiff) Empty() bool {
381381
return !d.Destroy && !d.DestroyTainted && len(d.Attributes) == 0
382382
}
383383

384+
// DeepCopy performs a deep copy of all parts of the InstanceDiff
385+
func (d *InstanceDiff) DeepCopy() *InstanceDiff {
386+
copy, err := copystructure.Config{Lock: true}.Copy(d)
387+
if err != nil {
388+
panic(err)
389+
}
390+
391+
return copy.(*InstanceDiff)
392+
}
393+
384394
func (d *InstanceDiff) GoString() string {
385395
return fmt.Sprintf("*%#v", InstanceDiff{
386396
Attributes: d.Attributes,

‎terraform/shadow_components.go‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,14 @@ func (f *shadowComponentFactory) CloseShadow() error {
104104
}
105105
}
106106

107-
// TODO: provisioners once they're done
107+
for _, n := range shared.provisionerKeys {
108+
_, shadow, err := shared.ResourceProvisioner(n, n)
109+
if err == nil && shadow != nil {
110+
if err := shadow.CloseShadow(); err != nil {
111+
result = multierror.Append(result, err)
112+
}
113+
}
114+
}
108115

109116
// Mark ourselves as closed
110117
shared.closed = true
@@ -141,6 +148,15 @@ func (f *shadowComponentFactory) ShadowError() error {
141148
}
142149
}
143150

151+
for _, n := range shared.provisionerKeys {
152+
_, shadow, err := shared.ResourceProvisioner(n, n)
153+
if err == nil && shadow != nil {
154+
if err := shadow.ShadowError(); err != nil {
155+
result = multierror.Append(result, err)
156+
}
157+
}
158+
}
159+
144160
return result
145161
}
146162

@@ -168,7 +184,7 @@ type shadowComponentFactoryProviderEntry struct {
168184

169185
type shadowComponentFactoryProvisionerEntry struct {
170186
Real ResourceProvisioner
171-
Shadow ResourceProvisioner
187+
Shadow shadowResourceProvisioner
172188
Err error
173189
}
174190

@@ -211,7 +227,7 @@ func (f *shadowComponentFactoryShared) ResourceProvider(
211227
}
212228

213229
func (f *shadowComponentFactoryShared) ResourceProvisioner(
214-
n, uid string) (ResourceProvisioner, ResourceProvisioner, error) {
230+
n, uid string) (ResourceProvisioner, shadowResourceProvisioner, error) {
215231
// Determine if we already have a value
216232
raw, ok := f.provisioners.ValueOk(uid)
217233
if !ok {
@@ -227,8 +243,7 @@ func (f *shadowComponentFactoryShared) ResourceProvisioner(
227243

228244
if p != nil {
229245
// For now, just create a mock since we don't support provisioners yet
230-
real := p
231-
shadow := new(MockResourceProvisioner)
246+
real, shadow := newShadowResourceProvisioner(p)
232247
entry.Real = real
233248
entry.Shadow = shadow
234249
}

‎terraform/shadow_resource_provider.go‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -146,11 +146,15 @@ func (p *shadowResourceProviderReal) Apply(
146146
info *InstanceInfo,
147147
state *InstanceState,
148148
diff *InstanceDiff) (*InstanceState, error) {
149+
// Thse have to be copied before the call since call can modify
150+
stateCopy := state.DeepCopy()
151+
diffCopy := diff.DeepCopy()
152+
149153
result, err := p.ResourceProvider.Apply(info, state, diff)
150154
p.Shared.Apply.SetValue(info.HumanId(), &shadowResourceProviderApply{
151-
State: state,
152-
Diff: diff,
153-
Result: result,
155+
State: stateCopy,
156+
Diff: diffCopy,
157+
Result: result.DeepCopy(),
154158
ResultErr: err,
155159
})
156160

@@ -161,11 +165,14 @@ func (p *shadowResourceProviderReal) Diff(
161165
info *InstanceInfo,
162166
state *InstanceState,
163167
desired *ResourceConfig) (*InstanceDiff, error) {
168+
// Thse have to be copied before the call since call can modify
169+
stateCopy := state.DeepCopy()
170+
164171
result, err := p.ResourceProvider.Diff(info, state, desired)
165172
p.Shared.Diff.SetValue(info.HumanId(), &shadowResourceProviderDiff{
166-
State: state,
173+
State: stateCopy,
167174
Desired: desired,
168-
Result: result,
175+
Result: result.DeepCopy(),
169176
ResultErr: err,
170177
})
171178

@@ -175,10 +182,13 @@ func (p *shadowResourceProviderReal) Diff(
175182
func (p *shadowResourceProviderReal) Refresh(
176183
info *InstanceInfo,
177184
state *InstanceState) (*InstanceState, error) {
185+
// Thse have to be copied before the call since call can modify
186+
stateCopy := state.DeepCopy()
187+
178188
result, err := p.ResourceProvider.Refresh(info, state)
179189
p.Shared.Refresh.SetValue(info.HumanId(), &shadowResourceProviderRefresh{
180-
State: state,
181-
Result: result,
190+
State: stateCopy,
191+
Result: result.DeepCopy(),
182192
ResultErr: err,
183193
})
184194

@@ -430,7 +440,7 @@ func (p *shadowResourceProviderShadow) Apply(
430440
if !state.Equal(result.State) {
431441
p.ErrorLock.Lock()
432442
p.Error = multierror.Append(p.Error, fmt.Errorf(
433-
"State had unequal states (real, then shadow):\n\n%#v\n\n%#v",
443+
"Apply: state had unequal states (real, then shadow):\n\n%#v\n\n%#v",
434444
result.State, state))
435445
p.ErrorLock.Unlock()
436446
}

‎terraform/shadow_resource_provider_test.go‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,96 @@ func TestShadowResourceProviderApply(t *testing.T) {
324324
}
325325
}
326326

327+
func TestShadowResourceProviderApply_modifyDiff(t *testing.T) {
328+
mock := new(MockResourceProvider)
329+
real, shadow := newShadowResourceProvider(mock)
330+
331+
// Test values
332+
info := &InstanceInfo{Id: "foo"}
333+
state := &InstanceState{ID: "foo"}
334+
diff := &InstanceDiff{}
335+
mockResult := &InstanceState{ID: "foo"}
336+
337+
// Configure the mock
338+
mock.ApplyFn = func(
339+
info *InstanceInfo,
340+
s *InstanceState, d *InstanceDiff) (*InstanceState, error) {
341+
d.Destroy = true
342+
return s, nil
343+
}
344+
345+
// Call the real func
346+
realResult, realErr := real.Apply(info, state.DeepCopy(), diff.DeepCopy())
347+
if !realResult.Equal(mockResult) {
348+
t.Fatalf("bad: %#v", realResult)
349+
}
350+
if realErr != nil {
351+
t.Fatalf("bad: %#v", realErr)
352+
}
353+
354+
// Verify the shadow returned the same values
355+
result, err := shadow.Apply(info, state.DeepCopy(), diff.DeepCopy())
356+
if !result.Equal(mockResult) {
357+
t.Fatalf("bad: %#v", result)
358+
}
359+
if err != nil {
360+
t.Fatalf("bad: %#v", err)
361+
}
362+
363+
// Verify we have no errors
364+
if err := shadow.CloseShadow(); err != nil {
365+
t.Fatalf("bad: %s", err)
366+
}
367+
if err := shadow.ShadowError(); err != nil {
368+
t.Fatalf("bad: %s", err)
369+
}
370+
}
371+
372+
func TestShadowResourceProviderApply_modifyState(t *testing.T) {
373+
mock := new(MockResourceProvider)
374+
real, shadow := newShadowResourceProvider(mock)
375+
376+
// Test values
377+
info := &InstanceInfo{Id: "foo"}
378+
state := &InstanceState{ID: ""}
379+
diff := &InstanceDiff{}
380+
mockResult := &InstanceState{ID: "foo"}
381+
382+
// Configure the mock
383+
mock.ApplyFn = func(
384+
info *InstanceInfo,
385+
s *InstanceState, d *InstanceDiff) (*InstanceState, error) {
386+
s.ID = "foo"
387+
return s, nil
388+
}
389+
390+
// Call the real func
391+
realResult, realErr := real.Apply(info, state.DeepCopy(), diff)
392+
if !realResult.Equal(mockResult) {
393+
t.Fatalf("bad: %#v", realResult)
394+
}
395+
if realErr != nil {
396+
t.Fatalf("bad: %#v", realErr)
397+
}
398+
399+
// Verify the shadow returned the same values
400+
result, err := shadow.Apply(info, state.DeepCopy(), diff)
401+
if !result.Equal(mockResult) {
402+
t.Fatalf("bad: %#v", result)
403+
}
404+
if err != nil {
405+
t.Fatalf("bad: %#v", err)
406+
}
407+
408+
// Verify we have no errors
409+
if err := shadow.CloseShadow(); err != nil {
410+
t.Fatalf("bad: %s", err)
411+
}
412+
if err := shadow.ShadowError(); err != nil {
413+
t.Fatalf("bad: %s", err)
414+
}
415+
}
416+
327417
func TestShadowResourceProviderDiff(t *testing.T) {
328418
mock := new(MockResourceProvider)
329419
real, shadow := newShadowResourceProvider(mock)

0 commit comments

Comments
 (0)