From 5c69ad670c295cd2b3e9e3cb6264482187199d7d Mon Sep 17 00:00:00 2001 From: Mike Azure <127820851+AzureMike@users.noreply.github.com> Date: Mon, 27 Jul 2026 16:07:23 -0700 Subject: [PATCH 1/2] Materialize schema property defaults before the resource is saved Resource type schemas could declare a default for a property, but nothing ever applied it. ValidateResourceAgainstSchema only validates, so declared defaults in objectStorage, mongoDatabases, kafka, rabbitMQ and models had no effect, and recipes reading an unset optional property through context.resource.properties.* found nothing to resolve. Add schema.ApplyDefaults, which fills absent properties from the default declared in the schema, and run it as an UpdateFilter on the dynamic resource PUT path ahead of the encryption filter so a defaulted sensitive field is still encrypted. Explicit values are never overwritten. Read-only properties are skipped because they are recipe outputs, and an absent object is not created just to hold defaults. Required properties are never defaulted, so omitting one still fails validation rather than being quietly satisfied. The rule blocks defaulting only, not recursion: a required object the author did supply is still descended into, because its own optional properties may declare defaults of their own. A body carrying no properties at all is defaulted too. The filled map is attached only when something was actually applied, so a genuinely empty resource serializes exactly as it did before. Defaults are reapplied on every write rather than carried forward from the previous version of the resource, which is what full-replace PUT semantics ask for. The consequence is that changing a declared default changes what an existing resource gets on its next deployment, so a default is part of a resource type's API contract and should change only alongside the API version. The manifests under deploy/manifest/built-in-providers are verbatim copies of the resource-types-contrib version pinned in go.mod, and CI re-runs the copy and fails on any diff. Built-in types therefore pick up the new defaults only after the contrib change merges and go.mod is bumped. Until then this change is inert for them rather than broken. Known follow-up: this filter and the encryption filter each fetch the same schema from UCP, so a PUT now makes two identical round trips. Sharing one fetch needs either a request-scoped cache seeded in the shared controller or the two filters merged, both of which reach past this change. With defaults declared, this resolves radius-project/radius#12532 on its own: an unset size resolves to Burstable/Standard_B1ms/Balanced_B0, and plain non-ternary paths such as database stop leaking too. Signed-off-by: Mike Azure <127820851+AzureMike@users.noreply.github.com> --- pkg/dynamicrp/frontend/defaultsfilter.go | 93 ++++++++ pkg/dynamicrp/frontend/routes.go | 4 + pkg/schema/defaults.go | 110 ++++++++++ pkg/schema/defaults_test.go | 266 +++++++++++++++++++++++ 4 files changed, 473 insertions(+) create mode 100644 pkg/dynamicrp/frontend/defaultsfilter.go create mode 100644 pkg/schema/defaults.go create mode 100644 pkg/schema/defaults_test.go diff --git a/pkg/dynamicrp/frontend/defaultsfilter.go b/pkg/dynamicrp/frontend/defaultsfilter.go new file mode 100644 index 00000000000..276aa5a03dc --- /dev/null +++ b/pkg/dynamicrp/frontend/defaultsfilter.go @@ -0,0 +1,93 @@ +/* +Copyright 2023 The Radius 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 frontend + +import ( + "context" + + v1 "github.com/radius-project/radius/pkg/armrpc/api/v1" + "github.com/radius-project/radius/pkg/armrpc/frontend/controller" + "github.com/radius-project/radius/pkg/armrpc/rest" + "github.com/radius-project/radius/pkg/dynamicrp/datamodel" + "github.com/radius-project/radius/pkg/schema" + "github.com/radius-project/radius/pkg/ucp/api/v20231001preview" + "github.com/radius-project/radius/pkg/ucp/ucplog" +) + +// makeDefaultsFilter creates an UpdateFilter that materializes schema defaults into the resource's +// Properties before it is saved, so the stored resource, the API response and the recipe agree. +// +// oldResource is deliberately not consulted: a PUT replaces the resource, so a dropped property +// returns to its default rather than keeping its previous value. +func makeDefaultsFilter(ucpClient *v20231001preview.ClientFactory) controller.UpdateFilter[datamodel.DynamicResource] { + return func( + ctx context.Context, + newResource *datamodel.DynamicResource, + oldResource *datamodel.DynamicResource, + options *controller.Options, + ) (rest.Response, error) { + return applySchemaDefaults(ctx, newResource, ucpClient) + } +} + +// applySchemaDefaults fills unset properties from the "default" values declared in the resource type schema. +func applySchemaDefaults( + ctx context.Context, + newResource *datamodel.DynamicResource, + ucpClient *v20231001preview.ClientFactory, +) (rest.Response, error) { + logger := ucplog.FromContextOrDiscard(ctx) + serviceCtx := v1.ARMRequestContextFromContext(ctx) + + if newResource == nil { + return nil, nil + } + + resourceID := serviceCtx.ResourceID.String() + resourceType := serviceCtx.ResourceID.Type() + apiVersion := serviceCtx.APIVersion + + schemaData, err := schema.GetSchema(ctx, ucpClient, resourceID, resourceType, apiVersion) + if err != nil { + logger.Error(err, "Failed to fetch schema for defaults", + "resourceType", resourceType, "apiVersion", apiVersion) + return rest.NewInternalServerErrorARMResponse(v1.ErrorResponse{ + Error: &v1.ErrorDetails{ + Code: v1.CodeInternal, + Message: "Failed to fetch schema to apply property defaults", + }, + }), nil + } + + if schemaData == nil { + return nil, nil + } + + // Attach the map only if something was applied, so an empty resource serializes as before. + properties := newResource.Properties + if properties == nil { + properties = map[string]any{} + } + + if applied := schema.ApplyDefaults(properties, schemaData); applied > 0 { + newResource.Properties = properties + logger.V(ucplog.LevelDebug).Info("Applied schema defaults", + "count", applied, "resourceType", resourceType, "resourceID", resourceID) + } + + return nil, nil +} diff --git a/pkg/dynamicrp/frontend/routes.go b/pkg/dynamicrp/frontend/routes.go index 5493969d605..164a102a078 100644 --- a/pkg/dynamicrp/frontend/routes.go +++ b/pkg/dynamicrp/frontend/routes.go @@ -53,11 +53,15 @@ func (s *Service) registerRoutes( // Create encryption filter for sensitive fields encryptionFilter := makeEncryptionFilter(ucpClient, handler) + // Materialize schema defaults before encryption, so a defaulted sensitive field is still encrypted. + defaultsFilter := makeDefaultsFilter(ucpClient) + // Resource options with encryption filter applied to PUT operations resourceOptions := controller.ResourceOptions[datamodel.DynamicResource]{ RequestConverter: converter.DynamicResourceDataModelFromVersioned, ResponseConverter: converter.DynamicResourceDataModelToVersioned, UpdateFilters: []controller.UpdateFilter[datamodel.DynamicResource]{ + defaultsFilter, encryptionFilter, }, AsyncOperationRetryAfter: time.Second * 5, diff --git a/pkg/schema/defaults.go b/pkg/schema/defaults.go new file mode 100644 index 00000000000..bfbdd2e6890 --- /dev/null +++ b/pkg/schema/defaults.go @@ -0,0 +1,110 @@ +/* +Copyright 2023 The Radius 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 schema + +// ApplyDefaults fills unset properties from the "default" declared in the schema and returns the +// number applied. +// +// Explicit values are never overwritten. Required properties are never defaulted, so omitting one +// still fails validation. Read-only properties are recipe outputs, and an absent object is not +// created just to hold defaults. +func ApplyDefaults(properties map[string]any, schemaData map[string]any) int { + if properties == nil || schemaData == nil { + return 0 + } + + declared, ok := schemaData["properties"].(map[string]any) + if !ok { + return 0 + } + + required := requiredProperties(schemaData) + + applied := 0 + for name, raw := range declared { + fieldSchema, ok := raw.(map[string]any) + if !ok { + continue + } + + if readOnly, ok := fieldSchema["readOnly"].(bool); ok && readOnly { + continue + } + + // A supplied object is descended into even when required, since its own optional + // properties may declare defaults. + if existing, present := properties[name]; present { + if nested, ok := existing.(map[string]any); ok { + applied += ApplyDefaults(nested, fieldSchema) + } + continue + } + + if required[name] { + continue + } + + if defaultValue, ok := fieldSchema["default"]; ok { + properties[name] = copyDefaultValue(defaultValue) + applied++ + } + } + + return applied +} + +// requiredProperties reads the schema's "required" list, accepting either the []any from JSON +// decoding or a []string. +func requiredProperties(schemaData map[string]any) map[string]bool { + required := map[string]bool{} + + switch list := schemaData["required"].(type) { + case []any: + for _, item := range list { + if name, ok := item.(string); ok { + required[name] = true + } + } + case []string: + for _, name := range list { + required[name] = true + } + } + + return required +} + +// copyDefaultValue deep copies a default so mutating a materialized property cannot reach back into +// a cached schema. +func copyDefaultValue(value any) any { + switch typed := value.(type) { + case map[string]any: + copied := make(map[string]any, len(typed)) + for key, item := range typed { + copied[key] = copyDefaultValue(item) + } + return copied + case []any: + copied := make([]any, len(typed)) + for i, item := range typed { + copied[i] = copyDefaultValue(item) + } + return copied + default: + return value + } +} diff --git a/pkg/schema/defaults_test.go b/pkg/schema/defaults_test.go new file mode 100644 index 00000000000..282fdafa04b --- /dev/null +++ b/pkg/schema/defaults_test.go @@ -0,0 +1,266 @@ +/* +Copyright 2023 The Radius 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 schema + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestApplyDefaults(t *testing.T) { + tests := []struct { + name string + properties map[string]any + schema map[string]any + expected map[string]any + applied int + }{ + { + name: "unset property takes the declared default", + properties: map[string]any{"environment": "env"}, + schema: map[string]any{"properties": map[string]any{ + "environment": map[string]any{"type": "string"}, + "size": map[string]any{"type": "string", "default": "S"}, + }}, + expected: map[string]any{"environment": "env", "size": "S"}, + applied: 1, + }, + { + name: "explicit value is never overwritten", + properties: map[string]any{"size": "L"}, + schema: map[string]any{"properties": map[string]any{ + "size": map[string]any{"type": "string", "default": "S"}, + }}, + expected: map[string]any{"size": "L"}, + applied: 0, + }, + { + name: "explicit empty string is kept, not defaulted", + properties: map[string]any{"database": ""}, + schema: map[string]any{"properties": map[string]any{ + "database": map[string]any{"type": "string", "default": "appdb"}, + }}, + expected: map[string]any{"database": ""}, + applied: 0, + }, + { + name: "property without a default is left absent", + properties: map[string]any{}, + schema: map[string]any{"properties": map[string]any{ + "application": map[string]any{"type": "string"}, + }}, + expected: map[string]any{}, + applied: 0, + }, + { + name: "read only property is skipped", + properties: map[string]any{}, + schema: map[string]any{"properties": map[string]any{ + "host": map[string]any{"type": "string", "default": "localhost", "readOnly": true}, + }}, + expected: map[string]any{}, + applied: 0, + }, + { + name: "non string defaults are applied", + properties: map[string]any{}, + schema: map[string]any{"properties": map[string]any{ + "port": map[string]any{"type": "integer", "default": 5432}, + "replicas": map[string]any{"type": "integer", "default": 1}, + "enabled": map[string]any{"type": "boolean", "default": true}, + }}, + expected: map[string]any{"port": 5432, "replicas": 1, "enabled": true}, + applied: 3, + }, + { + name: "nested object present is recursed into", + properties: map[string]any{"config": map[string]any{"tls": true}}, + schema: map[string]any{"properties": map[string]any{ + "config": map[string]any{"type": "object", "properties": map[string]any{ + "tls": map[string]any{"type": "boolean"}, + "mode": map[string]any{"type": "string", "default": "require"}, + }}, + }}, + expected: map[string]any{"config": map[string]any{"tls": true, "mode": "require"}}, + applied: 1, + }, + { + name: "absent object is not created to hold defaults", + properties: map[string]any{}, + schema: map[string]any{"properties": map[string]any{ + "config": map[string]any{"type": "object", "properties": map[string]any{ + "mode": map[string]any{"type": "string", "default": "require"}, + }}, + }}, + expected: map[string]any{}, + applied: 0, + }, + { + name: "schema without properties is a no-op", + properties: map[string]any{"size": "S"}, + schema: map[string]any{"type": "object"}, + expected: map[string]any{"size": "S"}, + applied: 0, + }, + { + name: "required property is not defaulted", + properties: map[string]any{"environment": "env"}, + schema: map[string]any{ + "required": []any{"environment", "database"}, + "properties": map[string]any{ + "environment": map[string]any{"type": "string"}, + "database": map[string]any{"type": "string", "default": "appdb"}, + }, + }, + expected: map[string]any{"environment": "env"}, + applied: 0, + }, + { + name: "required list given as strings is honoured", + properties: map[string]any{}, + schema: map[string]any{ + "required": []string{"database"}, + "properties": map[string]any{"database": map[string]any{"type": "string", "default": "appdb"}}, + }, + expected: map[string]any{}, + applied: 0, + }, + { + name: "optional property is still defaulted alongside a required one", + properties: map[string]any{"environment": "env"}, + schema: map[string]any{ + "required": []any{"environment"}, + "properties": map[string]any{ + "environment": map[string]any{"type": "string"}, + "size": map[string]any{"type": "string", "default": "S"}, + }, + }, + expected: map[string]any{"environment": "env", "size": "S"}, + applied: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + applied := ApplyDefaults(tt.properties, tt.schema) + require.Equal(t, tt.applied, applied) + require.Equal(t, tt.expected, tt.properties) + }) + } +} + +func TestApplyDefaults_NilInputs(t *testing.T) { + require.Equal(t, 0, ApplyDefaults(nil, map[string]any{"properties": map[string]any{}})) + require.Equal(t, 0, ApplyDefaults(map[string]any{}, nil)) +} + +// A map or slice default must be copied, not shared with the schema. +func TestApplyDefaults_DefaultsAreCopied(t *testing.T) { + schemaData := map[string]any{"properties": map[string]any{ + "tags": map[string]any{"type": "object", "default": map[string]any{"tier": "free"}}, + "origins": map[string]any{"type": "array", "default": []any{"a"}}, + }} + + first := map[string]any{} + require.Equal(t, 2, ApplyDefaults(first, schemaData)) + + first["tags"].(map[string]any)["tier"] = "paid" + first["origins"].([]any)[0] = "b" + + second := map[string]any{} + require.Equal(t, 2, ApplyDefaults(second, schemaData)) + require.Equal(t, map[string]any{"tier": "free"}, second["tags"]) + require.Equal(t, []any{"a"}, second["origins"]) +} + +// Defaults declared by the resource types in resource-types-contrib. +func TestApplyDefaults_ContribShapes(t *testing.T) { + schemaData := map[string]any{"properties": map[string]any{ + "environment": map[string]any{"type": "string"}, + "application": map[string]any{"type": "string"}, + "size": map[string]any{"type": "string", "enum": []any{"S", "M", "L"}, "default": "S"}, + "containerName": map[string]any{"type": "string", "default": "data"}, + "host": map[string]any{"type": "string", "readOnly": true}, + }} + + properties := map[string]any{"environment": "/planes/radius/local/resourcegroups/default/providers/Radius.Core/environments/test"} + require.Equal(t, 2, ApplyDefaults(properties, schemaData)) + require.Equal(t, "S", properties["size"]) + require.Equal(t, "data", properties["containerName"]) + require.NotContains(t, properties, "host") + require.NotContains(t, properties, "application") +} + +// A default on a required property must not fill it, or an omitted value would pass validation. +func TestApplyDefaults_RequiredWinsOverDeclaredDefault(t *testing.T) { + schemaData := map[string]any{ + "required": []any{"environment", "database", "username", "password"}, + "properties": map[string]any{ + "environment": map[string]any{"type": "string"}, + "database": map[string]any{"type": "string", "default": "appdb"}, + "application": map[string]any{"type": "string"}, + }, + } + + properties := map[string]any{"environment": "env"} + require.Equal(t, 0, ApplyDefaults(properties, schemaData)) + require.NotContains(t, properties, "database") +} + +// A supplied required object still needs its own optional properties defaulted. +func TestApplyDefaults_NestedDefaultsInsideSuppliedRequiredObject(t *testing.T) { + schemaData := map[string]any{ + "required": []any{"config"}, + "properties": map[string]any{ + "config": map[string]any{ + "type": "object", + "properties": map[string]any{ + "mode": map[string]any{"type": "string", "default": "fast"}, + "retries": map[string]any{"type": "integer", "default": 3}, + "name": map[string]any{"type": "string"}, + }, + }, + }, + } + + properties := map[string]any{"config": map[string]any{"name": "given"}} + require.Equal(t, 2, ApplyDefaults(properties, schemaData)) + + config := properties["config"].(map[string]any) + require.Equal(t, "fast", config["mode"]) + require.Equal(t, 3, config["retries"]) + require.Equal(t, "given", config["name"]) +} + +// An absent required object is not created just to hold defaults. +func TestApplyDefaults_AbsentRequiredObjectIsNotCreated(t *testing.T) { + schemaData := map[string]any{ + "required": []any{"config"}, + "properties": map[string]any{ + "config": map[string]any{ + "type": "object", + "default": map[string]any{"mode": "fast"}, + "properties": map[string]any{"mode": map[string]any{"type": "string", "default": "fast"}}, + }, + }, + } + + properties := map[string]any{} + require.Equal(t, 0, ApplyDefaults(properties, schemaData)) + require.NotContains(t, properties, "config") +} From 014d61876f758d9e00d04573d10accca01438883 Mon Sep 17 00:00:00 2001 From: Mike Azure <127820851+AzureMike@users.noreply.github.com> Date: Fri, 31 Jul 2026 12:13:50 -0700 Subject: [PATCH 2/2] Handle nested schema defaults and filter edge cases Apply defaults inside arrays and map values, cover the defaults filter, and keep nil properties safe during response conversion. Signed-off-by: Mike Azure <127820851+AzureMike@users.noreply.github.com> --- .../api/dynamicresource_conversion.go | 3 + .../api/dynamicresource_conversion_test.go | 11 ++ pkg/dynamicrp/frontend/defaultsfilter.go | 25 ++-- pkg/dynamicrp/frontend/defaultsfilter_test.go | 109 ++++++++++++++++ pkg/dynamicrp/frontend/encryptionfilter.go | 4 +- pkg/dynamicrp/frontend/routes.go | 6 +- pkg/schema/defaults.go | 64 +++++++--- pkg/schema/defaults_test.go | 117 +++++++++++++++++- 8 files changed, 303 insertions(+), 36 deletions(-) create mode 100644 pkg/dynamicrp/frontend/defaultsfilter_test.go diff --git a/pkg/dynamicrp/api/dynamicresource_conversion.go b/pkg/dynamicrp/api/dynamicresource_conversion.go index 6b0f87f50cf..f6b3b5ef14f 100644 --- a/pkg/dynamicrp/api/dynamicresource_conversion.go +++ b/pkg/dynamicrp/api/dynamicresource_conversion.go @@ -90,6 +90,9 @@ func (d *DynamicResource) ConvertFrom(src v1.DataModelInterface) error { if err != nil { return fmt.Errorf("failed to unmarshal properties: %w", err) } + if properties == nil { + properties = map[string]any{} + } d.ID = &dm.ID d.Name = &dm.Name diff --git a/pkg/dynamicrp/api/dynamicresource_conversion_test.go b/pkg/dynamicrp/api/dynamicresource_conversion_test.go index 12e0c181b01..1a0734cb805 100644 --- a/pkg/dynamicrp/api/dynamicresource_conversion_test.go +++ b/pkg/dynamicrp/api/dynamicresource_conversion_test.go @@ -124,3 +124,14 @@ func Test_DynamicResource_ConvertDataModelToVersioned(t *testing.T) { }) } } + +func Test_DynamicResource_ConvertDataModelToVersioned_NilProperties(t *testing.T) { + dm := &datamodel.DynamicResource{} + resource := &DynamicResource{} + + err := resource.ConvertFrom(dm) + require.NoError(t, err) + require.Equal(t, map[string]any{ + "provisioningState": fromProvisioningStateDataModel(dm.AsyncProvisioningState), + }, resource.Properties) +} diff --git a/pkg/dynamicrp/frontend/defaultsfilter.go b/pkg/dynamicrp/frontend/defaultsfilter.go index 276aa5a03dc..388c0d43065 100644 --- a/pkg/dynamicrp/frontend/defaultsfilter.go +++ b/pkg/dynamicrp/frontend/defaultsfilter.go @@ -28,12 +28,12 @@ import ( "github.com/radius-project/radius/pkg/ucp/ucplog" ) -// makeDefaultsFilter creates an UpdateFilter that materializes schema defaults into the resource's -// Properties before it is saved, so the stored resource, the API response and the recipe agree. +type defaultsUpdateFilter controller.UpdateFilter[datamodel.DynamicResource] + +// makeDefaultsFilter applies schema defaults before a resource is saved. // -// oldResource is deliberately not consulted: a PUT replaces the resource, so a dropped property -// returns to its default rather than keeping its previous value. -func makeDefaultsFilter(ucpClient *v20231001preview.ClientFactory) controller.UpdateFilter[datamodel.DynamicResource] { +// PUT replaces the resource, so omitted properties use current defaults instead of old values. +func makeDefaultsFilter(ucpClient *v20231001preview.ClientFactory) defaultsUpdateFilter { return func( ctx context.Context, newResource *datamodel.DynamicResource, @@ -44,7 +44,18 @@ func makeDefaultsFilter(ucpClient *v20231001preview.ClientFactory) controller.Up } } -// applySchemaDefaults fills unset properties from the "default" values declared in the resource type schema. +func makeUpdateFilters( + defaultsFilter defaultsUpdateFilter, + encryptionFilter encryptionUpdateFilter, +) []controller.UpdateFilter[datamodel.DynamicResource] { + // Distinct types prevent callers from passing encryption first. + return []controller.UpdateFilter[datamodel.DynamicResource]{ + controller.UpdateFilter[datamodel.DynamicResource](defaultsFilter), + controller.UpdateFilter[datamodel.DynamicResource](encryptionFilter), + } +} + +// applySchemaDefaults adds defaults from the resource schema. func applySchemaDefaults( ctx context.Context, newResource *datamodel.DynamicResource, @@ -77,7 +88,7 @@ func applySchemaDefaults( return nil, nil } - // Attach the map only if something was applied, so an empty resource serializes as before. + // Keep Properties nil when no default applies. properties := newResource.Properties if properties == nil { properties = map[string]any{} diff --git a/pkg/dynamicrp/frontend/defaultsfilter_test.go b/pkg/dynamicrp/frontend/defaultsfilter_test.go new file mode 100644 index 00000000000..04f9477cd41 --- /dev/null +++ b/pkg/dynamicrp/frontend/defaultsfilter_test.go @@ -0,0 +1,109 @@ +/* +Copyright 2023 The Radius 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 frontend + +import ( + "testing" + + v1 "github.com/radius-project/radius/pkg/armrpc/api/v1" + "github.com/radius-project/radius/pkg/armrpc/rest" + "github.com/radius-project/radius/pkg/dynamicrp/datamodel" + "github.com/stretchr/testify/require" +) + +func TestMakeDefaultsFilter_SchemaFetchError(t *testing.T) { + ucpClient, err := testUCPClientFactoryWithError() + require.NoError(t, err) + + resource := &datamodel.DynamicResource{ + Properties: map[string]any{"size": "L"}, + } + + response, err := makeDefaultsFilter(ucpClient)(createTestContext(), resource, nil, nil) + require.NoError(t, err) + + errorResponse, ok := response.(*rest.InternalServerErrorResponse) + require.True(t, ok) + require.Equal(t, v1.CodeInternal, errorResponse.Body.Error.Code) + require.Equal(t, "Failed to fetch schema to apply property defaults", errorResponse.Body.Error.Message) + require.Equal(t, map[string]any{"size": "L"}, resource.Properties) +} + +func TestMakeDefaultsFilter_NilProperties(t *testing.T) { + t.Run("materializes declared defaults", func(t *testing.T) { + ucpClient, err := createFakeUCPClientFactory(map[string]any{ + "type": "object", + "properties": map[string]any{ + "size": map[string]any{"type": "string", "default": "S"}, + }, + }) + require.NoError(t, err) + + resource := &datamodel.DynamicResource{} + response, err := makeDefaultsFilter(ucpClient)(createTestContext(), resource, nil, nil) + require.NoError(t, err) + require.Nil(t, response) + require.Equal(t, map[string]any{"size": "S"}, resource.Properties) + }) + + t.Run("remains nil when no defaults apply", func(t *testing.T) { + ucpClient, err := createFakeUCPClientFactory(map[string]any{ + "type": "object", + "properties": map[string]any{ + "name": map[string]any{"type": "string"}, + }, + }) + require.NoError(t, err) + + resource := &datamodel.DynamicResource{} + response, err := makeDefaultsFilter(ucpClient)(createTestContext(), resource, nil, nil) + require.NoError(t, err) + require.Nil(t, response) + require.Nil(t, resource.Properties) + }) +} + +func TestMakeUpdateFilters_DefaultsBeforeEncryption(t *testing.T) { + ucpClient, err := createFakeUCPClientFactory(map[string]any{ + "type": "object", + "properties": map[string]any{ + "password": map[string]any{ + "type": "string", + "default": "default-password", + "x-radius-sensitive": true, + }, + }, + }) + require.NoError(t, err) + + resource := &datamodel.DynamicResource{} + filters := makeUpdateFilters( + makeDefaultsFilter(ucpClient), + makeEncryptionFilter(ucpClient, createTestHandler(t)), + ) + for _, filter := range filters { + response, err := filter(createTestContext(), resource, nil, nil) + require.NoError(t, err) + require.Nil(t, response) + } + + encryptedPassword, ok := resource.Properties["password"].(map[string]any) + require.True(t, ok, "defaulted password should be encrypted") + require.Contains(t, encryptedPassword, "encrypted") + require.Contains(t, encryptedPassword, "nonce") + require.Contains(t, encryptedPassword, "version") +} diff --git a/pkg/dynamicrp/frontend/encryptionfilter.go b/pkg/dynamicrp/frontend/encryptionfilter.go index 4c6c4f6ccb1..c56bdf8d152 100644 --- a/pkg/dynamicrp/frontend/encryptionfilter.go +++ b/pkg/dynamicrp/frontend/encryptionfilter.go @@ -29,6 +29,8 @@ import ( "github.com/radius-project/radius/pkg/ucp/ucplog" ) +type encryptionUpdateFilter controller.UpdateFilter[datamodel.DynamicResource] + // makeEncryptionFilter creates an UpdateFilter that encrypts sensitive fields in the resource's // Properties map before saving to the database. // @@ -41,7 +43,7 @@ import ( func makeEncryptionFilter( ucpClient *v20231001preview.ClientFactory, handler *encryption.SensitiveDataHandler, -) controller.UpdateFilter[datamodel.DynamicResource] { +) encryptionUpdateFilter { return func( ctx context.Context, newResource *datamodel.DynamicResource, diff --git a/pkg/dynamicrp/frontend/routes.go b/pkg/dynamicrp/frontend/routes.go index 164a102a078..23f6343c031 100644 --- a/pkg/dynamicrp/frontend/routes.go +++ b/pkg/dynamicrp/frontend/routes.go @@ -53,17 +53,17 @@ func (s *Service) registerRoutes( // Create encryption filter for sensitive fields encryptionFilter := makeEncryptionFilter(ucpClient, handler) - // Materialize schema defaults before encryption, so a defaulted sensitive field is still encrypted. + // Apply defaults before encrypting sensitive fields. defaultsFilter := makeDefaultsFilter(ucpClient) // Resource options with encryption filter applied to PUT operations resourceOptions := controller.ResourceOptions[datamodel.DynamicResource]{ RequestConverter: converter.DynamicResourceDataModelFromVersioned, ResponseConverter: converter.DynamicResourceDataModelToVersioned, - UpdateFilters: []controller.UpdateFilter[datamodel.DynamicResource]{ + UpdateFilters: makeUpdateFilters( defaultsFilter, encryptionFilter, - }, + ), AsyncOperationRetryAfter: time.Second * 5, AsyncOperationTimeout: time.Hour * 24, } diff --git a/pkg/schema/defaults.go b/pkg/schema/defaults.go index bfbdd2e6890..e1d1bc73e6d 100644 --- a/pkg/schema/defaults.go +++ b/pkg/schema/defaults.go @@ -16,22 +16,16 @@ limitations under the License. package schema -// ApplyDefaults fills unset properties from the "default" declared in the schema and returns the -// number applied. +// ApplyDefaults adds defaults to missing properties and returns how many it added. // -// Explicit values are never overwritten. Required properties are never defaulted, so omitting one -// still fails validation. Read-only properties are recipe outputs, and an absent object is not -// created just to hold defaults. +// It keeps explicit values and skips required and read-only properties. It doesn't create missing +// objects or arrays unless their schema declares a default. func ApplyDefaults(properties map[string]any, schemaData map[string]any) int { if properties == nil || schemaData == nil { return 0 } - declared, ok := schemaData["properties"].(map[string]any) - if !ok { - return 0 - } - + declared, _ := schemaData["properties"].(map[string]any) required := requiredProperties(schemaData) applied := 0 @@ -45,12 +39,9 @@ func ApplyDefaults(properties map[string]any, schemaData map[string]any) int { continue } - // A supplied object is descended into even when required, since its own optional - // properties may declare defaults. + // Apply nested defaults to supplied objects and arrays, even when required. if existing, present := properties[name]; present { - if nested, ok := existing.(map[string]any); ok { - applied += ApplyDefaults(nested, fieldSchema) - } + applied += applyDefaultsToValue(existing, fieldSchema) continue } @@ -59,16 +50,50 @@ func ApplyDefaults(properties map[string]any, schemaData map[string]any) int { } if defaultValue, ok := fieldSchema["default"]; ok { - properties[name] = copyDefaultValue(defaultValue) + materialized := copyDefaultValue(defaultValue) + properties[name] = materialized applied++ + applied += applyDefaultsToValue(materialized, fieldSchema) } } + additionalProperties, ok := schemaData["additionalProperties"].(map[string]any) + if !ok { + return applied + } + + for name, existing := range properties { + if _, declared := declared[name]; declared { + continue + } + + applied += applyDefaultsToValue(existing, additionalProperties) + } + return applied } -// requiredProperties reads the schema's "required" list, accepting either the []any from JSON -// decoding or a []string. +func applyDefaultsToValue(value any, schemaData map[string]any) int { + switch typed := value.(type) { + case map[string]any: + return ApplyDefaults(typed, schemaData) + case []any: + itemSchema, ok := schemaData["items"].(map[string]any) + if !ok { + return 0 + } + + applied := 0 + for _, item := range typed { + applied += applyDefaultsToValue(item, itemSchema) + } + return applied + default: + return 0 + } +} + +// requiredProperties accepts required lists decoded as []any or []string. func requiredProperties(schemaData map[string]any) map[string]bool { required := map[string]bool{} @@ -88,8 +113,7 @@ func requiredProperties(schemaData map[string]any) map[string]bool { return required } -// copyDefaultValue deep copies a default so mutating a materialized property cannot reach back into -// a cached schema. +// copyDefaultValue copies maps and slices so resources don't share schema data. func copyDefaultValue(value any) any { switch typed := value.(type) { case map[string]any: diff --git a/pkg/schema/defaults_test.go b/pkg/schema/defaults_test.go index 282fdafa04b..402a3aeb802 100644 --- a/pkg/schema/defaults_test.go +++ b/pkg/schema/defaults_test.go @@ -99,6 +99,117 @@ func TestApplyDefaults(t *testing.T) { expected: map[string]any{"config": map[string]any{"tls": true, "mode": "require"}}, applied: 1, }, + { + name: "objects inside an array are recursed into", + properties: map[string]any{"workers": []any{ + map[string]any{"name": "first"}, + map[string]any{"name": "second", "replicas": 2}, + }}, + schema: map[string]any{"properties": map[string]any{ + "workers": map[string]any{ + "type": "array", + "items": map[string]any{ + "type": "object", + "properties": map[string]any{ + "name": map[string]any{"type": "string"}, + "replicas": map[string]any{"type": "integer", "default": 1}, + }, + }, + }, + }}, + expected: map[string]any{"workers": []any{ + map[string]any{"name": "first", "replicas": 1}, + map[string]any{"name": "second", "replicas": 2}, + }}, + applied: 1, + }, + { + name: "new object default is recursed into", + properties: map[string]any{}, + schema: map[string]any{"properties": map[string]any{ + "config": map[string]any{ + "type": "object", + "default": map[string]any{"name": "given"}, + "properties": map[string]any{ + "name": map[string]any{"type": "string"}, + "retries": map[string]any{"type": "integer", "default": 3}, + }, + }, + }}, + expected: map[string]any{"config": map[string]any{"name": "given", "retries": 3}}, + applied: 2, + }, + { + name: "new array default is recursed into", + properties: map[string]any{}, + schema: map[string]any{"properties": map[string]any{ + "workers": map[string]any{ + "type": "array", + "default": []any{map[string]any{"name": "first"}}, + "items": map[string]any{ + "type": "object", + "properties": map[string]any{ + "name": map[string]any{"type": "string"}, + "replicas": map[string]any{"type": "integer", "default": 1}, + }, + }, + }, + }}, + expected: map[string]any{"workers": []any{ + map[string]any{"name": "first", "replicas": 1}, + }}, + applied: 2, + }, + { + name: "additional property values are recursed into", + properties: map[string]any{"regions": map[string]any{ + "east": map[string]any{"name": "primary"}, + "west": map[string]any{"name": "secondary", "replicas": 3}, + }}, + schema: map[string]any{"properties": map[string]any{ + "regions": map[string]any{ + "type": "object", + "additionalProperties": map[string]any{ + "type": "object", + "properties": map[string]any{ + "name": map[string]any{"type": "string"}, + "replicas": map[string]any{"type": "integer", "default": 1}, + }, + }, + }, + }}, + expected: map[string]any{"regions": map[string]any{ + "east": map[string]any{"name": "primary", "replicas": 1}, + "west": map[string]any{"name": "secondary", "replicas": 3}, + }}, + applied: 1, + }, + { + name: "additional properties schema is not applied to declared properties", + properties: map[string]any{"settings": map[string]any{ + "fixed": map[string]any{}, + "custom": map[string]any{}, + }}, + schema: map[string]any{"properties": map[string]any{ + "settings": map[string]any{ + "type": "object", + "properties": map[string]any{ + "fixed": map[string]any{"type": "object"}, + }, + "additionalProperties": map[string]any{ + "type": "object", + "properties": map[string]any{ + "enabled": map[string]any{"type": "boolean", "default": true}, + }, + }, + }, + }}, + expected: map[string]any{"settings": map[string]any{ + "fixed": map[string]any{}, + "custom": map[string]any{"enabled": true}, + }}, + applied: 1, + }, { name: "absent object is not created to hold defaults", properties: map[string]any{}, @@ -169,7 +280,6 @@ func TestApplyDefaults_NilInputs(t *testing.T) { require.Equal(t, 0, ApplyDefaults(map[string]any{}, nil)) } -// A map or slice default must be copied, not shared with the schema. func TestApplyDefaults_DefaultsAreCopied(t *testing.T) { schemaData := map[string]any{"properties": map[string]any{ "tags": map[string]any{"type": "object", "default": map[string]any{"tier": "free"}}, @@ -188,7 +298,7 @@ func TestApplyDefaults_DefaultsAreCopied(t *testing.T) { require.Equal(t, []any{"a"}, second["origins"]) } -// Defaults declared by the resource types in resource-types-contrib. +// These shapes match resource-types-contrib schemas. func TestApplyDefaults_ContribShapes(t *testing.T) { schemaData := map[string]any{"properties": map[string]any{ "environment": map[string]any{"type": "string"}, @@ -206,7 +316,6 @@ func TestApplyDefaults_ContribShapes(t *testing.T) { require.NotContains(t, properties, "application") } -// A default on a required property must not fill it, or an omitted value would pass validation. func TestApplyDefaults_RequiredWinsOverDeclaredDefault(t *testing.T) { schemaData := map[string]any{ "required": []any{"environment", "database", "username", "password"}, @@ -222,7 +331,6 @@ func TestApplyDefaults_RequiredWinsOverDeclaredDefault(t *testing.T) { require.NotContains(t, properties, "database") } -// A supplied required object still needs its own optional properties defaulted. func TestApplyDefaults_NestedDefaultsInsideSuppliedRequiredObject(t *testing.T) { schemaData := map[string]any{ "required": []any{"config"}, @@ -247,7 +355,6 @@ func TestApplyDefaults_NestedDefaultsInsideSuppliedRequiredObject(t *testing.T) require.Equal(t, "given", config["name"]) } -// An absent required object is not created just to hold defaults. func TestApplyDefaults_AbsentRequiredObjectIsNotCreated(t *testing.T) { schemaData := map[string]any{ "required": []any{"config"},