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

Fix nil pointer dereference in ResourceData.HasChangeExcept #811

Merged
merged 3 commits into from
Oct 12, 2021

Conversation

w-leads
Copy link
Contributor

@w-leads w-leads commented Oct 7, 2021

ResourceData.HasChangeExcept and ResourceData. HasChangesExcept accesses .diff.Attributes which could be nil and cause nil pointer dereference error.
This PR fixes the issue by checking the value and if it's nil, then return false(meaning no changes)

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 7, 2021

CLA assistant check
All committers have signed the CLA.

@bflad
Copy link
Contributor

bflad commented Oct 7, 2021

Hi @w-leads 👋 Thank you for submitting this. Is this change required to fix an issue or is it purely for code correctness? The change looks good and makes sense, just trying to confirm this detail first to see if there are potentially other related issues as well. Thank you!

@bflad bflad added the waiting-response Issues or pull requests waiting for an external response label Oct 7, 2021
@w-leads
Copy link
Contributor Author

w-leads commented Oct 7, 2021

@bflad Thank you for the reply! This is just for fixing the error I encountered while developing a custom terraform provider. I quickly tried to search for a related issue or PR but could not find it.

@bflad bflad added bug Something isn't working and removed waiting-response Issues or pull requests waiting for an external response labels Oct 7, 2021
@bflad
Copy link
Contributor

bflad commented Oct 7, 2021

Great to know, @w-leads, and sorry you ran into trouble. Would you happen to have time to file a bug report so we can capture some of the details of how this happened? We would love to create some test cases to prevent future regressions. If not, no worries. Appreciate the time and effort you have put into this already.

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Thank you again.

@w-leads
Copy link
Contributor Author

w-leads commented Oct 11, 2021

@bflad While checking the behavior to file a bug report, I noticed that probably I'm using the function in a wrong way..
I wanted to run an additional step in terraform plan when a specific set of fields have planned changes, and I tried to use ResourceData.HasChangeExcept() in ReadContext, but should the func be called only by UpdateContext?

w-leads and others added 2 commits October 11, 2021 12:31
Co-authored-by: Brian Flad <bflad417@gmail.com>
Co-authored-by: Brian Flad <bflad417@gmail.com>
@bflad
Copy link
Contributor

bflad commented Oct 12, 2021

Thank you for the followup, @w-leads 👍

but should the func be called only by UpdateContext?

You are correct, the ResourceData.HasChange* methods are generally only applicable during the Resource.Update* methods. If you would like to customize the terraform plan operation before those resource methods are called, the Resource.CustomizeDiff functionality is available (note that it uses a similar, but separate ResourceDiff type).

More information about plan customization can be found at https://www.terraform.io/docs/extend/resources/customizing-differences.html, with some additional helpers available in the https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff package.

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Looks good to me 🚀

@bflad bflad merged commit c28e2a4 into hashicorp:main Oct 12, 2021
@bflad bflad added this to the v2.8.1 milestone Oct 12, 2021
bflad added a commit that referenced this pull request Oct 12, 2021
@w-leads
Copy link
Contributor Author

w-leads commented Oct 13, 2021

Thank you very much for the information! Will look into the documents
Also thank you for the review and merging the PR!

@w-leads w-leads deleted the fix-npe-haschangeexcept branch October 13, 2021 01:44
@bflad bflad modified the milestones: v2.8.1, v2.9.0 Oct 14, 2021
@github-actions
Copy link

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 contributions.
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 Nov 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants