chore(config): validate additionalProperties on all schema objects (AROSLSRE-233)#5360
Open
raelga wants to merge 4 commits into
Open
chore(config): validate additionalProperties on all schema objects (AROSLSRE-233)#5360raelga wants to merge 4 commits into
raelga wants to merge 4 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: raelga The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
raelga
changed the title
raelga
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens config validation by explicitly disallowing unknown fields in several JSON Schema object definitions and adds a CI-time verification step to prevent missing additionalProperties from regressing.
Changes:
- Add
additionalProperties: falseto five previously-permissive schema definitions inconfig/config.schema.json. - Introduce a new Go-based verifier (
hack/verify-schema-additional-properties/) and wire it intomake verifyvia averify-schematarget. - Add the new verifier module to the workspace (
go.work) sogo runworks from the repo root.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds verify-schema and runs it as part of make verify. |
| hack/verify-schema-additional-properties/main.go | New verifier that checks for presence of additionalProperties on root + top-level definitions. |
| hack/verify-schema-additional-properties/go.mod | Declares the verifier as a standalone Go module in the workspace. |
| go.work | Includes the new verifier module so workspace builds/runs succeed. |
| config/config.schema.json | Adds additionalProperties: false to the five missing object definitions. |
raelga
force-pushed
the
raelga/schema-additional-properties
branch
from
874d68f to
fee8f2b
Compare
raelga
force-pushed
the
raelga/schema-additional-properties
branch
from
fee8f2b to
90b73b3
Compare
…SRE-233) Add additionalProperties: false to the 5 object definitions that were missing it: certificateRef, entraApplication, hcpBackups, k8sDeploymentStrategy, k8sRollingUpdateDeploymentStrategy. Add verify-schema target to CI that checks all object definitions in config.schema.json have additionalProperties set, preventing regressions when new definitions are added.
…rties The verifier only checked root and top-level definitions but not nested objects under properties, patternProperties, or items. Rewrite to walk the full schema tree recursively. Fix the 13 nested objects found by the recursive check.
Handle nodes that define properties/patternProperties without explicit type: object, support array form of type, and recurse into not subtrees.
raelga
force-pushed
the
raelga/schema-additional-properties
branch
from
90b73b3 to
4f61721
Compare
Comment on lines
+15
to
+19
| // verify-schema-additional-properties checks that all object definitions in a | ||
| // JSON schema file have an "additionalProperties" field set. Without it, the | ||
| // schema silently allows unexpected properties, defeating typo detection. | ||
| // | ||
| // Exit code is 1 if any object definitions are missing the field. |
Comment on lines
+136
to
+142
| if len(missing) > 0 { | ||
| fmt.Fprintf(os.Stderr, "ERROR: %d object definition(s) missing additionalProperties in %s:\n", len(missing), path) | ||
| for _, m := range missing { | ||
| fmt.Fprintf(os.Stderr, " - %s\n", m) | ||
| } | ||
| fmt.Fprintf(os.Stderr, "\nAdd \"additionalProperties\": false (or true) to each definition.\n") | ||
| exitCode = 1 |
Comment on lines
+2534
to
+2560
| "imageRegistryPolicy": { | ||
| "type": "object", | ||
| "properties": { | ||
| "extraAllowedRegistries": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "validationActions": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "Deny", | ||
| "Audit", | ||
| "Warn" | ||
| ] | ||
| } | ||
| } | ||
| }, | ||
| "additionalProperties": false, | ||
| "required": [ | ||
| "extraAllowedRegistries", | ||
| "validationActions" | ||
| ] | ||
| }, |
Comment on lines
+2534
to
+2538
| "imageRegistryPolicy": { | ||
| "type": "object", | ||
| "properties": { | ||
| "extraAllowedRegistries": { | ||
| "type": "array", |
… wording Remove the imageRegistryPolicy schema definition that was reintroduced by rebase drift from the reverted FSI VAP PR (#4690). The definition is not referenced by any config property. Update verifier comments and error messages from "object definitions" to "object schemas" to reflect recursive validation scope.
| } | ||
|
|
||
| func (n schemaNode) isObject() bool { | ||
| if len(n.Properties) > 0 || len(n.PatternProperties) > 0 || n.AdditionalProperties != nil { |
Comment on lines
+78
to
+91
| for _, child := range node.PatternProperties { | ||
| walkSchema(child, joinPath(path, "(patternProperty)"), missing) | ||
| } | ||
| if node.Items != nil { | ||
| walkSchema(*node.Items, joinPath(path, "(items)"), missing) | ||
| } | ||
| for _, child := range node.AllOf { | ||
| walkSchema(child, path, missing) | ||
| } | ||
| for _, child := range node.OneOf { | ||
| walkSchema(child, path, missing) | ||
| } | ||
| for _, child := range node.AnyOf { | ||
| walkSchema(child, path, missing) |
Comment on lines
1069
to
1081
| "properties": { | ||
| "version": { | ||
| "type": "string" | ||
| }, | ||
| "bundle": { | ||
| "$ref": "#/definitions/containerImage" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "bundle" | ||
| ] | ||
| ], | ||
| "additionalProperties": false | ||
| }, |
|
@raelga: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AROSLSRE-233
What
Add
additionalProperties: falseto the 5 object definitions that were missing it and a CI validation check to prevent regressions.Why
JSON Schema defaults to allowing additional properties. Without explicit
additionalProperties: false, typos in config.yaml (e.g.stoargeAccountinstead ofstorageAccount) are silently accepted.Testing
make verifypasses (includes newverify-schematarget)make lintpasseshack/verify-schema-additional-properties/checks all definitionsDefinitions fixed
certificateRefentraApplicationhcpBackupsk8sDeploymentStrategyk8sRollingUpdateDeploymentStrategy