lunny/helm-chart
1
0
Fork 1
Code Issues36 Pull Requests14 Actions Packages Projects Releases93 Wiki Activity

WIP: Add validation schema #198

Closed
justusbunsi wants to merge 20 commits from feature/92-validation-schema into main
Conversation 8 Commits 20 Files Changed 3 +1883 -7957
justusbunsi commented 2021-06-30 13:37:29 +00:00 (Migrated from gitea.com)

⚠️ This is not yet done but more transparent as PR than just being on a random branch on my fork. 😉

And we'll see how the CI can handle it.

Before setting this as non-wip (after everything done):

  • drop first two commits
  • ensure top level "additionalProperties" is set to false
  • consider reverting changes outside of values.schema.json
  • Cleanup helpers.tpl: conditional key handling like enabled in gitea.oauth_settings
⚠️ This is not yet done but more transparent as PR than just being on a random branch on my fork. 😉 And we'll see how the CI can handle it. Before setting this as non-wip (after everything done): - [ ] drop first two commits - [ ] ensure top level "additionalProperties" is set to false - [ ] consider reverting changes outside of values.schema.json - [ ] Cleanup _helpers.tpl_: conditional key handling like _enabled_ in `gitea.oauth_settings`
justusbunsi commented 2021-07-02 15:58:55 +00:00 (Migrated from gitea.com)

I think all conditional logic in values.schema.json has to be dropped since Helm supports using/merging multiple files for template rendering. Right now our values.yaml is pretty much a default configuration set. As long as there are options enabled that should be exclusively set (e.g. gitea.admin.existingSecret vs. gitea.admin.username + gitea.admin.password) having such conditional validation seems to stop the entire template rendering due to unmatched conditions.

This would eliminate some really needed validation. ?

I think all conditional logic in _values.schema.json_ has to be dropped since Helm supports using/merging multiple files for template rendering. Right now our values.yaml is pretty much a default configuration set. As long as there are options enabled that should be exclusively set (e.g. `gitea.admin.existingSecret` vs. `gitea.admin.username` + `gitea.admin.password`) having such conditional validation seems to stop the entire template rendering due to unmatched conditions. This would eliminate some really needed validation. ?
justusbunsi commented 2021-09-29 15:22:00 +00:00 (Migrated from gitea.com)

We may need to discuss what's going to be the main purpose of a validation scheme for this Helm Chart. Currently I see various different ways to go and I'd like to discuss them prior further implementation.

  1. Just type safety for Chart related fields
  2. Validate Chart related fields only + type safety by either:
    2.1. Only ensure requirements are fulfilled; no conflict checks
    2.2. Ensure integrity of provided (combined) values sources
  3. All-in validation by either:
    3.1. Way 2.2 + Basic type validation for Kubernetes specific fields
    3.2. Way 3.1 + Basic integrity check for Kubernetes specific fields
    3.3. Way 3.2 + Ensure proper values for Kubernetes specific fields

Some more details for each way:

Way 2.1. would mean: When providing gitea.admin.username you'd need to set gitea.admin.password as well but it does not matter if one of them is missing as soon as you have defined gitea.admin.existingSecret. An provided reference to an existing secret would be always preferred over the inline values.

Way 2.2. would mean: If providing gitea.admin.existingSecret you must not have gitea.admin.username or gitea.admin.password). This could potentially mean that we need to drop default values for conflicting fields in favor of null values to distinguish between defined and non-defined ones. I'm not fully sure if this is really necessary, we'd need to evaluate the behavior of Helm when combining multiple values sources. Nonetheless, this way would prevent configuration constellations to cause templating errors (only for chart related fields) and we might be able to cleanup some checks inside the template files.

Way 3.1. would mean we don't care about consistency of fields passed directly to Kubernetes (e.g. extraVolumeMounts and let the cluster decide whether to like the configuration or not which does not prevent the Helm Chart from being deployed into the cluster.

Way 3.2. would prevent the scenario of way 3.1 since it would not allow you to generate such specifications and apply them to the cluster.

Way 3.3. might be the most strict one but from a Helm Chart's point of view ensures the best possible health for both Helm Chart and the cluster it is running in. It won't let you apply configurations that will not work. On the other hand it would require us to keep track of what's changing in various Kubernetes versions to support a wide version range. Which would be a massive recurring task.

I currently prefer 3.1. (maybe even 3.2.). What do you all think?

We may need to discuss what's going to be the main purpose of a validation scheme for this Helm Chart. Currently I see various different ways to go and I'd like to discuss them prior further implementation. 1. Just _type safety_ for Chart related fields 2. _Validate Chart related_ fields only + _type safety_ by either: 2.1. Only ensure requirements are fulfilled; no conflict checks 2.2. Ensure integrity of provided (combined) values sources 3. _All-in_ validation by either: 3.1. Way 2.2 + Basic type validation for Kubernetes specific fields 3.2. Way 3.1 + Basic integrity check for Kubernetes specific fields 3.3. Way 3.2 + Ensure proper values for Kubernetes specific fields Some more details for each way: Way 2.1. would mean: When providing `gitea.admin.username` you'd need to set `gitea.admin.password` as well but it does not matter if one of them is missing as soon as you have defined `gitea.admin.existingSecret`. An provided reference to an existing secret would be always preferred over the inline values. Way 2.2. would mean: If providing `gitea.admin.existingSecret` you must not have `gitea.admin.username` or `gitea.admin.password`). This could potentially mean that we need to drop default values for conflicting fields in favor of `null` values to distinguish between defined and non-defined ones. I'm not fully sure if this is really necessary, we'd need to evaluate the behavior of Helm when combining multiple values sources. Nonetheless, this way would prevent configuration constellations to cause templating errors (only for chart related fields) and we might be able to cleanup some checks inside the template files. Way 3.1. would mean we don't care about consistency of fields passed directly to Kubernetes (e.g. `extraVolumeMounts` and let the cluster decide whether to like the configuration or not which does not prevent the Helm Chart from being deployed into the cluster. Way 3.2. would prevent the scenario of way 3.1 since it would not allow you to generate such specifications and apply them to the cluster. Way 3.3. might be the most strict one but from a Helm Chart's point of view ensures the best possible health for both Helm Chart and the cluster it is running in. It won't let you apply configurations that will not work. On the other hand it would require us to keep track of what's changing in various Kubernetes versions to support a wide version range. Which would be a massive recurring task. --- I currently prefer 3.1. (maybe even 3.2.). What do you all think?
pat-s commented 2021-09-30 21:52:05 +00:00 (Migrated from gitea.com)

Thanks for taking the time to describe your thoughts in such a detail.

I think 3.1 would be great to have as it prevents the user from combining conflicting settings which may lead to unpredictable behaviour (which is hard to debug).

IIUC 3.2 would prevent from setting k8s fields that would not necessarily have an effect on Gitea. I can't really think of a good example here?

I think 2.2 is the main part here as Gitea can be a monster to some degree and it is not always obvious which settings might conflict with each other.

I don't think we should add too much k8s specific validation as the cost/value ratio here is rather small - so my vote goes to 3.1 (which is already a lot of work).

Thanks for taking the time to describe your thoughts in such a detail. I think 3.1 would be great to have as it prevents the user from combining conflicting settings which may lead to unpredictable behaviour (which is hard to debug). IIUC 3.2 would prevent from setting k8s fields that would not necessarily have an effect on Gitea. I can't really think of a good example here? I think 2.2 is the main part here as Gitea can be a monster to some degree and it is not always obvious which settings might conflict with each other. I don't think we should add too much k8s specific validation as the cost/value ratio here is rather small - so my vote goes to 3.1 (which is already a lot of work).
justusbunsi commented 2022-01-27 06:37:23 +00:00 (Migrated from gitea.com)

FYI: Helm lint seems to have more verbose output these days.

https://kubelist.com/issue/latest/ and https://www.arthurkoziel.com/validate-helm-chart-values-with-json-schemas/

FYI: Helm lint seems to have more verbose output these days. https://kubelist.com/issue/latest/ and https://www.arthurkoziel.com/validate-helm-chart-values-with-json-schemas/
justusbunsi commented 2022-06-11 17:56:35 +00:00 (Migrated from gitea.com)

We use https://github.com/bitnami-labs/readme-generator-for-helm to extract parameters from values.yaml. It also supports generating schema files from it.

I've tested that feature. Unfortunately, we are facing https://github.com/bitnami-labs/readme-generator-for-helm/issues/34. So right now not usable for schema.

We use https://github.com/bitnami-labs/readme-generator-for-helm to extract parameters from `values.yaml`. It also supports generating schema files from it. I've tested that feature. Unfortunately, we are facing https://github.com/bitnami-labs/readme-generator-for-helm/issues/34. So right now not usable for schema.
pat-s commented 2023-04-01 07:32:53 +00:00 (Migrated from gitea.com)

@justusbunsi Do you want to continue working on this or shall we close for the moment? Just asking as it's already around for two years.

@justusbunsi Do you want to continue working on this or shall we close for the moment? Just asking as it's already around for two years.
justusbunsi commented 2023-04-01 12:55:07 +00:00 (Migrated from gitea.com)

😅 At some point, yes. It will help validating the values. But let's close it for now. I keep the branch on my fork. If anyone want to continue with it before I do, feel free to use the branch as base or inspiration.

😅 At some point, yes. It will help validating the values. But let's close it for now. I keep the branch on my fork. If anyone want to continue with it before I do, feel free to use the branch as base or inspiration.
justusbunsi commented 2023-04-01 12:57:10 +00:00 (Migrated from gitea.com)

Apparently I cannot close the PR before closing the blocking issue. But this is still a valid one. So I am removing the dependency from this PR.

Apparently I cannot close the PR before closing the blocking issue. But this is still a valid one. So I am removing the dependency from this PR.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
No Reviewers
No Label
kind/feature
status/blocked
No Milestone
No project
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: lunny/helm-chart#198
No description provided.
Delete Branch "feature/92-validation-schema"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?