From 7ebc31cbe1ddbb4340b62d0b90c3eb003fd45b02 Mon Sep 17 00:00:00 2001 From: Alberto Losada Grande Date: Thu, 25 Jun 2026 13:04:47 +0200 Subject: [PATCH 1/6] fix: reject spec fields not listed in catalog item field_definitions When creating a resource via catalog item, the server now rejects any user-provided spec field that is not covered by a field_definition entry. The catalog_item and template fields are always allowed as system fields. A field_definition that points to a parent path covers all children (e.g., "network" covers "network.pod_cidr"). Ref: OSAC-1416 Assisted-by: Claude Code Signed-off-by: Alberto Losada Grande --- internal/servers/catalog_item_validation.go | 50 +++++++ .../servers/catalog_item_validation_test.go | 122 ++++++++++++++++++ 2 files changed, 172 insertions(+) diff --git a/internal/servers/catalog_item_validation.go b/internal/servers/catalog_item_validation.go index 545524bba..e1750884c 100644 --- a/internal/servers/catalog_item_validation.go +++ b/internal/servers/catalog_item_validation.go @@ -18,6 +18,7 @@ import ( "encoding/json" "errors" "fmt" + "slices" "strings" "sync" "time" @@ -89,6 +90,27 @@ func applyFieldDefinitions( return grpcstatus.Errorf(grpccodes.Internal, "failed to parse spec: %v", err) } + allowedPaths := map[string]bool{ + "catalog_item": true, + "template": 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 { @@ -234,6 +256,34 @@ func setNestedValue(m map[string]any, path string, value any) { } } +func collectLeafPaths(m map[string]any, prefix string) []string { + 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]] { + 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"). diff --git a/internal/servers/catalog_item_validation_test.go b/internal/servers/catalog_item_validation_test.go index fcb79aebe..de8b46180 100644 --- a/internal/servers/catalog_item_validation_test.go +++ b/internal/servers/catalog_item_validation_test.go @@ -18,6 +18,7 @@ import ( . "github.com/onsi/gomega" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/structpb" privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1" @@ -150,6 +151,127 @@ var _ = Describe("applyFieldDefinitions", func() { }) }) +var _ = Describe("applyFieldDefinitions rejects unlisted fields", func() { + It("rejects a single unlisted field on ClusterSpec", func() { + pullSecret := "my-secret" + spec := &privatev1.ClusterSpec{ + PullSecret: &pullSecret, + } + defaultVal, err := structpb.NewValue("ssh-ed25519 AAAA") + Expect(err).ToNot(HaveOccurred()) + fieldDefs := []*privatev1.FieldDefinition{{ + Path: "ssh_public_key", + Editable: true, + Default: defaultVal, + }} + err = applyFieldDefinitions(spec, fieldDefs) + Expect(err).To(HaveOccurred()) + Expect(status.Code(err)).To(Equal(codes.InvalidArgument)) + Expect(err.Error()).To(ContainSubstring("pull_secret")) + Expect(err.Error()).To(ContainSubstring("not allowed")) + }) + + It("rejects multiple unlisted fields on ClusterSpec", func() { + pullSecret := "my-secret" + releaseImage := "quay.io/ocp:4.21" + spec := &privatev1.ClusterSpec{ + PullSecret: &pullSecret, + ReleaseImage: &releaseImage, + } + defaultVal, err := structpb.NewValue("ssh-ed25519 AAAA") + Expect(err).ToNot(HaveOccurred()) + fieldDefs := []*privatev1.FieldDefinition{{ + Path: "ssh_public_key", + Editable: true, + Default: defaultVal, + }} + err = applyFieldDefinitions(spec, fieldDefs) + Expect(err).To(HaveOccurred()) + Expect(status.Code(err)).To(Equal(codes.InvalidArgument)) + Expect(err.Error()).To(ContainSubstring("pull_secret")) + Expect(err.Error()).To(ContainSubstring("release_image")) + }) + + It("accepts when all fields are covered by field_definitions", func() { + pullSecret := "my-secret" + sshKey := "ssh-ed25519 AAAA" + spec := &privatev1.ClusterSpec{ + PullSecret: &pullSecret, + SshPublicKey: &sshKey, + } + fieldDefs := []*privatev1.FieldDefinition{ + {Path: "pull_secret", Editable: true}, + {Path: "ssh_public_key", Editable: true}, + } + err := applyFieldDefinitions(spec, fieldDefs) + Expect(err).ToNot(HaveOccurred()) + }) + + It("always allows catalog_item without a field_definition", func() { + spec := privatev1.ClusterSpec_builder{ + CatalogItem: "cat-123", + }.Build() + defaultVal, err := structpb.NewValue("ssh-ed25519 AAAA") + Expect(err).ToNot(HaveOccurred()) + fieldDefs := []*privatev1.FieldDefinition{{ + Path: "ssh_public_key", + Editable: true, + Default: defaultVal, + }} + err = applyFieldDefinitions(spec, fieldDefs) + Expect(err).ToNot(HaveOccurred()) + }) + + It("always allows template without a field_definition", func() { + spec := privatev1.ClusterSpec_builder{ + Template: "my-template", + }.Build() + defaultVal, err := structpb.NewValue("ssh-ed25519 AAAA") + Expect(err).ToNot(HaveOccurred()) + fieldDefs := []*privatev1.FieldDefinition{{ + Path: "ssh_public_key", + Editable: true, + Default: defaultVal, + }} + err = applyFieldDefinitions(spec, fieldDefs) + Expect(err).ToNot(HaveOccurred()) + }) + + It("parent field_definition covers nested children", func() { + spec := privatev1.ClusterSpec_builder{ + Network: privatev1.ClusterNetwork_builder{ + PodCidr: proto.String("10.128.0.0/14"), + ServiceCidr: proto.String("172.30.0.0/16"), + }.Build(), + }.Build() + fieldDefs := []*privatev1.FieldDefinition{{ + Path: "network", + Editable: true, + }} + err := applyFieldDefinitions(spec, fieldDefs) + Expect(err).ToNot(HaveOccurred()) + }) + + It("rejects unlisted field on ComputeInstanceSpec", func() { + cores := int32(4) + spec := &privatev1.ComputeInstanceSpec{ + Cores: &cores, + } + defaultVal, err := structpb.NewValue("ssh-ed25519 AAAA") + Expect(err).ToNot(HaveOccurred()) + fieldDefs := []*privatev1.FieldDefinition{{ + Path: "ssh_key", + Editable: true, + Default: defaultVal, + }} + err = applyFieldDefinitions(spec, fieldDefs) + Expect(err).To(HaveOccurred()) + Expect(status.Code(err)).To(Equal(codes.InvalidArgument)) + Expect(err.Error()).To(ContainSubstring("cores")) + Expect(err.Error()).To(ContainSubstring("not allowed")) + }) +}) + var _ = Describe("addPublishedFilter", func() { var server *ClusterCatalogItemsServer From 0d4dc54688d278b15ffb9bd92ab9908c9e5fef04 Mon Sep 17 00:00:00 2001 From: Alberto Losada Grande Date: Thu, 25 Jun 2026 15:24:52 +0200 Subject: [PATCH 2/6] fix: reject user-provided values for non-editable catalog item fields When a field_definition is marked as non-editable and the user provides a value for it, the server now returns InvalidArgument instead of silently overwriting the value with the default. Ref: OSAC-1416 Assisted-by: Claude Code Signed-off-by: Alberto Losada Grande --- internal/servers/catalog_item_validation.go | 9 +++- .../servers/catalog_item_validation_test.go | 53 ++++++++++++++++++- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/internal/servers/catalog_item_validation.go b/internal/servers/catalog_item_validation.go index e1750884c..0071004f1 100644 --- a/internal/servers/catalog_item_validation.go +++ b/internal/servers/catalog_item_validation.go @@ -67,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 and template). +// 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( @@ -127,6 +128,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 } diff --git a/internal/servers/catalog_item_validation_test.go b/internal/servers/catalog_item_validation_test.go index de8b46180..b70bec4a0 100644 --- a/internal/servers/catalog_item_validation_test.go +++ b/internal/servers/catalog_item_validation_test.go @@ -65,7 +65,7 @@ var _ = Describe("applyFieldDefinitions", func() { Expect(spec.GetPullSecret()).To(Equal("default-secret")) }) - It("overrides user value with default for non-editable field", func() { + It("rejects user value for non-editable field", func() { userValue := "user-value" spec := &privatev1.ClusterSpec{ PullSecret: &userValue, @@ -78,10 +78,42 @@ var _ = Describe("applyFieldDefinitions", func() { Default: defaultVal, }} err = applyFieldDefinitions(spec, fieldDefs) + Expect(err).To(HaveOccurred()) + Expect(status.Code(err)).To(Equal(codes.InvalidArgument)) + Expect(err.Error()).To(ContainSubstring("not editable")) + }) + + It("applies default for non-editable field when user provides no value", func() { + spec := &privatev1.ClusterSpec{} + defaultVal, err := structpb.NewValue("admin-value") + Expect(err).ToNot(HaveOccurred()) + fieldDefs := []*privatev1.FieldDefinition{{ + Path: "pull_secret", + Editable: false, + Default: defaultVal, + }} + err = applyFieldDefinitions(spec, fieldDefs) Expect(err).ToNot(HaveOccurred()) Expect(spec.GetPullSecret()).To(Equal("admin-value")) }) + It("happy path: editable value preserved and non-editable default applied", func() { + sshKey := "ssh-ed25519 USER_KEY" + spec := &privatev1.ClusterSpec{ + SshPublicKey: &sshKey, + } + defaultRelease, err := structpb.NewValue("quay.io/ocp:4.16") + Expect(err).ToNot(HaveOccurred()) + fieldDefs := []*privatev1.FieldDefinition{ + {Path: "ssh_public_key", Editable: true}, + {Path: "release_image", Editable: false, Default: defaultRelease}, + } + err = applyFieldDefinitions(spec, fieldDefs) + Expect(err).ToNot(HaveOccurred()) + Expect(spec.GetSshPublicKey()).To(Equal("ssh-ed25519 USER_KEY")) + Expect(spec.GetReleaseImage()).To(Equal("quay.io/ocp:4.16")) + }) + It("returns no error for empty field definitions", func() { pullSecret := "my-secret" spec := &privatev1.ClusterSpec{ @@ -252,6 +284,25 @@ var _ = Describe("applyFieldDefinitions rejects unlisted fields", func() { Expect(err).ToNot(HaveOccurred()) }) + It("rejects unlisted field before checking non-editable override", func() { + pullSecret := "user-override" + spec := &privatev1.ClusterSpec{ + PullSecret: &pullSecret, + } + defaultVal, err := structpb.NewValue("admin-value") + Expect(err).ToNot(HaveOccurred()) + fieldDefs := []*privatev1.FieldDefinition{{ + Path: "release_image", + Editable: false, + Default: defaultVal, + }} + err = applyFieldDefinitions(spec, fieldDefs) + Expect(err).To(HaveOccurred()) + Expect(status.Code(err)).To(Equal(codes.InvalidArgument)) + Expect(err.Error()).To(ContainSubstring("not allowed")) + Expect(err.Error()).To(ContainSubstring("pull_secret")) + }) + It("rejects unlisted field on ComputeInstanceSpec", func() { cores := int32(4) spec := &privatev1.ComputeInstanceSpec{ From 301c039a531781db6e0e68f1d02fab6c0fa58c22 Mon Sep 17 00:00:00 2001 From: Alberto Losada Grande Date: Fri, 26 Jun 2026 10:36:28 +0200 Subject: [PATCH 3/6] docs: update catalog items doc to reflect field_definitions enforcement Update CATALOG_ITEMS.md to document that unlisted spec fields and user-provided values for non-editable fields are now rejected with InvalidArgument instead of being silently ignored or overwritten. Assisted-by: Claude Code Signed-off-by: Alberto Losada Grande --- docs/CATALOG_ITEMS.md | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/docs/CATALOG_ITEMS.md b/docs/CATALOG_ITEMS.md index 26304df66..023fa8776 100644 --- a/docs/CATALOG_ITEMS.md +++ b/docs/CATALOG_ITEMS.md @@ -171,8 +171,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: @@ -232,14 +233,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 + `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. 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. +running `--ssh-public-key "my-key"` results in an error — the user cannot override locked fields. ### Server-side processing @@ -248,7 +251,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 From 5ccff76b3607ce1122a9fa68731502c18d512b51 Mon Sep 17 00:00:00 2001 From: Alberto Losada Grande Date: Fri, 26 Jun 2026 11:09:57 +0200 Subject: [PATCH 4/6] fix: update server tests to expect rejection instead of silent override Existing tests in private_clusters_server_test.go and private_compute_instances_server_test.go expected non-editable fields to be silently overwritten. Update them to expect InvalidArgument. Also add network_attachments to field_definitions in compute instance tests so the unlisted field check does not trigger. Assisted-by: Claude Code Signed-off-by: Alberto Losada Grande --- docs/CATALOG_ITEMS.md | 22 ++++++++++------- .../servers/private_clusters_server_test.go | 12 ++++++---- .../private_compute_instances_server_test.go | 24 +++++++++++++++---- 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/docs/CATALOG_ITEMS.md b/docs/CATALOG_ITEMS.md index 023fa8776..733ce50bc 100644 --- a/docs/CATALOG_ITEMS.md +++ b/docs/CATALOG_ITEMS.md @@ -44,9 +44,8 @@ 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 @@ -136,14 +135,21 @@ 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: cores + display_name: CPU Cores + editable: true + default: 4 + - path: memory_gib + display_name: Memory (GiB) + editable: true + default: 8 + - path: network_attachments + display_name: Network Attachments editable: true - default: "fc430" ``` ### Create the catalog item diff --git a/internal/servers/private_clusters_server_test.go b/internal/servers/private_clusters_server_test.go index 81896c9a0..6f00fdabe 100644 --- a/internal/servers/private_clusters_server_test.go +++ b/internal/servers/private_clusters_server_test.go @@ -1176,7 +1176,7 @@ var _ = Describe("Private clusters server", func() { Expect(status.Message()).To(Equal("catalog_item and template are mutually exclusive")) }) - It("Overrides user value for non-editable field", func() { + It("Rejects user value for non-editable field", func() { createCatalogItem("cat-noneditable", true, []*privatev1.FieldDefinition{ privatev1.FieldDefinition_builder{ Path: "pull_secret", @@ -1185,7 +1185,7 @@ var _ = Describe("Private clusters server", func() { }.Build(), }) - response, err := server.Create(ctx, privatev1.ClustersCreateRequest_builder{ + _, err := server.Create(ctx, privatev1.ClustersCreateRequest_builder{ Object: privatev1.Cluster_builder{ Spec: privatev1.ClusterSpec_builder{ CatalogItem: "cat-noneditable", @@ -1196,9 +1196,11 @@ var _ = Describe("Private clusters server", func() { }.Build(), }.Build(), }.Build()) - Expect(err).ToNot(HaveOccurred()) - object := response.GetObject() - Expect(object.GetSpec().GetPullSecret()).To(Equal("forced-secret")) + Expect(err).To(HaveOccurred()) + status, ok := grpcstatus.FromError(err) + Expect(ok).To(BeTrue()) + Expect(status.Code()).To(Equal(grpccodes.InvalidArgument)) + Expect(status.Message()).To(ContainSubstring("not editable")) }) DescribeTable("validates editable field against JSON Schema", diff --git a/internal/servers/private_compute_instances_server_test.go b/internal/servers/private_compute_instances_server_test.go index bc56ba1f4..09e79c23b 100644 --- a/internal/servers/private_compute_instances_server_test.go +++ b/internal/servers/private_compute_instances_server_test.go @@ -1127,16 +1127,20 @@ var _ = Describe("Private compute instances server", func() { Expect(status.Message()).To(Equal("catalog_item and template are mutually exclusive")) }) - It("Overrides user value for non-editable field", func() { + It("Rejects user value for non-editable field", func() { createCICatalogItem("ci-cat-nonedit", true, []*privatev1.FieldDefinition{ privatev1.FieldDefinition_builder{ Path: "ssh_key", Editable: false, Default: structpb.NewStringValue("forced-key"), }.Build(), + privatev1.FieldDefinition_builder{ + Path: "network_attachments", + Editable: true, + }.Build(), }) - response, err := server.Create(ctx, privatev1.ComputeInstancesCreateRequest_builder{ + _, err := server.Create(ctx, privatev1.ComputeInstancesCreateRequest_builder{ Object: privatev1.ComputeInstance_builder{ Spec: privatev1.ComputeInstanceSpec_builder{ CatalogItem: "ci-cat-nonedit", @@ -1149,9 +1153,11 @@ var _ = Describe("Private compute instances server", func() { }.Build(), }.Build(), }.Build()) - Expect(err).ToNot(HaveOccurred()) - object := response.GetObject() - Expect(object.GetSpec().GetSshKey()).To(Equal("forced-key")) + Expect(err).To(HaveOccurred()) + status, ok := grpcstatus.FromError(err) + Expect(ok).To(BeTrue()) + Expect(status.Code()).To(Equal(grpccodes.InvalidArgument)) + Expect(status.Message()).To(ContainSubstring("not editable")) }) DescribeTable("validates editable field against JSON Schema", @@ -1162,6 +1168,10 @@ var _ = Describe("Private compute instances server", func() { Editable: true, ValidationSchema: `{"type":"string","minLength":10}`, }.Build(), + privatev1.FieldDefinition_builder{ + Path: "network_attachments", + Editable: true, + }.Build(), }) response, err := server.Create(ctx, privatev1.ComputeInstancesCreateRequest_builder{ @@ -1199,6 +1209,10 @@ var _ = Describe("Private compute instances server", func() { Editable: true, Default: structpb.NewStringValue("default-key"), }.Build(), + privatev1.FieldDefinition_builder{ + Path: "network_attachments", + Editable: true, + }.Build(), }) response, err := server.Create(ctx, privatev1.ComputeInstancesCreateRequest_builder{ From 08db59cb3fe3498f2038d5d729020cf0a6335596 Mon Sep 17 00:00:00 2001 From: Alberto Losada Grande Date: Mon, 13 Jul 2026 10:01:25 +0200 Subject: [PATCH 5/6] docs: replace deprecated cores/memory_gib with instance_type in catalog items doc Signed-off-by: Alberto Losada Grande --- docs/CATALOG_ITEMS.md | 49 ++++++++++++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/docs/CATALOG_ITEMS.md b/docs/CATALOG_ITEMS.md index 733ce50bc..9cd27dcc3 100644 --- a/docs/CATALOG_ITEMS.md +++ b/docs/CATALOG_ITEMS.md @@ -26,10 +26,10 @@ in two ways: - `osac create cluster --template ` — 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 ` — 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 @@ -52,9 +52,9 @@ Custom provisioning parameters (like `vpc_id`, `ip_block_id`, `ssh_key_group_id` 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 @@ -139,18 +139,40 @@ field_definitions: display_name: SSH Key editable: true default: "ssh-ed25519 AAAA..." - - path: cores - display_name: CPU Cores + - 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 + - path: boot_disk.size_gib + default: 20 + display_name: Boot Disk Size (GiB) editable: true - default: 4 - - path: memory_gib - display_name: Memory (GiB) + - path: instance_type + display_name: Instance Type + editable: true + - path: run_strategy + display_name: Run Strategy editable: true - default: 8 - 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: + +```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 @@ -198,8 +220,7 @@ catalog item, the server rejects any spec field not listed in `field_definitions | 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`) | @@ -247,8 +268,8 @@ the server applies `field_definitions` **after** receiving the request, which me `validation_schema` if one is defined. - If an editable field is **not provided** by the user, the catalog item's default is applied. -For example, given a catalog item with `ssh_public_key` set as non-editable with a fixed default, -running `--ssh-public-key "my-key"` results in an error — the user cannot override locked fields. +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 From 2c695a25d4f9df67783118e3d5eba8ee9610b3f6 Mon Sep 17 00:00:00 2001 From: Alberto Losada Grande Date: Mon, 13 Jul 2026 11:54:15 +0200 Subject: [PATCH 6/6] fix: allow template_parameters as system field and update tests Add template_parameters to the allowedPaths map in applyFieldDefinitions so BareMetalInstance specs that combine catalog items with template parameters are not rejected. Update is_windows and BareMetalInstance non-editable override tests to match the new enforcement behavior. Assisted-by: Claude Code Signed-off-by: Alberto Losada Grande --- internal/servers/catalog_item_validation.go | 7 ++++--- internal/servers/catalog_item_validation_test.go | 7 ++----- .../private_baremetal_instances_server_test.go | 12 +++++++----- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/internal/servers/catalog_item_validation.go b/internal/servers/catalog_item_validation.go index 0071004f1..9ec92016e 100644 --- a/internal/servers/catalog_item_validation.go +++ b/internal/servers/catalog_item_validation.go @@ -68,7 +68,7 @@ type catalogItem interface { } // 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 and template). +// 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. @@ -92,8 +92,9 @@ func applyFieldDefinitions( } allowedPaths := map[string]bool{ - "catalog_item": true, - "template": true, + "catalog_item": true, + "template": true, + "template_parameters": true, } for _, fd := range fieldDefinitions { if fd.GetPath() != "" { diff --git a/internal/servers/catalog_item_validation_test.go b/internal/servers/catalog_item_validation_test.go index b70bec4a0..c4371cf5b 100644 --- a/internal/servers/catalog_item_validation_test.go +++ b/internal/servers/catalog_item_validation_test.go @@ -165,11 +165,8 @@ var _ = Describe("applyFieldDefinitions", func() { Expect(spec.GetIsWindows()).To(BeTrue()) }) - It("forces is_windows value for non-editable field definition", func() { - falseVal := false - spec := &privatev1.ComputeInstanceSpec{ - IsWindows: &falseVal, - } + It("applies non-editable default for bool field is_windows on compute instance spec", func() { + spec := &privatev1.ComputeInstanceSpec{} defaultVal, err := structpb.NewValue(true) Expect(err).ToNot(HaveOccurred()) fieldDefs := []*privatev1.FieldDefinition{{ diff --git a/internal/servers/private_baremetal_instances_server_test.go b/internal/servers/private_baremetal_instances_server_test.go index 468438d93..f504092f3 100644 --- a/internal/servers/private_baremetal_instances_server_test.go +++ b/internal/servers/private_baremetal_instances_server_test.go @@ -991,7 +991,7 @@ var _ = Describe("Private bare metal instances server", func() { Expect(response.GetObject().GetSpec().GetTemplateParameters()).To(HaveKey("os_version")) }) - It("Overrides non-editable field_definition alongside template_parameters", func() { + It("Rejects user value for non-editable field_definition alongside template_parameters", func() { createTemplate("override-combo-template", []*privatev1.BareMetalInstanceTemplateParameterDefinition{ {Name: "os_version", Required: true, Type: "type.googleapis.com/google.protobuf.StringValue"}, }) @@ -1017,7 +1017,7 @@ var _ = Describe("Private bare metal instances server", func() { Expect(err).ToNot(HaveOccurred()) userKey := "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIUserProvidedKeyThatShouldBeOverridden user@test" - response, err := server.Create(ctx, privatev1.BareMetalInstancesCreateRequest_builder{ + _, err = server.Create(ctx, privatev1.BareMetalInstancesCreateRequest_builder{ Object: privatev1.BareMetalInstance_builder{ Spec: privatev1.BareMetalInstanceSpec_builder{ CatalogItem: catID, @@ -1026,9 +1026,11 @@ var _ = Describe("Private bare metal instances server", func() { }.Build(), }.Build(), }.Build()) - Expect(err).ToNot(HaveOccurred()) - Expect(response.GetObject().GetSpec().GetSshPublicKey()).To(Equal(testSSHPublicKey)) - Expect(response.GetObject().GetSpec().GetTemplateParameters()).To(HaveKey("os_version")) + Expect(err).To(HaveOccurred()) + st, ok := grpcstatus.FromError(err) + Expect(ok).To(BeTrue()) + Expect(st.Code()).To(Equal(grpccodes.InvalidArgument)) + Expect(st.Message()).To(ContainSubstring("not editable")) }) It("Accepts editable field_definition alongside template_parameters", func() {