Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 170 additions & 1 deletion cloudinit/config/cc_growpart.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,17 @@
# This file is part of cloud-init. See LICENSE file for license information.
"""Growpart: Grow partitions"""

import base64
import copy
import json
import os
import os.path
import re
import stat
from contextlib import suppress
from pathlib import Path
from textwrap import dedent
from typing import Tuple

from cloudinit import log as logging
from cloudinit import subp, temp_utils, util
Expand Down Expand Up @@ -85,6 +91,8 @@
"ignore_growroot_disabled": False,
}

KEYDATA_PATH = Path("/cc_growpart_keydata")


class RESIZE(object):
SKIPPED = "SKIPPED"
Expand Down Expand Up @@ -293,10 +301,128 @@ def devent2dev(devent):
return dev


def get_mapped_device(blockdev):
"""Returns underlying block device for a mapped device.

If it is mapped, blockdev will usually take the form of
/dev/mapper/some_name

If blockdev is a symlink pointing to a /dev/dm-* device, return
the device pointed to. Otherwise, return None.
"""
realpath = os.path.realpath(blockdev)
if realpath.startswith("/dev/dm-"):
LOG.debug("%s is a mapped device pointing to %s", blockdev, realpath)
return realpath
return None


def is_encrypted(blockdev, partition) -> bool:
"""
Check if a device is an encrypted device. blockdev should have
a /dev/dm-* path whereas partition is something like /dev/sda1.
"""
if not subp.which("cryptsetup"):
LOG.debug("cryptsetup not found. Assuming no encrypted partitions")
return False
try:
subp.subp(["cryptsetup", "status", blockdev])
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we want to make this catch more specific and log a warning in case there are other failure paths we might hit here which would force an is_encypted False value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

cryptsetup should only return 0 if it's actually encrypted. Any other rc will raise the ProcessExecutionError.

Are you saying we should log the return code, just in case there's a different problem with running cryptsetup?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess what I'm thinking here two things:

  • Should we really only surpress a specific non-zero exit code and log any unexpected error messages and codes here
  • Also, running cryptsetup status on a thin volume group I created still returns 0 exit code, but it isn't an encrypted device, so a zero exit code may be a false positive in this case for any LVM setup?
csmith@downtown:~$ sudo cryptsetup status /dev/mapper/sru_lvmpool-LXDThinPool_tdata
/dev/mapper/sru_lvmpool-LXDThinPool_tdata is active and is in use.
  type:    n/a
csmith@downtown:~$ echo $?
0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we can use dmsetup ls --target crypt here to list known crypt devices.
On my vm with luks enabled for the root FS I get

$ dmsetup ls --target crypt
dm_crypt-0     (253, 0)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The problem with dmsetup ls is that it's not easy to know how to tie values in that list to the specific device being resized.

In a previous iteration of this PR, I had a cryptsetup isLuks check. Based on a misunderstanding of a comment, I removed it. It looks like he was saying that we don't need to restrict resizing mapped devices to only Luks containers, but I want to specifically do that right now as we haven't had support for LVM sizing, and I'd rather not add it here without additional testing. So I added back the isLuks check.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

+1 this context makes sense. And crypsetup isLuks behaves as expected on my non-luks volume group. I'm +1 on restricting this behavior to just encrypted partitions at the moment given the artifacts which allow for resizing. We can iterate toward LVM resizing in separate efforts.

except subp.ProcessExecutionError as e:
if e.exit_code == 4:
LOG.debug("Determined that %s is not encrypted", blockdev)
else:
LOG.warning(
"Received unexpected exit code %s from "
"cryptsetup status. Assuming no encrypted partitions.",
e.exit_code,
)
return False
with suppress(subp.ProcessExecutionError):
subp.subp(["cryptsetup", "isLuks", partition])
LOG.debug("Determined that %s is encrypted", blockdev)
return True
return False


def get_underlying_partition(blockdev):
command = ["dmsetup", "deps", "--options=devname", blockdev]
dep: str = subp.subp(command)[0] # type: ignore
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looking over cryptsetup dependencies, dmsetup is a Depends and we do an early check/exit in is_encrypted. If cryptsetup is absent, I don't think we need to handle CalledProcessErrrors here because we expect dmsetup will exist in order to get to this path.

Question about this in general, do we always expect any images which support encrypted disk resize to add cryptsetup package to the image above and beyond ensuring cloud-init is installed in the image?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Question about this in general, do we always expect any images which support encrypted disk resize to add cryptsetup package to the image above and beyond ensuring cloud-init is installed in the image?

I'm assuming so, but that'd be a question for CPC. Even if we were to fail here though, it'd get caught here with a slightly uglier error message: https://github.com/canonical/cloud-init/pull/1316/files#diff-326a9a2b453b20b3c3bb518217c1ffc2849e8ee82824ecfe359027b1e4237c77R466

# Returned result should look something like:
# 1 dependencies : (vdb1)
if not dep.startswith("1 depend"):
Comment thread
blackboxsw marked this conversation as resolved.
raise RuntimeError(
f"Expecting '1 dependencies' from 'dmsetup'. Received: {dep}"
)
try:
return f'/dev/{dep.split(": (")[1].split(")")[0]}'
except IndexError as e:
raise RuntimeError(
f"Ran `{command}`, but received unexpected stdout: `{dep}`"
) from e


def resize_encrypted(blockdev, partition) -> Tuple[str, str]:
"""Use 'cryptsetup resize' to resize LUKS volume.

The loaded keyfile is json formatted with 'key' and 'slot' keys.
key is base64 encoded. Example:
{"key":"XFmCwX2FHIQp0LBWaLEMiHIyfxt1SGm16VvUAVledlY=","slot":5}
"""
if not KEYDATA_PATH.exists():
return (RESIZE.SKIPPED, "No encryption keyfile found")
try:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we pre-flight check existence of this KEYDATA_PATH and return SKIPPED in the event that it doesn't exist?

Then our try/except block below can only handle invalid data/format/decode errors if they exist and we avoid raising a FAILURE message in logs.

Suggested change
try:
if not KEYDATA_PATH.exists():
return (RESIZE.SKIPPED, f"No {KEYDATA_PATH} exists to decrypt device {blockdev}")
try:

with KEYDATA_PATH.open() as f:
keydata = json.load(f)
key = keydata["key"]
decoded_key = base64.b64decode(key)
slot = keydata["slot"]
except Exception as e:
raise RuntimeError(
"Could not load encryption key. This is expected if "
"the volume has been previously resized."
) from e
Comment on lines +380 to +383
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there a way we can make this idempotent by introspecting that we have already resized and encrypted partition? I'm thinking in terms of if someone runs cloud-init clean --logs --reboot will be generate a Runtime error in this case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The actual effect is just an extra log (see my examples up top):

2022-03-28 21:22:55,202 - cc_growpart.py[DEBUG]: '/' FAILED: Resizing encrypted device (/dev/mapper/cloudimg-rootfs-03de45d2-f355-457e-b8e8-87c48127d0a8) failed: Could not load encryption key. This is expected if the volume has been previously resized.

Do you have any suggestions? We can't count on our normal semaphores, so cloud-init would have to store something more permanent which seems could be problematic if disks ever get hot-swapped or anything along those lines.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The only suggestion I wonder about is if we should really just early exit on absence of KEYDATA_PATH as mentioned above.


try:
subp.subp(
["cryptsetup", "--key-file", "-", "resize", blockdev],
data=decoded_key,
)
finally:
try:
subp.subp(
[
"cryptsetup",
"luksKillSlot",
"--batch-mode",
partition,
str(slot),
]
)
except subp.ProcessExecutionError as e:
LOG.warning(
"Failed to kill luks slot after resizing encrypted volume: %s",
e,
)
try:
KEYDATA_PATH.unlink()
except Exception:
util.logexc(
LOG, "Failed to remove keyfile after resizing encrypted volume"
)

return (
RESIZE.CHANGED,
f"Successfully resized encrypted volume '{blockdev}'",
)


def resize_devices(resizer, devices):
# returns a tuple of tuples containing (entry-in-devices, action, message)
devices = copy.copy(devices)
info = []
for devent in devices:

while devices:
devent = devices.pop(0)
try:
blockdev = devent2dev(devent)
except ValueError as e:
Expand Down Expand Up @@ -333,6 +459,49 @@ def resize_devices(resizer, devices):
)
continue

underlying_blockdev = get_mapped_device(blockdev)
if underlying_blockdev:
try:
# We need to resize the underlying partition first
partition = get_underlying_partition(blockdev)
if is_encrypted(underlying_blockdev, partition):
if partition not in [x[0] for x in info]:
# We shouldn't attempt to resize this mapped partition
# until the underlying partition is resized, so re-add
# our device to the beginning of the list we're
# iterating over, then add our underlying partition
# so it can get processed first
devices.insert(0, devent)
devices.insert(0, partition)
Comment thread
blackboxsw marked this conversation as resolved.
continue
status, message = resize_encrypted(blockdev, partition)
info.append(
(
devent,
status,
message,
)
)
else:
info.append(
(
devent,
RESIZE.SKIPPED,
f"Resizing mapped device ({blockdev}) skipped "
"as it is not encrypted.",
)
)
except Exception as e:
info.append(
(
devent,
RESIZE.FAILED,
f"Resizing encrypted device ({blockdev}) failed: {e}",
)
)
# At this point, we WON'T resize a non-encrypted mapped device
# though we should probably grow the ability to
continue
try:
(disk, ptnum) = device_part_info(blockdev)
except (TypeError, ValueError) as e:
Expand Down
1 change: 1 addition & 0 deletions test-requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
httpretty>=0.7.1
pytest
pytest-cov
pytest-mock
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's add this to build-depends in packages/pkg-deps.json to make sure we are injecting that dep during package build/tests during local package builds from cloud-init tooling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As in add a new build-depends section to the debian section? Should we add other dependencies there too, like the reponses package we just added?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We looked over packages/bddeb (which sources information via the command ./tools/read-dependencies --distro debian during the local package build ./packages/bddeb. What I neglected to recall is that bddeb grew the ability to also read build dependencies from test-requirements.txt. So, I don't think we need to add those deps in pkg-deps.json too since you already added it to test-requirements.txt. You can validate that with python3-pytest-mock is listed in ./tools/read-dependencies --requirements-file test-requirements.txt --system-pkg-names


# Only really needed on older versions of python
setuptools
Expand Down
Loading