Merge user values with defaults before validation.

[absoludity]

Signed-off-by: Michael Nelson [email protected]

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 Helm: Kubeapps doesn't consider the default values set from the chart #4803

Additional information

I've not yet tested this IRL to ensure it fixes the scenario in the issue, but will do so tomorrow.

[netlify]

✅ Deploy Preview for kubeapps-dev canceled.

Name	Link
🔨 Latest commit	03bd1b1
🔍 Latest deploy log	https://app.netlify.com/sites/kubeapps-dev/deploys/637c12e7a9fa0e00093d9655

[antgamdia]

Thanks! Nice improvement here. Notwithstanding, I don't think the actual problem is gonna get sorted out.
I'd recall I disabled the schema validation entirely in my dev environment and it was the backend that was complaining about the validation. I mean, the schema being used by helm itself when creating the Helm release.
I wish it was just a frontend validation issue, but I'm not 100% sure of that.

Keep us posted when you test it IRL

[absoludity]

Thanks! Nice improvement here. Notwithstanding, I don't think the actual problem is gonna get sorted out. I'd recall I disabled the schema validation entirely in my dev environment and it was the backend that was complaining about the validation. I mean, the schema being used by helm itself when creating the Helm release. I wish it was just a frontend validation issue, but I'm not 100% sure of that.

Keep us posted when you test it IRL

Yep, will do - I was just seeing where the problem starts and will follow it through. Thanks.

[absoludity]

The main issue I faced is that the auto-generated html form validation for required fields of the schema tries to force you to enter a value, even if the package has defaults for that value. I switched that off to test, and then it works fine to remove a required field if the package has a default for that field.

I mean, the schema being used by helm itself when creating the Helm release.

You mentioned this once before, but Helm itself does not have an issue when a user does not specify a required field - as long as that field has a default. For example, in my case, I've added an updated Wordpress chart to chart-museum on my dev cluster, but for the examples below, it can all just be done via the CLI (no chart-museum required). The differences with my wordpress chart are (1) that I modified the schema to say:

head -n5 ~/tmp/wordpress/wordpress/values.schema.json         
{
  "$schema": "http://json-schema.org/schema#",
  "type": "object",
  "required": ["wordpressUsername", "wordpressPassword", "wordpressBlogName"],
  "properties": {

So all three fields are required, but note in the values all 3 have a default value normally (even if it's an empty string for the password before it's set randomly). And (2), I updated the values.yaml so that we don't provide a default for wordpressBlogName only. With this, the validation correctly picks up that I, the user, needs to provide the blog name when installing the chart:

$ helm upgrade --install check-wordpress ./wordpress-15.2.14.tgz
Error: UPGRADE FAILED: values don't meet the specifications of the schema(s) in the following chart(s):
wordpress:
- (root): wordpressBlogName is required

But if I provide the blog name, I'm not forced to provide the user name as well (since it has a default):

$ helm upgrade --install check-wordpress ./wordpress-15.2.14.tgz --set wordpressBlogName=test
Release "check-wordpress" has been upgraded. Happy Helming!
NAME: check-wordpress
LAST DEPLOYED: Thu Nov 10 14:21:05 2022
NAMESPACE: default

Once it's set, I can even upgrade without specifying the blog name, since it's already set:

helm upgrade --install check-wordpress ./wordpress-15.2.14.tgz                             
Release "check-wordpress" has been upgraded. Happy Helming!
NAME: check-wordpress
LAST DEPLOYED: Thu Nov 10 14:21:42 2022
NAMESPACE: default
STATUS: deployed
REVISION: 3

So AIUI,

• On install: the values provided by the user are merged with the default values by Helm. It does not raise a validation error for required fields which the user has not specified iff the required field has a default set, and
• On upgrade: the values provided by the user are merged with the deployed values, so no validation error is raised even if I don't set a required field without a default as it's already been set (not very declarative :) ). I'd have to unset that field to reproduce the validation (which I don't seem to be able to do with a string field - helm just claims that null is the wrong type).

Or, if I've missed something, maybe show me what you mean by the Helm CLI validation rejecting a required value that has a default?

[absoludity]

Let me know if you're OK with the extra changes @antgamdia (in particular, switching off the html validation for required fields, since it cannot take into account default values), and the explanation above.

[antgamdia]

Let me know if you're OK with the extra changes @antgamdia (in particular, switching off the html validation for required fields, since it cannot take into account default values), and the explanation above.

Sorry, I've been traveling with limited internet connection, but yep, I'm totally ok with both the changes and reasoning. Thanks, as always, for the details!

maybe show me what you mean by the Helm CLI validation rejecting a required value that has a default?

No, no, I did mean this rejection happening when using a value without a default one. Anyway, currently, BAC is not requiring any fields (bitnami/readme-generator-for-helm#38), so this issue shouldn't be happening in a default Kubeapps installation (w/o using your repo w/ your schemas).

So, as you proposed, merging default's before validation seems like a good solution. Thanks!