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.

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

Workaround attempt for perpetual diff in lambda with vpc_config #16124

Closed

Conversation

maciejp-ro
Copy link

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates #15952

Release note for CHANGELOG:

* resource/aws_lambda_function: workaround for perpetual plan with vpc_config [GH-15952]

WIP: Attempt at a workaround for perpetual plan on aws_lambda_function with vpc_config. #15952 is labeled "good first issue", so I tried.

I tracked the cause to d.HasChange("vpc_config") returning true even if vpc_config did not actually change (two results from GetChange are identical as confirmed by reflect.DeepEqual).

I'm not very familiar with either this provider's internals or mechanics of Terraform plugins in general. This might be a bug in terraform-plugin-sdk (search for HasChange in the SDK repo returns some open issues, but they seem to refer to v1 of the SDK) or some subtle issue in the provider itself. Falling back on reflection to compare values seems to be code smell, but this fixes the issue for me and might provide some hints for a real fix.

I haven't figured out how to update tests yet, I'll try to add a test case later. Publishing a draft in the meantime to gather feedback (or help maintainers with finding a proper fix).

TODO: Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

@ghost ghost added size/XS Managed by automation to categorize the size of a PR. service/lambda Issues and PRs that pertain to the lambda service. labels Nov 10, 2020
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Nov 10, 2020
@gdavison
Copy link
Contributor

gdavison commented Nov 11, 2020

Hi @maciejp-ro, thanks for putting this together. It's helped me do some deeper investigation. It's related to hashicorp/terraform-plugin-sdk#617; a nested structure with a TypeSet element always returns true for HasChange().

As a workaround, checking HasChange on vpc_config.0.subnet_ids and vpc_config.0.security_group_ids might work, but eventually we'll need to make the fix in https://github.com/hashicorp/terraform-plugin-sdk.

if hasVpcConfigChange {
// This returns true when vpc_config is set even when the values are the same
o, n := d.GetChange("vpc_config")
hasVpcConfigChange = reflect.DeepEqual(o, n)
Copy link
Contributor

@gdavison gdavison Nov 11, 2020

Choose a reason for hiding this comment

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

It took me a while to see this, but this is setting hasVpcConfigChange to true when they're DeepEqual. However, since vpc_config contains TypeSet elements, it will always return not equal, because TypeSets are never DeepEqual

Copy link
Author

Choose a reason for hiding this comment

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

I have tested this with a real Terraform project and this works: I don't have a plan with this change when a lambda_function has a vpc_config, and have a plan without that change. Additional debug logs show that hasChange("vpc_config") returns true, and reflect.DeepEqual(o, n) also returns true.

I have also tested it against a real change in vpc_config and made sure it shows up in plan.

However, since there can be only one vpc_config, if hasChange("vpc_config.0.subnet_ids") || hasChange("vpc_config.0.security_group_ids") does the trick, it seems to be a cleaner solution – it doesn't need reflection or conditionals. I'll test it.

@gdavison
Copy link
Contributor

For tests, you could make a copy of TestAccAWSLambdaFunction_VPC and its setup function testAccAWSLambdaConfigWithVPC and add publish = true to the aws_lambda_function. For more details on acceptance tests, see https://github.com/hashicorp/terraform-provider-aws/blob/master/docs/contributing/running-and-writing-acceptance-tests.md.

Base automatically changed from master to main January 23, 2021 00:59
@ewbankkit
Copy link
Contributor

@maciejp-ro Thanks for the contribution 👏.
I'm going to close this one in favor of #17610.

@ewbankkit ewbankkit removed the needs-triage Waiting for first response or review from a maintainer. label May 7, 2021
@ewbankkit ewbankkit closed this May 7, 2021
@github-actions
Copy link

github-actions bot commented Jun 7, 2021

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/lambda Issues and PRs that pertain to the lambda service. size/XS Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants