Merge pull request #1939 from shiftstack/filters-pointers

⚠️ ImageFilter - add exclusive validation -> pointers
kubernetes-sigs · Mar 15, 2024 · e7d0e19 · e7d0e19
2 parents 4ab8b3a + dc3959e
commit e7d0e19
Show file tree

Hide file tree

Showing 21 changed files with 183 additions and 73 deletions.
diff --git a/api/v1alpha5/conversion.go b/api/v1alpha5/conversion.go
@@ -361,11 +361,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
 
@@ -646,12 +645,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 {

diff --git a/api/v1alpha6/openstackmachine_conversion.go b/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 {

diff --git a/api/v1alpha7/openstackmachine_conversion.go b/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 {

diff --git a/api/v1beta1/openstackmachine_types.go b/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"`

diff --git a/api/v1beta1/types.go b/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"`
 }
 

diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml b/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:

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml b/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:

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachines.yaml b/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.

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackmachinetemplates.yaml b/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

diff --git a/controllers/openstackmachine_controller_test.go b/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{

diff --git a/docs/book/src/api/v1beta1/api.md b/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>

diff --git a/docs/book/src/topics/crd-changes/v1alpha7-to-v1beta1.md b/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"

diff --git a/pkg/cloud/services/compute/instance.go b/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)