Skip to content

Relax resource identity validation #36989

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 7, 2025
Merged

Relax resource identity validation #36989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e57a4f7
Remove identity change validation
dbanck May 7, 2025
76b4a64
Remove validation for null checks in identities
dbanck May 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions internal/terraform/context_plan_identity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ func TestContext2Plan_resource_identity_refresh(t *testing.T) {
ExpectedError: fmt.Errorf("failed to upgrade resource identity: provider was unable to do so"),
},
"identity sent to provider differs from returned one": {
// We don't throw an error here, because there are resource types with mutable identities
StoredIdentitySchemaVersion: 0,
StoredIdentityJSON: []byte(`{"id": "foo"}`),
IdentitySchema: providers.IdentitySchema{
Expand All @@ -171,9 +172,8 @@ func TestContext2Plan_resource_identity_refresh(t *testing.T) {
"id": cty.StringVal("bar"),
}),
ExpectedIdentity: cty.ObjectVal(map[string]cty.Value{
"id": cty.StringVal("foo"),
"id": cty.StringVal("bar"),
}),
ExpectedError: fmt.Errorf("Provider produced different identity: Provider \"registry.terraform.io/hashicorp/aws\" returned a different identity for aws_instance.web than the previously stored one. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker."),
},
"identity with unknowns": {
IdentitySchema: providers.IdentitySchema{
Expand Down Expand Up @@ -446,12 +446,13 @@ func TestContext2Plan_resource_identity_plan(t *testing.T) {
},
},
"create - null planned identity schema": {
// We allow null values in identities
plannedIdentity: cty.ObjectVal(map[string]cty.Value{
"id": cty.NullVal(cty.String),
}),
expectDiagnostics: tfdiags.Diagnostics{
tfdiags.Sourceless(tfdiags.Error, "Provider produced an identity that doesn't match the schema", "Provider \"registry.terraform.io/hashicorp/test\" returned an identity for test_resource.test that doesn't match the identity schema: attributes \"id\" are required and must not be null. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker."),
},
expectedIdentity: cty.ObjectVal(map[string]cty.Value{
"id": cty.NullVal(cty.String),
}),
},
"update": {
prevRunState: states.BuildState(func(s *states.SyncState) {
Expand Down Expand Up @@ -536,6 +537,7 @@ func TestContext2Plan_resource_identity_plan(t *testing.T) {
},

"update - changing identity": {
// We don't throw an error here, because there are resource types with mutable identities
prevRunState: states.BuildState(func(s *states.SyncState) {
s.SetResourceInstanceCurrent(
addrs.Resource{
Expand All @@ -559,9 +561,9 @@ func TestContext2Plan_resource_identity_plan(t *testing.T) {
"id": cty.StringVal("foo"),
}),

expectDiagnostics: tfdiags.Diagnostics{
tfdiags.Sourceless(tfdiags.Error, "Provider produced different identity", "Provider \"registry.terraform.io/hashicorp/test\" returned a different identity for test_resource.test than the previously stored one. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker."),
},
expectedIdentity: cty.ObjectVal(map[string]cty.Value{
"id": cty.StringVal("foo"),
}),
},

"update - updating identity schema version": {
Expand Down
42 changes: 0 additions & 42 deletions internal/terraform/node_resource_abstract_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,7 +671,6 @@ func (n *NodeAbstractResourceInstance) refresh(ctx EvalContext, deposedKey state
if !resp.Identity.IsNull() {
diags = diags.Append(n.validateIdentityKnown(resp.Identity))
diags = diags.Append(n.validateIdentity(resp.Identity, schema.Identity))
diags = diags.Append(n.validateIdentityDidNotChange(state, resp.Identity))
}
if resp.Deferred != nil {
deferred = resp.Deferred
Expand Down Expand Up @@ -1122,10 +1121,6 @@ func (n *NodeAbstractResourceInstance) plan(
if !plannedIdentity.IsNull() {
if !action.IsReplace() && action != plans.Create {
diags = diags.Append(n.validateIdentityKnown(plannedIdentity))
// If the identity is not known we can not validate it did not change
if !diags.HasErrors() {
diags = diags.Append(n.validateIdentityDidNotChange(currentState, plannedIdentity))
}
}

diags = diags.Append(n.validateIdentity(plannedIdentity, schema.Identity))
Expand Down Expand Up @@ -2648,9 +2643,6 @@ func (n *NodeAbstractResourceInstance) apply(
if !resp.NewIdentity.IsNull() {
diags = diags.Append(n.validateIdentityKnown(resp.NewIdentity))
diags = diags.Append(n.validateIdentity(resp.NewIdentity, schema.Identity))
if !change.Action.IsReplace() {
diags = diags.Append(n.validateIdentityDidNotChange(state, resp.NewIdentity))
}
}
}
applyDiags := resp.Diagnostics
Expand Down Expand Up @@ -2916,21 +2908,6 @@ func (n *NodeAbstractResourceInstance) validateIdentityKnown(newIdentity cty.Val
return diags
}

func (n *NodeAbstractResourceInstance) validateIdentityDidNotChange(state *states.ResourceInstanceObject, newIdentity cty.Value) (diags tfdiags.Diagnostics) {
if state != nil && !state.Identity.IsNull() && state.Identity.Equals(newIdentity).False() {
diags = diags.Append(tfdiags.Sourceless(
tfdiags.Error,
"Provider produced different identity",
fmt.Sprintf(
"Provider %q returned a different identity for %s than the previously stored one. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.",
n.ResolvedProvider.Provider, n.Addr,
),
))
}

return diags
}

func (n *NodeAbstractResourceInstance) validateIdentity(newIdentity cty.Value, identitySchema *configschema.Object) (diags tfdiags.Diagnostics) {
if _, marks := newIdentity.UnmarkDeep(); len(marks) > 0 {
diags = diags.Append(tfdiags.Sourceless(
Expand Down Expand Up @@ -2972,25 +2949,6 @@ func (n *NodeAbstractResourceInstance) validateIdentity(newIdentity cty.Value, i
return diags
}

// Check for required attributes
names := make([]string, 0, len(identitySchema.Attributes))
for name, attrS := range identitySchema.Attributes {
if attrS.Required && newIdentity.GetAttr(name).IsNull() {
names = append(names, name)
}
}
if len(names) > 0 {
diags = diags.Append(tfdiags.Sourceless(
tfdiags.Error,
"Provider produced an identity that doesn't match the schema",
fmt.Sprintf(
"Provider %q returned an identity for %s that doesn't match the identity schema: attributes %q are required and must not be null. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.",
n.ResolvedProvider.Provider, n.Addr,
strings.Join(names, ", "),
),
))
}

return diags
}

Expand Down
Loading