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

Add new resource random_bytes to generate an array of random bytes #272

Closed
wants to merge 15 commits into from

Conversation

Socolin
Copy link
Contributor

@Socolin Socolin commented Jul 8, 2022

Intended to be used as key or secret

Fix #63

@Socolin Socolin requested a review from a team as a code owner July 8, 2022 00:30
@hashicorp-cla
Copy link

hashicorp-cla commented Jul 8, 2022

CLA assistant check
All committers have signed the CLA.

@Socolin
Copy link
Contributor Author

Socolin commented Aug 6, 2022

@bendbennett Hello, is there any chance to see this PR merge (or if it's not something you want, rejected) soon ? I need this I would like to know if at least it's something you are considering or if I should instead create my own provider ?

@Socolin
Copy link
Contributor Author

Socolin commented Oct 11, 2022

@bendbennett Have you any ETA ? I really need this and I'm just going to move it to my own provider and close this if it's not going to be merged soon

@bendbennett
Copy link
Contributor

@Socolin apologies for delay in responding on this. I'm afraid that I don't have an ETA at this time but I will endeavour to get some feedback to you.

@dnlopes
Copy link

dnlopes commented Oct 18, 2022

I'm also waiting badly for this PR to get merged. Have a comment in my code base pointing here 😄

Copy link
Contributor

@bendbennett bendbennett 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 contribution.

Could you rebase on main?
Looks like this PR is using v0.9.0 of terraform-plugin-framework.

CHANGELOG.md Outdated Show resolved Hide resolved
@Socolin
Copy link
Contributor Author

Socolin commented Nov 30, 2022

I updated the code, can you please merge this soon ?

@sebhaub
Copy link

sebhaub commented Jan 9, 2023

nice - this PR is exactly what i currently miss in the random provider.
@bendbennett is there something missing / preventing to merge this ?

@jmgilman
Copy link

I'd be willing to help out if it means getting this merged sooner. A huge need for us, thanks!

@Socolin
Copy link
Contributor Author

Socolin commented Jan 24, 2023

@bendbennett Do you have an ETA on this ?

@bookshelfdave
Copy link
Contributor

We hope to release this in the next few weeks, it's queued behind some other work that we have planned.

Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

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

@Socolin thank you for your contribution.

Would you be happy to add a couple more tests to verify:

  • Behaviour of validator on length attribute is as expected.
  • Altering length forces a replacement.

CHANGELOG.md Outdated Show resolved Hide resolved
internal/provider/resource_bytes.go Outdated Show resolved Hide resolved
internal/provider/resource_bytes.go Outdated Show resolved Hide resolved
docs/resources/bytes.md Outdated Show resolved Hide resolved
docs/resources/bytes.md Outdated Show resolved Hide resolved
Sensitive: true,
},
"id": {
Description: "The generated bytes presented in string format.",

Choose a reason for hiding this comment

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

at a cursory glance I don't think this is accurate, "id" appears to always be set to "none". Other providers have this described as "A static value used internally by Terraform, this should not be referenced in configurations."

Should result be added to actually return the generated bytes in string format? probably a question for @bendbennett - not sure how the state file is expected to behave with binary data in it - or whether it could be achieved in the Read function - or if anyone would ever use it anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not set result to avoid confusion, since the result is always encoded. But I could add it, we just need to choose if the default is base64 or hex (or other ?)

Copy link
Contributor

Choose a reason for hiding this comment

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

As the base64 and hex encodings of result are available it doesn't seem like there is a need for result as @Socolin indicates it would need to be encoded.

internal/provider/resource_bytes.go Outdated Show resolved Hide resolved
internal/provider/resource_bytes_test.go Outdated Show resolved Hide resolved
internal/provider/resource_bytes_test.go Outdated Show resolved Hide resolved
@Socolin
Copy link
Contributor Author

Socolin commented Feb 14, 2023

Thanks for the comments, I pushed a fix for them.

@maaarghk
Copy link

Thanks for the comments, I pushed a fix for them.

Think everything I left looks good, although maybe I obscured a request from Ben for two more tests to be added :)

@Socolin
Copy link
Contributor Author

Socolin commented Feb 14, 2023

Thanks for the comments, I pushed a fix for them.

Think everything I left looks good, although maybe I obscured a request from Ben for two more tests to be added :)

Thanks I forgot about this, I'm checking this

@Socolin
Copy link
Contributor Author

Socolin commented Mar 13, 2023

I rebased the branch on main too

@ohemelaar
Copy link

Hello,
I've been lurking at this PR for a while now, I'm quite interested in using this. I see the PR is approved, is there a specific release schedule that's blocking the merge now?

@dominicroessner
Copy link

Any update or timeline on when this might go in would really be appreciated. Waiting for this as others. Thank you.

@Socolin
Copy link
Contributor Author

Socolin commented May 10, 2023

Hello,

@bendbennett What is the next steps for this to be merged and released ?

@bendbennett
Copy link
Contributor

Hi @Socolin 👋

I'd like to get an additional approval of the PR prior to merging as these changes are a significant alteration to the random utility provider. The efforts of the team are currently focussed elsewhere at the moment but we will endeavour to focus some attention on this as soon as we're able.

@GerardSoleCa
Copy link

Hi @bendbennett, another interested party here. I'd really appreciate to have this in the random provider. We are currently generating tokens for akamai CDN which require hex strings, and the only option I saw is to use the random_id resource, but that's a really insecure option since output is not sensitive :(

Hope you get soon a second approval for this PR.

Cheers!

@Socolin
Copy link
Contributor Author

Socolin commented Aug 28, 2023

@bendbennett Hello, do you have any ETA or "Not before" date for this ?
I would like to know to take some decision about this, I might publish this in another provider so I can start using it, but I will not do this if it's close to be merged.

@bendbennett
Copy link
Contributor

Hi @Socolin and @GerardSoleCa 👋

Apologies for the delay. The focus of the team is currently elsewhere at the moment so it's hard to give an ETA.

@Socolin
Copy link
Contributor Author

Socolin commented Aug 31, 2023

To anyone looking for this, I published this in a separate provider

https://registry.terraform.io/providers/Socolin/randombyte/latest

resource "randombyte_bytes" "name" {
length = 64
}

@bendbennett
Copy link
Contributor

Hi @Socolin 👋

Apologies for the delay in getting back to you. We have recently discussed your PR, and, if you're still amenable, we would like to move forward with the addition of the random_bytes resource.

The following suggestions came out of our discussion:

  • Could result_base64, and result_hex be renamed base64, and hex, respectively?
  • Could id be removed? Just to clarify, it is not used by practitioners, it is not required for the provider supported versions of Terraform (>= 0.12), and it is no longer needed for testing purposes (i.e., when the terraform-plugin-testing module is used).
  • With the addition of random_bytes, it would likely be valuable to update the documentation for both random_id, and random_bytes to provide some clarification around the use-case for using one or other of these two resources. The most obvious differentiator being when the output would be considered sensitive, and should not be displayed in CLI console output.

Thank you again for your contribution, and sorry it has taken a while for us to get back to you.

@bendbennett
Copy link
Contributor

Hi @Socolin 👋

I've incoporated all of your commits into Add random_bytes resource.

If you'd rather update this PR with the suggestions in this comment, then that would be great. If not, then we'll move forward with Add random_bytes resource.

Many thanks for all your work on this, and apologies for the delay.

@Socolin
Copy link
Contributor Author

Socolin commented Nov 30, 2023

Hello,

I'm working on other project right now, you can move forward with your PR, Thanks :)

dduportal referenced this pull request in jenkins-infra/azure Dec 5, 2023
<Actions>
<action
id="296d75eab55b9d23bd1e94dc34cea43b964c29945c12fefcb674e3c068a0a767">
        <h3>Bump Terraform `random` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>&#34;hashicorp/random&#34; updated from &#34;3.5.1&#34; to
&#34;3.6.0&#34; in file &#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.6.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-random/releases/tag/v3.6.0&#xA;FEATURES:&#xA;&#xA;*
resource/random_bytes: New resource that generates an array of random
bytes intended to be used as key or secret
([#272](https://github.com/hashicorp/terraform-provider-random/issues/272))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/terraform-jobs/job/azure/job/main/918/">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) <[email protected]>
Co-authored-by: Damien Duportal <[email protected]>
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 May 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

General random bytes, perhaps base64 encoded?