Skip to content

Commit 622c322

Browse files
austinvalleaustinvalle
committed
add private key to import and validation to read
1 parent bcd99f0 commit 622c322

13 files changed

+167
-38
lines changed

‎internal/fwserver/server_applyresourcechange_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1840,9 +1840,9 @@ func TestServerApplyResourceChange(t *testing.T) {
18401840
Diagnostics: diag.Diagnostics{
18411841
diag.NewErrorDiagnostic(
18421842
"Unexpected Identity Change",
1843-
"During the apply operation, the Terraform Provider unexpectedly returned a different identity then the previously stored one.\n\n"+
1843+
"During the update operation, the Terraform Provider unexpectedly returned a different identity then the previously stored one.\n\n"+
18441844
"This is always a problem with the provider and should be reported to the provider developer.\n\n"+
1845-
"Planned Identity: tftypes.Object[\"test_id\":tftypes.String]<\"test_id\":tftypes.String<\"id-123\">>\n"+
1845+
"Planned Identity: tftypes.Object[\"test_id\":tftypes.String]<\"test_id\":tftypes.String<\"id-123\">>\n\n"+
18461846
"New Identity: tftypes.Object[\"test_id\":tftypes.String]<\"test_id\":tftypes.String<\"new-id-123\">>",
18471847
),
18481848
},

‎internal/fwserver/server_importresourcestate.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,12 @@ func (s *Server) ImportResourceState(ctx context.Context, req *ImportResourceSta
205205

206206
private := &privatestate.Data{}
207207

208+
// Set an internal private field that will get sent alongside the imported resource. This will be cleared by
209+
// the following ReadResource RPC and is primarily used to control validation of resource identities during refresh.
210+
private.Framework = map[string][]byte{
211+
privatestate.ImportBeforeReadKey: []byte(`true`), // The actual data isn't important, we just use the map key to detect it.
212+
}
213+
208214
if importResp.Private != nil {
209215
private.Provider = importResp.Private
210216
}

‎internal/fwserver/server_importresourcestate_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,12 +166,18 @@ func TestServerImportResourceState(t *testing.T) {
166166
testProviderData := privatestate.MustProviderData(context.Background(), testProviderKeyValue)
167167

168168
testPrivate := &privatestate.Data{
169+
Framework: map[string][]byte{
170+
privatestate.ImportBeforeReadKey: []byte(`true`),
171+
},
169172
Provider: testProviderData,
170173
}
171174

172175
testEmptyProviderData := privatestate.EmptyProviderData(context.Background())
173176

174177
testEmptyPrivate := &privatestate.Data{
178+
Framework: map[string][]byte{
179+
privatestate.ImportBeforeReadKey: []byte(`true`),
180+
},
175181
Provider: testEmptyProviderData,
176182
}
177183

‎internal/fwserver/server_planresourcechange.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -385,7 +385,7 @@ func (s *Server) PlanResourceChange(ctx context.Context, req *PlanResourceChange
385385
"Unexpected Identity Change",
386386
"During the planning operation, the Terraform Provider unexpectedly returned a different identity then the previously stored one.\n\n"+
387387
"This is always a problem with the provider and should be reported to the provider developer.\n\n"+
388-
fmt.Sprintf("Prior Identity: %s\n", req.PriorIdentity.Raw.String())+
388+
fmt.Sprintf("Prior Identity: %s\n\n", req.PriorIdentity.Raw.String())+
389389
fmt.Sprintf("Planned Identity: %s", resp.PlannedIdentity.Raw.String()),
390390
)
391391

‎internal/fwserver/server_planresourcechange_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4077,7 +4077,7 @@ func TestServerPlanResourceChange(t *testing.T) {
40774077
"Unexpected Identity Change",
40784078
"During the planning operation, the Terraform Provider unexpectedly returned a different identity then the previously stored one.\n\n"+
40794079
"This is always a problem with the provider and should be reported to the provider developer.\n\n"+
4080-
"Prior Identity: tftypes.Object[\"test_id\":tftypes.String]<\"test_id\":tftypes.String<\"id-123\">>\n"+
4080+
"Prior Identity: tftypes.Object[\"test_id\":tftypes.String]<\"test_id\":tftypes.String<\"id-123\">>\n\n"+
40814081
"Planned Identity: tftypes.Object[\"test_id\":tftypes.String]<\"test_id\":tftypes.String<\"new-id-123\">>",
40824082
),
40834083
},
@@ -6728,7 +6728,7 @@ func TestServerPlanResourceChange(t *testing.T) {
67286728
"Unexpected Identity Change",
67296729
"During the planning operation, the Terraform Provider unexpectedly returned a different identity then the previously stored one.\n\n"+
67306730
"This is always a problem with the provider and should be reported to the provider developer.\n\n"+
6731-
"Prior Identity: tftypes.Object[\"test_id\":tftypes.String]<\"test_id\":tftypes.String<\"id-123\">>\n"+
6731+
"Prior Identity: tftypes.Object[\"test_id\":tftypes.String]<\"test_id\":tftypes.String<\"id-123\">>\n\n"+
67326732
"Planned Identity: tftypes.Object[\"test_id\":tftypes.String]<\"test_id\":tftypes.String<\"new-id-123\">>",
67336733
),
67346734
},

‎internal/fwserver/server_readresource.go

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package fwserver
55

66
import (
77
"context"
8+
"fmt"
89

910
"github.com/hashicorp/terraform-plugin-go/tftypes"
1011

@@ -110,12 +111,21 @@ func (s *Server) ReadResource(ctx context.Context, req *ReadResourceRequest, res
110111
readReq.Private = privateProviderData
111112
readResp.Private = privateProviderData
112113

114+
readFollowingImport := false
113115
if req.Private != nil {
114116
if req.Private.Provider != nil {
115117
readReq.Private = req.Private.Provider
116118
readResp.Private = req.Private.Provider
117119
}
118120

121+
// This internal private field is set on a resource during ImportResourceState to help framework determine if
122+
// the resource has been recently imported. We only need to read this once, so we immediately clear it after.
123+
_, ok := req.Private.Framework[privatestate.ImportBeforeReadKey]
124+
if ok {
125+
readFollowingImport = true
126+
delete(req.Private.Framework, privatestate.ImportBeforeReadKey)
127+
}
128+
119129
resp.Private = req.Private
120130
}
121131

@@ -162,14 +172,29 @@ func (s *Server) ReadResource(ctx context.Context, req *ReadResourceRequest, res
162172
return
163173
}
164174

165-
if resp.NewIdentity != nil && req.IdentitySchema == nil {
166-
resp.Diagnostics.AddError(
167-
"Unexpected Read Response",
168-
"An unexpected error was encountered when creating the read response. New identity data was returned by the provider read operation, but the resource does not indicate identity support.\n\n"+
169-
"This is always a problem with the provider and should be reported to the provider developer.",
170-
)
175+
if resp.NewIdentity != nil {
176+
if req.IdentitySchema == nil {
177+
resp.Diagnostics.AddError(
178+
"Unexpected Read Response",
179+
"An unexpected error was encountered when creating the read response. New identity data was returned by the provider read operation, but the resource does not indicate identity support.\n\n"+
180+
"This is always a problem with the provider and should be reported to the provider developer.",
181+
)
171182

172-
return
183+
return
184+
}
185+
186+
// If we're refreshing the resource state (excluding a recently imported resource), validate that the new identity isn't changing
187+
if !readFollowingImport && !req.CurrentIdentity.Raw.IsNull() && !req.CurrentIdentity.Raw.Equal(resp.NewIdentity.Raw) {
188+
resp.Diagnostics.AddError(
189+
"Unexpected Identity Change",
190+
"During the read operation, the Terraform Provider unexpectedly returned a different identity then the previously stored one.\n\n"+
191+
"This is always a problem with the provider and should be reported to the provider developer.\n\n"+
192+
fmt.Sprintf("Current Identity: %s\n\n", req.CurrentIdentity.Raw.String())+
193+
fmt.Sprintf("New Identity: %s", resp.NewIdentity.Raw.String()),
194+
)
195+
196+
return
197+
}
173198
}
174199

175200
semanticEqualityReq := SchemaSemanticEqualityRequest{

‎internal/fwserver/server_readresource_test.go

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,13 @@ func TestServerReadResource(t *testing.T) {
198198
Provider: testEmptyProviderData,
199199
}
200200

201+
testPrivateAfterImport := &privatestate.Data{
202+
Framework: map[string][]byte{
203+
privatestate.ImportBeforeReadKey: []byte(`true`),
204+
},
205+
Provider: testEmptyProviderData,
206+
}
207+
201208
testDeferralAllowed := resource.ReadClientCapabilities{
202209
DeferralAllowed: true,
203210
}
@@ -629,13 +636,41 @@ func TestServerReadResource(t *testing.T) {
629636
Private: testEmptyPrivate,
630637
},
631638
},
632-
"response-identity-update": {
639+
"response-identity-valid-update-null-currentidentity": {
640+
server: &fwserver.Server{
641+
Provider: &testprovider.Provider{},
642+
},
643+
request: &fwserver.ReadResourceRequest{
644+
CurrentState: testCurrentState,
645+
IdentitySchema: testIdentitySchema,
646+
Resource: &testprovider.ResourceWithIdentity{
647+
Resource: &testprovider.Resource{
648+
ReadMethod: func(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) {
649+
identityData := struct {
650+
TestID types.String `tfsdk:"test_id"`
651+
}{
652+
TestID: types.StringValue("new-id-123"),
653+
}
654+
655+
resp.Diagnostics.Append(resp.Identity.Set(ctx, &identityData)...)
656+
},
657+
},
658+
},
659+
},
660+
expectedResponse: &fwserver.ReadResourceResponse{
661+
NewState: testCurrentState,
662+
NewIdentity: testNewIdentity,
663+
Private: testEmptyPrivate,
664+
},
665+
},
666+
"response-identity-valid-update-after-import": {
633667
server: &fwserver.Server{
634668
Provider: &testprovider.Provider{},
635669
},
636670
request: &fwserver.ReadResourceRequest{
637671
CurrentState: testCurrentState,
638672
CurrentIdentity: testCurrentIdentity,
673+
Private: testPrivateAfterImport,
639674
IdentitySchema: testIdentitySchema,
640675
Resource: &testprovider.ResourceWithIdentity{
641676
Resource: &testprovider.Resource{
@@ -654,6 +689,48 @@ func TestServerReadResource(t *testing.T) {
654689
},
655690
},
656691
expectedResponse: &fwserver.ReadResourceResponse{
692+
NewState: testCurrentState,
693+
NewIdentity: testNewIdentity,
694+
Private: &privatestate.Data{
695+
Framework: make(map[string][]byte, 0), // Private import key should be cleared
696+
Provider: testEmptyProviderData,
697+
},
698+
},
699+
},
700+
"response-identity-invalid-update": {
701+
server: &fwserver.Server{
702+
Provider: &testprovider.Provider{},
703+
},
704+
request: &fwserver.ReadResourceRequest{
705+
CurrentState: testCurrentState,
706+
CurrentIdentity: testCurrentIdentity,
707+
IdentitySchema: testIdentitySchema,
708+
Resource: &testprovider.ResourceWithIdentity{
709+
Resource: &testprovider.Resource{
710+
ReadMethod: func(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) {
711+
var identityData struct {
712+
TestID types.String `tfsdk:"test_id"`
713+
}
714+
715+
resp.Diagnostics.Append(req.Identity.Get(ctx, &identityData)...)
716+
717+
identityData.TestID = types.StringValue("new-id-123")
718+
719+
resp.Diagnostics.Append(resp.Identity.Set(ctx, &identityData)...)
720+
},
721+
},
722+
},
723+
},
724+
expectedResponse: &fwserver.ReadResourceResponse{
725+
Diagnostics: diag.Diagnostics{
726+
diag.NewErrorDiagnostic(
727+
"Unexpected Identity Change",
728+
"During the read operation, the Terraform Provider unexpectedly returned a different identity then the previously stored one.\n\n"+
729+
"This is always a problem with the provider and should be reported to the provider developer.\n\n"+
730+
"Current Identity: tftypes.Object[\"test_id\":tftypes.String]<\"test_id\":tftypes.String<\"id-123\">>\n\n"+
731+
"New Identity: tftypes.Object[\"test_id\":tftypes.String]<\"test_id\":tftypes.String<\"new-id-123\">>",
732+
),
733+
},
657734
NewState: testCurrentState,
658735
NewIdentity: testNewIdentity,
659736
Private: testEmptyPrivate,

‎internal/fwserver/server_updateresource.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ func (s *Server) UpdateResource(ctx context.Context, req *UpdateResourceRequest,
190190
"Unexpected Identity Change",
191191
"During the update operation, the Terraform Provider unexpectedly returned a different identity then the previously stored one.\n\n"+
192192
"This is always a problem with the provider and should be reported to the provider developer.\n\n"+
193-
fmt.Sprintf("Planned Identity: %s\n", req.PlannedIdentity.Raw.String())+
193+
fmt.Sprintf("Planned Identity: %s\n\n", req.PlannedIdentity.Raw.String())+
194194
fmt.Sprintf("New Identity: %s", resp.NewIdentity.Raw.String()),
195195
)
196196

‎internal/privatestate/data.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,13 @@ import (
1717
"github.com/hashicorp/terraform-plugin-framework/internal/logging"
1818
)
1919

20+
// ImportBeforeReadKey is an internal private field used to indicate that the current resource state and identity
21+
// were provided most recently by the ImportResourceState RPC. This indicates that the state is an import stub and identity
22+
// has not been stored in state yet.
23+
//
24+
// When detected, this key should be cleared before returning from the ReadResource RPC.
25+
var ImportBeforeReadKey = ".import_before_read"
26+
2027
// Data contains private state data for the framework and providers.
2128
type Data struct {
2229
// Potential future usage:

‎internal/proto5server/server_importresourcestate_test.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,10 @@ func TestServerImportResourceState(t *testing.T) {
6060
t.Fatalf("unexpected error calling tfprotov5.NewDynamicValue(): %s", err)
6161
}
6262

63+
testEmptyPrivateBytes := privatestate.MustMarshalToJson(map[string][]byte{
64+
privatestate.ImportBeforeReadKey: []byte(`true`),
65+
})
66+
6367
testSchema := schema.Schema{
6468
Attributes: map[string]schema.Attribute{
6569
"id": schema.StringAttribute{
@@ -130,6 +134,7 @@ func TestServerImportResourceState(t *testing.T) {
130134
{
131135
State: testStateDynamicValue,
132136
TypeName: "test_resource",
137+
Private: testEmptyPrivateBytes,
133138
},
134139
},
135140
},
@@ -183,7 +188,8 @@ func TestServerImportResourceState(t *testing.T) {
183188
expectedResponse: &tfprotov5.ImportResourceStateResponse{
184189
ImportedResources: []*tfprotov5.ImportedResource{
185190
{
186-
State: &testEmptyStateDynamicValue,
191+
State: &testEmptyStateDynamicValue,
192+
Private: testEmptyPrivateBytes,
187193
Identity: &tfprotov5.ResourceIdentityData{
188194
IdentityData: testRequestIdentityValue,
189195
},
@@ -271,6 +277,7 @@ func TestServerImportResourceState(t *testing.T) {
271277
expectedResponse: &tfprotov5.ImportResourceStateResponse{
272278
ImportedResources: []*tfprotov5.ImportedResource{
273279
{
280+
Private: testEmptyPrivateBytes,
274281
State: testStateDynamicValue,
275282
TypeName: "test_resource",
276283
},
@@ -317,7 +324,8 @@ func TestServerImportResourceState(t *testing.T) {
317324
expectedResponse: &tfprotov5.ImportResourceStateResponse{
318325
ImportedResources: []*tfprotov5.ImportedResource{
319326
{
320-
State: &testEmptyStateDynamicValue,
327+
State: &testEmptyStateDynamicValue,
328+
Private: testEmptyPrivateBytes,
321329
Identity: &tfprotov5.ResourceIdentityData{
322330
IdentityData: testImportedResourceIdentityDynamicValue,
323331
},
@@ -366,7 +374,8 @@ func TestServerImportResourceState(t *testing.T) {
366374
State: testStateDynamicValue,
367375
TypeName: "test_resource",
368376
Private: privatestate.MustMarshalToJson(map[string][]byte{
369-
"providerKey": []byte(`{"key": "value"}`),
377+
privatestate.ImportBeforeReadKey: []byte(`true`),
378+
"providerKey": []byte(`{"key": "value"}`),
370379
}),
371380
},
372381
},

0 commit comments

Comments
 (0)