Skip to content

Commit

Permalink
autoinstall: prepare to enforce top-level keys
Browse files Browse the repository at this point in the history
Subiquity currently ignores invalid top-level keys, but this has likely
been a major source of confusion of autoinstall capabilities.

In the future, the following autoinstall config will throw an
AutoinstallValidationError:

    version: 1
    interactive-sections:
        - identity
    literally-anything: lmao

This patch adds warnings to the logger and the machinery to
conditionally warn or error depending on the specified autoinstall
version.
  • Loading branch information
Chris-Peterson444 committed Feb 6, 2024
1 parent b2441fd commit 7a5c552
Show file tree
Hide file tree
Showing 5 changed files with 152 additions and 10 deletions.
4 changes: 3 additions & 1 deletion doc/reference/autoinstall-reference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
Autoinstall configuration reference manual
==========================================

The autoinstall file uses the YAML format. At the top level, it must be a mapping containing the keys described in this document. Unrecognised keys are ignored.
The autoinstall file uses the YAML format. At the top level, it must be a
mapping containing the keys described in this document. Unrecognised keys are
currently ignored, but may cause a run-time failure in future versions.

.. _ai-schema:

Expand Down
12 changes: 9 additions & 3 deletions subiquity/server/autoinstall.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
import logging
from typing import Optional

log = logging.getLogger("subiquity.server.autoinstall")

Expand All @@ -24,8 +25,13 @@ class AutoinstallError(Exception):
class AutoinstallValidationError(AutoinstallError):
def __init__(
self,
owner: str,
section: str,
message: Optional[str] = None,
):
self.message = f"Malformed autoinstall in {owner!r} section"
self.owner = owner
if not message:
self.message: str = f"Malformed autoinstall in {section!r} section"
else:
self.message: str = message

self.section: str = section
super().__init__(self.message)
68 changes: 64 additions & 4 deletions subiquity/server/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.

import asyncio
import copy
import logging
import os
import sys
Expand Down Expand Up @@ -475,14 +476,73 @@ async def apply_autoinstall_config(self, context):
await controller.apply_autoinstall_config()
await controller.configured()

def _strip_controller_keys(self, autoinstall_config: dict) -> dict:
return_dict: dict = copy.deepcopy(autoinstall_config)

for controller in self.controllers.instances:
return_dict.pop(controller.autoinstall_key, None)

return return_dict

def _enforce_top_level_keys(self, base_config: dict) -> dict:
# Assumes base_config is the result of stripping away all
# controller related sections (via _strip_controller_keys)

top_level_keys: set[str] = set(base_config.keys())
valid_keys: set[str] = set(self.base_schema["properties"].keys())
bad_keys: set[str] = top_level_keys - valid_keys

# Return early if no bad keys
if not bad_keys:
return base_config

# If version early enough, only warn
version: int = base_config.get("version", None)

if not version:
raise AutoinstallValidationError(
section="top-level keys",
message="top-level key 'version' is missing",
)

if version <= 1:
for key in bad_keys:
log.warning(f"Unrecognized top-level autoinstall key {key!r}")

log.warning(
"Unrecognized keys may cause autoinstall to crash in future versions"
)

# Clean out bad keys
for key in bad_keys:
base_config.pop(key)

return base_config

# Otherwise, error
else:
for key in bad_keys:
log.error(f"Unrecognized top-level autoinstall key {key!r}")

raw_keys = (f"{key!r}" for key in bad_keys)
message: str = (
f"Unrecognized top-level autoinstall key(s): {', '.join(raw_keys)}",
)
raise AutoinstallValidationError(
section="top-level keys",
message=message,
)

def validate_autoinstall(self):
with self.context.child("core_validation", level="INFO"):
try:
jsonschema.validate(self.autoinstall_config, self.base_schema)
stripped_config: dict = self._strip_controller_keys(
self.autoinstall_config
)
base_config: dict = self._enforce_top_level_keys(stripped_config)
jsonschema.validate(base_config, self.base_schema)
except ValidationError as original_exception:
# SubiquityServer currently only checks for these sections
# of autoinstall. Hardcode until we have better validation.
section = "version or interative-sessions"
section: str = "top-level keys"
new_exception: AutoinstallValidationError = AutoinstallValidationError(
section,
)
Expand Down
2 changes: 1 addition & 1 deletion subiquity/server/tests/test_controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def test_autoinstall_validation(self):
exception = ctx.exception

# Assert error section is based on autoinstall_key
self.assertEquals(exception.owner, "some-key")
self.assertEquals(exception.section, "some-key")

# Assert apport report is not created
# This only checks that controllers do not manually create an apport
Expand Down
76 changes: 75 additions & 1 deletion subiquity/server/tests/test_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,9 @@ def test_bogus_autoinstall_argument(self):
with self.assertRaises(Exception):
self.server.select_autoinstall()

def test_early_commands_changes_autoinstall(self):
# Only care about changes to autoinstall, not validity
@patch("subiquity.server.server.SubiquityServer.validate_autoinstall")
def test_early_commands_changes_autoinstall(self, mocked_validator):
self.server.controllers = Mock()
self.server.controllers.instances = []
rootpath = self.path(root_autoinstall_path)
Expand Down Expand Up @@ -162,6 +164,17 @@ async def asyncSetUp(self):
}
self.server.make_apport_report = Mock()

# Pseudo Load controllers to avoid patching loading logic for each controller
# Only need to access class attributes
def pseudo_load_controllers(self):
controller_classes = []
for prefix in self.server.controllers.controller_names:
controller_classes.append(
self.server.controllers._get_controller_class(prefix)
)

self.server.controllers.instances = controller_classes

def test_valid_schema(self):
"""Test that the expected autoinstall JSON schema is valid"""

Expand Down Expand Up @@ -194,6 +207,67 @@ async def test_autoinstall_validation__no_error_report(self):

self.server.make_apport_report.assert_not_called()

@patch("subiquity.server.server.log")
def test_autoinstall_validation__enforce_top_level_keys(self, log_mock):
"""Test strict top level keys"""

# Reset base schema
self.server.base_schema = SubiquityServer.base_schema

# "apt" should be known by the MirrorController and not considered
# by the server's validation
bad_ai_data = {
"version": 1,
"apt": "Invalid but deferred",
"literally-anything": "lmao",
}

self.server.autoinstall_config = bad_ai_data

# Load the controllers
self.pseudo_load_controllers()

# OK in Version 1 but ensure warnings
self.server.validate_autoinstall()
log_mock.warning.assert_called()

# Not OK in Versions >=2
bad_ai_data["version"] = 2
self.server.autoinstall_config = bad_ai_data

log_mock.reset()

with self.assertRaises(AutoinstallValidationError) as ctx:
self.server.validate_autoinstall()

exception = ctx.exception

self.assertIn("literally-anything", str(exception))
self.assertNotIn("apt", str(exception))

log_mock.error.assert_called()

def test_autoinstall__strip_controller_keys(self):
"""Test only controller keys are stripped"""

# Mixed data: Has base sections, controller section, and a bad key
autoinstall_config = {
"version": 1, # Should stay
"interactive-sections": ["identity"], # Should stay
"apt": "...", # Should be stripped
"invalid_key": "...", # Should stay
}

# Load the controllers
self.pseudo_load_controllers()

result = self.server._strip_controller_keys(autoinstall_config)

self.assertIn("version", result)
self.assertIn("interactive-sections", result)
self.assertNotIn("apt", result)
self.assertIn("invalid_key", result)


class TestMetaController(SubiTestCase):
async def test_interactive_sections_not_present(self):
Expand Down

0 comments on commit 7a5c552

Please sign in to comment.