[TechDebt]: Investigate unreturned sdkdiags.AppendErrorf function calls

[jar-b]

Description

In most cases, calls to sdkdiags.AppendErrorf include an unrecoverable error which should cause the containing CRUD method to return. In some instances the return has been unintentionally omitted, causing unexpected behavior (see #38353). We should investigate additional places where calls to this function are unreturned to ensure this behavior is intentional and correct.

% rg "sdkdiag\.AppendErrorf" internal/service/*/*.go | rg -v "return"
internal/service/workspaces/ip_group.go:                        diags = sdkdiag.AppendErrorf(diags, "describing WorkSpaces Directories: %s", err)
internal/service/workspaces/ip_group.go:                                                diags = sdkdiag.AppendErrorf(diags, "disassociating WorkSpaces IP Group (%s) from WorkSpaces Directory (%s): %s", d.Id(), aws.ToString(dir.DirectoryId), err)
internal/service/ssm/patch_baseline.go:         diags = sdkdiag.AppendErrorf(diags, "deleting SSM Patch Baseline (%s): %s", d.Id(), err)
internal/service/rds/cluster.go:                        diags = sdkdiag.AppendErrorf(diags, `"master_username": required field is not set`)
internal/service/rolesanywhere/trust_anchor.go:                                 sdkdiag.AppendErrorf(diags, "enabling RolesAnywhere Trust Anchor (%s): %s", d.Id(), err)
internal/service/rolesanywhere/trust_anchor.go:                                 sdkdiag.AppendErrorf(diags, "disabling RolesAnywhere Trust Anchor (%s): %s", d.Id(), err)
internal/service/s3/bucket_lifecycle_configuration.go:          sdkdiag.AppendErrorf(diags, "waiting for S3 Bucket Lifecycle Configuration (%s) create: %s", d.Id(), err)
internal/service/s3/bucket_lifecycle_configuration.go:          sdkdiag.AppendErrorf(diags, "waiting for S3 Bucket Lifecycle Configuration (%s) update: %s", d.Id(), err)
internal/service/rds/instance.go:                       diags = sdkdiag.AppendErrorf(diags, `"allocated_storage": required field is not set`)
internal/service/rds/instance.go:                       diags = sdkdiag.AppendErrorf(diags, `"engine": required field is not set`)
internal/service/rds/instance.go:                       diags = sdkdiag.AppendErrorf(diags, `"username": required field is not set`)
internal/service/rds/instance.go:                       diags = sdkdiag.AppendErrorf(diags, `"allocated_storage": required field is not set`)
internal/service/rds/instance.go:                       diags = sdkdiag.AppendErrorf(diags, `"engine": required field is not set`)
internal/service/rds/instance.go:                       diags = sdkdiag.AppendErrorf(diags, `"username": required field is not set`)
internal/service/rds/instance.go:                                       diags = sdkdiag.AppendErrorf(diags, "updating RDS DB Instance (%s): deleting Blue/Green Deployment: %s", d.Get(names.AttrIdentifier).(string), err)
internal/service/rds/instance.go:                                               diags = sdkdiag.AppendErrorf(diags, "updating RDS DB Instance (%s): deleting Blue/Green Deployment: waiting for completion: %s", d.Get(names.AttrIdentifier).(string), err)
internal/service/rds/instance.go:                                       diags = sdkdiag.AppendErrorf(diags, "updating RDS DB Instance (%s): deleting Blue/Green Deployment source: waiting for completion: %s", d.Get(names.AttrIdentifier).(string), err)
internal/service/rolesanywhere/profile.go:                              sdkdiag.AppendErrorf(diags, "enabling RolesAnywhere Profile (%s): %s", d.Id(), err)
internal/service/rolesanywhere/profile.go:                              sdkdiag.AppendErrorf(diags, "disabling RolesAnywhere Profile (%s): %s", d.Id(), err)
internal/service/ecs/cluster_capacity_providers.go:             sdkdiag.AppendErrorf(diags, "[WARN] ECS Cluster Capacity Providers (%s) not found, removing from state", d.Id())
internal/service/eks/addon.go:          diags = sdkdiag.AppendErrorf(diags, "waiting for EKS Add-On (%s) create: %s", d.Id(), err)
internal/service/fms/policy.go:         sdkdiag.AppendErrorf(diags, "setting exclude_map: %s", err)
internal/service/fms/policy.go:         sdkdiag.AppendErrorf(diags, "setting include_map: %s", err)
internal/service/fms/policy.go:         sdkdiag.AppendErrorf(diags, "setting resource_tags: %s", err)
internal/service/fms/policy.go:         sdkdiag.AppendErrorf(diags, "setting security_service_policy_data: %s", err)
internal/service/controltower/landing_zone.go:          sdkdiag.AppendErrorf(diags, "waiting for ControlTower Landing Zone (%s) delete: %s", d.Id(), err)
internal/service/elbv2/trust_store_revocation.go:               sdkdiag.AppendErrorf(diags, "creating ELBv2 Trust Store (%s) Revocation (s3://%s/%s): %s", trustStoreARN, s3Bucket, s3Key, err)
internal/service/acm/certificate_data_source.go:                sdkdiag.AppendErrorf(diags, "XXX no ACM Certificate matching domain (%s)", domain)
internal/service/comprehend/document_classifier.go:             diags = sdkdiag.AppendErrorf(diags, "waiting for Amazon Comprehend Document Classifier (%s) %s: %s", d.Id(), tobe, err)
internal/service/appstream/stack.go:                    sdkdiag.AppendErrorf(diags, "updating Appstream Stack (%s): %s", d.Id(), err)
internal/service/comprehend/entity_recognizer.go:               diags = sdkdiag.AppendErrorf(diags, "waiting for Amazon Comprehend Entity Recognizer (%s) %s: %s", d.Id(), tobe, err)

• internal/service/workspaces/ip_group.go
• internal/service/ssm/patch_baseline.go
• internal/service/rds/cluster.go
• internal/service/rolesanywhere/trust_anchor.go
• internal/service/s3/bucket_lifecycle_configuration.go
• internal/service/rds/instance.go
• internal/service/rolesanywhere/profile.go
• internal/service/ecs/cluster_capacity_providers.go
• internal/service/eks/addon.go
• internal/service/fms/policy.go
• internal/service/controltower/landing_zone.go
• internal/service/elbv2/trust_store_revocation.go
• internal/service/acm/certificate_data_source.go
• internal/service/comprehend/document_classifier.go
• internal/service/appstream/stack.go
• internal/service/comprehend/entity_recognizer.go

References

Relates #38756

Would you like to implement a fix?

None

[github-actions]

Community Note

Voting for Prioritization

• Please vote on this issue by adding a 👍 reaction to the original post to help the community and maintainers prioritize this request.
• Please see our prioritization guide for information on how we prioritize.
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

Volunteering to Work on This Issue

• If you are interested in working on this issue, please leave a comment.
• If this would be your first contribution, please review the contribution guide.

[jar-b]

internal/service/workspaces/ip_group.go usage is fine, as the diags are collected and eventually returned.

terraform-provider-aws/internal/service/workspaces/ip_group.go

Lines 153 to 181 in 394da59

	for paginator.HasMorePages() {
	out, err := paginator.NextPage(ctx)
	if err != nil {
	diags = sdkdiag.AppendErrorf(diags, "describing WorkSpaces Directories: %s", err)
	}
	for _, dir := range out.Directories {
	for _, ipg := range dir.IpGroupIds {
	groupID := ipg
	if groupID == d.Id() {
	found = true
	log.Printf("[DEBUG] WorkSpaces IP Group (%s) associated with WorkSpaces Directory (%s), disassociating", groupID, aws.ToString(dir.DirectoryId))
	_, err := conn.DisassociateIpGroups(ctx, &workspaces.DisassociateIpGroupsInput{
	DirectoryId: dir.DirectoryId,
	GroupIds: []string{d.Id()},
	})
	if err != nil {
	diags = sdkdiag.AppendErrorf(diags, "disassociating WorkSpaces IP Group (%s) from WorkSpaces Directory (%s): %s", d.Id(), aws.ToString(dir.DirectoryId), err)
	continue
	}
	log.Printf("[INFO] WorkSpaces IP Group (%s) disassociated from WorkSpaces Directory (%s)", d.Id(), aws.ToString(dir.DirectoryId))
	}
	}
	}
	}
	if diags.HasError() {
	return diags
	}

[jar-b]

internal/service/ssm/patch_baseline.go is valid:

terraform-provider-aws/internal/service/ssm/patch_baseline.go

Lines 434 to 438 in 394da59

	if err != nil {
	diags = sdkdiag.AppendErrorf(diags, "deleting SSM Patch Baseline (%s): %s", d.Id(), err)
	}
	return diags

[jar-b]

internal/service/rds/cluster.go is valid:

terraform-provider-aws/internal/service/rds/cluster.go

Lines 772 to 777 in 394da59

	if _, ok := d.GetOk("master_username"); !ok {
	diags = sdkdiag.AppendErrorf(diags, `"master_username": required field is not set`)
	}
	if diags.HasError() {
	return diags
	}

[jar-b]

After investigating patterns in the remaining files where the result of this function is re-written to the diags var (ie. diags = sdkdiag.AppendErrorf(diags, ...) ), these patterns all appear to be valid. I'm going to refine the original grep to exclude these cases and focus on instances which neither return nor set the result to the existing diags variable.

% rg "sdkdiag\.AppendErrorf" internal/service/*/*.go | rg -v "return" | rg -v "diags \= sdkdiag\.AppendErrorf"
internal/service/rolesanywhere/profile.go:                              sdkdiag.AppendErrorf(diags, "enabling RolesAnywhere Profile (%s): %s", d.Id(), err)
internal/service/rolesanywhere/profile.go:                              sdkdiag.AppendErrorf(diags, "disabling RolesAnywhere Profile (%s): %s", d.Id(), err)
internal/service/s3/bucket_lifecycle_configuration.go:          sdkdiag.AppendErrorf(diags, "waiting for S3 Bucket Lifecycle Configuration (%s) create: %s", d.Id(), err)
internal/service/s3/bucket_lifecycle_configuration.go:          sdkdiag.AppendErrorf(diags, "waiting for S3 Bucket Lifecycle Configuration (%s) update: %s", d.Id(), err)
internal/service/rolesanywhere/trust_anchor.go:                                 sdkdiag.AppendErrorf(diags, "enabling RolesAnywhere Trust Anchor (%s): %s", d.Id(), err)
internal/service/rolesanywhere/trust_anchor.go:                                 sdkdiag.AppendErrorf(diags, "disabling RolesAnywhere Trust Anchor (%s): %s", d.Id(), err)
internal/service/fms/policy.go:         sdkdiag.AppendErrorf(diags, "setting exclude_map: %s", err)
internal/service/fms/policy.go:         sdkdiag.AppendErrorf(diags, "setting include_map: %s", err)
internal/service/fms/policy.go:         sdkdiag.AppendErrorf(diags, "setting resource_tags: %s", err)
internal/service/fms/policy.go:         sdkdiag.AppendErrorf(diags, "setting security_service_policy_data: %s", err)
internal/service/ecs/cluster_capacity_providers.go:             sdkdiag.AppendErrorf(diags, "[WARN] ECS Cluster Capacity Providers (%s) not found, removing from state", d.Id())
internal/service/controltower/landing_zone.go:          sdkdiag.AppendErrorf(diags, "waiting for ControlTower Landing Zone (%s) delete: %s", d.Id(), err)
internal/service/appstream/stack.go:                    sdkdiag.AppendErrorf(diags, "updating Appstream Stack (%s): %s", d.Id(), err)
internal/service/acm/certificate_data_source.go:                sdkdiag.AppendErrorf(diags, "XXX no ACM Certificate matching domain (%s)", domain)

[github-actions]

Warning

This issue has been closed, meaning that any additional comments are hard for our team to see. Please assume that the maintainers will not see them.

Ongoing conversations amongst community members are welcome, however, the issue will be locked after 30 days. Moving conversations to another venue, such as the AWS Provider forum, is recommended. If you have additional concerns, please open a new issue, referencing this one where needed.

[github-actions]

This functionality has been released in v5.63.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[github-actions]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.