diff --git a/docs/CATALOG_ITEMS.md b/docs/CATALOG_ITEMS.md index 26304df66..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 @@ -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 + - 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: + +```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 + `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. +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 diff --git a/internal/servers/catalog_item_validation.go b/internal/servers/catalog_item_validation.go index 545524bba..9ec92016e 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" @@ -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 { + 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..c4371cf5b 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" @@ -64,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, @@ -77,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{ @@ -132,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{{ @@ -150,6 +180,146 @@ 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 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{ + 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 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() { 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{