diff --git a/pkg/controller/machinepool/machinepool_controller.go b/pkg/controller/machinepool/machinepool_controller.go index b5034d472be..ddd1e065e65 100644 --- a/pkg/controller/machinepool/machinepool_controller.go +++ b/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: diff --git a/pkg/controller/machinepool/openstackactuator.go b/pkg/controller/machinepool/openstackactuator.go index 7e96e497146..0e38d3c3320 100644 --- a/pkg/controller/machinepool/openstackactuator.go +++ b/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 diff --git a/pkg/controller/machinepool/openstackactuator_test.go b/pkg/controller/machinepool/openstackactuator_test.go index 53bdc8ccdf9..35cd1edb4f1 100644 --- a/pkg/controller/machinepool/openstackactuator_test.go +++ b/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")