diff --git a/sunbeam-python/sunbeam/core/deployment.py b/sunbeam-python/sunbeam/core/deployment.py index 4abc8c6ff..17dcdb9e3 100644 --- a/sunbeam-python/sunbeam/core/deployment.py +++ b/sunbeam-python/sunbeam/core/deployment.py @@ -273,8 +273,7 @@ def _parse_feature( feature = features.get(name) group = groups.get(name) if not feature and not group: - LOG.warning("Feature %s is not found in feature manager", name) - continue + raise ValueError(f"Feature {name!r} is not found in feature manager") if feature and feature_or_group_manifest_dict: feature_manifests[name] = _parse_feature( feature, feature_or_group_manifest_dict @@ -287,10 +286,9 @@ def _parse_feature( ) in feature_or_group_manifest_dict.items(): feature = features.get(group.name + "." + name) if not feature: - LOG.warning( - "Feature %s is not found in group %s", name, group.name + raise ValueError( + f"Feature {name!r} is not found in group {group.name!r}" ) - continue if not feature_manifest_dict: continue group_manifest.root[name] = _parse_feature( @@ -332,6 +330,7 @@ def parse_storage_manifest( def parse_manifest(self, manifest_data: dict) -> Manifest: """Parse manifest data.""" + manifest_data = copy.deepcopy(manifest_data) features = manifest_data.pop("features", {}) storage = manifest_data.pop("storage", {}) manifest = Manifest.model_validate(manifest_data) @@ -357,10 +356,9 @@ def get_manifest(self, manifest_file: pathlib.Path | None = None) -> Manifest: else: try: client = self.get_client() - override_manifest = self.parse_manifest( - yaml.safe_load(client.cluster.get_latest_manifest()["data"]) + manifest_data = yaml.safe_load( + client.cluster.get_latest_manifest()["data"] ) - LOG.debug("Manifest loaded from clusterd") except ClusterServiceUnavailableException: LOG.debug( "Failed to get manifest from clusterd, might not be bootstrapped," @@ -376,6 +374,9 @@ def get_manifest(self, manifest_file: pathlib.Path | None = None) -> Manifest: "Failed to get clusterd client, might no be bootstrapped," " consider empty manifest from database" ) + else: + override_manifest = self.parse_manifest(manifest_data) + LOG.debug("Manifest loaded from clusterd") if override_manifest is None: # Only get manifest from embedded if manifest not present in clusterd snap = Snap() diff --git a/sunbeam-python/sunbeam/core/manifest.py b/sunbeam-python/sunbeam/core/manifest.py index 260bb0eab..1e03c7f1d 100644 --- a/sunbeam-python/sunbeam/core/manifest.py +++ b/sunbeam-python/sunbeam/core/manifest.py @@ -40,7 +40,18 @@ def embedded_manifest_path(snap: Snap, version: str, risk: str) -> Path: return snap.paths.snap / "etc" / "manifests" / version / f"{risk}.yml" -class JujuManifest(pydantic.BaseModel): +class ManifestModel(pydantic.BaseModel): + """Base model for fixed manifest schemas. + + Unknown keys are rejected rather than ignored: a misspelled or misplaced + field would otherwise be silently dropped, leaving configuration that is + hard to detect and clean up later. + """ + + model_config = pydantic.ConfigDict(extra="forbid") + + +class JujuManifest(ManifestModel): # Setting Field alias not supported in pydantic 1.10.0 # Old version of pydantic is used due to dependencies # with older version of paramiko from python-libjuju @@ -86,7 +97,7 @@ class CharmManifest(pydantic.BaseModel): # ) -class TerraformManifest(pydantic.BaseModel): +class TerraformManifest(ManifestModel): source: Path = Field(description="Path to Terraform plan") @pydantic.field_serializer("source") @@ -94,7 +105,7 @@ def _serialize_source(self, value: Path) -> str: return str(value) -class SoftwareConfig(pydantic.BaseModel): +class SoftwareConfig(ManifestModel): juju: JujuManifest = JujuManifest() charms: dict[str, CharmManifest] = {} terraform: dict[str, TerraformManifest] = {} @@ -143,7 +154,7 @@ def merge(self, other: "SoftwareConfig") -> "SoftwareConfig": return SoftwareConfig(juju=juju, charms=charms, terraform=terraform) -class FeatureConfig(pydantic.BaseModel): +class FeatureConfig(ManifestModel): pass @@ -187,25 +198,25 @@ def _str_serialize(value: Any | None) -> str | None: return None -class CoreConfig(pydantic.BaseModel): - class _ProxyConfig(pydantic.BaseModel): +class CoreConfig(ManifestModel): + class _ProxyConfig(ManifestModel): proxy_required: bool | None = None http_proxy: str | None = None https_proxy: str | None = None no_proxy: str | None = None - class _BootstrapConfig(pydantic.BaseModel): + class _BootstrapConfig(ManifestModel): management_cidr: str | None = pydantic.Field( default=None, description="Management network CIDR" ) - class _Addons(pydantic.BaseModel): + class _Addons(ManifestModel): metallb: str | None = None - class _K8sAddons(pydantic.BaseModel): + class _K8sAddons(ManifestModel): loadbalancer: str | None = None - class _User(pydantic.BaseModel): + class _User(ManifestModel): run_demo_setup: bool | None = None username: str | None = None password: str | None = None @@ -216,7 +227,7 @@ class _User(pydantic.BaseModel): # Default physnet for user demo network physnet: str | None = None - class _ExternalNetwork(pydantic.BaseModel): + class _ExternalNetwork(ManifestModel): nic: str | None = pydantic.Field( None, deprecated="Deprecated. Use `nics` instead." ) @@ -229,7 +240,7 @@ class _ExternalNetwork(pydantic.BaseModel): network_type: typing.Literal["vlan", "flat"] | None = None segmentation_id: int | None = None - class _HostMicroCephConfig(pydantic.BaseModel): + class _HostMicroCephConfig(ManifestModel): osd_devices: list[str] | None = None dangerous_i_acknowledge_i_will_lose_data_wipe_disks: bool = False @@ -240,12 +251,12 @@ def _validate_osd_devices(cls, v): return v.split(",") return v - class _Identity(pydantic.BaseModel): - class _IdentitySAML2KeyAndCert(pydantic.BaseModel): + class _Identity(ManifestModel): + class _IdentitySAML2KeyAndCert(ManifestModel): certificate: str key: str - class _IdentityProfile(pydantic.BaseModel): + class _IdentityProfile(ManifestModel): provider: str protocol: str config: dict[str, str] @@ -253,7 +264,7 @@ class _IdentityProfile(pydantic.BaseModel): profiles: dict[str, _IdentityProfile] saml2_x509: _IdentitySAML2KeyAndCert - class _PCI(pydantic.BaseModel): + class _PCI(ManifestModel): # Source: https://docs.openstack.org/nova/latest/configuration/config.html#pci.device_spec device_specs: list[dict[str, Any]] | None = None # https://docs.openstack.org/nova/latest/configuration/config.html#pci.alias @@ -261,8 +272,8 @@ class _PCI(pydantic.BaseModel): # Excluded PCI addresses per node. excluded_devices: dict[str, list[str]] | None = None - class _HorizonConfig(pydantic.BaseModel): - class _Resources(pydantic.BaseModel): + class _HorizonConfig(ManifestModel): + class _Resources(ManifestModel): custom_theme: Path | None = None @pydantic.field_validator("custom_theme", mode="before") @@ -274,8 +285,8 @@ def _validate_custom_theme(cls, v): resources: _Resources | None = None - class _Endpoints(pydantic.BaseModel): - class _Endpoint(pydantic.BaseModel): + class _Endpoints(ManifestModel): + class _Endpoint(ManifestModel): hostname: str | None = None ip: pydantic.IPvAnyAddress | None = None @@ -285,7 +296,7 @@ class _Endpoint(pydantic.BaseModel): ingress_public: _Endpoint | None = pydantic.Field(None, alias="ingress-public") ingress_rgw: _Endpoint | None = pydantic.Field(None, alias="ingress-rgw") - class _DPDK(pydantic.BaseModel): + class _DPDK(ManifestModel): enabled: bool = False datapath_cores: int = 0 control_plane_cores: int = 0 @@ -321,7 +332,7 @@ class _DPDK(pydantic.BaseModel): dpdk: _DPDK | None = None -class CoreManifest(pydantic.BaseModel): +class CoreManifest(ManifestModel): config: CoreConfig = CoreConfig() software: SoftwareConfig = pydantic.Field(default_factory=_default_software_config) @@ -340,7 +351,7 @@ def merge(self, other: "CoreManifest") -> "CoreManifest": T = typing.TypeVar("T", bound=pydantic.BaseModel) -class _AddonManifest(pydantic.BaseModel, typing.Generic[T]): +class _AddonManifest(ManifestModel, typing.Generic[T]): config: pydantic.SerializeAsAny[T] | None = None software: SoftwareConfig = SoftwareConfig() @@ -410,7 +421,7 @@ def validate_againt_default(self, default_manifest: "FeatureGroupManifest") -> N ) -class Manifest(pydantic.BaseModel): +class Manifest(ManifestModel): core: CoreManifest = pydantic.Field(default_factory=CoreManifest) features: dict[str, FeatureManifest | FeatureGroupManifest] = {} storage: StorageManifest = StorageManifest(root={}) diff --git a/sunbeam-python/sunbeam/features/baremetal/feature_config.py b/sunbeam-python/sunbeam/features/baremetal/feature_config.py index 58d612c6d..edb53ff2d 100644 --- a/sunbeam-python/sunbeam/features/baremetal/feature_config.py +++ b/sunbeam-python/sunbeam/features/baremetal/feature_config.py @@ -79,6 +79,8 @@ class _Config(pydantic.BaseModel): + model_config = pydantic.ConfigDict(extra="forbid") + configfile: str additional_files: dict[str, str] = pydantic.Field( alias="additional-files", @@ -87,6 +89,8 @@ class _Config(pydantic.BaseModel): class _SwitchConfigs(pydantic.BaseModel): + model_config = pydantic.ConfigDict(extra="forbid") + netconf: dict[str, _Config] = pydantic.Field(default={}) generic: dict[str, _Config] = pydantic.Field(default={}) diff --git a/sunbeam-python/sunbeam/features/loadbalancer/feature.py b/sunbeam-python/sunbeam/features/loadbalancer/feature.py index ca055106d..80dfe2bf3 100644 --- a/sunbeam-python/sunbeam/features/loadbalancer/feature.py +++ b/sunbeam-python/sunbeam/features/loadbalancer/feature.py @@ -202,6 +202,8 @@ def _build_nad_yaml( class _CertificateEntry(pydantic.BaseModel): """A single signed certificate plus its CA material, keyed by CSR subject.""" + model_config = pydantic.ConfigDict(extra="forbid") + certificate: str = "" ca_certificate: str = "" ca_chain: str = "" diff --git a/sunbeam-python/sunbeam/features/tls/ca.py b/sunbeam-python/sunbeam/features/tls/ca.py index 5ec2fd502..234885991 100644 --- a/sunbeam-python/sunbeam/features/tls/ca.py +++ b/sunbeam-python/sunbeam/features/tls/ca.py @@ -57,6 +57,8 @@ class _Certificate(pydantic.BaseModel): + model_config = pydantic.ConfigDict(extra="forbid") + certificate: str diff --git a/sunbeam-python/sunbeam/features/tls/vault.py b/sunbeam-python/sunbeam/features/tls/vault.py index 3410ec4ad..c47ba1fec 100644 --- a/sunbeam-python/sunbeam/features/tls/vault.py +++ b/sunbeam-python/sunbeam/features/tls/vault.py @@ -64,6 +64,8 @@ class _Certificate(pydantic.BaseModel): + model_config = pydantic.ConfigDict(extra="forbid") + certificate: str diff --git a/sunbeam-python/tests/unit/sunbeam/core/test_deployment.py b/sunbeam-python/tests/unit/sunbeam/core/test_deployment.py index a2ca3afe2..faa18820d 100644 --- a/sunbeam-python/tests/unit/sunbeam/core/test_deployment.py +++ b/sunbeam-python/tests/unit/sunbeam/core/test_deployment.py @@ -1,10 +1,12 @@ # SPDX-FileCopyrightText: 2024 - Canonical Ltd # SPDX-License-Identifier: Apache-2.0 +import copy import functools from pathlib import Path from unittest.mock import Mock, call, patch +import pydantic import pytest import yaml @@ -54,6 +56,9 @@ def deployment(mocker, snap): dep.parse_manifest.side_effect = functools.partial( Deployment.parse_manifest, dep ) + dep.parse_feature_manifest.side_effect = functools.partial( + Deployment.parse_feature_manifest, dep + ) dep._load_tfhelpers.side_effect = functools.partial( Deployment._load_tfhelpers, dep ) @@ -81,6 +86,99 @@ def test_get_default_manifest(self, deployment: Deployment): set(manifest.core.software.terraform.keys()) >= TERRAFORM_DIR_NAMES.keys() ) + def test_parse_manifest_rejects_unknown_top_level_keys( + self, deployment: Deployment + ): + with pytest.raises(pydantic.ValidationError) as exc_info: + deployment.parse_manifest( + { + "preseed": { + "user": {"remote_access_location": "remote"}, + }, + } + ) + assert exc_info.value.errors()[0]["loc"] == ("preseed",) + assert exc_info.value.errors()[0]["type"] == "extra_forbidden" + + def test_parse_manifest_rejects_unknown_nested_keys(self, deployment: Deployment): + with pytest.raises(pydantic.ValidationError) as exc_info: + deployment.parse_manifest( + { + "core": { + "config": { + "user": { + "remote_access_locaion": "remote", + }, + }, + }, + } + ) + assert exc_info.value.errors()[0]["loc"] == ( + "core", + "config", + "user", + "remote_access_locaion", + ) + assert exc_info.value.errors()[0]["type"] == "extra_forbidden" + + def test_parse_manifest_rejects_alias_only_field_name(self, deployment: Deployment): + with pytest.raises(pydantic.ValidationError) as exc_info: + deployment.parse_manifest( + {"core": {"config": {"k8s_addons": {"loadbalancer": "metallb"}}}} + ) + assert exc_info.value.errors()[0]["loc"] == ( + "core", + "config", + "k8s_addons", + ) + assert exc_info.value.errors()[0]["type"] == "extra_forbidden" + + def test_parse_manifest_accepts_alias_field_name(self, deployment: Deployment): + manifest = deployment.parse_manifest( + {"core": {"config": {"k8s-addons": {"loadbalancer": "metallb"}}}} + ) + assert manifest.core.config.k8s_addons is not None + assert manifest.core.config.k8s_addons.loadbalancer == "metallb" + + def test_parse_manifest_preserves_invalid_value_errors( + self, deployment: Deployment + ): + with pytest.raises(pydantic.ValidationError) as exc_info: + deployment.parse_manifest( + { + "core": { + "config": { + "external-network": {"network_type": "invalid"}, + }, + }, + } + ) + assert exc_info.value.errors()[0]["loc"] == ( + "core", + "config", + "external-network", + "network_type", + ) + + def test_parse_manifest_rejects_unknown_features(self, deployment: Deployment): + deployment.get_feature_manager.return_value.features.return_value = {} + deployment.get_feature_manager.return_value.groups.return_value = {} + + with pytest.raises(ValueError, match="loadbalncer"): + deployment.parse_manifest({"features": {"loadbalncer": {}}}) + + def test_parse_manifest_does_not_mutate_input(self, deployment: Deployment): + manifest_data = { + "core": {"config": {"user": {"remote_access_location": "remote"}}}, + "features": {}, + "storage": {}, + } + original_manifest_data = copy.deepcopy(manifest_data) + + deployment.parse_manifest(manifest_data) + + assert manifest_data == original_manifest_data + def test_load_on_default(self, deployment: Deployment, tmpdir): manifest_file = tmpdir.mkdir("manifests").join("test_manifest.yaml") manifest_file.write(test_manifest) @@ -115,6 +213,20 @@ def test_load_latest_from_clusterdb(self, deployment: Deployment): assert nova_manifest.revision is None assert nova_manifest.config is None + def test_invalid_manifest_value_from_clusterdb_is_not_ignored( + self, deployment: Deployment + ): + client = Mock() + client.cluster.get_latest_manifest.return_value = { + "data": "core:\n config:\n external-network:\n" + " network_type: invalid\n" + } + deployment.get_client.side_effect = None + deployment.get_client.return_value = client + + with pytest.raises(pydantic.ValidationError, match="literal_error"): + deployment.get_manifest() + def test_get_tfhelper(self, mocker, snap, copytree, deployment: Deployment): tfplan = "k8s-plan" mocker.patch.object(deployment_mod, "Snap", return_value=snap) diff --git a/sunbeam-python/tests/unit/sunbeam/core/test_manifest.py b/sunbeam-python/tests/unit/sunbeam/core/test_manifest.py index f9bc3dcb9..679adb4fb 100644 --- a/sunbeam-python/tests/unit/sunbeam/core/test_manifest.py +++ b/sunbeam-python/tests/unit/sunbeam/core/test_manifest.py @@ -60,17 +60,19 @@ """ test_manifest_incorrect_terraform_key = { - "software": { - "charms": { - "keystone-k8s": { - "channel": "2023.1/stable", - "revision": 234, - "config": {"debug": True}, - } - }, - "terraform": { - "fake-plan": {"source": "/home/ubuntu/tfplan"}, - }, + "core": { + "software": { + "charms": { + "keystone-k8s": { + "channel": "2023.1/stable", + "revision": 234, + "config": {"debug": True}, + } + }, + "terraform": { + "fake-plan": {"source": "/home/ubuntu/tfplan"}, + }, + } } } diff --git a/sunbeam-python/tests/unit/sunbeam/features/test_tls.py b/sunbeam-python/tests/unit/sunbeam/features/test_tls.py index ce475bcfe..f5ab4c6e2 100644 --- a/sunbeam-python/tests/unit/sunbeam/features/test_tls.py +++ b/sunbeam-python/tests/unit/sunbeam/features/test_tls.py @@ -5,9 +5,11 @@ from unittest.mock import Mock, patch import click +import pydantic import pytest import sunbeam.core.questions +import sunbeam.features.tls.ca as ca import sunbeam.features.tls.common as tls import sunbeam.features.tls.self_signed as self_signed import sunbeam.features.tls.vault as vault @@ -84,6 +86,24 @@ def vault_is_certificate_valid(): yield p +class TestTlsFeatureConfig: + @pytest.mark.parametrize( + "config_type", + [ca.CaTlsFeatureConfig, vault.VaultTlsFeatureConfig], + ) + def test_rejects_unknown_certificate_fields(self, config_type): + with pytest.raises(pydantic.ValidationError, match="extra_forbidden"): + config_type.model_validate( + { + "certificates": { + "subject": { + "certifcate": "certificate-data", + }, + }, + } + ) + + class TestAddCACertsToKeystoneStep: def test_is_skip(self, jhelper, step_context): name = "cabundle"