aws_lambda_function still updates non API properties on update errors

[vtstanescu]

Terraform and AWS Provider Version

OpenTofu v1.9.1
hashicorp/aws v5.98.0

Affected Resource(s) or Data Source(s)

• aws_lambda_function

Expected Behavior

s3_bucket and s3_key are not updated

Actual Behavior

s3_bucket and s3_key are updated

Relevant Error/Panic Output

Error: updating Lambda Function (lambda-function) code: operation error Lambda: UpdateFunctionCode, https response error StatusCode: 403, RequestID: , api error AccessDeniedException: Your access has been denied by S3, please make sure your request credentials have permission to GetObject for binaries-bucket/lambda-package.zip. S3 Error Code: AccessDenied. S3 Error Message: User: arn:aws:sts::123456789012:assumed-role/devops-role/session-name is not authorized to perform: kms:Decrypt on resource: arn:aws:kms:eu-west-1:123456789012:key/mrk-00000000000000000000000000000000 because no identity-based policy allows the kms:Decrypt action

Sample Terraform Configuration

Any aws_lambda_function resource with s3_bucket and s3_key properties will do.

Steps to Reproduce

1. Apply the configuration to create the function
2. Amend the IAM role used to apply the configuration to remove some permission
   required to update the function code, like the kms:Decrypt action for S3
3. Amend the configuration to change the s3_key
4. Try applying the updated configuration
5. Apply fails as your IAM role doesn't have kms:Decrypt required by s3:GetObject
s3_key is still updated in the Terraform config
6. Run terraform plan -> no changes required

Debug Logging

No response

GenAI / LLM Assisted Development

No response

Important Facts and References

Related to #25886, #28963

@ewbankkit I see you addressed this issue in the PR #28963.
Do you recall why the following code snippet was done this way (function.go#978)?

if err != nil {
	if errs.IsAErrorMessageContains[*awstypes.InvalidParameterValueException](err, "Error occurred while GetObject.") {
		// As s3_bucket, s3_key and s3_object_version aren't set in resourceFunctionRead(), don't ovewrite the last known good values.
		for _, key := range []string{names.AttrS3Bucket, "s3_key", "s3_object_version"} {
			old, _ := d.GetChange(key)
			d.Set(key, old)
		}
	}

	return sdkdiag.AppendErrorf(diags, "updating Lambda Function (%s) code: %s", d.Id(), err)
}

I think the setting of the old values for s3_bucket, s3_key, and s3_object_version should happen regardless of what the error was, but maybe you covered some scenarios I'm missing.

Would you like to implement a fix?

Yes

[github-actions]

Community Guidelines

This comment is added to every new Issue to provide quick reference to how the Terraform AWS Provider is maintained. Please review the information below, and thank you for contributing to the community that keeps the provider thriving! 🚀

Voting for Prioritization

• Please vote on this Issue by adding a 👍 reaction to the original post to help the community and maintainers prioritize it.
• Please see our prioritization guide for additional information on how the maintainers handle prioritization.
• Please do not leave +1 or other comments that do not add relevant new information or questions; they generate extra noise for others following the Issue 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.
• For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.

[tabito-hara]

@vtstanescu

Your issue seems similar to my PR #42829, which focuses on source_code_hash, another non-API attribute.
As with my PR for source_code_hash, I believe that s3_bucket, s3_key, and s3_object_version should be reset immediately before returning with sdkdiags.AppendError, and this reset should be applied at each return point where an error may occur—until UpdateFunctionCode succeeds.

(Actually, I also wondered whether the code you pointed out would work correctly when I was implementing my PR. Non-API attributes, which are not refreshed via AWS API calls, should be reset whenever an update fails. Therefore, the check for errs.IsAErrorMessageContains[*awstypes.InvalidParameterValueException] may not be necessary. Also, since some operations are performed before UpdateFunctionCode, these attributes should be reset whenever any of those operations fail.)

If my assumption is correct, the reset of s3_bucket, s3_key, and s3_object_version can be incorporated into the reset function I implemented in the PR above, and I will update the PR accordingly.

[vtstanescu]

Agree, @tabito-hara. Seems like all non-API attributes, at least those related to function code, should be reset as in your PR.

[tabito-hara]

@vtstanescu

Thank you for your response.

I will update my PR #42829 to incorporate other Non-API attributes such as s3_bucket, s3_key, and s3_object_version into the reset function.

[tabito-hara]

I’ve updated PR #42829 to incorporate additional non-API attributes, such as s3_bucket, s3_key, and s3_object_version, into the reset function. I also added an acceptance test to verify that a non-empty plan is still generated even when the update operation fails.
The PR is now ready for review again.