Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Jaisheesh-2006 wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b6a9ace
fix(live): validate annotation values to prevent silent data loss
Jaisheesh-2006 Feb 2, 2026
a0b112d
fix(e2e): add missing test resources for invalid-annotation-type test
Jaisheesh-2006 Feb 2, 2026
3798dcb
fix: add validation for annotations and labels with non-string types
Jaisheesh-2006 Feb 2, 2026
f9e9cf1
fix: add resourcegroup.yaml for invalid-annotation-type test
Jaisheesh-2006 Feb 2, 2026
fd2e2da
fix: remove inventory section from Kptfile to avoid conflict
Jaisheesh-2006 Feb 2, 2026
86a8a35
fix: add label validation E2E test and use kyaml constants
Jaisheesh-2006 Feb 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions e2e/testdata/live-apply/invalid-annotation-type/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Copyright 2026 The kpt Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

parallel: true

kptArgs:
- "live"
- "apply"

# Expected exit code is non-zero (failure)
exitCode: 1

# Error should be in stderr with clear message about which field failed
stdErr: |
error: annotation "example.com/enabled" must be a string, got boolean
4 changes: 4 additions & 0 deletions e2e/testdata/live-apply/invalid-annotation-type/resources/Kptfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
apiVersion: kpt.dev/v1
kind: Kptfile
metadata:
name: invalid-annotation-type
8 changes: 8 additions & 0 deletions e2e/testdata/live-apply/invalid-annotation-type/resources/configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
apiVersion: v1
kind: ConfigMap
metadata:
name: bad-config
annotations:
example.com/enabled: true
data:
key: value
7 changes: 7 additions & 0 deletions e2e/testdata/live-apply/invalid-annotation-type/resources/resourcegroup.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
apiVersion: kpt.dev/v1alpha1
kind: ResourceGroup
metadata:
name: inventory-obj
namespace: invalid-annotation-type
labels:
cli-utils.sigs.k8s.io/inventory-id: test-invalid-annotation
26 changes: 26 additions & 0 deletions e2e/testdata/live-apply/invalid-label-type/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Copyright 2026 The kpt Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

parallel: true

kptArgs:
- "live"
- "apply"

# Expected exit code is non-zero (failure)
exitCode: 1

# Error should be in stderr with clear message about which field failed
stdErr: |
error: label "app.kubernetes.io/enabled" must be a string, got boolean
4 changes: 4 additions & 0 deletions e2e/testdata/live-apply/invalid-label-type/resources/Kptfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
apiVersion: kpt.dev/v1
kind: Kptfile
metadata:
name: invalid-label-type
8 changes: 8 additions & 0 deletions e2e/testdata/live-apply/invalid-label-type/resources/configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
apiVersion: v1
kind: ConfigMap
metadata:
name: bad-label-config
labels:
app.kubernetes.io/enabled: true
data:
key: value
7 changes: 7 additions & 0 deletions e2e/testdata/live-apply/invalid-label-type/resources/resourcegroup.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
apiVersion: kpt.dev/v1alpha1
kind: ResourceGroup
metadata:
name: inventory-obj
namespace: invalid-label-type
labels:
cli-utils.sigs.k8s.io/inventory-id: test-invalid-label
18 changes: 18 additions & 0 deletions pkg/live/helpers_test.go
View file
Open in desktop
Copy link

@mozesl-nokia mozesl-nokia Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a test for labels here too?

Copy link
Contributor Author

@Jaisheesh-2006 Jaisheesh-2006 Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,24 @@ kind: ConfigMap
metadata:
name: cm
data: {}
`
configMapWithBadAnnotation = `
apiVersion: v1
kind: ConfigMap
metadata:
name: cm
annotations:
example.com/bad-anno: true
data: {}
`
configMapWithBadLabel = `
apiVersion: v1
kind: ConfigMap
metadata:
name: cm
labels:
app.kubernetes.io/enabled: true
data: {}
`
crd = `
apiVersion: apiextensions.k8s.io/v1
Expand Down
107 changes: 107 additions & 0 deletions pkg/live/rgpath.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package live

import (
"encoding/json"
"fmt"

"github.com/kptdev/kpt/internal/util/pathutil"
rgfilev1alpha1 "github.com/kptdev/kpt/pkg/api/resourcegroup/v1alpha1"
Expand Down Expand Up @@ -93,6 +94,9 @@ func removeAnnotations(n *yaml.RNode, annotations ...kioutil.AnnotationKey) erro
//
//nolint:interfacer
func kyamlNodeToUnstructured(n *yaml.RNode) (*unstructured.Unstructured, error) {
if err := validateMetadataStringMaps(n); err != nil {
return nil, err
}
b, err := n.MarshalJSON()
if err != nil {
return nil, err
Expand All @@ -109,6 +113,109 @@ func kyamlNodeToUnstructured(n *yaml.RNode) (*unstructured.Unstructured, error)
}, nil
}

Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
// validateMetadataStringMaps inspects the raw YAML nodes for metadata.annotations
// and metadata.labels to ensure all values are strings. This prevents silent data
// loss that occurs when non-string values (booleans, integers, floats) are
// unmarshaled into map[string]string fields.
//
// Parameters:
// - n: The RNode representing a Kubernetes resource
//
// Returns:
// - error: A descriptive error if any annotation or label value is not a string,
// nil otherwise
func validateMetadataStringMaps(n *yaml.RNode) error {
metadata := n.Field("metadata")
if metadata == nil || metadata.Value == nil {
return nil
}

// Validate annotations
if err := validateStringMap(metadata.Value, "annotations", "annotation"); err != nil {
return err
}

// Validate labels
if err := validateStringMap(metadata.Value, "labels", "label"); err != nil {
return err
}

return nil
}

// validateStringMap checks that all values in a metadata field (annotations or labels)
// are strings. It inspects the YAML tag of each value to detect non-string types
// that would be silently dropped during unmarshaling.
//
// Parameters:
// - metadata: The RNode for the metadata field
// - fieldName: The name of the field to validate ("annotations" or "labels")
// - fieldType: Human-readable name for error messages ("annotation" or "label")
//
// Returns:
// - error: A descriptive error if any value is not a string, nil otherwise
func validateStringMap(metadata *yaml.RNode, fieldName, fieldType string) error {
field := metadata.Field(fieldName)
if field == nil || field.Value == nil {
return nil
}

mapNode := field.Value.YNode()
if mapNode == nil {
return nil
}

// Handle explicit null (e.g., annotations: null)
if mapNode.Tag == yaml.NodeTagNull {
return nil
}

if mapNode.Kind != yaml.MappingNode {
return fmt.Errorf("metadata.%s must be a string map", fieldName)
}

for i := 0; i < len(mapNode.Content); i += 2 {
keyNode := mapNode.Content[i]
valueNode := mapNode.Content[i+1]
if valueNode.Kind != yaml.ScalarNode || valueNode.Tag != yaml.NodeTagString {
return fmt.Errorf("%s %q must be a string, got %s", fieldType, keyNode.Value, yamlTagToType(valueNode))
Copy link

@mozesl-nokia mozesl-nokia Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@Jaisheesh-2006 Jaisheesh-2006 Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

return nil
}

Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
// yamlTagToType converts a YAML node's tag to a human-readable type name
// for use in error messages.
//
// Parameters:
// - node: The YAML node to inspect
//
// Returns:
// - string: A human-readable type name (e.g., "boolean", "integer", "number")
func yamlTagToType(node *yaml.Node) string {
if node.Kind != yaml.ScalarNode {
return "non-scalar"
}
switch node.Tag {
case yaml.NodeTagBool:
return "boolean"
case yaml.NodeTagInt:
return "integer"
case yaml.NodeTagFloat:
return "number"
case yaml.NodeTagNull:
return "null"
case yaml.NodeTagString:
return "string"
default:
if node.Tag == yaml.NodeTagEmpty {
return "unknown"
}
return node.Tag
}
}

const NoLocalConfigAnnoVal = "false"

// filterLocalConfig returns a new slice of Unstructured where all resources
Expand Down
14 changes: 14 additions & 0 deletions pkg/live/rgpath_test.go
View file
Open in desktop
Copy link

@mozesl-nokia mozesl-nokia Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a test for labels here too?

Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,20 @@ func TestPathManifestReader_Read(t *testing.T) {
},
},
},
"Bad annotation type is rejected": {
manifests: map[string]string{
"bad-anno.yaml": configMapWithBadAnnotation,
},
namespace: "test-namespace",
expectedErrMsg: "annotation \"example.com/bad-anno\" must be a string, got boolean",
},
"Bad label type is rejected": {
manifests: map[string]string{
"bad-label.yaml": configMapWithBadLabel,
},
namespace: "test-namespace",
expectedErrMsg: "label \"app.kubernetes.io/enabled\" must be a string, got boolean",
},
}

for tn, tc := range testCases {
Expand Down
14 changes: 14 additions & 0 deletions pkg/live/rgstream_test.go
View file
Open in desktop
Copy link

@mozesl-nokia mozesl-nokia Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a test for labels here too?

Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,20 @@ func TestResourceStreamManifestReader_Read(t *testing.T) {
namespace: "test-namespace",
expectedErrMsg: "unknown resource types: custom.io/v1/Custom",
},
"Bad annotation type is rejected": {
manifests: map[string]string{
"bad-anno.yaml": configMapWithBadAnnotation,
},
namespace: "test-namespace",
expectedErrMsg: "annotation \"example.com/bad-anno\" must be a string, got boolean",
},
"Bad label type is rejected": {
manifests: map[string]string{
"bad-label.yaml": configMapWithBadLabel,
},
namespace: "test-namespace",
expectedErrMsg: "label \"app.kubernetes.io/enabled\" must be a string, got boolean",
},
}

for tn, tc := range testCases {
Expand Down
Loading
Loading