From da7422d1c4923644f03119dde21e1ffa51187fd9 Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Fri, 2 Feb 2024 10:19:00 -0600 Subject: [PATCH] Unstructured OpenstackProviderSpec Another attempt to address the move of OpenstackProviderSpec from cluster-api-provider-openstack to openstack/api/machine. See #2117 for background. In that PR, we simply replaced our imports. The result is that we could only decode the _new_ thing. The problem is that we may still get the _old_ thing if we're talking to an old cluster. We only need the decoding to steal the osImage from the master machines so we can use that value for new workers. Rather than importing both schemata, trying to figure out which version we're talking to, and decoding against the correct one, this commit unmarshals the providerSpec as raw JSON into an Unstructured object and explicitly paths into it map-wise. On the creation side, we're told that the spoke cluster itself will understand the new thing even if it's an old version (MAPI also unmarshals the JSON raw). So it should be fine to continue using recent vendored installer code to generate MachineSets (which will contain new-apiVersion OpenstackProviderSpec). HIVE-2308 (cherry picked from commit 28dac77cfc2d836956972ea9f2254de962ce63af) (cherry picked from commit 567cdd3ab38a0cc0d5d1cb3ede4ee3b4d31ce723) --- .../machinepool/machinepool_controller.go | 2 +- .../machinepool/openstackactuator.go | 57 ++++--- .../machinepool/openstackactuator_test.go | 149 ++++++++++++++++++ 3 files changed, 181 insertions(+), 27 deletions(-) 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")