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.

Sign up for GitHub

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

Jump to bottom

remove PREFERRED capabilities #1840

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

remove PREFERRED capabilities #1840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 2 additions & 5 deletions subiquity/common/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,13 +350,12 @@ class GuidedCapability(enum.Enum):

CORE_BOOT_ENCRYPTED = enum.auto()
CORE_BOOT_UNENCRYPTED = enum.auto()
# These two are not valid as GuidedChoiceV2.capability:
CORE_BOOT_PREFER_ENCRYPTED = enum.auto()
CORE_BOOT_PREFER_UNENCRYPTED = enum.auto()

DD = enum.auto()

def __lt__(self, other) -> bool:
if self.is_core_boot() and other.is_core_boot():
return False
Copy link
Member

Choose a reason for hiding this comment

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

So GC.CORE_BOOT_ENCRYPTED and GC.CORE_BOOT_UNENCRYPTED will not compare equal but they will both compare "greater or equal" to each other? Am I getting this right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Additionally, if we just removed these lines what would be the result? As the sort order is said to matter now and the first one is what we're suggesting as a default, if the only choices were CORE_BOOT_{UN,}ENCRYPTED, which would we want to show first?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if the only choices were CORE_BOOT_{UN,}ENCRYPTED, which would we want to show first

Well that's kind of the point of all this, the model decides. We look at the storage-safety field, which comes from here:

https://github.com/snapcore/snapd/blob/master/asserts/model.go#L362-L377

We interpret prefer-encrypted as "the encryption check box should default on" and prefer-unencrypted as "the check box should default off" (the other fields disable the check box, they are easier). So the answer to your question is "it depends".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Kind of ugly to handle from the enum.

The problem with these lines is that, as previously noted, this produces unstable behavior. A stable sort that is not generalized but correct for the current model is preferable to one that could produce an unpredictable sort order.

We could note the future work here, if that helps.

return self.value < other.value

def is_lvm(self) -> bool:
Expand All @@ -366,8 +365,6 @@ def is_core_boot(self) -> bool:
return self in [
GuidedCapability.CORE_BOOT_ENCRYPTED,
GuidedCapability.CORE_BOOT_UNENCRYPTED,
GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED,
GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED,
]

def supports_manual_customization(self) -> bool:
Expand Down
40 changes: 9 additions & 31 deletions subiquity/server/controllers/filesystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,11 +394,13 @@ def disallowed_encryption(msg):
info.capability_info.allowed = [GuidedCapability.CORE_BOOT_ENCRYPTED]
elif se.storage_safety == StorageSafety.PREFER_ENCRYPTED:
info.capability_info.allowed = [
GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED
GuidedCapability.CORE_BOOT_ENCRYPTED,
GuidedCapability.CORE_BOOT_UNENCRYPTED,
]
elif se.storage_safety == StorageSafety.PREFER_UNENCRYPTED:
info.capability_info.allowed = [
GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED
GuidedCapability.CORE_BOOT_UNENCRYPTED,
GuidedCapability.CORE_BOOT_ENCRYPTED,
]

return info
Expand Down Expand Up @@ -651,26 +653,8 @@ def start_guided_resize(

def set_info_for_capability(self, capability: GuidedCapability):
"""Given a request for a capability, select the variation to use."""
if capability == GuidedCapability.CORE_BOOT_ENCRYPTED:
# If the request is for encryption, a variation with any
# of these capabilities is OK:
caps = {
GuidedCapability.CORE_BOOT_ENCRYPTED,
GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED,
GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED,
}
elif capability == GuidedCapability.CORE_BOOT_UNENCRYPTED:
# Similar if the request is for uncrypted
caps = {
GuidedCapability.CORE_BOOT_UNENCRYPTED,
GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED,
GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED,
}
else:
# Otherwise, just look for what we were asked for.
caps = {capability}
for info in self._variation_info.values():
if caps & set(info.capability_info.allowed):
if capability in info.capability_info.allowed:
self._info = info
return
raise Exception("could not find variation for {}".format(capability))
Expand Down Expand Up @@ -1313,12 +1297,12 @@ async def run_autoinstall_guided(self, layout):
if name == "hybrid":
# this check is conceptually unnecessary but results in a
# much cleaner error message...
core_boot_caps = set()
core_boot_caps = []
for variation in self._variation_info.values():
if not variation.is_valid():
continue
if variation.is_core_boot_classic():
core_boot_caps.update(variation.capability_info.allowed)
core_boot_caps.extend(variation.capability_info.allowed)
if not core_boot_caps:
raise Exception(
"can only use name: hybrid when installing core boot classic"
Expand All @@ -1328,18 +1312,12 @@ async def run_autoinstall_guided(self, layout):
encrypted = layout.get("encrypted", None)
GC = GuidedCapability
if encrypted is None:
if (
GC.CORE_BOOT_ENCRYPTED in core_boot_caps
or GC.CORE_BOOT_PREFER_ENCRYPTED in core_boot_caps
):
capability = GC.CORE_BOOT_ENCRYPTED
else:
capability = GC.CORE_BOOT_UNENCRYPTED
capability = core_boot_caps[0]
elif encrypted:
capability = GC.CORE_BOOT_ENCRYPTED
else:
if (
core_boot_caps == {GuidedCapability.CORE_BOOT_ENCRYPTED}
core_boot_caps == [GuidedCapability.CORE_BOOT_ENCRYPTED]
and not encrypted
):
raise Exception("cannot install this model unencrypted")
Expand Down
40 changes: 35 additions & 5 deletions subiquity/server/controllers/tests/test_filesystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,19 +696,26 @@ async def test_bad_modes(self, mode):


class TestGuidedV2(IsolatedAsyncioTestCase):
async def _setup(self, bootloader, ptable, fix_bios=True, **kw):
async def _setup(
self, bootloader, ptable, fix_bios=True, snapd_system_labels=(None,), **kw
):
self.app = make_app()
self.app.opts.bootloader = bootloader.value
self.fsc = FilesystemController(app=self.app)
self.fsc.calculate_suggested_install_min = mock.Mock()
self.fsc.calculate_suggested_install_min.return_value = 10 << 30
self.fsc.model = self.model = make_model(bootloader)
self.fsc._examine_systems_task.start_sync()
self.app.snapdapi = snapdapi.make_api_client(AsyncSnapd(get_fake_connection()))
self.app.dr_cfg = DRConfig()
self.app.dr_cfg.systems_dir_exists = True
self.app.base_model.source.search_drivers = False
self.app.base_model.source.current.type = "fsimage"
self.app.base_model.source.current.variations = {
"default": CatalogEntryVariation(path="", size=1),
}
variations = self.app.base_model.source.current.variations = {}
for label in snapd_system_labels:
variations["var" + str(label)] = CatalogEntryVariation(
path="", size=1, snapd_system_label=label
)
self.app.controllers.Source.get_handler.return_value = TrivialSourceHandler("")
await self.fsc._examine_systems_task.wait()
self.disk = make_disk(self.model, ptable=ptable, **kw)
Expand All @@ -719,7 +726,7 @@ async def _setup(self, bootloader, ptable, fix_bios=True, **kw):
"filesystem": self.fs_probe,
}
self.fsc._probe_task.task = mock.Mock()
self.fsc._examine_systems_task.task = mock.Mock()
# self.fsc._examine_systems_task.task = mock.Mock()
if bootloader == Bootloader.BIOS and ptable != "msdos" and fix_bios:
make_partition(
self.model,
Expand Down Expand Up @@ -1153,6 +1160,29 @@ async def test_in_use_reformat_and_gap(self, bootloader, ptable):
self.assertEqual(expected, resp.targets)
self.assertEqual(ProbeStatus.DONE, resp.status)

@parameterized.expand(
[
("mandatory", ["ENCRYPTED"]),
("prefer-encrypted", ["ENCRYPTED", "UNENCRYPTED"]),
("prefer-unencrypted", ["UNENCRYPTED", "ENCRYPTED"]),
("unavailable", ["UNENCRYPTED"]),
]
)
async def test_core_boot_and_classic(self, label, core_boot_cap_names):
expected_core_boot_caps = [
getattr(GuidedCapability, f"CORE_BOOT_{cap_name}")
for cap_name in core_boot_cap_names
]
await self._setup(
Bootloader.UEFI,
"gpt",
snapd_system_labels=[None, label],
)
resp = await self.fsc.v2_guided_GET()
[target, man] = resp.targets
got_core_boot_caps = [cap for cap in target.allowed if cap.is_core_boot()]
self.assertEqual(got_core_boot_caps, expected_core_boot_caps)


class TestManualBoot(IsolatedAsyncioTestCase):
def _setup(self, bootloader, ptable, **kw):
Expand Down
2 changes: 1 addition & 1 deletion subiquity/tests/api/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,7 @@ async def test_basic_core_boot(self):
await inst.post("/source", source_id="ubuntu-desktop")
resp = await inst.get("/storage/v2/guided", wait=True)
[reformat, manual] = resp["targets"]
self.assertIn("CORE_BOOT_PREFER_ENCRYPTED", reformat["allowed"])
self.assertIn("CORE_BOOT_ENCRYPTED", reformat["allowed"])
data = dict(target=reformat, capability="CORE_BOOT_ENCRYPTED")
await inst.post("/storage/v2/guided", data)
v2resp = await inst.get("/storage/v2")
Expand Down
52 changes: 24 additions & 28 deletions subiquity/ui/views/filesystem/guided.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,39 +134,23 @@ class TPMChoice:
help: str


tpm_help_texts = {
"AVAILABLE_CAN_BE_DESELECTED": _(
"The entire disk will be encrypted and protected by the "
"TPM. If this option is deselected, the disk will be "
"unencrypted and without any protection."
),
"AVAILABLE_CANNOT_BE_DESELECTED": _(
tpm_help_texts_1 = {
GuidedCapability.CORE_BOOT_ENCRYPTED: _(
"The entire disk will be encrypted and protected by the TPM."
),
"UNAVAILABLE":
GuidedCapability.CORE_BOOT_UNENCRYPTED:
# for translators: 'reason' is the reason FDE is unavailable.
_(
"TPM backed full-disk encryption is not available "
'on this device (the reason given was "{reason}").'
),
}

choices = {
GuidedCapability.CORE_BOOT_ENCRYPTED: TPMChoice(
enabled=False,
default=True,
help=tpm_help_texts["AVAILABLE_CANNOT_BE_DESELECTED"],
),
GuidedCapability.CORE_BOOT_UNENCRYPTED: TPMChoice(
enabled=False, default=False, help=tpm_help_texts["UNAVAILABLE"]
),
GuidedCapability.CORE_BOOT_PREFER_ENCRYPTED: TPMChoice(
enabled=True, default=True, help=tpm_help_texts["AVAILABLE_CAN_BE_DESELECTED"]
),
GuidedCapability.CORE_BOOT_PREFER_UNENCRYPTED: TPMChoice(
enabled=True, default=False, help=tpm_help_texts["AVAILABLE_CAN_BE_DESELECTED"]
),
}
tpm_help_texts_can_be_deselected = _(
"The entire disk will be encrypted and protected by the "
"TPM. If this option is deselected, the disk will be "
"unencrypted and without any protection."
)


class GuidedChoiceForm(SubForm):
Expand Down Expand Up @@ -223,16 +207,28 @@ def _select_disk(self, sender, val):
self.use_lvm.enabled = GuidedCapability.LVM in val.allowed
core_boot_caps = [c for c in val.allowed if c.is_core_boot()]
if core_boot_caps:
assert len(val.allowed) == 1
cap = core_boot_caps[0]
assert len(val.allowed) == len(core_boot_caps)

reason = ""
for disallowed in val.disallowed:
if disallowed.capability == GuidedCapability.CORE_BOOT_ENCRYPTED:
reason = disallowed.message
self.tpm_choice = choices[cap]

cap0 = core_boot_caps[0]
if len(core_boot_caps) == 1:
self.tpm_choice = TPMChoice(
enabled=False,
default=cap0 == GuidedCapability.CORE_BOOT_ENCRYPTED,
help=tpm_help_texts_1[cap0],
)
else:
self.tpm_choice = TPMChoice(
enabled=True,
default=cap0 == GuidedCapability.CORE_BOOT_ENCRYPTED,
help=tpm_help_texts_can_be_deselected,
)
self.use_tpm.enabled = self.tpm_choice.enabled
self.use_tpm.value = self.tpm_choice.default
self.use_tpm.help = self.tpm_choice.help
self.use_tpm.help = self.tpm_choice.help.format(reason=reason)
else:
self.use_tpm.enabled = False
Expand Down
Loading