fix(live): validate annotation values to prevent silent data loss #4371
Conversation
Previously, invalid annotation types (like unquoted booleans) were silently dropped during apply. This adds a validation step to reject non-string values with a clear error message. Fixes kptdev#3784 Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
Jaisheesh-2006
requested review from
efiacor,
kispaljr and
mozesl-nokia
as code owners
✅ Deploy Preview for kptdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR implements strict validation for metadata.annotations values during the live apply pipeline to prevent silent data loss. Previously, non-string annotation values (e.g., unquoted booleans or integers) were silently discarded by the Go unmarshaler, resulting in annotations being dropped without warning.
Changes:
- Adds annotation value type validation in the live apply pipeline that inspects raw YAML tags
- Introduces comprehensive unit tests for annotation validation logic
- Adds integration tests to existing test suites to ensure validation is enforced in both path and stream readers
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/live/rgpath.go | Implements validateAnnotationTypes and yamlTagToType functions, integrates validation into kyamlNodeToUnstructured |
| pkg/live/validate_annotation_test.go | Comprehensive unit tests covering valid strings, invalid types (boolean, integer, float, null), edge cases, and error messages |
| pkg/live/rgstream_test.go | Adds integration test case for bad annotation type rejection in stream reader |
| pkg/live/rgpath_test.go | Adds integration test case for bad annotation type rejection in path reader |
| pkg/live/helpers_test.go | Defines configMapWithBadAnnotation test fixture used by integration tests |
| e2e/testdata/live-apply/invalid-annotation-type/config.yaml | E2E test configuration for invalid annotation type scenario |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
Jaisheesh-2006
force-pushed
the
fix/3784-validate-annotations
branch
from
5d17292 to
a0b112d
Compare
- Rename validateAnnotationTypes to validateMetadataStringMaps - Add validateStringMap helper function for reusability - Validate both annotations AND labels for non-string types - Add proper GoDoc comments to all functions - Create comprehensive test file validate_metadata_test.go - Fix E2E test expected error message prefix Fixes kptdev#3784 Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
Add resourcegroup.yaml to migrate from Kptfile-based inventory to ResourceGroup-based inventory, eliminating the warning message about using Kptfile for inventory information. Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Remove the inventory section from Kptfile since ResourceGroup is now used for inventory. Having both causes an inventory conflict error. Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mozesl-nokia
left a comment
mozesl-nokia
left a comment
There was a problem hiding this comment.
There should probably be an E2E test added for the label validation as well.
Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
3 participants
Description
This PR implements strict validation for
metadata.annotationsvalues during thelive applypipeline.Previously, the Go unmarshaler silently discarded non-string values (e.g., unquoted booleans like
trueor integers like123) because they did not match the expectedmap[string]stringschema. This resulted in annotations being dropped from the applied resource without warning.This change adds a validation step that inspects the raw YAML tags of
RNodes. It now returns a strict error if any annotation value is not a string, while correctly accepting quoted strings (e.g.,"true").Motivation
To prevent silent data loss and configuration drift.
Users frequently mistake YAML types (e.g., writing
enabled: trueinstead ofenabled: "true"). Currently,kptignores these fields silently. This fix ensures the user receives immediate feedback:New Error Output:
Error: annotation "example.com/enabled" must be a string, got booleanWhich issue does this PR fix?
Fixes #3784