diff --git a/README.md b/README.md index 16ac468..c6bd487 100644 --- a/README.md +++ b/README.md @@ -86,7 +86,7 @@ kubectl get xrdconversionconfig xpostgresqlinstances-conversion -o yaml ## Examples -[`examples/`](examples/) holds five self-contained conversion stories, smallest first — a field rename, an enum remap, a `forEach` array reshape, a three-version Crossplane XR migration, and the same model against a plain native CRD. Each directory has a schema, a config, sample objects at every served version, and a README explaining the scenario, and each is independently runnable offline: +[`examples/`](examples/) holds six self-contained conversion stories, smallest first — a field rename, an enum remap, a `forEach` array reshape, a `oneOf` union branch mapping, a three-version Crossplane XR migration, and the same model against a plain native CRD. Each directory has a schema, a config, sample objects at every served version, and a README explaining the scenario, and each is independently runnable offline: ```console go run ./cmd/convctl test --config examples/field-rename/xrdconversionconfig.yaml \ @@ -95,7 +95,7 @@ go run ./cmd/convctl test --config examples/field-rename/xrdconversionconfig.yam ## Conversion strategies -`fieldRename`, `scalarToObject` / `objectToScalar`, `singletonArrayToObject` / `objectToSingletonArray`, `fieldsToMap` / `mapToFields`, `toAnnotation` / `toLabel`, `fromAnnotation` / `fromLabel`, `enumRemap`, `defaultValue`, `constant`, `delete`, `jsonPatch` (escape hatch), `forEach` (per-array-element, up to two nested levels), `typeCoerce`, `scalarToFields` / `fieldsToScalar`, `arrayToMapByKey` / `mapToArrayByKey`, `numericScale`, `listJoin` / `listSplit`, `quantity`, `duration`, `mapKeyRename`, `cel` (always-lossy value-math escape hatch). Every rule that the engine determines is lossy in any direction requires `acknowledgeLossy: true` plus an optional `reason` — this is enforced by both the admission webhook and the controller, and the default posture is fail-closed: any hub or spoke field left uncovered by a rule (and not structurally identical on both sides) is a validation error, not a silent pass. +`fieldRename`, `scalarToObject` / `objectToScalar`, `singletonArrayToObject` / `objectToSingletonArray`, `fieldsToMap` / `mapToFields`, `toAnnotation` / `toLabel`, `fromAnnotation` / `fromLabel`, `enumRemap`, `defaultValue`, `constant`, `delete`, `jsonPatch` (escape hatch), `forEach` (per-array-element, up to two nested levels), `typeCoerce`, `scalarToFields` / `fieldsToScalar`, `arrayToMapByKey` / `mapToArrayByKey`, `numericScale`, `listJoin` / `listSplit`, `quantity`, `duration`, `mapKeyRename`, `cel` (always-lossy value-math escape hatch), `branchMap` (maps the branches of a `oneOf` union between versions). Every rule that the engine determines is lossy in any direction requires `acknowledgeLossy: true` plus an optional `reason` — this is enforced by both the admission webhook and the controller, and the default posture is fail-closed: any hub or spoke field left uncovered by a rule (and not structurally identical on both sides) is a validation error, not a silent pass. A few of the newer strategies are worth calling out specifically: @@ -104,6 +104,7 @@ A few of the newer strategies are worth calling out specifically: - **`arrayToMapByKey`** / **`mapToArrayByKey`** convert a list of objects into a map keyed by one of their fields, and back — the standard "list-map versus map" API-evolution pattern. Array→map is lossless (a duplicate or missing key is a hard runtime error, never a silent drop); map→array is always treated as lossy, since the reconstructed array is emitted sorted by key rather than reproducing whatever order the original array had. - **`numericScale`** rescales a numeric field by a fixed factor (`hubValue == spokeValue * factor`) — e.g. stored megabytes displayed as gigabytes. Whichever direction lands on an integer-typed field is treated as lossy, since the division/multiplication may not land on a whole number for every possible input. - **`listJoin`** / **`listSplit`** convert an array of scalars into a single delimited string, and back. Always lossless; an element that happens to contain the separator as a substring will fail to round-trip cleanly, which is correctly surfaced by `convctl test` as a genuine data problem rather than an expected characteristic of the strategy. +- **`branchMap`** maps the branches of a `oneOf` union — the "exactly one of `s3`, `gcs` or `azure`" shape — between versions, remapping an optional discriminator alongside and running nested rules scoped to each branch. In a CRD the apiserver requires every property named inside a `oneOf` to also be declared in the parent's own `properties`, so a branch is an ordinary addressable field and the active one is identified by which branch property is present, not by validating against each branch schema. Converting an object with no branch set, or with more than one, is a hard error in both directions; collapsing two hub branches onto one spoke branch is expressible but lossy coming back. `internal/cli/testdata/full` exercises all of these (and every other built-in strategy) end to end across a 3-version fixture — it's the best starting point for seeing exact YAML shapes in context. @@ -166,14 +167,14 @@ a [kind](https://kind.sigs.k8s.io/) cluster, builds this repo's `manager`/`webhook-server` images and loads them straight into the cluster (no registry push), and installs the operator via its own Helm chart: -- `make test-e2e` (`hack/e2e-test.sh`) — both features enabled (the common case): installs cert-manager and [Crossplane](https://crossplane.io) (v2 — this operator targets Crossplane's current `apiextensions.crossplane.io/v2` XRD API), applies a real `CompositeResourceDefinition` + `XRDConversionConfig` covering all 29 built-in strategies, and confirms composite resources created at every served version read back correctly converted at every other version. +- `make test-e2e` (`hack/e2e-test.sh`) — both features enabled (the common case): installs cert-manager and [Crossplane](https://crossplane.io) (v2 — this operator targets Crossplane's current `apiextensions.crossplane.io/v2` XRD API), applies a real `CompositeResourceDefinition` + `XRDConversionConfig` covering all 30 built-in strategies, and confirms composite resources created at every served version read back correctly converted at every other version. - `make test-e2e-crd-only` (`hack/e2e-test-crd-only.sh`) — `features.crossplane.enabled=false`, Crossplane never installed at all: confirms the manager comes up healthy with no Crossplane CRDs on the cluster, that a `CRDConversionConfig` against a plain native CRD converts correctly, and that an `XRDConversionConfig` is rejected outright by the admission webhook. - `make test-e2e-crossplane-only` (`hack/e2e-test-crossplane-only.sh`) — `features.nativeCRD.enabled=false`: confirms XRD/Crossplane conversion is unaffected by disabling native CRD support, and that a `CRDConversionConfig` is rejected outright. - `make test-e2e-legacy-claims` (`hack/e2e-test-legacy-claims.sh`) — `scope: LegacyCluster` with `claimNames`, the shape every cluster upgraded from Crossplane 1.x still runs and the only one that generates a **claim CRD**: proves a claim created at `v1` reads back correctly converted at `v2` and `v3`, that the bare `spec.*` machinery layout (`compositionRef`, `claimRef`, `resourceRef`, `compositeDeletePolicy`, `writeConnectionSecretToRef`) survives conversion on both object classes, that a condition the test itself writes survives alongside Crossplane's, that **both** generated CRDs carry `spec.conversion` and `ConversionPropagated` reaches True, and that `convctl test --live` and `migrate-storage --prune-stored-versions` cover both. - `make test-e2e-package-managed` (`hack/e2e-test-package-managed.sh`) — the **XRD conversion guard**: replays the Crossplane package establisher's full non-SSA replace of an XRD in a loop and asserts that not one read at a non-storage version ever comes back unconverted. Then repeats with the guard disabled and asserts the loop **does** catch bad reads — a guard test that cannot fail is not a test. The failure mode is an HTTP 200 with wrong data, so the loop checks converted field values rather than exit codes. Also needs `python3`. - `make test-e2e-load` (`hack/e2e-load.sh`) — native-CRD kind cluster, then synthetic `ConversionReview` batches of varying object count/size against the live webhook-server; prints latency/throughput for [Capacity planning](docs/operations/capacity.md). - `make test-e2e-reassign` (`hack/e2e-reassign.sh`) — moves a target between two `ConversionWebhookServer` instances, three times (an explicit `webhookServerRef` pin, an unpin, and a sharding-driven move), while sustained reads and writes flow through it, and asserts **zero failed requests and zero wrong values**. Also asserts each move was *verified* against the destination's published served targets rather than taking the unverified fallback, so it cannot pass with the handover mechanism removed. Also needs `python3`. -- `make test-e2e-scale` (`hack/e2e-scale.sh`) — native-CRD kind cluster, then a generated fleet of CRDs (3 versions each, 3–10 strategies per spoke, all 29 strategies used) plus parallel Get/List of live CRs through the apiserver conversion path. Override `TARGETS`, `INSTANCES`, and `PARALLEL` (for example `TARGETS=100 INSTANCES=100 PARALLEL=32`); set `RESULT_JSON` to write the measurements as JSON. Not in the PR matrix — it runs nightly (`.github/workflows/scale.yml`) at 300 × 20, publishing an artifact and failing on a relative regression. +- `make test-e2e-scale` (`hack/e2e-scale.sh`) — native-CRD kind cluster, then a generated fleet of CRDs (3 versions each, 3–10 strategies per spoke, all 30 strategies used) plus parallel Get/List of live CRs through the apiserver conversion path. Override `TARGETS`, `INSTANCES`, and `PARALLEL` (for example `TARGETS=100 INSTANCES=100 PARALLEL=32`); set `RESULT_JSON` to write the measurements as JSON. Not in the PR matrix — it runs nightly (`.github/workflows/scale.yml`) at 300 × 20, publishing an artifact and failing on a relative regression. Requires `docker`, `kind`, `kubectl`, and `helm` on `PATH` (plus `go` for `test-e2e-legacy-claims` and `python3` for `test-e2e-package-managed` and diff --git a/api/v1alpha1/xrdconversionconfig_convert.go b/api/v1alpha1/xrdconversionconfig_convert.go index 1bb8e3c..122dbba 100644 --- a/api/v1alpha1/xrdconversionconfig_convert.go +++ b/api/v1alpha1/xrdconversionconfig_convert.go @@ -389,6 +389,29 @@ func convertParams(r ConversionRule) (engine.RuleParams, error) { Renames: r.MapKeyRename.Renames, }, nil + case StrategyBranchMap: + if r.BranchMap == nil { + return nil, errors.New("requires branchMap params") + } + branches := make([]engine.BranchMapping, 0, len(r.BranchMap.Branches)) + for i, b := range r.BranchMap.Branches { + nested, err := convertRules(b.Rules) + if err != nil { + return nil, fmt.Errorf("branch %d (%s): %w", i, b.HubBranch, err) + } + branches = append(branches, engine.BranchMapping{ + HubBranch: b.HubBranch, SpokeBranch: b.SpokeBranch, + HubDiscriminatorValue: b.HubDiscriminatorValue, + SpokeDiscriminatorValue: b.SpokeDiscriminatorValue, + Rules: nested, + }) + } + return engine.BranchMapParams{ + HubPath: engine.ParsePath(r.BranchMap.HubPath), SpokePath: engine.ParsePath(r.BranchMap.SpokePath), + Discriminator: r.BranchMap.Discriminator, + Branches: branches, + }, nil + case StrategyCEL: if r.CEL == nil { return nil, errors.New("requires cel params") diff --git a/api/v1alpha1/xrdconversionconfig_types.go b/api/v1alpha1/xrdconversionconfig_types.go index f285a3f..a7e4392 100644 --- a/api/v1alpha1/xrdconversionconfig_types.go +++ b/api/v1alpha1/xrdconversionconfig_types.go @@ -72,7 +72,7 @@ const ( ) // Strategy names one of the engine's built-in conversion strategies. -// +kubebuilder:validation:Enum=FieldRename;ScalarToObject;ObjectToScalar;SingletonArrayToObject;ObjectToSingletonArray;FieldsToMap;MapToFields;ToAnnotation;ToLabel;FromAnnotation;FromLabel;EnumRemap;DefaultValue;Constant;Delete;JSONPatch;ForEach;TypeCoerce;ScalarToFields;FieldsToScalar;ArrayToMapByKey;MapToArrayByKey;NumericScale;ListJoin;ListSplit;Quantity;Duration;MapKeyRename;CEL +// +kubebuilder:validation:Enum=FieldRename;ScalarToObject;ObjectToScalar;SingletonArrayToObject;ObjectToSingletonArray;FieldsToMap;MapToFields;ToAnnotation;ToLabel;FromAnnotation;FromLabel;EnumRemap;DefaultValue;Constant;Delete;JSONPatch;ForEach;TypeCoerce;ScalarToFields;FieldsToScalar;ArrayToMapByKey;MapToArrayByKey;NumericScale;ListJoin;ListSplit;Quantity;Duration;MapKeyRename;CEL;BranchMap type Strategy string const ( @@ -105,6 +105,7 @@ const ( StrategyDuration Strategy = "Duration" StrategyMapKeyRename Strategy = "MapKeyRename" StrategyCEL Strategy = "CEL" + StrategyBranchMap Strategy = "BranchMap" ) // TargetXRDRef identifies the Crossplane CompositeResourceDefinition this @@ -303,6 +304,59 @@ type JSONPatchParams struct { LosslessOverride bool `json:"losslessOverride,omitempty"` } +// BranchMapParams maps the branches of a union-typed field between hub and +// spoke — the "one of s3, gcs or azure" shape mature platform APIs express +// with `oneOf`. +// +// The active branch is identified by **which branch property is present**, +// not by validating the object against each branch schema. That is what a +// union looks like in a legal CRD: the apiserver requires every property +// named inside a `oneOf` to also be declared in the parent's own +// properties, so a union is a set of declared, mutually-exclusive fields. +// +// No branch set, or more than one, is a hard runtime conversion error. +// That is oneOf's contract — exactly one — and not anyOf's, which permits +// overlap. BranchMap fits an anyOf whose branches are mutually exclusive +// in practice; one that genuinely allows two at once has no single +// correspondence to map, and is better handled with ordinary rules over +// the individual branch properties. +type BranchMapParams struct { + // HubPath and SpokePath are the union-typed OBJECT on each side, not a + // branch within it. + HubPath string `json:"hubPath"` + SpokePath string `json:"spokePath"` + // Discriminator optionally names a sibling property whose value also + // identifies the branch (`backend: s3`). It is remapped alongside the + // branch, so hub and spoke may spell their branch names differently. + // It must be a declared property of both unions. + // +optional + Discriminator string `json:"discriminator,omitempty"` + // +kubebuilder:validation:MinItems=1 + Branches []BranchMapping `json:"branches"` +} + +// BranchMapping is one branch correspondence. +type BranchMapping struct { + // HubBranch and SpokeBranch are property names inside the two union + // objects. Both must be declared properties. + HubBranch string `json:"hubBranch"` + SpokeBranch string `json:"spokeBranch"` + // HubDiscriminatorValue and SpokeDiscriminatorValue are used only when + // BranchMapParams.Discriminator is set. Empty means the branch's own + // name, which is the common case. + // +optional + HubDiscriminatorValue string `json:"hubDiscriminatorValue,omitempty"` + // +optional + SpokeDiscriminatorValue string `json:"spokeDiscriminatorValue,omitempty"` + // Rules apply to this branch pair, with paths relative to the branch + // itself — `bucket`, not `spec.backup.s3.bucket`. The same scoping + // ForEach gives an array element. + // +optional + // +kubebuilder:pruning:PreserveUnknownFields + // +kubebuilder:validation:Schemaless + Rules []ConversionRule `json:"rules,omitempty"` +} + // ForEachParams applies a nested rule list to each element of a hub array // and the corresponding spoke array. Nested rule paths are relative to a // single array element. Nesting is capped at depth 2 (a ForEach may wrap @@ -508,6 +562,8 @@ type ConversionRule struct { MapKeyRename *MapKeyRenameParams `json:"mapKeyRename,omitempty"` // +optional CEL *CELParams `json:"cel,omitempty"` + // +optional + BranchMap *BranchMapParams `json:"branchMap,omitempty"` // AcknowledgeLossy must be true if this rule is lossy in any // direction, or validation fails (fail-closed default posture). diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 4f3874a..7c9eae9 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -73,6 +73,50 @@ func (in *AutoscalingSpec) DeepCopy() *AutoscalingSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *BranchMapParams) DeepCopyInto(out *BranchMapParams) { + *out = *in + if in.Branches != nil { + in, out := &in.Branches, &out.Branches + *out = make([]BranchMapping, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new BranchMapParams. +func (in *BranchMapParams) DeepCopy() *BranchMapParams { + if in == nil { + return nil + } + out := new(BranchMapParams) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *BranchMapping) DeepCopyInto(out *BranchMapping) { + *out = *in + if in.Rules != nil { + in, out := &in.Rules, &out.Rules + *out = make([]ConversionRule, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new BranchMapping. +func (in *BranchMapping) DeepCopy() *BranchMapping { + if in == nil { + return nil + } + out := new(BranchMapping) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *CELParams) DeepCopyInto(out *CELParams) { *out = *in @@ -429,6 +473,11 @@ func (in *ConversionRule) DeepCopyInto(out *ConversionRule) { *out = new(CELParams) (*in).DeepCopyInto(*out) } + if in.BranchMap != nil { + in, out := &in.BranchMap, &out.BranchMap + *out = new(BranchMapParams) + (*in).DeepCopyInto(*out) + } if in.When != nil { in, out := &in.When, &out.When *out = new(RuleWhen) diff --git a/charts/declarative-conversion-operator/crds/terasky.com_crdconversionconfigs.yaml b/charts/declarative-conversion-operator/crds/terasky.com_crdconversionconfigs.yaml index 5f29d97..12c6e2a 100644 --- a/charts/declarative-conversion-operator/crds/terasky.com_crdconversionconfigs.yaml +++ b/charts/declarative-conversion-operator/crds/terasky.com_crdconversionconfigs.yaml @@ -113,6 +113,75 @@ spec: - keyField - spokePath type: object + branchMap: + description: |- + BranchMapParams maps the branches of a union-typed field between hub and + spoke — the "one of s3, gcs or azure" shape mature platform APIs express + with `oneOf`. + + The active branch is identified by **which branch property is present**, + not by validating the object against each branch schema. That is what a + union looks like in a legal CRD: the apiserver requires every property + named inside a `oneOf` to also be declared in the parent's own + properties, so a union is a set of declared, mutually-exclusive fields. + + No branch set, or more than one, is a hard runtime conversion error. + That is oneOf's contract — exactly one — and not anyOf's, which permits + overlap. BranchMap fits an anyOf whose branches are mutually exclusive + in practice; one that genuinely allows two at once has no single + correspondence to map, and is better handled with ordinary rules over + the individual branch properties. + properties: + branches: + items: + description: BranchMapping is one branch correspondence. + properties: + hubBranch: + description: |- + HubBranch and SpokeBranch are property names inside the two union + objects. Both must be declared properties. + type: string + hubDiscriminatorValue: + description: |- + HubDiscriminatorValue and SpokeDiscriminatorValue are used only when + BranchMapParams.Discriminator is set. Empty means the branch's own + name, which is the common case. + type: string + rules: + description: |- + Rules apply to this branch pair, with paths relative to the branch + itself — `bucket`, not `spec.backup.s3.bucket`. The same scoping + ForEach gives an array element. + x-kubernetes-preserve-unknown-fields: true + spokeBranch: + type: string + spokeDiscriminatorValue: + type: string + required: + - hubBranch + - spokeBranch + type: object + minItems: 1 + type: array + discriminator: + description: |- + Discriminator optionally names a sibling property whose value also + identifies the branch (`backend: s3`). It is remapped alongside the + branch, so hub and spoke may spell their branch names differently. + It must be a declared property of both unions. + type: string + hubPath: + description: |- + HubPath and SpokePath are the union-typed OBJECT on each side, not a + branch within it. + type: string + spokePath: + type: string + required: + - branches + - hubPath + - spokePath + type: object cel: description: |- CELParams is an always-lossy escape hatch. Both expressions receive the @@ -705,6 +774,7 @@ spec: - Duration - MapKeyRename - CEL + - BranchMap type: string toAnnotation: description: ToMetadataParams backs both ToAnnotation @@ -1026,6 +1096,7 @@ spec: - Duration - MapKeyRename - CEL + - BranchMap type: string warnings: items: diff --git a/charts/declarative-conversion-operator/crds/terasky.com_xrdconversionconfigs.yaml b/charts/declarative-conversion-operator/crds/terasky.com_xrdconversionconfigs.yaml index b21c4b8..cde299e 100644 --- a/charts/declarative-conversion-operator/crds/terasky.com_xrdconversionconfigs.yaml +++ b/charts/declarative-conversion-operator/crds/terasky.com_xrdconversionconfigs.yaml @@ -113,6 +113,75 @@ spec: - keyField - spokePath type: object + branchMap: + description: |- + BranchMapParams maps the branches of a union-typed field between hub and + spoke — the "one of s3, gcs or azure" shape mature platform APIs express + with `oneOf`. + + The active branch is identified by **which branch property is present**, + not by validating the object against each branch schema. That is what a + union looks like in a legal CRD: the apiserver requires every property + named inside a `oneOf` to also be declared in the parent's own + properties, so a union is a set of declared, mutually-exclusive fields. + + No branch set, or more than one, is a hard runtime conversion error. + That is oneOf's contract — exactly one — and not anyOf's, which permits + overlap. BranchMap fits an anyOf whose branches are mutually exclusive + in practice; one that genuinely allows two at once has no single + correspondence to map, and is better handled with ordinary rules over + the individual branch properties. + properties: + branches: + items: + description: BranchMapping is one branch correspondence. + properties: + hubBranch: + description: |- + HubBranch and SpokeBranch are property names inside the two union + objects. Both must be declared properties. + type: string + hubDiscriminatorValue: + description: |- + HubDiscriminatorValue and SpokeDiscriminatorValue are used only when + BranchMapParams.Discriminator is set. Empty means the branch's own + name, which is the common case. + type: string + rules: + description: |- + Rules apply to this branch pair, with paths relative to the branch + itself — `bucket`, not `spec.backup.s3.bucket`. The same scoping + ForEach gives an array element. + x-kubernetes-preserve-unknown-fields: true + spokeBranch: + type: string + spokeDiscriminatorValue: + type: string + required: + - hubBranch + - spokeBranch + type: object + minItems: 1 + type: array + discriminator: + description: |- + Discriminator optionally names a sibling property whose value also + identifies the branch (`backend: s3`). It is remapped alongside the + branch, so hub and spoke may spell their branch names differently. + It must be a declared property of both unions. + type: string + hubPath: + description: |- + HubPath and SpokePath are the union-typed OBJECT on each side, not a + branch within it. + type: string + spokePath: + type: string + required: + - branches + - hubPath + - spokePath + type: object cel: description: |- CELParams is an always-lossy escape hatch. Both expressions receive the @@ -705,6 +774,7 @@ spec: - Duration - MapKeyRename - CEL + - BranchMap type: string toAnnotation: description: ToMetadataParams backs both ToAnnotation @@ -1072,6 +1142,7 @@ spec: - Duration - MapKeyRename - CEL + - BranchMap type: string warnings: items: diff --git a/charts/declarative-conversion-operator/files/dashboards/conversion-stability.json b/charts/declarative-conversion-operator/files/dashboards/conversion-stability.json index 5ed4c5c..1c4d4b0 100644 --- a/charts/declarative-conversion-operator/files/dashboards/conversion-stability.json +++ b/charts/declarative-conversion-operator/files/dashboards/conversion-stability.json @@ -986,7 +986,7 @@ "type": "timeseries", "gridPos": { "h": 8, - "w": 12, + "w": 8, "x": 0, "y": 27 }, @@ -1033,8 +1033,8 @@ "type": "timeseries", "gridPos": { "h": 8, - "w": 12, - "x": 12, + "w": 8, + "x": 8, "y": 27 }, "datasource": { @@ -1073,6 +1073,58 @@ ], "description": "Mean objects inside each ConversionReview. List/watch conversion drives this up; Get stays near 1. High batch size plus high p99 often means a large List hitting the webhook." }, + { + "id": 35, + "title": "Conversion route mix", + "type": "timeseries", + "gridPos": { + "h": 8, + "w": 8, + "x": 16, + "y": 27 + }, + "datasource": { + "type": "prometheus", + "uid": "${datasource}" + }, + "fieldConfig": { + "defaults": { + "custom": { + "fillOpacity": 10, + "lineWidth": 1, + "showPoints": "never", + "spanNulls": true, + "stacking": { + "mode": "normal", + "group": "A" + } + }, + "unit": "percentunit", + "min": 0 + }, + "overrides": [] + }, + "options": { + "legend": { + "displayMode": "list", + "placement": "bottom", + "showLegend": true, + "calcs": [] + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, + "targets": [ + { + "expr": "sum by (route) (rate(dco_webhook_conversion_objects_total[5m])) / ignoring(route) group_left sum(rate(dco_webhook_conversion_objects_total[5m]))", + "legendFormat": "{{route}}", + "refId": "A" + } + ], + "description": "Share of conversions by route shape. The version pair above cannot answer this on its own because which version is the hub is a per-target fact rather than a label. A material spoke_to_spoke share means clients at two different non-hub versions are reading the same resources; each such conversion runs two hops through the hub, so a direct plan could save at most one of them \u2014 about half the cost where the two hops cost about the same. See docs/operations/capacity.md for why direct spoke-to-spoke plans are deliberately not built." + }, { "id": 40, "title": "Latency", diff --git a/config/crd/bases/terasky.com_crdconversionconfigs.yaml b/config/crd/bases/terasky.com_crdconversionconfigs.yaml index 5f29d97..12c6e2a 100644 --- a/config/crd/bases/terasky.com_crdconversionconfigs.yaml +++ b/config/crd/bases/terasky.com_crdconversionconfigs.yaml @@ -113,6 +113,75 @@ spec: - keyField - spokePath type: object + branchMap: + description: |- + BranchMapParams maps the branches of a union-typed field between hub and + spoke — the "one of s3, gcs or azure" shape mature platform APIs express + with `oneOf`. + + The active branch is identified by **which branch property is present**, + not by validating the object against each branch schema. That is what a + union looks like in a legal CRD: the apiserver requires every property + named inside a `oneOf` to also be declared in the parent's own + properties, so a union is a set of declared, mutually-exclusive fields. + + No branch set, or more than one, is a hard runtime conversion error. + That is oneOf's contract — exactly one — and not anyOf's, which permits + overlap. BranchMap fits an anyOf whose branches are mutually exclusive + in practice; one that genuinely allows two at once has no single + correspondence to map, and is better handled with ordinary rules over + the individual branch properties. + properties: + branches: + items: + description: BranchMapping is one branch correspondence. + properties: + hubBranch: + description: |- + HubBranch and SpokeBranch are property names inside the two union + objects. Both must be declared properties. + type: string + hubDiscriminatorValue: + description: |- + HubDiscriminatorValue and SpokeDiscriminatorValue are used only when + BranchMapParams.Discriminator is set. Empty means the branch's own + name, which is the common case. + type: string + rules: + description: |- + Rules apply to this branch pair, with paths relative to the branch + itself — `bucket`, not `spec.backup.s3.bucket`. The same scoping + ForEach gives an array element. + x-kubernetes-preserve-unknown-fields: true + spokeBranch: + type: string + spokeDiscriminatorValue: + type: string + required: + - hubBranch + - spokeBranch + type: object + minItems: 1 + type: array + discriminator: + description: |- + Discriminator optionally names a sibling property whose value also + identifies the branch (`backend: s3`). It is remapped alongside the + branch, so hub and spoke may spell their branch names differently. + It must be a declared property of both unions. + type: string + hubPath: + description: |- + HubPath and SpokePath are the union-typed OBJECT on each side, not a + branch within it. + type: string + spokePath: + type: string + required: + - branches + - hubPath + - spokePath + type: object cel: description: |- CELParams is an always-lossy escape hatch. Both expressions receive the @@ -705,6 +774,7 @@ spec: - Duration - MapKeyRename - CEL + - BranchMap type: string toAnnotation: description: ToMetadataParams backs both ToAnnotation @@ -1026,6 +1096,7 @@ spec: - Duration - MapKeyRename - CEL + - BranchMap type: string warnings: items: diff --git a/config/crd/bases/terasky.com_xrdconversionconfigs.yaml b/config/crd/bases/terasky.com_xrdconversionconfigs.yaml index b21c4b8..cde299e 100644 --- a/config/crd/bases/terasky.com_xrdconversionconfigs.yaml +++ b/config/crd/bases/terasky.com_xrdconversionconfigs.yaml @@ -113,6 +113,75 @@ spec: - keyField - spokePath type: object + branchMap: + description: |- + BranchMapParams maps the branches of a union-typed field between hub and + spoke — the "one of s3, gcs or azure" shape mature platform APIs express + with `oneOf`. + + The active branch is identified by **which branch property is present**, + not by validating the object against each branch schema. That is what a + union looks like in a legal CRD: the apiserver requires every property + named inside a `oneOf` to also be declared in the parent's own + properties, so a union is a set of declared, mutually-exclusive fields. + + No branch set, or more than one, is a hard runtime conversion error. + That is oneOf's contract — exactly one — and not anyOf's, which permits + overlap. BranchMap fits an anyOf whose branches are mutually exclusive + in practice; one that genuinely allows two at once has no single + correspondence to map, and is better handled with ordinary rules over + the individual branch properties. + properties: + branches: + items: + description: BranchMapping is one branch correspondence. + properties: + hubBranch: + description: |- + HubBranch and SpokeBranch are property names inside the two union + objects. Both must be declared properties. + type: string + hubDiscriminatorValue: + description: |- + HubDiscriminatorValue and SpokeDiscriminatorValue are used only when + BranchMapParams.Discriminator is set. Empty means the branch's own + name, which is the common case. + type: string + rules: + description: |- + Rules apply to this branch pair, with paths relative to the branch + itself — `bucket`, not `spec.backup.s3.bucket`. The same scoping + ForEach gives an array element. + x-kubernetes-preserve-unknown-fields: true + spokeBranch: + type: string + spokeDiscriminatorValue: + type: string + required: + - hubBranch + - spokeBranch + type: object + minItems: 1 + type: array + discriminator: + description: |- + Discriminator optionally names a sibling property whose value also + identifies the branch (`backend: s3`). It is remapped alongside the + branch, so hub and spoke may spell their branch names differently. + It must be a declared property of both unions. + type: string + hubPath: + description: |- + HubPath and SpokePath are the union-typed OBJECT on each side, not a + branch within it. + type: string + spokePath: + type: string + required: + - branches + - hubPath + - spokePath + type: object cel: description: |- CELParams is an always-lossy escape hatch. Both expressions receive the @@ -705,6 +774,7 @@ spec: - Duration - MapKeyRename - CEL + - BranchMap type: string toAnnotation: description: ToMetadataParams backs both ToAnnotation @@ -1072,6 +1142,7 @@ spec: - Duration - MapKeyRename - CEL + - BranchMap type: string warnings: items: diff --git a/docs/architecture.md b/docs/architecture.md index 31531ee..5de998e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -61,6 +61,22 @@ type SchemaSource interface { `pkg/xrdadapter` is the package that knows Crossplane XRDs exist — it implements `SchemaSource` by reading an XRD's `spec.versions[]`. `pkg/crdadapter` is its sibling for plain native `CustomResourceDefinition`s, reading `spec.versions[].{name,served,storage,schema}` directly (CRDs already use the exact vendored Go types this package needs, so no unstructured conversion is required the way it is for Crossplane). Neither adapter changes anything about `pkg/engine` itself — that's the point of the seam. +**Schemas are normalised once, at the seam.** Before anything flattens a +schema, `NormalizeSchema` folds each `allOf` branch into its parent and +resolves local `#/...` references, so `flattenSchema`, every resolver, the +leftover-field scan and the passthrough tree all see one ordinary, +junctor-free shape and none of them has to know those constructs existed. +The result is an analysis artifact and is never written back to a cluster, +which is what lets the merge keep exactly what the engine reads — the field +set, types, required-ness, enum vocabularies, opacity — and drop the value +validations it never looks at. + +That the merge cannot *discover* a field is a fact about CRDs, not an +assumption: the apiserver requires every property named inside a junctor to +be declared outside it as well. `pkg/engine/structural_facts_test.go` pins +that, and four related facts, against the apiserver's own validator, so the +engine's model cannot drift away from what a cluster actually accepts. + **Two entry points, both operating on a precompiled `Plan`:** - `Compile(rules, hubSchema, spokeSchema) (*Plan, []Diagnostic, error)` — flattens both schemas to leaf paths, resolves every rule's declared path(s) against them, computes per-rule, per-direction losslessness, and fails the whole compile if any hub or spoke leaf path is left unclaimed and isn't structurally identical on both sides. A `Plan` only ever comes out of a successful compile with zero errors. diff --git a/docs/cli.md b/docs/cli.md index b5c0b7d..b416813 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -188,7 +188,7 @@ fuzz-1 (conversion) v3 → v2 error jsonPatch: apply: move operation does no ``` That example is not hypothetical: it is what `--fuzz` reports against this -repository's own full-coverage fixture, which exercises all 29 strategies. +repository's own full-coverage fixture, which exercises all 30 strategies. Four classes come out of it. Three are the same shape — a rule that assumes an optional field is present — and the fourth is a schema that admits a lexical form its rule cannot represent: diff --git a/docs/examples/index.md b/docs/examples/index.md index 0eb9bd7..2cc419a 100644 --- a/docs/examples/index.md +++ b/docs/examples/index.md @@ -1,7 +1,7 @@ # Examples The [`examples/`](https://github.com/terasky-oss/declarative-conversion-operator/tree/main/examples) -directory in the repository holds five self-contained conversion stories, +directory in the repository holds six self-contained conversion stories, smallest first. Each one is a directory with a schema (an XRD or a CRD), the conversion config for it, sample objects at every served version, and a README explaining the scenario — everything `convctl` needs to validate and test the @@ -17,6 +17,7 @@ isolation, these show a complete, runnable config you can copy and adapt. | [`field-rename/`](https://github.com/terasky-oss/declarative-conversion-operator/tree/main/examples/field-rename) | One field was renamed between two versions — the smallest useful config, and a demonstration of why identical fields need no rule. | [`FieldRename`](../strategies/field-rename.md) | | [`enum-remap/`](https://github.com/terasky-oss/declarative-conversion-operator/tree/main/examples/enum-remap) | The same field's allowed values were abbreviated (`Large` → `L`). Shows which mistakes `validate` catches versus which only `test` catches. | [`EnumRemap`](../strategies/enum-remap.md) | | [`for-each/`](https://github.com/terasky-oss/declarative-conversion-operator/tree/main/examples/for-each) | Every element of an array changed shape, with nested rules scoped to one element. | [`ForEach`](../strategies/for-each.md) | +| [`branch-map/`](https://github.com/terasky-oss/declarative-conversion-operator/tree/main/examples/branch-map) | A `oneOf` union field — branch names, discriminator values and inner fields all spelled differently on the two versions. Shows why a union's branches are ordinary declared fields. | [`BranchMap`](../strategies/branch-map.md), [`FieldRename`](../strategies/field-rename.md) | | [`crossplane-xr-multiversion/`](https://github.com/terasky-oss/declarative-conversion-operator/tree/main/examples/crossplane-xr-multiversion) | Staged Crossplane XR lifecycle: one-version XRD + ConfigMap Composition, add a spoke, promote the hub (new Composition + retarget `compositionRef`), add `v3`, promote `v3` as the standard, deprecate `v1` (including `convctl migrate-storage` and dropping the version block). GitOps alternative: [`gitops/`](https://github.com/terasky-oss/declarative-conversion-operator/tree/main/examples/crossplane-xr-multiversion/gitops) + [`convctl generate kyverno`](../cli.md#convctl-generate-kyverno) (`--gitops-engine simulate\|flux\|argo`). | [`FieldRename`](../strategies/field-rename.md) — see the [lifecycle walkthrough](xr-lifecycle.md) | | [`native-crd/`](https://github.com/terasky-oss/declarative-conversion-operator/tree/main/examples/native-crd) | The same model against a plain Kubernetes CRD, with no Crossplane anywhere. | [`FieldRename`](../strategies/field-rename.md), [`Delete`](../strategies/delete.md) | diff --git a/docs/examples/kitchen-sink.md b/docs/examples/kitchen-sink.md index 0c1d8b2..cd13242 100644 --- a/docs/examples/kitchen-sink.md +++ b/docs/examples/kitchen-sink.md @@ -1,9 +1,9 @@ # Kitchen sink: every strategy at once [`internal/cli/testdata/full/`](https://github.com/terasky-oss/declarative-conversion-operator/tree/main/internal/cli/testdata/full) -is the one place in the repository where **all 29 built-in strategies** are +is the one place in the repository where **all 30 built-in strategies** are exercised against a single schema: a three-version XRD with a hub and two -spokes, 30 rules covering all 29 strategies, plus sample objects at every +spokes, 31 rules covering all 30 strategies, plus sample objects at every version. It is the fixture the CLI's own end-to-end tests and the e2e suite run against, so it is correct by construction — if a strategy's YAML shape ever changed, this fixture @@ -19,7 +19,7 @@ gallery](index.md) instead — those are written to be read top to bottom. | File | What it is | |---|---| | [`xrd.yaml`](https://github.com/terasky-oss/declarative-conversion-operator/blob/main/internal/cli/testdata/full/xrd.yaml) | `xwidgets.example.org` with three served versions: `v3` (hub, `referenceable: true`), `v2`, and `v1`. | -| [`config.yaml`](https://github.com/terasky-oss/declarative-conversion-operator/blob/main/internal/cli/testdata/full/config.yaml) | The `XRDConversionConfig`: 14 rules for the `v2` spoke, 16 for `v1`, 30 in total covering 29 distinct strategies. | +| [`config.yaml`](https://github.com/terasky-oss/declarative-conversion-operator/blob/main/internal/cli/testdata/full/config.yaml) | The `XRDConversionConfig`: 14 rules for the `v2` spoke, 17 for `v1`, 31 in total covering 30 distinct strategies. | | [`config-norules.yaml`](https://github.com/terasky-oss/declarative-conversion-operator/blob/main/internal/cli/testdata/full/config-norules.yaml) | The same config with every rule stripped — the "new spoke version, no mapping yet" starting point for `convctl suggest`. | | [`samples/`](https://github.com/terasky-oss/declarative-conversion-operator/tree/main/internal/cli/testdata/full/samples) | One object per version: `hub-v3.yaml`, `spoke-v2.yaml`, `spoke-v1.yaml`. | @@ -58,7 +58,7 @@ Two more things the report shows that are easy to miss: - **Spoke-to-spoke paths match rules from both spokes.** `v1→v2` lists `v2:` and `v1:` rules because every spoke-to-spoke conversion routes through the hub — two conversions, exactly as in a live cluster. -- **`RULE COVERAGE` lists all 30 rules with a match count.** A rule no sample +- **`RULE COVERAGE` lists all 31 rules with a match count.** A rule no sample exercised would show up here as a warning, which `--strict` (or `--fail-on warn`) escalates to a failure. @@ -88,7 +88,7 @@ Rule numbers are the indices the `convctl test` report prints | 12 | [`NumericScale`](../strategies/numeric-scale.md) | `spec.memoryMB` ⇄ `spec.memoryGB`, `factor: 1024` — lossy on the integer side | | 13 | [`ListJoin`](../strategies/list-join-split.md) | `spec.dnsServers` array ⇄ `spec.dnsServersCSV` string | -### `v1` spoke — 16 rules +### `v1` spoke — 17 rules | # | Strategy | What it maps here | |---|---|---| @@ -108,6 +108,7 @@ Rule numbers are the indices the `convctl test` report prints | 13 | [`Duration`](../strategies/duration.md) | `spec.timeout` duration string ⇄ `spec.timeoutSeconds` — lossy on the string side | | 14 | [`MapKeyRename`](../strategies/map-key-rename.md) | `spec.extraLabels`: rename `app` ⇄ `application`, other keys pass through | | 15 | [`CEL`](../strategies/cel.md) | `spec.packed` ⇄ `spec.bitHigh` + `spec.bitLow` (always lossy) | +| 16 | [`BranchMap`](../strategies/branch-map.md) | `spec.store` union: `s3`/`gcs` ⇄ `objectStore`/`googleStore`, discriminator remapped | ## What the schema is quietly demonstrating @@ -117,7 +118,7 @@ alongside the config explains two things no single strategy page can: - **Fields with an identical shape on both sides need no rule.** Each spoke mirrors the fields *the other* spoke has rules for, keeping them byte-identical to the hub — and they are covered automatically. That is why - 30 rules are enough for a schema this wide, and why fail-closed coverage + 31 rules are enough for a schema this wide, and why fail-closed coverage isn't as noisy in practice as it sounds. - **`status` is not special.** `v2` maps `status.phase` → `status.state` with an ordinary `FieldRename`, while `v1` leaves `status` untouched because its shape @@ -148,7 +149,7 @@ go run ./cmd/convctl suggest \ ``` And `convctl diff` between the two configs, which reads as "what did writing all -30 rules actually accomplish?" — every field that stopped being uncovered, per +31 rules actually accomplish?" — every field that stopped being uncovered, per spoke: ```console diff --git a/docs/limitations.md b/docs/limitations.md index 7f809b3..c559696 100755 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -16,7 +16,10 @@ This page is deliberately blunt about what the operator does *not* do today, so - **`forEach` nesting is capped at depth 2.** A `forEach` may wrap another `forEach` for arrays-of-arrays; a third level is rejected at compile and admission time (not silently truncated). This bounds the CRD schema recursion the engine has to reason about. See [For Each](strategies/for-each.md). - **`forEach` requires strict positional correspondence** between the hub and spoke arrays — same length, same order. If both the hub and spoke item paths are present on the input as arrays of different lengths, conversion fails with a hard runtime error (it does not silently coerce to the source length). When the destination path is absent, the output array is sized from the source alone. - **Free-form maps (`additionalProperties: true`) and preserve-unknown-fields subtrees are opaque, all-or-nothing units.** A rule must claim the whole subtree; the engine does not reason about individual keys inside one. If you need field-level control over part of a free-form map, model those fields explicitly in the schema instead. -- **`oneOf` / `anyOf` / `allOf` / `$ref` nodes are treated opaquely.** Analyze does not flatten through those JSON Schema constructs. An uncovered leaf sitting inside one is reported with the construct name in the diagnostic (not a generic "uncovered field" message). Transforming across a `oneOf` branch is out of scope. +- **`oneOf` / `anyOf` over declared properties is no longer opaque; [`branchMap`](strategies/branch-map.md) maps *mutually exclusive* branches.** In a CRD the apiserver requires every property named inside a junctor to also be declared in the parent's own `properties`, so the branches of a union are ordinary addressable leaves, which the engine now flattens. What remains opaque is a union on a node the engine cannot classify on its own terms — the int-or-string shape `anyOf: [{type: integer}, {type: string}]` is the case that occurs, and there the union really is all the node says. An uncovered leaf inside one is still reported with the construct name in the diagnostic rather than a generic "uncovered field". + + **`branchMap` requires exactly one branch to be set, which is `oneOf`'s contract and not `anyOf`'s.** `oneOf` means exactly one subschema matches; `anyOf` means at least one, and permits overlap. `branchMap` identifies the active branch by which branch property is present and treats zero or several as a hard conversion error in both directions — so it fits an `anyOf` only where the branches happen to be mutually exclusive in practice. An `anyOf` that genuinely allows two branches at once has no single correspondence to map, and objects that exercise the overlap will fail conversion even though the schema admits them. Model those with ordinary rules over the individual properties instead. Mapping two hub branches onto one spoke branch is a different matter: it is expressible, lossy coming back, and needs `acknowledgeLossy`. +- **`allOf` is merged, and `$ref` is resolved, before anything else looks at a schema.** A normalisation pass folds each `allOf` branch into its parent and resolves local `#/...` references, so a field declared alongside one of those constructs is an ordinary addressable field rather than part of an opaque blob. Two things it will not do: a **remote** `$ref` is rejected rather than fetched — following one would make analysis depend on the network — and **conflicting** `allOf` constraints (two types, or two enums with no value in common) are a compile error naming both sides rather than a silent last-writer-wins. A `$ref` cycle is reported, never recursed into. The merge keeps everything the engine reasons about — the field set, types, required-ness, enum vocabularies, opacity — and drops the value validations it never reads, such as patterns and bounds; the normalised schema is an analysis artifact and is never written back to a cluster. In a *CRD* none of this changes what is addressable, because the apiserver already requires every property named inside a junctor to be declared outside it too (pinned against the apiserver's own validator in [`pkg/engine/structural_facts_test.go`](https://github.com/TeraSky-OSS/declarative-conversion-operator/blob/main/pkg/engine/structural_facts_test.go)) — what changes is that the engine stops hiding it. - **`arrayToMapByKey` requires unique key values.** A duplicate `keyField` value across array elements is a runtime conversion error, not a silent overwrite. - **Some strategies can never be statically proven lossless**, because the engine can't reason about arbitrary regexes, templates, JSON Patch documents, or CEL at compile time: `jsonPatch`, `scalarToFields`/`fieldsToScalar` (unless you set `losslessOverride: true` and back that claim up with `convctl test` against representative data), `cel` (always lossy — no `losslessOverride`), and `mapToArrayByKey`/round-tripping through a sorted array (always lossy — see [Array ⇄ Map by Key](strategies/array-map-key.md)). @@ -35,7 +38,7 @@ This page is deliberately blunt about what the operator does *not* do today, so - **A handover is verified against Lease reports, not against pod identity.** The operator counts the live Leases an instance's replicas publish and requires at least as many as the Deployment reports ready. It does not check that each Lease belongs to a *currently* ready pod — that would need a Pod informer in the operator, and the reconcile loop deliberately depends on no pod-level state. The window this leaves is small and bounded from two sides: a Lease is owned by its pod, so it is garbage-collected when the pod goes, and one whose `renewTime` stops advancing is discounted after 90 seconds. Inside that window a just-replaced replica's Lease could stand in for its successor, and a move could be approved a few seconds before the new replica has compiled the target. - **A target stays on its old webhook server for 30 seconds after a move.** The apiserver refreshes a CRD's conversion configuration asynchronously after the write that changed it, so a replica that dropped its plan the instant the target stopped naming it would answer the in-flight calls with a 503. The drain is fixed, not configurable: it is short enough not to matter and long enough to cover the propagation, and the only cost of it being longer than necessary is one compiled plan (~18 KiB) held on a replica that no longer needs it. See [Moving a target between instances](architecture.md#moving-a-target-between-instances). - **Automatic sharding balances by count, not by cost.** Rendezvous hashing distributes *targets* evenly across the pool (weighted, if you set weights); it knows nothing about how large each target's schema is or how much traffic it takes. A fleet with one enormous XRD and a hundred small ones can land the enormous one anywhere. Use `spec.webhookServerRef` to pin the outliers and let sharding spread the rest, or set `spec.sharding.weight` to bias the split. -- **Spoke-to-spoke conversions always route through the hub** — two `Convert` calls, never a direct spoke-to-spoke path. This keeps compilation cost linear in the number of spoke versions. A 1000-element `forEach` spoke-to-spoke convert is ~2.3× a single hop and still under 1 ms ([Capacity planning](operations/capacity.md)); shortcut plans are not implemented. +- **Spoke-to-spoke conversions always route through the hub** — two `Convert` calls, never a direct spoke-to-spoke path. This keeps compilation cost linear in the number of spoke versions rather than quadratic in spoke pairs. A spoke-to-spoke conversion costs exactly `A→hub` plus `hub→B`, so a direct plan could save **at most one hop**. Measured on a `forEach` fixture across object sizes from 0 to 1000 elements, where the two hops cost about the same, that is a flat **~2×** — 0.6 µs extra on a small object, 330 µs on the 1000-element worst case, still under 1 ms. Direct shortcut plans were evaluated in Phase 16 and **deliberately not built**: the saving is one hop — microseconds, inside a request that has already paid milliseconds of apiserver overhead — while the cost is `O(N²)` compiled plans and a third mapping to keep consistent with the two it shortcuts. The full reasoning, the per-size table and the `route` label for measuring how much spoke-to-spoke traffic a cluster actually has are in [Capacity planning](operations/capacity.md#spoke-to-spoke-vs-hub-hop). - **`--request-timeout` bounds a batch, not a single object.** The conversion loop checks the deadline before each object and again before reporting success, so a large batch cannot run indefinitely. It cannot interrupt one object mid-conversion: `engine.Convert`'s operations take no context, so a `forEach` over a very large array runs to completion first. `--max-request-bytes` is what bounds how large that array can be. Plumbing a context through every operation is the real fix and is not done. - **The webhook-server's cache is built from the feature flags, not from what is on the cluster.** `--enable-xrd-support=false` is what keeps `CompositeResourceDefinition` out of the informer cache entirely. controller-runtime resolves every cached kind through the RESTMapper when the manager is constructed, so on a cluster without Crossplane a replica told to support XRDs does not degrade — it exits at startup with `no matches for kind "CompositeResourceDefinition"`. That is deliberate and matches the manager's own behaviour, but it means the flags must match the cluster rather than describing a preference. - **Required-field analysis treats a required source leaf as present whenever its own parent object is.** Asking "does the rule set always produce this required destination field?" needs the mirror question about the source, and the analysis answers it one level deep: `spec.network.cidr` marked required inside an **optional** `spec.network` counts as guaranteed. Where an optional ancestor gates it, a genuinely conditional source can therefore be read as unconditional, and that case goes unreported. Tightening the rule to "every ancestor must itself be required" is not the fix: nothing under `spec` is ever listed in a CRD root's `required`, so every ordinary rename would be flagged. The sound version has to model whether the destination parent's creation is coupled to the same source path — a rule that creates `spec.network` *only* by writing `cidr` into it is correct, and a naive check calls it broken. Until then this is a missed diagnostic rather than a wrong one, and `--validate-output` and `--fuzz` are the empirical checks that cover it. This is about the **source** side only: on the destination side an optional ancestor is handled — a required field whose containing object is produced whole by a rule, rather than written into, is reported as *unprovable* rather than skipped. diff --git a/docs/observability.md b/docs/observability.md index 9205281..1b3d48e 100755 --- a/docs/observability.md +++ b/docs/observability.md @@ -36,11 +36,11 @@ Emitted by each ConversionWebhookServer replica (dedicated registry in |---|---|---|---| | `dco_webhook_conversion_review_duration_seconds` | Histogram | `target`, `direction`, `result` | End-to-end ConversionReview latency | | `dco_webhook_conversion_review_requests_total` | Counter | `target`, `result` | ConversionReview requests handled | -| `dco_webhook_conversion_objects_total` | Counter | `target`, `from_version`, `to_version`, `result` | Individual objects converted inside reviews | +| `dco_webhook_conversion_objects_total` | Counter | `target`, `from_version`, `to_version`, `route`, `result` | Individual objects converted inside reviews. `route` classifies the conversion by shape — `hub_to_spoke`, `spoke_to_hub`, `spoke_to_spoke`, `identity` — which the version pair cannot: which version is the hub is a per-target fact, not a label. It is a function of labels the series already carries, so it adds no cardinality | | `dco_webhook_conversion_object_duration_seconds` | Histogram | `target`, `direction`, `result` | Per-object conversion latency. Prefer this over the review histogram for anything sliced by `direction` — see the note below | | `dco_webhook_conversion_batch_size` | Histogram | `target` | Objects carried by one ConversionReview. The input for sizing `--max-request-bytes` | | `dco_webhook_conversion_panics_total` | Counter | `target` | Panics recovered while serving a review. Always a bug in this operator; alert on any increase | -| `dco_webhook_lossy_conversion_total` | Counter | `target`, `direction` | Conversions on a direction statically known to be lossy | +| `dco_webhook_lossy_conversion_total` | Counter | `target`, `direction` | Conversions on a direction statically known to be lossy. A spoke-to-spoke conversion passes through the hub and is counted once for each of its two hops that is lossy | | `dco_webhook_registry_size` | Gauge | — | Registry entries on this replica (includes error-only placeholders) | | `dco_webhook_registry_entry_loaded` | Gauge | `target` | `1` if this replica has a compiled, servable plan for that target; `0` if error-only | | `dco_webhook_registry_last_reload_timestamp_seconds` | Gauge | `target` | Unix time of last successful compile | @@ -98,8 +98,16 @@ sum by (target) (rate(dco_webhook_conversion_review_requests_total{result="error / sum by (target) (rate(dco_webhook_conversion_review_requests_total[5m])) -# Lossy conversion rate +# Lossy conversion rate. A spoke-to-spoke conversion is two hops and is +# counted once per lossy hop, so this can exceed the object rate. sum by (target, direction) (rate(dco_webhook_lossy_conversion_total[5m])) + +# Share of traffic by route shape. This is what says whether direct +# spoke-to-spoke plans would be worth building for your cluster — see +# Capacity planning for why the shipped answer is "no". +sum by (route) (rate(dco_webhook_conversion_objects_total[5m])) + / ignoring(route) group_left +sum(rate(dco_webhook_conversion_objects_total[5m])) ``` --- diff --git a/docs/operations/capacity.md b/docs/operations/capacity.md index d0e4ed6..457d46e 100755 --- a/docs/operations/capacity.md +++ b/docs/operations/capacity.md @@ -61,19 +61,100 @@ patch documents over many tiny ones. ## Spoke-to-spoke vs hub hop Router always goes spoke A → hub → spoke B (`O(N)` compiled plans, never -pairwise). For a 1000-element `forEach` object: +pairwise). Phase 9 measured that at a single worst-case point; Phase 16 +swept it, because one point cannot show whether the ratio holds +(`BenchmarkRouter_SpokeToSpoke_vs_HubHop`, `forEach` over an array of +volumes, medians of eight runs): + +| `forEach` elements | hub → spoke | spoke → spoke | ratio | allocations | +|---:|---:|---:|---:|---| +| 0 | 0.4 µs | 1.0 µs | 2.4× | 5 → 10 | +| 1 | 0.8 µs | 1.8 µs | 2.2× | 9 → 18 | +| 5 | 2.2 µs | 4.9 µs | 2.2× | 21 → 42 | +| 10 | 4.2 µs | 8.4 µs | 2.0× | 36 → 72 | +| 100 | 38 µs | 79 µs | 2.1× | 306 → 612 | +| 1000 | 378 µs | 705 µs | 1.9× | 3006 → 6012 | + +**The ratio is a flat ~2× across four orders of magnitude**, and the +allocation counts are exactly 2× at every size. There is no fixed per-call +overhead that a direct plan would remove, and nothing that grows +super-linearly. + +Be precise about what generalises from that, because the table is one +fixture — a `forEach` over an array of volumes — and a different strategy +mix would put different numbers in it. What does not depend on the fixture +is the identity underneath: + +> `cost(A → B) = cost(A → hub) + cost(hub → B)`, exactly, because that is +> literally what `Router.Convert` executes. + +So the most a direct plan could ever save is **one hop**, whatever a hop +costs for your schemas. The ~2× is what that identity becomes when the two +hops cost about the same, which is the common case and is what this fixture +measures; a spoke whose rules are much more expensive in one direction +would shift the ratio without changing the conclusion, because the saving +is still bounded by a single hop. + +What that hop costs for a given config is already measurable without a new +benchmark: `dco_webhook_conversion_object_duration_seconds` histograms it +per direction, on live traffic. The per-strategy table further down gives +the offline version. + +### Measuring how much spoke-to-spoke traffic you actually have + +`dco_webhook_conversion_objects_total` carries a `route` label — +`hub_to_spoke`, `spoke_to_hub`, `spoke_to_spoke`, `identity`. It exists +because `from_version` and `to_version` cannot answer the question on their +own: deciding whether a pair is spoke-to-spoke needs to know which version +is the hub, which is a per-target fact rather than a label, so a PromQL +query would have to hard-code every target's hub version and be re-edited +whenever a hub is promoted. -| Route | ns/op | vs hub→spoke | -|---|---:|---| -| hub → spoke | 328k | 1.0× | -| spoke → spoke | 765k | 2.3× | +```promql +sum by (route) (rate(dco_webhook_conversion_objects_total[5m])) + / ignoring(route) group_left +sum(rate(dco_webhook_conversion_objects_total[5m])) +``` -Spoke-to-spoke is essentially two Converts. Even in this worst-case array -shape it stays under 1 ms. The apiserver stores at the hub version, so -spoke-to-spoke is rare in production. Direct shortcut plans were evaluated -and rejected: they would push compilation toward `O(N²)` spoke pairs for a -gain that does not show up under the 1s p99 ConversionReview alert. Hub-and- -spoke remains the only routing mode. +The "Conversion route mix" panel on the shipped **Conversion stability** +dashboard is this query. Spoke-to-spoke only happens when a client reads at +one non-hub version an object that a client at a *different* non-hub +version wrote — in practice, when a controller and a human prefer different +legacy versions of the same resource at the same time. + +### The recommendation: no direct plans + +Direct shortcut plans were evaluated and rejected, twice now, and the +reasoning is worth keeping rather than re-deriving: + +- **The saving is bounded by one hop.** In this fixture, a realistic + composite resource is in the 0–10 element rows, where that hop costs + **0.6–4 µs**. A ConversionReview that reaches this webhook has already + paid apiserver admission, TLS and JSON round-trips measured in + milliseconds. Removing 4 µs from that is not observable, let alone under + the 1 s p99 ConversionReview alert. If your own hop latency is orders of + magnitude higher than this — check + `dco_webhook_conversion_object_duration_seconds` before assuming it is — + the arithmetic is worth redoing, but the other two objections below are + not about latency at all. +- **The cost is not bounded.** Pairwise plans are `O(N²)` in served + versions, and each plan is compiled, validated and retained per target — + the memory figures below are per plan. Lazy per-pair compilation would + keep the common case `O(N)` but would move plan compilation onto the + conversion path, where a first request for a new pair pays compilation + latency inside the apiserver's timeout. +- **The correctness surface doubles.** A direct plan is a third mapping to + keep consistent with the two it shortcuts, and any disagreement between + them is a conversion that silently depends on which route it took. + +**Hub-and-spoke remains the only routing mode.** The seam for anyone +revisiting this is `Router.Convert` in +[`pkg/engine/convert.go`](https://github.com/terasky-oss/declarative-conversion-operator/blob/main/pkg/engine/convert.go): +it is the single place that decides the route, it already has both plans in +hand, and a per-pair cache keyed on `(from, to)` would slot in there without +touching any strategy. Bring the `route` panel showing spoke-to-spoke as a +material share of traffic, and an object size in the rows where 2× is worth +paying for. ## Memory: the manager @@ -494,9 +575,9 @@ native-CRD kind cluster, then **generates** a fleet of CRDs and drives real apiserver Get/List (which invoke the conversion webhook) in parallel: - Each CRD has **3 versions** (`v3` storage hub, `v1`/`v2` spokes). -- Each spoke conversion has **3–10 strategies**, assigned so **all 29** +- Each spoke conversion has **3–10 strategies**, assigned so **all 30** built-in strategies appear across the fleet (`2 × targets × strategies-max` - must be ≥ 29). + must be ≥ 30). - Instances are created at `v1`; Get/List run at both spoke versions so the apiserver converts hub↔spoke on every call. - Every CRD is in the `widgets` category: `kubectl get widgets -n dco-scale` @@ -527,7 +608,7 @@ go run ./cmd/scalegen --reset --targets 20 --instances 20 --parallel 16 --qps 10 Same workstation as the microbenchmarks (Intel Core Ultra 9 285HX, WSL2, kindest/node v1.35, one control-plane node). 100 CRDs × 3 versions, 3–10 -strategies per spoke (all 29 used across the fleet), 100 instances created +strategies per spoke (all 30 used across the fleet), 100 instances created at `v1`, then parallel Get/List at both spokes (`PARALLEL=32`, QPS 100 / burst 200). Create of 10,000 objects took **1m38s**. Zero conversion errors. @@ -549,7 +630,7 @@ the serving path. Latest local run, same workstation and kind topology as 100×100 (Intel Core Ultra 9 285HX, WSL2, kindest/node v1.35, one control-plane node). 100 CRDs -× 3 versions, 3–10 strategies per spoke (all 29 used across the fleet), +× 3 versions, 3–10 strategies per spoke (all 30 used across the fleet), 1000 instances created at `v1` (**100,000** objects), then parallel Get/List at both spokes (`PARALLEL=60`, QPS 100 / burst 200). Create of 100,000 objects took **16m38s**. Zero conversion errors. diff --git a/docs/proposals/next-phases.md b/docs/proposals/next-phases.md index f54f74f..47fbdc7 100644 --- a/docs/proposals/next-phases.md +++ b/docs/proposals/next-phases.md @@ -1065,19 +1065,87 @@ apiserver's write path". ## Phase 16 — Engine and strategy expansion +> **Shipped.** All three deliverables landed, and two of them came out +> differently from the way this section describes them: +> +> - **The `$ref`/`allOf` item was reframed by its own investigation, and it +> turned out to be a correctness fix rather than a capability.** Before +> designing anything, the work asserted what apiextensions actually +> accepts — against the apiserver's own validator, in +> `pkg/engine/structural_facts_test.go`, so the engine's model cannot +> drift from what a cluster accepts. One of the five facts decides +> everything: **every property named inside a junctor must also be +> declared outside it.** A junctor in a legal CRD can therefore only +> *constrain* fields the engine already sees; it can never introduce one. +> So flattening through an `allOf` is not a discovery, and marking a node +> opaque because it carried one was not incomplete — it was wrong. A node +> with `allOf: [{required: [bucket]}]` had its entire field set disappear +> because of a constraint. `$ref` is reframed too: it is rejected outright +> in a CRD, so resolving it serves only the offline path, where an +> unresolvable reference is an authoring mistake that deserves a message +> naming the reference rather than an opaque leaf. +> - **The same fact decided how `branchMap` works.** Because a branch is an +> ordinary declared, addressable property, the active branch is identified +> by *which branch property is present* rather than by validating the +> object against each branch schema — structural matching would put a JSON +> Schema validator on the apiserver's admission path to learn what a map +> lookup already knows. It also forced the un-hiding: once a branch's +> leaves are visible, a complete mapping has to claim all of them, so +> `oneOf`/`anyOf` over declared properties stopped being opaque. The +> int-or-string shape, which has no type of its own, still is. +> - **Spoke-to-spoke closed as a documented no, which this section already +> expected.** What it did not expect is how flat the answer is. Swept from +> 0 to 1000 `forEach` elements, the whole two-hop route costs ~2× a +> single hop at *every* size — exactly 2× the allocations and 2× the +> bytes — with no fixed overhead to amortise and nothing super-linear. +> That second hop therefore adds about one hop's worth, not two. The ~2× +> belongs to that fixture, +> whose two hops cost about the same; what does not is the identity under +> it, that a spoke-to-spoke conversion costs exactly `A→hub` plus +> `hub→B`, so a direct plan could save **at most one hop** whatever a hop +> costs. At realistic object sizes that hop is 0.6–4 µs, inside a request +> that has already paid milliseconds of apiserver overhead. The 2.3× +> quoted below was one point on that curve, not a constant. +> +> Two things were added that this section does not mention. A `route` label +> on `dco_webhook_conversion_objects_total`, because the acceptance criteria +> asked to measure real spoke-to-spoke frequency and the existing labels +> cannot: whether a version pair is spoke-to-spoke depends on which version +> is the hub, which is a per-target fact rather than a label. And a fix it +> surfaced — `dco_webhook_lossy_conversion_total` used a hub-or-nothing test, +> so a spoke-to-spoke conversion, which can lose something on each of its two +> hops, counted neither. The traffic class carrying the most loss was +> reporting none. + - ~~**Required-field satisfaction analysis**~~ — **shipped in 12.3**, where it belonged: it converts a production admission failure into a compile-time error, which is the same job as the rest of that deliverable. -- **`oneOf` / `anyOf` branch mapping.** Currently opaque and documented as out - of scope. Union-typed API fields are common in mature XRDs, and a branch-aware - strategy (`branchMap`) would unblock migrations that today need `jsonPatch`. -- **`$ref` / `allOf` flattening** so shared sub-schemas stop being opaque units. -- **Spoke-to-spoke shortcut plans**, if the 2.3× hub-hop cost ever shows up in - a real profile. Listed for completeness; the measured numbers do not justify - it yet. +- ~~**`oneOf` / `anyOf` branch mapping.** Currently opaque and documented as + out of scope.~~ — **shipped in 16.1** as [`branchMap`](../strategies/branch-map.md), + and the premise was half wrong: a union's branches were never out of reach, + only hidden. In a CRD every property named inside a junctor must also be + declared outside it, so a branch is an ordinary addressable field; what + 16.1 added is the *correspondence* between hub and spoke branches, plus a + fix for the engine treating such a node as opaque. `jsonPatch` is no longer + the only way to touch one. +- ~~**`$ref` / `allOf` flattening** so shared sub-schemas stop being opaque + units.~~ — **shipped in 16.2**, as a correctness fix rather than a + capability: a junctor can only constrain fields the engine already sees, so + a node with `allOf: [{required: [bucket]}]` was losing its entire field set + because of a constraint. +- ~~**Spoke-to-spoke shortcut plans**, if the 2.3× hub-hop cost ever shows up + in a real profile.~~ — **investigated in 16.3 and deliberately not built.** + The 2.3× was one point on a curve that turns out to be flat. A + spoke-to-spoke conversion costs exactly `A→hub` plus `hub→B`, so a direct + plan could save **at most one hop** — 0.6–4 µs at realistic object sizes, + inside a request that has already paid milliseconds of apiserver overhead, + against `O(N²)` compiled plans and a third mapping to keep consistent with + the two it shortcuts. The reasoning and the per-size table are in + [Capacity planning](../operations/capacity.md#spoke-to-spoke-vs-hub-hop); + `Router.Convert` stays the seam for anyone revisiting it. - **Strategy additions driven by real migrations only.** The `Strategy` enum and discriminated union were built for this; the discipline of "a real migration - asked for it" is what has kept 29 strategies coherent. + asked for it" is what has kept the strategy set coherent. ## Sequencing diff --git a/docs/roadmap.md b/docs/roadmap.md index dd05ad6..10c670b 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -5,7 +5,7 @@ timeline. Phases already shipped stay listed so the arc is visible; later phases are invitations to [open an issue or PR](https://github.com/terasky-oss/declarative-conversion-operator/issues) if one of them matters to you sooner. -## Shipped (phases 0–15) +## Shipped (phases 0–16) | Phase | Epic | Intent | |---|---|---| @@ -25,10 +25,11 @@ if one of them matters to you sooner. | **13 — CI/CD: official GitHub Actions** | [#133](https://github.com/terasky-oss/declarative-conversion-operator/issues/133) | A supported way to get `convctl` into a pipeline, where before the reference workflow's install step was `exit 1`. First-party composite Actions — `setup-convctl`, which verifies the cosign signature by default and still verifies on a cache hit; `convctl-test`, with a JUnit artifact, a job summary and annotations on the diff; `convctl-diff`, as a sticky PR comment that says "no deltas" rather than vanishing; `convctl-fleet`, one aggregated report across clusters — each exercised by a test workflow whose most important job proves verification *fails* on a tampered archive. CI-native output formats (`github`, `sarif`, `markdown`) built on real source locations, so a finding lands on the line of the config that produced it. `convctl lint` over a whole tree, with unpaired and duplicate configs reported rather than skipped. A published `convctl` image. Bounded `--live` sampling that says plainly when it sampled. `--package`, so a Configuration's XRDs can be tested from a local `.xpkg` before publishing. Homebrew, Scoop, deb and rpm, and a `version` command that reports something a bug report can use. | | **14 — Production readiness** | [#145](https://github.com/terasky-oss/declarative-conversion-operator/issues/145) | Informer caches that scale with the number of conversion configs rather than with the cluster: no Secret informer at all in the manager, label-scoped owned workloads, and a webhook-server cache transform that strips what the engine never reads. Timeouts and a body limit on both HTTP servers, with an oversized body answered as a well-formed failing `ConversionReview`. The panic path preserving the request UID, so its message actually arrives. Per-object conversion metrics, so a mixed-direction batch stops being attributed to whichever object was last. Rollout safety — `preStop`, grace period, spread, `maxUnavailable: 0` — proved by a nightly soak that rolls the webhook-server under load and asserts zero failed **and zero wrong** conversions. A curated `.golangci.yml` with every finding fixed, `govulncheck`/CodeQL/Trivy/Scorecard/Dependabot, a chart `values.schema.json`, `helm-unittest` in place of the CI `grep` block, and a [deprecation policy](deprecation-policy.md). | | **15 — Performance and scale** | [#156](https://github.com/terasky-oss/declarative-conversion-operator/issues/156) | Scaling out stops being a manual assignment exercise: `spec.sharding` distributes unpinned configs across every instance that opts in, by rendezvous hashing, with explicit `webhookServerRef` still winning over everything. The move is the hard part, and it is gated — each replica publishes what it can actually serve into a Lease, `status.servedTargets` is the intersection across them, and the operator does not repoint a target until the destination confirms it — the one exception being an instance that publishes nothing at all for a 30-second grace period, where it proceeds unverified rather than blocking a fleet that structurally cannot report, and says so on the condition. The source keeps serving a target that still names it. Proved by an e2e that reassigns three times under sustained load and asserts zero failed **and zero wrong** conversions. A cold start that is measured (`dco_webhook_initial_sync_duration_seconds`), parallel, and bounded: the health endpoint now comes up *before* the registry sync, so a cold replica is visibly alive while `/readyz` and the conversion endpoint still wait, and a `startupProbe` on `/readyz` turns `periodSeconds × failureThreshold` into a real deadline on the sync — without which a replica wedged mid-sync would stay liveness-healthy and never ready, out of the Service and never restarted. Memory per target published for the first time, which found that compiling churns twenty times what it retains while the Go runtime had no idea a container limit existed: `GOMEMLIMIT` tells it, which roughly halves the cold-start transient — a soft target rather than a ceiling, so the live working set is still the thing to size the limit against. A nightly scale run at a raised envelope, publishing an artifact and failing on a relative regression that it names. And controller-runtime's workqueue depth — the leading indicator of a reconcile backlog — on the shipped dashboard and in two alerts, with `--max-concurrent-reconciles` as the lever it prompts. | +| **16 — Engine and strategy expansion** | [#162](https://github.com/terasky-oss/declarative-conversion-operator/issues/162) | The engine stops hiding fields that were always there. `$ref` and `allOf` are normalised away before analysis, after a check against the apiserver's own validator established the decisive fact: every property named inside a junctor must also be declared outside it, so a junctor in a legal CRD can only constrain fields the engine already sees — which means marking a node opaque because it carried one was not incomplete, it was wrong. `branchMap` maps the branches of a `oneOf` union between versions, identifying the active branch by which branch property is present, remapping an optional discriminator alongside it, and running nested rules scoped to the branch; no branch set or more than one is a hard error in both directions. And the spoke-to-spoke question closed with a measured no: a spoke-to-spoke conversion costs exactly its two hops, so a direct plan could save at most one of them — measured at a flat ~2x across four orders of magnitude of object size, exactly 2x allocations at every size, which is microseconds inside a request that has already paid milliseconds of apiserver overhead, against `O(N²)` plans and a third mapping to keep consistent. A `route` label makes the frequency measurable in a real cluster, and the seam stays documented for anyone revisiting. | ## Proposed next phases -Phases 0–15 are complete — 14 was taken out of order because four of its +Phases 0–16 are complete — 14 was taken out of order because four of its items were concrete defects on the apiserver's write path and did not warrant waiting for a phase, and 13 landed after 12 rather than before it despite the epic's own note to build the road first. Each phase below has an epic with per-deliverable @@ -36,13 +37,15 @@ sub-issues carrying a priority label, a size label, and a full PRD. The detail the review that produced the plan and the `file:line` findings behind each entry — is in [Review and proposed next phases](proposals/next-phases.md). -| Phase | Epic | Intent | -|---|---|---| -| **16 — Engine and strategy expansion** | [#162](https://github.com/terasky-oss/declarative-conversion-operator/issues/162) | `oneOf`/`anyOf` branch mapping, `$ref`/`allOf` flattening, and further strategies driven by real migrations. | +Nothing is scheduled. The next phase will be proposed from real migrations +rather than from the backlog — [open an issue](https://github.com/terasky-oss/declarative-conversion-operator/issues) +with an XRD/CRD shape this engine cannot express and it becomes a candidate. ## Design seams worth knowing - **Strategies stay additive.** A new strategy is a `*Params` type, an `Op`, a compile resolver, webhook validation, a CLI fixture, and a docs page — see [Adding a strategy](contributing/adding-a-strategy.md). +- **A union's branches are ordinary declared fields.** The apiserver requires every property named inside a junctor to also be declared in the parent's own `properties`, so a `oneOf` can only say which of several declared, addressable fields may be set — it can never introduce one. That fact is pinned against the apiserver's own validator in `pkg/engine/structural_facts_test.go`, and it is why `branchMap` identifies a branch by presence rather than by structural matching. +- **Routing is hub-and-spoke, permanently.** Spoke A → hub → spoke B, never a direct plan. The two-hop route costs a flat ~2x a single hop where the two hops cost about the same, measured across object sizes — so a direct plan could save at most one hop; direct plans were evaluated twice and rejected both times ([Capacity planning](operations/capacity.md#spoke-to-spoke-vs-hub-hop)). `Router.Convert` is the seam if that ever changes. - **Observability is chart-optional.** ServiceMonitor / PrometheusRule / Grafana dashboards ship with the chart and stay off unless enabled. - **Crossplane 2.x is the target; 1.x control planes are a non-goal.** The v1 compatibility layer inside 2.x — `scope: LegacyCluster`, claims, connection secrets — is in scope, and phase 11 made it first-class. - **"Applied" and "converting" are different claims.** The operator patching the XRD is not the same as Crossplane having re-rendered the generated CRD with that webhook; `ConversionPropagated` is the condition that distinguishes them ([Limitations](limitations.md), [XRDConversionConfig](configuration/xrdconversionconfig.md#applied-is-not-the-same-as-conversion-works)). diff --git a/docs/strategies/branch-map.md b/docs/strategies/branch-map.md new file mode 100644 index 0000000..8e7ff99 --- /dev/null +++ b/docs/strategies/branch-map.md @@ -0,0 +1,220 @@ +# Branch Map + +## What it does + +Maps the branches of a **union-typed field** — the "one of `s3`, `gcs` or +`azure`" shape mature platform APIs express with `oneOf` — between the hub +and a spoke. It writes the branch that corresponds to whichever one the +input has set, remaps an optional discriminator alongside it, and can run +nested rules scoped to that branch's own subtree. + +## When to use it + +A union-typed field whose **branch names or branch shapes differ between +versions**. + +You do not need it for a union whose branches happen to keep the same names +and shapes — those are ordinary declared fields and ordinary rules reach +them. What `branchMap` adds is the *correspondence*: a declaration of which +hub branch becomes which spoke branch, checked at compile time and enforced +at runtime. + +!!! warning "`oneOf`'s contract, not `anyOf`'s" + `branchMap` requires **exactly one** branch to be set, and treats zero + or several as a hard conversion error. That is what `oneOf` means; it + is not what `anyOf` means, which is *at least* one and permits overlap. + + It fits an `anyOf` whose branches are mutually exclusive in practice. + It does not fit one that genuinely allows two at once: there is no + single correspondence to map, and an object exercising the overlap + would fail conversion even though the schema admits it. Map those + branches with ordinary rules over the individual properties instead — + they are declared fields like any other. + +!!! conditional-lossy "Lossless unless branches collapse" + Mapping each hub branch to a distinct spoke branch is lossless in both + directions, plus whatever the nested rules contribute. Mapping **two + hub branches onto one spoke branch** is lossy coming back — the engine + cannot tell which one it started from — and needs + `acknowledgeLossy: true`, the same verdict `enumRemap` gives a + non-injective mapping. + + A hub branch the rule does not name at all is not a lossiness question: + its fields are left uncovered, which is an ordinary validation error. + Map it, or `delete` it deliberately. + +## What a union looks like in a CRD + +This matters, because it is narrower than general JSON Schema and it is +what the strategy is built against. + +The apiserver requires **every property named inside a `oneOf` to also be +declared in the parent's own `properties`**. So a union is a set of +declared, optional, mutually-exclusive fields, and the `oneOf` only says +which of them may be set. A branch is not a hidden shape — it is a +property, with a path, that ordinary rules could already address. This is +pinned against the apiserver's own validator in +[`pkg/engine/structural_facts_test.go`](https://github.com/TeraSky-OSS/declarative-conversion-operator/blob/main/pkg/engine/structural_facts_test.go). + +That is why the active branch is identified by **which branch property is +present** rather than by validating the object against each branch schema. +Structural matching would mean running a full JSON Schema validator on the +apiserver's admission path to learn something a map lookup already knows. + +## Example + +=== "Hub schema (v3)" + + ```yaml + spec: + properties: + store: + type: object + properties: + backend: + type: string + s3: + type: object + properties: + bucket: + type: string + gcs: + type: object + properties: + bucket: + type: string + oneOf: + - required: ["s3"] + - required: ["gcs"] + ``` + +=== "Spoke schema (v1)" + + ```yaml + spec: + properties: + store: + type: object + properties: + backend: + type: string + objectStore: + type: object + properties: + name: + type: string + googleStore: + type: object + properties: + name: + type: string + oneOf: + - required: ["objectStore"] + - required: ["googleStore"] + ``` + +### Rule + +```yaml +- strategy: BranchMap + branchMap: + hubPath: spec.store # the union OBJECT, not a branch within it + spokePath: spec.store + discriminator: backend # optional + branches: + - hubBranch: s3 + spokeBranch: objectStore + rules: # paths are relative to the branch + - strategy: FieldRename + fieldRename: + hubPath: bucket + spokePath: name + - hubBranch: gcs + spokeBranch: googleStore + rules: + - strategy: FieldRename + fieldRename: + hubPath: bucket + spokePath: name +``` + +### Objects + +=== "Hub (v3)" + + ```yaml + spec: + store: + backend: s3 + s3: + bucket: logs + ``` + +=== "Spoke (v1)" + + ```yaml + spec: + store: + backend: objectStore + objectStore: + name: logs + ``` + +## No branch, or several, is a hard error + +Converting an object with **no** branch set, or with **more than one**, +fails the conversion with a message naming the paths — in both directions. + +That is the fail-closed posture the rest of the engine uses, and here it +buys something specific. Without it, an object with no branch set would +convert to an object with no branch set, and the *destination's* own +`oneOf` would reject it at admission — with a message about the schema, +arriving after the conversion that produced it, pointing at the wrong +thing. An object with two branches set is the shape a `oneOf` added after +the fact leaves behind in already-stored objects. + +## The discriminator + +`discriminator` names a sibling property whose value also identifies the +branch. It is remapped alongside, so hub and spoke may spell their branch +names differently: + +```yaml + discriminator: backend + branches: + - hubBranch: s3 + spokeBranch: objectStore + # Defaults to the branch names themselves, which is usually right. + hubDiscriminatorValue: s3 + spokeDiscriminatorValue: objectStore +``` + +It must be a declared property of both unions. Leave it out when the union +has no such field, or when you would rather map it yourself with +[`enumRemap`](enum-remap.md) — `branchMap` claims the discriminator only +when it is configured to manage it. + +## Coverage and ordering + +Each mapped branch is claimed as a subtree on both sides, and each branch +pair's nested rules are resolved against the two branch schemas — the same +scoping [`forEach`](for-each.md) gives an array element. A leaf inside a +branch that no nested rule covers is reported like any other uncovered +field, qualified with the branch path. + +`branchMap` writes each branch at its own path rather than replacing the +union object. A union object may carry properties that are not branches at +all — a retention period alongside `s3` and `gcs` — and those are covered +by ordinary rules. Writing the object wholesale would make the result +depend on the order the two rules were declared in, which nothing else in +this engine does. + +## Field reference + +| Field | Meaning | +|---|---| +| `hubPath` / `spokePath` | The union-typed **object** on each side, not a branch within it. | +| `discriminator` | Optional sibling property naming the active branch. Must be declared on both sides. | +| `branches[].hubBranch` / `.spokeBranch` | Property names inside the two unions. Both must be declared properties. | +| `branches[].hubDiscriminatorValue` / `.spokeDiscriminatorValue` | Values written to the discriminator. Default to the branch names. | +| `branches[].rules` | Rules for this branch pair, with paths relative to the branch. | diff --git a/docs/strategies/index.md b/docs/strategies/index.md index 9beaf17..6bbd0d1 100644 --- a/docs/strategies/index.md +++ b/docs/strategies/index.md @@ -37,6 +37,7 @@ The default posture is **fail-closed**: any hub or spoke field left unclaimed by | [`duration`](duration.md) | Go duration string ⇄ integer seconds. | :material-alert:{ style="color:#ef6c00" } Integer→canonical string is lossy; sub-second strings error | | [`mapKeyRename`](map-key-rename.md) | Rename known keys in a free-form map; other keys pass through. | :material-check-all:{ style="color:#2e7d32" } Always (injective renames) | | [`cel`](cel.md) | Arbitrary CEL expressions over declared paths. | :material-close:{ style="color:#c62828" } Always lossy (requires `acknowledgeLossy`; no `losslessOverride`) | +| [`branchMap`](branch-map.md) | Maps the branches of a `oneOf` union between versions. Requires exactly one branch set, so an `anyOf` that permits overlap is out of scope. | :material-alert:{ style="color:#ef6c00" } Lossless unless two hub branches collapse onto one | ## Reading the examples diff --git a/examples/README.md b/examples/README.md index 76371b4..9e42d31 100644 --- a/examples/README.md +++ b/examples/README.md @@ -1,6 +1,6 @@ # Examples -Five conversion stories, smallest first. Most directories hold a schema, a +Six conversion stories, smallest first. Most directories hold a schema, a conversion config, and `samples/` — everything `convctl` needs offline. [`crossplane-xr-multiversion/`](crossplane-xr-multiversion/) is a **staged** walkthrough (XRD + Composition that writes a ConfigMap) rather than a single @@ -13,6 +13,7 @@ installing the operator itself (CRDs → `ConversionWebhookServer` → config). | [`field-rename/`](field-rename/) | One field was renamed between two versions. | `FieldRename` | | [`enum-remap/`](enum-remap/) | The same field's allowed values were abbreviated. | `EnumRemap` | | [`for-each/`](for-each/) | Each element of an array changed shape. | `ForEach` + `FieldRename` | +| [`branch-map/`](branch-map/) | A `oneOf` union field whose branch names, discriminator values and inner fields all differ between versions. | `BranchMap` + `FieldRename` | | [`crossplane-xr-multiversion/`](crossplane-xr-multiversion/) | Staged XRD lifecycle: v1 + ConfigMap Composition, add v2, promote the hub, add v3, promote v3 as the standard, deprecate v1 (migrate storage, drop the version block). | `FieldRename` | | [`native-crd/`](native-crd/) | The same model against a plain Kubernetes CRD instead of a Crossplane XRD. | `FieldRename`, `Delete` | diff --git a/examples/branch-map/README.md b/examples/branch-map/README.md new file mode 100644 index 0000000..c00d04d --- /dev/null +++ b/examples/branch-map/README.md @@ -0,0 +1,73 @@ +# Branch map (union-typed fields) + +`xbackups.example.org` stores a backup destination as a **union**: exactly one +of two object-typed branches, with a `provider` discriminator naming which. The +two versions disagree about every part of it — the branch names (`s3`/`gcs` vs +`awsBucket`/`googleBucket`), the discriminator values (`s3`/`gcs` vs +`aws`/`google`), and the field inside each branch (`bucket` vs `name`). + +| File | What it is | +|---|---| +| `xrd.yaml` | The XRD. `spec.destination` is a `oneOf` union on both versions. | +| `xrdconversionconfig.yaml` | One `BranchMap` rule with nested per-branch rules, plus one ordinary `FieldRename` next to it. | +| `samples/` | One object at each served version, each with a different branch set. | + +## The interesting part + +**A union in a CRD is not a hidden shape.** The apiserver requires every +property named inside a `oneOf` to also be declared in the parent's own +`properties`, so a branch is an ordinary declared, optional field with a path. +That is why `branchMap` identifies the active branch by *which branch property +is present*, rather than by validating the object against each branch schema — +and why the engine can see, and insist on covering, every leaf inside a branch. + +Three things follow from that, all visible in this example: + +- **`hubPath`/`spokePath` name the union object, not a branch.** The rule needs + to see all the branches at once to know which one is set. +- **Nested rules are scoped to the branch.** `hubPath: bucket`, not + `hubPath: spec.destination.s3.bucket` — the same scoping `forEach` gives an + array element. `region` and `location` keep their shape on both sides, so + they need no rule inside their branches. +- **`branchMap` writes each branch at its own path, not the union object + wholesale.** `spec.destination.retentionDays` → `spec.destination.retention` + is a plain `FieldRename` sitting alongside, because the rule claimed only the + branches and the discriminator. Setting the whole object would have made the + result depend on which of the two rules ran first. + +**Converting a union with no branch set, or with two, is a hard error** — in +both directions: + +```text +branchMap: branches [gcs s3] are all set at "spec.destination"; exactly one must be +``` + +The alternative is worse than it sounds. An object with no branch converts to +an object with no branch, and the *destination's* `oneOf` then rejects it at +admission — with a message about the schema, arriving after the conversion that +caused it, pointing at the wrong thing. + +## Run it + +```console +convctl validate --config xrdconversionconfig.yaml --xrd xrd.yaml +convctl test --config xrdconversionconfig.yaml --xrd xrd.yaml --samples ./samples/ +``` + +Both conversions are lossless, so the report is four `PASS` results and no +acknowledged loss. Two edits worth trying: + +- **Drop one branch mapping.** `validate` reports the fields inside the + unmapped branch as uncovered, on both sides — an unmapped branch is an + ordinary coverage error, not a lossiness question. +- **Point both hub branches at `awsBucket`.** Collapsing branches is + expressible, but it cannot be undone: coming back, the engine cannot tell + which branch it started from, so `validate` demands `acknowledgeLossy: true` + — the same verdict `enumRemap` gives a non-injective mapping. (In *this* + schema you would also have to say what becomes of `gcs.location` and of the + now-unmapped `googleBucket`, because a collapse only makes sense when the + branches it merges have a shape the survivor can hold.) + +## Reference + +- [Branch Map strategy](https://terasky-oss.github.io/declarative-conversion-operator/strategies/branch-map/) diff --git a/examples/branch-map/samples/backup-v1.yaml b/examples/branch-map/samples/backup-v1.yaml new file mode 100644 index 0000000..ac0476a --- /dev/null +++ b/examples/branch-map/samples/backup-v1.yaml @@ -0,0 +1,12 @@ +apiVersion: example.org/v1 +kind: XBackup +metadata: + name: nightly-legacy +spec: + schedule: "0 3 * * *" + destination: + provider: aws + retention: 14 + awsBucket: + name: legacy-backups + region: eu-central-1 diff --git a/examples/branch-map/samples/backup-v2.yaml b/examples/branch-map/samples/backup-v2.yaml new file mode 100644 index 0000000..24abe7a --- /dev/null +++ b/examples/branch-map/samples/backup-v2.yaml @@ -0,0 +1,12 @@ +apiVersion: example.org/v2 +kind: XBackup +metadata: + name: nightly-hub +spec: + schedule: "0 2 * * *" + destination: + provider: gcs + retentionDays: 30 + gcs: + bucket: nightly-backups + location: europe-west4 diff --git a/examples/branch-map/xrd.yaml b/examples/branch-map/xrd.yaml new file mode 100644 index 0000000..61141a5 --- /dev/null +++ b/examples/branch-map/xrd.yaml @@ -0,0 +1,93 @@ +apiVersion: apiextensions.crossplane.io/v2 +kind: CompositeResourceDefinition +metadata: + name: xbackups.example.org +spec: + scope: Namespaced + group: example.org + names: + kind: XBackup + plural: xbackups + versions: + # Hub. spec.destination is a union: exactly one of s3 or gcs, with a + # `provider` discriminator naming which. Note that both branches are + # ordinary declared properties — the apiserver requires every property + # named inside a oneOf to also be declared in the parent's own + # `properties`, so the oneOf only says which of them may be set. + - name: v2 + served: true + referenceable: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + schedule: + type: string + destination: + type: object + properties: + provider: + type: string + enum: ["s3", "gcs"] + retentionDays: + type: integer + s3: + type: object + properties: + bucket: + type: string + region: + type: string + gcs: + type: object + properties: + bucket: + type: string + location: + type: string + oneOf: + - required: ["s3"] + - required: ["gcs"] + # Spoke. Same union, different spelling on every axis: branch names, + # discriminator values, the field inside each branch, and the + # non-branch sibling next to them. + - name: v1 + served: true + referenceable: false + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + schedule: + type: string + destination: + type: object + properties: + provider: + type: string + enum: ["aws", "google"] + retention: + type: integer + awsBucket: + type: object + properties: + name: + type: string + region: + type: string + googleBucket: + type: object + properties: + name: + type: string + location: + type: string + oneOf: + - required: ["awsBucket"] + - required: ["googleBucket"] diff --git a/examples/branch-map/xrdconversionconfig.yaml b/examples/branch-map/xrdconversionconfig.yaml new file mode 100644 index 0000000..3c825a0 --- /dev/null +++ b/examples/branch-map/xrdconversionconfig.yaml @@ -0,0 +1,53 @@ +apiVersion: terasky.com/v1alpha1 +kind: XRDConversionConfig +metadata: + name: xbackups-conversion +spec: + targetXRD: + name: xbackups.example.org + hubVersion: v2 + spokes: + - version: v1 + rules: + # The union itself. hubPath/spokePath name the union OBJECT, not a + # branch within it — branchMap writes each branch at its own path + # rather than replacing the object, which is what leaves room for + # the retentionDays rule below. + - strategy: BranchMap + branchMap: + hubPath: spec.destination + spokePath: spec.destination + # A sibling property whose value also identifies the branch. + # Remapped alongside it, so the two sides may spell their + # branch names differently — as they do here. + discriminator: provider + branches: + - hubBranch: s3 + spokeBranch: awsBucket + hubDiscriminatorValue: s3 + spokeDiscriminatorValue: aws + # Paths are relative to the branch: `bucket`, not + # `spec.destination.s3.bucket`. `region` has the same shape + # on both sides, so it needs no rule. + rules: + - strategy: FieldRename + fieldRename: + hubPath: bucket + spokePath: name + - hubBranch: gcs + spokeBranch: googleBucket + hubDiscriminatorValue: gcs + spokeDiscriminatorValue: google + rules: + - strategy: FieldRename + fieldRename: + hubPath: bucket + spokePath: name + + # A non-branch property of the same union object. An ordinary rule + # reaches it, because branchMap claimed only the branches and the + # discriminator — not the object wholesale. + - strategy: FieldRename + fieldRename: + hubPath: spec.destination.retentionDays + spokePath: spec.destination.retention diff --git a/internal/cli/engine_run.go b/internal/cli/engine_run.go index d650010..71f5393 100644 --- a/internal/cli/engine_run.go +++ b/internal/cli/engine_run.go @@ -33,7 +33,12 @@ import ( // and admission webhook use, against a locally loaded XRD and config. func runAnalyze(xrd *unstructured.Unstructured, cfg *teraskyv1alpha1.XRDConversionConfig) (engine.AnalyzeReport, []engine.VersionSchema, error) { source := xrdadapter.New(xrd) - versions, err := source.Versions() + // NormalizedVersions, not Versions: these schemas are handed to + // callers that flatten them themselves (suggest, rehub), and Analyze + // reports against the normalised shape. Handing out the raw one would + // have those callers see a different field set from the report they + // are reading. + versions, err := engine.NormalizedVersions(source) if err != nil { return engine.AnalyzeReport{}, nil, fmt.Errorf("reading XRD versions: %w", err) } @@ -64,7 +69,7 @@ func buildRouter(cfg *teraskyv1alpha1.XRDConversionConfig, report engine.Analyze // of pkg/xrdadapter. func runAnalyzeCRD(crd *extv1.CustomResourceDefinition, cfg *teraskyv1alpha1.CRDConversionConfig) (engine.AnalyzeReport, []engine.VersionSchema, error) { source := crdadapter.New(crd) - versions, err := source.Versions() + versions, err := engine.NormalizedVersions(source) if err != nil { return engine.AnalyzeReport{}, nil, fmt.Errorf("reading CRD versions: %w", err) } diff --git a/internal/cli/rehub.go b/internal/cli/rehub.go index 6265044..764cb4a 100644 --- a/internal/cli/rehub.go +++ b/internal/cli/rehub.go @@ -204,7 +204,7 @@ func stripStatus(obj any) (map[string]any, error) { } func versionLeafSetsXRD(xrd *unstructured.Unstructured) (map[string]map[string]bool, error) { - versions, err := xrdadapter.New(xrd).Versions() + versions, err := engine.NormalizedVersions(xrdadapter.New(xrd)) if err != nil { return nil, fmt.Errorf("reading XRD versions: %w", err) } @@ -212,7 +212,7 @@ func versionLeafSetsXRD(xrd *unstructured.Unstructured) (map[string]map[string]b } func versionLeafSetsCRD(crd *extv1.CustomResourceDefinition) (map[string]map[string]bool, error) { - versions, err := crdadapter.New(crd).Versions() + versions, err := engine.NormalizedVersions(crdadapter.New(crd)) if err != nil { return nil, fmt.Errorf("reading CRD versions: %w", err) } diff --git a/internal/cli/testdata/full/config.yaml b/internal/cli/testdata/full/config.yaml index ecf6603..297d3c2 100644 --- a/internal/cli/testdata/full/config.yaml +++ b/internal/cli/testdata/full/config.yaml @@ -244,3 +244,28 @@ spec: spokeToHub: '{"spec.packed": int(object.spec.bitHigh) * 256 + int(object.spec.bitLow)}' acknowledgeLossy: true reason: "integer packing via CEL is an escape hatch the engine cannot prove lossless" + + # A union field: the hub offers s3/gcs, the v1 spoke spells the same + # two branches objectStore/googleStore and names the inner field + # differently. The branch that is set decides which nested rules + # run; the discriminator is remapped alongside. + - strategy: BranchMap + branchMap: + hubPath: spec.store + spokePath: spec.store + discriminator: backend + branches: + - hubBranch: s3 + spokeBranch: objectStore + rules: + - strategy: FieldRename + fieldRename: + hubPath: bucket + spokePath: name + - hubBranch: gcs + spokeBranch: googleStore + rules: + - strategy: FieldRename + fieldRename: + hubPath: bucket + spokePath: name diff --git a/internal/cli/testdata/full/golden/hub-v3/v3-to-v1.yaml b/internal/cli/testdata/full/golden/hub-v3/v3-to-v1.yaml index b7335e8..0b19a9c 100644 --- a/internal/cli/testdata/full/golden/hub-v3/v3-to-v1.yaml +++ b/internal/cli/testdata/full/golden/hub-v3/v3-to-v1.yaml @@ -39,6 +39,10 @@ spec: replicaCount: 3 size: Large storageGB: "500" + store: + backend: objectStore + objectStore: + name: logs teamTag: platform tierLimits: - limit: "1000" diff --git a/internal/cli/testdata/full/golden/hub-v3/v3-to-v2.yaml b/internal/cli/testdata/full/golden/hub-v3/v3-to-v2.yaml index 29e3b3c..52c262a 100644 --- a/internal/cli/testdata/full/golden/hub-v3/v3-to-v2.yaml +++ b/internal/cli/testdata/full/golden/hub-v3/v3-to-v2.yaml @@ -40,6 +40,10 @@ spec: schemaVersion: v2 size: L storageSize: "500" + store: + backend: s3 + s3: + bucket: logs tags: env: prod team: platform diff --git a/internal/cli/testdata/full/golden/manifest.yaml b/internal/cli/testdata/full/golden/manifest.yaml index d141e29..2b75110 100644 --- a/internal/cli/testdata/full/golden/manifest.yaml +++ b/internal/cli/testdata/full/golden/manifest.yaml @@ -1,6 +1,6 @@ config: xwidgets-conversion convctlVersion: dev hubVersion: v3 -planHash: sha256:3tCTCOp7XqLJNcfpi49nkv4HvbQzLtOURLa-yCAJ7Ho +planHash: sha256:zvPRjj-18mRlXxHdmnLPZXxb-oAzxZ-K0Ny5GZ20Ubs resource: xwidgets.example.org -schemaHash: sha256:c6pGPtrmYUt87711WbdYxYK3ju_trkOwggG4fM3QjiM +schemaHash: sha256:er4CRli3vArwSn9YIaNdCYoN-fzZ7cgatG-fJ0l19ks diff --git a/internal/cli/testdata/full/golden/spoke-v1/v1-to-v2.yaml b/internal/cli/testdata/full/golden/spoke-v1/v1-to-v2.yaml index b9a57d1..8218e6b 100644 --- a/internal/cli/testdata/full/golden/spoke-v1/v1-to-v2.yaml +++ b/internal/cli/testdata/full/golden/spoke-v1/v1-to-v2.yaml @@ -40,6 +40,10 @@ spec: schemaVersion: v2 size: S storageSize: "64" + store: + backend: gcs + gcs: + bucket: archive tags: env: dev team: core diff --git a/internal/cli/testdata/full/golden/spoke-v1/v1-to-v3.yaml b/internal/cli/testdata/full/golden/spoke-v1/v1-to-v3.yaml index f30cb39..1df88b3 100644 --- a/internal/cli/testdata/full/golden/spoke-v1/v1-to-v3.yaml +++ b/internal/cli/testdata/full/golden/spoke-v1/v1-to-v3.yaml @@ -37,6 +37,10 @@ spec: replicaCount: 1 size: Small storageGB: "64" + store: + backend: gcs + gcs: + bucket: archive tags: env: dev team: core diff --git a/internal/cli/testdata/full/golden/spoke-v2/v2-to-v1.yaml b/internal/cli/testdata/full/golden/spoke-v2/v2-to-v1.yaml index ee85cf1..0b4250e 100644 --- a/internal/cli/testdata/full/golden/spoke-v2/v2-to-v1.yaml +++ b/internal/cli/testdata/full/golden/spoke-v2/v2-to-v1.yaml @@ -36,6 +36,10 @@ spec: replicaCount: 2 size: Medium storageGB: "256" + store: + backend: googleStore + googleStore: + name: archive teamTag: growth tierLimits: - limit: "100" diff --git a/internal/cli/testdata/full/golden/spoke-v2/v2-to-v3.yaml b/internal/cli/testdata/full/golden/spoke-v2/v2-to-v3.yaml index ac3f2d4..656c3e1 100644 --- a/internal/cli/testdata/full/golden/spoke-v2/v2-to-v3.yaml +++ b/internal/cli/testdata/full/golden/spoke-v2/v2-to-v3.yaml @@ -35,6 +35,10 @@ spec: replicaCount: 2 size: Medium storageGB: "256" + store: + backend: gcs + gcs: + bucket: archive tags: env: staging team: growth diff --git a/internal/cli/testdata/full/samples/hub-v3.yaml b/internal/cli/testdata/full/samples/hub-v3.yaml index a53b5f7..643ee05 100644 --- a/internal/cli/testdata/full/samples/hub-v3.yaml +++ b/internal/cli/testdata/full/samples/hub-v3.yaml @@ -50,6 +50,10 @@ spec: app: "widget" region: "us-east-1" packed: 1025 + store: + backend: "s3" + s3: + bucket: "logs" status: phase: "Ready" observedGeneration: 5 diff --git a/internal/cli/testdata/full/samples/spoke-v1.yaml b/internal/cli/testdata/full/samples/spoke-v1.yaml index 1679e1b..f33749b 100644 --- a/internal/cli/testdata/full/samples/spoke-v1.yaml +++ b/internal/cli/testdata/full/samples/spoke-v1.yaml @@ -50,6 +50,10 @@ spec: region: "eu-central-1" bitHigh: 3 bitLow: 16 + store: + backend: "googleStore" + googleStore: + name: "archive" status: phase: "Pending" observedGeneration: 1 diff --git a/internal/cli/testdata/full/samples/spoke-v2.yaml b/internal/cli/testdata/full/samples/spoke-v2.yaml index 931d518..35c9165 100644 --- a/internal/cli/testdata/full/samples/spoke-v2.yaml +++ b/internal/cli/testdata/full/samples/spoke-v2.yaml @@ -46,6 +46,10 @@ spec: app: "gadget" region: "us-west-1" packed: 528 + store: + backend: "gcs" + gcs: + bucket: "archive" status: state: "Provisioning" observedGeneration: 2 diff --git a/internal/cli/testdata/full/xrd.yaml b/internal/cli/testdata/full/xrd.yaml index 66c578f..85aadfa 100644 --- a/internal/cli/testdata/full/xrd.yaml +++ b/internal/cli/testdata/full/xrd.yaml @@ -109,6 +109,28 @@ spec: type: string packed: type: integer + # A union: every branch is a declared, optional property + # and the oneOf only says which one may be set. That is the + # only shape a CRD accepts — see + # pkg/engine/structural_facts_test.go. + store: + type: object + properties: + backend: + type: string + s3: + type: object + properties: + bucket: + type: string + gcs: + type: object + properties: + bucket: + type: string + oneOf: + - required: ["s3"] + - required: ["gcs"] status: type: object properties: @@ -223,6 +245,27 @@ spec: type: string packed: type: integer + # Identical to the hub's, so the v2 spoke needs no rule for + # it — the leftover scan auto-covers a leaf with the same + # shape on both sides. Only v1 renames the branches. + store: + type: object + properties: + backend: + type: string + s3: + type: object + properties: + bucket: + type: string + gcs: + type: object + properties: + bucket: + type: string + oneOf: + - required: ["s3"] + - required: ["gcs"] status: type: object properties: @@ -311,6 +354,25 @@ spec: type: integer bitLow: type: integer + # The same union, with both branches renamed (BranchMap). + store: + type: object + properties: + backend: + type: string + objectStore: + type: object + properties: + name: + type: string + googleStore: + type: object + properties: + name: + type: string + oneOf: + - required: ["objectStore"] + - required: ["googleStore"] # -- mirrors of fields owned by the v2 spoke's rules, kept # identical in shape so they're auto-covered here without a # rule -- diff --git a/internal/scalegen/catalog.go b/internal/scalegen/catalog.go index f56a002..8686091 100644 --- a/internal/scalegen/catalog.go +++ b/internal/scalegen/catalog.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "math/rand" + "sort" extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -201,9 +202,52 @@ func slots() []Slot { SpokeToHub: `has(object.spec) && has(object.spec.bitHigh) && has(object.spec.bitLow) ? {"spec.packed": int(object.spec.bitHigh) * 256 + int(object.spec.bitLow)} : {}`, }}), SpokeSpec: map[string]any{"bitHigh": 4, "bitLow": 1}}, + {Name: v1a.StrategyBranchMap, + HubProps: map[string]extv1.JSONSchemaProps{"store": unionProp("backend", map[string]extv1.JSONSchemaProps{ + "s3": objProp(map[string]extv1.JSONSchemaProps{"bucket": strProp()}), + "gcs": objProp(map[string]extv1.JSONSchemaProps{"bucket": strProp()}), + })}, + SpokeProps: map[string]extv1.JSONSchemaProps{"store": unionProp("backend", map[string]extv1.JSONSchemaProps{ + "objectStore": objProp(map[string]extv1.JSONSchemaProps{"name": strProp()}), + "googleStore": objProp(map[string]extv1.JSONSchemaProps{"name": strProp()}), + })}, + Rule: v1a.ConversionRule{Strategy: v1a.StrategyBranchMap, BranchMap: &v1a.BranchMapParams{ + HubPath: "spec.store", SpokePath: "spec.store", Discriminator: "backend", + Branches: []v1a.BranchMapping{ + {HubBranch: "s3", SpokeBranch: "objectStore", Rules: []v1a.ConversionRule{ + {Strategy: v1a.StrategyFieldRename, FieldRename: &v1a.FieldRenameParams{HubPath: "bucket", SpokePath: "name"}}, + }}, + {HubBranch: "gcs", SpokeBranch: "googleStore", Rules: []v1a.ConversionRule{ + {Strategy: v1a.StrategyFieldRename, FieldRename: &v1a.FieldRenameParams{HubPath: "bucket", SpokePath: "name"}}, + }}, + }, + }}, + SpokeSpec: map[string]any{"store": map[string]any{"backend": "objectStore", "objectStore": map[string]any{"name": "logs"}}}}, } } +// unionProp is the shape a CRD actually accepts for a union: every branch +// is a declared, optional property and the oneOf only says which of them +// may be set. See pkg/engine/structural_facts_test.go. +func unionProp(discriminator string, branches map[string]extv1.JSONSchemaProps) extv1.JSONSchemaProps { + props := map[string]extv1.JSONSchemaProps{discriminator: strProp()} + names := make([]string, 0, len(branches)) + for name := range branches { + names = append(names, name) + } + // Sorted so a generated CRD is byte-identical between runs at the same + // seed, which is what makes --reset idempotent. + sort.Strings(names) + oneOf := make([]extv1.JSONSchemaProps, 0, len(names)) + for _, name := range names { + props[name] = branches[name] + oneOf = append(oneOf, extv1.JSONSchemaProps{Required: []string{name}}) + } + p := objProp(props) + p.OneOf = oneOf + return p +} + func mergeProps(dst map[string]extv1.JSONSchemaProps, src map[string]extv1.JSONSchemaProps) { for k, v := range src { dst[k] = v diff --git a/internal/scalegen/scalegen_test.go b/internal/scalegen/scalegen_test.go index 980a51c..dd81534 100644 --- a/internal/scalegen/scalegen_test.go +++ b/internal/scalegen/scalegen_test.go @@ -28,8 +28,8 @@ import ( func TestSlotsCount(t *testing.T) { t.Parallel() - if n := len(slots()); n != 29 { - t.Fatalf("catalog has %d strategies, want 29", n) + if n := len(slots()); n != 30 { + t.Fatalf("catalog has %d strategies, want 30", n) } } @@ -65,7 +65,7 @@ func TestAssign_TooSmallToCover(t *testing.T) { t.Parallel() _, _, err := Assign(1, 3, 10, 1) if err == nil { - t.Fatal("expected error when 2*targets*max < 29") + t.Fatal("expected error when 2*targets*max < 30") } } @@ -75,8 +75,8 @@ func TestBuildTargets_AnalyzeAndConvert(t *testing.T) { if err != nil { t.Fatal(err) } - if cov := StrategyCoverage(targets); len(cov) != 29 { - t.Fatalf("coverage has %d strategies, want 29", len(cov)) + if cov := StrategyCoverage(targets); len(cov) != 30 { + t.Fatalf("coverage has %d strategies, want 30", len(cov)) } if got := targets[0].CRD.Spec.Names.Categories; len(got) != 1 || got[0] != Category { t.Fatalf("CRD categories = %v, want [%s]", got, Category) @@ -189,8 +189,8 @@ func TestRun_DryRun(t *testing.T) { if err != nil { t.Fatal(err) } - if res.Targets != 4 || len(res.Coverage) != 29 { - t.Fatalf("targets=%d coverage=%d, want 4 and 29", res.Targets, len(res.Coverage)) + if res.Targets != 4 || len(res.Coverage) != 30 { + t.Fatalf("targets=%d coverage=%d, want 4 and 30", res.Targets, len(res.Coverage)) } } diff --git a/internal/webhook/xrdconversionconfig_webhook.go b/internal/webhook/xrdconversionconfig_webhook.go index a9fbd22..25ea1b2 100644 --- a/internal/webhook/xrdconversionconfig_webhook.go +++ b/internal/webhook/xrdconversionconfig_webhook.go @@ -286,9 +286,39 @@ func validateOneRule(r teraskyv1alpha1.ConversionRule, depth int) error { if r.CEL != nil { set++ } + if r.BranchMap != nil { + set++ + } if set != 1 { return fmt.Errorf("strategy %q requires exactly one matching params field to be set, found %d", r.Strategy, set) } + if r.BranchMap != nil { + if len(r.BranchMap.Branches) == 0 { + return errors.New("branchMap: at least one branch mapping is required") + } + // Structural checks only. Whether a branch names a declared + // property is a schema question, answered by the live-schema + // analysis stage that runs after this one; what is checkable here + // is that the document does not contradict itself. + // + // Note what is deliberately NOT rejected: two hub branches mapping + // to the same spoke branch. Collapsing several backends onto one + // is a legitimate migration — it is simply lossy coming back, and + // the engine says so, which is what acknowledgeLossy is for. + seenHub := map[string]bool{} + for i, b := range r.BranchMap.Branches { + if strings.TrimSpace(b.HubBranch) == "" || strings.TrimSpace(b.SpokeBranch) == "" { + return fmt.Errorf("branchMap: branch %d must name both hubBranch and spokeBranch", i) + } + if seenHub[b.HubBranch] { + return fmt.Errorf("branchMap: hubBranch %q is mapped twice; a branch has exactly one counterpart", b.HubBranch) + } + seenHub[b.HubBranch] = true + if err := validateRules(b.Rules, depth); err != nil { + return fmt.Errorf("branchMap: branch %d (%s): %w", i, b.HubBranch, err) + } + } + } if r.ForEach != nil { if depth >= engine.MaxForEachDepth { return fmt.Errorf("ForEach nesting depth exceeds the supported maximum of %d", engine.MaxForEachDepth) diff --git a/internal/webhookserver/metrics.go b/internal/webhookserver/metrics.go index d69ff17..838d978 100755 --- a/internal/webhookserver/metrics.go +++ b/internal/webhookserver/metrics.go @@ -94,8 +94,8 @@ func NewMetrics(reg prometheus.Registerer, gatherer prometheus.Gatherer) *Metric }, []string{"target", "result"}), ObjectsTotal: prometheus.NewCounterVec(prometheus.CounterOpts{ Name: "dco_webhook_conversion_objects_total", - Help: "Total individual objects converted.", - }, []string{"target", "from_version", "to_version", "result"}), + Help: "Total individual objects converted. The route label classifies the conversion by shape — hub_to_spoke, spoke_to_hub, spoke_to_spoke, identity — which from_version and to_version cannot, because which version is the hub is a per-target fact and not a label.", + }, []string{"target", "from_version", "to_version", "route", "result"}), ObjectDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Name: "dco_webhook_conversion_object_duration_seconds", Help: "Latency of converting one object, excluding request decode. Unlike the review-level histogram this metric's direction label is always exact, so it is the one to use for per-direction capacity planning.", diff --git a/internal/webhookserver/server.go b/internal/webhookserver/server.go index ef415ba..c09817b 100755 --- a/internal/webhookserver/server.go +++ b/internal/webhookserver/server.go @@ -336,6 +336,9 @@ func (s *Server) handleConvert(w http.ResponseWriter, r *http.Request) { // without it an operator raising the limit is guessing. s.Metrics.BatchSize.WithLabelValues(xrdName).Observe(float64(len(review.Request.Objects))) } + // Lossy hops for the objects converted so far, applied only once the + // whole review has succeeded — see the switch below. + var lossyHops []lossyHop for _, raw := range review.Request.Objects { if err := ctx.Err(); err != nil { s.writeReview(w, review.Request.UID, nil, fmt.Sprintf( @@ -363,11 +366,7 @@ func (s *Server) handleConvert(w http.ResponseWriter, r *http.Request) { attribute.String("from_version", fromVersion), attribute.String("to_version", toVersion), )) - if fromVersion == entry.Router.Hub { - s.recordLossy(entry, xrdName, "hub_to_spoke", toVersion) - } else if toVersion == entry.Router.Hub { - s.recordLossy(entry, xrdName, "spoke_to_hub", fromVersion) - } + route := routeLabel(entry.Router.Hub, fromVersion, toVersion) out, err := entry.Router.Convert(obj, fromVersion, toVersion) if err != nil { @@ -379,12 +378,32 @@ func (s *Server) handleConvert(w http.ResponseWriter, r *http.Request) { span.RecordError(err) span.SetStatus(codes.Error, err.Error()) if s.Metrics != nil { - s.Metrics.ObjectsTotal.WithLabelValues(xrdName, fromVersion, toVersion, "error").Inc() + s.Metrics.ObjectsTotal.WithLabelValues(xrdName, fromVersion, toVersion, route, "error").Inc() s.Metrics.ObjectDuration.WithLabelValues(xrdName, objDirection, "error").Observe(time.Since(objStart).Seconds()) } return } objSpan.End() + // Accumulated, not recorded yet. The counter means "a lossy + // conversion was delivered", and delivery is a property of the + // whole ConversionReview: a later object that fails takes the + // entire response down with it, so an earlier object that + // converted lossily is never returned and never stored either. + // Recording here would count loss for an object the apiserver + // discarded. + // + // A spoke-to-spoke conversion is two hops through the hub and each + // one can be lossy on its own, so both are noted. The earlier + // hub-or-nothing test counted neither, which left the traffic + // class that carries the most loss reporting none. + switch route { + case routeHubToSpoke: + lossyHops = append(lossyHops, lossyHop{"hub_to_spoke", toVersion}) + case routeSpokeToHub: + lossyHops = append(lossyHops, lossyHop{"spoke_to_hub", fromVersion}) + case routeSpokeToSpoke: + lossyHops = append(lossyHops, lossyHop{"spoke_to_hub", fromVersion}, lossyHop{"hub_to_spoke", toVersion}) + } out["apiVersion"] = review.Request.DesiredAPIVersion ensureConvertedMetadata(out, obj) b, err := json.Marshal(out) @@ -397,7 +416,7 @@ func (s *Server) handleConvert(w http.ResponseWriter, r *http.Request) { } converted = append(converted, runtime.RawExtension{Raw: b}) if s.Metrics != nil { - s.Metrics.ObjectsTotal.WithLabelValues(xrdName, fromVersion, toVersion, "success").Inc() + s.Metrics.ObjectsTotal.WithLabelValues(xrdName, fromVersion, toVersion, route, "success").Inc() s.Metrics.ObjectDuration.WithLabelValues(xrdName, objDirection, "success").Observe(time.Since(objStart).Seconds()) } } @@ -422,10 +441,54 @@ func (s *Server) handleConvert(w http.ResponseWriter, r *http.Request) { return } + // The review is going out. Only now has anything actually been + // delivered, so only now does a lossy conversion count as performed. + for _, hop := range lossyHops { + s.recordLossy(entry, xrdName, hop.direction, hop.spokeVersion) + } s.writeReview(w, review.Request.UID, converted, "") s.observe(xrdName, direction, "success", start) } +// lossyHop is one hub<->spoke hop, of one object, that ran in a direction +// statically known to be lossy. Held until the whole ConversionReview +// succeeds, because a review that fails delivers none of its objects. +type lossyHop struct { + direction string + spokeVersion string +} + +// Route classes for dco_webhook_conversion_objects_total. They exist +// because the metric's from_version/to_version pair cannot answer "how +// much of this traffic is spoke-to-spoke?" on its own: that needs to know +// which version is the hub, which is a per-target fact and not a label. +// Deriving it in PromQL would mean hard-coding every target's hub version +// into the query, and re-editing the query whenever a hub is promoted. +// +// The label is a function of labels the series already carries, so it adds +// no cardinality beyond the four constants below. +const ( + routeIdentity = "identity" + routeHubToSpoke = "hub_to_spoke" + routeSpokeToHub = "spoke_to_hub" + routeSpokeToSpoke = "spoke_to_spoke" +) + +// routeLabel classifies a conversion by its shape rather than by version +// names. +func routeLabel(hub, from, to string) string { + switch { + case from == to: + return routeIdentity + case from == hub: + return routeHubToSpoke + case to == hub: + return routeSpokeToHub + default: + return routeSpokeToSpoke + } +} + func (s *Server) recordLossy(entry *CompiledEntry, xrdName, direction, spokeVersion string) { if s.Metrics == nil || entry.Lossless == nil { return diff --git a/internal/webhookserver/server_route_label_test.go b/internal/webhookserver/server_route_label_test.go new file mode 100644 index 0000000..cd19d15 --- /dev/null +++ b/internal/webhookserver/server_route_label_test.go @@ -0,0 +1,250 @@ +/* +Copyright 2026 The declarative-conversion-operator Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package webhookserver + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/prometheus/client_golang/prometheus/testutil" + extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + + "github.com/terasky-oss/declarative-conversion-operator/pkg/engine" +) + +func TestRouteLabel(t *testing.T) { + for _, tc := range []struct{ from, to, want string }{ + {"v3", "v3", routeIdentity}, + {"v3", "v1", routeHubToSpoke}, + {"v1", "v3", routeSpokeToHub}, + {"v1", "v2", routeSpokeToSpoke}, + } { + if got := routeLabel("v3", tc.from, tc.to); got != tc.want { + t.Errorf("routeLabel(v3, %s, %s) = %q, want %q", tc.from, tc.to, got, tc.want) + } + } +} + +// threeVersionServer is a hub plus two spokes, which is the smallest shape +// that can route spoke-to-spoke at all. +func threeVersionServer(t *testing.T, lossless map[string]engine.LosslessVerdict) (*Server, *Metrics) { + t.Helper() + const hub = "v3" + plans := map[string]*engine.Plan{} + for _, spoke := range []string{"v1", "v2"} { + plans[spoke] = &engine.Plan{HubVersion: hub, SpokeVersion: spoke, HubToSpoke: []engine.Op{}, SpokeToHub: []engine.Op{}} + } + registry := NewRegistry() + registry.Set("xfoos.example.org", &CompiledEntry{ + Router: &engine.Router{Hub: hub, Plans: plans}, + Lossless: lossless, + }) + metrics := newTestMetrics() + return &Server{Registry: registry, Metrics: metrics}, metrics +} + +func convertOne(t *testing.T, s *Server, from, to string) { + t.Helper() + obj := map[string]any{"apiVersion": "example.org/" + from, "kind": "Foo", "metadata": map[string]any{"name": "x"}} + raw, err := json.Marshal(obj) + if err != nil { + t.Fatalf("marshaling the sample object: %v", err) + } + body, err := json.Marshal(extv1.ConversionReview{Request: &extv1.ConversionRequest{ + UID: "abc", DesiredAPIVersion: "example.org/" + to, + Objects: []runtime.RawExtension{{Raw: raw}}, + }}) + if err != nil { + t.Fatalf("marshaling the review: %v", err) + } + req := httptest.NewRequest(http.MethodPost, "/convert/xfoos.example.org", bytes.NewReader(body)) + rec := httptest.NewRecorder() + s.handleConvert(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("%s->%s: expected 200, got %d: %s", from, to, rec.Code, rec.Body.String()) + } +} + +// The point of the label: "how much of this traffic is spoke-to-spoke?" is +// not answerable from from_version and to_version, because which version +// is the hub is a per-target fact that is not a label. +func TestHandleConvert_RouteLabelClassifiesEveryShape(t *testing.T) { + s, metrics := threeVersionServer(t, nil) + convertOne(t, s, "v3", "v1") + convertOne(t, s, "v1", "v3") + convertOne(t, s, "v1", "v2") + convertOne(t, s, "v2", "v2") + + for _, tc := range []struct{ from, to, route string }{ + {"v3", "v1", routeHubToSpoke}, + {"v1", "v3", routeSpokeToHub}, + {"v1", "v2", routeSpokeToSpoke}, + {"v2", "v2", routeIdentity}, + } { + got := testutil.ToFloat64(metrics.ObjectsTotal.WithLabelValues("xfoos.example.org", tc.from, tc.to, tc.route, "success")) + if got != 1 { + t.Errorf("%s->%s: route=%s counter = %v, want 1", tc.from, tc.to, tc.route, got) + } + } +} + +// A spoke-to-spoke conversion is two hops through the hub, and each one +// can be lossy on its own. Counting neither — which the old hub-or-nothing +// test did — makes the lossy counter silently under-report exactly the +// traffic class that carries the most loss. +func TestHandleConvert_SpokeToSpokeCountsBothLossyHops(t *testing.T) { + s, metrics := threeVersionServer(t, map[string]engine.LosslessVerdict{ + "v1": {HubToSpoke: true, SpokeToHub: false}, // v1->hub loses something + "v2": {HubToSpoke: false, SpokeToHub: true}, // hub->v2 loses something + }) + convertOne(t, s, "v1", "v2") + + if got := testutil.ToFloat64(metrics.LossyTotal.WithLabelValues("xfoos.example.org", "spoke_to_hub")); got != 1 { + t.Errorf("inbound v1->hub hop: spoke_to_hub counter = %v, want 1", got) + } + if got := testutil.ToFloat64(metrics.LossyTotal.WithLabelValues("xfoos.example.org", "hub_to_spoke")); got != 1 { + t.Errorf("outbound hub->v2 hop: hub_to_spoke counter = %v, want 1", got) + } +} + +// A conversion that failed delivered nothing: the object is never returned +// and never stored, so nothing observable lost anything. Counting it as a +// lossy conversion would double-signal a failure the objects and reviews +// counters already record as an error. +func TestHandleConvert_FailedConversionRecordsNoLoss(t *testing.T) { + const hub = "v3" + registry := NewRegistry() + registry.Set("xfoos.example.org", &CompiledEntry{ + // v2 has no compiled plan, so routing v1 -> v2 fails on the + // second hop, after the first one has already run. + Router: &engine.Router{Hub: hub, Plans: map[string]*engine.Plan{ + "v1": {HubVersion: hub, SpokeVersion: "v1", HubToSpoke: []engine.Op{}, SpokeToHub: []engine.Op{}}, + }}, + Lossless: map[string]engine.LosslessVerdict{ + "v1": {HubToSpoke: true, SpokeToHub: false}, + "v2": {HubToSpoke: false, SpokeToHub: true}, + }, + }) + metrics := newTestMetrics() + s := &Server{Registry: registry, Metrics: metrics} + + obj := map[string]any{"apiVersion": "example.org/v1", "kind": "Foo", "metadata": map[string]any{"name": "x"}} + raw, err := json.Marshal(obj) + if err != nil { + t.Fatalf("marshaling the sample object: %v", err) + } + body, err := json.Marshal(extv1.ConversionReview{Request: &extv1.ConversionRequest{ + UID: "abc", DesiredAPIVersion: "example.org/v2", + Objects: []runtime.RawExtension{{Raw: raw}}, + }}) + if err != nil { + t.Fatalf("marshaling the review: %v", err) + } + req := httptest.NewRequest(http.MethodPost, "/convert/xfoos.example.org", bytes.NewReader(body)) + rec := httptest.NewRecorder() + s.handleConvert(rec, req) + + var got extv1.ConversionReview + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decoding response: %v", err) + } + if got.Response.Result.Status != metav1.StatusFailure { + t.Fatalf("expected the conversion to fail, got %+v", got.Response.Result) + } + for _, direction := range []string{"spoke_to_hub", "hub_to_spoke"} { + if n := testutil.ToFloat64(metrics.LossyTotal.WithLabelValues("xfoos.example.org", direction)); n != 0 { + t.Errorf("%s counter = %v after a failed conversion, want 0", direction, n) + } + } + if n := testutil.ToFloat64(metrics.ObjectsTotal.WithLabelValues("xfoos.example.org", "v1", "v2", routeSpokeToSpoke, "error")); n != 1 { + t.Errorf("the failure should be counted as an error on the objects counter, got %v", n) + } +} + +// Delivery is a property of the whole ConversionReview, not of one object +// in it. A later object that fails takes the entire response down, so an +// earlier object that converted lossily was never returned either — and +// counting its loss would describe something that did not reach anybody. +func TestHandleConvert_LossIsNotCountedWhenALaterObjectFails(t *testing.T) { + const hub = "v3" + registry := NewRegistry() + registry.Set("xfoos.example.org", &CompiledEntry{ + Router: &engine.Router{Hub: hub, Plans: map[string]*engine.Plan{ + "v1": {HubVersion: hub, SpokeVersion: "v1", HubToSpoke: []engine.Op{}, SpokeToHub: []engine.Op{}}, + }}, + Lossless: map[string]engine.LosslessVerdict{"v1": {HubToSpoke: true, SpokeToHub: false}}, + }) + metrics := newTestMetrics() + s := &Server{Registry: registry, Metrics: metrics} + + // First object: v1 -> hub, a hop the verdict above marks lossy, and it + // converts fine. Second object: v2 has no compiled plan, so the review + // fails as a whole. + raw := func(version string) runtime.RawExtension { + b, err := json.Marshal(map[string]any{ + "apiVersion": "example.org/" + version, "kind": "Foo", + "metadata": map[string]any{"name": "x-" + version}, + }) + if err != nil { + t.Fatalf("marshaling the %s object: %v", version, err) + } + return runtime.RawExtension{Raw: b} + } + body, err := json.Marshal(extv1.ConversionReview{Request: &extv1.ConversionRequest{ + UID: "abc", DesiredAPIVersion: "example.org/" + hub, + Objects: []runtime.RawExtension{raw("v1"), raw("v2")}, + }}) + if err != nil { + t.Fatalf("marshaling the review: %v", err) + } + req := httptest.NewRequest(http.MethodPost, "/convert/xfoos.example.org", bytes.NewReader(body)) + rec := httptest.NewRecorder() + s.handleConvert(rec, req) + + var got extv1.ConversionReview + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decoding response: %v", err) + } + if got.Response.Result.Status != metav1.StatusFailure { + t.Fatalf("expected the review to fail on the second object, got %+v", got.Response.Result) + } + if n := testutil.ToFloat64(metrics.LossyTotal.WithLabelValues("xfoos.example.org", "spoke_to_hub")); n != 0 { + t.Errorf("spoke_to_hub counter = %v; the first object's loss was never delivered", n) + } +} + +// The mirror image: a spoke-to-spoke route whose two hops are both +// lossless must not be counted at all, or the fix above would just be a +// louder wrong answer. +func TestHandleConvert_SpokeToSpokeCountsNoLossWhenBothHopsAreLossless(t *testing.T) { + s, metrics := threeVersionServer(t, map[string]engine.LosslessVerdict{ + "v1": {HubToSpoke: true, SpokeToHub: true}, + "v2": {HubToSpoke: true, SpokeToHub: true}, + }) + convertOne(t, s, "v1", "v2") + + for _, direction := range []string{"spoke_to_hub", "hub_to_spoke"} { + if got := testutil.ToFloat64(metrics.LossyTotal.WithLabelValues("xfoos.example.org", direction)); got != 0 { + t.Errorf("%s counter = %v, want 0", direction, got) + } + } +} diff --git a/mkdocs.yml b/mkdocs.yml index 60ea6b2..51d3f55 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -113,6 +113,7 @@ nav: - Duration: strategies/duration.md - Map Key Rename: strategies/map-key-rename.md - CEL: strategies/cel.md + - Branch Map: strategies/branch-map.md - Contributing: - Adding a strategy: contributing/adding-a-strategy.md - FAQ: faq.md diff --git a/pkg/engine/analyze.go b/pkg/engine/analyze.go index 9ab6fd7..7a28455 100644 --- a/pkg/engine/analyze.go +++ b/pkg/engine/analyze.go @@ -33,7 +33,11 @@ type AnalyzeInput struct { // last known-good plan on a failed re-validation" trivial for callers: a // failing re-analysis simply never yields a replacement Plan. func Analyze(in AnalyzeInput) (AnalyzeReport, error) { - versions, err := in.Source.Versions() + // Normalised once, here, at the only boundary where schemas enter the + // engine — so flattenSchema, every resolver, the leftover scan and the + // passthrough tree all see the same ordinary, junctor-free shape and + // none of them has to know that $ref or allOf ever existed. + versions, err := NormalizedVersions(in.Source) if err != nil { return AnalyzeReport{}, fmt.Errorf("analyze: reading versions: %w", err) } diff --git a/pkg/engine/branchmap_test.go b/pkg/engine/branchmap_test.go new file mode 100644 index 0000000..998947a --- /dev/null +++ b/pkg/engine/branchmap_test.go @@ -0,0 +1,457 @@ +/* +Copyright 2026 The declarative-conversion-operator Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package engine + +import ( + "reflect" + "strings" + "testing" + + extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" +) + +// unionSchema builds the shape structural_facts_test.go proves a CRD +// actually accepts: every branch is a declared, optional property, and the +// oneOf only says which of them may be set. +func unionSchema(discriminator string, branches map[string]extv1.JSONSchemaProps) extv1.JSONSchemaProps { + props := map[string]extv1.JSONSchemaProps{} + var oneOf []extv1.JSONSchemaProps + for name, schema := range branches { + props[name] = schema + oneOf = append(oneOf, extv1.JSONSchemaProps{Required: []string{name}}) + } + if discriminator != "" { + props[discriminator] = strSchema() + } + s := objSchema(props) + s.OneOf = oneOf + return s +} + +func hubUnion() extv1.JSONSchemaProps { + return objSchema(map[string]extv1.JSONSchemaProps{ + "backup": unionSchema("backend", map[string]extv1.JSONSchemaProps{ + "s3": objSchema(map[string]extv1.JSONSchemaProps{"bucket": strSchema()}), + "gcs": objSchema(map[string]extv1.JSONSchemaProps{"bucket": strSchema()}), + }), + }) +} + +func spokeUnion() extv1.JSONSchemaProps { + return objSchema(map[string]extv1.JSONSchemaProps{ + "backup": unionSchema("backend", map[string]extv1.JSONSchemaProps{ + "objectStore": objSchema(map[string]extv1.JSONSchemaProps{"name": strSchema()}), + "googleStore": objSchema(map[string]extv1.JSONSchemaProps{"name": strSchema()}), + }), + }) +} + +func branchMapRuleSet() RuleSet { + return RuleSet{HubVersion: "v2", SpokeVersion: "v1", Rules: []Rule{{ + Strategy: StrategyBranchMap, + Params: BranchMapParams{ + HubPath: ParsePath("backup"), SpokePath: ParsePath("backup"), + Discriminator: "backend", + Branches: []BranchMapping{ + { + HubBranch: "s3", SpokeBranch: "objectStore", + Rules: []Rule{{Strategy: StrategyFieldRename, Params: FieldRenameParams{HubPath: ParsePath("bucket"), SpokePath: ParsePath("name")}}}, + }, + { + HubBranch: "gcs", SpokeBranch: "googleStore", + Rules: []Rule{{Strategy: StrategyFieldRename, Params: FieldRenameParams{HubPath: ParsePath("bucket"), SpokePath: ParsePath("name")}}}, + }, + }, + }, + }}} +} + +// The headline: the branches of a union are ordinary fields now, so the +// leftover scan sees every leaf inside them — and branchMap has to claim +// them all, or a perfectly complete config would report as uncovered. +func TestBranchMap_CoverageClaimsEveryBranchLeaf(t *testing.T) { + hub, spoke := hubUnion(), spokeUnion() + _, diags, err := Compile(branchMapRuleSet(), &hub, &spoke) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if errs := diagMessages(diags, SeverityError); len(errs) != 0 { + t.Fatalf("expected a complete branch mapping to compile cleanly, got %v", errs) + } +} + +func TestBranchMap_ConvertsBothDirections(t *testing.T) { + hub, spoke := hubUnion(), spokeUnion() + plan, diags, err := Compile(branchMapRuleSet(), &hub, &spoke) + if err != nil { + t.Fatalf("compile: %v", err) + } + if errs := diagMessages(diags, SeverityError); len(errs) != 0 { + t.Fatalf("compile errors: %v", errs) + } + + hubObj := map[string]any{"backup": map[string]any{"backend": "s3", "s3": map[string]any{"bucket": "logs"}}} + out, err := Convert(ConvertInput{Plan: plan, Direction: HubToSpoke, Object: hubObj}) + if err != nil { + t.Fatalf("hub->spoke: %v", err) + } + want := map[string]any{"backend": "objectStore", "objectStore": map[string]any{"name": "logs"}} + if got := out["backup"]; !reflect.DeepEqual(got, want) { + t.Fatalf("hub->spoke backup = %#v, want %#v", got, want) + } + + back, err := Convert(ConvertInput{Plan: plan, Direction: SpokeToHub, Object: map[string]any{"backup": want}}) + if err != nil { + t.Fatalf("spoke->hub: %v", err) + } + wantBack := map[string]any{"backend": "s3", "s3": map[string]any{"bucket": "logs"}} + if got := back["backup"]; !reflect.DeepEqual(got, wantBack) { + t.Fatalf("round trip = %#v, want %#v", got, wantBack) + } +} + +// Fail closed, both ways. A union with nothing set, or with two branches +// set, is an object the destination's own oneOf would reject — but it +// would reject it with a message about the schema, long after the +// conversion that produced it. +func TestBranchMap_NoBranchAndMultipleBranchesAreRuntimeErrors(t *testing.T) { + hub, spoke := hubUnion(), spokeUnion() + plan, _, err := Compile(branchMapRuleSet(), &hub, &spoke) + if err != nil { + t.Fatalf("compile: %v", err) + } + + _, err = Convert(ConvertInput{Plan: plan, Direction: HubToSpoke, Object: map[string]any{ + "backup": map[string]any{"backend": "s3"}, + }}) + if err == nil || !strings.Contains(err.Error(), "no branch is set") { + t.Fatalf("expected a no-branch error, got %v", err) + } + for _, want := range []string{"gcs", "s3"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the error should name the branches it expected; %q missing from %v", want, err) + } + } + + _, err = Convert(ConvertInput{Plan: plan, Direction: HubToSpoke, Object: map[string]any{ + "backup": map[string]any{"s3": map[string]any{"bucket": "a"}, "gcs": map[string]any{"bucket": "b"}}, + }}) + if err == nil || !strings.Contains(err.Error(), "all set") { + t.Fatalf("expected a multiple-branch error, got %v", err) + } +} + +// A union object can carry properties that are not branches. branchMap +// writes each branch at its own path rather than replacing the object, so +// a rule covering one of those cannot be clobbered — whichever order the +// two rules are declared in. +func TestBranchMap_DoesNotClobberSiblingsOfTheUnion(t *testing.T) { + hub := objSchema(map[string]extv1.JSONSchemaProps{ + "backup": withProp(unionSchema("", map[string]extv1.JSONSchemaProps{ + "s3": objSchema(map[string]extv1.JSONSchemaProps{"bucket": strSchema()}), + }), "retentionDays", intSchema()), + }) + spoke := objSchema(map[string]extv1.JSONSchemaProps{ + "backup": withProp(unionSchema("", map[string]extv1.JSONSchemaProps{ + "objectStore": objSchema(map[string]extv1.JSONSchemaProps{"bucket": strSchema()}), + }), "keepDays", intSchema()), + }) + + rename := Rule{Strategy: StrategyFieldRename, Params: FieldRenameParams{ + HubPath: ParsePath("backup.retentionDays"), SpokePath: ParsePath("backup.keepDays"), + }} + branch := Rule{Strategy: StrategyBranchMap, Params: BranchMapParams{ + HubPath: ParsePath("backup"), SpokePath: ParsePath("backup"), + Branches: []BranchMapping{{HubBranch: "s3", SpokeBranch: "objectStore"}}, + }} + + obj := map[string]any{"backup": map[string]any{"retentionDays": int64(7), "s3": map[string]any{"bucket": "logs"}}} + want := map[string]any{"keepDays": int64(7), "objectStore": map[string]any{"bucket": "logs"}} + + // Both orders, because "ordering can never matter" is a property this + // engine claims and a wholesale object write would break. + for name, rules := range map[string][]Rule{ + "branch first": {branch, rename}, + "rename first": {rename, branch}, + } { + t.Run(name, func(t *testing.T) { + plan, diags, err := Compile(RuleSet{Rules: rules}, &hub, &spoke) + if err != nil { + t.Fatalf("compile: %v", err) + } + if errs := diagMessages(diags, SeverityError); len(errs) != 0 { + t.Fatalf("compile errors: %v", errs) + } + out, err := Convert(ConvertInput{Plan: plan, Direction: HubToSpoke, Object: obj}) + if err != nil { + t.Fatalf("convert: %v", err) + } + if got := out["backup"]; !reflect.DeepEqual(got, want) { + t.Fatalf("backup = %#v, want %#v", got, want) + } + }) + } +} + +// A hub branch the rule does not name is data the conversion would drop. +// It surfaces through the ordinary coverage machinery, as an error, which +// is stronger than a lossiness warning would be. +func TestBranchMap_UnmappedBranchIsUncovered(t *testing.T) { + hub := objSchema(map[string]extv1.JSONSchemaProps{ + "backup": unionSchema("", map[string]extv1.JSONSchemaProps{ + "s3": objSchema(map[string]extv1.JSONSchemaProps{"bucket": strSchema()}), + "azure": objSchema(map[string]extv1.JSONSchemaProps{"container": strSchema()}), + }), + }) + spoke := objSchema(map[string]extv1.JSONSchemaProps{ + "backup": unionSchema("", map[string]extv1.JSONSchemaProps{ + "objectStore": objSchema(map[string]extv1.JSONSchemaProps{"bucket": strSchema()}), + }), + }) + rs := RuleSet{Rules: []Rule{{Strategy: StrategyBranchMap, Params: BranchMapParams{ + HubPath: ParsePath("backup"), SpokePath: ParsePath("backup"), + Branches: []BranchMapping{{HubBranch: "s3", SpokeBranch: "objectStore"}}, + }}}} + + _, diags, err := Compile(rs, &hub, &spoke) + if err != nil { + t.Fatalf("compile: %v", err) + } + errs := diagMessages(diags, SeverityError) + var sawAzure bool + for _, msg := range errs { + if strings.Contains(msg, "backup.azure") { + sawAzure = true + } + } + if !sawAzure { + t.Fatalf("expected the unmapped hub branch to be reported as uncovered, got %v", errs) + } +} + +// Collapsing two hub branches onto one spoke branch is expressible and +// sometimes intended, but it cannot be undone: coming back, the engine +// cannot tell which one it started from. +func TestBranchMap_NonInjectiveIsLossyInTheCollapsingDirection(t *testing.T) { + hub := objSchema(map[string]extv1.JSONSchemaProps{ + "backup": unionSchema("", map[string]extv1.JSONSchemaProps{ + "s3": objSchema(map[string]extv1.JSONSchemaProps{"bucket": strSchema()}), + "gcs": objSchema(map[string]extv1.JSONSchemaProps{"bucket": strSchema()}), + }), + }) + spoke := objSchema(map[string]extv1.JSONSchemaProps{ + "backup": unionSchema("", map[string]extv1.JSONSchemaProps{ + "objectStore": objSchema(map[string]extv1.JSONSchemaProps{"bucket": strSchema()}), + }), + }) + rs := RuleSet{Rules: []Rule{{Strategy: StrategyBranchMap, Params: BranchMapParams{ + HubPath: ParsePath("backup"), SpokePath: ParsePath("backup"), + Branches: []BranchMapping{ + {HubBranch: "s3", SpokeBranch: "objectStore"}, + {HubBranch: "gcs", SpokeBranch: "objectStore"}, + }, + }}}} + + _, diags, err := Compile(rs, &hub, &spoke) + if err != nil { + t.Fatalf("compile: %v", err) + } + var sawLossy bool + for _, msg := range diagMessages(diags, SeverityError) { + if strings.Contains(msg, "spoke->hub conversion is lossy") { + sawLossy = true + } + } + if !sawLossy { + t.Fatalf("expected the collapse to be reported as lossy spoke->hub, got %v", diagMessages(diags, SeverityError)) + } + + // "Expressible" has to mean it actually compiles once the loss is + // acknowledged. The claim map records paths, not the rules holding + // them, so claiming the shared spoke branch once per mapping would + // have reported this rule as conflicting with itself — leaving no way + // to write a collapse at all. + rs.Rules[0].AcknowledgeLossy = true + plan, diags, err := Compile(rs, &hub, &spoke) + if err != nil { + t.Fatalf("compile: %v", err) + } + if errs := diagMessages(diags, SeverityError); len(errs) != 0 { + t.Fatalf("an acknowledged collapse should compile cleanly, got %v", errs) + } + + // Compiling is only half of "expressible". Both hub branches must + // reach the shared spoke branch... + for _, hubBranch := range []string{"s3", "gcs"} { + hubObj := map[string]any{"backup": map[string]any{hubBranch: map[string]any{"bucket": "logs"}}} + out, err := Convert(ConvertInput{Plan: plan, Direction: HubToSpoke, Object: hubObj}) + if err != nil { + t.Fatalf("hub->spoke from %q: %v", hubBranch, err) + } + want := map[string]any{"objectStore": map[string]any{"bucket": "logs"}} + if got := out["backup"]; !reflect.DeepEqual(got, want) { + t.Errorf("hub->spoke from %q = %#v, want %#v", hubBranch, got, want) + } + } + + // ...and the way back must work at all. branchMapOp identifies the + // active branch by counting entries whose srcBranch is present, so a + // second compiled entry for the shared spoke branch made a valid + // single-branch object look like two branches set at once and failed + // every conversion back. The collapse compiled and could never convert. + spokeObj := map[string]any{"backup": map[string]any{"objectStore": map[string]any{"bucket": "logs"}}} + back, err := Convert(ConvertInput{Plan: plan, Direction: SpokeToHub, Object: spokeObj}) + if err != nil { + t.Fatalf("spoke->hub on a valid single-branch object: %v", err) + } + // The first mapping wins — the deterministic reading of "cannot tell + // which hub branch it started from", and the reason this is lossy. + want := map[string]any{"s3": map[string]any{"bucket": "logs"}} + if got := back["backup"]; !reflect.DeepEqual(got, want) { + t.Errorf("spoke->hub = %#v, want %#v (the first mapping wins)", got, want) + } +} + +func TestBranchMap_RejectsABranchThatIsNotADeclaredProperty(t *testing.T) { + hub, spoke := hubUnion(), spokeUnion() + rs := RuleSet{Rules: []Rule{{Strategy: StrategyBranchMap, Params: BranchMapParams{ + HubPath: ParsePath("backup"), SpokePath: ParsePath("backup"), + Branches: []BranchMapping{{HubBranch: "azure", SpokeBranch: "objectStore"}}, + }}}} + _, diags, err := Compile(rs, &hub, &spoke) + if err != nil { + t.Fatalf("compile: %v", err) + } + var saw bool + for _, msg := range diagMessages(diags, SeverityError) { + if strings.Contains(msg, "azure") && strings.Contains(msg, "declared property") { + saw = true + } + } + if !saw { + t.Fatalf("expected an undeclared branch to be rejected, got %v", diagMessages(diags, SeverityError)) + } +} + +func TestBranchMap_RejectsAnUndeclaredDiscriminator(t *testing.T) { + hub := objSchema(map[string]extv1.JSONSchemaProps{ + "backup": unionSchema("", map[string]extv1.JSONSchemaProps{ + "s3": objSchema(map[string]extv1.JSONSchemaProps{"bucket": strSchema()}), + }), + }) + spoke := objSchema(map[string]extv1.JSONSchemaProps{ + "backup": unionSchema("", map[string]extv1.JSONSchemaProps{ + "objectStore": objSchema(map[string]extv1.JSONSchemaProps{"bucket": strSchema()}), + }), + }) + rs := RuleSet{Rules: []Rule{{Strategy: StrategyBranchMap, Params: BranchMapParams{ + HubPath: ParsePath("backup"), SpokePath: ParsePath("backup"), + Discriminator: "kind", + Branches: []BranchMapping{{HubBranch: "s3", SpokeBranch: "objectStore"}}, + }}}} + _, diags, err := Compile(rs, &hub, &spoke) + if err != nil { + t.Fatalf("compile: %v", err) + } + var saw bool + for _, msg := range diagMessages(diags, SeverityError) { + if strings.Contains(msg, "discriminator") && strings.Contains(msg, "kind") { + saw = true + } + } + if !saw { + t.Fatalf("expected an undeclared discriminator to be rejected, got %v", diagMessages(diags, SeverityError)) + } +} + +// Each side's diagnostic has to name that side's path. The two are +// routinely different — a union that was moved as well as reshaped — and +// an error about the spoke that quotes the hub path sends the reader to a +// schema where the field is not missing. +func TestBranchMap_DiscriminatorDiagnosticNamesTheFailingSidesPath(t *testing.T) { + hub := objSchema(map[string]extv1.JSONSchemaProps{ + "backup": unionSchema("backend", map[string]extv1.JSONSchemaProps{ + "s3": objSchema(map[string]extv1.JSONSchemaProps{"bucket": strSchema()}), + }), + }) + spoke := objSchema(map[string]extv1.JSONSchemaProps{ + "legacyBackup": unionSchema("", map[string]extv1.JSONSchemaProps{ + "objectStore": objSchema(map[string]extv1.JSONSchemaProps{"bucket": strSchema()}), + }), + }) + rs := RuleSet{Rules: []Rule{{Strategy: StrategyBranchMap, Params: BranchMapParams{ + HubPath: ParsePath("backup"), SpokePath: ParsePath("legacyBackup"), + Discriminator: "backend", + Branches: []BranchMapping{{HubBranch: "s3", SpokeBranch: "objectStore"}}, + }}}} + _, diags, err := Compile(rs, &hub, &spoke) + if err != nil { + t.Fatalf("compile: %v", err) + } + var saw bool + for _, msg := range diagMessages(diags, SeverityError) { + if !strings.Contains(msg, "discriminator") { + continue + } + saw = true + if !strings.Contains(msg, "spoke union at \"legacyBackup\"") { + t.Errorf("the spoke diagnostic must name the spoke path, got %q", msg) + } + } + if !saw { + t.Fatalf("expected the missing spoke discriminator to be reported, got %v", diagMessages(diags, SeverityError)) + } +} + +// Before this strategy existed, a union node was one opaque leaf: every +// field inside it was invisible, and jsonPatch was the only way to touch +// one. This asserts the un-hiding directly. +func TestUnion_BranchesAreOrdinaryLeavesNow(t *testing.T) { + hub := hubUnion() + var paths []string + for _, l := range flattenSchema(&hub) { + paths = append(paths, l.Path.String()) + } + want := map[string]bool{"backup.backend": true, "backup.s3.bucket": true, "backup.gcs.bucket": true} + if len(paths) != len(want) { + t.Fatalf("leaves = %v, want the three declared fields", paths) + } + for _, p := range paths { + if !want[p] { + t.Fatalf("unexpected leaf %q in %v", p, paths) + } + } +} + +// The int-or-string shape has no type of its own, so the engine still +// cannot describe it and it stays one opaque leaf named after the +// construct. De-opaquing unions must not have de-opaqued this. +func TestUnion_UntypedStaysOpaque(t *testing.T) { + yes := true + node := extv1.JSONSchemaProps{ + XIntOrString: yes, + AnyOf: []extv1.JSONSchemaProps{{Type: "integer"}, {Type: "string"}}, + } + schema := objSchema(map[string]extv1.JSONSchemaProps{"port": node}) + leaves := flattenSchema(&schema) + if len(leaves) != 1 || leaves[0].Path.String() != "port" { + t.Fatalf("leaves = %+v, want one leaf for port", leaves) + } + if !leaves[0].Opaque || leaves[0].Construct != "anyOf" { + t.Fatalf("leaf = %+v, want an opaque leaf named after anyOf", leaves[0]) + } +} diff --git a/pkg/engine/compile.go b/pkg/engine/compile.go index 157bce6..2028426 100644 --- a/pkg/engine/compile.go +++ b/pkg/engine/compile.go @@ -39,12 +39,29 @@ import ( // callers that want the fail-closed default behavior ("never execute a // plan derived from an invalid analysis") must check for errors themselves // (see Analyze, which enforces this). Compile returns a non-nil error only -// for structural Go-level problems (nil schema), never for validation -// findings — those are reported as Diagnostics. +// when the schemas themselves cannot be made sense of — a nil schema, or +// one whose $ref or allOf cannot be normalised — never for a validation +// finding about the rules, which are reported as Diagnostics. The +// distinction is that a rule finding is about this config, while an +// unresolvable schema construct means there is no field set to judge any +// config against. func Compile(rules RuleSet, hub, spoke *extv1.JSONSchemaProps) (*Plan, []Diagnostic, error) { if hub == nil || spoke == nil { return nil, nil, errors.New("compile: hub and spoke schemas must both be non-nil") } + // Analyze normalises at the SchemaSource boundary; Compile is also a + // public entry point taking raw schemas, so it normalises its own two. + // Doing it twice is a no-op — normalisation is idempotent — and not + // doing it here would make the lower-level API behave differently from + // the higher-level one on the same input. + hub, err := NormalizeSchema(hub) + if err != nil { + return nil, nil, fmt.Errorf("compile: hub schema: %w", err) + } + spoke, err = NormalizeSchema(spoke) + if err != nil { + return nil, nil, fmt.Errorf("compile: spoke schema: %w", err) + } h2s, s2h, _, diags, _ := resolveAndBuildOps(rules.Rules, hub, spoke, effectivePolicy(rules.UnmappedFieldPolicy), 0) return &Plan{HubVersion: rules.HubVersion, SpokeVersion: rules.SpokeVersion, HubToSpoke: h2s, SpokeToHub: s2h}, diags, nil } @@ -224,6 +241,11 @@ func resolveAndBuildOps(rules []Rule, hub, spoke *extv1.JSONSchemaProps, policy rr.HubPaths = []string{p.HubPath.String()} rr.SpokePaths = []string{p.SpokePath.String()} + case BranchMapParams: + h2sOp, s2hOp, lossless, ruleDiags = resolveBranchMap(idx, p, hub, spoke, claimedHub, claimedSpoke, policy, depth) + rr.HubPaths = []string{p.HubPath.String()} + rr.SpokePaths = []string{p.SpokePath.String()} + case CELParams: h2sOp, s2hOp, lossless, ruleDiags = resolveCEL(idx, p, hub, spoke, claimedHub, claimedSpoke) for _, hp := range p.HubPaths { @@ -928,6 +950,177 @@ func qualifyUnderItemsPath(itemsPath FieldPath, relative string) string { return itemsPath.String() + "." + relative } +// resolveBranchMap wires up a union-typed field's branch correspondence. +// +// Coverage is the part that takes the work. A union's branches are +// ordinary declared properties — see structural_facts_test.go — so the +// leftover scan sees every leaf inside every branch and would report all +// of them uncovered. Each mapped branch is therefore claimed as a subtree +// on both sides, and each branch pair's own rules are resolved against the +// two branch schemas exactly the way ForEach resolves an array element's. +// +// A branch the rule does not name stays unclaimed, so it surfaces as an +// ordinary uncovered-field error. That is deliberate and stronger than a +// lossiness warning: a hub branch with no spoke counterpart is data the +// conversion would silently drop, and the author has to say what happens +// to it — map it, or Delete it with acknowledgeLossy. +func resolveBranchMap(idx int, p BranchMapParams, hub, spoke *extv1.JSONSchemaProps, claimedHub, claimedSpoke map[string]bool, policy UnmappedFieldPolicy, depth int) (Op, Op, LosslessVerdict, []Diagnostic) { + var diags []Diagnostic + lossless := LosslessVerdict{HubToSpoke: true, SpokeToHub: true} + + hubNode, err := lookupPath(hub, p.HubPath) + if err != nil { + diags = append(diags, errorf(idx, "rule %d (BranchMap): hub: %v", idx, err)) + } + spokeNode, err := lookupPath(spoke, p.SpokePath) + if err != nil { + diags = append(diags, errorf(idx, "rule %d (BranchMap): spoke: %v", idx, err)) + } + if len(p.Branches) == 0 { + diags = append(diags, errorf(idx, "rule %d (BranchMap): at least one branch mapping is required", idx)) + } + if hubNode == nil || spokeNode == nil { + return nil, nil, lossless, diags + } + if len(hubNode.Properties) == 0 || len(spokeNode.Properties) == 0 { + diags = append(diags, errorf(idx, "rule %d (BranchMap): both paths must resolve to objects with declared properties; a union whose branches are not declared properties is not something the engine can address", idx)) + return nil, nil, lossless, diags + } + + if p.Discriminator != "" { + // A slice, not a map: map iteration order is unspecified, and + // Analyze copies these diagnostics into its report without + // re-sorting, so a config wrong on both sides would report its + // two errors in a different order from run to run. Each side also + // names its own path — the spoke error used to quote HubPath. + for _, side := range []struct { + name string + node *extv1.JSONSchemaProps + path FieldPath + }{ + {"hub", hubNode, p.HubPath}, + {"spoke", spokeNode, p.SpokePath}, + } { + if _, ok := side.node.Properties[p.Discriminator]; !ok { + diags = append(diags, errorf(idx, "rule %d (BranchMap): discriminator %q is not a declared property of the %s union at %q", idx, p.Discriminator, side.name, side.path)) + } + } + } + + var h2sBranches, s2hBranches []compiledBranch + hubSeen, spokeSeen := map[string]bool{}, map[string]bool{} + for bi, b := range p.Branches { + hubBranch, hubOK := hubNode.Properties[b.HubBranch] + if !hubOK { + diags = append(diags, errorf(idx, "rule %d (BranchMap): branch %d: %q is not a declared property of the hub union at %q", idx, bi, b.HubBranch, p.HubPath)) + } + spokeBranch, spokeOK := spokeNode.Properties[b.SpokeBranch] + if !spokeOK { + diags = append(diags, errorf(idx, "rule %d (BranchMap): branch %d: %q is not a declared property of the spoke union at %q", idx, bi, b.SpokeBranch, p.SpokePath)) + } + + // Two hub branches mapping to one spoke branch is expressible and + // sometimes intended (three storage backends collapsing to one + // "objectStore"), but the collapse cannot be undone: coming back, + // the engine cannot tell which hub branch it started from. Same + // reasoning as EnumRemap's non-injective check, and the same + // verdict. + if spokeSeen[b.SpokeBranch] { + lossless.SpokeToHub = false + } + if hubSeen[b.HubBranch] { + lossless.HubToSpoke = false + } + // A collapsed branch is claimed once, by the first mapping that + // names it. The claim map records paths, not the rules that took + // them, so claiming the same subtree twice from inside one rule + // would report this rule as conflicting with itself — and make the + // collapse the paragraph above deliberately allows unexpressible. + firstHubUse, firstSpokeUse := !hubSeen[b.HubBranch], !spokeSeen[b.SpokeBranch] + hubSeen[b.HubBranch], spokeSeen[b.SpokeBranch] = true, true + + if !hubOK || !spokeOK { + continue + } + + hubBranchPath := append(p.HubPath.Clone(), b.HubBranch) + spokeBranchPath := append(p.SpokePath.Clone(), b.SpokeBranch) + if firstHubUse { + diags = append(diags, claimSubtree(claimedHub, hubBranchPath, &hubBranch, idx, "hub")...) + } + if firstSpokeUse { + diags = append(diags, claimSubtree(claimedSpoke, spokeBranchPath, &spokeBranch, idx, "spoke")...) + } + + var nestedH2S, nestedS2H []Op + if len(b.Rules) > 0 { + var nestedDiags []Diagnostic + var nestedVerdict LosslessVerdict + nestedH2S, nestedS2H, _, nestedDiags, nestedVerdict = resolveAndBuildOps(b.Rules, &hubBranch, &spokeBranch, policy, depth) + for _, d := range nestedDiags { + d.Message = fmt.Sprintf("rule %d (BranchMap) branch %q: %s", idx, b.HubBranch, d.Message) + switch d.UncoveredSide { + case UncoveredSideHub: + d.FieldPath = qualifyUnderItemsPath(hubBranchPath, d.FieldPath) + case UncoveredSideSpoke: + d.FieldPath = qualifyUnderItemsPath(spokeBranchPath, d.FieldPath) + } + diags = append(diags, d) + } + lossless = lossless.and(nestedVerdict) + } + + // One compiled branch per *source* name, per direction. A collapse + // names the same spoke branch twice, and branchMapOp identifies + // the active branch by counting entries whose srcBranch is + // present — so two entries for `objectStore` would make a + // perfectly valid single-branch spoke object look like two + // branches set at once, and every conversion back would fail with + // the ambiguity error. The first mapping wins, which is the + // deterministic reading of "the engine cannot tell which hub + // branch it started from". + if firstHubUse { + h2sBranches = append(h2sBranches, compiledBranch{ + srcBranch: b.HubBranch, dstBranch: b.SpokeBranch, + dstDiscriminator: discriminatorValue(b.SpokeDiscriminatorValue, b.SpokeBranch), + nested: nestedH2S, + }) + } + if firstSpokeUse { + s2hBranches = append(s2hBranches, compiledBranch{ + srcBranch: b.SpokeBranch, dstBranch: b.HubBranch, + dstDiscriminator: discriminatorValue(b.HubDiscriminatorValue, b.HubBranch), + nested: nestedS2H, + }) + } + } + + if p.Discriminator != "" { + if d := claim(claimedHub, append(p.HubPath.Clone(), p.Discriminator), idx, "hub"); d != nil { + diags = append(diags, *d) + } + if d := claim(claimedSpoke, append(p.SpokePath.Clone(), p.Discriminator), idx, "spoke"); d != nil { + diags = append(diags, *d) + } + } + + if len(h2sBranches) == 0 { + return nil, nil, lossless, diags + } + h2s := branchMapOp{srcPath: p.HubPath, dstPath: p.SpokePath, discriminator: p.Discriminator, branches: h2sBranches} + s2h := branchMapOp{srcPath: p.SpokePath, dstPath: p.HubPath, discriminator: p.Discriminator, branches: s2hBranches} + return h2s, s2h, lossless, diags +} + +// discriminatorValue defaults an unset discriminator value to the branch's +// own property name, which is what it almost always is. +func discriminatorValue(explicit, branch string) string { + if explicit != "" { + return explicit + } + return branch +} + func resolveTypeCoerce(idx int, p TypeCoerceParams, hub, spoke *extv1.JSONSchemaProps, claimedHub, claimedSpoke map[string]bool) (Op, Op, LosslessVerdict, []Diagnostic) { var diags []Diagnostic hubKind := FieldKindUnknown diff --git a/pkg/engine/compile_test.go b/pkg/engine/compile_test.go index 4968aaf..1f92207 100644 --- a/pkg/engine/compile_test.go +++ b/pkg/engine/compile_test.go @@ -95,11 +95,9 @@ func TestWhen_PathMustExistOnBothSchemas(t *testing.T) { } } -func TestUncovered_NamesOneOfAndRefConstructs(t *testing.T) { - ref := "#/definitions/Payload" +func TestUncovered_NamesOneOfAndAnyOfConstructs(t *testing.T) { hub := objSchema(map[string]extv1.JSONSchemaProps{ "payload": {OneOf: []extv1.JSONSchemaProps{strSchema(), {Type: "object"}}}, - "blob": {Ref: &ref}, "choice": {AnyOf: []extv1.JSONSchemaProps{strSchema(), intSchema()}}, }) spoke := objSchema(map[string]extv1.JSONSchemaProps{ @@ -110,14 +108,11 @@ func TestUncovered_NamesOneOfAndRefConstructs(t *testing.T) { t.Fatalf("unexpected error: %v", err) } errs := diagMessages(diags, SeverityError) - var sawOneOf, sawRef, sawAnyOf bool + var sawOneOf, sawAnyOf bool for _, msg := range errs { if strings.Contains(msg, "oneOf") && strings.Contains(msg, "payload") { sawOneOf = true } - if strings.Contains(msg, "$ref") && strings.Contains(msg, "blob") { - sawRef = true - } if strings.Contains(msg, "anyOf") && strings.Contains(msg, "choice") { sawAnyOf = true } @@ -125,14 +120,32 @@ func TestUncovered_NamesOneOfAndRefConstructs(t *testing.T) { if !sawOneOf { t.Fatalf("expected uncovered diagnostic naming oneOf for payload, got %v", errs) } - if !sawRef { - t.Fatalf("expected uncovered diagnostic naming $ref for blob, got %v", errs) - } if !sawAnyOf { t.Fatalf("expected uncovered diagnostic naming anyOf for choice, got %v", errs) } } +// $ref used to be reported the same way, as "an uncovered field sitting +// inside a construct". It is now resolved before anything reaches the +// coverage scan, so an unresolvable one is an authoring error with a +// message that says which reference and why — which is what an author can +// act on, where the old diagnostic was not. +func TestRef_UnresolvableIsASchemaError(t *testing.T) { + ref := "#/definitions/Payload" + hub := objSchema(map[string]extv1.JSONSchemaProps{"blob": {Ref: &ref}}) + spoke := objSchema(map[string]extv1.JSONSchemaProps{"blob": strSchema()}) + + _, _, err := Compile(RuleSet{Rules: nil}, &hub, &spoke) + if err == nil { + t.Fatal("expected an unresolvable $ref to be an error") + } + for _, want := range []string{"$ref", "Payload", "blob"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error does not mention %q: %v", want, err) + } + } +} + func TestFieldRename_LosslessRoundTrip(t *testing.T) { hub := objSchema(map[string]extv1.JSONSchemaProps{"storageGB": strSchema()}) spoke := objSchema(map[string]extv1.JSONSchemaProps{"storageSize": strSchema()}) diff --git a/pkg/engine/convert_bench_test.go b/pkg/engine/convert_bench_test.go index 9973f43..b6cd6e3 100644 --- a/pkg/engine/convert_bench_test.go +++ b/pkg/engine/convert_bench_test.go @@ -172,28 +172,46 @@ func BenchmarkRouter_SpokeToSpoke_vs_HubHop(b *testing.B) { }}, }, &hubSchema, &v2Spoke) router := &Router{Hub: "v3", Plans: map[string]*Plan{"v1": v1, "v2": v2}} - hubObj := volumesObject(1000) - spokeObj, err := Convert(ConvertInput{Plan: v1, Direction: HubToSpoke, Object: hubObj}) - if err != nil { - b.Fatal(err) - } - b.Run("hub_to_spoke", func(b *testing.B) { - b.ReportAllocs() - for i := 0; i < b.N; i++ { - if _, err := router.Convert(hubObj, "v3", "v1"); err != nil { - b.Fatal(err) - } + // Swept rather than pinned at the 1000-element worst case, because the + // ratio is the whole question and a single point cannot show whether + // it holds. 0-10 volumes is what a composite resource actually + // carries; 1000 is the bound, kept for continuity with the Phase 9 + // measurement. + // + // The answer, across four orders of magnitude, is a flat ~2x: exactly + // 2x the allocations and 2x the bytes at every size. No fixed per-call + // overhead to amortise, nothing super-linear. + // + // The ~2x itself belongs to this fixture, whose two hops cost about + // the same; a strategy mix that is much more expensive in one + // direction would shift it. What does not depend on the fixture is + // that cost(A->B) is exactly cost(A->hub) + cost(hub->B), because that + // is what Router.Convert executes — so a direct plan could save at + // most one hop, whatever a hop costs. See docs/operations/capacity.md. + for _, elements := range []int{0, 1, 5, 10, 100, 1000} { + hubObj := volumesObject(elements) + spokeObj, err := Convert(ConvertInput{Plan: v1, Direction: HubToSpoke, Object: hubObj}) + if err != nil { + b.Fatal(err) } - }) - b.Run("spoke_to_spoke", func(b *testing.B) { - b.ReportAllocs() - for i := 0; i < b.N; i++ { - if _, err := router.Convert(spokeObj, "v1", "v2"); err != nil { - b.Fatal(err) + b.Run(fmt.Sprintf("elements=%d/hub_to_spoke", elements), func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + if _, err := router.Convert(hubObj, "v3", "v1"); err != nil { + b.Fatal(err) + } } - } - }) + }) + b.Run(fmt.Sprintf("elements=%d/spoke_to_spoke", elements), func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + if _, err := router.Convert(spokeObj, "v1", "v2"); err != nil { + b.Fatal(err) + } + } + }) + } } func jsonPatchReplaceRule(field, h2s, s2h string) Rule { diff --git a/pkg/engine/normalize.go b/pkg/engine/normalize.go new file mode 100644 index 0000000..5a4b281 --- /dev/null +++ b/pkg/engine/normalize.go @@ -0,0 +1,465 @@ +/* +Copyright 2026 The declarative-conversion-operator Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package engine + +import ( + "encoding/json" + "errors" + "fmt" + "sort" + "strings" + + extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" +) + +// NormalizeSchema rewrites a schema into the ordinary, junctor-free form +// the rest of this package reasons about: local `$ref` resolved, `allOf` +// merged into its parent. Its output is an ordinary JSONSchemaProps, so +// nothing downstream — flattenSchema, the resolvers, the leftover scan, +// the passthrough tree — needs to know it happened. +// +// **The result is an analysis artifact and is never written back to a +// cluster.** That is what makes the merge below tractable: it has to be +// faithful about the things the engine reads — the field set, its types +// and required-ness, enum vocabularies, and which subtrees are opaque — +// and it may drop the value validations nothing here ever looks at +// (patterns, bounds, formats), because the apiserver enforces those on its +// own and no conversion decision depends on them. +// +// What the two constructs actually mean in a CRD is pinned by +// structural_facts_test.go, against the apiserver's own validator: +// +// - `$ref` is rejected outright in a CRD schema. Resolving it therefore +// serves the offline path only — hand-written YAML handed to convctl — +// where an unresolvable reference is an authoring mistake that deserves +// a message rather than an opaque node. +// - Every property named inside an `allOf` must also be declared outside +// it. A junctor can only ever *constrain* fields the engine already +// sees; it can never introduce one. Merging it is therefore never a +// discovery, it is an un-hiding. +// +// A nil schema normalises to nil. +func NormalizeSchema(schema *extv1.JSONSchemaProps) (*extv1.JSONSchemaProps, error) { + if schema == nil { + return nil, nil //nolint:nilnil // a nil schema normalises to a nil schema; every caller already handles one, and a sentinel error would make "this version has no schema" an error condition it is not + } + n := &normalizer{root: schema, inFlight: map[string]bool{}} + return n.node(schema, nil) +} + +// NormalizedVersions reads a source's versions and normalises every +// schema, so callers that flatten schemas outside Analyze — convctl's +// suggester and rehub — see exactly the field set Analyze reported on. +// Without it, a schema carrying an allOf would flatten one way inside the +// report and another way in the tool reading it. +func NormalizedVersions(src SchemaSource) ([]VersionSchema, error) { + versions, err := src.Versions() + if err != nil { + return nil, err + } + out := make([]VersionSchema, 0, len(versions)) + for _, v := range versions { + normalized, err := NormalizeSchema(v.Schema) + if err != nil { + return nil, fmt.Errorf("version %q: %w", v.Name, err) + } + v.Schema = normalized + out = append(out, v) + } + return out, nil +} + +type normalizer struct { + root *extv1.JSONSchemaProps + // inFlight is the set of $ref pointers currently being resolved. + // A reference that reappears while its own resolution is still on the + // stack is a cycle, and the only alternative to detecting it is + // recursing until the stack runs out. + inFlight map[string]bool +} + +func (n *normalizer) node(s *extv1.JSONSchemaProps, path FieldPath) (*extv1.JSONSchemaProps, error) { + if s == nil { + return nil, nil //nolint:nilnil // see NormalizeSchema + } + if s.Ref != nil && *s.Ref != "" { + return n.resolveRef(s, path) + } + + out := s.DeepCopy() + + // allOf first, so anything it contributes is then walked like the rest. + if len(out.AllOf) > 0 { + branches := out.AllOf + out.AllOf = nil + for i := range branches { + branch, err := n.node(&branches[i], path) + if err != nil { + return nil, err + } + if err := mergeSchema(out, branch, path, i); err != nil { + return nil, err + } + } + } + + if len(out.Properties) > 0 { + props := make(map[string]extv1.JSONSchemaProps, len(out.Properties)) + for name, prop := range out.Properties { + normalized, err := n.node(&prop, append(path.Clone(), name)) + if err != nil { + return nil, err + } + if normalized != nil { + props[name] = *normalized + } + } + out.Properties = props + } + if out.Items != nil && out.Items.Schema != nil { + normalized, err := n.node(out.Items.Schema, append(path.Clone(), "[]")) + if err != nil { + return nil, err + } + out.Items = &extv1.JSONSchemaPropsOrArray{Schema: normalized, JSONSchemas: out.Items.JSONSchemas} + } + if out.AdditionalProperties != nil && out.AdditionalProperties.Schema != nil { + normalized, err := n.node(out.AdditionalProperties.Schema, append(path.Clone(), "{}")) + if err != nil { + return nil, err + } + out.AdditionalProperties = &extv1.JSONSchemaPropsOrBool{Allows: out.AdditionalProperties.Allows, Schema: normalized} + } + return out, nil +} + +// resolveRef replaces a $ref node with the schema it points at. +// +// Only local references are resolvable, and that is not a simplification: +// Kubernetes structural schemas do not permit `$ref` at all, so anything +// this sees comes from a hand-written document, and following a remote one +// would mean fetching a URL during analysis — a correctness problem (the +// answer depends on the network) and a security one (a config could make +// the operator issue requests). +func (n *normalizer) resolveRef(s *extv1.JSONSchemaProps, path FieldPath) (*extv1.JSONSchemaProps, error) { + ref := *s.Ref + if !strings.HasPrefix(ref, "#") { + return nil, fmt.Errorf("schema at %q: $ref %q is not local; only references within the same document (#/...) are supported, because resolving a remote one would make analysis depend on the network", pathOrRoot(path), ref) + } + if extra := refSiblings(s); len(extra) > 0 { + return nil, fmt.Errorf("schema at %q: $ref %q also sets %s; a $ref node is replaced by what it points at, so those would be silently discarded — move them into the referenced schema", pathOrRoot(path), ref, strings.Join(extra, ", ")) + } + if n.inFlight[ref] { + return nil, fmt.Errorf("schema at %q: $ref %q is cyclic; it resolves, directly or indirectly, back to itself", pathOrRoot(path), ref) + } + + target, err := resolvePointer(n.root, ref) + if err != nil { + return nil, fmt.Errorf("schema at %q: %w", pathOrRoot(path), err) + } + + n.inFlight[ref] = true + defer delete(n.inFlight, ref) + return n.node(target, path) +} + +// refSiblings names the fields set alongside a $ref that would be lost by +// replacing the node. Description and title are excluded: they carry no +// meaning the engine reads, and a documented reference is idiomatic. +func refSiblings(s *extv1.JSONSchemaProps) []string { + stripped := s.DeepCopy() + stripped.Ref = nil + stripped.Description = "" + stripped.Title = "" + if stripped.Size() == 0 { + return nil + } + // Marshal rather than reflect over sixty fields: the shape of the + // remainder is exactly what a user needs to see, and JSON gives it for + // free. + raw, err := json.Marshal(stripped) + if err != nil { + return []string{"other fields"} + } + var asMap map[string]any + if err := json.Unmarshal(raw, &asMap); err != nil { + return []string{"other fields"} + } + names := make([]string, 0, len(asMap)) + for k := range asMap { + names = append(names, k) + } + sort.Strings(names) + return names +} + +// resolvePointer walks an RFC 6901 JSON Pointer from the document root. +// Only the containers a JSONSchemaProps actually has are traversable: +// definitions, properties, items, and additionalProperties. +func resolvePointer(root *extv1.JSONSchemaProps, ref string) (*extv1.JSONSchemaProps, error) { + ptr := strings.TrimPrefix(ref, "#") + ptr = strings.TrimPrefix(ptr, "/") + if ptr == "" { + return root, nil + } + cur := root + segs := strings.Split(ptr, "/") + for i := 0; i < len(segs); i++ { + seg := unescapePointerSegment(segs[i]) + switch seg { + case "definitions", "properties": + if i+1 >= len(segs) { + return nil, fmt.Errorf("$ref %q ends at %q without naming a schema", ref, seg) + } + key := unescapePointerSegment(segs[i+1]) + i++ + var ( + next extv1.JSONSchemaProps + ok bool + ) + if seg == "definitions" { + next, ok = cur.Definitions[key] + } else { + next, ok = cur.Properties[key] + } + if !ok { + return nil, fmt.Errorf("$ref %q: no %s named %q at that point in the document", ref, strings.TrimSuffix(seg, "s"), key) + } + cur = &next + case "items": + if cur.Items == nil || cur.Items.Schema == nil { + return nil, fmt.Errorf("$ref %q: no items schema at that point in the document", ref) + } + cur = cur.Items.Schema + case "additionalProperties": + if cur.AdditionalProperties == nil || cur.AdditionalProperties.Schema == nil { + return nil, fmt.Errorf("$ref %q: no additionalProperties schema at that point in the document", ref) + } + cur = cur.AdditionalProperties.Schema + default: + return nil, fmt.Errorf("$ref %q: %q is not a traversable part of a schema (expected definitions, properties, items or additionalProperties)", ref, seg) + } + } + return cur, nil +} + +func unescapePointerSegment(s string) string { + s = strings.ReplaceAll(s, "~1", "/") + return strings.ReplaceAll(s, "~0", "~") +} + +func pathOrRoot(p FieldPath) string { + if len(p) == 0 { + return "(root)" + } + return p.String() +} + +// mergeSchema folds one allOf branch into its parent. +// +// Conflicting constraints are a compile error naming both sides, never a +// last-writer-wins: the apiserver's own behaviour for a contradictory +// allOf is to reject every object, and guessing which half the author +// meant would make the engine's field set disagree with what the cluster +// actually accepts. +// +// Only the fields the engine reasons about are merged — see +// NormalizeSchema for why the rest may be dropped. +func mergeSchema(dst, src *extv1.JSONSchemaProps, path FieldPath, branch int) error { + if src == nil { + return nil + } + where := fmt.Sprintf("schema at %q, allOf[%d]", pathOrRoot(path), branch) + + if src.Type != "" { + if dst.Type != "" && dst.Type != src.Type { + return fmt.Errorf("%s: conflicting type %q against %q on the parent; an allOf cannot narrow a field to two different types", where, src.Type, dst.Type) + } + dst.Type = src.Type + } + // Nullability intersects rather than conflicting. "Must be a string" + // and "may be null" is not a contradiction — it is a string, because a + // value has to satisfy every branch. A branch that carries a type + // fully specifies both, so its nullability constrains the parent's; a + // branch that carries only `nullable: true` cannot widen a parent that + // does not permit null. + // + // Kubernetes rejects `nullable` on a junctor outright, so none of this + // is reachable through a CRD. It is reachable through NormalizeSchema, + // which is exported and takes hand-written offline schemas, and there + // the old behaviour was a hard error on a schema that has a perfectly + // well-defined meaning. + if src.Type != "" { + dst.Nullable = dst.Nullable && src.Nullable + } + if src.XPreserveUnknownFields != nil { + if dst.XPreserveUnknownFields != nil && *dst.XPreserveUnknownFields != *src.XPreserveUnknownFields { + return fmt.Errorf("%s: conflicting x-kubernetes-preserve-unknown-fields against the parent", where) + } + dst.XPreserveUnknownFields = src.XPreserveUnknownFields + } + + if len(src.Enum) > 0 { + merged, err := intersectEnums(dst.Enum, src.Enum) + if err != nil { + return fmt.Errorf("%s: %w", where, err) + } + dst.Enum = merged + } + + if len(src.Required) > 0 { + dst.Required = unionRequired(dst.Required, src.Required) + } + + // A union inside an allOf branch is carried up, not dropped. The + // engine reads oneOf/anyOf — unionConstruct decides whether a node is + // opaque, and branchMap maps the branches — so discarding one here + // would silently hand every later stage a different schema from the + // one the author wrote. + // + // Two unions cannot be merged into one list: `allOf: [{oneOf: A}, + // {oneOf: B}]` means "satisfies A *and* satisfies B", which no single + // oneOf expresses. That is an error naming both, for the same reason + // two conflicting types are. + for _, u := range []struct { + name string + dst, src *[]extv1.JSONSchemaProps + }{ + {"oneOf", &dst.OneOf, &src.OneOf}, + {"anyOf", &dst.AnyOf, &src.AnyOf}, + } { + if len(*u.src) == 0 { + continue + } + if len(*u.dst) > 0 { + return fmt.Errorf("%s: declares %s while the parent already does; two unions over the same node cannot be merged into one, so express the combination as a single %s", where, u.name, u.name) + } + *u.dst = *u.src + } + + if len(src.Properties) > 0 { + if dst.Properties == nil { + dst.Properties = map[string]extv1.JSONSchemaProps{} + } + names := make([]string, 0, len(src.Properties)) + for name := range src.Properties { + names = append(names, name) + } + // Sorted so a conflict is reported against the same property on + // every run, rather than whichever the map iterated to first. + sort.Strings(names) + for _, name := range names { + srcProp := src.Properties[name] + if dstProp, ok := dst.Properties[name]; ok { + merged := dstProp.DeepCopy() + if err := mergeSchema(merged, &srcProp, append(path.Clone(), name), branch); err != nil { + return err + } + dst.Properties[name] = *merged + continue + } + dst.Properties[name] = srcProp + } + } + + if src.Items != nil && src.Items.Schema != nil { + if dst.Items == nil || dst.Items.Schema == nil { + dst.Items = src.Items + } else { + merged := dst.Items.Schema.DeepCopy() + if err := mergeSchema(merged, src.Items.Schema, append(path.Clone(), "[]"), branch); err != nil { + return err + } + dst.Items = &extv1.JSONSchemaPropsOrArray{Schema: merged} + } + } + + if src.AdditionalProperties != nil { + if dst.AdditionalProperties == nil { + dst.AdditionalProperties = src.AdditionalProperties + } else if src.AdditionalProperties.Schema != nil && dst.AdditionalProperties.Schema != nil { + merged := dst.AdditionalProperties.Schema.DeepCopy() + if err := mergeSchema(merged, src.AdditionalProperties.Schema, append(path.Clone(), "{}"), branch); err != nil { + return err + } + dst.AdditionalProperties = &extv1.JSONSchemaPropsOrBool{Allows: dst.AdditionalProperties.Allows, Schema: merged} + } + } + return nil +} + +// intersectEnums is allOf's meaning for a value vocabulary: a value must +// satisfy every branch, so the permitted set is the intersection. An empty +// intersection permits nothing at all, which is a contradiction rather +// than a very strict field. +func intersectEnums(dst, src []extv1.JSON) ([]extv1.JSON, error) { + if len(dst) == 0 { + return src, nil + } + inSrc := make(map[string]bool, len(src)) + for _, v := range src { + inSrc[canonicalJSON(v.Raw)] = true + } + var out []extv1.JSON + for _, v := range dst { + if inSrc[canonicalJSON(v.Raw)] { + out = append(out, v) + } + } + if len(out) == 0 { + return nil, errors.New("its enum shares no value with the parent's, so no value could ever satisfy both") + } + return out, nil +} + +// canonicalJSON reduces an enum entry to a form two equal JSON *values* +// share, so the intersection is over values rather than over bytes. JSON +// Schema enum equality is value equality: `{"a":1,"b":2}` and `{ "b": 2, +// "a": 1 }` are the same member, and comparing Raw made them different — +// turning an enum that intersects perfectly well into "shares no value +// with the parent's", a hard compile error on a valid schema. +// +// encoding/json sorts object keys on the way out, which is what does the +// work here; it also normalises whitespace and number formatting. Input +// that does not parse is compared as its literal bytes, because the +// alternative is claiming two unparseable values are equal. +func canonicalJSON(raw []byte) string { + var v any + if err := json.Unmarshal(raw, &v); err != nil { + return string(raw) + } + out, err := json.Marshal(v) + if err != nil { + return string(raw) + } + return string(out) +} + +func unionRequired(dst, src []string) []string { + seen := make(map[string]bool, len(dst)+len(src)) + out := make([]string, 0, len(dst)+len(src)) + for _, list := range [][]string{dst, src} { + for _, name := range list { + if !seen[name] { + seen[name] = true + out = append(out, name) + } + } + } + sort.Strings(out) + return out +} diff --git a/pkg/engine/normalize_test.go b/pkg/engine/normalize_test.go new file mode 100644 index 0000000..c8602a5 --- /dev/null +++ b/pkg/engine/normalize_test.go @@ -0,0 +1,444 @@ +/* +Copyright 2026 The declarative-conversion-operator Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package engine + +import ( + "reflect" + "sort" + "strings" + "testing" + + extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" +) + +func mustNormalize(t *testing.T, in extv1.JSONSchemaProps) *extv1.JSONSchemaProps { + t.Helper() + out, err := NormalizeSchema(&in) + if err != nil { + t.Fatalf("normalize: %v", err) + } + return out +} + +func leafPaths(schema *extv1.JSONSchemaProps) []string { + var out []string + for _, l := range flattenSchema(schema) { + out = append(out, l.Path.String()) + } + sort.Strings(out) + return out +} + +// The whole point, stated as one test: a node carrying an allOf used to be +// opaque, so every field declared right there in the same node was +// invisible to coverage analysis and unreachable by any rule. +func TestNormalize_AllOfNoLongerHidesDeclaredFields(t *testing.T) { + minLen := int64(1) + in := objSchema(map[string]extv1.JSONSchemaProps{ + "backup": { + Type: "object", + Properties: map[string]extv1.JSONSchemaProps{ + "bucket": strSchema(), + "region": strSchema(), + }, + AllOf: []extv1.JSONSchemaProps{{ + Required: []string{"bucket"}, + Properties: map[string]extv1.JSONSchemaProps{"bucket": {MinLength: &minLen}}, + }}, + }, + }) + + before := leafPaths(&in) + if !reflect.DeepEqual(before, []string{"backup"}) { + t.Fatalf("fixture is wrong: before normalisation the whole node should be one opaque leaf, got %v", before) + } + + after := leafPaths(mustNormalize(t, in)) + if !reflect.DeepEqual(after, []string{"backup.bucket", "backup.region"}) { + t.Fatalf("after normalisation = %v, want the two declared fields", after) + } +} + +func TestNormalize_MergesRequiredAndEnum(t *testing.T) { + out := mustNormalize(t, objSchema(map[string]extv1.JSONSchemaProps{ + "tier": {Type: "string", Enum: []extv1.JSON{jsonRaw(`"a"`), jsonRaw(`"b"`), jsonRaw(`"c"`)}}, + "name": strSchema(), + }, "name")) + + // Nothing to merge; the schema must survive unchanged. + if len(out.Required) != 1 || out.Required[0] != "name" { + t.Fatalf("required = %v, want [name]", out.Required) + } + + withAllOf := objSchema(map[string]extv1.JSONSchemaProps{ + "tier": {Type: "string", Enum: []extv1.JSON{jsonRaw(`"a"`), jsonRaw(`"b"`), jsonRaw(`"c"`)}}, + "name": strSchema(), + }, "name") + withAllOf.AllOf = []extv1.JSONSchemaProps{{ + Required: []string{"tier"}, + // allOf means "and", so the permitted vocabulary is the + // intersection, not the union. + Properties: map[string]extv1.JSONSchemaProps{ + "tier": {Enum: []extv1.JSON{jsonRaw(`"b"`), jsonRaw(`"c"`), jsonRaw(`"d"`)}}, + }, + }} + + merged := mustNormalize(t, withAllOf) + if got := merged.Required; !reflect.DeepEqual(got, []string{"name", "tier"}) { + t.Fatalf("required = %v, want the union [name tier]", got) + } + tier := merged.Properties["tier"] + var vals []string + for _, e := range tier.Enum { + vals = append(vals, string(e.Raw)) + } + if !reflect.DeepEqual(vals, []string{`"b"`, `"c"`}) { + t.Fatalf("enum = %v, want the intersection [\"b\" \"c\"]", vals) + } +} + +// A contradiction must be an error naming both sides, not a silent +// last-writer-wins: the apiserver rejects every object against such a +// schema, and guessing which half the author meant would make the engine's +// field set disagree with what the cluster accepts. +func TestNormalize_ConflictingConstraintsAreAnError(t *testing.T) { + cases := map[string]struct { + in extv1.JSONSchemaProps + want []string + }{ + "type": { + in: objSchema(map[string]extv1.JSONSchemaProps{ + "port": {Type: "integer", AllOf: []extv1.JSONSchemaProps{{Type: "string"}}}, + }), + want: []string{"port", "integer", "string"}, + }, + "enum": { + in: objSchema(map[string]extv1.JSONSchemaProps{ + "tier": { + Type: "string", Enum: []extv1.JSON{jsonRaw(`"a"`)}, + AllOf: []extv1.JSONSchemaProps{{Enum: []extv1.JSON{jsonRaw(`"b"`)}}}, + }, + }), + want: []string{"tier", "no value could ever satisfy both"}, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + _, err := NormalizeSchema(&tc.in) + if err == nil { + t.Fatal("expected a conflict error") + } + for _, want := range tc.want { + if !strings.Contains(err.Error(), want) { + t.Errorf("error does not mention %q: %v", want, err) + } + } + }) + } +} + +func TestNormalize_ResolvesLocalRef(t *testing.T) { + ref := "#/definitions/Endpoint" + in := objSchema(map[string]extv1.JSONSchemaProps{ + "primary": {Ref: &ref}, + "backup": {Ref: &ref}, + }) + in.Definitions = extv1.JSONSchemaDefinitions{ + "Endpoint": objSchema(map[string]extv1.JSONSchemaProps{ + "host": strSchema(), + "port": intSchema(), + }), + } + + got := leafPaths(mustNormalize(t, in)) + want := []string{"backup.host", "backup.port", "primary.host", "primary.port"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("leaves = %v, want %v", got, want) + } +} + +func TestNormalize_RejectsRemoteRef(t *testing.T) { + ref := "https://example.org/schemas/endpoint.json" + in := objSchema(map[string]extv1.JSONSchemaProps{"primary": {Ref: &ref}}) + _, err := NormalizeSchema(&in) + if err == nil { + t.Fatal("expected a remote $ref to be rejected: following one would make analysis depend on the network") + } + if !strings.Contains(err.Error(), "not local") { + t.Fatalf("error should say why: %v", err) + } +} + +func TestNormalize_RejectsUnresolvableRef(t *testing.T) { + ref := "#/definitions/Nope" + in := objSchema(map[string]extv1.JSONSchemaProps{"primary": {Ref: &ref}}) + _, err := NormalizeSchema(&in) + if err == nil || !strings.Contains(err.Error(), "Nope") { + t.Fatalf("expected an error naming the missing definition, got %v", err) + } +} + +// Cycles have to be detected, not recursed into: the alternative is a +// stack overflow taking the process down, on input an author controls. +func TestNormalize_DetectsRefCycle(t *testing.T) { + self := "#/definitions/Node" + in := objSchema(map[string]extv1.JSONSchemaProps{"root": {Ref: &self}}) + in.Definitions = extv1.JSONSchemaDefinitions{ + "Node": objSchema(map[string]extv1.JSONSchemaProps{ + "name": strSchema(), + "child": {Ref: &self}, + }), + } + _, err := NormalizeSchema(&in) + if err == nil || !strings.Contains(err.Error(), "cyclic") { + t.Fatalf("expected a cycle to be reported, got %v", err) + } +} + +func TestNormalize_RejectsRefWithSiblings(t *testing.T) { + ref := "#/definitions/Endpoint" + in := objSchema(map[string]extv1.JSONSchemaProps{ + // The type would be silently discarded by replacing the node. + "primary": {Ref: &ref, Type: "object"}, + }) + in.Definitions = extv1.JSONSchemaDefinitions{"Endpoint": objSchema(map[string]extv1.JSONSchemaProps{"host": strSchema()})} + _, err := NormalizeSchema(&in) + if err == nil || !strings.Contains(err.Error(), "type") { + t.Fatalf("expected the discarded sibling to be named, got %v", err) + } +} + +// Normalisation must be a no-op for the overwhelming majority of schemas, +// which carry neither construct — otherwise every existing config would be +// analysed against a subtly different field set. +func TestNormalize_IsANoOpForAnOrdinarySchema(t *testing.T) { + in := objSchema(map[string]extv1.JSONSchemaProps{ + "name": strSchema(), + "replica": intSchema(), + "nested": objSchema(map[string]extv1.JSONSchemaProps{"a": strSchema()}, "a"), + "list": arrSchema(objSchema(map[string]extv1.JSONSchemaProps{"b": strSchema()}), nil), + }, "name") + + out := mustNormalize(t, in) + if !reflect.DeepEqual(&in, out) { + t.Fatalf("normalisation changed a schema with no $ref and no allOf:\n before: %+v\n after: %+v", in, out) + } +} + +// Normalising twice must give the same answer as normalising once, because +// Compile normalises its own inputs and Analyze has already normalised the +// schemas it hands around. +func TestNormalize_IsIdempotent(t *testing.T) { + minLen := int64(2) + ref := "#/definitions/Endpoint" + in := objSchema(map[string]extv1.JSONSchemaProps{ + "primary": {Ref: &ref}, + "backup": { + Type: "object", + Properties: map[string]extv1.JSONSchemaProps{"bucket": strSchema()}, + AllOf: []extv1.JSONSchemaProps{{Required: []string{"bucket"}, Properties: map[string]extv1.JSONSchemaProps{"bucket": {MinLength: &minLen}}}}, + }, + }) + in.Definitions = extv1.JSONSchemaDefinitions{"Endpoint": objSchema(map[string]extv1.JSONSchemaProps{"host": strSchema()})} + + once := mustNormalize(t, in) + twice, err := NormalizeSchema(once) + if err != nil { + t.Fatalf("second pass: %v", err) + } + if !reflect.DeepEqual(once, twice) { + t.Fatalf("normalisation is not idempotent:\n once: %+v\n twice: %+v", once, twice) + } +} + +// The passthrough tree is built from the same schemas, so a field only +// visible after normalisation must be recognised as declared — otherwise +// passthroughUnknownOp would treat it as an undeclared field and copy it +// verbatim, overwriting whatever a rule had just written there. +func TestNormalize_PassthroughTreeSeesMergedFields(t *testing.T) { + minLen := int64(1) + hub := *mustNormalize(t, objSchema(map[string]extv1.JSONSchemaProps{ + "backup": { + Type: "object", + Properties: map[string]extv1.JSONSchemaProps{"bucket": strSchema()}, + AllOf: []extv1.JSONSchemaProps{{Properties: map[string]extv1.JSONSchemaProps{"bucket": {MinLength: &minLen}}}}, + }, + })) + tree := buildKnownTree(&hub) + backup, ok := tree.children["backup"] + if !ok { + t.Fatal("the known tree has no backup node") + } + if _, ok := backup.children["bucket"]; !ok { + t.Fatal("bucket is not in the known tree, so passthrough would treat it as an undeclared field and clobber a rule's output") + } +} + +// A union inside an allOf branch survives the merge. The engine reads +// oneOf/anyOf — unionConstruct decides opacity, branchMap maps the +// branches — so dropping one during normalisation would hand every later +// stage a schema the author did not write, silently. +func TestNormalize_CarriesAUnionUpFromAnAllOfBranch(t *testing.T) { + for _, tc := range []struct { + name string + build func() *extv1.JSONSchemaProps + got func(*extv1.JSONSchemaProps) []extv1.JSONSchemaProps + }{ + { + name: "oneOf", + build: func() *extv1.JSONSchemaProps { + s := unionParent() + s.AllOf = []extv1.JSONSchemaProps{{OneOf: []extv1.JSONSchemaProps{ + {Required: []string{"s3"}}, {Required: []string{"gcs"}}, + }}} + return s + }, + got: func(s *extv1.JSONSchemaProps) []extv1.JSONSchemaProps { return s.OneOf }, + }, + { + name: "anyOf", + build: func() *extv1.JSONSchemaProps { + s := unionParent() + s.AllOf = []extv1.JSONSchemaProps{{AnyOf: []extv1.JSONSchemaProps{ + {Required: []string{"s3"}}, {Required: []string{"gcs"}}, + }}} + return s + }, + got: func(s *extv1.JSONSchemaProps) []extv1.JSONSchemaProps { return s.AnyOf }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + out, err := NormalizeSchema(tc.build()) + if err != nil { + t.Fatalf("normalize: %v", err) + } + if len(tc.got(out)) != 2 { + t.Fatalf("%s was discarded by the merge: got %d branches, want 2", tc.name, len(tc.got(out))) + } + if len(out.AllOf) != 0 { + t.Errorf("the allOf itself should be gone, got %d branches", len(out.AllOf)) + } + // The whole point: the union is still visible to the code that + // reads it. + if c := unionConstruct(out); c != tc.name { + t.Errorf("unionConstruct = %q, want %q", c, tc.name) + } + }) + } +} + +// Two unions over one node cannot become one list: `allOf: [{oneOf: A}]` +// on a parent that already has a oneOf means "satisfies both", which no +// single oneOf expresses. An error naming it, for the same reason two +// conflicting types are an error rather than last-writer-wins. +func TestNormalize_RejectsTwoUnionsOverTheSameNode(t *testing.T) { + s := unionParent() + s.OneOf = []extv1.JSONSchemaProps{{Required: []string{"s3"}}} + s.AllOf = []extv1.JSONSchemaProps{{OneOf: []extv1.JSONSchemaProps{{Required: []string{"gcs"}}}}} + _, err := NormalizeSchema(s) + if err == nil { + t.Fatal("expected two unions over one node to be rejected") + } + if !strings.Contains(err.Error(), "oneOf") { + t.Errorf("the error should name the construct, got %v", err) + } +} + +func unionParent() *extv1.JSONSchemaProps { + return &extv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]extv1.JSONSchemaProps{ + "s3": {Type: "object", Properties: map[string]extv1.JSONSchemaProps{"bucket": {Type: "string"}}}, + "gcs": {Type: "object", Properties: map[string]extv1.JSONSchemaProps{"bucket": {Type: "string"}}}, + }, + } +} + +// Enum intersection is over JSON *values*, not over the bytes they were +// written as. JSON Schema treats `{"a":1,"b":2}` and `{ "b": 2, "a": 1 }` +// as the same enum member; comparing Raw made them different and turned an +// enum that intersects perfectly well into a hard compile error. +func TestNormalize_IntersectsEnumsByValueNotByBytes(t *testing.T) { + for _, tc := range []struct{ name, parent, branch string }{ + {"object key order", `{"a":1,"b":2}`, `{ "b": 2, "a": 1 }`}, + {"whitespace", `["x","y"]`, `[ "x", "y" ]`}, + {"number formatting", `1`, `1.0`}, + } { + t.Run(tc.name, func(t *testing.T) { + s := &extv1.JSONSchemaProps{ + Enum: []extv1.JSON{{Raw: []byte(tc.parent)}}, + AllOf: []extv1.JSONSchemaProps{{Enum: []extv1.JSON{{Raw: []byte(tc.branch)}}}}, + } + out, err := NormalizeSchema(s) + if err != nil { + t.Fatalf("two spellings of the same value must intersect: %v", err) + } + if len(out.Enum) != 1 { + t.Fatalf("enum = %v, want the one shared value", out.Enum) + } + // The parent's spelling is kept, so the surviving entry is a + // representative rather than a re-serialised approximation. + if string(out.Enum[0].Raw) != tc.parent { + t.Errorf("kept %q, want the parent's own entry %q", out.Enum[0].Raw, tc.parent) + } + }) + } +} + +// ...and genuinely disjoint enums are still a contradiction. +func TestNormalize_StillRejectsDisjointEnums(t *testing.T) { + s := &extv1.JSONSchemaProps{ + Enum: []extv1.JSON{{Raw: []byte(`"a"`)}}, + AllOf: []extv1.JSONSchemaProps{{Enum: []extv1.JSON{{Raw: []byte(`"b"`)}}}}, + } + if _, err := NormalizeSchema(s); err == nil { + t.Fatal("expected disjoint enums to be rejected") + } +} + +// Nullability intersects. "Must be a string" and "may be null" is not a +// contradiction — it is a string, because a value has to satisfy every +// branch. Kubernetes rejects nullable on a junctor outright, so this is +// only reachable through the exported offline path, where the old +// behaviour was a hard error on a well-defined schema. +func TestNormalize_IntersectsNullability(t *testing.T) { + for _, tc := range []struct { + name string + parentNullable bool + branch extv1.JSONSchemaProps + want bool + }{ + {"nullable branch cannot widen a non-nullable parent", false, extv1.JSONSchemaProps{Nullable: true}, false}, + {"a typed non-nullable branch narrows a nullable parent", true, extv1.JSONSchemaProps{Type: "string"}, false}, + {"both nullable stays nullable", true, extv1.JSONSchemaProps{Type: "string", Nullable: true}, true}, + {"an untyped branch says nothing about nullability", true, extv1.JSONSchemaProps{Enum: []extv1.JSON{{Raw: []byte(`"a"`)}}}, true}, + } { + t.Run(tc.name, func(t *testing.T) { + s := &extv1.JSONSchemaProps{ + Type: "string", Nullable: tc.parentNullable, + AllOf: []extv1.JSONSchemaProps{tc.branch}, + } + out, err := NormalizeSchema(s) + if err != nil { + t.Fatalf("normalize: %v", err) + } + if out.Nullable != tc.want { + t.Errorf("Nullable = %v, want %v", out.Nullable, tc.want) + } + }) + } +} diff --git a/pkg/engine/ops.go b/pkg/engine/ops.go index dcfa6ea..17d7139 100644 --- a/pkg/engine/ops.go +++ b/pkg/engine/ops.go @@ -809,3 +809,103 @@ func (o splitListOp) apply(ctx *execContext) error { } return setValue(ctx.output, o.arrayPath, out) } + +// branchMapOp converts a union-typed field by finding which branch the +// input has set, writing the corresponding branch on the other side, and +// remapping the discriminator if there is one. +// +// It writes each branch at its own path rather than replacing the union +// object wholesale. A union object can carry properties that are not +// branches at all — a retention period alongside `s3` and `gcs` — and +// those are covered by ordinary rules or by an identityOp. Setting the +// whole object would make the result depend on rule order, which nothing +// else in this engine does. +type branchMapOp struct { + srcPath, dstPath FieldPath + discriminator string + branches []compiledBranch +} + +// compiledBranch is one resolved branch correspondence in one direction. +type compiledBranch struct { + srcBranch, dstBranch string + // srcDiscriminator is unused at runtime — the branch is identified by + // presence — but dstDiscriminator is what gets written when the rule + // declares a discriminator. + dstDiscriminator string + nested []Op +} + +func (o branchMapOp) apply(ctx *execContext) error { + v, ok := getValue(ctx.input, o.srcPath) + if !ok { + return nil + } + obj, ok := v.(map[string]any) + if !ok { + return fmt.Errorf("branchMap: value at %q is not an object", o.srcPath) + } + + var present []compiledBranch + for _, b := range o.branches { + if _, has := obj[b.srcBranch]; has { + present = append(present, b) + } + } + + // Fail closed, in both directions, for the same reason + // arrayToMapByKey does on a duplicate key: the alternative to an error + // is producing an object that is wrong in a way nothing downstream can + // see. A union with no branch set converts to a union with no branch + // set, which the destination's own oneOf then rejects at admission + // with a message about the schema rather than about the conversion. + switch len(present) { + case 1: + case 0: + return fmt.Errorf("branchMap: no branch is set at %q; exactly one of %v must be", o.srcPath, o.branchNames()) + default: + var names []string + for _, b := range present { + names = append(names, b.srcBranch) + } + sort.Strings(names) + return fmt.Errorf("branchMap: branches %v are all set at %q; exactly one must be", names, o.srcPath) + } + + active := present[0] + value := obj[active.srcBranch] + if len(active.nested) > 0 { + branchObj, ok := value.(map[string]any) + if !ok { + return fmt.Errorf("branchMap: branch %q at %q is not an object, so its nested rules cannot apply", active.srcBranch, o.srcPath) + } + branchCtx := &execContext{input: branchObj, output: map[string]any{}} + for _, op := range active.nested { + if err := op.apply(branchCtx); err != nil { + return fmt.Errorf("branchMap: branch %q: %w", active.srcBranch, err) + } + } + value = branchCtx.output + } else { + value = deepCopyValue(value) + } + + if err := setValue(ctx.output, append(o.dstPath.Clone(), active.dstBranch), value); err != nil { + return err + } + if o.discriminator != "" { + if err := setValue(ctx.output, append(o.dstPath.Clone(), o.discriminator), active.dstDiscriminator); err != nil { + return err + } + } + return nil +} + +func (o branchMapOp) branchNames() []string { + names := make([]string, 0, len(o.branches)) + for _, b := range o.branches { + names = append(names, b.srcBranch) + } + sort.Strings(names) + return names +} diff --git a/pkg/engine/rules.go b/pkg/engine/rules.go index 8862f84..b436177 100644 --- a/pkg/engine/rules.go +++ b/pkg/engine/rules.go @@ -49,6 +49,7 @@ const ( StrategyDuration Strategy = "Duration" StrategyMapKeyRename Strategy = "MapKeyRename" StrategyCEL Strategy = "CEL" + StrategyBranchMap Strategy = "BranchMap" ) // UnmappedFieldPolicy controls what happens when a field exists in a hub or @@ -458,6 +459,47 @@ type CELParams struct { func (CELParams) isRuleParams() {} +// BranchMapParams maps the branches of a union-typed field between hub and +// spoke — the "one of s3, gcs or azure" shape that mature platform APIs +// express with `oneOf`. +// +// **The branch is identified by which property is present**, not by +// validating the object against each branch schema. That is not a +// simplification of the general case, it is what a union looks like in a +// legal CRD: the apiserver requires every property named inside a `oneOf` +// to also be declared in the parent's own properties, so a union is a set +// of declared, optional, mutually-exclusive fields and the `oneOf` only +// says which of them may be set. Structural matching would mean running a +// full JSON Schema validator on the admission critical path to learn +// something a map lookup already knows. +// +// Discriminator, when set, names a sibling property whose value also +// identifies the branch (`backend: s3`). It is remapped alongside, so hub +// and spoke may spell their branch names differently. +// +// Neither direction writes the union object wholesale: each branch is +// written at its own path, so a rule covering some *other* property of the +// same object cannot be clobbered by rule ordering. +type BranchMapParams struct { + HubPath, SpokePath FieldPath + Discriminator string + Branches []BranchMapping +} + +func (BranchMapParams) isRuleParams() {} + +// BranchMapping is one branch correspondence. Rules, when present, are +// resolved against the two branches' own schemas with paths relative to +// the branch — the same scoping ForEach gives an array element. +type BranchMapping struct { + HubBranch, SpokeBranch string + // HubDiscriminatorValue/SpokeDiscriminatorValue are used only when + // BranchMapParams.Discriminator is set. Empty means "the branch name", + // which is the common case. + HubDiscriminatorValue, SpokeDiscriminatorValue string + Rules []Rule +} + // RuleSet is every rule declared for one hub<->spoke version pair. type RuleSet struct { HubVersion, SpokeVersion string diff --git a/pkg/engine/schema.go b/pkg/engine/schema.go index 03e7a8a..b0facdc 100644 --- a/pkg/engine/schema.go +++ b/pkg/engine/schema.go @@ -99,6 +99,10 @@ type LeafField struct { // Exported for tooling that has to reason about uncovered fields in the // same terms Analyze does — notably convctl's rule-stub suggester, which // needs a leaf's FieldKind to tell a rename apart from a type coercion. +// +// It expects an already-normalised schema. Callers reading schemas +// straight from an adapter should go through NormalizedVersions, or they +// will flatten a schema differently from the way Analyze reported on it. func FlattenSchema(schema *extv1.JSONSchemaProps) []LeafField { return flattenSchema(schema) } @@ -119,15 +123,39 @@ func flattenInto(schema *extv1.JSONSchemaProps, path FieldPath, requiredSet map[ if schema == nil { return } - if c := schemaConstruct(schema); c != "" { - kind, _ := classify(schema) + kind, opaque := classify(schema) + + // $ref and allOf are gone by this point — NormalizeSchema removes + // them — so a node still carrying one has been reached without going + // through normalisation. Staying opaque is the loud answer to that: + // flattening it anyway would be *mostly* right and would silently + // drop whatever the construct contributed. + if c := unresolvedConstruct(schema); c != "" { + *out = append(*out, LeafField{ + Path: path.Clone(), Kind: kind, Schema: schema, Opaque: true, + Construct: c, Required: requiredSet[lastSegment(path)], + }) + return + } + + if c := unionConstruct(schema); c != "" && !isStructuralNode(kind, opaque) { + // A union on a node the engine cannot otherwise classify — the + // int-or-string shape, `anyOf: [{type: integer}, {type: + // string}]`, which has no type of its own — stays one opaque + // leaf, named after the construct so the diagnostic says why. + // + // A union over *declared properties* does not. The apiserver + // requires every property named inside a junctor to also be + // declared outside it (structural_facts_test.go), so the fields + // of a union-typed object are right there in the same node. + // Hiding them behind the construct hid fields that were never + // ambiguous, and left `jsonPatch` as the only way to touch one. *out = append(*out, LeafField{ Path: path.Clone(), Kind: kind, Schema: schema, Opaque: true, Construct: c, Required: requiredSet[lastSegment(path)], }) return } - kind, opaque := classify(schema) switch { case opaque: @@ -201,24 +229,45 @@ func classify(schema *extv1.JSONSchemaProps) (FieldKind, bool) { } } -// schemaConstruct reports a JSON Schema combinator the engine does not -// flatten through. Empty means an ordinary typed node. -func schemaConstruct(schema *extv1.JSONSchemaProps) string { +// isStructuralNode reports whether the engine can describe a node's shape +// on its own terms — an object with declared properties, an array with a +// known item schema, or a scalar. A node that is opaque or unclassifiable +// is not structural, whatever constructs it carries. +func isStructuralNode(kind FieldKind, opaque bool) bool { + return !opaque && kind != FieldKindUnknown +} + +// unresolvedConstruct reports a combinator NormalizeSchema should already +// have removed. Seeing one means a caller flattened a schema without +// normalising it first. +func unresolvedConstruct(schema *extv1.JSONSchemaProps) string { if schema == nil { return "" } if schema.Ref != nil && *schema.Ref != "" { return "$ref" } + if len(schema.AllOf) > 0 { + return "allOf" + } + return "" +} + +// unionConstruct reports a combinator that expresses a choice between +// shapes. Unlike the two above, these legitimately survive normalisation: +// a union is not something that can be merged away, only reasoned about — +// by branchMap, or by ordinary rules over the properties it selects +// between. +func unionConstruct(schema *extv1.JSONSchemaProps) string { + if schema == nil { + return "" + } if len(schema.OneOf) > 0 { return "oneOf" } if len(schema.AnyOf) > 0 { return "anyOf" } - if len(schema.AllOf) > 0 { - return "allOf" - } return "" } diff --git a/pkg/engine/structural_facts_test.go b/pkg/engine/structural_facts_test.go new file mode 100644 index 0000000..e06c2e1 --- /dev/null +++ b/pkg/engine/structural_facts_test.go @@ -0,0 +1,167 @@ +/* +Copyright 2026 The declarative-conversion-operator Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package engine + +import ( + "strings" + "testing" + + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apischema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +// This file is the investigation 16.2 asks for, written as a test rather +// than as prose: what does apiextensions ACTUALLY accept in a CRD schema? +// +// It matters because the answer bounds the whole normalisation problem. It +// is asserted against the apiserver's own validator — the same +// k8s.io/apiextensions-apiserver code a real cluster runs — so the answer +// cannot drift away from reality without this failing. Designing against a +// reading of the documentation would have been guessing. +// +// The findings, and what the engine does with each: +// +// 1. `$ref` is rejected outright, anywhere in a CRD schema. So resolving +// it is not about CRDs at all — it is about the hand-written YAML +// convctl is handed offline, where a `$ref` is the author's mistake +// and deserves a clear message rather than an opaque node. +// +// 2. A logical junctor (`allOf`, `anyOf`, `oneOf`, `not`) may not carry +// structural fields: no `type`, no `additionalProperties`, no +// `default`, no `nullable`. +// +// 3. Every property named inside a junctor must ALSO be declared in the +// structural schema outside it. This is the decisive one: it means a +// junctor can only ever add value validations to fields the engine can +// already see. There is no such thing, in a legal CRD, as a field that +// exists only inside an `allOf` or a `oneOf` branch. +// +// (3) is why the engine's treatment of these nodes was wrong rather than +// merely incomplete: `schemaConstruct` marked any node carrying one as +// opaque, hiding properties that were fully declared right there in the +// same node. + +// validateAsCRDSchema runs a v1 JSONSchemaProps through the apiserver's own +// structural-schema checks and returns the errors, joined. +func validateAsCRDSchema(t *testing.T, in *extv1.JSONSchemaProps) string { + t.Helper() + var internal apiextensions.JSONSchemaProps + if err := extv1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(in, &internal, nil); err != nil { + t.Fatalf("converting to the internal type: %v", err) + } + s, err := apischema.NewStructural(&internal) + if err != nil { + // NewStructural itself rejects the unsupported keywords, $ref + // among them, before structurality is even considered. + return err.Error() + } + var msgs []string + for _, e := range apischema.ValidateStructural(field.NewPath("root"), s) { + msgs = append(msgs, e.Error()) + } + return strings.Join(msgs, "; ") +} + +func TestApiextensions_RejectsRef(t *testing.T) { + ref := "#/definitions/Thing" + got := validateAsCRDSchema(t, &extv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]extv1.JSONSchemaProps{ + "thing": {Ref: &ref}, + }, + }) + if !strings.Contains(got, "$ref") { + t.Fatalf("expected $ref to be rejected, got %q", got) + } +} + +func TestApiextensions_AcceptsAllOfCarryingOnlyValueValidations(t *testing.T) { + minLen := int64(1) + got := validateAsCRDSchema(t, &extv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]extv1.JSONSchemaProps{ + "name": {Type: "string"}, + }, + // A constraint on a property that IS declared outside. + AllOf: []extv1.JSONSchemaProps{{ + Properties: map[string]extv1.JSONSchemaProps{"name": {MinLength: &minLen}}, + }}, + }) + if got != "" { + t.Fatalf("expected an allOf of pure value validations to be accepted, got %q", got) + } +} + +// The decisive fact. A CRD cannot introduce a field inside a junctor, so +// flattening through one can never reveal a field the engine could not +// already see — which is why normalisation is about un-hiding what is +// already there, not about discovering anything new. +func TestApiextensions_RejectsAPropertyDeclaredOnlyInsideAllOf(t *testing.T) { + minLen := int64(1) + got := validateAsCRDSchema(t, &extv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]extv1.JSONSchemaProps{"name": {Type: "string"}}, + AllOf: []extv1.JSONSchemaProps{{ + Properties: map[string]extv1.JSONSchemaProps{"undeclared": {MinLength: &minLen}}, + }}, + }) + if !strings.Contains(got, "undeclared") { + t.Fatalf("expected a property declared only inside allOf to be rejected, got %q", got) + } +} + +func TestApiextensions_RejectsStructuralFieldsInsideAJunctor(t *testing.T) { + for name, branch := range map[string]extv1.JSONSchemaProps{ + "type": {Type: "string"}, + "additionalProperties": {AdditionalProperties: &extv1.JSONSchemaPropsOrBool{Allows: true}}, + "nullable": {Nullable: true}, + } { + t.Run(name, func(t *testing.T) { + got := validateAsCRDSchema(t, &extv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]extv1.JSONSchemaProps{"name": {Type: "string"}}, + AllOf: []extv1.JSONSchemaProps{branch}, + }) + if got == "" { + t.Fatalf("expected %q inside allOf to be rejected as non-structural", name) + } + }) + } +} + +// What a union actually looks like in a legal CRD, and the shape 16.1's +// branchMap is built against: every branch is an ordinary declared +// property, and the oneOf only says which of them may be present. +func TestApiextensions_AcceptsAPresenceDiscriminatedUnion(t *testing.T) { + got := validateAsCRDSchema(t, &extv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]extv1.JSONSchemaProps{ + "backend": {Type: "string", Enum: []extv1.JSON{{Raw: []byte(`"s3"`)}, {Raw: []byte(`"gcs"`)}}}, + "s3": {Type: "object", Properties: map[string]extv1.JSONSchemaProps{"bucket": {Type: "string"}}}, + "gcs": {Type: "object", Properties: map[string]extv1.JSONSchemaProps{"bucket": {Type: "string"}}}, + }, + OneOf: []extv1.JSONSchemaProps{ + {Required: []string{"s3"}}, + {Required: []string{"gcs"}}, + }, + }) + if got != "" { + t.Fatalf("expected a presence-discriminated union to be accepted, got %q", got) + } +} diff --git a/pkg/engine/testutil_test.go b/pkg/engine/testutil_test.go index 7b83106..4abb093 100644 --- a/pkg/engine/testutil_test.go +++ b/pkg/engine/testutil_test.go @@ -61,3 +61,16 @@ func diagMessages(diags []Diagnostic, sev Severity) []string { } return out } + +// jsonRaw wraps a raw JSON literal for an enum entry. +func jsonRaw(s string) extv1.JSON { return extv1.JSON{Raw: []byte(s)} } + +// withProp returns a copy of schema with one extra declared property. +func withProp(schema extv1.JSONSchemaProps, name string, prop extv1.JSONSchemaProps) extv1.JSONSchemaProps { + out := *schema.DeepCopy() + if out.Properties == nil { + out.Properties = map[string]extv1.JSONSchemaProps{} + } + out.Properties[name] = prop + return out +}