Skip to content

Validate identity in import response #36893

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 1 commit into from
Apr 29, 2025
Merged

Validate identity in import response #36893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
58 changes: 58 additions & 0 deletions internal/terraform/context_plan_import_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2217,3 +2217,61 @@ func TestContext2Plan_importIdentityModuleWithOptional(t *testing.T) {
tfdiags.ObjectToString(wantIdentity))
}
}

func TestContext2Plan_importIdentityMissingResponse(t *testing.T) {
p := testProvider("aws")
m := testModule(t, "import-identity-module")

p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&providerSchema{
ResourceTypes: map[string]*configschema.Block{
"aws_lb": {
Attributes: map[string]*configschema.Attribute{
"id": {
Type: cty.String,
Computed: true,
},
},
},
},
IdentityTypes: map[string]*configschema.Object{
"aws_lb": {
Attributes: map[string]*configschema.Attribute{
"name": {
Type: cty.String,
Required: true,
},
},
Nesting: configschema.NestingSingle,
},
},
})
p.ImportResourceStateResponse = &providers.ImportResourceStateResponse{
ImportedResources: []providers.ImportedResource{
{
TypeName: "aws_lb",
State: cty.ObjectVal(map[string]cty.Value{
"id": cty.StringVal("foo"),
}),
// No identity returned
},
},
}
ctx := testContext2(t, &ContextOpts{
Providers: map[addrs.Provider]providers.Factory{
addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p),
},
})

diags := ctx.Validate(m, &ValidateOpts{})
if diags.HasErrors() {
t.Fatalf("unexpected errors\n%s", diags.Err().Error())
}

_, diags = ctx.Plan(m, states.NewState(), DefaultPlanOpts)
if !diags.HasErrors() {
t.Fatal("succeeded; want errors")
}
if got, want := diags.Err().Error(), `import of aws_lb.foo didn't return an identity`; !strings.Contains(got, want) {
t.Fatalf("wrong error:\ngot: %s\nwant: message containing %q", got, want)
}
}
7 changes: 6 additions & 1 deletion internal/terraform/node_resource_plan_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,12 @@ func (n *NodePlannableResourceInstance) importState(ctx EvalContext, addr addrs.
return nil, deferred, diags
}

// Providers are supposed to return an identity when importing by identity
if importTarget.Type().IsObjectType() && imported[0].Identity.IsNull() {
Copy link
Member

@SarahFrench SarahFrench Apr 16, 2025

Choose a reason for hiding this comment

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

👋🏻 I'm dipping my toe into Resource Identity stuff as this PR looked manageable for a newbie.

At this point could the importType still be either "ID" or "Identity"? If Terraform was trying to import by ID, would the identity being null still be a problem here?

Copy link
Member

@SarahFrench SarahFrench Apr 16, 2025

Choose a reason for hiding this comment

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

Basically I'm wondering if the condition also needs importType == "Identity" in it

Copy link
Member Author

@dbanck dbanck Apr 16, 2025

Choose a reason for hiding this comment

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

We're doing the same check (importTarget.Type().IsObjectType()) further up to set the value of importType.

So checking importType == "Identity" is equal to checking importTarget.Type().IsObjectType()

Copy link
Member

@SarahFrench SarahFrench Apr 16, 2025

Choose a reason for hiding this comment

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

Ah yes I understand now, thanks! 😅

Copy link
Member Author

@dbanck dbanck Apr 16, 2025

Choose a reason for hiding this comment

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

Do you think changing importTarget.Type().IsObjectType() to importType == "Identity" would make it more readable in this case?

The sprinkled .IsObjectType() checks trough the codebase feel a bit odd to me

Copy link
Member

@SarahFrench SarahFrench Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As someone less familiar with the code I think it'd be clearer if there was something more explicit like importType used in conditional logic, versus the implicit meaning of having object-type or string data as the target.

Maybe the strings "ID" and "Identity" could be made into constants to help with any comparison expressions

Copy link
Member

@jbardin jbardin Apr 16, 2025

Choose a reason for hiding this comment

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

I'm not sure what would be more descriptive if you're unfamiliar with the code. The prior knowledge of knowing the difference in usage of a string vs an object isn't far from knowing the difference between ID and Identity. I'd rather not have multiple fields that need to be updated in concert, since that invites them to end up out of sync, so the other option would probably be a struct containing an enum of the possible types, providing a type declaration with appropriate comments. We already have an ImportTarget type which could be used, though that might have been dropped because it makes some other part of the expansion code simpler.

diags = diags.Append(fmt.Errorf("import of %s didn't return an identity", n.Addr.String()))
return nil, deferred, diags
}

// Providers are supposed to return null values for all write-only attributes
writeOnlyDiags := ephemeral.ValidateWriteOnlyAttributes(
"Import returned a non-null value for a write-only attribute",
Expand Down Expand Up @@ -782,7 +788,6 @@ func (n *NodePlannableResourceInstance) importState(ctx EvalContext, addr addrs.
importType, importValue,
),
))

}

// refresh
Expand Down
Loading