Prevent panic on random_string and random_password when character set is empty
#551
Conversation
…pper, lower, numeric/number are set to false (#549) Previously, a panic would be generated if the following configuration was used: ``` resource "random_string" "random" { length = 16 special = false upper = false lower = false numeric = false } ``` ``` ╷ │ Error: Plugin did not respond │ │ with random_string.random, │ on resource.tf line 1, in resource "random_string" "random": │ 1: resource "random_string" "random" { │ │ The plugin encountered an error, and failed to respond to the plugin.(*GRPCProvider).ApplyResourceChange call. The plugin logs may contain more details. ╵ Stack trace from the terraform-provider-random plugin: panic: crypto/rand: argument to Int is <= 0 ``` This change will cause validation to fail if special, upper, lower, and numeric/number are all set to false Output from acceptance testing: ```console TF_ACC=1 go test -count=1 -run='TestAccResourceString_NumericFalse' -timeout=10m -v ./internal/provider === RUN TestAccResourceString_NumericFalse === PAUSE TestAccResourceString_NumericFalse === CONT TestAccResourceString_NumericFalse --- PASS: TestAccResourceString_NumericFalse (0.15s) ```
| path.MatchRoot("upper"), | ||
| path.MatchRoot("lower"), | ||
| ), | ||
| }, | ||
| }, | ||
|
|
||
| "numeric": schema.BoolAttribute{ |
There was a problem hiding this comment.
Should we update the description of this attribute to indicate that it needs to be set with one of special, upper, or lower being true? Same with the number attribute.
There was a problem hiding this comment.
Good shout. I've updated the description for number and numeric for both resource_password and resource_string and regenerated the docs.
| "github.com/hashicorp/terraform-plugin-framework/types" | ||
| ) | ||
|
|
||
| // AtLeastOneOfTrueValidator is the underlying struct implementing AtLeastOneOfTrue. |
There was a problem hiding this comment.
We can probably accomplish the same thing with some sort of combination of the built-in any(), all(), atLeastOneOf() bool validators with an equals() validator, but I think that your approach is more readable.
There was a problem hiding this comment.
Agreed. But like you, this seemed more readable to me.
| @@ -442,6 +450,13 @@ func stringSchemaV3() schema.Schema { | |||
| boolplanmodifiers.NumberNumericAttributePlanModifier(), | |||
| boolplanmodifier.RequiresReplace(), | |||
| }, | |||
| Validators: []validator.Bool{ | |||
| validators.AtLeastOneOfTrue( | |||
There was a problem hiding this comment.
Nota Bene: Definitely don't need to change this, but another consideration here might be to introduce this multiple attribute validation at the resource level as a ConfigValidator and specify all 4 attributes in there. Maybe its just me but I tend to get a little confused reading attribute-based validators (remembering "oh yeah, it includes this attribute too") and whether that attribute-based validation should be added to all the relevant attributes (confusingly, no, since it will generate multiple similar diagnostics). 😅
There was a problem hiding this comment.
Happy to refactor along these lines, but if you're ok with the current implementation could you approve the PR if there's nothing blocking?
…ssword and resource_string
<Actions>
<action
id="296d75eab55b9d23bd1e94dc34cea43b964c29945c12fefcb674e3c068a0a767">
<h3>Bump Terraform `random` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
<summary>Update Terraform lock file</summary>
<p>changes detected:
	"hashicorp/random" updated from
"3.6.0" to "3.6.1" in file
".terraform.lock.hcl"</p>
<details>
<summary>3.6.1</summary>
<pre>Changelog retrieved
from:
	https://github.com/hashicorp/terraform-provider-random/releases/tag/v3.6.1
BUG
FIXES:

* all: Prevent `keepers` from triggering an in-place
update following import
([#385](https://github.com/hashicorp/terraform-provider-random/issues/385))
*
resource/random_shuffle: Prevent inconsistent result after apply when
result_count is set to 0
([#409](https://github.com/hashicorp/terraform-provider-random/issues/409))
*
provider/random_password: Fix bug which causes panic when special,
upper, lower and number/numeric are all false
([#551](https://github.com/hashicorp/terraform-provider-random/issues/551))
*
provider/random_string: Fix bug which causes panic when special, upper,
lower and number/numeric are all false
([#551](https://github.com/hashicorp/terraform-provider-random/issues/551))


</pre>
</details>
</details>
<a
href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/118/">Jenkins
pipeline link</a>
</action>
</Actions>
---
<table>
<tr>
<td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
</td>
<td>
<p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
</p>
<details><summary>Options:</summary>
<br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
<ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
</ul>
<p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
</p>
</details>
</td>
</tr>
</table>
Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>
|
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. |
Closes: #549