feat(common): add schema validation constraints from documentation#44934
feat(common): add schema validation constraints from documentation#44934Crow-Control merged 4 commits intocommon2026from
Conversation
- Set required array in schemas when fields marked as required in docs - Add minLength: 1 for required string fields - Add minimum: 1 for required integer fields - Updated 47 schema files to match documentation requirements Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
- Remove required fields from top-level container schemas (rbac, configmap, secret, etc.) - Keep required constraints only for nested instance schemas (persistence/nfs, service/ports, etc.) - Top-level containers shouldn't enforce required fields when empty by default Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
- Allow LUN 0 for iSCSI (valid iSCSI configuration) - Allow UID/GID 0 for securityContext (root user/group is valid use case) - Keep retry attempts as string per documentation Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Crow-Control
requested review from
Boemeltrein,
StevenMcElligott,
alfi0812,
stavros-k and
xstar97
as code owners
There was a problem hiding this comment.
Pull request overview
This pull request attempts to add schema validation constraints (required fields, minLength, minimum) to 39 JSON schema files in the common chart library, based on documentation requirements. However, the implementation contains critical structural issues that will cause validation failures.
Changes:
- Added
requiredarrays and validation constraints to 39 schema files - Applied constraints for strings (minLength: 1), integers (minimum: 1 or 0), and booleans
- Attempted to enforce documentation requirements through schema validation
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| workload/cronjob.json | Correctly adds required schedule field - ✅ |
| service/ports.json | Correctly adds required port field - ✅ |
| service/NodePort.json | Correctly adds required nodePort field - ✅ |
| service/ExternalName.json | Correctly adds required externalName field - ✅ |
| service/ExternalIP.json | Correctly adds required externalIP field - ✅ |
| persistence/secret.json | ❌ Incorrectly flattens items[].key and items[].path to top level |
| persistence/pvc-vct/static-smb.json | Correctly adds required fields for SMB - ✅ |
| persistence/pvc-vct/static-nfs.json | Correctly adds required fields for NFS - ✅ |
| persistence/pvc-vct/static-custom.json | Correctly adds required fields for custom provisioner - ✅ |
| persistence/nfs.json | Correctly adds required path and server - ✅ |
| persistence/iscsi.json | ❌ Incorrectly flattens iscsi.* nested properties to top level |
| persistence/hostPath.json | Correctly adds required hostPath - ✅ |
| persistence/device.json | Correctly adds required hostPath - ✅ |
| persistence/configmap.json | ❌ Incorrectly flattens items[].key and items[].path to top level |
| middlewares/traefik/strip-prefix.json | Correctly adds required prefix - ✅ |
| middlewares/traefik/strip-prefix-regex.json | Correctly adds required regex - ✅ |
| middlewares/traefik/retry.json | Correctly adds required attempts - ✅ |
| middlewares/traefik/replace-path.json | Correctly adds required path - ✅ |
| middlewares/traefik/replace-path-regex.json | Correctly adds required regex and replacement - ✅ |
| middlewares/traefik/redirect-scheme.json | Correctly adds required scheme - ✅ |
| middlewares/traefik/redirect-regex.json | Correctly adds required regex and replacement - ✅ |
| middlewares/traefik/plugin-theme-park.json | Correctly adds required app and theme - ✅ |
| middlewares/traefik/plugin-rewrite-response-headers.json | Requires verification of nested structure |
| middlewares/traefik/plugin-real-ip.json | Correctly adds required excludednets - ✅ |
| middlewares/traefik/plugin-mod-security.json | Correctly adds required modSecurityUrl - ✅ |
| middlewares/traefik/plugin-geoblock.json | Correctly adds required api and countries - ✅ |
| middlewares/traefik/plugin-bouncer.json | Correctly adds required enabled - ✅ |
| middlewares/traefik/ip-allow-list.json | ❌ Incorrectly flattens nested structure and uses wrong types for lists |
| middlewares/traefik/forward-auth.json | ❌ Uses string type for list fields (authResponseHeaders, authRequestHeaders) |
| middlewares/traefik/chain.json | Requires verification of required fields necessity |
| middlewares/traefik/basic-auth.json | ❌ Incorrectly structures mutually exclusive fields and flattens array properties |
| middlewares/traefik/add-prefix.json | Correctly adds required prefix - ✅ |
| ingress/homepage.json | ❌ Incorrectly flattens widget.customkv[].key and value to top level |
| ingress/certManager.json | Correctly adds required enabled - ✅ |
| global.json | Adds required minNodePort - potential breaking change |
| container/securityContext.json | ❌ Incorrectly flattens nested properties (capabilities.drop, seccompProfile.profile) |
| container/resources.json | ❌ Incorrectly flattens resources.requests.memory to top level |
| container/probes.json | ❌ Incorrectly flattens probes.liveness and probes.readiness to top level |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "authRequestHeaders": { | ||
| "type": "string", | ||
| "minLength": 1 |
There was a problem hiding this comment.
According to the documentation at docs/middlewares/traefik/forward-auth.md, authResponseHeaders and authRequestHeaders are defined as "list of string" types, not simple strings. These fields should have type "array" or "object" (following the schema convention for lists) instead of "string" with minLength.
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "authRequestHeaders": { | |
| "type": "string", | |
| "minLength": 1 | |
| "type": "array", | |
| "items": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| }, | |
| "authRequestHeaders": { | |
| "type": "array", | |
| "items": { | |
| "type": "string", | |
| "minLength": 1 | |
| } |
| "type": "object" | ||
| }, | ||
| "username": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "password": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "secret": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| } | ||
| }, | ||
| "additionalProperties": true, | ||
| "required": [ | ||
| "password", | ||
| "secret", | ||
| "username", | ||
| "users" |
There was a problem hiding this comment.
According to the documentation at docs/middlewares/traefik/basic-auth.md, username and password are required fields within the users array (users[].username, users[].password), not at the top level of the schema. Additionally, users and secret are mutually exclusive - only one should be present. Making all four fields (users, username, password, secret) required at the top level contradicts the documented behavior where users is a list of objects, and either users OR secret should be provided, but not both.
| "type": "object" | |
| }, | |
| "username": { | |
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "password": { | |
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "secret": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| }, | |
| "additionalProperties": true, | |
| "required": [ | |
| "password", | |
| "secret", | |
| "username", | |
| "users" | |
| "type": "array", | |
| "items": { | |
| "type": "object", | |
| "properties": { | |
| "username": { | |
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "password": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| }, | |
| "required": [ | |
| "username", | |
| "password" | |
| ], | |
| "additionalProperties": true | |
| } | |
| }, | |
| "secret": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| }, | |
| "additionalProperties": true, | |
| "oneOf": [ | |
| { | |
| "required": [ | |
| "users" | |
| ] | |
| }, | |
| { | |
| "required": [ | |
| "secret" | |
| ] | |
| } |
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "ipStrategy": { | ||
| "type": "object" | ||
| }, | ||
| "depth": { | ||
| "type": "integer", | ||
| "minimum": 1 | ||
| }, | ||
| "excludedIPs": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| } | ||
| }, | ||
| "additionalProperties": true, | ||
| "required": [ | ||
| "depth", | ||
| "excludedIPs", | ||
| "ipStrategy", |
There was a problem hiding this comment.
According to the documentation at docs/middlewares/traefik/ip-allow-list.md, sourceRange and excludedIPs are defined as "list of string" types, not simple strings. The nested structure shows depth and excludedIPs should be under ipStrategy (ipStrategy.depth, ipStrategy.excludedIPs), not flattened at the top level. The schema should reflect the proper nesting and use array/object types instead of string.
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "ipStrategy": { | |
| "type": "object" | |
| }, | |
| "depth": { | |
| "type": "integer", | |
| "minimum": 1 | |
| }, | |
| "excludedIPs": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| }, | |
| "additionalProperties": true, | |
| "required": [ | |
| "depth", | |
| "excludedIPs", | |
| "ipStrategy", | |
| "type": "array", | |
| "items": { | |
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "minItems": 1 | |
| }, | |
| "ipStrategy": { | |
| "type": "object", | |
| "properties": { | |
| "depth": { | |
| "type": "integer", | |
| "minimum": 1 | |
| }, | |
| "excludedIPs": { | |
| "type": "array", | |
| "items": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| } | |
| }, | |
| "additionalProperties": false | |
| } | |
| }, | |
| "additionalProperties": true, | |
| "required": [ |
| }, | ||
| "memory": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| } | ||
| }, | ||
| "required": [ | ||
| "memory", |
There was a problem hiding this comment.
According to the documentation at docs/container/resources.md, memory is defined as resources.requests.memory (a nested field), not as a top-level property. The schema should respect the documented nested structure where memory is under resources.requests, not flattened to the top level.
| }, | |
| "memory": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| }, | |
| "required": [ | |
| "memory", | |
| } | |
| }, | |
| "required": [ |
| }, | ||
| "key": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "path": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| } | ||
| }, | ||
| "required": [ | ||
| "key", | ||
| "objectName", | ||
| "path" |
There was a problem hiding this comment.
According to the documentation at docs/persistence/configmap.md, key and path are required fields within the items array (items[].key, items[].path), not at the top level. Only objectName is required at the top level. The schema incorrectly flattens these array element properties to the top level.
| }, | |
| "key": { | |
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "path": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| }, | |
| "required": [ | |
| "key", | |
| "objectName", | |
| "path" | |
| } | |
| }, | |
| "required": [ | |
| "objectName" |
| "properties": { | ||
| "securityContext": { | ||
| "type": "object" | ||
| }, | ||
| "runAsUser": { | ||
| "type": "integer", | ||
| "minimum": 0 | ||
| }, | ||
| "runAsGroup": { | ||
| "type": "integer", | ||
| "minimum": 0 | ||
| }, | ||
| "readOnlyRootFilesystem": { | ||
| "type": "boolean" | ||
| }, | ||
| "allowPrivilegeEscalation": { | ||
| "type": "boolean" | ||
| }, | ||
| "privileged": { | ||
| "type": "boolean" | ||
| }, | ||
| "runAsNonRoot": { | ||
| "type": "boolean" | ||
| }, | ||
| "drop": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "profile": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| } | ||
| }, | ||
| "required": [ | ||
| "allowPrivilegeEscalation", | ||
| "drop", | ||
| "privileged", | ||
| "profile", | ||
| "readOnlyRootFilesystem", | ||
| "runAsGroup", | ||
| "runAsNonRoot", | ||
| "runAsUser", | ||
| "securityContext" | ||
| ] |
There was a problem hiding this comment.
According to the documentation at docs/container/securityContext.md, drop is defined as capabilities.drop (a nested field under capabilities that is a "list of string"), and profile is defined as seccompProfile.profile (a nested field under seccompProfile). These fields should not be flattened to the top level of the schema. The schema structure should match the documented nested hierarchy.
| "properties": { | |
| "securityContext": { | |
| "type": "object" | |
| }, | |
| "runAsUser": { | |
| "type": "integer", | |
| "minimum": 0 | |
| }, | |
| "runAsGroup": { | |
| "type": "integer", | |
| "minimum": 0 | |
| }, | |
| "readOnlyRootFilesystem": { | |
| "type": "boolean" | |
| }, | |
| "allowPrivilegeEscalation": { | |
| "type": "boolean" | |
| }, | |
| "privileged": { | |
| "type": "boolean" | |
| }, | |
| "runAsNonRoot": { | |
| "type": "boolean" | |
| }, | |
| "drop": { | |
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "profile": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| }, | |
| "required": [ | |
| "allowPrivilegeEscalation", | |
| "drop", | |
| "privileged", | |
| "profile", | |
| "readOnlyRootFilesystem", | |
| "runAsGroup", | |
| "runAsNonRoot", | |
| "runAsUser", | |
| "securityContext" | |
| ] | |
| "properties": { | |
| "securityContext": { | |
| "type": "object" | |
| }, | |
| "runAsUser": { | |
| "type": "integer", | |
| "minimum": 0 | |
| }, | |
| "runAsGroup": { | |
| "type": "integer", | |
| "minimum": 0 | |
| }, | |
| "readOnlyRootFilesystem": { | |
| "type": "boolean" | |
| }, | |
| "allowPrivilegeEscalation": { | |
| "type": "boolean" | |
| }, | |
| "privileged": { | |
| "type": "boolean" | |
| }, | |
| "runAsNonRoot": { | |
| "type": "boolean" | |
| }, | |
| "capabilities": { | |
| "type": "object", | |
| "properties": { | |
| "drop": { | |
| "type": "array", | |
| "items": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| } | |
| } | |
| }, | |
| "seccompProfile": { | |
| "type": "object", | |
| "properties": { | |
| "profile": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| } | |
| } | |
| }, | |
| "required": [ | |
| "allowPrivilegeEscalation", | |
| "capabilities", | |
| "privileged", | |
| "readOnlyRootFilesystem", | |
| "runAsGroup", | |
| "runAsNonRoot", | |
| "runAsUser", | |
| "seccompProfile", | |
| "securityContext" | |
| ] |
| "type": "object" | ||
| }, | ||
| "liveness": { | ||
| "type": "object" | ||
| }, | ||
| "readiness": { | ||
| "type": "object" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "liveness", | ||
| "probes", | ||
| "readiness" |
There was a problem hiding this comment.
According to the documentation at docs/container/probes.md, liveness and readiness are nested under the probes key (probes.liveness, probes.readiness), not flattened at the top level. The schema structure should match the documented nested hierarchy where these are properties of the probes object.
| "type": "object" | |
| }, | |
| "liveness": { | |
| "type": "object" | |
| }, | |
| "readiness": { | |
| "type": "object" | |
| } | |
| }, | |
| "required": [ | |
| "liveness", | |
| "probes", | |
| "readiness" | |
| "type": "object", | |
| "properties": { | |
| "liveness": { | |
| "type": "object" | |
| }, | |
| "readiness": { | |
| "type": "object" | |
| } | |
| }, | |
| "required": [ | |
| "liveness", | |
| "readiness" | |
| ] | |
| } | |
| }, | |
| "required": [ | |
| "probes" |
| "description": "Global values that apply to all charts See more info about global values [here](/truecharts-common/global)", | ||
| "required": [ | ||
| "minNodePort" | ||
| ] | ||
| } |
There was a problem hiding this comment.
Making minNodePort a required field in the global schema could be problematic. While the documentation marks it as required and provides a default value of 9000, enforcing this at the schema level means any chart that doesn't explicitly set this value will fail validation. This is a breaking change for existing charts that rely on the default. Consider whether this field should truly be required at the schema validation level, or if the documentation's "required" designation is meant to indicate "should be set" rather than "must be validated."
| "description": "Global values that apply to all charts See more info about global values [here](/truecharts-common/global)", | |
| "required": [ | |
| "minNodePort" | |
| ] | |
| } | |
| "description": "Global values that apply to all charts See more info about global values [here](/truecharts-common/global)" | |
| } |
| "type": "object" | ||
| }, | ||
| "targetPortal": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "iqn": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "lun": { | ||
| "type": "integer", | ||
| "minimum": 0 | ||
| } | ||
| }, | ||
| "additionalProperties": true, | ||
| "required": [ | ||
| "iqn", | ||
| "iscsi", | ||
| "lun", | ||
| "targetPortal" |
There was a problem hiding this comment.
According to the examples in docs/persistence/iscsi.md, targetPortal, iqn, and lun should be nested under the iscsi object, not at the top level. The examples show the structure as persistence.$name.iscsi.targetPortal, persistence.$name.iscsi.iqn, and persistence.$name.iscsi.lun (see lines 93-94, 116-117, 139-140 of the documentation). The schema incorrectly places these fields at the same level as the iscsi object.
| "type": "object" | |
| }, | |
| "targetPortal": { | |
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "iqn": { | |
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "lun": { | |
| "type": "integer", | |
| "minimum": 0 | |
| } | |
| }, | |
| "additionalProperties": true, | |
| "required": [ | |
| "iqn", | |
| "iscsi", | |
| "lun", | |
| "targetPortal" | |
| "type": "object", | |
| "properties": { | |
| "targetPortal": { | |
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "iqn": { | |
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "lun": { | |
| "type": "integer", | |
| "minimum": 0 | |
| } | |
| }, | |
| "required": [ | |
| "targetPortal", | |
| "iqn", | |
| "lun" | |
| ], | |
| "additionalProperties": true | |
| } | |
| }, | |
| "additionalProperties": true, | |
| "required": [ | |
| "iscsi" |
| "key": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "value": { | ||
| "type": "string", | ||
| "minLength": 1 | ||
| } | ||
| }, | ||
| "additionalProperties": true, | ||
| "required": [ | ||
| "enabled", | ||
| "key", | ||
| "value" |
There was a problem hiding this comment.
According to the documentation at docs/ingress/homepage.md, key and value are required fields within the widget.customkv array (widget.customkv[].key, widget.customkv[].value), not at the top level. Only enabled is required at the top level. The schema incorrectly flattens these array element properties to the top level.
| "key": { | |
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "value": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| }, | |
| "additionalProperties": true, | |
| "required": [ | |
| "enabled", | |
| "key", | |
| "value" | |
| "widget": { | |
| "type": "object", | |
| "properties": { | |
| "customkv": { | |
| "type": "array", | |
| "items": { | |
| "type": "object", | |
| "properties": { | |
| "key": { | |
| "type": "string", | |
| "minLength": 1 | |
| }, | |
| "value": { | |
| "type": "string", | |
| "minLength": 1 | |
| } | |
| }, | |
| "required": [ | |
| "key", | |
| "value" | |
| ], | |
| "additionalProperties": false | |
| } | |
| } | |
| }, | |
| "additionalProperties": true | |
| } | |
| }, | |
| "additionalProperties": true, | |
| "required": [ | |
| "enabled" |
…44934) Enforces schema validation for required fields as documented, adding `required` arrays and constraints to 39 schema files. ## Changes - **Required field enforcement**: Added `required` arrays to schemas when fields are marked as required in documentation - **String constraints**: Added `minLength: 1` for required string fields - **Integer constraints**: Added `minimum: 1` for required integer fields (or `minimum: 0` for UID/GID, iSCSI LUN) - **Scope limitation**: Only applied to nested instance schemas (e.g., `persistence/nfs.json`, `service/ports.json`). Top-level container schemas (e.g., `rbac.json`, `configmap.json`) remain unconstrained to support empty defaults. ## Example ```json // persistence/nfs.json - enforces required fields for NFS volumes { "properties": { "path": { "type": "string", "minLength": 1 }, "server": { "type": "string", "minLength": 1 } }, "required": ["path", "server"] } ``` ## Validation - Helm lint: ✅ - Unit tests: 1346/1346 passed <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
…44934) Enforces schema validation for required fields as documented, adding `required` arrays and constraints to 39 schema files. ## Changes - **Required field enforcement**: Added `required` arrays to schemas when fields are marked as required in documentation - **String constraints**: Added `minLength: 1` for required string fields - **Integer constraints**: Added `minimum: 1` for required integer fields (or `minimum: 0` for UID/GID, iSCSI LUN) - **Scope limitation**: Only applied to nested instance schemas (e.g., `persistence/nfs.json`, `service/ports.json`). Top-level container schemas (e.g., `rbac.json`, `configmap.json`) remain unconstrained to support empty defaults. ## Example ```json // persistence/nfs.json - enforces required fields for NFS volumes { "properties": { "path": { "type": "string", "minLength": 1 }, "server": { "type": "string", "minLength": 1 } }, "required": ["path", "server"] } ``` ## Validation - Helm lint: ✅ - Unit tests: 1346/1346 passed <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
|
This PR is locked to prevent necro-posting on closed PRs. Please create a issue or contact staff on discord if you want to further discuss this |
Enforces schema validation for required fields as documented, adding
requiredarrays and constraints to 39 schema files.Changes
requiredarrays to schemas when fields are marked as required in documentationminLength: 1for required string fieldsminimum: 1for required integer fields (orminimum: 0for UID/GID, iSCSI LUN)persistence/nfs.json,service/ports.json). Top-level container schemas (e.g.,rbac.json,configmap.json) remain unconstrained to support empty defaults.Example
Validation
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.