From 5fca0e5d02a1fb7d20cdd57c8a3c20ee96ae91e7 Mon Sep 17 00:00:00 2001 From: Emilien Macchi Date: Mon, 18 Dec 2023 13:28:48 -0500 Subject: [PATCH] Convert Image to use `ImageFilfter` instead of string Right now, the image for an OpenStackMachine can be defined either its name via Spec.Image or via Spec.ImageUUID. Now we will use a single parameter which would be of type ImageFilter. --- api/v1alpha5/conversion.go | 17 +++ api/v1alpha5/conversion_test.go | 4 +- api/v1alpha5/zz_generated.conversion.go | 7 +- api/v1alpha6/conversion.go | 18 ++++ api/v1alpha6/zz_generated.conversion.go | 7 +- api/v1alpha7/conversion.go | 18 ++++ api/v1alpha7/zz_generated.conversion.go | 7 +- api/v1alpha8/filter_convert.go | 9 ++ api/v1alpha8/openstackcluster_webhook_test.go | 4 +- api/v1alpha8/openstackmachine_types.go | 10 +- .../openstackmachinetemplate_webhook_test.go | 16 +-- api/v1alpha8/types.go | 13 +++ api/v1alpha8/zz_generated.deepcopy.go | 21 ++++ ...re.cluster.x-k8s.io_openstackclusters.yaml | 33 ++++-- ...er.x-k8s.io_openstackclustertemplates.yaml | 32 ++++-- ...re.cluster.x-k8s.io_openstackmachines.yaml | 32 ++++-- ...er.x-k8s.io_openstackmachinetemplates.yaml | 29 +++-- controllers/openstackcluster_controller.go | 24 +++-- controllers/openstackmachine_controller.go | 3 +- .../openstackmachine_controller_test.go | 7 +- .../crd-changes/v1alpha7-to-v1alpha8.md | 35 ++++++ .../v1alpha8/default/cluster-template.yaml | 6 +- .../flatcar-sysext/patch-flatcar.yaml | 6 +- kustomize/v1alpha8/flatcar/patch-flatcar.yaml | 6 +- pkg/cloud/services/compute/instance.go | 38 ++----- pkg/cloud/services/compute/instance_test.go | 101 +++++++++--------- pkg/cloud/services/compute/instance_types.go | 3 +- .../services/compute/referenced_resources.go | 10 ++ .../compute/referenced_resources_test.go | 53 +++++++-- .../cluster-template-flatcar-sysext.yaml | 6 +- templates/cluster-template-flatcar.yaml | 6 +- templates/cluster-template-without-lb.yaml | 6 +- templates/cluster-template.yaml | 6 +- .../common-patches/cni/patch-cluster.yaml | 3 +- .../k8s-upgrade/upgrade-from-template.yaml | 6 +- .../k8s-upgrade/upgrade-to-template.yaml | 6 +- test/e2e/data/kustomize/v1alpha5/bastion.yaml | 9 ++ .../kustomize/v1alpha5/kustomization.yaml | 6 ++ test/e2e/data/kustomize/v1alpha6/bastion.yaml | 9 ++ .../kustomize/v1alpha6/kustomization.yaml | 4 + test/e2e/data/kustomize/v1alpha7/bastion.yaml | 9 ++ .../kustomize/v1alpha7/kustomization.yaml | 4 + test/e2e/suites/e2e/e2e_test.go | 12 ++- 43 files changed, 472 insertions(+), 189 deletions(-) create mode 100644 test/e2e/data/kustomize/v1alpha5/bastion.yaml create mode 100644 test/e2e/data/kustomize/v1alpha6/bastion.yaml create mode 100644 test/e2e/data/kustomize/v1alpha7/bastion.yaml diff --git a/api/v1alpha5/conversion.go b/api/v1alpha5/conversion.go index dc675d0c6a..3029ab1038 100644 --- a/api/v1alpha5/conversion.go +++ b/api/v1alpha5/conversion.go @@ -247,6 +247,15 @@ func Convert_v1alpha5_OpenStackMachineSpec_To_v1alpha8_OpenStackMachineSpec(in * out.ServerGroup = &infrav1.ServerGroupFilter{} } + imageFilter := infrav1.ImageFilter{} + if in.Image != "" { + imageFilter.Name = in.Image + } + if in.ImageUUID != "" { + imageFilter.ID = in.ImageUUID + } + out.Image = imageFilter + return nil } @@ -480,6 +489,14 @@ func Convert_v1alpha8_OpenStackMachineSpec_To_v1alpha5_OpenStackMachineSpec(in * out.ServerGroupID = in.ServerGroup.ID } + if in.Image.Name != "" { + out.Image = in.Image.Name + } + + if in.Image.ID != "" { + out.ImageUUID = in.Image.ID + } + return nil } diff --git a/api/v1alpha5/conversion_test.go b/api/v1alpha5/conversion_test.go index 839e9bd4a4..f8ad758fca 100644 --- a/api/v1alpha5/conversion_test.go +++ b/api/v1alpha5/conversion_test.go @@ -79,7 +79,7 @@ func TestConvertFrom(t *testing.T) { Spec: OpenStackMachineSpec{}, ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - "cluster.x-k8s.io/conversion-data": "{\"spec\":{\"cloudName\":\"\",\"flavor\":\"\"},\"status\":{\"ready\":false,\"referencedResources\":{}}}", + "cluster.x-k8s.io/conversion-data": "{\"spec\":{\"cloudName\":\"\",\"flavor\":\"\",\"image\":{}},\"status\":{\"ready\":false,\"referencedResources\":{}}}", }, }, }, @@ -94,7 +94,7 @@ func TestConvertFrom(t *testing.T) { Spec: OpenStackMachineTemplateSpec{}, ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - "cluster.x-k8s.io/conversion-data": "{\"spec\":{\"template\":{\"spec\":{\"cloudName\":\"\",\"flavor\":\"\"}}}}", + "cluster.x-k8s.io/conversion-data": "{\"spec\":{\"template\":{\"spec\":{\"cloudName\":\"\",\"flavor\":\"\",\"image\":{}}}}}", }, }, }, diff --git a/api/v1alpha5/zz_generated.conversion.go b/api/v1alpha5/zz_generated.conversion.go index b1deb78263..23e8f4595c 100644 --- a/api/v1alpha5/zz_generated.conversion.go +++ b/api/v1alpha5/zz_generated.conversion.go @@ -1057,8 +1057,8 @@ func autoConvert_v1alpha5_OpenStackMachineSpec_To_v1alpha8_OpenStackMachineSpec( out.InstanceID = (*string)(unsafe.Pointer(in.InstanceID)) out.CloudName = in.CloudName out.Flavor = in.Flavor - out.Image = in.Image - out.ImageUUID = in.ImageUUID + // WARNING: in.Image requires manual conversion: inconvertible types (string vs sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha8.ImageFilter) + // WARNING: in.ImageUUID requires manual conversion: does not exist in peer-type out.SSHKeyName = in.SSHKeyName // WARNING: in.Networks requires manual conversion: does not exist in peer-type if in.Ports != nil { @@ -1100,8 +1100,7 @@ func autoConvert_v1alpha8_OpenStackMachineSpec_To_v1alpha5_OpenStackMachineSpec( out.InstanceID = (*string)(unsafe.Pointer(in.InstanceID)) out.CloudName = in.CloudName out.Flavor = in.Flavor - out.Image = in.Image - out.ImageUUID = in.ImageUUID + // WARNING: in.Image requires manual conversion: inconvertible types (sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha8.ImageFilter vs string) out.SSHKeyName = in.SSHKeyName if in.Ports != nil { in, out := &in.Ports, &out.Ports diff --git a/api/v1alpha6/conversion.go b/api/v1alpha6/conversion.go index 56aab5fbf7..ab8d50a1bd 100644 --- a/api/v1alpha6/conversion.go +++ b/api/v1alpha6/conversion.go @@ -62,6 +62,7 @@ func restorev1alpha8MachineSpec(previous *infrav1.OpenStackMachineSpec, dst *inf dst.Ports = previous.Ports dst.AdditionalBlockDevices = previous.AdditionalBlockDevices dst.ServerGroup = previous.ServerGroup + dst.Image = previous.Image } func restorev1alpha8Bastion(previous **infrav1.Bastion, dst **infrav1.Bastion) { @@ -393,6 +394,15 @@ func Convert_v1alpha6_OpenStackMachineSpec_To_v1alpha8_OpenStackMachineSpec(in * out.ServerGroup = nil } + imageFilter := infrav1.ImageFilter{} + if in.Image != "" { + imageFilter.Name = in.Image + } + if in.ImageUUID != "" { + imageFilter.ID = in.ImageUUID + } + out.Image = imageFilter + return nil } @@ -721,6 +731,14 @@ func Convert_v1alpha8_OpenStackMachineSpec_To_v1alpha6_OpenStackMachineSpec(in * out.ServerGroupID = in.ServerGroup.ID } + if in.Image.Name != "" { + out.Image = in.Image.Name + } + + if in.Image.ID != "" { + out.ImageUUID = in.Image.ID + } + return nil } diff --git a/api/v1alpha6/zz_generated.conversion.go b/api/v1alpha6/zz_generated.conversion.go index 1501975342..63ab1b88b3 100644 --- a/api/v1alpha6/zz_generated.conversion.go +++ b/api/v1alpha6/zz_generated.conversion.go @@ -1070,8 +1070,8 @@ func autoConvert_v1alpha6_OpenStackMachineSpec_To_v1alpha8_OpenStackMachineSpec( out.InstanceID = (*string)(unsafe.Pointer(in.InstanceID)) out.CloudName = in.CloudName out.Flavor = in.Flavor - out.Image = in.Image - out.ImageUUID = in.ImageUUID + // WARNING: in.Image requires manual conversion: inconvertible types (string vs sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha8.ImageFilter) + // WARNING: in.ImageUUID requires manual conversion: does not exist in peer-type out.SSHKeyName = in.SSHKeyName // WARNING: in.Networks requires manual conversion: does not exist in peer-type if in.Ports != nil { @@ -1113,8 +1113,7 @@ func autoConvert_v1alpha8_OpenStackMachineSpec_To_v1alpha6_OpenStackMachineSpec( out.InstanceID = (*string)(unsafe.Pointer(in.InstanceID)) out.CloudName = in.CloudName out.Flavor = in.Flavor - out.Image = in.Image - out.ImageUUID = in.ImageUUID + // WARNING: in.Image requires manual conversion: inconvertible types (sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha8.ImageFilter vs string) out.SSHKeyName = in.SSHKeyName if in.Ports != nil { in, out := &in.Ports, &out.Ports diff --git a/api/v1alpha7/conversion.go b/api/v1alpha7/conversion.go index 89fa5563b5..25dab7a655 100644 --- a/api/v1alpha7/conversion.go +++ b/api/v1alpha7/conversion.go @@ -67,6 +67,7 @@ var v1alpha8OpenStackClusterRestorer = conversion.RestorerFor[*infrav1.OpenStack func restorev1alpha8MachineSpec(previous *infrav1.OpenStackMachineSpec, dst *infrav1.OpenStackMachineSpec) { dst.ServerGroup = previous.ServerGroup + dst.Image = previous.Image } func restorev1alpha8Bastion(previous **infrav1.Bastion, dst **infrav1.Bastion) { @@ -273,6 +274,14 @@ func Convert_v1alpha8_OpenStackMachineSpec_To_v1alpha7_OpenStackMachineSpec(in * out.ServerGroupID = in.ServerGroup.ID } + if in.Image.Name != "" { + out.Image = in.Image.Name + } + + if in.Image.ID != "" { + out.ImageUUID = in.Image.ID + } + return nil } @@ -288,6 +297,15 @@ func Convert_v1alpha7_OpenStackMachineSpec_To_v1alpha8_OpenStackMachineSpec(in * out.ServerGroup = nil } + imageFilter := infrav1.ImageFilter{} + if in.Image != "" { + imageFilter.Name = in.Image + } + if in.ImageUUID != "" { + imageFilter.ID = in.ImageUUID + } + out.Image = imageFilter + return nil } diff --git a/api/v1alpha7/zz_generated.conversion.go b/api/v1alpha7/zz_generated.conversion.go index dc95a9500f..25b03fcc7e 100644 --- a/api/v1alpha7/zz_generated.conversion.go +++ b/api/v1alpha7/zz_generated.conversion.go @@ -1232,8 +1232,8 @@ func autoConvert_v1alpha7_OpenStackMachineSpec_To_v1alpha8_OpenStackMachineSpec( out.InstanceID = (*string)(unsafe.Pointer(in.InstanceID)) out.CloudName = in.CloudName out.Flavor = in.Flavor - out.Image = in.Image - out.ImageUUID = in.ImageUUID + // WARNING: in.Image requires manual conversion: inconvertible types (string vs sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha8.ImageFilter) + // WARNING: in.ImageUUID requires manual conversion: does not exist in peer-type out.SSHKeyName = in.SSHKeyName out.Ports = *(*[]v1alpha8.PortOpts)(unsafe.Pointer(&in.Ports)) out.FloatingIP = in.FloatingIP @@ -1254,8 +1254,7 @@ func autoConvert_v1alpha8_OpenStackMachineSpec_To_v1alpha7_OpenStackMachineSpec( out.InstanceID = (*string)(unsafe.Pointer(in.InstanceID)) out.CloudName = in.CloudName out.Flavor = in.Flavor - out.Image = in.Image - out.ImageUUID = in.ImageUUID + // WARNING: in.Image requires manual conversion: inconvertible types (sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha8.ImageFilter vs string) out.SSHKeyName = in.SSHKeyName out.Ports = *(*[]PortOpts)(unsafe.Pointer(&in.Ports)) out.FloatingIP = in.FloatingIP diff --git a/api/v1alpha8/filter_convert.go b/api/v1alpha8/filter_convert.go index 23e5330588..378642ce5f 100644 --- a/api/v1alpha8/filter_convert.go +++ b/api/v1alpha8/filter_convert.go @@ -17,6 +17,7 @@ limitations under the License. package v1alpha8 import ( + "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/layer3/routers" securitygroups "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/groups" "github.com/gophercloud/gophercloud/openstack/networking/v2/networks" @@ -79,3 +80,11 @@ func (routerFilter RouterFilter) ToListOpt() routers.ListOpts { NotTagsAny: routerFilter.NotTagsAny, } } + +func (imageFilter ImageFilter) ToListOpt() images.ListOpts { + return images.ListOpts{ + ID: imageFilter.ID, + Name: imageFilter.Name, + Tags: imageFilter.Tags, + } +} diff --git a/api/v1alpha8/openstackcluster_webhook_test.go b/api/v1alpha8/openstackcluster_webhook_test.go index 256988c81a..cf4f33da27 100644 --- a/api/v1alpha8/openstackcluster_webhook_test.go +++ b/api/v1alpha8/openstackcluster_webhook_test.go @@ -119,7 +119,7 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { Bastion: &Bastion{ Instance: OpenStackMachineSpec{ CloudName: "foobar", - Image: "foobar", + Image: ImageFilter{Name: "foobar"}, Flavor: "minimal", }, Enabled: true, @@ -137,7 +137,7 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { Bastion: &Bastion{ Instance: OpenStackMachineSpec{ CloudName: "foobarbaz", - Image: "foobarbaz", + Image: ImageFilter{Name: "foobarbaz"}, Flavor: "medium", }, Enabled: true, diff --git a/api/v1alpha8/openstackmachine_types.go b/api/v1alpha8/openstackmachine_types.go index dc06385276..d5b27b4648 100644 --- a/api/v1alpha8/openstackmachine_types.go +++ b/api/v1alpha8/openstackmachine_types.go @@ -45,13 +45,9 @@ type OpenStackMachineSpec struct { // The flavor reference for the flavor for your server instance. Flavor string `json:"flavor"` - // The name of the image to use for your server instance. - // If the RootVolume is specified, this will be ignored and use rootVolume directly. - Image string `json:"image,omitempty"` - - // The uuid of the image to use for your server instance. - // if it's empty, Image name will be used - ImageUUID string `json:"imageUUID,omitempty"` + // The image to use for your server instance. + // If the rootVolume is specified, this will be used when creating the root volume. + Image ImageFilter `json:"image,omitempty"` // The ssh key to inject in the instance SSHKeyName string `json:"sshKeyName,omitempty"` diff --git a/api/v1alpha8/openstackmachinetemplate_webhook_test.go b/api/v1alpha8/openstackmachinetemplate_webhook_test.go index 6728f9d5fe..e441ef0583 100644 --- a/api/v1alpha8/openstackmachinetemplate_webhook_test.go +++ b/api/v1alpha8/openstackmachinetemplate_webhook_test.go @@ -45,7 +45,7 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { Template: OpenStackMachineTemplateResource{ Spec: OpenStackMachineSpec{ Flavor: "foo", - Image: "bar", + Image: ImageFilter{Name: "bar"}, }, }, }, @@ -55,7 +55,7 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { Template: OpenStackMachineTemplateResource{ Spec: OpenStackMachineSpec{ Flavor: "foo", - Image: "NewImage", + Image: ImageFilter{Name: "NewImage"}, }, }, }, @@ -70,7 +70,7 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { Template: OpenStackMachineTemplateResource{ Spec: OpenStackMachineSpec{ Flavor: "foo", - Image: "bar", + Image: ImageFilter{Name: "bar"}, }, }, }, @@ -83,7 +83,7 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { Template: OpenStackMachineTemplateResource{ Spec: OpenStackMachineSpec{ Flavor: "foo", - Image: "bar", + Image: ImageFilter{Name: "bar"}, }, }, }, @@ -100,7 +100,7 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { Template: OpenStackMachineTemplateResource{ Spec: OpenStackMachineSpec{ Flavor: "foo", - Image: "bar", + Image: ImageFilter{Name: "bar"}, }, }, }, @@ -110,7 +110,7 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { Template: OpenStackMachineTemplateResource{ Spec: OpenStackMachineSpec{ Flavor: "foo", - Image: "NewImage", + Image: ImageFilter{Name: "NewImage"}, }, }, }, @@ -125,7 +125,7 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { Template: OpenStackMachineTemplateResource{ Spec: OpenStackMachineSpec{ Flavor: "foo", - Image: "bar", + Image: ImageFilter{Name: "bar"}, }, }, }, @@ -140,7 +140,7 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { Template: OpenStackMachineTemplateResource{ Spec: OpenStackMachineSpec{ Flavor: "foo", - Image: "NewImage", + Image: ImageFilter{Name: "NewImage"}, }, }, }, diff --git a/api/v1alpha8/types.go b/api/v1alpha8/types.go index cccf992e70..0878df6862 100644 --- a/api/v1alpha8/types.go +++ b/api/v1alpha8/types.go @@ -22,6 +22,15 @@ type OpenStackMachineTemplateResource struct { Spec OpenStackMachineSpec `json:"spec"` } +type ImageFilter struct { + // The ID of the desired image. If this is provided, the other filters will be ignored. + ID string `json:"id,omitempty"` + // The name of the desired image. If specified, the combination of name and tags must return a single matching image or an error will be raised. + Name string `json:"name,omitempty"` + // The tags associated with the desired image. If specified, the combination of name and tags must return a single matching image or an error will be raised. + Tags []string `json:"tags,omitempty"` +} + type ExternalRouterIPParam struct { // The FixedIP in the corresponding subnet FixedIP string `json:"fixedIP,omitempty"` @@ -368,6 +377,10 @@ type ReferencedMachineResources struct { // ServerGroupID is the ID of the server group the machine should be added to and is calculated based on ServerGroupFilter. // +optional ServerGroupID string `json:"serverGroupID,omitempty"` + + // ImageID is the ID of the image to use for the machine and is calculated based on ImageFilter. + // +optional + ImageID string `json:"imageID,omitempty"` } // ValueSpec represents a single value_spec key-value pair. diff --git a/api/v1alpha8/zz_generated.deepcopy.go b/api/v1alpha8/zz_generated.deepcopy.go index c2d6b9fbc0..6598146562 100644 --- a/api/v1alpha8/zz_generated.deepcopy.go +++ b/api/v1alpha8/zz_generated.deepcopy.go @@ -201,6 +201,26 @@ func (in *FixedIP) DeepCopy() *FixedIP { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ImageFilter) DeepCopyInto(out *ImageFilter) { + *out = *in + if in.Tags != nil { + in, out := &in.Tags, &out.Tags + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ImageFilter. +func (in *ImageFilter) DeepCopy() *ImageFilter { + if in == nil { + return nil + } + out := new(ImageFilter) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LoadBalancer) DeepCopyInto(out *LoadBalancer) { *out = *in @@ -647,6 +667,7 @@ func (in *OpenStackMachineSpec) DeepCopyInto(out *OpenStackMachineSpec) { *out = new(string) **out = **in } + in.Image.DeepCopyInto(&out.Image) if in.Ports != nil { in, out := &in.Ports, &out.Ports *out = make([]PortOpts, len(*in)) diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml index 2041780515..0a8cf90fd1 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml @@ -4912,14 +4912,27 @@ spec: - name type: object image: - description: The name of the image to use for your server - instance. If the RootVolume is specified, this will be ignored - and use rootVolume directly. - type: string - imageUUID: - description: The uuid of the image to use for your server - instance. if it's empty, Image name will be used - type: string + description: The image to use for your server instance. If + the rootVolume is specified, this will be used when creating + the root volume. + properties: + id: + description: The ID of the desired image. If this is provided, + the other filters will be ignored. + type: string + name: + description: The name of the desired image. If specified, + the combination of name and tags must return a single + matching image or an error will be raised. + type: string + tags: + description: The tags associated with the desired image. + If specified, the combination of name and tags must + return a single matching image or an error will be raised. + items: + type: string + type: array + type: object instanceID: description: InstanceID is the OpenStack instance ID for this machine. @@ -5480,6 +5493,10 @@ spec: description: ReferencedMachineResources contains resolved references to resources required by the machine. properties: + imageID: + description: ImageID is the ID of the image to use for the + machine and is calculated based on ImageFilter. + type: string serverGroupID: description: ServerGroupID is the ID of the server group the machine should be added to and is calculated based on ServerGroupFilter. diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml index b6a64b4fde..08e725eae4 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml @@ -2445,15 +2445,29 @@ spec: - name type: object image: - description: The name of the image to use for your - server instance. If the RootVolume is specified, - this will be ignored and use rootVolume directly. - type: string - imageUUID: - description: The uuid of the image to use for your - server instance. if it's empty, Image name will - be used - type: string + description: The image to use for your server instance. + If the rootVolume is specified, this will be used + when creating the root volume. + properties: + id: + description: The ID of the desired image. If this + is provided, the other filters will be ignored. + type: string + name: + description: The name of the desired image. If + specified, the combination of name and tags + must return a single matching image or an error + will be raised. + type: string + tags: + description: The tags associated with the desired + image. If specified, the combination of name + and tags must return a single matching image + or an error will be raised. + items: + type: string + type: array + type: object instanceID: description: InstanceID is the OpenStack instance ID for this machine. diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml index 0a0d924c8c..a0bde06ad8 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml @@ -1749,14 +1749,26 @@ spec: - name type: object image: - description: The name of the image to use for your server instance. - If the RootVolume is specified, this will be ignored and use rootVolume - directly. - type: string - imageUUID: - description: The uuid of the image to use for your server instance. - if it's empty, Image name will be used - type: string + description: The image to use for your server instance. If the rootVolume + is specified, this will be used when creating the root volume. + properties: + id: + description: The ID of the desired image. If this is provided, + the other filters will be ignored. + type: string + name: + description: The name of the desired image. If specified, the + combination of name and tags must return a single matching image + or an error will be raised. + type: string + tags: + description: The tags associated with the desired image. If specified, + the combination of name and tags must return a single matching + image or an error will be raised. + items: + type: string + type: array + type: object instanceID: description: InstanceID is the OpenStack instance ID for this machine. type: string @@ -2114,6 +2126,10 @@ spec: description: ReferencedResources contains resolved references to resources that the machine depends on. properties: + imageID: + description: ImageID is the ID of the image to use for the machine + and is calculated based on ImageFilter. + type: string serverGroupID: description: ServerGroupID is the ID of the server group the machine should be added to and is calculated based on ServerGroupFilter. diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachinetemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachinetemplates.yaml index 8fccb2d323..16da499668 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachinetemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachinetemplates.yaml @@ -1464,14 +1464,27 @@ spec: - name type: object image: - description: The name of the image to use for your server - instance. If the RootVolume is specified, this will be ignored - and use rootVolume directly. - type: string - imageUUID: - description: The uuid of the image to use for your server - instance. if it's empty, Image name will be used - type: string + description: The image to use for your server instance. If + the rootVolume is specified, this will be used when creating + the root volume. + properties: + id: + description: The ID of the desired image. If this is provided, + the other filters will be ignored. + type: string + name: + description: The name of the desired image. If specified, + the combination of name and tags must return a single + matching image or an error will be raised. + type: string + tags: + description: The tags associated with the desired image. + If specified, the combination of name and tags must + return a single matching image or an error will be raised. + items: + type: string + type: array + type: object instanceID: description: InstanceID is the OpenStack instance ID for this machine. diff --git a/controllers/openstackcluster_controller.go b/controllers/openstackcluster_controller.go index f83b8b75a9..78bff28628 100644 --- a/controllers/openstackcluster_controller.go +++ b/controllers/openstackcluster_controller.go @@ -237,7 +237,10 @@ func deleteBastion(scope scope.Scope, cluster *clusterv1.Cluster, openStackClust } } - instanceSpec := bastionToInstanceSpec(openStackCluster, cluster.Name) + instanceSpec, err := bastionToInstanceSpec(openStackCluster, cluster.Name, computeService) + if err != nil { + return fmt.Errorf("failed to create bastion InstanceSpec: %w", err) + } if err = computeService.DeleteInstance(openStackCluster, openStackCluster, instanceStatus, instanceSpec); err != nil { handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to delete bastion: %w", err)) @@ -320,7 +323,11 @@ func reconcileBastion(scope scope.Scope, cluster *clusterv1.Cluster, openStackCl return err } - instanceSpec := bastionToInstanceSpec(openStackCluster, cluster.Name) + instanceSpec, err := bastionToInstanceSpec(openStackCluster, cluster.Name, computeService) + if err != nil { + return fmt.Errorf("failed to create bastion InstanceSpec: %w", err) + } + bastionHash, err := compute.HashInstanceSpec(instanceSpec) if err != nil { return fmt.Errorf("failed computing bastion hash from instance spec: %w", err) @@ -386,14 +393,19 @@ func reconcileBastion(scope scope.Scope, cluster *clusterv1.Cluster, openStackCl return nil } -func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, clusterName string) *compute.InstanceSpec { +func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, clusterName string, computeService *compute.Service) (*compute.InstanceSpec, error) { name := fmt.Sprintf("%s-bastion", clusterName) + + imageID, err := computeService.GetImageID(openStackCluster.Spec.Bastion.Instance.Image) + if err != nil { + return nil, fmt.Errorf("failed to get image ID for bastion: %w", err) + } + instanceSpec := &compute.InstanceSpec{ Name: name, Flavor: openStackCluster.Spec.Bastion.Instance.Flavor, SSHKeyName: openStackCluster.Spec.Bastion.Instance.SSHKeyName, - Image: openStackCluster.Spec.Bastion.Instance.Image, - ImageUUID: openStackCluster.Spec.Bastion.Instance.ImageUUID, + ImageID: imageID, FailureDomain: openStackCluster.Spec.Bastion.AvailabilityZone, RootVolume: openStackCluster.Spec.Bastion.Instance.RootVolume, } @@ -409,7 +421,7 @@ func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, clusterNa instanceSpec.Ports = openStackCluster.Spec.Bastion.Instance.Ports - return instanceSpec + return instanceSpec, nil } // bastionHashHasChanged returns a boolean whether if the latest bastion hash, built from the instance spec, has changed or not. diff --git a/controllers/openstackmachine_controller.go b/controllers/openstackmachine_controller.go index c797e218b9..4ea0cccd3c 100644 --- a/controllers/openstackmachine_controller.go +++ b/controllers/openstackmachine_controller.go @@ -466,8 +466,7 @@ func (r *OpenStackMachineReconciler) getOrCreate(logger logr.Logger, cluster *cl func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine *clusterv1.Machine, openStackMachine *infrav1.OpenStackMachine, userData string) *compute.InstanceSpec { instanceSpec := compute.InstanceSpec{ Name: openStackMachine.Name, - Image: openStackMachine.Spec.Image, - ImageUUID: openStackMachine.Spec.ImageUUID, + ImageID: openStackMachine.Status.ReferencedResources.ImageID, Flavor: openStackMachine.Spec.Flavor, SSHKeyName: openStackMachine.Spec.SSHKeyName, UserData: userData, diff --git a/controllers/openstackmachine_controller_test.go b/controllers/openstackmachine_controller_test.go index cfdeed7bcf..bc0c3b96c5 100644 --- a/controllers/openstackmachine_controller_test.go +++ b/controllers/openstackmachine_controller_test.go @@ -35,10 +35,10 @@ const ( controlPlaneSecurityGroupUUID = "c9817a91-4821-42db-8367-2301002ab659" workerSecurityGroupUUID = "9c6c0d28-03c9-436c-815d-58440ac2c1c8" serverGroupUUID = "7b940d62-68ef-4e42-a76a-1a62e290509c" + imageUUID = "ce96e584-7ebc-46d6-9e55-987d72e3806c" openStackMachineName = "test-openstack-machine" namespace = "test-namespace" - imageName = "test-image" flavorName = "test-flavor" sshKeyName = "test-ssh-key" failureDomain = "test-failure-domain" @@ -83,7 +83,7 @@ func getDefaultOpenStackMachine() *infrav1.OpenStackMachine { // TODO: Test Networks, Ports, Subnet, and Trunk separately CloudName: "test-cloud", Flavor: flavorName, - Image: imageName, + Image: infrav1.ImageFilter{ID: imageUUID}, SSHKeyName: sshKeyName, Tags: []string{"test-tag"}, ServerMetadata: map[string]string{ @@ -95,6 +95,7 @@ func getDefaultOpenStackMachine() *infrav1.OpenStackMachine { }, Status: infrav1.OpenStackMachineStatus{ ReferencedResources: infrav1.ReferencedMachineResources{ + ImageID: imageUUID, ServerGroupID: serverGroupUUID, }, }, @@ -104,7 +105,7 @@ func getDefaultOpenStackMachine() *infrav1.OpenStackMachine { func getDefaultInstanceSpec() *compute.InstanceSpec { return &compute.InstanceSpec{ Name: openStackMachineName, - Image: imageName, + ImageID: imageUUID, Flavor: flavorName, SSHKeyName: sshKeyName, UserData: "user-data", diff --git a/docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md b/docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md index ed8558bbfe..1183805d26 100644 --- a/docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md +++ b/docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md @@ -9,6 +9,8 @@ - [Change to `serverGroupID`](#change-to-servergroupid) - [`OpenStackCluster`](#openstackcluster) - [Change to externalNetworkID](#change-to-externalnetworkid) + - [Changes to image](#change-to-image) + - [Removal of imageUUID](#removal-of-imageuuid) @@ -88,3 +90,36 @@ It is now possible for a user to specify that no external network should be used ```yaml disableExternalNetwork: true ``` + +#### ⚠️ Change to image + +The field `image` is now an `ImageFilter` object rather than a string name. +The `ImageFilter` object allows selection of an image by name, by ID or by tags. + +```yaml +image: "test-image" +``` + +becomes + +```yaml +image: + name: "test-image" +``` + +The image ID will be added to `OpenStackMachine.Status.ReferencedResources.ImageID`. If the image can't be found or filter matches multiple images, an error will be returned. + +#### ⚠️ Removal of imageUUID + +The fild `imageUUID` has been removed in favor of the `image` field. + +```yaml +imageUUID: "72a6a1e6-3e0a-4a8b-9b4c-2d6f9e3e5f0a" +``` + +becomes + +```yaml +image: + id: "72a6a1e6-3e0a-4a8b-9b4c-2d6f9e3e5f0a" +``` diff --git a/kustomize/v1alpha8/default/cluster-template.yaml b/kustomize/v1alpha8/default/cluster-template.yaml index 5c0d1113d6..0dc7472f18 100644 --- a/kustomize/v1alpha8/default/cluster-template.yaml +++ b/kustomize/v1alpha8/default/cluster-template.yaml @@ -77,7 +77,8 @@ spec: template: spec: flavor: ${OPENSTACK_CONTROL_PLANE_MACHINE_FLAVOR} - image: ${OPENSTACK_IMAGE_NAME} + image: + name: ${OPENSTACK_IMAGE_NAME} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} cloudName: ${OPENSTACK_CLOUD} identityRef: @@ -120,7 +121,8 @@ spec: name: ${CLUSTER_NAME}-cloud-config kind: Secret flavor: ${OPENSTACK_NODE_MACHINE_FLAVOR} - image: ${OPENSTACK_IMAGE_NAME} + image: + name: ${OPENSTACK_IMAGE_NAME} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} --- apiVersion: bootstrap.cluster.x-k8s.io/v1beta1 diff --git a/kustomize/v1alpha8/flatcar-sysext/patch-flatcar.yaml b/kustomize/v1alpha8/flatcar-sysext/patch-flatcar.yaml index f1616ac759..e66245f777 100644 --- a/kustomize/v1alpha8/flatcar-sysext/patch-flatcar.yaml +++ b/kustomize/v1alpha8/flatcar-sysext/patch-flatcar.yaml @@ -173,7 +173,8 @@ metadata: spec: template: spec: - image: ${FLATCAR_IMAGE_NAME} + image: + name: ${FLATCAR_IMAGE_NAME} --- apiVersion: infrastructure.cluster.x-k8s.io/v1alpha8 kind: OpenStackMachineTemplate @@ -182,4 +183,5 @@ metadata: spec: template: spec: - image: ${FLATCAR_IMAGE_NAME} + image: + name: ${FLATCAR_IMAGE_NAME} diff --git a/kustomize/v1alpha8/flatcar/patch-flatcar.yaml b/kustomize/v1alpha8/flatcar/patch-flatcar.yaml index d3750014fb..4fb1c3a8d1 100644 --- a/kustomize/v1alpha8/flatcar/patch-flatcar.yaml +++ b/kustomize/v1alpha8/flatcar/patch-flatcar.yaml @@ -97,7 +97,8 @@ metadata: spec: template: spec: - image: ${OPENSTACK_FLATCAR_IMAGE_NAME} + image: + name: ${OPENSTACK_FLATCAR_IMAGE_NAME} --- apiVersion: infrastructure.cluster.x-k8s.io/v1alpha8 kind: OpenStackMachineTemplate @@ -106,4 +107,5 @@ metadata: spec: template: spec: - image: ${OPENSTACK_FLATCAR_IMAGE_NAME} + image: + name: ${OPENSTACK_FLATCAR_IMAGE_NAME} diff --git a/pkg/cloud/services/compute/instance.go b/pkg/cloud/services/compute/instance.go index 15e9756895..979e370acf 100644 --- a/pkg/cloud/services/compute/instance.go +++ b/pkg/cloud/services/compute/instance.go @@ -30,7 +30,6 @@ import ( "github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/schedulerhints" "github.com/gophercloud/gophercloud/openstack/compute/v2/flavors" "github.com/gophercloud/gophercloud/openstack/compute/v2/servers" - "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" "github.com/gophercloud/gophercloud/openstack/networking/v2/ports" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/wait" @@ -238,11 +237,6 @@ func (s *Service) createInstanceImpl(eventObject runtime.Object, openStackCluste var server *clients.ServerExt portList := []servers.Network{} - imageID, err := s.getImageID(instanceSpec.ImageUUID, instanceSpec.Image) - if err != nil { - return nil, fmt.Errorf("error getting image ID: %v", err) - } - flavor, err := s.getAndValidateFlavor(instanceSpec.Flavor) if err != nil { return nil, err @@ -297,7 +291,7 @@ func (s *Service) createInstanceImpl(eventObject runtime.Object, openStackCluste // Don't set ImageRef on the server if we're booting from volume var serverImageRef string if !hasRootVolume(instanceSpec) { - serverImageRef = imageID + serverImageRef = instanceSpec.ImageID } var serverCreateOpts servers.CreateOptsBuilder = servers.CreateOpts{ @@ -312,7 +306,7 @@ func (s *Service) createInstanceImpl(eventObject runtime.Object, openStackCluste ConfigDrive: &instanceSpec.ConfigDrive, } - blockDevices, err := s.getBlockDevices(eventObject, instanceSpec, imageID, instanceCreateTimeout, retryInterval) + blockDevices, err := s.getBlockDevices(eventObject, instanceSpec, instanceSpec.ImageID, instanceCreateTimeout, retryInterval) if err != nil { return nil, err } @@ -562,40 +556,28 @@ func applyServerGroupID(opts servers.CreateOptsBuilder, serverGroupID string) se return opts } -// Helper function for getting image id from name. -func (s *Service) getImageIDFromName(imageName string) (string, error) { - var opts images.ListOpts - - opts.Name = imageName +// Helper function for getting image ID from name, ID, or tags. +func (s *Service) GetImageID(image infrav1.ImageFilter) (string, error) { + if image.ID != "" { + return image.ID, nil + } - allImages, err := s.getImageClient().ListImages(opts) + allImages, err := s.getImageClient().ListImages(image.ToListOpt()) if err != nil { return "", err } switch len(allImages) { case 0: - return "", fmt.Errorf("no image with the Name %s could be found", imageName) + return "", fmt.Errorf("no images were found with the given image filter: %v", image) case 1: return allImages[0].ID, nil default: // this should never happen - return "", fmt.Errorf("too many images with the name, %s, were found", imageName) + return "", fmt.Errorf("too many images were found with the given image filter: %v", image) } } -// Helper function for getting image ID from name or ID. -func (s *Service) getImageID(imageUUID, imageName string) (string, error) { - if imageUUID != "" { - // we return imageUUID without check - return imageUUID, nil - } else if imageName != "" { - return s.getImageIDFromName(imageName) - } - - return "", nil -} - // GetManagementPort returns the port which is used for management and external // traffic. Cluster floating IPs must be associated with this port. func (s *Service) GetManagementPort(openStackCluster *infrav1.OpenStackCluster, instanceStatus *InstanceStatus) (*ports.Port, error) { diff --git a/pkg/cloud/services/compute/instance_test.go b/pkg/cloud/services/compute/instance_test.go index 814fd807a9..605099a3bc 100644 --- a/pkg/cloud/services/compute/instance_test.go +++ b/pkg/cloud/services/compute/instance_test.go @@ -119,51 +119,51 @@ func Test_getPortName(t *testing.T) { } func TestService_getImageID(t *testing.T) { - const imageIDA = "ce96e584-7ebc-46d6-9e55-987d72e3806c" - const imageIDB = "8f536889-5198-42d7-8314-cb78f4f4755c" - const imageIDC = "8f536889-5198-42d7-8314-cb78f4f4755d" + const imageID = "ce96e584-7ebc-46d6-9e55-987d72e3806c" + const imageName = "test-image" + imageTags := []string{"test-tag"} tests := []struct { - testName string - imageUUID string - imageName string - expect func(m *mock.MockImageClientMockRecorder) - want string - wantErr bool + testName string + image infrav1.ImageFilter + expect func(m *mock.MockImageClientMockRecorder) + want string + wantErr bool }{ { - testName: "Return image uuid if uuid given", - imageUUID: imageIDC, - want: imageIDC, - expect: func(m *mock.MockImageClientMockRecorder) { - }, - wantErr: false, + testName: "Return image ID when ID given", + image: infrav1.ImageFilter{ID: imageID}, + want: imageID, + expect: func(m *mock.MockImageClientMockRecorder) {}, + wantErr: false, }, { - testName: "Return through uuid if both uuid and name given", - imageName: "dummy", - imageUUID: imageIDC, + testName: "Return image ID when name given", + image: infrav1.ImageFilter{Name: imageName}, + want: imageID, expect: func(m *mock.MockImageClientMockRecorder) { + m.ListImages(images.ListOpts{Name: imageName}).Return( + []images.Image{{ID: imageID, Name: imageName}}, + nil) }, - want: imageIDC, wantErr: false, }, { - testName: "Return image ID", - imageName: "test-image", + testName: "Return image ID when tags given", + image: infrav1.ImageFilter{Tags: imageTags}, + want: imageID, expect: func(m *mock.MockImageClientMockRecorder) { - m.ListImages(images.ListOpts{Name: "test-image"}).Return( - []images.Image{{ID: imageIDA, Name: "test-image"}}, + m.ListImages(images.ListOpts{Tags: imageTags}).Return( + []images.Image{{ID: imageID, Name: imageName, Tags: imageTags}}, nil) }, - want: imageIDA, wantErr: false, }, { - testName: "Return no results", - imageName: "test-image", + testName: "Return no results", + image: infrav1.ImageFilter{Name: imageName}, expect: func(m *mock.MockImageClientMockRecorder) { - m.ListImages(images.ListOpts{Name: "test-image"}).Return( + m.ListImages(images.ListOpts{Name: imageName}).Return( []images.Image{}, nil) }, @@ -171,21 +171,21 @@ func TestService_getImageID(t *testing.T) { wantErr: true, }, { - testName: "Return multiple results", - imageName: "test-image", + testName: "Return multiple results", + image: infrav1.ImageFilter{Name: imageName}, expect: func(m *mock.MockImageClientMockRecorder) { m.ListImages(images.ListOpts{Name: "test-image"}).Return( []images.Image{ - {ID: imageIDA, Name: "test-image"}, - {ID: imageIDB, Name: "test-image"}, + {ID: imageID, Name: "test-image"}, + {ID: "123", Name: "test-image"}, }, nil) }, want: "", wantErr: true, }, { - testName: "OpenStack returns error", - imageName: "test-image", + testName: "OpenStack returns error", + image: infrav1.ImageFilter{Name: imageName}, expect: func(m *mock.MockImageClientMockRecorder) { m.ListImages(images.ListOpts{Name: "test-image"}).Return( nil, @@ -206,7 +206,7 @@ func TestService_getImageID(t *testing.T) { } tt.expect(mockScopeFactory.ImageClient.EXPECT()) - got, err := s.getImageID(tt.imageUUID, tt.imageName) + got, err := s.GetImageID(tt.image) if (err != nil) != tt.wantErr { t.Errorf("Service.getImageID() error = %v, wantErr %v", err, tt.wantErr) return @@ -261,7 +261,7 @@ func getDefaultOpenStackCluster() *infrav1.OpenStackCluster { func getDefaultInstanceSpec() *InstanceSpec { return &InstanceSpec{ Name: openStackMachineName, - Image: imageName, + ImageID: imageUUID, Flavor: flavorName, SSHKeyName: sshKeyName, UserData: "user-data", @@ -375,9 +375,8 @@ func TestService_ReconcileInstance(t *testing.T) { networkRecorder.DeletePort(portUUID).Return(nil) } - // Expected calls when using default image and flavor - expectDefaultImageAndFlavor := func(computeRecorder *mock.MockComputeClientMockRecorder, imageRecorder *mock.MockImageClientMockRecorder) { - imageRecorder.ListImages(images.ListOpts{Name: imageName}).Return([]images.Image{{ID: imageUUID}}, nil) + // Expected calls when using default flavor + expectDefaultFlavor := func(computeRecorder *mock.MockComputeClientMockRecorder) { f := flavors.Flavor{ ID: flavorUUID, VCPUs: 2, @@ -457,7 +456,7 @@ func TestService_ReconcileInstance(t *testing.T) { getInstanceSpec: getDefaultInstanceSpec, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) - expectDefaultImageAndFlavor(r.compute, r.image) + expectDefaultFlavor(r.compute) expectCreateServer(r.compute, getDefaultServerMap(), false) expectServerPollSuccess(r.compute) @@ -469,7 +468,7 @@ func TestService_ReconcileInstance(t *testing.T) { getInstanceSpec: getDefaultInstanceSpec, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) - expectDefaultImageAndFlavor(r.compute, r.image) + expectDefaultFlavor(r.compute) expectCreateServer(r.compute, getDefaultServerMap(), true) @@ -489,7 +488,7 @@ func TestService_ReconcileInstance(t *testing.T) { return s }, expect: func(r *recorders) { - expectDefaultImageAndFlavor(r.compute, r.image) + expectDefaultFlavor(r.compute) expectUseExistingDefaultPort(r.network) // Looking up the second port fails @@ -508,7 +507,7 @@ func TestService_ReconcileInstance(t *testing.T) { getInstanceSpec: getDefaultInstanceSpec, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) - expectDefaultImageAndFlavor(r.compute, r.image) + expectDefaultFlavor(r.compute) expectCreateServer(r.compute, getDefaultServerMap(), false) expectServerPoll(r.compute, []string{"BUILDING", "ACTIVE"}) @@ -520,7 +519,7 @@ func TestService_ReconcileInstance(t *testing.T) { getInstanceSpec: getDefaultInstanceSpec, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) - expectDefaultImageAndFlavor(r.compute, r.image) + expectDefaultFlavor(r.compute) expectCreateServer(r.compute, getDefaultServerMap(), false) expectServerPoll(r.compute, []string{"BUILDING", "ERROR"}) @@ -540,7 +539,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) - expectDefaultImageAndFlavor(r.compute, r.image) + expectDefaultFlavor(r.compute) r.volume.ListVolumes(volumes.ListOpts{Name: fmt.Sprintf("%s-root", openStackMachineName)}). Return([]volumes.Volume{}, nil) @@ -586,7 +585,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) - expectDefaultImageAndFlavor(r.compute, r.image) + expectDefaultFlavor(r.compute) r.volume.ListVolumes(volumes.ListOpts{Name: fmt.Sprintf("%s-root", openStackMachineName)}). Return([]volumes.Volume{}, nil) @@ -631,7 +630,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) - expectDefaultImageAndFlavor(r.compute, r.image) + expectDefaultFlavor(r.compute) r.volume.ListVolumes(volumes.ListOpts{Name: fmt.Sprintf("%s-root", openStackMachineName)}). Return([]volumes.Volume{}, nil) @@ -679,7 +678,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) - expectDefaultImageAndFlavor(r.compute, r.image) + expectDefaultFlavor(r.compute) r.volume.ListVolumes(volumes.ListOpts{Name: fmt.Sprintf("%s-root", openStackMachineName)}). Return([]volumes.Volume{}, nil) @@ -767,7 +766,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) - expectDefaultImageAndFlavor(r.compute, r.image) + expectDefaultFlavor(r.compute) r.volume.ListVolumes(volumes.ListOpts{Name: fmt.Sprintf("%s-etcd", openStackMachineName)}). Return([]volumes.Volume{}, nil) @@ -836,7 +835,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) - expectDefaultImageAndFlavor(r.compute, r.image) + expectDefaultFlavor(r.compute) r.volume.ListVolumes(volumes.ListOpts{Name: fmt.Sprintf("%s-etcd", openStackMachineName)}). Return([]volumes.Volume{}, nil) @@ -893,7 +892,7 @@ func TestService_ReconcileInstance(t *testing.T) { }, expect: func(r *recorders) { expectUseExistingDefaultPort(r.network) - expectDefaultImageAndFlavor(r.compute, r.image) + expectDefaultFlavor(r.compute) // Make sure we delete ports expectCleanupDefaultPort(r.network) @@ -911,7 +910,7 @@ func TestService_ReconcileInstance(t *testing.T) { return s }, expect: func(r *recorders) { - expectDefaultImageAndFlavor(r.compute, r.image) + expectDefaultFlavor(r.compute) extensions := []extensions.Extension{ {Extension: common.Extension{Alias: "trunk"}}, } diff --git a/pkg/cloud/services/compute/instance_types.go b/pkg/cloud/services/compute/instance_types.go index d7b5634605..4170bc143e 100644 --- a/pkg/cloud/services/compute/instance_types.go +++ b/pkg/cloud/services/compute/instance_types.go @@ -34,8 +34,7 @@ import ( // all of them can be set on a new instance. type InstanceSpec struct { Name string - Image string - ImageUUID string + ImageID string Flavor string SSHKeyName string UserData string diff --git a/pkg/cloud/services/compute/referenced_resources.go b/pkg/cloud/services/compute/referenced_resources.go index 7b80947480..b9350f8d06 100644 --- a/pkg/cloud/services/compute/referenced_resources.go +++ b/pkg/cloud/services/compute/referenced_resources.go @@ -30,6 +30,7 @@ func ResolveReferencedMachineResources(scope scope.Scope, spec *infrav1.OpenStac return err } + // ServerGroup is optional, so we only need to resolve it if it's set in the spec and not in ReferencedMachineResources yet. if spec.ServerGroup != nil && resources.ServerGroupID == "" { serverGroupID, err := compute.GetServerGroupID(spec.ServerGroup) if err != nil { @@ -38,5 +39,14 @@ func ResolveReferencedMachineResources(scope scope.Scope, spec *infrav1.OpenStac resources.ServerGroupID = serverGroupID } + // Image is required, so we need to resolve it if it's not set in ReferencedMachineResources yet. + if resources.ImageID == "" { + imageID, err := compute.GetImageID(spec.Image) + if err != nil { + return err + } + resources.ImageID = imageID + } + return nil } diff --git a/pkg/cloud/services/compute/referenced_resources_test.go b/pkg/cloud/services/compute/referenced_resources_test.go index 97e606ee71..6a976325c4 100644 --- a/pkg/cloud/services/compute/referenced_resources_test.go +++ b/pkg/cloud/services/compute/referenced_resources_test.go @@ -23,6 +23,7 @@ import ( "github.com/golang/mock/gomock" "github.com/google/go-cmp/cmp" "github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/servergroups" + "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" . "github.com/onsi/gomega" infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha8" @@ -32,43 +33,67 @@ import ( func Test_ResolveReferencedMachineResources(t *testing.T) { const serverGroupID1 = "ce96e584-7ebc-46d6-9e55-987d72e3806c" + const imageID1 = "de96e584-7ebc-46d6-9e55-987d72e3806c" + + minimumReferences := &infrav1.ReferencedMachineResources{ + ImageID: imageID1, + } tests := []struct { testName string serverGroupFilter *infrav1.ServerGroupFilter - expect func(m *mock.MockComputeClientMockRecorder) + imageFilter *infrav1.ImageFilter + expectComputeMock func(m *mock.MockComputeClientMockRecorder) + expectImageMock func(m *mock.MockImageClientMockRecorder) want *infrav1.ReferencedMachineResources wantErr bool }{ { - testName: "Server group ID passed", + testName: "Resources ID passed", serverGroupFilter: &infrav1.ServerGroupFilter{ID: serverGroupID1}, - expect: func(m *mock.MockComputeClientMockRecorder) {}, - want: &infrav1.ReferencedMachineResources{ServerGroupID: serverGroupID1}, + imageFilter: &infrav1.ImageFilter{ID: imageID1}, + expectComputeMock: func(m *mock.MockComputeClientMockRecorder) {}, + expectImageMock: func(m *mock.MockImageClientMockRecorder) {}, + want: &infrav1.ReferencedMachineResources{ImageID: imageID1, ServerGroupID: serverGroupID1}, wantErr: false, }, { testName: "Server group filter nil", serverGroupFilter: nil, - expect: func(m *mock.MockComputeClientMockRecorder) {}, - want: &infrav1.ReferencedMachineResources{}, + expectComputeMock: func(m *mock.MockComputeClientMockRecorder) {}, + expectImageMock: func(m *mock.MockImageClientMockRecorder) {}, + want: minimumReferences, wantErr: false, }, { testName: "Server group ID empty", serverGroupFilter: &infrav1.ServerGroupFilter{}, - expect: func(m *mock.MockComputeClientMockRecorder) {}, - want: &infrav1.ReferencedMachineResources{}, + expectComputeMock: func(m *mock.MockComputeClientMockRecorder) {}, + expectImageMock: func(m *mock.MockImageClientMockRecorder) {}, + want: minimumReferences, wantErr: false, }, { testName: "Server group by Name not found", serverGroupFilter: &infrav1.ServerGroupFilter{Name: "test-server-group"}, - expect: func(m *mock.MockComputeClientMockRecorder) { + expectComputeMock: func(m *mock.MockComputeClientMockRecorder) { m.ListServerGroups().Return( []servergroups.ServerGroup{}, nil) }, + expectImageMock: func(m *mock.MockImageClientMockRecorder) {}, + want: &infrav1.ReferencedMachineResources{}, + wantErr: true, + }, + { + testName: "Image by Name not found", + imageFilter: &infrav1.ImageFilter{Name: "test-image"}, + expectComputeMock: func(m *mock.MockComputeClientMockRecorder) {}, + expectImageMock: func(m *mock.MockImageClientMockRecorder) { + m.ListImages(images.ListOpts{Name: "test-image"}).Return( + []images.Image{}, + nil) + }, want: &infrav1.ReferencedMachineResources{}, wantErr: true, }, @@ -79,10 +104,18 @@ func Test_ResolveReferencedMachineResources(t *testing.T) { mockCtrl := gomock.NewController(t) mockScopeFactory := scope.NewMockScopeFactory(mockCtrl, "", logr.Discard()) - tt.expect(mockScopeFactory.ComputeClient.EXPECT()) + tt.expectComputeMock(mockScopeFactory.ComputeClient.EXPECT()) + tt.expectImageMock(mockScopeFactory.ImageClient.EXPECT()) + + // Set defaults for required fields + imageFilter := &infrav1.ImageFilter{ID: imageID1} + if tt.imageFilter != nil { + imageFilter = tt.imageFilter + } machineSpec := &infrav1.OpenStackMachineSpec{ ServerGroup: tt.serverGroupFilter, + Image: *imageFilter, } resources := &infrav1.ReferencedMachineResources{} diff --git a/templates/cluster-template-flatcar-sysext.yaml b/templates/cluster-template-flatcar-sysext.yaml index 1aefb0aaff..3994a67311 100644 --- a/templates/cluster-template-flatcar-sysext.yaml +++ b/templates/cluster-template-flatcar-sysext.yaml @@ -247,7 +247,8 @@ spec: identityRef: kind: Secret name: ${CLUSTER_NAME}-cloud-config - image: ${FLATCAR_IMAGE_NAME} + image: + name: ${FLATCAR_IMAGE_NAME} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} --- apiVersion: infrastructure.cluster.x-k8s.io/v1alpha8 @@ -262,5 +263,6 @@ spec: identityRef: kind: Secret name: ${CLUSTER_NAME}-cloud-config - image: ${FLATCAR_IMAGE_NAME} + image: + name: ${FLATCAR_IMAGE_NAME} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} diff --git a/templates/cluster-template-flatcar.yaml b/templates/cluster-template-flatcar.yaml index 29b3443f0a..ff093ece03 100644 --- a/templates/cluster-template-flatcar.yaml +++ b/templates/cluster-template-flatcar.yaml @@ -171,7 +171,8 @@ spec: identityRef: kind: Secret name: ${CLUSTER_NAME}-cloud-config - image: ${OPENSTACK_FLATCAR_IMAGE_NAME} + image: + name: ${OPENSTACK_FLATCAR_IMAGE_NAME} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} --- apiVersion: infrastructure.cluster.x-k8s.io/v1alpha8 @@ -186,5 +187,6 @@ spec: identityRef: kind: Secret name: ${CLUSTER_NAME}-cloud-config - image: ${OPENSTACK_FLATCAR_IMAGE_NAME} + image: + name: ${OPENSTACK_FLATCAR_IMAGE_NAME} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} diff --git a/templates/cluster-template-without-lb.yaml b/templates/cluster-template-without-lb.yaml index a9e6755bf1..665437a0a1 100644 --- a/templates/cluster-template-without-lb.yaml +++ b/templates/cluster-template-without-lb.yaml @@ -128,7 +128,8 @@ spec: identityRef: kind: Secret name: ${CLUSTER_NAME}-cloud-config - image: ${OPENSTACK_IMAGE_NAME} + image: + name: ${OPENSTACK_IMAGE_NAME} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} --- apiVersion: infrastructure.cluster.x-k8s.io/v1alpha8 @@ -143,5 +144,6 @@ spec: identityRef: kind: Secret name: ${CLUSTER_NAME}-cloud-config - image: ${OPENSTACK_IMAGE_NAME} + image: + name: ${OPENSTACK_IMAGE_NAME} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} diff --git a/templates/cluster-template.yaml b/templates/cluster-template.yaml index 6760b7e917..af65b8865f 100644 --- a/templates/cluster-template.yaml +++ b/templates/cluster-template.yaml @@ -130,7 +130,8 @@ spec: identityRef: kind: Secret name: ${CLUSTER_NAME}-cloud-config - image: ${OPENSTACK_IMAGE_NAME} + image: + name: ${OPENSTACK_IMAGE_NAME} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} --- apiVersion: infrastructure.cluster.x-k8s.io/v1alpha8 @@ -145,5 +146,6 @@ spec: identityRef: kind: Secret name: ${CLUSTER_NAME}-cloud-config - image: ${OPENSTACK_IMAGE_NAME} + image: + name: ${OPENSTACK_IMAGE_NAME} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} diff --git a/test/e2e/data/kustomize/common-patches/cni/patch-cluster.yaml b/test/e2e/data/kustomize/common-patches/cni/patch-cluster.yaml index 5399786f2f..26f94a6d4a 100644 --- a/test/e2e/data/kustomize/common-patches/cni/patch-cluster.yaml +++ b/test/e2e/data/kustomize/common-patches/cni/patch-cluster.yaml @@ -5,7 +5,8 @@ enabled: true instance: flavor: ${OPENSTACK_BASTION_MACHINE_FLAVOR} - image: ${OPENSTACK_BASTION_IMAGE_NAME} + image: + name: ${OPENSTACK_BASTION_IMAGE_NAME} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} - op: add path: /spec/controlPlaneAvailabilityZones diff --git a/test/e2e/data/kustomize/k8s-upgrade/upgrade-from-template.yaml b/test/e2e/data/kustomize/k8s-upgrade/upgrade-from-template.yaml index 4bba2d5165..339b52535a 100644 --- a/test/e2e/data/kustomize/k8s-upgrade/upgrade-from-template.yaml +++ b/test/e2e/data/kustomize/k8s-upgrade/upgrade-from-template.yaml @@ -11,7 +11,8 @@ spec: identityRef: kind: Secret name: ${CLUSTER_NAME}-cloud-config - image: ${OPENSTACK_IMAGE_NAME_UPGRADE_FROM} + image: + name: ${OPENSTACK_IMAGE_NAME_UPGRADE_FROM} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} tags: - control-plane @@ -28,7 +29,8 @@ spec: identityRef: kind: Secret name: ${CLUSTER_NAME}-cloud-config - image: ${OPENSTACK_IMAGE_NAME_UPGRADE_FROM} + image: + name: ${OPENSTACK_IMAGE_NAME_UPGRADE_FROM} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} tags: - machine diff --git a/test/e2e/data/kustomize/k8s-upgrade/upgrade-to-template.yaml b/test/e2e/data/kustomize/k8s-upgrade/upgrade-to-template.yaml index 035aad375a..6e426d14a5 100644 --- a/test/e2e/data/kustomize/k8s-upgrade/upgrade-to-template.yaml +++ b/test/e2e/data/kustomize/k8s-upgrade/upgrade-to-template.yaml @@ -18,7 +18,8 @@ spec: identityRef: kind: Secret name: ${CLUSTER_NAME}-cloud-config - image: ${OPENSTACK_IMAGE_NAME} + image: + name: ${OPENSTACK_IMAGE_NAME} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} tags: - control-plane @@ -37,7 +38,8 @@ spec: identityRef: kind: Secret name: ${CLUSTER_NAME}-cloud-config - image: ${OPENSTACK_IMAGE_NAME} + image: + name: ${OPENSTACK_IMAGE_NAME} sshKeyName: ${OPENSTACK_SSH_KEY_NAME} tags: - machine diff --git a/test/e2e/data/kustomize/v1alpha5/bastion.yaml b/test/e2e/data/kustomize/v1alpha5/bastion.yaml new file mode 100644 index 0000000000..45c9e0f508 --- /dev/null +++ b/test/e2e/data/kustomize/v1alpha5/bastion.yaml @@ -0,0 +1,9 @@ +--- +- op: add + path: /spec/bastion + value: + enabled: true + instance: + flavor: ${OPENSTACK_BASTION_MACHINE_FLAVOR} + image: ${OPENSTACK_BASTION_IMAGE_NAME} + sshKeyName: ${OPENSTACK_SSH_KEY_NAME} diff --git a/test/e2e/data/kustomize/v1alpha5/kustomization.yaml b/test/e2e/data/kustomize/v1alpha5/kustomization.yaml index d11eead116..ae29db64d9 100644 --- a/test/e2e/data/kustomize/v1alpha5/kustomization.yaml +++ b/test/e2e/data/kustomize/v1alpha5/kustomization.yaml @@ -6,3 +6,9 @@ resources: components: - ../common-patches/cni - ../common-patches/ccm + +patches: +- path: bastion.yaml + target: + kind: OpenStackCluster + name: \${CLUSTER_NAME} diff --git a/test/e2e/data/kustomize/v1alpha6/bastion.yaml b/test/e2e/data/kustomize/v1alpha6/bastion.yaml new file mode 100644 index 0000000000..45c9e0f508 --- /dev/null +++ b/test/e2e/data/kustomize/v1alpha6/bastion.yaml @@ -0,0 +1,9 @@ +--- +- op: add + path: /spec/bastion + value: + enabled: true + instance: + flavor: ${OPENSTACK_BASTION_MACHINE_FLAVOR} + image: ${OPENSTACK_BASTION_IMAGE_NAME} + sshKeyName: ${OPENSTACK_SSH_KEY_NAME} diff --git a/test/e2e/data/kustomize/v1alpha6/kustomization.yaml b/test/e2e/data/kustomize/v1alpha6/kustomization.yaml index f455c1d60c..aa3e9c0e5c 100644 --- a/test/e2e/data/kustomize/v1alpha6/kustomization.yaml +++ b/test/e2e/data/kustomize/v1alpha6/kustomization.yaml @@ -20,3 +20,7 @@ patches: - {} target: kind: OpenStackMachineTemplate +- path: bastion.yaml + target: + kind: OpenStackCluster + name: \${CLUSTER_NAME} diff --git a/test/e2e/data/kustomize/v1alpha7/bastion.yaml b/test/e2e/data/kustomize/v1alpha7/bastion.yaml new file mode 100644 index 0000000000..45c9e0f508 --- /dev/null +++ b/test/e2e/data/kustomize/v1alpha7/bastion.yaml @@ -0,0 +1,9 @@ +--- +- op: add + path: /spec/bastion + value: + enabled: true + instance: + flavor: ${OPENSTACK_BASTION_MACHINE_FLAVOR} + image: ${OPENSTACK_BASTION_IMAGE_NAME} + sshKeyName: ${OPENSTACK_SSH_KEY_NAME} diff --git a/test/e2e/data/kustomize/v1alpha7/kustomization.yaml b/test/e2e/data/kustomize/v1alpha7/kustomization.yaml index 3ad33a43b0..fe53c35f3f 100644 --- a/test/e2e/data/kustomize/v1alpha7/kustomization.yaml +++ b/test/e2e/data/kustomize/v1alpha7/kustomization.yaml @@ -20,3 +20,7 @@ patches: - {} target: kind: OpenStackMachineTemplate +- path: bastion.yaml + target: + kind: OpenStackCluster + name: \${CLUSTER_NAME} diff --git a/test/e2e/suites/e2e/e2e_test.go b/test/e2e/suites/e2e/e2e_test.go index 0086a22e23..ddf3b5f3c5 100644 --- a/test/e2e/suites/e2e/e2e_test.go +++ b/test/e2e/suites/e2e/e2e_test.go @@ -769,8 +769,10 @@ func makeOpenStackMachineTemplate(namespace, clusterName, name string) *infrav1. Spec: infrav1.OpenStackMachineTemplateSpec{ Template: infrav1.OpenStackMachineTemplateResource{ Spec: infrav1.OpenStackMachineSpec{ - Flavor: e2eCtx.E2EConfig.GetVariable(shared.OpenStackNodeMachineFlavor), - Image: e2eCtx.E2EConfig.GetVariable(shared.OpenStackImageName), + Flavor: e2eCtx.E2EConfig.GetVariable(shared.OpenStackNodeMachineFlavor), + Image: infrav1.ImageFilter{ + Name: e2eCtx.E2EConfig.GetVariable(shared.OpenStackImageName), + }, SSHKeyName: shared.DefaultSSHKeyPairName, CloudName: e2eCtx.E2EConfig.GetVariable(shared.OpenStackCloud), IdentityRef: &infrav1.OpenStackIdentityReference{ @@ -792,8 +794,10 @@ func makeOpenStackMachineTemplateWithPortOptions(namespace, clusterName, name st Spec: infrav1.OpenStackMachineTemplateSpec{ Template: infrav1.OpenStackMachineTemplateResource{ Spec: infrav1.OpenStackMachineSpec{ - Flavor: e2eCtx.E2EConfig.GetVariable(shared.OpenStackNodeMachineFlavor), - Image: e2eCtx.E2EConfig.GetVariable(shared.OpenStackImageName), + Flavor: e2eCtx.E2EConfig.GetVariable(shared.OpenStackNodeMachineFlavor), + Image: infrav1.ImageFilter{ + Name: e2eCtx.E2EConfig.GetVariable(shared.OpenStackImageName), + }, SSHKeyName: shared.DefaultSSHKeyPairName, CloudName: e2eCtx.E2EConfig.GetVariable(shared.OpenStackCloud), IdentityRef: &infrav1.OpenStackIdentityReference{