-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
terraform init: add link to documentation when a checksum is missing from the lock file #31408
terraform init: add link to documentation when a checksum is missing from the lock file #31408
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this seems like a reasonable compromise to me, but I have some specific concerns about the way we communicate the suggested resolution in the error message.
I left some more details inline. I apologize that this isn't a particularly actionable comment on its own, but maybe you have some thoughts on what we might try here, or maybe you disagree with my proposition in the first place, in which case let me know! 😀
internal/command/init.go
Outdated
diags = diags.Append(tfdiags.Sourceless( | ||
tfdiags.Error, | ||
"Failed to install provider", | ||
fmt.Sprintf("Error while installing %s v%s: %s\n\nYou can ensure the current platform, %s, is included in the dependency lock file by running: `terraform providers lock -provider=%s`.\n\nIf this does not fix the problem you may need to reset any provider caching present in your setup or make sure you are connecting to valid provider distributions.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need to be careful about making this suggestion without some more context about the implications.
Specifically, when combining with this with the other change we recently made so that terraform providers lock
will not honor existing checksums, I think this suggestion amounts to "The checksums didn't match the lock file, which you should fix by discarding the checksums in the lock file".
I must admit I'm not sure how to inject further subtlety here without making the message incredibly verbose. I think the main qualifier we want to get across here is "If you got into this situation by not having a complete set of checksums for all platforms you intend to use...", but I don't really know how to concisely explain to a user how they would determine if that predicate is true, and how to securely differentiate it from the bad case where someone has actually maliciously tampered with the plugin package. 🤔
I think we might need to ask the product security team for a second opinion on whatever we decide here too, since we're changing the characteristics of a pretty sensitive area for the threat models of some users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this so we just link directly to the relevant documentation, hopefully that avoids any potential security problems while still giving users a jump start on the potential fixes.
The relevant documentation does concretely suggest terraform providers lock
, while providing context around how this could have happened.
…te/init-provider-checksums-failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did leave a error message grammar nit that is so nit-picky that I'm cringing about it, but otherwise this looks like a great addition to this chain of work to improve these errors messages. Thanks!
"the current package for %s %s doesn't match any of the checksums previously recorded in the dependency lock file", | ||
"the current package for %s %s doesn't match any of the checksums previously recorded in the dependency lock file, for more information: https://www.terraform.io/language/provider-checksum-verification", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nittiest possible grammar nit: this seems like a semicolon-appropriate situation rather than a comma-appropriate situation, because this new addition reads to me as an independent clause.
(and same feedback for the other example below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
…te/init-provider-checksums-failed
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
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. |
terraform init
will now tell users to tryterraform providers lock
whenever it cannot download a provider due to a checksum mismatch.The error message explaining a provider could not be downloaded due to a checksum mismatch now contains a link to the documentation explaining why this might have happened and how to work around it. We're holding off on suggesting anything too strongly in the actual error message because of the security implications.
We are using a redirect in the terraform-website added here so that if/when the documentation moves we can update the redirect and older versions of terraform will carry on pointing to the right place.