Merge pull request #107725 from neolit123/1.24-kubeadm-improve-strict-validation

kubeadm: improve the strict unmarshaling of component config

cmd/kubeadm/app/componentconfigs/configset.go

@@ -31,6 +31,7 @@ import (
 	"k8s.io/kubernetes/cmd/kubeadm/app/apis/output"
 	kubeadmutil "k8s.io/kubernetes/cmd/kubeadm/app/util"
 	"k8s.io/kubernetes/cmd/kubeadm/app/util/apiclient"
+	"k8s.io/kubernetes/cmd/kubeadm/app/util/config/strict"
 )
 
 // handler is a package internal type that handles component config factory and common functionality.
@@ -171,6 +172,11 @@ func (cb *configBase) Unmarshal(from kubeadmapi.DocumentMap, into runtime.Object
 			}
 		}
 
+		// Print warnings for strict errors
+		if err := strict.VerifyUnmarshalStrict([]*runtime.Scheme{Scheme}, gvk, yaml); err != nil {
+			klog.Warning(err.Error())
+		}
+
 		// As long as we support only component configs with a single kind, this is allowed
 		return runtime.DecodeInto(Codecs.UniversalDecoder(), yaml, into)
 	}

cmd/kubeadm/app/util/config/cluster.go

@@ -33,6 +33,7 @@ import (
 	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/clientcmd"
 	certutil "k8s.io/client-go/util/cert"
+	"k8s.io/klog/v2"
 
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
 	kubeadmscheme "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/scheme"
@@ -40,6 +41,7 @@ import (
 	"k8s.io/kubernetes/cmd/kubeadm/app/componentconfigs"
 	"k8s.io/kubernetes/cmd/kubeadm/app/constants"
 	"k8s.io/kubernetes/cmd/kubeadm/app/util/apiclient"
+	"k8s.io/kubernetes/cmd/kubeadm/app/util/config/strict"
 )
 
 // FetchInitConfigurationFromCluster fetches configuration from a ConfigMap in the cluster
@@ -82,6 +84,12 @@ func getInitConfigurationFromCluster(kubeconfigDir string, client clientset.Inte
 	if !ok {
 		return nil, errors.Errorf("unexpected error when reading kubeadm-config ConfigMap: %s key value pair missing", constants.ClusterConfigurationConfigMapKey)
 	}
+	// If ClusterConfiguration was patched by something other than kubeadm, it may have errors. Warn about them.
+	if err := strict.VerifyUnmarshalStrict([]*runtime.Scheme{kubeadmscheme.Scheme},
+		kubeadmapiv1.SchemeGroupVersion.WithKind(constants.ClusterConfigurationKind),
+		[]byte(clusterConfigurationData)); err != nil {
+		klog.Warning(err.Error())
+	}
 	if err := runtime.DecodeInto(kubeadmscheme.Codecs.UniversalDecoder(), []byte(clusterConfigurationData), &initcfg.ClusterConfiguration); err != nil {
 		return nil, errors.Wrap(err, "failed to decode cluster configuration data")
 	}

cmd/kubeadm/app/util/config/initconfiguration.go

@@ -302,7 +302,9 @@ func documentMapToInitConfiguration(gvkmap kubeadmapi.DocumentMap, allowDeprecat
 		}
 
 		// verify the validity of the YAML
-		strict.VerifyUnmarshalStrict(fileContent, gvk)
+		if err := strict.VerifyUnmarshalStrict([]*runtime.Scheme{kubeadmscheme.Scheme, componentconfigs.Scheme}, gvk, fileContent); err != nil {
+			klog.Warning(err.Error())
+		}
 
 		if kubeadmutil.GroupVersionKindsHasInitConfiguration(gvk) {
 			// Set initcfg to an empty struct value the deserializer will populate

cmd/kubeadm/app/util/config/joinconfiguration.go

@@ -104,7 +104,9 @@ func documentMapToJoinConfiguration(gvkmap kubeadmapi.DocumentMap, allowDeprecat
 		}
 
 		// verify the validity of the YAML
-		strict.VerifyUnmarshalStrict(bytes, gvk)
+		if err := strict.VerifyUnmarshalStrict([]*runtime.Scheme{kubeadmscheme.Scheme}, gvk, bytes); err != nil {
+			klog.Warning(err.Error())
+		}
 
 		joinBytes = bytes
 	}

cmd/kubeadm/app/util/config/strict/strict.go

@@ -19,41 +19,31 @@ package strict
 import (
 	"github.com/pkg/errors"
 
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/klog/v2"
-	"sigs.k8s.io/yaml"
-
-	"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/scheme"
-	"k8s.io/kubernetes/cmd/kubeadm/app/componentconfigs"
+	"k8s.io/apimachinery/pkg/runtime/serializer/json"
 )
 
-// VerifyUnmarshalStrict takes a YAML byte slice and a GroupVersionKind and verifies if the YAML
-// schema is known and if it unmarshals with strict mode.
-//
-// TODO(neolit123): The returned error here is currently ignored everywhere and a klog warning is thrown instead.
-// We don't want to turn this into an actual error yet. Eventually this can be controlled with an optional CLI flag.
-func VerifyUnmarshalStrict(bytes []byte, gvk schema.GroupVersionKind) error {
-
-	var (
-		iface interface{}
-		err   error
-	)
-
-	iface, err = scheme.Scheme.New(gvk)
-	if err != nil {
-		iface, err = componentconfigs.Scheme.New(gvk)
-		if err != nil {
-			err := errors.Errorf("unknown configuration %#v for scheme definitions in %q and %q",
-				gvk, scheme.Scheme.Name(), componentconfigs.Scheme.Name())
-			klog.Warning(err.Error())
-			return err
+// VerifyUnmarshalStrict takes a slice of schems, a JSON/YAML byte slice and a GroupVersionKind
+// and verifies if the schema is known and if the byte slice unmarshals with strict mode.
+func VerifyUnmarshalStrict(schemes []*runtime.Scheme, gvk schema.GroupVersionKind, bytes []byte) error {
+	var scheme *runtime.Scheme
+	for _, s := range schemes {
+		if _, err := s.New(gvk); err == nil {
+			scheme = s
+			break
 		}
 	}
-
-	if err := yaml.UnmarshalStrict(bytes, iface); err != nil {
-		err := errors.Wrapf(err, "error unmarshaling configuration %#v", gvk)
-		klog.Warning(err.Error())
-		return err
+	if scheme == nil {
+		return errors.Errorf("unknown configuration %#v", gvk)
 	}
+
+	opt := json.SerializerOptions{Yaml: true, Pretty: false, Strict: true}
+	serializer := json.NewSerializerWithOptions(json.DefaultMetaFactory, scheme, scheme, opt)
+	_, _, err := serializer.Decode(bytes, &gvk, nil)
+	if err != nil {
+		return errors.Wrapf(err, "error unmarshaling configuration %#v", gvk)
+	}
+
 	return nil
 }

cmd/kubeadm/app/util/config/strict/strict_test.go

@@ -14,19 +14,23 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package strict
+package strict_test
 
 import (
 	"os"
 	"path/filepath"
 	"testing"
 
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	kubeproxyconfigv1alpha1 "k8s.io/kube-proxy/config/v1alpha1"
 	kubeletconfigv1beta1 "k8s.io/kubelet/config/v1beta1"
 
+	kubeadmscheme "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/scheme"
 	kubeadmapiv1 "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta3"
+	"k8s.io/kubernetes/cmd/kubeadm/app/componentconfigs"
 	"k8s.io/kubernetes/cmd/kubeadm/app/constants"
+	"k8s.io/kubernetes/cmd/kubeadm/app/util/config/strict"
 )
 
 func TestVerifyUnmarshalStrict(t *testing.T) {
@@ -34,6 +38,7 @@ func TestVerifyUnmarshalStrict(t *testing.T) {
 		pathTestData = "testdata/"
 	)
 
+	var schemes = []*runtime.Scheme{kubeadmscheme.Scheme, componentconfigs.Scheme}
 	var testFiles = []struct {
 		fileName      string
 		kind          string
@@ -41,6 +46,12 @@ func TestVerifyUnmarshalStrict(t *testing.T) {
 		expectedError bool
 	}{
 		// tests with file errors
+		{
+			fileName:      "invalid_casesensitive_field.yaml",
+			kind:          constants.ClusterConfigurationKind,
+			groupVersion:  kubeadmapiv1.SchemeGroupVersion,
+			expectedError: true,
+		},
 		{
 			fileName:      "invalid_duplicate_field_clustercfg.yaml",
 			kind:          constants.InitConfigurationKind,
@@ -152,7 +163,7 @@ func TestVerifyUnmarshalStrict(t *testing.T) {
 				Version: test.groupVersion.Version,
 				Kind:    test.kind,
 			}
-			err = VerifyUnmarshalStrict(bytes, gvk)
+			err = strict.VerifyUnmarshalStrict(schemes, gvk, bytes)
 			if (err != nil) != test.expectedError {
 				t.Errorf("expected error %v, got %v, error: %v", err != nil, test.expectedError, err)
 			}

cmd/kubeadm/app/util/config/strict/testdata/invalid_casesensitive_field.yaml

@@ -0,0 +1,3 @@
+apiVersion: kubeadm.k8s.io/v1beta2
+kind: ClusterConfiguration
+ControlPlaneEndpoint: test1