From 0752f0c043698dc1ef66266413ad08d373975989 Mon Sep 17 00:00:00 2001 From: RonaldHensbergen Date: Tue, 25 Aug 2026 22:24:00 +0200 Subject: [PATCH 1/5] Add profile composition via extends (#175) Profiles can now declare a top-level `extends: [name-or-path, ...]` field to compose from one or more parent profiles instead of duplicating shared configuration. Parents are resolved and merged left-to-right (later parents win), then the child profile is merged on top of all parents; extends chains are transitive. This reuses the exact deep-merge/module-merge-by-id engine and provenance model built for environment overlays (#229), rather than introducing a second merge engine, per the issue's explicit constraint. New cli/overlay.py helpers: - _merge_profile_docs(): the merge step shared by environment overlays and extends composition (previously duplicated inline in resolve_profile()). - _compose_extends(): resolves a profile's extends chain recursively, detecting cycles (E106), non-list/empty extends (E103), parents that resolve outside the profiles root (E104), and missing parents (E105). - resolve_extends(): a lighter entry point (no environment overlay, no full validate_loaded_profile()) used by build_plan()/validate_profile() so extends applies even without --environment, while preserving their own defensive diagnostic handling for malformed profiles. cli/planner.py's build_plan() and cli/validator.py's validate_profile() now resolve `extends` unconditionally rather than only when an --environment is selected, since extends is a property of the profile file itself. extends and --environment compose together: parents are merged first, then the child, then the environment overlay on top. Added `extends` to cli/resources/profile.schema.json and documented the feature in README.md alongside Environment Overlays. Closes #175. --- README.md | 39 ++++ cli/overlay.py | 333 ++++++++++++++++++++++-------- cli/planner.py | 18 +- cli/resources/profile.schema.json | 9 + cli/validator.py | 23 +-- tests/test_overlay.py | 300 +++++++++++++++++++++++++++ tests/test_planner.py | 55 +++++ 7 files changed, 670 insertions(+), 107 deletions(-) diff --git a/README.md b/README.md index 44a15ec7..ee16268e 100644 --- a/README.md +++ b/README.md @@ -602,6 +602,45 @@ before and never look for an `environments/` directory. --- +### Profile Composition (`extends`) + +A profile can also factor out shared configuration into one or more parent +profiles instead of duplicating it, using a top-level `extends` field: + +```yaml +# profiles/analytics-prod/profile.yaml +apiVersion: cds/v1alpha1 +kind: Profile +metadata: + name: analytics-prod + environment: production +extends: + - analytics-base # a bare name resolves to profiles/analytics-base/profile.yaml +spec: + modules: + - id: postgres + config: + storage: + size: 20Gi +``` + +`extends` accepts a non-empty list of parent references, each either a bare +profile name (resolved under the profiles root) or a path relative to the +child profile's directory (e.g. `../shared/profile.yaml`). Parents are +resolved and merged left-to-right — later parents win over earlier ones — +and then the child profile's own document is merged on top of all parents. +Composition uses the exact same deep-merge/module-merge-by-id engine as +environment overlays: mappings merge recursively, `spec.modules` entries +merge by stable `id`, and any other array is replaced wholesale rather than +concatenated. Parent profiles may themselves use `extends` (chains are +resolved transitively); cycles, missing parents, and parents that resolve +outside the profiles root are rejected with a diagnostic before anything +else runs. `extends` and `--environment` compose together: parents are +merged first, then the child, then the selected environment overlay is +applied on top of that fully-composed result. + +--- + ## ⚙️ CLI |Command|Description| diff --git a/cli/overlay.py b/cli/overlay.py index 5349c37f..3b60ebed 100644 --- a/cli/overlay.py +++ b/cli/overlay.py @@ -53,6 +53,235 @@ def _merge_value( return overlay +def _validate_modules_shape(label: str, modules: Any) -> list[Diagnostic]: + """ + Shared spec.modules shape validation: used for both environment overlays + and extends parents/child so extends does not need a second validation + path. Returns error diagnostics only; an empty list means modules is a + well-formed list of module mappings with unique, present ids. + """ + diagnostics: list[Diagnostic] = [] + + if not isinstance(modules, list): + return [ + Diagnostic( + level="error", + code="E093", + message=f"spec.modules in {label} must be a list, got {type(modules).__name__}.", + path="spec.modules", + ) + ] + + non_dict_indices = [i for i, m in enumerate(modules) if not isinstance(m, dict)] + if non_dict_indices: + diagnostics.append( + Diagnostic( + level="error", + code="E093", + message=( + f"Module entr{'y' if len(non_dict_indices) == 1 else 'ies'} in {label} " + f"must be a mapping, not a scalar/list, at index {non_dict_indices}." + ), + path="spec.modules", + ) + ) + return diagnostics + + missing_id = [i for i, m in enumerate(modules) if not m.get("id")] + if missing_id: + diagnostics.append( + Diagnostic( + level="error", + code="E093", + message=f"Module entr{'y' if len(missing_id) == 1 else 'ies'} in {label} missing required 'id' at index {missing_id}.", + path="spec.modules", + ) + ) + return diagnostics + + dupes = _duplicate_module_ids(modules) + if dupes: + diagnostics.append( + Diagnostic( + level="error", + code="E093", + message=f"Duplicate module id(s) in {label}: {sorted(dupes)}.", + path="spec.modules", + ) + ) + + return diagnostics + + +def _merge_profile_docs( + base: dict[str, Any], + overlay: dict[str, Any], + base_source: str, + overlay_source: str, + provenance: dict[str, str], +) -> tuple[dict[str, Any], list[Diagnostic]]: + """ + Merges two profile-shaped documents (deep-merge for everything except + spec.modules, which is merged by stable id). This is the single merge + path shared by environment overlays and `extends` composition, per + issue #175's requirement to reuse the resolver built for #229 rather + than introduce a second merge engine. + """ + diagnostics: list[Diagnostic] = [] + + base_spec = base.get("spec", {}) + base_modules = base_spec.get("modules", []) if isinstance(base_spec, dict) else [] + overlay_spec = overlay.get("spec", {}) + overlay_modules = overlay_spec.get("modules", []) if isinstance(overlay_spec, dict) else [] + + for label, modules in ((base_source, base_modules), (overlay_source, overlay_modules)): + diagnostics += _validate_modules_shape(label, modules) + if any(d.level == "error" for d in diagnostics): + return {}, diagnostics + + base_without_modules = dict(base) + base_spec_without_modules = ( + {k: v for k, v in base_spec.items() if k != "modules"} if isinstance(base_spec, dict) else {} + ) + base_without_modules["spec"] = base_spec_without_modules + + overlay_without_modules = dict(overlay) + if isinstance(overlay.get("spec"), dict): + overlay_without_modules["spec"] = {k: v for k, v in overlay["spec"].items() if k != "modules"} + + merged = _merge_value( + base_without_modules, overlay_without_modules, base_source, overlay_source, "", provenance + ) + merged.setdefault("spec", {}) + merged["spec"]["modules"] = _merge_modules( + base_modules, overlay_modules, base_source, overlay_source, provenance + ) + return merged, diagnostics + + +def _derive_profiles_root(profile_dir: Path) -> Path: + parts = profile_dir.parts + for index, part in enumerate(parts): + if part == "profiles": + return Path(*parts[: index + 1]) + return profile_dir.parent + + +def _resolve_extends_ref(ref: str, profile_dir: Path, profiles_root: Path) -> Path: + ref_path = Path(ref) + if ref_path.suffix in (".yaml", ".yml") or ".." in ref_path.parts or ref_path.is_absolute(): + return (profile_dir / ref_path).resolve() + if len(ref_path.parts) > 1: + return (profile_dir / ref_path / "profile.yaml").resolve() + return (profiles_root / ref / "profile.yaml").resolve() + + +def _compose_extends( + profile_file: Path, + stack: tuple[Path, ...], +) -> tuple[dict[str, Any] | None, dict[str, str], list[Diagnostic]]: + """ + Recursively resolves `extends` on the profile at profile_file, merging + one or more parent profiles left-to-right (later parents win) and then + the child's own document on top of all parents. Reuses + _merge_profile_docs so this is the same merge/provenance model as + environment overlays, not a separate engine, per issue #175. + """ + resolved_file = profile_file.resolve() + if resolved_file in stack: + chain = " -> ".join(str(p) for p in (*stack, resolved_file)) + return None, {}, [ + Diagnostic( + level="error", + code="E106", + message=f"Cycle detected in profile extends chain: {chain}.", + path="extends", + ) + ] + + doc, diagnostics = load_yaml_file(profile_file) + if doc is None: + return None, {}, diagnostics + + extends = doc.get("extends") + if extends is None: + return doc, {}, diagnostics + + if not isinstance(extends, list) or not extends or not all( + isinstance(ref, str) and ref.strip() for ref in extends + ): + diagnostics.append( + Diagnostic( + level="error", + code="E103", + message="'extends' must be a non-empty list of non-empty profile references.", + path="extends", + ) + ) + return None, {}, diagnostics + + profile_dir = profile_file.parent.resolve() + profiles_root = _derive_profiles_root(profile_dir) + new_stack = (*stack, resolved_file) + + merged: dict[str, Any] | None = None + merged_source: str | None = None + provenance: dict[str, str] = {} + + for ref in extends: + parent_path = _resolve_extends_ref(ref, profile_dir, profiles_root) + + if not _is_within(parent_path, profiles_root): + diagnostics.append( + Diagnostic( + level="error", + code="E104", + message=f'Parent profile "{ref}" resolves outside the profiles root "{profiles_root}".', + path="extends", + ) + ) + return None, {}, diagnostics + + if not parent_path.is_file(): + diagnostics.append( + Diagnostic( + level="error", + code="E105", + message=f'Parent profile "{ref}" not found (looked for {parent_path}).', + path="extends", + ) + ) + return None, {}, diagnostics + + parent_doc, parent_provenance, parent_diagnostics = _compose_extends(parent_path, new_stack) + diagnostics += parent_diagnostics + if parent_doc is None: + return None, {}, diagnostics + + if merged is None: + merged = parent_doc + merged_source = str(parent_path) + provenance = parent_provenance + else: + merged, merge_diagnostics = _merge_profile_docs( + merged, parent_doc, merged_source, str(parent_path), provenance + ) + diagnostics += merge_diagnostics + if merge_diagnostics: + return None, {}, diagnostics + merged_source = str(parent_path) + + child_without_extends = {k: v for k, v in doc.items() if k != "extends"} + merged, merge_diagnostics = _merge_profile_docs( + merged, child_without_extends, merged_source, str(profile_file), provenance + ) + diagnostics += merge_diagnostics + if merge_diagnostics: + return None, {}, diagnostics + + return merged, provenance, diagnostics + + def _merge_modules( base_modules: list[dict[str, Any]], overlay_modules: list[dict[str, Any]], @@ -83,6 +312,20 @@ def _merge_modules( return [by_id[mid] for mid in order] +def resolve_extends( + profile_path: str, +) -> tuple[dict[str, Any] | None, dict[str, str], list[Diagnostic]]: + """ + Resolves only a profile's `extends` chain (no --environment overlay, no + full validate_loaded_profile pass). Used by build_plan()/validate_profile() + in place of a bare load_yaml_file() so `extends` composition applies even + when no --environment is selected, while preserving their own + lighter-weight/defensive diagnostic handling instead of resolve_profile()'s + full validation, which would mask those diagnostics. + """ + return _compose_extends(Path(profile_path), ()) + + def resolve_profile( profile_path: str, environment: str | None = None, @@ -101,13 +344,12 @@ def resolve_profile( standalone profiles are unaffected by this resolver existing. """ profile_file = Path(profile_path) - base, diagnostics = load_yaml_file(profile_file) + base, provenance, diagnostics = _compose_extends(profile_file, ()) if base is None: return None, {}, diagnostics if environment is None: diagnostics += validate_loaded_profile(base, profile_file) - provenance = {} return (base, provenance, diagnostics) if not any( d.level == "error" for d in diagnostics ) else (None, provenance, diagnostics) @@ -146,92 +388,11 @@ def resolve_profile( base_source = str(profile_file) overlay_source = str(overlay_file) - base_spec = base.get("spec", {}) - base_modules = base_spec.get("modules", []) if isinstance(base_spec, dict) else [] - overlay_spec = overlay.get("spec", {}) - overlay_modules = overlay_spec.get("modules", []) if isinstance(overlay_spec, dict) else [] - - for label, modules in (("base profile", base_modules), (f"overlay {overlay_source}", overlay_modules)): - if not isinstance(modules, list): - diagnostics.append( - Diagnostic( - level="error", - code="E093", - message=f"spec.modules in {label} must be a list, got {type(modules).__name__}.", - path="spec.modules", - ) - ) - continue - - non_dict_indices = [i for i, m in enumerate(modules) if not isinstance(m, dict)] - if non_dict_indices: - diagnostics.append( - Diagnostic( - level="error", - code="E093", - message=( - f"Module entr{'y' if len(non_dict_indices) == 1 else 'ies'} in {label} " - f"must be a mapping, not a scalar/list, at index {non_dict_indices}." - ), - path="spec.modules", - ) - ) - - if any(d.level == "error" for d in diagnostics): - return None, {}, diagnostics - - for label, modules in (("base profile", base_modules), (f"overlay {overlay_source}", overlay_modules)): - missing_id = [i for i, m in enumerate(modules) if not m.get("id")] - if missing_id: - diagnostics.append( - Diagnostic( - level="error", - code="E093", - message=f"Module entr{'y' if len(missing_id) == 1 else 'ies'} in {label} missing required 'id' at index {missing_id}.", - path="spec.modules", - ) - ) - if any(d.level == "error" for d in diagnostics): - return None, {}, diagnostics - - for label, modules in (("base profile", base_modules), (f"overlay {overlay_source}", overlay_modules)): - dupes = _duplicate_module_ids(modules) - if dupes: - diagnostics.append( - Diagnostic( - level="error", - code="E093", - message=f"Duplicate module id(s) in {label}: {sorted(dupes)}.", - path="spec.modules", - ) - ) - if any(d.level == "error" for d in diagnostics): + merged, merge_diagnostics = _merge_profile_docs(base, overlay, base_source, overlay_source, provenance) + diagnostics += merge_diagnostics + if merge_diagnostics: return None, {}, diagnostics - provenance: dict[str, str] = {} - - base_without_modules = dict(base) - base_spec_val = base.get("spec", {}) - base_spec_without_modules = ( - {k: v for k, v in base_spec_val.items() if k != "modules"} - if isinstance(base_spec_val, dict) - else {} - ) - base_without_modules["spec"] = base_spec_without_modules - - overlay_without_modules = dict(overlay) - if "spec" in overlay and isinstance(overlay.get("spec"), dict): - overlay_spec_without_modules = {k: v for k, v in overlay["spec"].items() if k != "modules"} - overlay_without_modules["spec"] = overlay_spec_without_modules - - merged = _merge_value( - base_without_modules, overlay_without_modules, base_source, overlay_source, "", provenance - ) - merged.setdefault("spec", {}) - merged["spec"]["modules"] = _merge_modules( - base_modules, overlay_modules, base_source, overlay_source, provenance - ) - diagnostics += validate_loaded_profile(merged, profile_file) if any(d.level == "error" for d in diagnostics): return None, provenance, diagnostics diff --git a/cli/planner.py b/cli/planner.py index 8db704ac..896b23e9 100644 --- a/cli/planner.py +++ b/cli/planner.py @@ -57,16 +57,20 @@ def build_plan( diagnostics: list[Diagnostic] = [] profile_file = Path(profile_path) - provenance: dict[str, str] = {} - if environment is not None: - # Local import: cli.overlay imports from cli.validator, which this - # module does not otherwise depend on; keep the dependency scoped to - # avoid pulling in an import cycle for callers that never use overlays. - from .overlay import resolve_profile + # Local import: cli.overlay imports from cli.validator, which this + # module does not otherwise depend on; keep the dependency scoped to + # avoid pulling in an import cycle for callers that never use overlays. + from .overlay import resolve_extends, resolve_profile + if environment is not None: profile, provenance, diags = resolve_profile(profile_path, environment) else: - profile, diags = load_yaml_file(profile_file) + # resolve_extends() (not a bare load_yaml_file()) so a profile's own + # `extends` chain still applies with no --environment selected, but + # without resolve_profile()'s full validate_loaded_profile() pass, + # which would replace build_plan()'s own defensive diagnostics for + # malformed profiles with a single upfront validation failure. + profile, provenance, diags = resolve_extends(profile_path) diagnostics.extend(diags) if profile is None: diff --git a/cli/resources/profile.schema.json b/cli/resources/profile.schema.json index 5080815d..b8e81a07 100644 --- a/cli/resources/profile.schema.json +++ b/cli/resources/profile.schema.json @@ -14,6 +14,15 @@ "type": "string", "const": "Profile" }, + "extends": { + "type": "array", + "minItems": 1, + "items": { + "type": "string", + "minLength": 1 + }, + "description": "One or more parent profile references (profile name or relative path) this profile composes from, left-to-right with later parents winning, then this profile winning over all parents. Resolved before validation; the resolved profile never contains this field." + }, "metadata": { "type": "object", "additionalProperties": true, diff --git a/cli/validator.py b/cli/validator.py index d70f166e..39def627 100644 --- a/cli/validator.py +++ b/cli/validator.py @@ -25,20 +25,15 @@ def _load_schema(name: str) -> dict[str, Any]: def validate_profile(profile_path: str, environment: str | None = None) -> list[Diagnostic]: - if environment is not None: - # Local import: cli.overlay imports validate_loaded_profile from this - # module, so importing it back at module scope would be circular. - from .overlay import resolve_profile - - _, _, diagnostics = resolve_profile(profile_path, environment) - return diagnostics - - profile_file = Path(profile_path) - profile, diagnostics = load_yaml_file(profile_file) - if profile is None: - return diagnostics - - return diagnostics + validate_loaded_profile(profile, profile_file) + # Local import: cli.overlay imports validate_loaded_profile from this + # module, so importing it back at module scope would be circular. + # resolve_profile() is called unconditionally (not just when environment + # is set) because it also resolves a profile's own `extends` chain, which + # must apply even when no --environment overlay is selected. + from .overlay import resolve_profile + + _, _, diagnostics = resolve_profile(profile_path, environment) + return diagnostics def validate_loaded_profile(profile: dict[str, Any], profile_file: Path) -> list[Diagnostic]: diff --git a/tests/test_overlay.py b/tests/test_overlay.py index 6064f90d..d511db18 100644 --- a/tests/test_overlay.py +++ b/tests/test_overlay.py @@ -276,5 +276,305 @@ def test_overlay_module_missing_source_gets_the_normal_diagnostic(self): self.assertTrue(any("source" in d.message.lower() for d in diagnostics), diagnostics) +class ExtendsCompositionTest(unittest.TestCase): + """resolve_profile() honoring `extends` (issue #175).""" + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.addCleanup(self._tmp.cleanup) + self.root = Path(self._tmp.name) + self.profiles_dir = self.root / "profiles" + self.profiles_dir.mkdir(parents=True) + self.modules_dir = self.root / "modules" / "warehouse" / "postgres" + self.modules_dir.mkdir(parents=True) + + (self.modules_dir / "module.yaml").write_text( + yaml.safe_dump( + { + "apiVersion": "cds/v1alpha1", + "kind": "Module", + "metadata": {"name": "postgres", "category": "warehouse", "version": "0.1.0"}, + "spec": { + "runtime": { + "type": "container", + "service": { + "name": "postgres", + "ports": [{"name": "db", "containerPort": 5432, "protocol": "TCP"}], + }, + }, + "configSchema": {"type": "object", "additionalProperties": True}, + "implementation": {"kind": "docker-compose", "compose": {"services": {}}}, + }, + } + ) + ) + + def _write_profile(self, name, content): + profile_dir = self.profiles_dir / name + profile_dir.mkdir(parents=True, exist_ok=True) + (profile_dir / "profile.yaml").write_text(yaml.safe_dump(content)) + return profile_dir / "profile.yaml" + + def _base_module(self, replicas=1): + return { + "id": "db", + "source": "../../modules/warehouse/postgres", + "version": "0.1.0", + "enabled": True, + "config": {"replicas": replicas}, + } + + def test_single_parent_child_wins_over_parent(self): + self._write_profile( + "base", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "base", "environment": "local"}, + "spec": {"runtime": {"type": "docker-compose"}, "modules": [self._base_module(1)]}, + }, + ) + child = self._write_profile( + "child", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "child", "environment": "local"}, + "extends": ["base"], + "spec": {"modules": [{"id": "db", "config": {"replicas": 5}}]}, + }, + ) + resolved, provenance, diagnostics = resolve_profile(str(child), environment=None) + self.assertFalse(any(d.level == "error" for d in diagnostics), diagnostics) + self.assertEqual(resolved["spec"]["modules"][0]["config"]["replicas"], 5) + self.assertEqual(provenance["spec.modules[db]"], str(child)) + + def test_multiple_parents_resolve_left_to_right_later_wins(self): + self._write_profile( + "p1", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "p1", "environment": "local"}, + "spec": {"runtime": {"type": "docker-compose"}, "modules": [self._base_module(1)]}, + }, + ) + self._write_profile( + "p2", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "p2", "environment": "local"}, + "spec": {"modules": [{"id": "db", "config": {"replicas": 2}}]}, + }, + ) + child = self._write_profile( + "child", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "child", "environment": "local"}, + "extends": ["p1", "p2"], + "spec": {}, + }, + ) + resolved, _prov, diagnostics = resolve_profile(str(child), environment=None) + self.assertFalse(any(d.level == "error" for d in diagnostics), diagnostics) + self.assertEqual(resolved["spec"]["modules"][0]["config"]["replicas"], 2) + + def test_relative_path_parent_reference_is_supported(self): + self._write_profile( + "base", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "base", "environment": "local"}, + "spec": {"runtime": {"type": "docker-compose"}, "modules": [self._base_module(1)]}, + }, + ) + child = self._write_profile( + "child", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "child", "environment": "local"}, + "extends": ["../base/profile.yaml"], + "spec": {}, + }, + ) + resolved, _prov, diagnostics = resolve_profile(str(child), environment=None) + self.assertFalse(any(d.level == "error" for d in diagnostics), diagnostics) + self.assertEqual(resolved["spec"]["modules"][0]["id"], "db") + + def test_transitive_extends_chain_is_supported(self): + self._write_profile( + "grandparent", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "grandparent", "environment": "local"}, + "spec": {"runtime": {"type": "docker-compose"}, "modules": [self._base_module(1)]}, + }, + ) + self._write_profile( + "parent", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "parent", "environment": "local"}, + "extends": ["grandparent"], + "spec": {"modules": [{"id": "db", "config": {"replicas": 7}}]}, + }, + ) + child = self._write_profile( + "child", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "child", "environment": "local"}, + "extends": ["parent"], + "spec": {}, + }, + ) + resolved, _prov, diagnostics = resolve_profile(str(child), environment=None) + self.assertFalse(any(d.level == "error" for d in diagnostics), diagnostics) + self.assertEqual(resolved["spec"]["modules"][0]["config"]["replicas"], 7) + + def test_direct_cycle_is_rejected(self): + a = self.profiles_dir / "a" + a.mkdir(parents=True) + b = self.profiles_dir / "b" + b.mkdir(parents=True) + (a / "profile.yaml").write_text( + yaml.safe_dump( + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "a", "environment": "local"}, + "extends": ["b"], + "spec": {"runtime": {"type": "docker-compose"}, "modules": []}, + } + ) + ) + (b / "profile.yaml").write_text( + yaml.safe_dump( + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "b", "environment": "local"}, + "extends": ["a"], + "spec": {"runtime": {"type": "docker-compose"}, "modules": []}, + } + ) + ) + resolved, _prov, diagnostics = resolve_profile(str(a / "profile.yaml"), environment=None) + self.assertIsNone(resolved) + self.assertTrue(any(d.code == "E106" for d in diagnostics), diagnostics) + + def test_missing_parent_profile_is_rejected(self): + child = self._write_profile( + "child", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "child", "environment": "local"}, + "extends": ["does-not-exist"], + "spec": {"runtime": {"type": "docker-compose"}, "modules": []}, + }, + ) + resolved, _prov, diagnostics = resolve_profile(str(child), environment=None) + self.assertIsNone(resolved) + self.assertTrue(any(d.code == "E105" for d in diagnostics), diagnostics) + + def test_extends_outside_profiles_root_is_rejected(self): + outside_dir = self.root / "outside" + outside_dir.mkdir() + (outside_dir / "profile.yaml").write_text( + yaml.safe_dump( + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "outside", "environment": "local"}, + "spec": {"runtime": {"type": "docker-compose"}, "modules": []}, + } + ) + ) + child = self._write_profile( + "child", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "child", "environment": "local"}, + "extends": ["../../outside/profile.yaml"], + "spec": {"runtime": {"type": "docker-compose"}, "modules": []}, + }, + ) + resolved, _prov, diagnostics = resolve_profile(str(child), environment=None) + self.assertIsNone(resolved) + self.assertTrue(any(d.code == "E104" for d in diagnostics), diagnostics) + + def test_extends_not_a_list_is_rejected(self): + child = self._write_profile( + "child", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "child", "environment": "local"}, + "extends": "base", + "spec": {"runtime": {"type": "docker-compose"}, "modules": []}, + }, + ) + resolved, _prov, diagnostics = resolve_profile(str(child), environment=None) + self.assertIsNone(resolved) + self.assertTrue(any(d.code == "E103" for d in diagnostics), diagnostics) + + def test_extends_combined_with_environment_overlay_applies_on_top(self): + self._write_profile( + "base", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "base", "environment": "local"}, + "spec": {"runtime": {"type": "docker-compose"}, "modules": [self._base_module(1)]}, + }, + ) + child_dir = self.profiles_dir / "child" + child_dir.mkdir(parents=True, exist_ok=True) + (child_dir / "profile.yaml").write_text( + yaml.safe_dump( + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "child", "environment": "local"}, + "extends": ["base"], + "spec": {}, + } + ) + ) + env_dir = child_dir / "environments" + env_dir.mkdir() + (env_dir / "prod.yaml").write_text( + yaml.safe_dump({"spec": {"modules": [{"id": "db", "config": {"replicas": 9}}]}}) + ) + resolved, _prov, diagnostics = resolve_profile(str(child_dir / "profile.yaml"), environment="prod") + self.assertFalse(any(d.level == "error" for d in diagnostics), diagnostics) + self.assertEqual(resolved["spec"]["modules"][0]["config"]["replicas"], 9) + + def test_no_extends_field_behaves_exactly_as_before(self): + child = self._write_profile( + "child", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "child", "environment": "local"}, + "spec": {"runtime": {"type": "docker-compose"}, "modules": [self._base_module(1)]}, + }, + ) + resolved, provenance, diagnostics = resolve_profile(str(child), environment=None) + self.assertFalse(any(d.level == "error" for d in diagnostics), diagnostics) + self.assertEqual(provenance, {}) + self.assertEqual(resolved["spec"]["modules"][0]["config"]["replicas"], 1) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_planner.py b/tests/test_planner.py index a77ced2d..14bb8e4a 100644 --- a/tests/test_planner.py +++ b/tests/test_planner.py @@ -833,6 +833,61 @@ def test_build_plan_converts_deep_config_nesting_into_diagnostic(self): errors = [d for d in diagnostics if d.code == "E094"] self.assertEqual(len(errors), 1) + def test_build_plan_honors_extends_without_environment_flag(self): + """ + A profile's `extends` chain must be resolved even when build_plan() + is called with no --environment, since `extends` is a property of + the profile file itself, not something gated behind that flag. + """ + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + module_dir = root / "modules" / "deep" + module_dir.mkdir(parents=True) + (module_dir / "module.yaml").write_text( + "apiVersion: cds/v1alpha1\n" + "kind: Module\n" + "metadata:\n" + " name: deep\n" + "spec:\n" + " configSchema: {type: object, additionalProperties: true}\n" + " implementation: {kind: docker-compose, compose: {services: {}}}\n" + ) + + base_dir = root / "profiles" / "base" + base_dir.mkdir(parents=True) + (base_dir / "profile.yaml").write_text( + "apiVersion: cds/v1alpha1\n" + "kind: Profile\n" + "metadata: {name: base, environment: local}\n" + "spec:\n" + " runtime: {type: docker-compose}\n" + " modules:\n" + " - id: svc\n" + " source: ../../modules/deep\n" + " config: {replicas: 1}\n" + ) + + child_dir = root / "profiles" / "child" + child_dir.mkdir(parents=True) + profile_path = child_dir / "profile.yaml" + profile_path.write_text( + "apiVersion: cds/v1alpha1\n" + "kind: Profile\n" + "metadata: {name: child, environment: local}\n" + "extends: [base]\n" + "spec:\n" + " modules:\n" + " - id: svc\n" + " config: {replicas: 5}\n" + ) + + plan, diagnostics = planner.build_plan(str(profile_path)) + + self.assertFalse(any(d.level == "error" for d in diagnostics), diagnostics) + self.assertIsNotNone(plan) + module = next(m for m in plan["modules"] if m["id"] == "svc") + self.assertEqual(module["config"]["replicas"], 5) + def test_substitute_values_raises_max_nesting_depth_exceeded_on_deep_dict(self): obj: dict = {} node = obj From af3d9f6abeb712b9cd3f6e2bff5a2834c54510c1 Mon Sep 17 00:00:00 2001 From: RonaldHensbergen Date: Tue, 25 Aug 2026 22:47:16 +0200 Subject: [PATCH 2/5] Expand extends documentation: no-new-flags note, multi-parent example, error codes Clarifies that extends is read from profile.yaml (not a CLI flag), adds a multi-parent example, and documents the E103-E106 diagnostic codes introduced for extends composition. --- README.md | 41 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index ee16268e..e769bcb2 100644 --- a/README.md +++ b/README.md @@ -633,11 +633,42 @@ Composition uses the exact same deep-merge/module-merge-by-id engine as environment overlays: mappings merge recursively, `spec.modules` entries merge by stable `id`, and any other array is replaced wholesale rather than concatenated. Parent profiles may themselves use `extends` (chains are -resolved transitively); cycles, missing parents, and parents that resolve -outside the profiles root are rejected with a diagnostic before anything -else runs. `extends` and `--environment` compose together: parents are -merged first, then the child, then the selected environment overlay is -applied on top of that fully-composed result. +resolved transitively). + +`extends` is not a CLI flag — it's read directly from `profile.yaml`, so +every profile-consuming command resolves it automatically, with no new +syntax to learn: + +```bash +cds validate analytics-prod +cds plan analytics-prod +cds up analytics-prod +``` + +A profile can extend more than one parent, which is merged in the order +listed (later entries win over earlier ones): + +```yaml +extends: + - networking-base + - observability-base +``` + +`extends` and `--environment` compose together: parents are merged first, +then the child, then the selected environment overlay is applied on top of +that fully-composed result — so a shared base profile and environment +promotion can both be used without duplicating configuration in either +dimension. + +A malformed `extends` chain fails validation/planning before anything else +runs, with a dedicated diagnostic code: + +|Code|Meaning| +|---|---| +|E103|`extends` is missing, not a list, empty, or contains a non-string/empty entry| +|E104|A parent reference resolves outside the profiles root| +|E105|A referenced parent profile does not exist| +|E106|A cycle was detected in the `extends` chain| --- From 732e9be6383df9bfabdf4286fb21e2f76484549a Mon Sep 17 00:00:00 2001 From: RonaldHensbergen Date: Wed, 26 Aug 2026 07:40:44 +0200 Subject: [PATCH 3/5] Fail closed when extends' profiles root cannot be derived Code review of PR #525 found that _derive_profiles_root() silently fell back to the profile's parent directory when no "profiles/" segment was present in its path, quietly widening the extends security boundary (E104) to an ad hoc root instead of rejecting resolution outright. This mirrors the existing fail-closed pattern already used by _derive_allowed_module_root() in cli/loader.py, which returns None (causing outright rejection) rather than substituting a narrower/wrong root. _derive_profiles_root() now returns None instead of profile_dir.parent when no "profiles" path segment is found, and _compose_extends() raises E104 in that case rather than resolving extends against a guessed root. Added a regression test. --- cli/overlay.py | 18 ++++++++++++++++-- tests/test_overlay.py | 23 +++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/cli/overlay.py b/cli/overlay.py index 3b60ebed..a252fd24 100644 --- a/cli/overlay.py +++ b/cli/overlay.py @@ -159,12 +159,12 @@ def _merge_profile_docs( return merged, diagnostics -def _derive_profiles_root(profile_dir: Path) -> Path: +def _derive_profiles_root(profile_dir: Path) -> Path | None: parts = profile_dir.parts for index, part in enumerate(parts): if part == "profiles": return Path(*parts[: index + 1]) - return profile_dir.parent + return None def _resolve_extends_ref(ref: str, profile_dir: Path, profiles_root: Path) -> Path: @@ -222,6 +222,20 @@ def _compose_extends( profile_dir = profile_file.parent.resolve() profiles_root = _derive_profiles_root(profile_dir) + if profiles_root is None: + diagnostics.append( + Diagnostic( + level="error", + code="E104", + message=( + f'Cannot resolve "extends" for {profile_file}: its directory does not ' + 'reside under a "profiles/" root.' + ), + path="extends", + ) + ) + return None, {}, diagnostics + new_stack = (*stack, resolved_file) merged: dict[str, Any] | None = None diff --git a/tests/test_overlay.py b/tests/test_overlay.py index d511db18..32f31faa 100644 --- a/tests/test_overlay.py +++ b/tests/test_overlay.py @@ -560,6 +560,29 @@ def test_extends_combined_with_environment_overlay_applies_on_top(self): self.assertFalse(any(d.level == "error" for d in diagnostics), diagnostics) self.assertEqual(resolved["spec"]["modules"][0]["config"]["replicas"], 9) + def test_extends_rejected_when_profile_not_under_a_profiles_root(self): + # Regression test: _derive_profiles_root() must fail closed (reject + # with a diagnostic) rather than silently widen the security + # boundary to the profile's parent directory when the profile isn't + # conventionally located under a "profiles/" directory. + outside_root = self.root / "not-profiles" / "child" + outside_root.mkdir(parents=True) + child = outside_root / "profile.yaml" + child.write_text( + yaml.safe_dump( + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "child", "environment": "local"}, + "extends": ["../sibling/profile.yaml"], + "spec": {"runtime": {"type": "docker-compose"}, "modules": []}, + } + ) + ) + resolved, _prov, diagnostics = resolve_profile(str(child), environment=None) + self.assertIsNone(resolved) + self.assertTrue(any(d.code == "E104" for d in diagnostics), diagnostics) + def test_no_extends_field_behaves_exactly_as_before(self): child = self._write_profile( "child", From 504a91b261af9fde5a55c5dffc1bc20f7e851e07 Mon Sep 17 00:00:00 2001 From: RonaldHensbergen Date: Wed, 26 Aug 2026 07:56:56 +0200 Subject: [PATCH 4/5] Fix extends bypass in cds init/get and profiles-root ambiguity Third review round found: - cli/main.py's _collect_profile_env_vars() (cds init) called load_yaml_file() directly when environment is None, dropping secrets/env vars declared only in parent profiles. - cli/getter.py's _collect_asset_roots() (cds get) likewise bypassed extends, silently omitting modules contributed only by a parent profile from asset fetching. - _derive_profiles_root() matched the first (outermost) "profiles" path segment instead of the last (innermost) one, letting an extends ref potentially escape the intended profiles tree. Both cds init and cds get now resolve through cli.overlay's resolve_extends()/resolve_profile(), and _derive_profiles_root() anchors on the innermost "profiles" segment. Added regression tests for all three fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/getter.py | 46 +++++++++++++++++++++++++++++++++++-- cli/main.py | 11 +++++---- cli/overlay.py | 10 ++++++-- tests/test_getter.py | 51 +++++++++++++++++++++++++++++++++++++++++ tests/test_main.py | 37 ++++++++++++++++++++++++++++++ tests/test_overlay.py | 53 +++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 199 insertions(+), 9 deletions(-) diff --git a/cli/getter.py b/cli/getter.py index e2f40f89..52509732 100644 --- a/cli/getter.py +++ b/cli/getter.py @@ -18,6 +18,7 @@ from urllib.request import Request, urlopen from .loader import load_yaml_file, resolve_module_dir +from .overlay import _derive_profiles_root, _resolve_extends_ref, resolve_extends from .planner import MaxNestingDepthExceeded, apply_defaults, substitute_string # The upstream repository `cds get` downloads from when no `--remote` is @@ -233,15 +234,55 @@ def _resolve_source_profile_path(source_repo: Path, profile: str) -> Path: ) +def _collect_extends_profile_dirs(source_repo: Path, profile_path: Path) -> set[Path]: + """ + Returns every parent profile directory reachable via `extends`, so + `cds get` copies parent profile.yaml files (and, via + _collect_asset_roots's module walk, their modules) too instead of + silently omitting anything only declared in a parent profile. Reuses + cli.overlay's own extends-ref resolution rather than re-implementing it. + """ + doc, _diagnostics = load_yaml_file(profile_path) + if doc is None: + raise GetError(f"Could not load source profile {profile_path}") + + extends = doc.get("extends") + if not extends: + return set() + + profile_dir = profile_path.parent.resolve() + profiles_root = _derive_profiles_root(profile_dir) + if profiles_root is None: + raise GetError(f'Could not derive a profiles root for extends in {profile_path}') + + dirs: set[Path] = set() + for ref in extends: + parent_path = _resolve_extends_ref(ref, profile_dir, profiles_root) + if not parent_path.is_file(): + raise GetError(f'Could not resolve extends parent "{ref}" for {profile_path}') + _require_within_repo(parent_path, source_repo, f'extends parent "{ref}"') + dirs.add(parent_path.parent) + dirs.update(_collect_extends_profile_dirs(source_repo, parent_path)) + + return dirs + + def _collect_asset_roots(source_repo: Path, profile_path: Path) -> list[Path]: profile_dir = profile_path.parent asset_roots: set[Path] = { profile_dir if profile_path.name == "profile.yaml" else profile_path } - profile_doc, profile_diags = load_yaml_file(profile_path) - if profile_diags or profile_doc is None: + + # Resolve through `extends` so modules/config contributed only by a + # parent profile (not redeclared in the child) are still collected; + # cli.overlay is the single source of truth for extends semantics (see + # cli.planner/cli.validator, which route through it the same way). + profile_doc, _provenance, diagnostics = resolve_extends(str(profile_path)) + if profile_doc is None or any(d.level == "error" for d in diagnostics): raise GetError(f"Could not load source profile {profile_path}") + asset_roots.update(_collect_extends_profile_dirs(source_repo, profile_path)) + spec = profile_doc.get("spec") modules = spec.get("modules", []) if isinstance(spec, dict) else [] if not isinstance(modules, list): @@ -266,6 +307,7 @@ def _collect_asset_roots(source_repo: Path, profile_path: Path) -> list[Path]: return sorted(asset_roots) + def _collect_module_runtime_assets( source_repo: Path, module_dir: Path, diff --git a/cli/main.py b/cli/main.py index 6d333ab4..95fdb1dd 100644 --- a/cli/main.py +++ b/cli/main.py @@ -28,8 +28,7 @@ load_policy_from_env, verify_images, ) -from .loader import load_yaml_file -from .overlay import resolve_profile +from .overlay import resolve_extends, resolve_profile from .planner import build_plan from .preflight import preflight_passed, run_preflight from .renderer import render_compose @@ -469,12 +468,14 @@ def _collect_profile_env_vars( identifiers like database/user names, so callers can fill in friendlier defaults for them instead of a placeholder. """ + # Always resolve via cli.overlay so a profile's `extends` chain (and, if + # selected, --environment overlay) is applied; environment=None still + # resolves extends via resolve_extends() (lighter than resolve_profile(), + # which also runs full validate_loaded_profile()). if environment is not None: - from .overlay import resolve_profile - profile, _, diags = resolve_profile(profile_path, environment) else: - profile, diags = load_yaml_file(Path(profile_path)) + profile, _, diags = resolve_extends(profile_path) if profile is None: error_messages = [d.format() for d in diags if d.level == "error"] raise ValueError("Could not load profile: " + "; ".join(error_messages or ["unknown error"])) diff --git a/cli/overlay.py b/cli/overlay.py index a252fd24..416b8cfe 100644 --- a/cli/overlay.py +++ b/cli/overlay.py @@ -160,9 +160,15 @@ def _merge_profile_docs( def _derive_profiles_root(profile_dir: Path) -> Path | None: + # Use the innermost (last, i.e. closest to profile_dir) "profiles" + # segment, not the first/outermost one. A path can contain more than one + # segment literally named "profiles" (e.g. a checkout at + # ".../profiles//profiles/prod"); picking the outermost match would + # derive an overly broad root and let `extends` escape the profile's own + # repo-local profiles/ tree into an unrelated sibling project. parts = profile_dir.parts - for index, part in enumerate(parts): - if part == "profiles": + for index in range(len(parts) - 1, -1, -1): + if parts[index] == "profiles": return Path(*parts[: index + 1]) return None diff --git a/tests/test_getter.py b/tests/test_getter.py index 66bcfbac..58b08d18 100644 --- a/tests/test_getter.py +++ b/tests/test_getter.py @@ -99,6 +99,57 @@ def test_fetch_profile_copies_profile_module_and_build_assets(self) -> None: self.assertIn("modules/apps/demo", entry["assetRoots"]) self.assertIn("images/demo/Dockerfile", entry["assetRoots"]) + def test_fetch_profile_copies_extends_parent_and_its_module(self) -> None: + # Regression test: _collect_asset_roots() must resolve `extends` so + # `cds get` fetches a parent profile's own modules even when the + # child profile doesn't redeclare them. + with tempfile.TemporaryDirectory() as source_dir, tempfile.TemporaryDirectory() as dest_dir: + source_root = Path(source_dir) + destination_root = Path(dest_dir) + _make_source_repo(source_root) + _write( + source_root / "profiles" / "base" / "profile.yaml", + """apiVersion: cds/v1alpha1 +kind: Profile +metadata: + name: base +spec: + runtime: + type: docker-compose + modules: + - id: demo + source: ../../modules/apps/demo + config: {} +""", + ) + _write( + source_root / "profiles" / "child" / "profile.yaml", + """apiVersion: cds/v1alpha1 +kind: Profile +metadata: + name: child +extends: + - base +spec: {} +""", + ) + + actions, manifest_path = fetch_profile( + "child", + local=str(source_root), + destination_root=destination_root, + ) + + self.assertGreater(len(actions), 0) + self.assertTrue((destination_root / "profiles" / "child" / "profile.yaml").exists()) + self.assertTrue((destination_root / "profiles" / "base" / "profile.yaml").exists()) + self.assertTrue((destination_root / "modules" / "apps" / "demo" / "module.yaml").exists()) + + manifest = json.loads(manifest_path.read_text(encoding="utf-8")) + entry = manifest["profiles"]["child"] + self.assertIn("modules/apps/demo", entry["assetRoots"]) + self.assertIn("profiles/base", entry["assetRoots"]) + def test_fetch_profile_requires_force_for_conflicting_files(self) -> None: with tempfile.TemporaryDirectory() as source_dir, tempfile.TemporaryDirectory() as dest_dir: source_root = Path(source_dir) diff --git a/tests/test_main.py b/tests/test_main.py index 961283a5..4fa9451e 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -12,6 +12,7 @@ from cli.diagnostics import Diagnostic from cli.image_updates import collect_module_images from cli.main import ( + _collect_profile_env_vars, _resolve_profile_root, list_modules, list_profiles, @@ -480,6 +481,42 @@ def test_init_generates_env_file_from_profile_secrets(self): finally: output_file.unlink(missing_ok=True) + def test_collect_profile_env_vars_honors_extends_without_environment_flag(self): + # Regression test: _collect_profile_env_vars() previously called + # load_yaml_file() directly when environment=None, bypassing + # extends resolution and silently dropping secrets declared only in + # a parent profile from the generated .env file. + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + base_dir = root / "profiles" / "base" + base_dir.mkdir(parents=True) + (base_dir / "profile.yaml").write_text( + "apiVersion: cds/v1alpha1\n" + "kind: Profile\n" + "metadata: {name: base, environment: local}\n" + "spec:\n" + " runtime: {type: docker-compose}\n" + " modules: []\n" + " secrets:\n" + " values:\n" + " dbPassword: {env: CDS_DB_PASSWORD}\n" + ) + child_dir = root / "profiles" / "child" + child_dir.mkdir(parents=True) + child_path = child_dir / "profile.yaml" + child_path.write_text( + "apiVersion: cds/v1alpha1\n" + "kind: Profile\n" + "metadata: {name: child, environment: local}\n" + "extends: [base]\n" + "spec: {}\n" + ) + + env_vars, secret_env_vars = _collect_profile_env_vars(str(child_path), environment=None) + + self.assertIn("CDS_DB_PASSWORD", env_vars) + self.assertIn("CDS_DB_PASSWORD", secret_env_vars) + def test_get_command_dry_run_reports_planned_files_without_writing(self): import tempfile diff --git a/tests/test_overlay.py b/tests/test_overlay.py index 32f31faa..48e8640c 100644 --- a/tests/test_overlay.py +++ b/tests/test_overlay.py @@ -583,6 +583,59 @@ def test_extends_rejected_when_profile_not_under_a_profiles_root(self): self.assertIsNone(resolved) self.assertTrue(any(d.code == "E104" for d in diagnostics), diagnostics) + def test_extends_root_derivation_uses_innermost_profiles_segment(self): + # Regression test: _derive_profiles_root() must anchor on the + # innermost "profiles" path segment (closest to the profile), not + # the outermost one, or an extends ref could escape a nested repo + # checkout (e.g. ".../profiles//profiles/prod") into an + # unrelated sibling project that also happens to sit under an outer + # directory literally named "profiles". + outer_profiles = self.root / "profiles" + nested_repo_dir = outer_profiles / "myrepo" / "profiles" + nested_repo_dir.mkdir(parents=True) + (nested_repo_dir / "base").mkdir() + (nested_repo_dir / "base" / "profile.yaml").write_text( + yaml.safe_dump( + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "base", "environment": "local"}, + "spec": {"runtime": {"type": "docker-compose"}, "modules": []}, + } + ) + ) + # A sibling project several directories up, still nominally "under" + # the outer "profiles" directory, but outside the nested repo's own + # profiles/ tree. + (outer_profiles / "other-project" / "secret").mkdir(parents=True) + (outer_profiles / "other-project" / "secret" / "profile.yaml").write_text( + yaml.safe_dump( + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "secret", "environment": "local"}, + "spec": {"runtime": {"type": "docker-compose"}, "modules": []}, + } + ) + ) + (nested_repo_dir / "child").mkdir() + child = nested_repo_dir / "child" / "profile.yaml" + child.write_text( + yaml.safe_dump( + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "child", "environment": "local"}, + "extends": ["../../../other-project/secret"], + "spec": {"runtime": {"type": "docker-compose"}, "modules": []}, + } + ) + ) + + resolved, _prov, diagnostics = resolve_profile(str(child), environment=None) + self.assertIsNone(resolved) + self.assertTrue(any(d.code == "E104" for d in diagnostics), diagnostics) + def test_no_extends_field_behaves_exactly_as_before(self): child = self._write_profile( "child", From c266adf832afd99649278e5536171848fc6f77ea Mon Sep 17 00:00:00 2001 From: RonaldHensbergen Date: Wed, 26 Aug 2026 08:21:36 +0200 Subject: [PATCH 5/5] Fix 3 more extends bugs found in rounds 4-6 of review Round 4 (cli/getter.py): _collect_extends_profile_dirs() had no cycle detection of its own, so a cyclic extends chain caused unbounded recursion / RecursionError in cds get instead of a clean error. Threaded a _visited frozenset through the recursion, mirroring _compose_extends's cycle-tracking stack. Round 5 (cli/security.py): validate/run_security_validation still called _load_yaml() directly on the profile when --environment was omitted, so cds security silently skipped config/modules/secrets declared only in a parent profile. Now routes through resolve_extends(), consistent with cds init/get/plan/validate. Removed the now-dead _load_yaml() helper. Round 6 (cli/overlay.py): _merge_profile_docs() crashed with a TypeError if a parent profile or environment overlay set `spec: null`, since merged.setdefault("spec", {}) is a no-op when "spec" is already present with value None. Normalize back to a dict before assigning spec.modules. Added regression tests: cycle detection with diamond-shaped (non- cyclic) extends graphs in cds get, extends-parent-only findings surfacing in cds security without --environment, and the spec:null crash fix, plus a new tests/fixtures/security/extends-parent-secret fixture. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/getter.py | 16 +++- cli/overlay.py | 7 ++ cli/security.py | 16 ++-- .../profiles/base/profile.yaml | 17 +++++ .../profiles/child/profile.yaml | 9 +++ tests/test_getter.py | 72 ++++++++++++++++++ tests/test_overlay.py | 76 +++++++++++++++++++ tests/test_security.py | 27 +++++++ 8 files changed, 231 insertions(+), 9 deletions(-) create mode 100644 tests/fixtures/security/extends-parent-secret/profiles/base/profile.yaml create mode 100644 tests/fixtures/security/extends-parent-secret/profiles/child/profile.yaml diff --git a/cli/getter.py b/cli/getter.py index 52509732..d99b7a67 100644 --- a/cli/getter.py +++ b/cli/getter.py @@ -234,14 +234,26 @@ def _resolve_source_profile_path(source_repo: Path, profile: str) -> Path: ) -def _collect_extends_profile_dirs(source_repo: Path, profile_path: Path) -> set[Path]: +def _collect_extends_profile_dirs( + source_repo: Path, profile_path: Path, _visited: frozenset[Path] = frozenset() +) -> set[Path]: """ Returns every parent profile directory reachable via `extends`, so `cds get` copies parent profile.yaml files (and, via _collect_asset_roots's module walk, their modules) too instead of silently omitting anything only declared in a parent profile. Reuses cli.overlay's own extends-ref resolution rather than re-implementing it. + + `_visited` tracks resolved profile paths already seen along the current + extends chain so a cyclic `extends` graph raises a clean GetError + instead of recursing until Python's recursion limit is hit; this + mirrors cli.overlay._compose_extends's own cycle-detection stack. """ + resolved_profile_path = profile_path.resolve() + if resolved_profile_path in _visited: + raise GetError(f"Cycle detected in extends chain at {profile_path}") + _visited = _visited | {resolved_profile_path} + doc, _diagnostics = load_yaml_file(profile_path) if doc is None: raise GetError(f"Could not load source profile {profile_path}") @@ -262,7 +274,7 @@ def _collect_extends_profile_dirs(source_repo: Path, profile_path: Path) -> set[ raise GetError(f'Could not resolve extends parent "{ref}" for {profile_path}') _require_within_repo(parent_path, source_repo, f'extends parent "{ref}"') dirs.add(parent_path.parent) - dirs.update(_collect_extends_profile_dirs(source_repo, parent_path)) + dirs.update(_collect_extends_profile_dirs(source_repo, parent_path, _visited)) return dirs diff --git a/cli/overlay.py b/cli/overlay.py index 416b8cfe..5f276ac1 100644 --- a/cli/overlay.py +++ b/cli/overlay.py @@ -153,6 +153,13 @@ def _merge_profile_docs( base_without_modules, overlay_without_modules, base_source, overlay_source, "", provenance ) merged.setdefault("spec", {}) + if not isinstance(merged["spec"], dict): + # An overlay/parent that sets `spec: null` (or any non-mapping) wins + # the deep-merge in _merge_value since it's a full-value override, + # not a sub-key merge. Normalize back to a dict here so the + # spec.modules assignment below doesn't crash; the profile will + # still fail downstream shape/schema validation as expected. + merged["spec"] = {} merged["spec"]["modules"] = _merge_modules( base_modules, overlay_modules, base_source, overlay_source, provenance ) diff --git a/cli/security.py b/cli/security.py index 7645bfd7..a7dfc443 100644 --- a/cli/security.py +++ b/cli/security.py @@ -64,11 +64,6 @@ def _load_json(path: Path | Traversable) -> Any: return json.load(f) -def _load_yaml(path: Path) -> Any: - with path.open("r", encoding="utf-8") as f: - return yaml.safe_load(f) - - # --------------------------------------------------------------------------- # Rule set loading # --------------------------------------------------------------------------- @@ -703,8 +698,15 @@ def run_security_validation( if profile is None: return [], overlay_diags else: - profile = _load_yaml(profile_path) - overlay_diags = [] + # Resolve extends even without --environment so security scanning + # sees the fully composed profile, not just the child document; + # otherwise config/modules/secrets introduced only by a parent + # profile would silently escape scanning (see cli.overlay.resolve_extends). + from .overlay import resolve_extends + + profile, _, overlay_diags = resolve_extends(str(profile_path)) + if profile is None: + return [], overlay_diags rule_set = _validate_rule_set(rule_schema_path, rule_set_path) profile_class = infer_profile_class(profile) diff --git a/tests/fixtures/security/extends-parent-secret/profiles/base/profile.yaml b/tests/fixtures/security/extends-parent-secret/profiles/base/profile.yaml new file mode 100644 index 00000000..4d8546e3 --- /dev/null +++ b/tests/fixtures/security/extends-parent-secret/profiles/base/profile.yaml @@ -0,0 +1,17 @@ +apiVersion: cds/v1alpha1 +kind: Profile +metadata: + name: base + environment: local +spec: + modules: + - id: superset + source: ../../../../rendered-command-secret/modules/superset + version: 0.1.0 + enabled: true + config: + services: + superset: + environment: + ADMIN_USERNAME: admin + ADMIN_PASSWORD: password diff --git a/tests/fixtures/security/extends-parent-secret/profiles/child/profile.yaml b/tests/fixtures/security/extends-parent-secret/profiles/child/profile.yaml new file mode 100644 index 00000000..9278ee6e --- /dev/null +++ b/tests/fixtures/security/extends-parent-secret/profiles/child/profile.yaml @@ -0,0 +1,9 @@ +apiVersion: cds/v1alpha1 +kind: Profile +metadata: + name: child + environment: local +extends: + - base +spec: + modules: [] diff --git a/tests/test_getter.py b/tests/test_getter.py index 58b08d18..22136ce0 100644 --- a/tests/test_getter.py +++ b/tests/test_getter.py @@ -150,6 +150,78 @@ def test_fetch_profile_copies_extends_parent_and_its_module(self) -> None: self.assertIn("modules/apps/demo", entry["assetRoots"]) self.assertIn("profiles/base", entry["assetRoots"]) + def test_fetch_profile_diamond_extends_is_not_a_false_positive_cycle(self) -> None: + # Regression test: _collect_extends_profile_dirs() must not treat a + # diamond-shaped extends graph (child extends left and right, both + # of which extend the same shared-base) as a cycle -- the shared + # parent is legitimately reached twice via two different paths. + with tempfile.TemporaryDirectory() as source_dir, tempfile.TemporaryDirectory() as dest_dir: + source_root = Path(source_dir) + destination_root = Path(dest_dir) + _make_source_repo(source_root) + _write( + source_root / "profiles" / "shared-base" / "profile.yaml", + """apiVersion: cds/v1alpha1 +kind: Profile +metadata: + name: shared-base +spec: + runtime: + type: docker-compose + modules: + - id: demo + source: ../../modules/apps/demo + config: {} +""", + ) + _write( + source_root / "profiles" / "left" / "profile.yaml", + """apiVersion: cds/v1alpha1 +kind: Profile +metadata: + name: left +extends: + - shared-base +spec: {} +""", + ) + _write( + source_root / "profiles" / "right" / "profile.yaml", + """apiVersion: cds/v1alpha1 +kind: Profile +metadata: + name: right +extends: + - shared-base +spec: {} +""", + ) + _write( + source_root / "profiles" / "diamond-child" / "profile.yaml", + """apiVersion: cds/v1alpha1 +kind: Profile +metadata: + name: diamond-child +extends: + - left + - right +spec: {} +""", + ) + + actions, manifest_path = fetch_profile( + "diamond-child", + local=str(source_root), + destination_root=destination_root, + ) + + self.assertGreater(len(actions), 0) + self.assertTrue((destination_root / "profiles" / "diamond-child" / "profile.yaml").exists()) + self.assertTrue((destination_root / "profiles" / "left" / "profile.yaml").exists()) + self.assertTrue((destination_root / "profiles" / "right" / "profile.yaml").exists()) + self.assertTrue((destination_root / "profiles" / "shared-base" / "profile.yaml").exists()) + self.assertTrue((destination_root / "modules" / "apps" / "demo" / "module.yaml").exists()) + def test_fetch_profile_requires_force_for_conflicting_files(self) -> None: with tempfile.TemporaryDirectory() as source_dir, tempfile.TemporaryDirectory() as dest_dir: source_root = Path(source_dir) diff --git a/tests/test_overlay.py b/tests/test_overlay.py index 48e8640c..4a955d34 100644 --- a/tests/test_overlay.py +++ b/tests/test_overlay.py @@ -8,6 +8,7 @@ _duplicate_module_ids, _merge_modules, _merge_value, + resolve_extends, resolve_profile, ) @@ -651,6 +652,81 @@ def test_no_extends_field_behaves_exactly_as_before(self): self.assertEqual(provenance, {}) self.assertEqual(resolved["spec"]["modules"][0]["config"]["replicas"], 1) + def test_diamond_shaped_extends_is_not_treated_as_a_cycle(self): + # A (child) extends B and C; both B and C extend D. D is reached + # twice via two different paths but this is NOT a cycle -- it's a + # legitimate diamond-shaped extends graph and must resolve cleanly. + self._write_profile( + "shared-base", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "shared-base", "environment": "local"}, + "spec": {"runtime": {"type": "docker-compose"}, "modules": [self._base_module(1)]}, + }, + ) + self._write_profile( + "left", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "left", "environment": "local"}, + "extends": ["shared-base"], + "spec": {"runtime": {"type": "docker-compose"}, "modules": []}, + }, + ) + self._write_profile( + "right", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "right", "environment": "local"}, + "extends": ["shared-base"], + "spec": {"runtime": {"type": "docker-compose"}, "modules": []}, + }, + ) + child = self._write_profile( + "diamond-child", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "diamond-child", "environment": "local"}, + "extends": ["left", "right"], + "spec": {"runtime": {"type": "docker-compose"}, "modules": []}, + }, + ) + resolved, _prov, diagnostics = resolve_profile(str(child), environment=None) + self.assertFalse(any(d.level == "error" for d in diagnostics), diagnostics) + self.assertEqual(resolved["spec"]["modules"][0]["id"], "db") + + def test_merge_profile_docs_does_not_crash_when_overlay_spec_is_null(self): + # Regression test: a parent profile or environment overlay with an + # explicit `spec: null` (YAML `~`) must not crash resolve_profile() + # with a TypeError; it should fail cleanly with diagnostics instead. + self._write_profile( + "base-null-spec", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "base-null-spec", "environment": "local"}, + "spec": {"runtime": {"type": "docker-compose"}, "modules": [self._base_module(1)]}, + }, + ) + child = self._write_profile( + "child-null-spec", + { + "apiVersion": "cds/v1alpha1", + "kind": "Profile", + "metadata": {"name": "child-null-spec", "environment": "local"}, + "extends": ["base-null-spec"], + "spec": None, + }, + ) + # Must not raise; diagnostics may report errors, but resolution + # itself must complete without an unhandled exception. + resolve_profile(str(child), environment=None) + resolve_extends(str(child)) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_security.py b/tests/test_security.py index e29a1a90..bdd92e4e 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -560,5 +560,32 @@ def test_does_not_plan_or_render_when_no_enabled_rule_uses_rendered_compose_scop tmp_rule_set_path.unlink() +class ExtendsAwareSecurityScanTest(unittest.TestCase): + """cds security must resolve `extends` even without --environment (issue #175).""" + + def test_finding_declared_only_in_extends_parent_is_still_detected(self): + # base/profile.yaml declares default admin credentials on a + # "superset" module; child/profile.yaml only has `extends: [base]` + # and no modules of its own. Scanning the child without an + # --environment flag must still surface the parent's finding. + profile_path = ( + _REPO_ROOT + / "tests" + / "fixtures" + / "security" + / "extends-parent-secret" + / "profiles" + / "child" + / "profile.yaml" + ) + + findings, diags = run_security_validation(profile_path, _RULE_SCHEMA_PATH, _RULE_SET_PATH) + + self.assertFalse(any(d.level == "error" for d in diags), diags) + hits = {f["path"] for f in findings if f["rule_id"] == "CDS-SEC-010"} + self.assertIn("services.superset.environment.ADMIN_USERNAME", hits) + self.assertIn("services.superset.environment.ADMIN_PASSWORD", hits) + + if __name__ == "__main__": unittest.main()