diff --git a/CHANGELOG.md b/CHANGELOG.md index aa39fa97..d71cb048 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -73,6 +73,16 @@ The format is based on Keep a Changelog. ### Fixed +- Fixed the `helm` render target lacking the provide-side contract-host + cross-check that the compose target got for issue #1010: the Kubernetes + renderer only validated the consume side (E084), so a provided + `http-service`, `sql-database`, `cache-service`, or `object-storage` + contract advertising a host no rendered Kubernetes Service matches did + not fail closed. `render_helm` now cross-checks every provided + contract's advertised network host (the `host` field, the `endpoint`'s + hostname part, or a `serviceName` resolved to the serving workload's + Service name) against the Services the render produces for that module + and fails with `E150` on mismatch (issue #1035). - The `Kubernetes E2E` workflow now pins its k3d cluster name (`cds-k8s-e2e-ci`) instead of deriving `cds-head` from the detached-HEAD checkout, so CI logs and failure diagnostics are attributable at a glance diff --git a/README.md b/README.md index cdec7f44..f5c57a98 100644 --- a/README.md +++ b/README.md @@ -788,7 +788,7 @@ Common errors from `cds validate`, `cds plan`, and `cds render`, and how to fix | `[E134] ... Production backups must be encrypted ...` / `... must be declared required: true` | A production profile has no `spec.backup.encryption.keyFrom`, or the referenced secret alias is declared `required: false` so the key can be missing at runtime. | Declare the encryption key alias with `required: true` so a missing key fails closed at startup. | | `[E135] ... Production backup policy "" must declare restoreVerification` | A production backup policy has no `restoreVerification` block. | Add `restoreVerification.frequencyDays` to every production backup policy.| | `[E136] ... Reverse-proxy route host ...` (unknown target / no resolvable http-service endpoint / backend address not a rendered compose service / entrypoint not declared by the proxy / undeclared certificate alias / duplicate route host) | A `config.routes` entry on a reverse-proxy provider names a module that is missing, does not provide an `http-service` contract with host/port (or a port outside 1-65535), advertises a backend address that matches no rendered compose service (multi-service modules must advertise `serviceName`), names a `.` contract the target does not provide, names an entrypoint the proxy's `config.entryPoints` does not declare, references an undeclared certificate alias, or duplicates the host of an earlier route on the same proxy (#581). | Fix the route's `target`/`certificate`/`host`/`entryPoints` (a bare module id routes to its first http-service contract; `.` picks a specific one; entrypoints must match the proxy's declared `config.entryPoints` keys), or advertise the serving compose service via `serviceName` in the target's http-service provide entry. | -| `[E150] ... Provided contract ... advertises network host ...` | A module's provided `http-service`, `sql-database`, `cache-service`, or `object-storage` contract advertises a network host (the `host` field, the `endpoint`'s hostname part, or a `serviceName` mapped through the renderer's service-naming rule) that matches no compose service the render produced for that module, so consumers would resolve a hostname nothing answers (#1010). | Instantiate the module with an id matching its compose service name, advertise the serving service via a `serviceName` field in the provide entry, or build the endpoint from `${service.dns}`. | +| `[E150] ... Provided contract ... advertises network host ...` | A module's provided `http-service`, `sql-database`, `cache-service`, or `object-storage` contract advertises a network host (the `host` field, the `endpoint`'s hostname part, or a `serviceName` mapped through the renderer's service-naming rule) that matches no service the render produced for that module -- no compose service on the compose target, no rendered Kubernetes Service on the helm target -- so consumers would resolve a hostname nothing answers (#1010, #1035). | Compose: instantiate the module with an id matching its compose service name, advertise the serving service via a `serviceName` field in the provide entry, or build the endpoint from `${service.dns}`. Helm: render the serving workload with a `service` block (or as a `StatefulSet`, which gets a governing Service), advertise the serving workload via a `serviceName` field, or build the endpoint from `${service.dns}`. | | `[E103] ... config.image.tag is required ... when config.image.source is "registry"` | A module instance sets `config.image.source: registry` without also setting `config.image.tag`. | Set `config.image.tag` to a version, or switch back to `config.image.source: build`. | | `[E139] ... without an explicit host authentication advertisement` / `... host authentication method "trust"` | A `metadata.environment: production` profile runs a `sql-database` provider whose contract does not advertise a host authentication method, or advertises `trust` (issue #206). | Configure the provider's authentication explicitly (e.g. postgres `config.authentication.hostAuth: scram-sha-256`); trust authentication is prohibited in production profiles. | | `[E140] ... configures webServer.mode "development"` | A production profile sets an HTTP-serving module's `config.webServer.mode` to `development`, selecting its built-in development web server (issue #216). | Set `config.webServer.mode` to `production` (or remove the override) so the service runs behind its production process manager; see `docs/serving-modes.md`. | diff --git a/cli/k8s_renderer.py b/cli/k8s_renderer.py index fa6c660b..ec846a27 100644 --- a/cli/k8s_renderer.py +++ b/cli/k8s_renderer.py @@ -44,6 +44,7 @@ _resolve_profile_dir, _resolve_project_root, _substitute_values, + _validate_provided_contract_hosts_against, apply_production_gates, ) from .secret_leak import ( @@ -149,6 +150,7 @@ def render_helm( # ${k8s.service.*} can point forward to a workload declared later. service_names = _collect_service_names(renderable) diagnostics.extend(_check_binding_services(renderable, service_names)) + diagnostics.extend(_check_provided_contract_services(renderable, service_names)) # The built-in backup capability (config.backup.enabled, issue #665) is # only implemented for the compose target: there is no Kubernetes @@ -1873,6 +1875,49 @@ def _check_binding_services( return diagnostics +def _check_provided_contract_services( + modules: list[dict[str, Any]], service_names: dict[str, str] +) -> list[Diagnostic]: + """ + Provide-side counterpart of the E084 consume-side check (issue #1035): + cross-check every provided contract's advertised network host against + the Kubernetes Services the render actually produces for that module. + + Delegates to the E150 core shared with the compose renderer, with the + rendered service names taken from the workloads that get a Service + manifest (`_collect_service_names`). A `serviceName` advertisement + names a workload and is resolved to the Service name that workload + renders (a workload-level `serviceName` override may rename it), so a + contract advertising a host no rendered Service answers fails closed + instead of shipping an endpoint consumers cannot resolve. + """ + rendered_by_module: dict[str, list[str]] = {} + for module in modules: + if not isinstance(module, dict): + continue + module_id = str(module.get("id")) + k8s = (module.get("implementation") or {}).get("kubernetes") or {} + workloads = k8s.get("workloads") or {} + rendered = { + service_names[full] + for workload_name in workloads + if (full := _compose_service_name(module_id, workload_name)) in service_names + } + if rendered: + rendered_by_module[module_id] = sorted(rendered) + return _validate_provided_contract_hosts_against( + modules, + rendered_by_module, + service_kind="Kubernetes Service", + fix_hint=( + "Render the serving workload with a service block (or as a " + "StatefulSet), advertise the serving workload via a serviceName " + "field, or build the endpoint from ${service.dns}." + ), + resolve_service_name=lambda _module_id, host: service_names.get(host, host), + ) + + def _sanitize_name(name: str) -> str: return _k8s_name(name) diff --git a/cli/renderer.py b/cli/renderer.py index 8077c4f7..bb57beac 100644 --- a/cli/renderer.py +++ b/cli/renderer.py @@ -7,6 +7,7 @@ import os import re +from collections.abc import Callable from copy import deepcopy from pathlib import Path from typing import Any @@ -282,28 +283,57 @@ def _endpoint_hostname(value: Any) -> str | None: return value -def _validate_provided_contract_hosts( - plan: dict[str, Any], - module_service_names: dict[str, list[str]], +def _provided_contract_host_candidates( + kind: Any, spec: dict[str, Any], module_id: str +) -> list[tuple[str, str | None]]: + """ + The (field, host) pairs a provided contract's spec advertises as network + addresses, per contract kind. Shared by both render targets' E150 + cross-checks (issues #1010, #1035). + """ + candidates: list[tuple[str, str | None]] = [] + if kind == "http-service": + advertised_service = spec.get("serviceName") + if isinstance(advertised_service, str) and advertised_service: + candidates.append( + ("serviceName", _compose_service_name(module_id, advertised_service)) + ) + else: + candidates.append(("host", spec.get("host"))) + elif kind in ("sql-database", "cache-service"): + candidates.append(("host", spec.get("host"))) + elif kind == "object-storage": + candidates.append(("endpoint", _endpoint_hostname(spec.get("endpoint")))) + return candidates + + +def _validate_provided_contract_hosts_against( + modules: Any, + rendered_by_module: dict[str, list[str]], + *, + service_kind: str, + fix_hint: str, + resolve_service_name: Callable[[str, str], str] | None = None, ) -> list[Diagnostic]: """ - Cross-check every provided contract's advertised network host against - the compose services the render actually produced (issue #1010). - - Mirrors the E136 route check: a contract advertising a host nothing - answers on the Compose network fails closed with E150 instead of - shipping a broken endpoint. Multi-service modules advertise their - serving service via `serviceName`; that name is mapped through the same - service-naming rule the renderer uses. Hosts outside the Compose - namespace (dots, ports, credentials) are not checked, and modules - whose services failed to render are left to their own diagnostics. + E150 cross-check core shared by both render targets (issues #1010, + #1035): every provided contract's advertised network host must match + a service the render produced for that module, else the render fails + closed instead of shipping a broken endpoint. `service_kind` and + `fix_hint` word the diagnostic for the target ("compose service" vs + "Kubernetes Service"); `resolve_service_name` optionally maps a + `serviceName` advertisement to the name the workload actually renders + (the helm target's workload-level `serviceName` overrides). Hosts + outside the cluster namespace (dots, ports, credentials) are not + checked, and modules whose services failed to render are left to + their own diagnostics. """ diagnostics: list[Diagnostic] = [] - for module in plan.get("modules", []): + for module in modules: if not isinstance(module, dict): continue module_id = str(module.get("id")) - rendered = module_service_names.get(module_id) or [] + rendered = rendered_by_module.get(module_id) or [] if not rendered: continue provides = module.get("provides") @@ -316,22 +346,11 @@ def _validate_provided_contract_hosts( spec = provided.get("spec") if not isinstance(spec, dict): continue - candidates: list[tuple[str, str | None]] = [] - if kind == "http-service": - advertised_service = spec.get("serviceName") - if isinstance(advertised_service, str) and advertised_service: - candidates.append( - ("serviceName", _compose_service_name(module_id, advertised_service)) - ) - else: - candidates.append(("host", spec.get("host"))) - elif kind in ("sql-database", "cache-service"): - candidates.append(("host", spec.get("host"))) - elif kind == "object-storage": - candidates.append(("endpoint", _endpoint_hostname(spec.get("endpoint")))) - for field, host in candidates: + for field, host in _provided_contract_host_candidates(kind, spec, module_id): if not isinstance(host, str) or not _DNS_LABEL_RE.match(host): continue + if field == "serviceName" and resolve_service_name is not None: + host = resolve_service_name(module_id, host) if host in rendered: continue diagnostics.append(Diagnostic( @@ -339,20 +358,42 @@ def _validate_provided_contract_hosts( code="E150", message=( f'Provided contract "{provide_name}" of module "{module_id}" ' - f'advertises network host "{host}", which is not a compose ' - "service the render produced for that module (rendered " + f'advertises network host "{host}", which is not a {service_kind} ' + "the render produced for that module (rendered " f'services: {", ".join(rendered)}); consumers of the contract ' - "would resolve a hostname nothing answers. Instantiate " - "the module with an id matching its compose service " - "name, advertise the serving service via a " - "serviceName field, or build the endpoint from " - "${service.dns}." + "would resolve a hostname nothing answers. " + fix_hint ), path=f"module:{module_id}.provides.{provide_name}.spec.{field}", )) return diagnostics +def _validate_provided_contract_hosts( + plan: dict[str, Any], + module_service_names: dict[str, list[str]], +) -> list[Diagnostic]: + """ + Cross-check every provided contract's advertised network host against + the compose services the render actually produced (issue #1010). + + Mirrors the E136 route check: a contract advertising a host nothing + answers on the Compose network fails closed with E150 instead of + shipping a broken endpoint. Multi-service modules advertise their + serving service via `serviceName`; that name is mapped through the same + service-naming rule the renderer uses. + """ + return _validate_provided_contract_hosts_against( + plan.get("modules", []), + module_service_names, + service_kind="compose service", + fix_hint=( + "Instantiate the module with an id matching its compose service " + "name, advertise the serving service via a serviceName field, or " + "build the endpoint from ${service.dns}." + ), + ) + + def _generate_reverse_proxy_routes( plan: dict[str, Any], module_service_names: dict[str, list[str]] ) -> tuple[dict[str, dict[str, Any]], list[Diagnostic]]: diff --git a/docs/modules.md b/docs/modules.md index 42e24154..62c30eee 100644 --- a/docs/modules.md +++ b/docs/modules.md @@ -249,7 +249,12 @@ mapped through the renderer's service-naming rule first. Mismatches fail with `E150` instead of shipping a contract endpoint nothing answers. `${service.dns}` resolves through the same naming rule during planning, so modules with a single or namesake compose service -advertise a resolvable host for any instance id. +advertise a resolvable host for any instance id. On the `helm` target +the same cross-check runs against the Kubernetes Services the render +produces for the module (workloads with a `service` block, plus the +governing Service of every StatefulSet), so a provided contract whose +host matches no rendered Service also fails with `E150` there +(issue #1035). A route's optional `certificate` names a profile certificate alias (`spec.certificates.values`, issue #578). The alias is only a name: CDS diff --git a/tests/test_k8s_renderer.py b/tests/test_k8s_renderer.py index 3f6f0e1e..bbdc4a21 100644 --- a/tests/test_k8s_renderer.py +++ b/tests/test_k8s_renderer.py @@ -536,6 +536,178 @@ def test_contract_provider_without_a_service_is_rejected(self) -> None: self.assertEqual(files, {}) self.assertIn("E084", {diagnostic.code for diagnostic in diagnostics}) + def test_provided_contract_host_without_matching_service_is_rejected(self) -> None: + # Provide-side counterpart of the E084 test above (issue #1035): a + # provided contract advertising a host no rendered k8s Service + # matches must fail closed with E150, like the compose renderer does. + plan = self.minimal_plan() + plan["modules"][0]["provides"] = { + "http": { + "kind": "http-service", + "spec": {"host": "nowhere", "port": 8080}, + } + } + + files, diagnostics = render_helm(plan) + + self.assertEqual(files, {}) + matching = [d for d in diagnostics if d.code == "E150"] + self.assertEqual(len(matching), 1) + self.assertEqual(matching[0].level, "error") + self.assertEqual( + matching[0].path, "module:demo.provides.http.spec.host" + ) + self.assertIn("not a Kubernetes Service", matching[0].message) + + def test_provided_contract_matching_a_rendered_service_passes(self) -> None: + plan = self.minimal_plan() + plan["modules"][0]["provides"] = { + "http": { + "kind": "http-service", + "spec": {"host": "demo-app", "port": 8080}, + }, + "sql": { + "kind": "sql-database", + "spec": {"host": "demo-app", "port": 5432}, + }, + } + + files, diagnostics = render_helm(plan) + + self.assertFalse(any(d.level == "error" for d in diagnostics)) + self.assertTrue(files) + + def test_provided_contract_service_name_maps_through_naming_rule(self) -> None: + plan = self.minimal_plan() + plan["modules"][0]["provides"] = { + "http": { + "kind": "http-service", + "spec": {"host": "demo", "serviceName": "app", "port": 8080}, + } + } + + # The workload "app" renders Service "demo-app", so serviceName + # "app" resolves even though the bare host "demo" does not. + files, diagnostics = render_helm(plan) + self.assertFalse(any(d.level == "error" for d in diagnostics)) + self.assertTrue(files) + + broken = self.minimal_plan() + broken["modules"][0]["provides"] = { + "http": { + "kind": "http-service", + "spec": {"host": "demo", "serviceName": "missing", "port": 8080}, + } + } + files, diagnostics = render_helm(broken) + self.assertEqual(files, {}) + matching = [d for d in diagnostics if d.code == "E150"] + self.assertEqual(len(matching), 1) + self.assertEqual( + matching[0].path, "module:demo.provides.http.spec.serviceName" + ) + + def test_provided_sql_database_and_object_storage_hosts_are_checked(self) -> None: + plan = self.minimal_plan() + plan["modules"][0]["provides"] = { + "sql": {"kind": "sql-database", "spec": {"host": "no-db"}}, + "storage": { + "kind": "object-storage", + "spec": {"endpoint": "http://no-store:3900"}, + }, + } + + files, diagnostics = render_helm(plan) + + self.assertEqual(files, {}) + matching = [d for d in diagnostics if d.code == "E150"] + self.assertEqual( + [d.path for d in matching], + [ + "module:demo.provides.sql.spec.host", + "module:demo.provides.storage.spec.endpoint", + ], + ) + + def test_provided_contract_external_hosts_are_skipped(self) -> None: + plan = self.minimal_plan() + plan["modules"][0]["provides"] = { + "sql": { + "kind": "sql-database", + "spec": {"host": "db.internal.example.com"}, + }, + "storage": { + "kind": "object-storage", + "spec": {"endpoint": "https://s3.example.com:443"}, + }, + "http": {"kind": "http-service", "spec": {}}, + } + + files, diagnostics = render_helm(plan) + + self.assertFalse(any(d.level == "error" for d in diagnostics)) + self.assertTrue(files) + + def test_provided_contract_module_without_rendered_services_is_skipped(self) -> None: + # Mirrors the compose check: a module whose workloads render no + # Service at all is left to its own diagnostics. + plan = self.minimal_plan() + workload = plan["modules"][0]["implementation"]["kubernetes"]["workloads"][ + "app" + ] + workload.pop("service") + workload["kind"] = "Deployment" + plan["modules"][0]["provides"] = { + "http": {"kind": "http-service", "spec": {"host": "nowhere"}} + } + + files, diagnostics = render_helm(plan) + + self.assertNotIn("E150", {diagnostic.code for diagnostic in diagnostics}) + self.assertFalse(any(d.level == "error" for d in diagnostics)) + + def test_provided_contract_host_resolves_workload_service_name_override(self) -> None: + # A workload-level serviceName renames the rendered Service, so the + # advertised host must be the Service name, and a serviceName + # advertisement resolves through the override. + plan = self.minimal_plan() + workload = plan["modules"][0]["implementation"]["kubernetes"]["workloads"][ + "app" + ] + workload["serviceName"] = "custom-svc" + plan["modules"][0]["provides"] = { + "http": { + "kind": "http-service", + "spec": {"host": "custom-svc", "port": 8080}, + } + } + + files, diagnostics = render_helm(plan) + self.assertFalse(any(d.level == "error" for d in diagnostics)) + self.assertIn( + "name: '{{ .Release.Name }}-custom-svc'", + files["templates/demo-app-service.yaml"], + ) + + plan["modules"][0]["provides"] = { + "http": { + "kind": "http-service", + "spec": {"host": "demo-app", "port": 8080}, + } + } + files, diagnostics = render_helm(plan) + self.assertEqual(files, {}) + self.assertIn("E150", {d.code for d in diagnostics}) + + plan["modules"][0]["provides"] = { + "http": { + "kind": "http-service", + "spec": {"serviceName": "app", "port": 8080}, + } + } + files, diagnostics = render_helm(plan) + self.assertFalse(any(d.level == "error" for d in diagnostics)) + def test_missing_resources_emit_warning(self) -> None: plan = self.minimal_plan() plan["modules"][0]["implementation"]["kubernetes"]["workloads"]["app"].pop(