-
Notifications
You must be signed in to change notification settings - Fork 81
OSAC-1416: enforce field_definitions as complete contract on catalog item usage #783
Changes from all commits
7ebc31c
0d4dc54
301c039
5ccff76
08db59c
2c695a2
8d1cb41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,10 +26,10 @@ in two ways: | |
|
|
||
| - `osac create cluster --template <id>` — direct template access, no field restrictions. Supports | ||
| `--template-parameter` to pass custom values (e.g., `vpc_id`, `vlan`) that AAP uses for | ||
| provisioning | ||
| provisioning. | ||
| - `osac create cluster --catalog-item <id>` — the server resolves the template from the catalog | ||
| item and applies `field_definitions` to enforce defaults and validation. Only the fixed set of | ||
| known fields can be controlled; custom template parameters are not supported | ||
| known fields can be controlled; custom template parameters are not supported. | ||
|
|
||
| Both templates and catalog items are created and managed by the platform admin through the private | ||
| API. The distinction is not one of roles but of purpose: templates define what infrastructure | ||
|
|
@@ -44,18 +44,17 @@ Catalog items control a **fixed set of known fields** on the resource (e.g., `pu | |
| flags (`--pull-secret`, `--ssh-public-key`, `--pod-cidr`). | ||
|
|
||
| **You cannot define custom parameters in a catalog item.** For example, you cannot create a | ||
| `field_definition` with `path: vlan` or `path: vpc_id` and expect AAP to receive it. If a path | ||
| does not correspond to a known field, the value is silently discarded by the server — it has no | ||
| effect on provisioning. | ||
| `field_definition` with `path: vlan` or `path: vpc_id` and expect AAP to receive it. Paths must | ||
| correspond to known fields in the resource spec — unknown paths have no effect on provisioning. | ||
|
|
||
| Custom provisioning parameters (like `vpc_id`, `ip_block_id`, `ssh_key_group_id`) are defined as | ||
| **template parameters**, which are a separate mechanism. Template parameters are passed by the user | ||
| with `--template-parameter` and forwarded to AAP as extra variables. However, `--template-parameter` | ||
| is **not supported with `--catalog-item`**, which means: | ||
|
|
||
| - Catalog items work well with templates that only need the standard spec fields | ||
| - Catalog items work well with templates that only need the standard spec fields. | ||
| - Templates that require custom parameters (e.g., `osac.templates.ocp_4_20_small_nico`) cannot be | ||
| fully used through catalog items — users must use `--template` directly instead | ||
| fully used through catalog items — users must use `--template` directly instead. | ||
|
|
||
| ## Creating Catalog Items | ||
|
|
||
|
|
@@ -136,14 +135,43 @@ description: General-purpose virtual machine with KubeVirt. | |
| template: "osac.templates.ocp_virt_vm" | ||
| published: true | ||
| field_definitions: | ||
| - path: ssh_public_key | ||
| display_name: SSH Public Key | ||
| - path: ssh_key | ||
| display_name: SSH Key | ||
| editable: true | ||
| default: "ssh-ed25519 AAAA..." | ||
| - path: host_type | ||
| display_name: Host Type | ||
| - path: image.source_type | ||
| default: registry | ||
| display_name: Image Source Type | ||
| - path: image.source_ref | ||
| default: quay.io/containerdisks/fedora:latest | ||
| display_name: Image Reference | ||
|
alosadagrande marked this conversation as resolved.
|
||
| - path: boot_disk.size_gib | ||
| default: 20 | ||
| display_name: Boot Disk Size (GiB) | ||
| editable: true | ||
| default: "fc430" | ||
| - path: instance_type | ||
| display_name: Instance Type | ||
| editable: true | ||
| - path: run_strategy | ||
| display_name: Run Strategy | ||
| editable: true | ||
| - path: network_attachments | ||
| display_name: Network Attachments | ||
| editable: true | ||
| ``` | ||
| > IMPORTANT: Notice that ComputeInstanceCatalogItem uses instance type. So, in order to use it you'll need to have an instance type. For example: | ||
|
alosadagrande marked this conversation as resolved.
|
||
|
|
||
| ```yaml | ||
| '@type': type.googleapis.com/osac.private.v1.InstanceType | ||
| id: simple-1-2 | ||
| metadata: | ||
| creator: system | ||
| name: simple-1-2 | ||
| tenant: shared | ||
| spec: | ||
| cores: 1 | ||
| memory_gib: 2 | ||
| state: INSTANCE_TYPE_STATE_ACTIVE | ||
| ``` | ||
|
|
||
| ### Create the catalog item | ||
|
|
@@ -171,8 +199,9 @@ default, optionally constrained by a `validation_schema`. | |
|
|
||
| ### Available Paths | ||
|
|
||
| The following paths can be used in `field_definitions`. These are the only values that have an | ||
| effect — any other path is silently ignored. | ||
| The following paths can be used in `field_definitions`. When a user creates a resource from a | ||
| catalog item, the server rejects any spec field not listed in `field_definitions` with an | ||
| `InvalidArgument` error. | ||
|
|
||
| **ClusterCatalogItem** paths: | ||
|
|
||
|
|
@@ -191,8 +220,7 @@ effect — any other path is silently ignored. | |
| | Path | Description | | ||
| |------|-------------| | ||
| | `ssh_key` | SSH public key | | ||
| | `cores` | Number of CPU cores | | ||
| | `memory_gib` | Memory in GiB | | ||
| | `instance_type` | Instance Type includes number of CPU cores, memory, etc. | | ||
| | `run_strategy` | VM run strategy (e.g., `Always`, `Halted`) | | ||
| | `user_data` | Cloud-init or ignition user data | | ||
| | `image.source_type` | Image source type (e.g., `registry`) | | ||
|
|
@@ -232,14 +260,16 @@ osac create cluster --catalog-item dev-sandbox \ | |
| CLI flags like `--pull-secret` and `--ssh-public-key` set values in the resource spec. However, | ||
| the server applies `field_definitions` **after** receiving the request, which means: | ||
|
|
||
| - If a field is **non-editable** in the catalog item, the CLI flag value is **overridden** by the | ||
| catalog item's default — the user's value is silently discarded. | ||
| - If a field is **not listed** in `field_definitions`, the server **rejects** the request with | ||
|
ygalblum marked this conversation as resolved.
|
||
| `InvalidArgument`. | ||
| - If a field is **non-editable** in the catalog item and the user provides a value, the server | ||
| **rejects** the request with `InvalidArgument`. | ||
| - If a field is **editable**, the CLI flag value is accepted and validated against | ||
| `validation_schema` if one is defined. | ||
| - If an editable field is **not provided** by the user, the catalog item's default is applied. | ||
|
Comment on lines
+263
to
269
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Document that editable fields without a default require user input. The doc states editable fields without a user value get the default applied (lines 269, 284), but the code returns 📝 Suggested additionsAdd a bullet to the "How CLI flags interact" section (after line 269): - If an editable field is **not provided** by the user, the catalog item's default is applied.
+- If an editable field has **no default** and the user does not provide a value, the server
+ **rejects** the request with `InvalidArgument` — the field is required.And update the "Server-side processing" section (line 284): - **Editable fields without a user value**: the default is applied
+ (or rejected with `InvalidArgument` if no default is defined)Also applies to: 281-284 🤖 Prompt for AI Agents |
||
|
|
||
| For example, given a catalog item with `ssh_public_key` set as non-editable with a fixed default, | ||
| running `--ssh-public-key "my-key"` has no effect — the catalog item's default is always used. | ||
| For example, given a catalog item with `release_image` set as non-editable with a fixed default, | ||
| running `--release-image "quay.io/user1/ocp-release:4.12.0"` results in an error — the user cannot override locked fields. | ||
|
|
||
| ### Server-side processing | ||
|
|
||
|
|
@@ -248,7 +278,8 @@ When the server processes the request, it: | |
| 1. Looks up the catalog item and verifies it is published | ||
| 2. Sets the resource's `spec.template` to the template referenced by the catalog item | ||
| 3. Applies `field_definitions`: | ||
| - **Non-editable fields**: the default value is enforced, overriding any user-provided value | ||
| - **Unlisted fields**: any spec field not in `field_definitions` is rejected (`InvalidArgument`) | ||
| - **Non-editable fields**: rejects if the user provided a value; otherwise applies the default | ||
| - **Editable fields with a user value**: validated against `validation_schema` if present | ||
| - **Editable fields without a user value**: the default is applied | ||
| 4. Validates the resulting spec and creates the resource | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ import ( | |
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
| "slices" | ||
| "strings" | ||
| "sync" | ||
| "time" | ||
|
|
@@ -66,8 +67,9 @@ type catalogItem interface { | |
| GetMetadata() *privatev1.Metadata | ||
| } | ||
|
|
||
| // applyFieldDefinitions processes field definitions from a catalog item against a resource spec. | ||
| // For non-editable fields: overrides user-provided values with the catalog item default. | ||
| // applyFieldDefinitions validates and applies field definitions from a catalog item against a resource spec. | ||
| // Rejects any spec field not listed in field_definitions (except system fields catalog_item, template and template_parameters). | ||
| // For non-editable fields: rejects user-provided values; applies the catalog item default. | ||
| // For editable fields with user values: validates against the JSON Schema. | ||
| // For editable fields without user values: applies the catalog item default. | ||
| func applyFieldDefinitions( | ||
|
|
@@ -89,6 +91,28 @@ func applyFieldDefinitions( | |
| return grpcstatus.Errorf(grpccodes.Internal, "failed to parse spec: %v", err) | ||
| } | ||
|
|
||
| allowedPaths := map[string]bool{ | ||
| "catalog_item": true, | ||
| "template": true, | ||
| "template_parameters": true, | ||
| } | ||
| for _, fd := range fieldDefinitions { | ||
| if fd.GetPath() != "" { | ||
| allowedPaths[fd.GetPath()] = true | ||
| } | ||
| } | ||
| var unlisted []string | ||
| for _, path := range collectLeafPaths(specMap, "") { | ||
| if !isPathCovered(path, allowedPaths) { | ||
| unlisted = append(unlisted, path) | ||
| } | ||
| } | ||
| if len(unlisted) > 0 { | ||
| slices.Sort(unlisted) | ||
| return grpcstatus.Errorf(grpccodes.InvalidArgument, | ||
| "fields not allowed by catalog item: %s", strings.Join(unlisted, ", ")) | ||
| } | ||
|
|
||
| compiler := jsonschema.NewCompiler() | ||
|
|
||
| for _, fd := range fieldDefinitions { | ||
|
|
@@ -105,6 +129,10 @@ func applyFieldDefinitions( | |
| return grpcstatus.Errorf(grpccodes.Internal, | ||
| "catalog item misconfigured: non-editable field '%s' has no default value", path) | ||
| } | ||
| if userHasValue && userVal != nil { | ||
| return grpcstatus.Errorf(grpccodes.InvalidArgument, | ||
| "field '%s' is not editable", path) | ||
| } | ||
| if err := applyDefault(specMap, path, defaultVal); err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -234,6 +262,34 @@ func setNestedValue(m map[string]any, path string, value any) { | |
| } | ||
| } | ||
|
|
||
| func collectLeafPaths(m map[string]any, prefix string) []string { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 When a user sends a nested message with no fields set (e.g., Low practical impact since protojson omits empty sub-messages for non-optional fields, but for optional message fields that can serialize as Consider either treating empty nested maps as leaf paths (emit the parent path when the nested map has zero keys), or documenting this as accepted behavior. |
||
| var paths []string | ||
| for key, val := range m { | ||
| fullPath := key | ||
| if prefix != "" { | ||
| fullPath = prefix + "." + key | ||
| } | ||
| if nested, ok := val.(map[string]any); ok { | ||
| paths = append(paths, collectLeafPaths(nested, fullPath)...) | ||
| } else { | ||
| paths = append(paths, fullPath) | ||
| } | ||
| } | ||
| return paths | ||
| } | ||
|
|
||
| func isPathCovered(path string, allowedPaths map[string]bool) bool { | ||
| if allowedPaths[path] { | ||
| return true | ||
| } | ||
| for i := range path { | ||
| if path[i] == '.' && allowedPaths[path[:i]] { | ||
|
alosadagrande marked this conversation as resolved.
|
||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // validateInstanceTypeState looks up an instance type by name and validates its state. | ||
| // Returns warnings for DEPRECATED types, error for OBSOLETE or not-found types. | ||
| // The source parameter provides context for error messages (e.g., " in spec_defaults", " in field_definitions"). | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.