Unstructured OpenstackProviderSpec

pkg/controller/machinepool/machinepool_controller.go

@@ -1105,7 +1105,7 @@ func (r *ReconcileMachinePool) createActuator(
 		}
 		return NewAzureActuator(creds, cd.Spec.Platform.Azure.CloudName.Name(), logger)
 	case cd.Spec.Platform.OpenStack != nil:
-		return NewOpenStackActuator(masterMachine, r.scheme, r.Client, logger)
+		return NewOpenStackActuator(masterMachine, r.Client, logger)
 	case cd.Spec.Platform.VSphere != nil:
 		return NewVSphereActuator(masterMachine, r.scheme, logger)
 	case cd.Spec.Platform.Ovirt != nil:

pkg/controller/machinepool/openstackactuator.go

@@ -11,12 +11,12 @@ import (
 	"gopkg.in/yaml.v2"
 
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/runtime/serializer"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/json"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
-	machinev1alpha1 "github.com/openshift/api/machine/v1alpha1"
 	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
 	installosp "github.com/openshift/installer/pkg/asset/machines/openstack"
 	installertypes "github.com/openshift/installer/pkg/types"
@@ -38,8 +38,8 @@ type OpenStackActuator struct {
 var _ Actuator = &OpenStackActuator{}
 
 // NewOpenStackActuator is the constructor for building a OpenStackActuator
-func NewOpenStackActuator(masterMachine *machinev1beta1.Machine, scheme *runtime.Scheme, kubeClient client.Client, logger log.FieldLogger) (*OpenStackActuator, error) {
-	osImage, err := getOpenStackOSImage(masterMachine, scheme, logger)
+func NewOpenStackActuator(masterMachine *machinev1beta1.Machine, kubeClient client.Client, logger log.FieldLogger) (*OpenStackActuator, error) {
+	osImage, err := getOpenStackOSImage(masterMachine, logger)
 	if err != nil {
 		logger.WithError(err).Error("error getting os image from master machine")
 		return nil, err
@@ -139,37 +139,42 @@ func (a *OpenStackActuator) GenerateMachineSets(cd *hivev1.ClusterDeployment, po
 }
 
 // Get the OS image from an existing master machine.
-func getOpenStackOSImage(masterMachine *machinev1beta1.Machine, scheme *runtime.Scheme, logger log.FieldLogger) (string, error) {
-	providerSpec, err := decodeOpenStackMachineProviderSpec(masterMachine.Spec.ProviderSpec.Value, scheme)
-	if err != nil {
+func getOpenStackOSImage(masterMachine *machinev1beta1.Machine, logger log.FieldLogger) (string, error) {
+	var u unstructured.Unstructured
+	if err := json.Unmarshal(masterMachine.Spec.ProviderSpec.Value.Raw, &u); err != nil {
 		logger.WithError(err).Warn("cannot decode OpenstackProviderSpec from master machine")
-		return "", errors.Wrap(err, "cannot decode OpenstackProviderSpec from master machine")
+		return "", errors.Wrap(err, "Failed to unmarshal master machine OpenstackProviderSpec to Unstructured")
 	}
-	var osImage string
-	if providerSpec.RootVolume != nil {
-		osImage = providerSpec.RootVolume.SourceUUID
-	} else {
-		osImage = providerSpec.Image
+	osImage, err := osImageFromUnstructuredProviderSpec(&u)
+	if err != nil {
+		logger.WithError(err).Warn("cannot glean OSImage from master machine OpenstackProviderSpec")
+		return "", err
 	}
 	logger.WithField("image", osImage).Debug("resolved image to use for new machinesets")
 	return osImage, nil
 }
 
-func decodeOpenStackMachineProviderSpec(rawExt *runtime.RawExtension, scheme *runtime.Scheme) (*machinev1alpha1.OpenstackProviderSpec, error) {
-	codecFactory := serializer.NewCodecFactory(scheme)
-	decoder := codecFactory.UniversalDecoder(machinev1alpha1.GroupVersion)
-	if rawExt == nil {
-		return nil, fmt.Errorf("MachineSet has no ProviderSpec")
+// osImageFromUnstructuredProviderSpec exists because it's hard to decode OpenstackProviderSpec into a go object
+// because, depending on the version of the cluster from which it was obtained, it may be one of two different
+// apiVersions. Rather than trying to figure out which one, just path into it in its Unstructured form.
+func osImageFromUnstructuredProviderSpec(u *unstructured.Unstructured) (string, error) {
+	sourceUUID, sfound, serr := unstructured.NestedString(u.Object, "rootVolume", "sourceUUID")
+	if serr == nil && sfound && sourceUUID != "" {
+		return sourceUUID, nil
 	}
-	obj, gvk, err := decoder.Decode([]byte(rawExt.Raw), nil, nil)
-	if err != nil {
-		return nil, fmt.Errorf("could not decode OpenStack ProviderSpec: %v", err)
+	image, ifound, ierr := unstructured.NestedString(u.Object, "image")
+	if ierr == nil && ifound && image != "" {
+		return image, nil
 	}
-	spec, ok := obj.(*machinev1alpha1.OpenstackProviderSpec)
-	if !ok {
-		return nil, fmt.Errorf("unexpected object: %#v", gvk)
+	// Now it's about making the error helpful
+	if ierr != nil || serr != nil {
+		// Aggregate will filter out nil errors
+		return "", utilerrors.NewAggregate([]error{serr, ierr})
+	}
+	if !sfound && !ifound {
+		return "", fmt.Errorf("cannot find rootVolume.sourceUUID or image fields")
 	}
-	return spec, nil
+	return "", fmt.Errorf("empty OSImage found")
 }
 
 // yamlOptsBuilder lets us provide our own functions to return a 'clouds.yaml' file that has been

pkg/controller/machinepool/openstackactuator_test.go

@@ -9,6 +9,8 @@ import (
 	"github.com/stretchr/testify/require"
 
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/yaml"
 
 	machinev1alpha1 "github.com/openshift/api/machine/v1alpha1"
 	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
@@ -54,6 +56,153 @@ func BROKEN__TestOpenStackActuator(t *testing.T) {
 	}
 }
 
+func Test_getOpenStackOSImage(t *testing.T) {
+	logger := log.WithField("actuator", "openstackactuator_test")
+	tests := []struct {
+		name             string
+		providerSpecYaml string
+		wantOSImage      string
+		wantErr          bool
+	}{
+		{
+			name:             "Un-unmarshalable providerSpec",
+			providerSpecYaml: "this is not valid yaml",
+			wantErr:          true,
+		},
+		{
+			name: "Old apiVersion, OSImage in image",
+			// This is a real providerSpec from a real live test. We won't bother with
+			// complete ones after this.
+			providerSpecYaml: `apiVersion: openstackproviderconfig.openshift.io/v1alpha1
+cloudName: openstack
+cloudsSecret:
+  name: openstack-cloud-credentials
+  namespace: openshift-machine-api
+flavor: m1.xlarge
+image: clc-auto-psi-fh6rg-rhcos
+kind: OpenstackProviderSpec
+metadata:
+  creationTimestamp: null
+networks:
+- filter: {}
+  subnets:
+  - filter:
+    name: clc-auto-psi-fh6rg-nodes
+    tags: openshiftClusterID=clc-auto-psi-fh6rg
+securityGroups:
+- filter: {}
+  name: clc-auto-psi-fh6rg-worker
+serverGroupName: clc-auto-psi-fh6rg-worker
+serverMetadata:
+  Name: clc-auto-psi-fh6rg-worker
+  openshiftClusterID: clc-auto-psi-fh6rg
+tags:
+- openshiftClusterID=clc-auto-psi-fh6rg
+trunk: true
+userDataSecret:
+  name: worker-user-data
+`,
+			wantOSImage: "clc-auto-psi-fh6rg-rhcos",
+		},
+		{
+			name: "Future apiVersion, OSImage in rootVolume",
+			providerSpecYaml: `apiVersion: something.we.do.not.know.yet/v1theta1
+kind: OpenstackProviderSpec
+rootVolume:
+  sourceUUID: the-image
+`,
+			wantOSImage: "the-image",
+		},
+		{
+			name: "sourceUUID takes precedence over image",
+			providerSpecYaml: `apiVersion: whatever/v1
+image: not-the-image
+kind: OpenstackProviderSpec
+rootVolume:
+  sourceUUID: the-image
+`,
+			wantOSImage: "the-image",
+		},
+		{
+			name: "empty rootVolume ignored",
+			providerSpecYaml: `apiVersion: whatever/v1
+image: the-image
+kind: OpenstackProviderSpec
+rootVolume: {}
+`,
+			wantOSImage: "the-image",
+		},
+		{
+			name: "empty sourceUUID ignored",
+			providerSpecYaml: `apiVersion: whatever/v1
+image: the-image
+kind: OpenstackProviderSpec
+rootVolume:
+  sourceUUID: ""
+`,
+			wantOSImage: "the-image",
+		},
+		{
+			name: "rootVolume not castable to map",
+			providerSpecYaml: `apiVersion: whatever/v1
+kind: OpenstackProviderSpec
+rootVolume: 42
+`,
+			wantErr: true,
+		},
+		{
+			name: "sourceUUID not castable to string",
+			providerSpecYaml: `apiVersion: whatever/v1
+kind: OpenstackProviderSpec
+rootVolume:
+  sourceUUID: 42
+`,
+			wantErr: true,
+		},
+		{
+			name: "image not castable to string",
+			providerSpecYaml: `apiVersion: whatever/v1
+image: 42
+kind: OpenstackProviderSpec
+`,
+			wantErr: true,
+		},
+		{
+			name: "no osImage found",
+			providerSpecYaml: `apiVersion: whatever/v1
+image: ""
+kind: OpenstackProviderSpec
+rootVolume:
+  sourceUUID: ""
+`,
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			providerSpecJSON, err := yaml.YAMLToJSON([]byte(tt.providerSpecYaml))
+			assert.NoError(t, err, "Couldn't convert yaml providerSpec test input to JSON")
+			masterMachine := &machinev1beta1.Machine{
+				Spec: machinev1beta1.MachineSpec{
+					ProviderSpec: machinev1beta1.ProviderSpec{
+						Value: &runtime.RawExtension{
+							Raw: providerSpecJSON,
+						},
+					},
+				},
+			}
+			gotOSImage, err := getOpenStackOSImage(masterMachine, logger)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("getOpenStackOSImage() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if gotOSImage != tt.wantOSImage {
+				t.Errorf("getOpenStackOSImage() = %v, want %v", gotOSImage, tt.wantOSImage)
+			}
+		})
+	}
+}
+
 func validateOSPMachineSets(t *testing.T, mSets []*machinev1beta1.MachineSet, expectedMSReplicas map[string]int64) {
 	assert.Equal(t, len(expectedMSReplicas), len(mSets), "different number of machine sets generated than expected")