fix(live): validate annotation values to prevent silent data loss #4371

Jaisheesh-2006 · 2026-02-02T17:31:51Z

Description

This PR implements strict validation for metadata.annotations values during the live apply pipeline.

Previously, the Go unmarshaler silently discarded non-string values (e.g., unquoted booleans like true or integers like 123) because they did not match the expected map[string]string schema. 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: true instead of enabled: "true"). Currently, kpt ignores these fields silently. This fix ensures the user receives immediate feedback:

New Error Output:
Error: annotation "example.com/enabled" must be a string, got boolean

Which issue does this PR fix?

Fixes #3784

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 <[email protected]>

netlify · 2026-02-02T17:31:58Z

✅ Deploy Preview for kptdocs ready!

Name	Link
🔨 Latest commit	`86a8a35`
🔍 Latest deploy log	https://app.netlify.com/projects/kptdocs/deploys/6982cb5d9af57a000853dbed
😎 Deploy Preview	https://deploy-preview-4371--kptdocs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot

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.

Copilot · 2026-02-02T17:40:16Z

pkg/live/rgpath.go

@@ -109,6 +113,60 @@ func kyamlNodeToUnstructured(n *yaml.RNode) (*unstructured.Unstructured, error)
 	}, nil
 }



This function is missing a documentation comment. According to Go conventions and best practices visible throughout the codebase (e.g., kyamlNodeToUnstructured on line 92, removeAnnotations on line 81), exported and internal functions should have documentation comments that explain their purpose, parameters, and behavior. Add a comment explaining what this function does, what errors it returns, and when those errors occur.

Suggested change

// validateAnnotationTypes verifies that the metadata.annotations field on the

// provided RNode, if present and non-null, is a mapping node whose values are

// all string scalars. It returns nil when metadata or annotations are missing

// or explicitly null. It returns an error if metadata.annotations is not a

// mapping node or if any annotation value is not a string.

Copilot · 2026-02-02T17:40:16Z

pkg/live/rgpath.go

+
+	return nil
+}
+


This helper function is missing a documentation comment. According to Go conventions and best practices visible throughout the codebase, functions should have documentation comments that explain their purpose. Add a comment explaining that this function converts YAML type tags to human-readable type names for error messages.

Suggested change

// yamlTagToType converts YAML type tags to human-readable type names for error messages.

Copilot · 2026-02-02T17:40:17Z

pkg/live/rgpath.go

+func validateAnnotationTypes(n *yaml.RNode) error {
+	metadata := n.Field("metadata")
+	if metadata == nil || metadata.Value == nil {
+		return nil
+	}
+
+	annotations := metadata.Value.Field("annotations")
+	if annotations == nil || annotations.Value == nil {
+		return nil
+	}
+
+	annotationNode := annotations.Value.YNode()
+	// Handle explicit null annotations (annotations: null)
+	if annotationNode.Tag == "!!null" {
+		return nil
+	}
+	if annotationNode.Kind != yaml.MappingNode {
+		return fmt.Errorf("metadata.annotations must be a string map")
+	}
+
+	for i := 0; i < len(annotationNode.Content); i += 2 {
+		keyNode := annotationNode.Content[i]
+		valueNode := annotationNode.Content[i+1]
+		if valueNode.Kind != yaml.ScalarNode || valueNode.Tag != "!!str" {
+			return fmt.Errorf("annotation %q must be a string, got %s", keyNode.Value, yamlTagToType(valueNode))
+		}
+	}
+
+	return nil
+}


The validation logic only checks annotations but not labels. According to Kubernetes specifications, metadata.labels also must be a map of strings to strings, and they suffer from the same silent data loss issue when non-string values are used. Consider extending the validation to also check labels to prevent the same problem. You could either add label validation to this function or create a separate validateLabelTypes function and call it from kyamlNodeToUnstructured.

Copilot · 2026-02-02T17:40:17Z

e2e/testdata/live-apply/invalid-annotation-type/config.yaml

+
+# Error should be in stderr with clear message about which field failed
+stdErr: |
+  annotation "example.com/enabled" must be a string, got boolean


This e2e test configuration is incomplete. The test expects a "resources" subdirectory containing the actual resource manifests to test, but this directory is missing from the PR. Based on the test runner implementation (pkg/test/live/runner.go line 79), the kpt command will be executed in the "resources" subdirectory of this test case. Without the resources directory containing a manifest with "example.com/enabled: true" annotation, this test will fail to run correctly. You should add a resources directory with a Kptfile and a resource manifest that has the problematic annotation (e.g., a ConfigMap with "example.com/enabled: true").

Suggested change

annotation "example.com/enabled" must be a string, got boolean

annotation "example.com/enabled" must be a string, got boolean

# NOTE: The resources that trigger this error are expected to be in the

# "resources" subdirectory of this test case, including a Kptfile and a

# manifest (e.g., a ConfigMap) with annotation "example.com/enabled: true".

Copilot · 2026-02-02T17:40:17Z

pkg/live/rgpath.go

+		return nil
+	}
+
+	annotationNode := annotations.Value.YNode()


There's a potential nil pointer dereference here. The YNode() method could return nil, but the code immediately accesses annotationNode.Tag on line 129 without checking if annotationNode is nil. While the check on line 123 ensures annotations.Value is not nil, you should add a nil check for annotationNode before accessing its fields to prevent potential panics.

Suggested change

annotationNode := annotations.Value.YNode()

annotationNode := annotations.Value.YNode()

if annotationNode == nil {

return nil

}

Signed-off-by: Jaisheesh-2006 <[email protected]>

- 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 <[email protected]>

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 <[email protected]>

Copilot

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 <[email protected]>

Copilot

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

There should probably be an E2E test added for the label validation as well.

mozesl-nokia · 2026-02-03T12:21:46Z

pkg/live/rgpath.go

+		keyNode := mapNode.Content[i]
+		valueNode := mapNode.Content[i+1]
+		if valueNode.Kind != yaml.ScalarNode || valueNode.Tag != "!!str" {
+			return fmt.Errorf("%s %q must be a string, got %s", fieldType, keyNode.Value, yamlTagToType(valueNode))


I think we don't need to convert the type to more human readable, but curious about the others' opinion.

Maybe separate the checks to kind != scalar and tag != !!str.

Also, is "!!str" not available as a const from somewhere?

Hi @mozesl-nokia , indeed !!str is available as a constant in yaml.NodeTagString, I will shortly replace the other ones as well and push it

mozesl-nokia · 2026-02-03T12:23:21Z

pkg/live/helpers_test.go

Should there be a test for labels here too?

Hi @mozesl-nokia , i will add the test for labels and commit it soon

mozesl-nokia · 2026-02-03T12:23:27Z

pkg/live/rgpath_test.go

Should there be a test for labels here too?

mozesl-nokia · 2026-02-03T12:23:32Z

pkg/live/rgstream_test.go

Should there be a test for labels here too?

Signed-off-by: Jaisheesh-2006 <[email protected]>

Jaisheesh-2006 requested a review from liamfallon as a code owner February 2, 2026 17:31

Copilot AI review requested due to automatic review settings February 2, 2026 17:31

Jaisheesh-2006 requested review from efiacor, kispaljr and mozesl-nokia as code owners February 2, 2026 17:31

Copilot started reviewing on behalf of Jaisheesh-2006 February 2, 2026 17:32 View session

Copilot AI reviewed Feb 2, 2026

View reviewed changes

fix(e2e): add missing test resources for invalid-annotation-type test

a0b112d

Signed-off-by: Jaisheesh-2006 <[email protected]>

Jaisheesh-2006 force-pushed the fix/3784-validate-annotations branch from 5d17292 to a0b112d Compare February 2, 2026 17:54

Copilot AI review requested due to automatic review settings February 2, 2026 18:05

Copilot started reviewing on behalf of Jaisheesh-2006 February 2, 2026 18:05 View session

fix: add resourcegroup.yaml for invalid-annotation-type test

f9e9cf1

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 <[email protected]>

Copilot AI reviewed Feb 2, 2026

View reviewed changes

fix: remove inventory section from Kptfile to avoid conflict

fd2e2da

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 <[email protected]>

Copilot AI review requested due to automatic review settings February 2, 2026 18:19

Copilot started reviewing on behalf of Jaisheesh-2006 February 2, 2026 18:19 View session

Copilot AI reviewed Feb 2, 2026

View reviewed changes

mozesl-nokia reviewed Feb 3, 2026

View reviewed changes

fix: add label validation E2E test and use kyaml constants

86a8a35

Signed-off-by: Jaisheesh-2006 <[email protected]>

		@@ -109,6 +113,60 @@ func kyamlNodeToUnstructured(n yaml.RNode) (unstructured.Unstructured, error)
		}, nil
		}

+// validateAnnotationTypes verifies that the metadata.annotations field on the
+// provided RNode, if present and non-null, is a mapping node whose values are
+// all string scalars. It returns nil when metadata or annotations are missing
+// or explicitly null. It returns an error if metadata.annotations is not a
+// mapping node or if any annotation value is not a string.



	// yamlTagToType converts YAML type tags to human-readable type names for error messages.

-  annotation "example.com/enabled" must be a string, got boolean
+  annotation "example.com/enabled" must be a string, got boolean
+# NOTE: The resources that trigger this error are expected to be in the
+# "resources" subdirectory of this test case, including a Kptfile and a
+# manifest (e.g., a ConfigMap) with annotation "example.com/enabled: true".

fix(live): validate annotation values to prevent silent data loss #4371

Are you sure you want to change the base?

fix(live): validate annotation values to prevent silent data loss #4371

Uh oh!

Conversation

Jaisheesh-2006 commented Feb 2, 2026

Description

Motivation

Which issue does this PR fix?

Uh oh!

netlify bot commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kptdocs ready!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mozesl-nokia left a comment

Choose a reason for hiding this comment

Uh oh!

mozesl-nokia Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Jaisheesh-2006 Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

mozesl-nokia Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Jaisheesh-2006 Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

mozesl-nokia Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

mozesl-nokia Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

netlify bot commented Feb 2, 2026 •

edited

Loading