From bb80588298cd92eff424174249d7080c98edf812 Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Tue, 22 Nov 2022 13:24:20 +1100 Subject: [PATCH] Merge user values with defaults before validation. (#5623) Signed-off-by: Michael Nelson ### Description of the change Updates the `validateValuesSchema` function to additionally use the package default values, when specified, so that user values will be confirmed as valid even if they don't set certain values that are required by the schema but have default values specified by the package. ### Benefits Enables the scenario outlined by @dud225 in #4803, where a helm chart is deployed via the Helm CLI, rather than Kubeapps, and so the values set by the user are only those actually set by the user (as opposed to when deployed with Kubeapps where we currently incorrectly send all the default values as user-set values - I may open a separate issue for that, depending on a few things). ### Possible drawbacks ### Applicable issues - fixes #4803 ### Additional information I've not yet tested this IRL to ensure it fixes the scenario in the issue, but will do so tomorrow. Signed-off-by: Michael Nelson --- dashboard/src/actions/installedpackages.ts | 12 ++++- dashboard/src/shared/schema.test.ts | 54 +++++++++++++++++++++- dashboard/src/shared/schema.ts | 21 +++++++-- 3 files changed, 79 insertions(+), 8 deletions(-) diff --git a/dashboard/src/actions/installedpackages.ts b/dashboard/src/actions/installedpackages.ts index a4b56702e70..7cb60d9ab54 100644 --- a/dashboard/src/actions/installedpackages.ts +++ b/dashboard/src/actions/installedpackages.ts @@ -243,7 +243,11 @@ export function installPackage( `The schema for this package is not valid. Please contact the package author. The following errors were found:\n${errorText}`, ); } - const validation = validateValuesSchema(values, schema); + const validation = validateValuesSchema( + values, + schema, + availablePackageDetail.defaultValues, + ); if (!validation.valid) { const errorText = validation?.errors ?.map(e => ` - ${e.instancePath}: ${e.message}`) @@ -301,7 +305,11 @@ export function updateInstalledPackage( `The schema for this package is not valid. Please contact the package author. The following errors were found:\n${errorText}`, ); } - const validation = validateValuesSchema(values, schema); + const validation = validateValuesSchema( + values, + schema, + availablePackageDetail.defaultValues, + ); if (!validation.valid) { const errorText = validation?.errors ?.map(e => ` - ${e.instancePath}: ${e.message}`) diff --git a/dashboard/src/shared/schema.test.ts b/dashboard/src/shared/schema.test.ts index 512a9c7205b..82f3d9cc385 100644 --- a/dashboard/src/shared/schema.test.ts +++ b/dashboard/src/shared/schema.test.ts @@ -434,15 +434,67 @@ describe("validateValuesSchema", () => { { description: "Should validate a valid object", values: "foo: bar\n", + defaultValues: "foo: default\n", schema: { properties: { foo: { type: "string" } }, }, valid: true, errors: null, }, + { + description: "Should error if required value not provided", + values: "foo: bar\n", + defaultValues: "foo: default\n", + schema: { + required: ["bar"], + properties: { + foo: { type: "string" }, + bar: { type: "string" }, + }, + }, + valid: false, + errors: [ + { + keyword: "required", + instancePath: "", + message: "must have required property 'bar'", + params: { missingProperty: "bar" }, + schemaPath: "#/required", + }, + ], + }, + { + description: "Should not error if required value has a default", + values: "foo: bar\n", + defaultValues: "foo: default\nbar: default\n", + schema: { + required: ["bar"], + properties: { + foo: { type: "string" }, + bar: { type: "string" }, + }, + }, + valid: true, + errors: null, + }, + { + description: "Should not error if extra value not present in defaults is included", + values: "foo: bar\nelsewhere: bang", + defaultValues: "foo: default\nbar: default\n", + schema: { + required: ["bar"], + properties: { + foo: { type: "string" }, + bar: { type: "string" }, + }, + }, + valid: true, + errors: null, + }, { description: "Should validate an invalid object", values: "foo: bar\n", + defaultValues: "foo: 1\n", schema: { properties: { foo: { type: "integer" } } }, valid: false, errors: [ @@ -459,7 +511,7 @@ describe("validateValuesSchema", () => { }, ].forEach(t => { it(t.description, () => { - const res = validateValuesSchema(t.values, t.schema); + const res = validateValuesSchema(t.values, t.schema, t.defaultValues); expect(res.valid).toBe(t.valid); expect(res.errors).toEqual(t.errors); }); diff --git a/dashboard/src/shared/schema.ts b/dashboard/src/shared/schema.ts index e80cd65e2dc..96ffb9aed01 100644 --- a/dashboard/src/shared/schema.ts +++ b/dashboard/src/shared/schema.ts @@ -5,7 +5,8 @@ import Ajv, { ErrorObject, JSONSchemaType } from "ajv"; import { findIndex, isEmpty, set } from "lodash"; import { DeploymentEvent, IAjvValidateResult, IBasicFormParam } from "shared/types"; import YAML from "yaml"; -import { getPathValueInYamlNode, getPathValueInYamlNodeWithDefault, parseToJS } from "./yamlUtils"; +import * as jsonpatch from "fast-json-patch"; +import { getPathValueInYamlNode, getPathValueInYamlNodeWithDefault } from "./yamlUtils"; const ajv = new Ajv({ strict: false }); @@ -22,7 +23,6 @@ export function retrieveBasicFormParams( let params: IBasicFormParam[] = []; if (schema?.properties && !isEmpty(schema.properties)) { const properties = schema.properties; - const requiredProperties = schema.required; const schemaExamples = schema.examples; Object.keys(properties).forEach(propertyKey => { const schemaProperty = properties[propertyKey] as JSONSchemaType; @@ -59,8 +59,9 @@ export function retrieveBasicFormParams( : undefined, // get the string values of the enum array enum: schemaProperty?.enum?.map((item: { toString: () => any }) => item?.toString() ?? ""), - // check if the "required" array contains the current property - isRequired: requiredProperties?.includes(propertyKey), + // We leave the validation of user values to the Helm backend, since it will take + // into account default values (on install) and previously set values (on upgrade). + isRequired: false, examples: examples, // If exists, the value that is currently deployed deployedValue: isLeaf @@ -155,8 +156,18 @@ export function schemaToObject(schema?: string): JSONSchemaType { export function validateValuesSchema( values: string, schema: JSONSchemaType | any, + defaultValues?: string, ): { valid: boolean; errors: ErrorObject[] | null | undefined } { - const valid = ajv.validate(schema, parseToJS(values)); + let valuesToCheck = YAML.parse(values); + if (defaultValues) { + const defaultYAML = YAML.parse(defaultValues); + let patches = jsonpatch.compare(defaultYAML as any, valuesToCheck as any); + patches = patches.filter(function (d) { + return ["add", "replace"].includes(d.op); + }); + valuesToCheck = jsonpatch.applyPatch(defaultYAML, patches).newDocument; + } + const valid = ajv.validate(schema, valuesToCheck); return { valid: !!valid, errors: ajv.errors } as IAjvValidateResult; }