Merge pull request #120549 from pacoxu/kubeadm-etcd-manifests

kubeadm: fix diff order and add tests for new default value manifest

cmd/kubeadm/app/phases/etcd/local_test.go

@@ -22,11 +22,13 @@ package etcd
 import (
 	"fmt"
 	"os"
+	"path"
 	"path/filepath"
 	"reflect"
 	"sort"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/lithammer/dedent"
 
 	v1 "k8s.io/api/core/v1"
@@ -73,6 +75,7 @@ func TestCreateLocalEtcdStaticPodManifestFile(t *testing.T) {
 	var tests = []struct {
 		cfg              *kubeadmapi.ClusterConfiguration
 		expectedError    bool
+		expectedManifest string
 	}{
 		{
 			cfg: &kubeadmapi.ClusterConfiguration{
@@ -84,6 +87,89 @@ func TestCreateLocalEtcdStaticPodManifestFile(t *testing.T) {
 				},
 			},
 			expectedError: false,
+			expectedManifest: fmt.Sprintf(`apiVersion: v1
+kind: Pod
+metadata:
+  annotations:
+    kubeadm.kubernetes.io/etcd.advertise-client-urls: https://:2379
+  creationTimestamp: null
+  labels:
+    component: etcd
+    tier: control-plane
+  name: etcd
+  namespace: kube-system
+spec:
+  containers:
+  - command:
+    - etcd
+    - --advertise-client-urls=https://:2379
+    - --cert-file=etcd/server.crt
+    - --client-cert-auth=true
+    - --data-dir=%s/etcd
+    - --experimental-initial-corrupt-check=true
+    - --experimental-watch-progress-notify-interval=5s
+    - --initial-advertise-peer-urls=https://:2380
+    - --initial-cluster==https://:2380
+    - --key-file=etcd/server.key
+    - --listen-client-urls=https://127.0.0.1:2379,https://:2379
+    - --listen-metrics-urls=http://127.0.0.1:2381
+    - --listen-peer-urls=https://:2380
+    - --name=
+    - --peer-cert-file=etcd/peer.crt
+    - --peer-client-cert-auth=true
+    - --peer-key-file=etcd/peer.key
+    - --peer-trusted-ca-file=etcd/ca.crt
+    - --snapshot-count=10000
+    - --trusted-ca-file=etcd/ca.crt
+    image: /etcd:%s
+    imagePullPolicy: IfNotPresent
+    livenessProbe:
+      failureThreshold: 8
+      httpGet:
+        host: 127.0.0.1
+        path: /health?exclude=NOSPACE&serializable=true
+        port: 2381
+        scheme: HTTP
+      initialDelaySeconds: 10
+      periodSeconds: 10
+      timeoutSeconds: 15
+    name: etcd
+    resources:
+      requests:
+        cpu: 100m
+        memory: 100Mi
+    startupProbe:
+      failureThreshold: 24
+      httpGet:
+        host: 127.0.0.1
+        path: /health?serializable=false
+        port: 2381
+        scheme: HTTP
+      initialDelaySeconds: 10
+      periodSeconds: 10
+      timeoutSeconds: 15
+    volumeMounts:
+    - mountPath: %s/etcd
+      name: etcd-data
+    - mountPath: /etcd
+      name: etcd-certs
+  hostNetwork: true
+  priority: 2000001000
+  priorityClassName: system-node-critical
+  securityContext:
+    seccompProfile:
+      type: RuntimeDefault
+  volumes:
+  - hostPath:
+      path: /etcd
+      type: DirectoryOrCreate
+    name: etcd-certs
+  - hostPath:
+      path: %s/etcd
+      type: DirectoryOrCreate
+    name: etcd-data
+status: {}
+`, tmpdir, kubeadmconstants.DefaultEtcdVersion, tmpdir, tmpdir),
 		},
 		{
 			cfg: &kubeadmapi.ClusterConfiguration{
@@ -115,6 +201,16 @@ func TestCreateLocalEtcdStaticPodManifestFile(t *testing.T) {
 			// Assert expected files are there
 			testutil.AssertFilesCount(t, manifestPath, 1)
 			testutil.AssertFileExists(t, manifestPath, kubeadmconstants.Etcd+".yaml")
+			manifestBytes, err := os.ReadFile(path.Join(manifestPath, kubeadmconstants.Etcd+".yaml"))
+			if err != nil {
+				t.Errorf("failed to load generated manifest file: %v", err)
+			}
+			if test.expectedManifest != string(manifestBytes) {
+				t.Errorf(
+					"File created by CreateLocalEtcdStaticPodManifestFile is not as expected. Diff: \n%s",
+					cmp.Diff(string(manifestBytes), test.expectedManifest),
+				)
+			}
 		} else {
 			testutil.AssertError(t, err, "etcd static pod manifest cannot be generated for cluster using external etcd")
 		}
@@ -165,6 +261,9 @@ func TestCreateLocalEtcdStaticPodManifestFileWithPatches(t *testing.T) {
 		t.Errorf("Error executing ReadStaticPodFromDisk: %v", err)
 		return
 	}
+	if pod.Spec.DNSPolicy != "" {
+		t.Errorf("DNSPolicy should be empty but: %v", pod.Spec.DNSPolicy)
+	}
 
 	if _, ok := pod.ObjectMeta.Annotations["patched"]; !ok {
 		t.Errorf("Patches were not applied to %s", kubeadmconstants.Etcd)

cmd/kubeadm/app/phases/upgrade/staticpods.go

@@ -214,13 +214,15 @@ func upgradeComponent(component string, certsRenewMgr *renewal.Manager, waiter a
 	recoverManifests[component] = backupManifestPath
 
 	// Skip upgrade if current and new manifests are equal
-	equal, err := staticpod.ManifestFilesAreEqual(currentManifestPath, newManifestPath)
+	equal, diff, err := staticpod.ManifestFilesAreEqual(currentManifestPath, newManifestPath)
 	if err != nil {
 		return err
 	}
 	if equal {
 		fmt.Printf("[upgrade/staticpods] Current and new manifests of %s are equal, skipping upgrade\n", component)
 		return nil
+	} else {
+		klog.V(4).Infof("Pod manifest files diff:\n%s\n", diff)
 	}
 
 	// if certificate renewal should be performed

cmd/kubeadm/app/util/marshal.go

@@ -54,23 +54,11 @@ func MarshalToYamlForCodecs(obj runtime.Object, gv schema.GroupVersion, codecs s
 	return runtime.Encode(encoder, obj)
 }
 
-// UnmarshalFromYaml unmarshals yaml into an object.
+// UniversalUnmarshal unmarshals YAML or JSON into a runtime.Object using the universal deserializer.
-func UnmarshalFromYaml(buffer []byte, gv schema.GroupVersion) (runtime.Object, error) {
+func UniversalUnmarshal(buffer []byte) (runtime.Object, error) {
-	return UnmarshalFromYamlForCodecs(buffer, gv, clientsetscheme.Codecs)
+	codecs := clientsetscheme.Codecs
-}
+	decoder := codecs.UniversalDeserializer()
-
+	obj, _, err := decoder.Decode(buffer, nil, nil)
-// UnmarshalFromYamlForCodecs unmarshals yaml into an object using the specified codec
-// TODO: Is specifying the gv really needed here?
-// TODO: Can we support json out of the box easily here?
-func UnmarshalFromYamlForCodecs(buffer []byte, gv schema.GroupVersion, codecs serializer.CodecFactory) (runtime.Object, error) {
-	const mediaType = runtime.ContentTypeYAML
-	info, ok := runtime.SerializerInfoForMediaType(codecs.SupportedMediaTypes(), mediaType)
-	if !ok {
-		return nil, errors.Errorf("unsupported media type %q", mediaType)
-	}
-
-	decoder := codecs.DecoderToVersion(info.Serializer, gv)
-	obj, err := runtime.Decode(decoder, buffer)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to decode %s into runtime.Object", buffer)
 	}

cmd/kubeadm/app/util/marshal_test.go

@@ -24,13 +24,9 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apimachinery/pkg/runtime/serializer"
 
-	bootstraptokenv1 "k8s.io/kubernetes/cmd/kubeadm/app/apis/bootstraptoken/v1"
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
-	kubeadmapiv1 "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta3"
 	"k8s.io/kubernetes/cmd/kubeadm/app/constants"
 )
 
@@ -84,7 +80,7 @@ func TestMarshalUnmarshalYaml(t *testing.T) {
 
 	t.Logf("\n%s", bytes)
 
-	obj2, err := UnmarshalFromYaml(bytes, corev1.SchemeGroupVersion)
+	obj2, err := UniversalUnmarshal(bytes)
 	if err != nil {
 		t.Fatalf("unexpected error marshalling: %v", err)
 	}
@@ -111,50 +107,48 @@ func TestMarshalUnmarshalYaml(t *testing.T) {
 	}
 }
 
-func TestMarshalUnmarshalToYamlForCodecs(t *testing.T) {
+func TestUnmarshalJson(t *testing.T) {
-	cfg := &kubeadmapiv1.InitConfiguration{
+	bytes := []byte(string(`{
-		TypeMeta: metav1.TypeMeta{
+  "apiVersion": "v1",
-			Kind:       constants.InitConfigurationKind,
+  "kind": "Pod",
-			APIVersion: kubeadmapiv1.SchemeGroupVersion.String(),
+  "metadata": {
-		},
+    "name": "someName",
-		NodeRegistration: kubeadmapiv1.NodeRegistrationOptions{
+    "namespace": "testNamespace",
-			Name:      "testNode",
+	"labels": {
-			CRISocket: "unix:///var/run/cri.sock",
+		"test": "yes"
-		},
-		BootstrapTokens: []bootstraptokenv1.BootstrapToken{
-			{
-				Token: &bootstraptokenv1.BootstrapTokenString{ID: "abcdef", Secret: "abcdef0123456789"},
-			},
-		},
-		// NOTE: Using MarshalToYamlForCodecs and UnmarshalFromYamlForCodecs for ClusterConfiguration fields here won't work
-		// by design. This is because we have a `json:"-"` annotation in order to avoid struct duplication. See the comment
-		// at the kubeadmapiv1.InitConfiguration definition.
 	}
+  },
+  "spec": {
+	"restartPolicy": "Always"
+  }
+}`))
 
-	kubeadmapiv1.SetDefaults_InitConfiguration(cfg)
-	scheme := runtime.NewScheme()
-	if err := kubeadmapiv1.AddToScheme(scheme); err != nil {
-		t.Fatal(err)
-	}
-	codecs := serializer.NewCodecFactory(scheme)
-
-	bytes, err := MarshalToYamlForCodecs(cfg, kubeadmapiv1.SchemeGroupVersion, codecs)
-	if err != nil {
-		t.Fatalf("unexpected error marshalling InitConfiguration: %v", err)
-	}
 	t.Logf("\n%s", bytes)
 
-	obj, err := UnmarshalFromYamlForCodecs(bytes, kubeadmapiv1.SchemeGroupVersion, codecs)
+	obj2, err := UniversalUnmarshal(bytes)
 	if err != nil {
-		t.Fatalf("unexpected error unmarshalling InitConfiguration: %v", err)
+		t.Fatalf("unexpected error marshalling: %v", err)
 	}
 
-	cfg2, ok := obj.(*kubeadmapiv1.InitConfiguration)
+	pod2, ok := obj2.(*corev1.Pod)
-	if !ok || cfg2 == nil {
+	if !ok {
-		t.Fatal("did not get InitConfiguration back")
+		t.Fatal("did not get a Pod")
 	}
-	if !reflect.DeepEqual(*cfg, *cfg2) {
+
-		t.Errorf("expected %v, got %v", *cfg, *cfg2)
+	if pod2.Name != "someName" {
+		t.Errorf("expected someName, got %q", pod2.Name)
+	}
+
+	if pod2.Namespace != "testNamespace" {
+		t.Errorf("expected testNamespace, got %q", pod2.Namespace)
+	}
+
+	if !reflect.DeepEqual(pod2.Labels, map[string]string{"test": "yes"}) {
+		t.Errorf("expected [test:yes], got %v", pod2.Labels)
+	}
+
+	if pod2.Spec.RestartPolicy != "Always" {
+		t.Errorf("expected Always, got %q", pod2.Spec.RestartPolicy)
 	}
 }
 

cmd/kubeadm/app/util/staticpod/utils.go

@@ -37,7 +37,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/dump"
 	"k8s.io/apimachinery/pkg/util/intstr"
-	"k8s.io/klog/v2"
 
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
 	kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
@@ -190,9 +189,9 @@ func PatchStaticPod(pod *v1.Pod, patchesDir string, output io.Writer) (*v1.Pod,
 		return pod, err
 	}
 
-	obj, err := kubeadmutil.UnmarshalFromYaml(patchTarget.Data, v1.SchemeGroupVersion)
+	obj, err := kubeadmutil.UniversalUnmarshal(patchTarget.Data)
 	if err != nil {
-		return pod, errors.Wrap(err, "failed to unmarshal patched manifest from YAML")
+		return pod, errors.Wrap(err, "failed to unmarshal patched manifest")
 	}
 
 	pod2, ok := obj.(*v1.Pod)
@@ -233,12 +232,15 @@ func ReadStaticPodFromDisk(manifestPath string) (*v1.Pod, error) {
 		return &v1.Pod{}, errors.Wrapf(err, "failed to read manifest for %q", manifestPath)
 	}
 
-	obj, err := kubeadmutil.UnmarshalFromYaml(buf, v1.SchemeGroupVersion)
+	obj, err := kubeadmutil.UniversalUnmarshal(buf)
 	if err != nil {
-		return &v1.Pod{}, errors.Errorf("failed to unmarshal manifest for %q from YAML: %v", manifestPath, err)
+		return &v1.Pod{}, errors.Errorf("failed to unmarshal manifest for %q: %v", manifestPath, err)
 	}
 
-	pod := obj.(*v1.Pod)
+	pod, ok := obj.(*v1.Pod)
+	if !ok {
+		return &v1.Pod{}, errors.Errorf("failed to parse Pod object defined in %q", manifestPath)
+	}
 
 	return pod, nil
 }
@@ -354,14 +356,14 @@ func GetEtcdProbeEndpoint(cfg *kubeadmapi.Etcd, isIPv6 bool) (string, int32, v1.
 }
 
 // ManifestFilesAreEqual compares 2 files. It returns true if their contents are equal, false otherwise
-func ManifestFilesAreEqual(path1, path2 string) (bool, error) {
+func ManifestFilesAreEqual(path1, path2 string) (bool, string, error) {
 	pod1, err := ReadStaticPodFromDisk(path1)
 	if err != nil {
-		return false, err
+		return false, "", err
 	}
 	pod2, err := ReadStaticPodFromDisk(path2)
 	if err != nil {
-		return false, err
+		return false, "", err
 	}
 
 	hasher := md5.New()
@@ -370,10 +372,9 @@ func ManifestFilesAreEqual(path1, path2 string) (bool, error) {
 	DeepHashObject(hasher, pod2)
 	hash2 := hasher.Sum(nil)[0:]
 	if bytes.Equal(hash1, hash2) {
-		return true, nil
+		return true, "", nil
 	}
-	klog.V(4).Infof("Pod manifest files diff:\n%s\n", cmp.Diff(pod1, pod2))
+	return false, cmp.Diff(pod2, pod1), nil
-	return false, nil
 }
 
 // getProbeAddress returns a valid probe address.

cmd/kubeadm/app/util/staticpod/utils_test.go

@@ -23,6 +23,7 @@ import (
 	"reflect"
 	"sort"
 	"strconv"
+	"strings"
 	"testing"
 
 	v1 "k8s.io/api/core/v1"
@@ -652,6 +653,22 @@ spec:
   - image: gcr.io/google_containers/etcd-amd64:3.1.11
 status: {}
 `
+	invalidWithDefaultFields = `
+apiVersion: v1
+kind: Pod
+metadata:
+  labels:
+    tier: control-plane
+    component: etcd
+  name: etcd
+  namespace: kube-system
+spec:
+  containers:
+  - image: gcr.io/google_containers/etcd-amd64:3.1.11
+  restartPolicy: "Always"
+status: {}
+`
+
 	validPod2 = `
 apiVersion: v1
 kind: Pod
@@ -729,6 +746,7 @@ func TestManifestFilesAreEqual(t *testing.T) {
 		description    string
 		podYamls       []string
 		expectedResult bool
+		expectedDiff   string
 		expectErr      bool
 	}{
 		{
@@ -748,6 +766,18 @@ func TestManifestFilesAreEqual(t *testing.T) {
 			podYamls:       []string{validPod, validPod2},
 			expectedResult: false,
 			expectErr:      false,
+			expectedDiff: string(`
+- 					"2",
++ 					"1",`),
+		},
+		{
+			description:    "manifests are not equal for adding new defaults",
+			podYamls:       []string{validPod, invalidWithDefaultFields},
+			expectedResult: false,
+			expectErr:      false,
+			expectedDiff: string(`
+- 		RestartPolicy:                 "Always",
++ 		RestartPolicy:                 "",`),
 		},
 		{
 			description:    "first manifest doesn't exist",
@@ -780,7 +810,7 @@ func TestManifestFilesAreEqual(t *testing.T) {
 			}
 
 			// compare them
-			result, actualErr := ManifestFilesAreEqual(filepath.Join(tmpdir, "0.yaml"), filepath.Join(tmpdir, "1.yaml"))
+			result, diff, actualErr := ManifestFilesAreEqual(filepath.Join(tmpdir, "0.yaml"), filepath.Join(tmpdir, "1.yaml"))
 			if result != rt.expectedResult {
 				t.Errorf(
 					"ManifestFilesAreEqual failed\n%s\nexpected result: %t\nactual result: %t",
@@ -798,6 +828,14 @@ func TestManifestFilesAreEqual(t *testing.T) {
 					actualErr,
 				)
 			}
+			if !strings.Contains(diff, rt.expectedDiff) {
+				t.Errorf(
+					"ManifestFilesAreEqual diff doesn't expected\n%s\n\texpected diff: %s\nactual diff: %s",
+					rt.description,
+					rt.expectedDiff,
+					diff,
+				)
+			}
 		})
 	}
 }