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 potential nil panic in azure remote state #32821

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lonegunmanb
Copy link

@lonegunmanb lonegunmanb commented Mar 10, 2023

Adding a nil check before reading Http status code from response returned by sdk.

terraform-azurerm-provider hashicorp/terraform#32872

Waiting for user create a new issue in this repo.

Fixes #

Target Release

1.4.1

Draft CHANGELOG entry

BUG FIXES

Write a short description of the user-facing change. Examples:

  • terraform init: Fixed crash when the storage account doesn't exist

@lonegunmanb lonegunmanb requested a review from a team as a code owner March 10, 2023 15:15
@lonegunmanb lonegunmanb mentioned this pull request Mar 10, 2023
1 task
@crw
Copy link
Collaborator

crw commented Mar 10, 2023

Thanks for this submission, I've notified the Azure provider team, who would be providing the review for this feature.

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Hi @lonegunmanb

Thanks for this PR. Please see below for a suggestion on how this could be changed and kept more consistent with implementations in the AzureRM provider.

Additionally, a 404 should result in a non-nil value for err here, did you look into why we are getting into this condition?

Thanks!

internal/backend/remote-state/azure/client.go Outdated Show resolved Hide resolved
@jackofallops jackofallops added the waiting-response An issue/pull request is waiting for a response from the community label Mar 13, 2023
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 14, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @lonegunmanb - This LGTM now 👍

Did you find out why we're not getting the err when the response object has a 404 in it?

@manicminer manicminer linked an issue Mar 16, 2023 that may be closed by this pull request
1 task
@lonegunmanb
Copy link
Author

lonegunmanb commented Mar 17, 2023

Thanks for the updates @lonegunmanb - This LGTM now 👍

Did you find out why we're not getting the err when the response object has a 404 in it?

No, it's weird since the whole Reponse is nil. At the crash point, we do have an err but the core was crashed by this nil Response before we can read err (which means no 404 even), I cannot reproduce the error in my side, maybe we can fix this crash, then ask the user to try agin, then he'll be able to read the actual error and share it with us. Could we merge this patch @jackofallops?

@lonegunmanb
Copy link
Author

Hi @jackofallops @manicminer is there anything I can do to get this pr be merged? Thanks!

@manicminer
Copy link
Member

Hi @lonegunmanb, we appreciate the work on this, however I don't believe the proposed change will resolve the original error and as such I am not convinced this is worth changing in its current state?

@lonegunmanb
Copy link
Author

Hi @manicminer , the user is facing a core crash so the actual error cannot be displayed, as we should avoid any nil panic in any case, could we just merge this pr first, so the user could report the actual ereor to us?

@lonegunmanb
Copy link
Author

Hi @manicminer, a kindly ping, is there anything I can do to get this pr merged? Or do you have any suggestion to fix the panic? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend/azure waiting-response An issue/pull request is waiting for a response from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform crashed
5 participants