ImageFilter - add validations, switch to pointers

This commit changes `ID` and `Name` of `ImageFilter` to pointers which
should only affect go marshalling.

Other than that it adds CEL validation of the ImageFilter, so that Name
or Tags can only be set when ID is unset. Conversions are updated
accordingly to make sure we only set Name when ID is unset.

Moreover validation is added that ID has to be UUID. It's not enforced
in conversions, as non-UUID IDs would produce clusters or machines that
would not work properly.

api/v1alpha5/conversion.go

@@ -358,11 +358,10 @@ func Convert_v1alpha5_OpenStackMachineSpec_To_v1beta1_OpenStackMachineSpec(in *O
 	}
 
 	imageFilter := infrav1.ImageFilter{}
-	if in.Image != "" {
-		imageFilter.Name = in.Image
-	}
 	if in.ImageUUID != "" {
-		imageFilter.ID = in.ImageUUID
+		imageFilter.ID = &in.ImageUUID
+	} else if in.Image != "" { // Only add name when ID is not set, in v1beta1 it's not possible to set both.
+		imageFilter.Name = &in.Image
 	}
 	out.Image = imageFilter
 
@@ -643,12 +642,12 @@ func Convert_v1beta1_OpenStackMachineSpec_To_v1alpha5_OpenStackMachineSpec(in *i
 		out.ServerGroupID = in.ServerGroup.ID
 	}
 
-	if in.Image.Name != "" {
-		out.Image = in.Image.Name
+	if in.Image.Name != nil && *in.Image.Name != "" {
+		out.Image = *in.Image.Name
 	}
 
-	if in.Image.ID != "" {
-		out.ImageUUID = in.Image.ID
+	if in.Image.ID != nil && *in.Image.ID != "" {
+		out.ImageUUID = *in.Image.ID
 	}
 
 	if in.IdentityRef != nil {

api/v1alpha6/openstackmachine_conversion.go

@@ -152,6 +152,11 @@ func restorev1alpha6MachineSpec(previous *OpenStackMachineSpec, dst *OpenStackMa
 			restorev1alpha6SecurityGroupFilter(&previous.SecurityGroups[i].Filter, &dst.SecurityGroups[i].Filter)
 		}
 	}
+
+	// Conversion to v1beta1 removes Image when ImageUUID is set
+	if dst.Image == "" && previous.Image != "" {
+		dst.Image = previous.Image
+	}
 }
 
 func restorev1beta1MachineSpec(previous *infrav1.OpenStackMachineSpec, dst *infrav1.OpenStackMachineSpec) {
@@ -258,11 +263,10 @@ func Convert_v1alpha6_OpenStackMachineSpec_To_v1beta1_OpenStackMachineSpec(in *O
 	}
 
 	imageFilter := infrav1.ImageFilter{}
-	if in.Image != "" {
-		imageFilter.Name = in.Image
-	}
 	if in.ImageUUID != "" {
-		imageFilter.ID = in.ImageUUID
+		imageFilter.ID = &in.ImageUUID
+	} else if in.Image != "" { // Only add name when ID is not set, in v1beta1 it's not possible to set both.
+		imageFilter.Name = &in.Image
 	}
 	out.Image = imageFilter
 
@@ -306,12 +310,12 @@ func Convert_v1beta1_OpenStackMachineSpec_To_v1alpha6_OpenStackMachineSpec(in *i
 		out.ServerGroupID = in.ServerGroup.ID
 	}
 
-	if in.Image.Name != "" {
-		out.Image = in.Image.Name
+	if in.Image.Name != nil && *in.Image.Name != "" {
+		out.Image = *in.Image.Name
 	}
 
-	if in.Image.ID != "" {
-		out.ImageUUID = in.Image.ID
+	if in.Image.ID != nil && *in.Image.ID != "" {
+		out.ImageUUID = *in.Image.ID
 	}
 
 	if len(in.ServerMetadata) > 0 {

api/v1alpha7/openstackmachine_conversion.go

@@ -126,6 +126,11 @@ func restorev1alpha7MachineSpec(previous *OpenStackMachineSpec, dst *OpenStackMa
 			restorev1alpha7SecurityGroupFilter(&previous.SecurityGroups[i], &dst.SecurityGroups[i])
 		}
 	}
+
+	// Conversion to v1beta1 removes Image when ImageUUID is set
+	if dst.Image == "" && previous.Image != "" {
+		dst.Image = previous.Image
+	}
 }
 
 func restorev1beta1MachineSpec(previous *infrav1.OpenStackMachineSpec, dst *infrav1.OpenStackMachineSpec) {
@@ -152,11 +157,10 @@ func Convert_v1alpha7_OpenStackMachineSpec_To_v1beta1_OpenStackMachineSpec(in *O
 	}
 
 	imageFilter := infrav1.ImageFilter{}
-	if in.Image != "" {
-		imageFilter.Name = in.Image
-	}
 	if in.ImageUUID != "" {
-		imageFilter.ID = in.ImageUUID
+		imageFilter.ID = &in.ImageUUID
+	} else if in.Image != "" { // Only add name when ID is not set, in v1beta1 it's not possible to set both.
+		imageFilter.Name = &in.Image
 	}
 	out.Image = imageFilter
 
@@ -197,12 +201,12 @@ func Convert_v1beta1_OpenStackMachineSpec_To_v1alpha7_OpenStackMachineSpec(in *i
 		out.ServerGroupID = in.ServerGroup.ID
 	}
 
-	if in.Image.Name != "" {
-		out.Image = in.Image.Name
+	if in.Image.Name != nil && *in.Image.Name != "" {
+		out.Image = *in.Image.Name
 	}
 
-	if in.Image.ID != "" {
-		out.ImageUUID = in.Image.ID
+	if in.Image.ID != nil && *in.Image.ID != "" {
+		out.ImageUUID = *in.Image.ID
 	}
 
 	if len(in.ServerMetadata) > 0 {

api/v1beta1/openstackmachine_types.go

@@ -43,7 +43,8 @@ type OpenStackMachineSpec struct {
 
 	// 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"`
+	// +required
+	Image ImageFilter `json:"image"`
 
 	// The ssh key to inject in the instance
 	SSHKeyName string `json:"sshKeyName,omitempty"`

api/v1beta1/types.go

@@ -28,12 +28,19 @@ type OpenStackMachineTemplateResource struct {
 	Spec OpenStackMachineSpec `json:"spec"`
 }
 
+// ImageFilter describes the data needed to identify which image to use. If ID is provided it is required that all other fields are unset.
+// +kubebuilder:validation:XValidation:rule="(has(self.id) && !has(self.name) && !has(self.tags)) || !has(self.id)",message="when ID is set you cannot set other options"
 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 ID of the desired image. If ID is provided, the other filters cannot be provided. Must be in UUID format.
+	// +kubebuilder:validation:Format:=uuid
+	// +optional
+	ID optional.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"`
+	// +optional
+	Name optional.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.
+	// +listType=set
+	// +optional
 	Tags []string `json:"tags,omitempty"`
 }
 

config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml

@@ -5022,8 +5022,10 @@ spec:
                           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.
+                            description: The ID of the desired image. If ID is provided,
+                              the other filters cannot be provided. Must be in UUID
+                              format.
+                            format: uuid
                             type: string
                           name:
                             description: The name of the desired image. If specified,
@@ -5037,7 +5039,12 @@ spec:
                             items:
                               type: string
                             type: array
+                            x-kubernetes-list-type: set
                         type: object
+                        x-kubernetes-validations:
+                        - message: when ID is set you cannot set other options
+                          rule: (has(self.id) && !has(self.name) && !has(self.tags))
+                            || !has(self.id)
                       instanceID:
                         description: InstanceID is the OpenStack instance ID for this
                           machine.
@@ -5540,6 +5547,7 @@ spec:
                         type: boolean
                     required:
                     - flavor
+                    - image
                     type: object
                 type: object
               controlPlaneAvailabilityZones:

config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml

@@ -2447,8 +2447,10 @@ spec:
                                   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.
+                                    description: The ID of the desired image. If ID
+                                      is provided, the other filters cannot be provided.
+                                      Must be in UUID format.
+                                    format: uuid
                                     type: string
                                   name:
                                     description: The name of the desired image. If
@@ -2464,7 +2466,12 @@ spec:
                                     items:
                                       type: string
                                     type: array
+                                    x-kubernetes-list-type: set
                                 type: object
+                                x-kubernetes-validations:
+                                - message: when ID is set you cannot set other options
+                                  rule: (has(self.id) && !has(self.name) && !has(self.tags))
+                                    || !has(self.id)
                               instanceID:
                                 description: InstanceID is the OpenStack instance
                                   ID for this machine.
@@ -2972,6 +2979,7 @@ spec:
                                 type: boolean
                             required:
                             - flavor
+                            - image
                             type: object
                         type: object
                       controlPlaneAvailabilityZones:

config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml

@@ -1810,8 +1810,9 @@ spec:
                   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.
+                    description: The ID of the desired image. If ID is provided, the
+                      other filters cannot be provided. Must be in UUID format.
+                    format: uuid
                     type: string
                   name:
                     description: The name of the desired image. If specified, the
@@ -1825,7 +1826,11 @@ spec:
                     items:
                       type: string
                     type: array
+                    x-kubernetes-list-type: set
                 type: object
+                x-kubernetes-validations:
+                - message: when ID is set you cannot set other options
+                  rule: (has(self.id) && !has(self.name) && !has(self.tags)) || !has(self.id)
               instanceID:
                 description: InstanceID is the OpenStack instance ID for this machine.
                 type: string
@@ -2324,6 +2329,7 @@ spec:
                 type: boolean
             required:
             - flavor
+            - image
             type: object
           status:
             description: OpenStackMachineStatus defines the observed state of OpenStackMachine.

config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachinetemplates.yaml

@@ -1483,8 +1483,10 @@ spec:
                           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.
+                            description: The ID of the desired image. If ID is provided,
+                              the other filters cannot be provided. Must be in UUID
+                              format.
+                            format: uuid
                             type: string
                           name:
                             description: The name of the desired image. If specified,
@@ -1498,7 +1500,12 @@ spec:
                             items:
                               type: string
                             type: array
+                            x-kubernetes-list-type: set
                         type: object
+                        x-kubernetes-validations:
+                        - message: when ID is set you cannot set other options
+                          rule: (has(self.id) && !has(self.name) && !has(self.tags))
+                            || !has(self.id)
                       instanceID:
                         description: InstanceID is the OpenStack instance ID for this
                           machine.
@@ -2001,6 +2008,7 @@ spec:
                         type: boolean
                     required:
                     - flavor
+                    - image
                     type: object
                 required:
                 - spec

controllers/openstackmachine_controller_test.go

@@ -83,7 +83,7 @@ func getDefaultOpenStackMachine() *infrav1.OpenStackMachine {
 			// FloatingIP is only used by the cluster controller for the Bastion
 			// TODO: Test Networks, Ports, Subnet, and Trunk separately
 			Flavor:     flavorName,
-			Image:      infrav1.ImageFilter{ID: imageUUID},
+			Image:      infrav1.ImageFilter{ID: pointer.String(imageUUID)},
 			SSHKeyName: sshKeyName,
 			Tags:       []string{"test-tag"},
 			ServerMetadata: []infrav1.ServerMetadata{

docs/book/src/api/v1beta1/api.md

@@ -1589,6 +1589,7 @@ address in any subnet of the port&rsquo;s network.</p>
 <a href="#infrastructure.cluster.x-k8s.io/v1beta1.OpenStackMachineSpec">OpenStackMachineSpec</a>)
 </p>
 <p>
+<p>ImageFilter describes the data needed to identify which image to use. If ID is provided it is required that all other fields are unset.</p>
 </p>
 <table>
 <thead>
@@ -1606,7 +1607,8 @@ string
 </em>
 </td>
 <td>
-<p>The ID of the desired image. If this is provided, the other filters will be ignored.</p>
+<em>(Optional)</em>
+<p>The ID of the desired image. If ID is provided, the other filters cannot be provided. Must be in UUID format.</p>
 </td>
 </tr>
 <tr>
@@ -1617,6 +1619,7 @@ string
 </em>
 </td>
 <td>
+<em>(Optional)</em>
 <p>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.</p>
 </td>
 </tr>
@@ -1628,6 +1631,7 @@ string
 </em>
 </td>
 <td>
+<em>(Optional)</em>
 <p>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.</p>
 </td>
 </tr>

docs/book/src/topics/crd-changes/v1alpha7-to-v1beta1.md

@@ -91,7 +91,7 @@ If empty object or null is provided, Machine will not be added to any server gro
 #### 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.
+The `ImageFilter` object allows selection of an image by ID or by name and tags. If ID is set, no other fields can be set in the object.
 
 ```yaml
 image: "test-image"

pkg/cloud/services/compute/instance.go

@@ -333,8 +333,8 @@ func applyServerGroupID(opts servers.CreateOptsBuilder, serverGroupID string) se
 
 // 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
+	if image.ID != nil {
+		return *image.ID, nil
 	}
 
 	listOpts := filterconvert.ImageFilterToListOpts(&image)