Skip to content

fix: warn about unknown manifest fields #918

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

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

fix: warn about unknown manifest fields #918

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
17 changes: 9 additions & 8 deletions sunbeam-python/sunbeam/core/deployment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(
Expand Down Expand Up @@ -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)
Expand All @@ -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(

@gboutry gboutry Sep 29, 2026

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 really not a fan of

try:
  ...
else:
  ...

Does this addition help the code make more sense?

@zhhuabj zhhuabj Sep 29, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi @gboutry , thanks for the review. This else is needed, see the following code:

try:
client = self.get_client()
manifest_data = yaml.safe_load(
client.cluster.get_latest_manifest()["data"]
)
except ClusterServiceUnavailableException:
...
except ConfigItemNotFoundException:
...
except ValueError:
LOG.debug("Failed to get clusterd client, might no be bootstrapped, ...")
else:
override_manifest = self.parse_manifest(manifest_data)
LOG.debug("Manifest loaded from clusterd")

Because parse_manifest() now raises ValueError for
unknown feature names, while the existing except ValueError is there to catch
get_client() failing (e.g. MAAS raising Clusterd address not set.).

If the parse stayed inside the try, a ValueError from an unknown feature in
the clusterd manifest would be swallowed by that handler and the code would
silently fall back to the embedded manifest — exactly the "silently ignored"
behaviour this PR removes.

So try covers "get a client and read the manifest", else covers "parse it".

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,"
Expand All @@ -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()
Expand Down
59 changes: 35 additions & 24 deletions sunbeam-python/sunbeam/core/manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -86,15 +97,15 @@ class CharmManifest(pydantic.BaseModel):
# )


class TerraformManifest(pydantic.BaseModel):
class TerraformManifest(ManifestModel):
source: Path = Field(description="Path to Terraform plan")

@pydantic.field_serializer("source")
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] = {}
Expand Down Expand Up @@ -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


Expand Down Expand Up @@ -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
Expand All @@ -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."
)
Expand All @@ -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

Expand All @@ -240,29 +251,29 @@ 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]

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
aliases: list[dict[str, Any]] | None = None
# 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")
Expand All @@ -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

Expand All @@ -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
Expand Down Expand Up @@ -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)

Expand All @@ -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()

Expand Down Expand Up @@ -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={})
Expand Down
4 changes: 4 additions & 0 deletions sunbeam-python/sunbeam/features/baremetal/feature_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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={})

Expand Down
2 changes: 2 additions & 0 deletions sunbeam-python/sunbeam/features/loadbalancer/feature.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ""
Expand Down
2 changes: 2 additions & 0 deletions sunbeam-python/sunbeam/features/tls/ca.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@


class _Certificate(pydantic.BaseModel):
model_config = pydantic.ConfigDict(extra="forbid")

certificate: str


Expand Down
2 changes: 2 additions & 0 deletions sunbeam-python/sunbeam/features/tls/vault.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@


class _Certificate(pydantic.BaseModel):
model_config = pydantic.ConfigDict(extra="forbid")

certificate: str


Expand Down
Loading
Loading