Skip to content

Commit

Permalink
autoinstall: Enforce top-level keys
Browse files Browse the repository at this point in the history
Subiquity has, until now, silently ignored invalid top-level keys.
This has likely been a major source of confusion of autoinstall
capabilities.

The following autoinstall config:

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

will now throw an AutoinstallValidationError.
  • Loading branch information
Chris-Peterson444 committed Jan 30, 2024
1 parent 34ffe1f commit 7170f85
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 9 deletions.
2 changes: 1 addition & 1 deletion autoinstall-schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -547,5 +547,5 @@
"required": [
"version"
],
"additionalProperties": true
"additionalProperties": false
}
2 changes: 1 addition & 1 deletion autoinstall-system-setup-schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -144,5 +144,5 @@
"required": [
"version"
],
"additionalProperties": true
"additionalProperties": false
}
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
not allowed and will result in an error.

.. _ai-schema:

Expand Down
17 changes: 12 additions & 5 deletions subiquity/server/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ class SubiquityServer(Application):
},
},
"required": ["version"],
"additionalProperties": True,
"additionalProperties": False,
}

project = "subiquity"
Expand Down Expand Up @@ -474,14 +474,21 @@ async def apply_autoinstall_config(self, context):
await controller.apply_autoinstall_config()
await controller.configured()

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

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

return return_dict

def validate_autoinstall(self):
with self.context.child("core_validation", level="INFO"):
try:
jsonschema.validate(self.autoinstall_config, self.base_schema)
stripped_config = self._strip_controller_keys(self.autoinstall_config)
jsonschema.validate(stripped_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 = "top-level keys"
new_exception: AutoinstallValidationError = AutoinstallValidationError(
section,
original_exception,
Expand Down
62 changes: 61 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,16 @@ async def asyncSetUp(self):
}
self.server.make_apport_report = Mock()

# Pseudo Load controllers to avoid patching loading logic for each controller
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 @@ -197,6 +209,54 @@ def test_autoinstall_validation__error_report_type(self):
exc=exception,
)

def test_autoinstall_validation__enforce_top_level_keys(self):
"""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()

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

exception = ctx.exception

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

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 7170f85

Please sign in to comment.