Repository navigation
Add kernel update to Terraform - #4
pegahnikbakht wants to merge 14 commits into
Conversation
antoniacobaeus
left a comment
antoniacobaeus
left a comment
There was a problem hiding this comment.
Looks good in general, just a few questions, see comments.
Would it be suitable to gate this feature behind a variable, or should we enforce it always?
| @@ -0,0 +1,23 @@ | |||
| resource "null_resource" "update_kernel" { | |||
| depends_on = [aws_instance.cvm] | |||
There was a problem hiding this comment.
I think this makes it run after the server has been provisioned? and attested?
With the kernel update I believe the measurement would change and thus the first attestation isn't really valid for this enclave then. An attestation after the kernel update would be more accurate.
There was a problem hiding this comment.
I think this still applies? (previous comment on attestation flow)
antoniacobaeus
left a comment
antoniacobaeus
left a comment
There was a problem hiding this comment.
This looks good overall and I think it is the correct approach to go via the package managers. It does seem a little unstable from just reading the script but I haven't tested it. In any case I have some small improvement suggestions and questions.
I think the expected usage would be that if you request a kernel update then I would expect that the kernel update succeeds and otherwise tower fails and you need to re-provision.
The one thing I would liked changed is the attestation flow, which currently runs before the kernel update. We would need to somehow move it to run afterwards.
| provisioner "remote-exec" { | ||
| inline = [ | ||
| "cloud-init status --wait", | ||
| <<-EOT |
There was a problem hiding this comment.
Can this be a script in a file instead of inlined? Mostly so that all modules could reference the same update script, since I believe they are the same?
| . /etc/os-release | ||
| KERNEL_VERSION="${var.kernel_version == null ? "" : var.kernel_version}" | ||
| case "$ID" in | ||
| ubuntu) |
There was a problem hiding this comment.
should debian be added to this list?
There was a problem hiding this comment.
Debian doesn't seem to have support for SNP or TDX, I checked Azure and AWS, they didn't have support for it.
| @@ -0,0 +1,23 @@ | |||
| resource "null_resource" "update_kernel" { | |||
| depends_on = [aws_instance.cvm] | |||
There was a problem hiding this comment.
I think this still applies? (previous comment on attestation flow)
| default = true | ||
| } | ||
|
|
||
| variable "kernel_update_enabled" { |
There was a problem hiding this comment.
This could be combined into one variable with an object
kernel_update = object({
enable = bool
version = string
})
| esac | ||
|
|
||
| if [ -f /var/run/reboot-required ] || { command -v needs-restarting >/dev/null 2>&1 && sudo needs-restarting --reboothint; }; then | ||
| sudo shutdown -r +1 'Kernel update completed' |
There was a problem hiding this comment.
Is there a way to get the version of the kernel that will be updated to? Or maybe the one that got updated to and respond with that via terraform?
This is a really minor thing, and the attestation report will include it, I was mostly thinking that it could be a nice to have. Same goes for if the requested kernel version doesn't exist.
| <<-EOT | ||
| set -eu | ||
| . /etc/os-release | ||
| KERNEL_VERSION="${var.kernel_version == null ? "" : var.kernel_version}" |
There was a problem hiding this comment.
What format should this be? Is it always semver, or does it vary across distributions?
If the latter, we might need some documentation surrounding this.
| rhel|centos|fedora|rocky|almalinux|amzn) | ||
| dnf_retry() { | ||
| attempts=0 | ||
| until sudo dnf "$@"; do |
There was a problem hiding this comment.
how come attempts here, but not for ubuntu or suse?
Does dnf sometimes just fail?
There was a problem hiding this comment.
Yeah on azure for RHEL it failed in the beginning and I added this attempts. Seems to be a cache issue.
There was a problem hiding this comment.
Does it always fail like that? Does retrying do anything unless the cache gets refreshed?
Would running this dnf_retry makecache --refresh before change anything?
| host = azurerm_linux_virtual_machine.cvm.public_ip_address | ||
| } | ||
|
|
||
| provisioner "remote-exec" { |
There was a problem hiding this comment.
a general question, how strict is this update? Does it prevent the whole creation if kernel can't be updated, or otherwise fails?
If it fails is the vm left as is or do we need to destroy it afterwards?
There was a problem hiding this comment.
Terraform will finish with an error but I guess the CVM still be usable.
There was a problem hiding this comment.
Yeah, on second thought, that probably is the correct behavior and you will have to decide what you want to do with the information
2 participants
No description provided.