Make APIVersion/Kind mandatory for MachineList (kubernetes-sigs#756)

Reworked util.ParseMachineYaml to enforce APIVersion and Kind as requirements to parse a MachineList object. As this is a breaking change a new message is introduced when parsing a YAML file that uses the old convention, test suite was upgraded with a few cases. Recommended to move to UniversialDeserializer once the API is stabilized and added to k8s.io/client-go Fixes: kubernetes-sigs#717
jichenjc · Feb 19, 2019 · eca8ab1 · eca8ab1
1 parent 770a417
commit eca8ab1
Show file tree

Hide file tree

Showing 2 changed files with 102 additions and 71 deletions.
diff --git a/pkg/util/util.go b/pkg/util/util.go
@@ -18,6 +18,7 @@ package util
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"math/rand"
@@ -40,6 +41,9 @@ import (
 const (
 	// CharSet defines the alphanumeric set for random string generation
 	CharSet = "0123456789abcdefghijklmnopqrstuvwxyz"
+	// MachineListFormatDeprecationMessage notifies the user that the old
+	// MachineList format is no longer supported
+	MachineListFormatDeprecationMessage = "Your MachineList items must include Kind and APIVersion"
 )
 
 var (
@@ -232,58 +236,50 @@ func ParseMachinesYaml(file string) ([]*clusterv1.Machine, error) {
 	defer reader.Close()
 
 	decoder := yaml.NewYAMLOrJSONDecoder(reader, 32)
-	machineList, err := decodeMachineLists(decoder)
 
-	if err != nil {
+	var (
+		bytes       [][]byte
+		machineList clusterv1.MachineList
+		machine     clusterv1.Machine
+		machines    = []clusterv1.Machine{}
+	)
+
+	// TODO: use the universal decoder instead of doing this.
+	if bytes, err = decodeClusterV1Kinds(decoder, "MachineList"); err != nil {
+		if isMissingKind(err) {
+			err = errors.New(MachineListFormatDeprecationMessage)
+		}
 		return nil, err
 	}
+	// TODO: this is O(n^2) and must be optimized
+	for _, ml := range bytes {
+		if err := json.Unmarshal(ml, &machineList); err != nil {
+			return nil, err
+		}
+		for _, machine := range machineList.Items {
+			if machine.APIVersion == "" || machine.Kind == "" {
+				return nil, errors.New(MachineListFormatDeprecationMessage)
+			}
+			machines = append(machines, machine)
+		}
+	}
 
-	// Will reread the file to find items which aren't a list.
-	// TODO: Make the Kind field mandatory on machines.yaml and then use the
-	// universal decoder instead of doing this.
-	// https://github.com/kubernetes-sigs/cluster-api/issues/717
+	// reset reader to search for discrete Machine definitions
 	if _, err := reader.Seek(0, 0); err != nil {
 		return nil, err
 	}
 
-	bytes, err := decodeClusterV1Kinds(decoder, "Machine")
-
-	// Original set of MachineLists did not have Kind field
-	if err != nil && !isMissingKind(err) {
+	if bytes, err = decodeClusterV1Kinds(decoder, "Machine"); err != nil {
 		return nil, err
 	}
-
-	machines := []clusterv1.Machine{}
-
 	for _, m := range bytes {
-		var machine clusterv1.Machine
-		err = json.Unmarshal(m, &machine)
-		if err != nil {
+		if err := json.Unmarshal(m, &machine); err != nil {
 			return nil, err
 		}
 		machines = append(machines, machine)
 	}
 
-	machinesP := MachineP(machines)
-
-	return append(machinesP, machineList...), nil
-}
-
-// decodeMachineLists extracts MachineLists from a byte reader
-func decodeMachineLists(decoder *yaml.YAMLOrJSONDecoder) ([]*clusterv1.Machine, error) {
-
-	outs := []clusterv1.Machine{}
-
-	for {
-		var out clusterv1.MachineList
-		err := decoder.Decode(&out)
-
-		if err == io.EOF {
-			break
-		}
-		outs = append(outs, out.Items...)
-	}
-	return MachineP(outs), nil
+	return MachineP(machines), nil
 }
 
 // isMissingKind reimplements runtime.IsMissingKind as the YAMLOrJSONDecoder

diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go
@@ -30,14 +30,6 @@ metadata:
 spec:`
 
 const validMachines1 = `
-items:
-- apiVersion: "cluster.k8s.io/v1alpha1"
-  kind: Machine
-  metadata:
-    name: machine1
-  spec:`
-
-const validMachines2 = `
 ---
 apiVersion: "cluster.k8s.io/v1alpha1"
 kind: Machine
@@ -49,15 +41,6 @@ kind: Machine
 metadata:
   name: machine2`
 
-const validMachines3 = `
-items:
-- metadata:
-    name: machine1
-  spec:
-- metadata:
-    name: machine2
-`
-
 const validUnified1 = `
 apiVersion: "cluster.k8s.io/v1alpha1"
 kind: Cluster
@@ -115,7 +98,24 @@ kind: Machine
 metadata:
   name: machine2`
 
-const validUnified4 = `
+const invalidMachines1 = `
+items:
+- apiVersion: "cluster.k8s.io/v1alpha1"
+  kind: Machine
+  metadata:
+    name: machine1
+  spec:`
+
+const invalidMachines2 = `
+items:
+- metadata:
+    name: machine1
+  spec:
+- metadata:
+    name: machine2
+`
+
+const invalidUnified1 = `
 apiVersion: v1
 data:
   cluster_name: cluster1
@@ -142,6 +142,31 @@ items:
     name: machine2
 `
 
+const invalidUnified2 = `
+apiVersion: v1
+data:
+  cluster_name: cluster1
+  cluster_network_pods_cidrBlock: 192.168.0.0/16
+  cluster_network_services_cidrBlock: 10.96.0.0/12
+  cluster_sshKeyName: default
+kind: ConfigMap
+metadata:
+  name: cluster-api-shared-configuration
+  namespace: cluster-api-test
+---
+apiVersion: "cluster.k8s.io/v1alpha1"
+kind: Cluster
+metadata:
+  name: cluster1
+---
+items:
+- metadata:
+    name: machine1
+  spec:
+- metadata:
+		name: machine2
+`
+
 func TestParseClusterYaml(t *testing.T) {
 	t.Run("File does not exist", func(t *testing.T) {
 		_, err := ParseClusterYaml("fileDoesNotExist")
@@ -176,10 +201,15 @@ func TestParseClusterYaml(t *testing.T) {
 			expectedName: "cluster1",
 		},
 		{
-			name:         "valid unified file with machinelist (only with type info) and a configmap",
-			contents:     validUnified4,
+			name:         "valid unified for cluster with invalid machinelist (only with type info) and a configmap",
+			contents:     invalidUnified1,
 			expectedName: "cluster1",
 		},
+		{
+			name:      "valid unified for cluster with invalid machinelist (old top-level items list) and a configmap",
+			contents:  invalidUnified2,
+			expectErr: true,
+		},
 		{
 			name:      "gibberish in file",
 			contents:  `blah ` + validCluster + ` blah`,
@@ -224,19 +254,9 @@ func TestParseMachineYaml(t *testing.T) {
 		expectErr            bool
 		expectedMachineCount int
 	}{
-		{
-			name:                 "valid file using MachineList",
-			contents:             validMachines1,
-			expectedMachineCount: 1,
-		},
 		{
 			name:                 "valid file using Machines",
-			contents:             validMachines2,
-			expectedMachineCount: 2,
-		},
-		{
-			name:                 "valid file using MachineList without type info",
-			contents:             validMachines3,
+			contents:             validMachines1,
 			expectedMachineCount: 2,
 		},
 		{
@@ -255,13 +275,28 @@ func TestParseMachineYaml(t *testing.T) {
 			expectedMachineCount: 2,
 		},
 		{
-			name:                 "valid unified file with machinelist (only with type info) and a configmap",
-			contents:             validUnified4,
-			expectedMachineCount: 2,
+			name:      "invalid file using MachineList",
+			contents:  invalidMachines1,
+			expectErr: true,
+		},
+		{
+			name:      "invalid file using MachineList without type info",
+			contents:  invalidMachines2,
+			expectErr: true,
+		},
+		{
+			name:      "valid unified for cluster with invalid machinelist (only with type info) and a configmap",
+			contents:  invalidUnified1,
+			expectErr: true,
+		},
+		{
+			name:      "valid unified for cluster with invalid machinelist (old top-level items list) and a configmap",
+			contents:  invalidUnified2,
+			expectErr: true,
 		},
 		{
 			name:      "gibberish in file",
-			contents:  `blah ` + validMachines1 + ` blah`,
+			contents:  `!@#blah ` + validMachines1 + ` blah!@#`,
 			expectErr: true,
 		},
 	}