Always validate YAML

go.mod

@@ -40,7 +40,7 @@ require (
 	github.com/spf13/viper v1.16.0
 	github.com/stretchr/testify v1.10.0
 	github.com/twpayne/go-vfs v1.7.2
-	gopkg.in/yaml.v1 v1.0.0-20140924161607-9f9df34309c0
+	gopkg.in/yaml.v1 v1.0.0-20140924161607-9f9df34309c0 // indirect
 	k8s.io/api v0.34.5
 	k8s.io/apimachinery v0.34.5
 	k8s.io/client-go v12.0.0+incompatible
@@ -54,8 +54,8 @@ require (
 	github.com/melbahja/goph v1.4.0
 	github.com/prometheus/procfs v0.15.1
 	github.com/urfave/cli/v2 v2.3.0
+	go.yaml.in/yaml/v4 v4.0.0-rc.4
 	golang.org/x/sys v0.31.0
-	gopkg.in/yaml.v3 v3.0.1
 )
 
 require (
@@ -127,6 +127,7 @@ require (
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/ini.v1 v1.67.0 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
+	gopkg.in/yaml.v3 v3.0.1 // indirect
 	k8s.io/apiextensions-apiserver v0.33.1 // indirect
 	k8s.io/code-generator v0.34.5 // indirect
 	k8s.io/gengo v0.0.0-20250130153323-76c5745d3511 // indirect

go.sum

@@ -342,6 +342,8 @@ go.yaml.in/yaml/v2 v2.4.2 h1:DzmwEr2rDGHl7lsFgAHxmNz/1NlQ7xLIrlN2h5d1eGI=
 go.yaml.in/yaml/v2 v2.4.2/go.mod h1:081UH+NErpNdqlCXm3TtEran0rJZGxAYx9hb/ELlsPU=
 go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc=
 go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg=
+go.yaml.in/yaml/v4 v4.0.0-rc.4 h1:UP4+v6fFrBIb1l934bDl//mmnoIZEDK0idg1+AIvX5U=
+go.yaml.in/yaml/v4 v4.0.0-rc.4/go.mod h1:aZqd9kCMsGL7AuUv/m/PvWLdg5sjJsZ4oHDEnfPPfY0=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20190510104115-cbcb75029529/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
 golang.org/x/crypto v0.0.0-20190605123033-f99c8df09eb5/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=

pkg/admitter/cloudinit.go

@@ -7,13 +7,13 @@ import (
 	"path/filepath"
 
 	"github.com/harvester/webhook/pkg/server/admission"
-	"gopkg.in/yaml.v3"
+	"go.yaml.in/yaml/v4"
 	admissionregv1 "k8s.io/api/admissionregistration/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/rest"
 
-	v1beta1 "github.com/harvester/node-manager/pkg/apis/node.harvesterhci.io/v1beta1"
+	"github.com/harvester/node-manager/pkg/apis/node.harvesterhci.io/v1beta1"
 	clientset "github.com/harvester/node-manager/pkg/generated/clientset/versioned"
 	cloudinitv1beta1 "github.com/harvester/node-manager/pkg/generated/clientset/versioned/typed/node.harvesterhci.io/v1beta1"
 )
@@ -58,14 +58,8 @@ func (v *CloudInit) Create(_ *admission.Request, newObj runtime.Object) error {
 	return v.validate(newCloudInit)
 }
 
-func (v *CloudInit) Update(_ *admission.Request, oldObj runtime.Object, newObj runtime.Object) error {
-	oldCloudInit := oldObj.(*v1beta1.CloudInit)
+func (v *CloudInit) Update(_ *admission.Request, _ runtime.Object, newObj runtime.Object) error {
 	newCloudInit := newObj.(*v1beta1.CloudInit)
-
-	if oldCloudInit.Spec.Filename == newCloudInit.Spec.Filename {
-		return nil
-	}
-
 	return v.validate(newCloudInit)
 }
 
@@ -78,7 +72,7 @@ func (v *CloudInit) validate(cloudinit *v1beta1.CloudInit) error {
 		return errProtectedFilename
 	}
 
-	taken, err := v.isFilenameTaken(cloudinit.Spec.Filename)
+	taken, err := v.isFilenameTaken(cloudinit)
 	if err != nil {
 		return fmt.Errorf("check for duplicate filename: %w", err)
 	}
@@ -87,13 +81,7 @@ func (v *CloudInit) validate(cloudinit *v1beta1.CloudInit) error {
 		return errFilenameTaken
 	}
 
-	var obj map[string]any
-	var typeErr *yaml.TypeError
-	err = yaml.Unmarshal([]byte(cloudinit.Spec.Contents), &obj)
-	if errors.As(err, &typeErr) {
-		return errNotYAML
-	}
-	if err != nil {
+	if err := isYaml(cloudinit.Spec.Contents); err != nil {
 		return err
 	}
 
@@ -105,14 +93,17 @@ func (v *CloudInit) missingExtension(name string) bool {
 	return ext != ".yaml" && ext != ".yml"
 }
 
-func (v *CloudInit) isFilenameTaken(name string) (bool, error) {
+func (v *CloudInit) isFilenameTaken(cloudinit *v1beta1.CloudInit) (bool, error) {
 	cloudinits, err := v.cloudinits.List(context.TODO(), metav1.ListOptions{})
 	if err != nil {
 		return true, err
 	}
 
-	for _, cloudinit := range cloudinits.Items {
-		if cloudinit.Spec.Filename == name {
+	for _, existing := range cloudinits.Items {
+		if existing.Name == cloudinit.Name {
+			continue
+		}
+		if existing.Spec.Filename == cloudinit.Spec.Filename {
 			return true, nil
 		}
 	}
@@ -129,6 +120,21 @@ func (v *CloudInit) isProtectedFilename(name string) bool {
 	return false
 }
 
+// isYaml checks whether content is empty or a valid YAML mapping document,
+// which is the only structure cloud-init accepts.
+func isYaml(contents string) error {
+	var obj map[string]any
+	var typeErr *yaml.LoadErrors
+	err := yaml.Unmarshal([]byte(contents), &obj)
+	if errors.As(err, &typeErr) {
+		return errNotYAML
+	}
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
 func (v *CloudInit) Resource() admission.Resource {
 	return admission.Resource{
 		Names:      []string{v1beta1.CloudInitResourceName},

pkg/admitter/cloudinit_test.go

@@ -6,12 +6,14 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/watch"
 
-	v1beta1 "github.com/harvester/node-manager/pkg/apis/node.harvesterhci.io/v1beta1"
 	"github.com/harvester/webhook/pkg/server/admission"
+
+	"github.com/harvester/node-manager/pkg/apis/node.harvesterhci.io/v1beta1"
 )
 
 func TestProtectedFilenames(t *testing.T) {
@@ -55,7 +57,8 @@ func TestCreate(t *testing.T) {
 		{"filename collision", v1beta1.CloudInitSpec{Filename: "99_ssh.yaml"}, errFilenameTaken},
 		{"conflicts with protected file", v1beta1.CloudInitSpec{Filename: "helloworld.yaml"}, errProtectedFilename},
 		{"not yaml or yml file ext", v1beta1.CloudInitSpec{Filename: "a"}, errMissingExt},
-		{"not yaml contents", v1beta1.CloudInitSpec{Filename: "not.yaml", Contents: "hello, there"}, errNotYAML},
+		{"scalar root yaml contents", v1beta1.CloudInitSpec{Filename: "not.yaml", Contents: "hello, there"}, errNotYAML},
+		{"sequence root yaml contents", v1beta1.CloudInitSpec{Filename: "not.yaml", Contents: "- a\n- b"}, errNotYAML},
 	}
 
 	for _, tt := range tests {
@@ -96,7 +99,8 @@ func TestUpdate(t *testing.T) {
 		{"filename collision", v1beta1.CloudInitSpec{Filename: "99_ssh.yaml"}, errFilenameTaken},
 		{"conflicts with protected file", v1beta1.CloudInitSpec{Filename: "helloworld.yaml"}, errProtectedFilename},
 		{"not yaml or yml file ext", v1beta1.CloudInitSpec{Filename: "a"}, errMissingExt},
-		{"not yaml contents", v1beta1.CloudInitSpec{Filename: "not.yaml", Contents: "hello, there"}, errNotYAML},
+		{"scalar root yaml contents", v1beta1.CloudInitSpec{Filename: "not.yaml", Contents: "hello, there"}, errNotYAML},
+		{"sequence root yaml contents", v1beta1.CloudInitSpec{Filename: "not.yaml", Contents: "- a\n- b"}, errNotYAML},
 	}
 
 	for _, tt := range tests {
@@ -121,6 +125,128 @@ func TestUpdate(t *testing.T) {
 	}
 }
 
+func TestUpdateRegressionCases(t *testing.T) {
+	tests := []struct {
+		name      string
+		existing  []v1beta1.CloudInit
+		oldSpec   v1beta1.CloudInitSpec
+		newSpec   v1beta1.CloudInitSpec
+		wantError error
+	}{
+		{
+			name:      "unchanged filename still validates contents",
+			oldSpec:   v1beta1.CloudInitSpec{Filename: "same.yaml", Contents: "a: b"},
+			newSpec:   v1beta1.CloudInitSpec{Filename: "same.yaml", Contents: "hello, there"},
+			wantError: errNotYAML,
+		},
+		{
+			name: "ignore current object for duplicate filename",
+			existing: []v1beta1.CloudInit{{
+				ObjectMeta: v1.ObjectMeta{Name: "test-cloudinit"},
+				Spec:       v1beta1.CloudInitSpec{Filename: "same.yaml"},
+			}},
+			oldSpec: v1beta1.CloudInitSpec{Filename: "same.yaml", Contents: "k: v"},
+			newSpec: v1beta1.CloudInitSpec{Filename: "same.yaml", Contents: "k: new"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctl := &CloudInit{cloudinits: &mockClient{list: tt.existing}}
+			old := &v1beta1.CloudInit{
+				ObjectMeta: v1.ObjectMeta{Name: "test-cloudinit"},
+				Spec:       tt.oldSpec,
+			}
+			newObj := &v1beta1.CloudInit{
+				ObjectMeta: v1.ObjectMeta{Name: "test-cloudinit"},
+				Spec:       tt.newSpec,
+			}
+			got := ctl.Update(new(admission.Request), old, newObj)
+			if tt.wantError != nil {
+				assert.ErrorIs(t, got, tt.wantError)
+			} else {
+				assert.NoError(t, got)
+			}
+		})
+	}
+}
+
+func TestIsYaml(t *testing.T) {
+	tests := []struct {
+		name           string
+		contents       string
+		wantErrNotYAML bool
+		wantOtherErr   bool
+	}{
+		{name: "empty content is allowed", contents: ""},
+		{name: "whitespace content is allowed", contents: "   \n"},
+		{name: "mapping YAML is allowed", contents: "a: b"},
+		{name: "flow sequence YAML is allowed", contents: "a: [1, 2]"},
+		{name: "block sequence YAML is allowed", contents: "a:\n  - 1\n  - 2"},
+		{name: "nested mapping YAML is allowed", contents: "a:\n  b: c"},
+		{name: "scalar YAML is rejected", contents: "hello, there", wantErrNotYAML: true},
+		{name: "sequence YAML is rejected", contents: "- a\n- b", wantErrNotYAML: true},
+		{name: "invalid YAML syntax returns parser error", contents: "a: [1, 2", wantOtherErr: true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := isYaml(tt.contents)
+			switch {
+			case tt.wantErrNotYAML:
+				assert.ErrorContains(t, err, errNotYAML.Error())
+			case tt.wantOtherErr:
+				assert.Error(t, err)
+			default:
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestIsFilenameTaken(t *testing.T) {
+	existing := []v1beta1.CloudInit{
+		{ObjectMeta: v1.ObjectMeta{Name: "ssh-access"}, Spec: v1beta1.CloudInitSpec{Filename: "99_ssh.yaml"}},
+		{ObjectMeta: v1.ObjectMeta{Name: "ntp-config"}, Spec: v1beta1.CloudInitSpec{Filename: "99_ntp.yaml"}},
+	}
+
+	tests := []struct {
+		name      string
+		cloudInit *v1beta1.CloudInit
+		wantTaken bool
+	}{
+		{
+			name:      "filename not in use",
+			cloudInit: &v1beta1.CloudInit{ObjectMeta: v1.ObjectMeta{Name: "new"}, Spec: v1beta1.CloudInitSpec{Filename: "new-file.yaml"}},
+			wantTaken: false,
+		},
+		{
+			name:      "filename already taken",
+			cloudInit: &v1beta1.CloudInit{ObjectMeta: v1.ObjectMeta{Name: "new"}, Spec: v1beta1.CloudInitSpec{Filename: "99_ssh.yaml"}},
+			wantTaken: true,
+		},
+		{
+			name:      "filename taken but ignored because it belongs to the same object",
+			cloudInit: &v1beta1.CloudInit{ObjectMeta: v1.ObjectMeta{Name: "ssh-access"}, Spec: v1beta1.CloudInitSpec{Filename: "99_ssh.yaml"}},
+			wantTaken: false,
+		},
+		{
+			name:      "filename taken by another object",
+			cloudInit: &v1beta1.CloudInit{ObjectMeta: v1.ObjectMeta{Name: "ssh-access"}, Spec: v1beta1.CloudInitSpec{Filename: "99_ntp.yaml"}},
+			wantTaken: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctl := &CloudInit{cloudinits: &mockClient{list: existing}}
+			taken, err := ctl.isFilenameTaken(tt.cloudInit)
+			assert.NoError(t, err)
+			assert.Equal(t, tt.wantTaken, taken)
+		})
+	}
+}
+
 // Sadly, github.com/rancher/wrangler/pkg/generic/fake package generates mock clients that lack
 // the ctx parameter that is required by the CloudInitInterface.
 

pkg/utils/oem.go

@@ -6,7 +6,7 @@ import (
 	"github.com/mudler/yip/pkg/schema"
 	"github.com/sirupsen/logrus"
 	govfs "github.com/twpayne/go-vfs"
-	"gopkg.in/yaml.v1"
+	"go.yaml.in/yaml/v4"
 )
 
 var validateStages = []string{