Skip to content

Commit 5f117d1

Browse files
Jaisheesh-2006Jaisheesh-2006
committed
fix(kptfile): validate and sanitize Kptfile updates
Reuse DecodeKptfile validation in UpdateKptfileContent so invalid, deprecated, or unknown-field Kptfiles fail before SDK processing. Strip SDK-internal metadata annotations before applying mutations to avoid writing internal config.kubernetes.io fields back to user Kptfiles. Add regression tests covering validation parity, annotation stripping, empty annotation cleanup, and nil-safe handling. Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
1 parent a4335c5 commit 5f117d1

2 files changed

Lines changed: 177 additions & 13 deletions

File tree

‎pkg/kptfile/kptfileutil/util.go‎

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -473,13 +473,7 @@ func DecodeKptfile(in io.Reader) (*kptfilev1.KptFile, error) {
473473
if err != nil {
474474
return kf, err
475475
}
476-
if err := checkKptfileVersion(c); err != nil {
477-
return kf, err
478-
}
479-
480-
d := yaml.NewDecoder(bytes.NewBuffer(c))
481-
d.KnownFields(true)
482-
if err := d.Decode(&kptfilev1.KptFile{}); err != nil {
476+
if err := validateKptfileContent(c); err != nil {
483477
return kf, err
484478
}
485479

@@ -497,19 +491,18 @@ func DecodeKptfile(in io.Reader) (*kptfilev1.KptFile, error) {
497491
return kf, err
498492
}
499493

500-
for _, key := range sdkInternalKptfileAnnotations {
501-
delete(kf.Annotations, key)
502-
}
503-
if len(kf.Annotations) == 0 {
504-
kf.Annotations = nil
505-
}
494+
stripSDKInternalKptfileAnnotations(kf)
506495

507496
return kf, nil
508497
}
509498

510499
// UpdateKptfileContent updates Kptfile YAML content in-memory using SDK Kptfile
511500
// read/write APIs while preserving existing YAML document structure and comments.
512501
func UpdateKptfileContent(content string, mutator func(*kptfilev1.KptFile)) (string, error) {
502+
if err := validateKptfileContent([]byte(content)); err != nil {
503+
return "", err
504+
}
505+
513506
resources := map[string]string{kptfilev1.KptFileName: content}
514507
sdkKptfile, err := fn.NewKptfileFromPackage(resources)
515508
if err != nil {
@@ -520,6 +513,7 @@ func UpdateKptfileContent(content string, mutator func(*kptfilev1.KptFile)) (str
520513
if err := sdkKptfile.Obj.As(typedKptfile); err != nil {
521514
return "", err
522515
}
516+
stripSDKInternalKptfileAnnotations(typedKptfile)
523517

524518
mutator(typedKptfile)
525519

@@ -534,6 +528,33 @@ func UpdateKptfileContent(content string, mutator func(*kptfilev1.KptFile)) (str
534528
return resources[kptfilev1.KptFileName], nil
535529
}
536530

531+
func validateKptfileContent(content []byte) error {
532+
if err := checkKptfileVersion(content); err != nil {
533+
return err
534+
}
535+
536+
d := yaml.NewDecoder(bytes.NewBuffer(content))
537+
d.KnownFields(true)
538+
if err := d.Decode(&kptfilev1.KptFile{}); err != nil {
539+
return err
540+
}
541+
542+
return nil
543+
}
544+
545+
func stripSDKInternalKptfileAnnotations(kf *kptfilev1.KptFile) {
546+
if kf == nil || kf.ObjectMeta.Annotations == nil {
547+
return
548+
}
549+
550+
for _, key := range sdkInternalKptfileAnnotations {
551+
delete(kf.ObjectMeta.Annotations, key)
552+
}
553+
if len(kf.ObjectMeta.Annotations) == 0 {
554+
kf.ObjectMeta.Annotations = nil
555+
}
556+
}
557+
537558
// checkKptfileVersion verifies the apiVersion and kind of the resource
538559
// within the Kptfile. If the legacy version is found, the DeprecatedKptfileError
539560
// is returned. If the currently supported apiVersion and kind is found, no

‎pkg/kptfile/kptfileutil/util_test.go‎

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -836,6 +836,149 @@ metadata: [bad
836836
assert.Error(t, err)
837837
}
838838

839+
func TestUpdateKptfileContent_UsesDecodeValidation(t *testing.T) {
840+
testCases := map[string]struct {
841+
content string
842+
expectedErr any
843+
expectedDecodeError string
844+
}{
845+
"deprecated version": {
846+
content: `
847+
apiVersion: kpt.dev/v1alpha2
848+
kind: Kptfile
849+
metadata:
850+
name: sample
851+
`,
852+
expectedErr: &DeprecatedKptfileError{},
853+
expectedDecodeError: "old resource version \"v1alpha2\" found in Kptfile",
854+
},
855+
"unknown kind": {
856+
content: `
857+
apiVersion: kpt.dev/v1
858+
kind: ConfigMap
859+
metadata:
860+
name: sample
861+
`,
862+
expectedErr: &UnknownKptfileResourceError{},
863+
expectedDecodeError: "unknown resource type \"kpt.dev/v1, Kind=ConfigMap\" found in Kptfile",
864+
},
865+
"unknown field": {
866+
content: `
867+
apiVersion: kpt.dev/v1
868+
kind: Kptfile
869+
metadata:
870+
name: sample
871+
unexpectedField: true
872+
`,
873+
expectedDecodeError: "yaml: unmarshal errors:\n line 6: field unexpectedField not found in type v1.KptFile",
874+
},
875+
}
876+
877+
for tn, tc := range testCases {
878+
t.Run(tn, func(t *testing.T) {
879+
_, decodeErr := DecodeKptfile(strings.NewReader(tc.content))
880+
_, updateErr := UpdateKptfileContent(tc.content, func(*kptfilev1.KptFile) {})
881+
882+
if !assert.EqualError(t, decodeErr, tc.expectedDecodeError) {
883+
t.FailNow()
884+
}
885+
if !assert.EqualError(t, updateErr, decodeErr.Error()) {
886+
t.FailNow()
887+
}
888+
if tc.expectedErr != nil {
889+
assert.IsType(t, tc.expectedErr, decodeErr)
890+
assert.IsType(t, tc.expectedErr, updateErr)
891+
}
892+
})
893+
}
894+
}
895+
896+
func TestUpdateKptfileContent_StripsSDKInternalAnnotations(t *testing.T) {
897+
t.Run("preserves user annotations", func(t *testing.T) {
898+
content := `
899+
apiVersion: kpt.dev/v1
900+
kind: Kptfile
901+
metadata:
902+
name: sample
903+
annotations:
904+
config.kubernetes.io/index: "0"
905+
internal.config.kubernetes.io/path: Kptfile
906+
user.example.com/keep: value
907+
`
908+
909+
updatedContent, err := UpdateKptfileContent(content, func(kf *kptfilev1.KptFile) {
910+
kf.Name = "updated-sample"
911+
})
912+
if !assert.NoError(t, err) {
913+
t.FailNow()
914+
}
915+
916+
updatedKf, err := DecodeKptfile(strings.NewReader(updatedContent))
917+
if !assert.NoError(t, err) {
918+
t.FailNow()
919+
}
920+
921+
assert.Equal(t, "updated-sample", updatedKf.Name)
922+
if assert.NotNil(t, updatedKf.Annotations) {
923+
assert.Equal(t, "value", updatedKf.Annotations["user.example.com/keep"])
924+
for _, key := range sdkInternalKptfileAnnotations {
925+
assert.NotContains(t, updatedKf.Annotations, key)
926+
}
927+
}
928+
assert.NotContains(t, updatedContent, "config.kubernetes.io/index")
929+
assert.NotContains(t, updatedContent, "internal.config.kubernetes.io/path")
930+
})
931+
932+
t.Run("removes empty annotation map", func(t *testing.T) {
933+
content := `
934+
apiVersion: kpt.dev/v1
935+
kind: Kptfile
936+
metadata:
937+
name: sample
938+
annotations:
939+
config.kubernetes.io/index: "0"
940+
internal.config.kubernetes.io/index: "0"
941+
`
942+
943+
updatedContent, err := UpdateKptfileContent(content, func(*kptfilev1.KptFile) {})
944+
if !assert.NoError(t, err) {
945+
t.FailNow()
946+
}
947+
948+
updatedKf, err := DecodeKptfile(strings.NewReader(updatedContent))
949+
if !assert.NoError(t, err) {
950+
t.FailNow()
951+
}
952+
953+
assert.Nil(t, updatedKf.Annotations)
954+
assert.NotContains(t, updatedContent, "annotations:")
955+
})
956+
957+
t.Run("handles missing annotations safely", func(t *testing.T) {
958+
content := `
959+
apiVersion: kpt.dev/v1
960+
kind: Kptfile
961+
metadata:
962+
name: sample
963+
`
964+
965+
updatedContent, err := UpdateKptfileContent(content, func(kf *kptfilev1.KptFile) {
966+
kf.Name = "updated-sample"
967+
})
968+
if !assert.NoError(t, err) {
969+
t.FailNow()
970+
}
971+
972+
updatedKf, err := DecodeKptfile(strings.NewReader(updatedContent))
973+
if !assert.NoError(t, err) {
974+
t.FailNow()
975+
}
976+
977+
assert.Equal(t, "updated-sample", updatedKf.Name)
978+
assert.Nil(t, updatedKf.Annotations)
979+
})
980+
}
981+
839982
func TestMerge(t *testing.T) {
840983
testCases := map[string]struct {
841984
origin string

0 commit comments

Comments
 (0)