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.

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

TFECO-9206 Add validation to check whether the identity that we're about to send back to Terraform is complete #1460

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+566 −51
Open

TFECO-9206 Add validation to check whether the identity that we're about to send back to Terraform is complete #1460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
160dced
Add support for ResourceIdentity#SchemaFunc to allow providers to sav…
ansgarm Mar 27, 2025
057357d
handle nil ResourceIdentity for SchemaMap() and simplify code
ansgarm Mar 27, 2025
ece69ed
add copyright header
ansgarm Mar 27, 2025
f4995e1
Add validation to check whether the identity that we're about to send…
ansgarm Mar 27, 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
4 changes: 2 additions & 2 deletions helper/schema/core_schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,11 +389,11 @@ func (r *Resource) CoreIdentitySchema() (*configschema.Block, error) {
}

func (r *Resource) coreIdentitySchema() (*configschema.Block, error) {
if r.Identity == nil || r.Identity.Schema == nil {
if r.Identity.SchemaMap() == nil {
return nil, fmt.Errorf("resource does not have an identity schema")
}
// while there is schemaMapWithIdentity, we don't need to use it here
// as we're only interested in the existing CoreConfigSchema() method
// to convert our schema
return schemaMap(r.Identity.Schema).CoreConfigSchema(), nil
return schemaMap(r.Identity.SchemaMap()).CoreConfigSchema(), nil
}
40 changes: 38 additions & 2 deletions helper/schema/grpc_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -929,6 +929,15 @@ func (s *GRPCProviderServer) ReadResource(ctx context.Context, req *tfprotov5.Re
return resp, nil
}

// validate every attribute exists and is not null
identityAttrs := newIdentityVal.AsValueMap()
for k := range identityBlock.Attributes {
if v, ok := identityAttrs[k]; !ok || v.IsNull() || !v.IsWhollyKnown() {
resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("identity data missing for attribute '%s'. This is a bug in the provider, which should be reported in the provider's own issue tracker.", k))
return resp, nil
}
}
Comment on lines +932 to +939
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we should duplicate this validation in the provider? We know core needs/will have this logic at some point and we don't typically do validation like this in the SDKs. (For example, ReadResource it's invalid to return a NewState that has unknown values, which core validates, but the SDKs do not)

If we do decide that we want to validate this in the providers, we might consider doing it at the terraform-plugin-go level, so it applies to terraform-plugin-framework as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call!


newIdentityMP, err := msgpack.Marshal(newIdentityVal, identityBlock.ImpliedType())
if err != nil {
resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, err)
Expand Down Expand Up @@ -1091,7 +1100,7 @@ func (s *GRPCProviderServer) PlanResourceChange(ctx context.Context, req *tfprot
// TODO: we could error here if a new Diff got no Identity set
}

if diff == nil || (len(diff.Attributes) == 0 && len(diff.Identity) == 0) {
if diff == nil || (len(diff.Attributes) == 0 && res.Identity == nil) {
// schema.Provider.Diff returns nil if it ends up making a diff with no
// changes, but our new interface wants us to return an actual change
// description that _shows_ there are no changes. This is always the
Expand Down Expand Up @@ -1257,12 +1266,26 @@ func (s *GRPCProviderServer) PlanResourceChange(ctx context.Context, req *tfprot
return resp, nil
}

if len(diff.Identity) == 0 {
resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("no identity data found in diff for resource '%s'. This is a bug in the provider, which should be reported in the provider's own issue tracker.", req.TypeName))
return resp, nil
}

newIdentityVal, err := hcl2shim.HCL2ValueFromFlatmap(diff.Identity, identityBlock.ImpliedType())
if err != nil {
resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, err)
return resp, nil
}

// validate every attribute exists and is not null
identityAttrs := newIdentityVal.AsValueMap()
for k := range identityBlock.Attributes {
if v, ok := identityAttrs[k]; !ok || v.IsNull() || !v.IsWhollyKnown() {
resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("identity data missing for attribute '%s'. This is a bug in the provider, which should be reported in the provider's own issue tracker.", k))
return resp, nil
}
}

newIdentityMP, err := msgpack.Marshal(newIdentityVal, identityBlock.ImpliedType())
if err != nil {
resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, err)
Expand Down Expand Up @@ -1475,20 +1498,33 @@ func (s *GRPCProviderServer) ApplyResourceChange(ctx context.Context, req *tfpro
}
resp.Private = meta

// TODO: if schema defines identity, we should error if there's none written to newInstanceState
if res.Identity != nil {
identityBlock, err := s.getResourceIdentitySchemaBlock(req.TypeName)
if err != nil {
resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("getting identity schema failed for resource '%s': %w", req.TypeName, err))
return resp, nil
}

if len(newInstanceState.Identity) == 0 {
resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("no identity data found in diff for resource '%s'. This is a bug in the provider, which should be reported in the provider's own issue tracker.", req.TypeName))
return resp, nil
} // TODO: test

newIdentityVal, err := hcl2shim.HCL2ValueFromFlatmap(newInstanceState.Identity, identityBlock.ImpliedType())
if err != nil {
resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, err)
return resp, nil
}

// validate every attribute exists and is not null
identityAttrs := newIdentityVal.AsValueMap()
for k := range identityBlock.Attributes {
if v, ok := identityAttrs[k]; !ok || v.IsNull() || !v.IsWhollyKnown() {
resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("identity data missing for attribute '%s'. This is a bug in the provider, which should be reported in the provider's own issue tracker.", k))
return resp, nil
}
} // TODO: test

newIdentityMP, err := msgpack.Marshal(newIdentityVal, identityBlock.ImpliedType())
if err != nil {
resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, err)
Expand Down
Loading
Loading