add VritIO RNG device

[mabeett]

Adds RNG device

Closes #126

[mabeett]

I need help:
when the user does not provides a value for max_bytes the plugin must send to the API max_bytes=0.

From the Proxmox PVE API documentation:

Maximum bytes of entropy allowed to get injected into the guest every 'period' milliseconds. Prefer a lower value when using '/dev/random' as source. Use 0 to disable limiting (potentially dangerous!).",

I am not presenting Tests and Docs because I do not know if solving this part requires changing some part of the Code.

[lbajolet-hashicorp]

Hi @mabeett,

Thanks for the initial code, much appreciated! I did a quick review, but I'll wait until the PR is in a ready state before doing a more thorough review.

Regarding your question about max_bytes, I'm not sure I understand the concern, but setting max_bytes to 0 if undefined might be a solution, it's not recommended for limited sources of entropy like /dev/random, but it will (presumably) work the same, although this may take some time to gather enough entropy for the data.
In the comment you left, you mentioned that max_bytes MUST be set to 0 if undefined, is it a requirement of the API? As in the attribute cannot be left out of the payload?

Alternatively, we could do either:

• Decide a default, non-zero, value: if the max_bytes attribute is not set, its value will be 0, and in this case we can fallback on 1024 for example. Same for the period, if undefined, we can pick a default value for this (we could go for 1000 as shown in the screenshot shared by @modem7 in the linked issue).
• Make the attribute mandatory: if defining a virtRNG for the VM, we can make max_bytes a mandatory attribute, in which case the problem of the value being undefined goes away, and we only have to check that it's >= 0, and include it in the payload to the server. You can specify that in the tags for mapstructure, i.e.: mapstructure:"max_bytes" required:"true".

Let me know if one solution garners your interest, and if you need a hand coming up with the code, I can probably help.

[mabeett]

Hi @mabeett,

Thanks for the initial code, much appreciated! I did a quick review, but I'll wait until the PR is in a ready state before doing a more thorough review.

Regarding your question about max_bytes, I'm not sure I understand the concern, but setting max_bytes to 0 if undefined might be a solution, it's not recommended for limited sources of entropy like /dev/random, but it will (presumably) work the same, although this may take some time to gather enough entropy for the data. In the comment you left, you mentioned that max_bytes MUST be set to 0 if undefined, is it a requirement of the API? As in the attribute cannot be left out of the payload?

Hello @lbajolet-hashicorp ,

Definitely I did not express myself clearly. I am sorry for the misunderstandings. :(

Alternatively, we could do either:

* Decide a default, non-zero, value: if the `max_bytes` attribute is not set, its value will be 0, and in this case we can fallback on `1024` for example. Same for the period, if undefined, we can pick a default value for this (we could go for `1000` as shown in the screenshot shared by @modem7 in the linked issue).

* Make the attribute mandatory: if defining a virtRNG for the VM, we can make `max_bytes` a mandatory attribute, in which case the problem of the value being undefined goes away, and we only have to check that it's `>= 0`, and include it in the payload to the server. You can specify that in the tags for mapstructure, i.e.: `mapstructure:"max_bytes" required:"true"`.

None of both =( . Maybe with the documentation from the Proxmox PVE API[0] is simpler to explain:

json doc from the API Viewer

{
 "description": "Configure a VirtIO-based Random Number Generator.",
 "format": {
  "max_bytes": {
   "default": 1024,
   "description": "Maximum bytes of entropy allowed to get injected into the guest every 'period' milliseconds. Prefer a lower value when using '/dev/random' as source. Use `0` to disable limiting (potentially dangerous!).",
   "optional": 1,
   "type": "integer"
  },
  "period": {
   "default": 1000,
   "description": "Every 'period' milliseconds the entropy-injection quota is reset, allowing the guest to retrieve another 'max_bytes' of entropy.",
   "optional": 1,
   "type": "integer"
  },
  "source": {
   "default_key": 1,
   "description": "The file on the host to gather entropy from. In most cases '/dev/urandom' should be preferred over '/dev/random' to avoid entropy-starvation issues on the host. Using urandom does *not* decrease security in any meaningful way, as it's still seeded from real entropy, and the bytes provided will most likely be mixed with real entropy on the guest as well. '/dev/hwrng' can be used to pass through a hardware RNG from the host.",
   "enum": [
    "/dev/urandom",
    "/dev/random",
    "/dev/hwrng"
   ],
   "type": "string"
  }
 },
 "optional": 1,
 "type": "string",
 "typetext": "[source=]</dev/urandom|/dev/random|/dev/hwrng> [,max_bytes=<integer>] [,period=<integer>]"
}

So, rng0 optional key admits:

• as optional keys:
  • max_bytes: 0 is a non-default admitted "risky" value.
  • period: There are no considerations for them
• as default and mandatory
  • source: Because it is the unique default value the API requester may send one of the three valid strings values without key.

IMO the expected behavior should be:

• No presence in the HCL of max_bytes ?
  • YES -> API request does not send a key-value of max_bytes. The server criteria is going to be applied.
  • NO -> API request sends the value ( in case of being valid ).
• No presence in the HCL of period ?
  • YES -> API request does not send a key-value for period. The server criteria is going to be applied.
  • NO -> API request sends the value ( in case of being valid ).

In the WebUI when the user leaves the default 1024 related with max_bytes, the request from the frontend does not send the key. The same consideration for the default-empty field related with period.

From my side I've also checked to call the API excluding the optional keys and (as expected) it works normally.

I propose this because I consider the plugin should admit leaving implicit/undefined the API keys which admit it, plus the potential future not-match of default values plugin-packer versus server.

I hope this make sense

Let me know if one solution garners your interest, and if you need a hand coming up with the code, I can probably help.

[0]: The apivewer on the server is engined via javascript, soy we may get the same info via as json format:

curl -s -k https://${myserver}:8006/pve-docs/api-viewer/apidoc.js  \
| sed  -e 's/const apiSchema = //g' \
|tr -d '\n' | sed  's/;let method2cm.*$//g' \
| jq --indent 1 '.[1].children[0].children[0].children[0].children[4].info.PUT.parameters.properties.rng0  ' -r

[lbajolet-hashicorp]

I think I understand, so the problem would be the default value for max_bytes being 0, we don't know if it's really a voluntary 0 to disable the limit, or if it's undefined, is that right?

In this case it's a bit tricky, since there's no way to distinguish the two AFAICT from a language perspective, so for this option at least, I would consider making it mandatory, and state in the docs that the recommended value be 1024, and state that setting it to 0 disables this limit on PVE, which may imply that the entropy source can starve if used without limitation.
This is not ideal, but it would make it possible for users to set whichever value they want to, including 0 if they so choose.

Regarding period, I believe 0 is not a valid value, so we can rely on it being 0 as undefined, and in this case, we can leave it out of the payload we send to PVE.

Would that be acceptable?

[mabeett]

I think I understand, so the problem would be the default value for max_bytes being 0, we don't know if it's really a voluntary 0 to disable the limit, or if it's undefined, is that right?

Exactly. That is the case.

In this case it's a bit tricky, since there's no way to distinguish the two AFAICT from a language perspective, so for this option at least, I would consider making it mandatory, and state in the docs that the recommended value be 1024, and state that setting it to 0 disables this limit on PVE, which may imply that the entropy source can starve if used without limitation. This is not ideal, but it would make it possible for users to set whichever value they want to, including 0 if they so choose.

OK. I understand. I don't know how to refer as mandatory this option. Could you help with an snippet in this plugin or a similar one? Thanks a lot.

Regarding period, I believe 0 is not a valid value, so we can rely on it being 0 as undefined, and in this case, we can leave it out of the payload we send to PVE.

Yes, I've just checked, proxmox API accepts 0 and negative values, but when starting the VM kvm-qemu fails:

kvm: -device virtio-rng-pci,rng=rng0,max-bytes=512,period=0,bus=pci.1,addr=0x1d: 'period' parameter expects a positive integer

Would that be acceptable?

Yes, indeed.

Regarding the documentation I was going to edit docs/builders/clone.mdx and docs/builders/iso.mdx. Let me know if the is a way documenting in the go sourcecode.
Thanks,

[lbajolet-hashicorp]

You can add a required part to the tag that defines the attribute for mapstructure, if you set it to true, the attribute becomes mandatory and configurations that don't define it will get rejected.

You can refer to https://github.com/hashicorp/packer-plugin-proxmox/blob/main/builder/proxmox/clone/config.go#L23 as an example for this, let me know if you need more assistance on that front.

Regarding the docs, if you write documentation on the attributes or the encompassing structure, they will get compiled into the docs-partials directory, and you can then use an @include directive in the builders' mdx files to include the contents of this file. This would avoid having to manually maintain the docs in both places, since the partial gets recompiled when you run make generate.

You can find similar use-cases in some other plugins, Amazon for example: https://github.com/hashicorp/packer-plugin-amazon/blob/main/docs/builders/ebs.mdx?plain=1#L50

Again for the docs, let me know if you need help with those, I can chime in if need be.

Thanks!

[mabeett]

I' ve just pushed the changes according we agreed . There might be one or more errors.
I wrote as much as I could the tests. Unfortunately tests code is broken.

[lbajolet-hashicorp]

Hi @mabeett,

I took a look at the changes, overall I think we're near good-to-go on that PR. I left a suggestion to fix the failing test, feel free to adapt it however you see fit, and I'll let you iterate on them to get them all to pass.

I also left a suggestion regarding docs so they're clearer for users of the plugin.

Once you've addressed my latest comments, I think we'll be ready to merge this in, thanks for the continued updates on this!

[mabeett]

Once you've addressed my latest comments, I think we'll be ready to merge this in, thanks for the continued updates on this!

Hello,
I worked on each one of your comments. I also commented about the Documentation Output.
Let me know if you need more info

regards,

[lbajolet-hashicorp]

Hi @mabeett,

Sorry I didn't take another look at your PR, I was away on vacation for a few weeks and it slipped out of my mind, apologies for that.

I'll take a look at your PR this week and let you know if there are some more changes I can think of, and if it's good, we'll merge this in.

Thanks for the reminder!

[lbajolet-hashicorp]

Hi @mabeett,

In the current state I'm confident we can merge this! I've allowed myself to push a small nit regarding an error message, but aside from that, it looks good to me, so I'll wait until tests are green, and then trigger the merge.

Sorry for making you wait for this, and thanks again for submitting this PR and following up on it, we appreciate that!

[mabeett]

Sorry for making you wait for this, and thanks again for submitting this PR and following up on it, we appreciate that!

There is no problem!.

I was considering proposing a PR changing the place on which is written the documentation: The idea is -when possible- moving the texts from the mdx files to the Clang-related code when possible (as in this PR) .

On t his way future contributors should see easier/more natural adding the info directly to the code.

What do you think?

[lbajolet-hashicorp]

Hi @mabeett,

Regarding your last message, I'm not sure what you mean by moving the texts from the mdx files to the Clang-related code, I assume you mean moving away from the duplicated docs in the final mdx and importing partials instead?

If so, yes very much so! I considered doing this myself when I have time for it, but still haven't found that just yet, so if you wanted to do this, please feel free to do so, and let me know if you need some help at any point in the process.

[mabeett]

Hi!
Great, I'm working on it during my spare time.
Thanks,

[mabeett]

If so, yes very much so! I considered doing this myself when I have time for it, but still haven't found that just yet, so if you wanted to do this, please feel free to do so, and let me know if you need some help at any point in the process.

I've just left the PR #214 . (now it is a draft)