Fix network/image webhook validations (#523)

Prajyot-Parab · web-flow · commit b56542d56041 · 2022-01-31T00:54:24.000-08:00
Signed-off-by: Prajyot-Parab &lt;prajyot.parab2@ibm.com&gt;
diff --git a/api/v1beta1/common.go b/api/v1beta1/common.go
@@ -16,6 +16,8 @@ limitations under the License.
 
 package v1beta1
 
+import "k8s.io/apimachinery/pkg/util/validation/field"
+
 func defaultIBMPowerVSMachineSpec(spec *IBMPowerVSMachineSpec) {
 
 	if spec.Memory == "" {
@@ -52,11 +54,11 @@ func validateIBMPowerVSProcType(spec IBMPowerVSMachineSpec) (bool, IBMPowerVSMac
 	return false, spec
 }
 
-func validateIBMPowerVSNetwork(network IBMPowerVSResourceReference) (bool, IBMPowerVSResourceReference) {
-	if network.ID != nil && network.Name != nil {
-		return false, network
+func validateIBMPowerVSResourceReference(res IBMPowerVSResourceReference, resType string) (bool, *field.Error) {
+	if res.ID != nil && res.Name != nil {
+		return false, field.Invalid(field.NewPath("spec", resType), res, "Only one of "+resType+" - ID or Name may be specified")
 	}
-	return true, IBMPowerVSResourceReference{}
+	return true, nil
 }
 
 func defaultIBMVPCMachineSpec(spec *IBMVPCMachineSpec) {
diff --git a/api/v1beta1/ibmpowervscluster_types.go b/api/v1beta1/ibmpowervscluster_types.go
@@ -35,6 +35,7 @@ type IBMPowerVSClusterSpec struct {
 	// Important: Run "make" to regenerate code after modifying this file
 
 	// ServiceInstanceID is the id of the power cloud instance where the vsi instance will get deployed
+	// +kubebuilder:validation:MinLength=1
 	ServiceInstanceID string `json:"serviceInstanceID"`
 
 	// Network is the reference to the Network to use for this cluster.
diff --git a/api/v1beta1/ibmpowervscluster_webhook.go b/api/v1beta1/ibmpowervscluster_webhook.go
@@ -82,8 +82,8 @@ func (r *IBMPowerVSCluster) validateIBMPowerVSCluster() error {
 }
 
 func (r *IBMPowerVSCluster) validateIBMPowerVSClusterNetwork() *field.Error {
-	if res, net := validateIBMPowerVSNetwork(r.Spec.Network); !res {
-		return field.Invalid(field.NewPath("spec", "network"), net, "Only one of Network - ID or Name may be specified")
+	if res, err := validateIBMPowerVSResourceReference(r.Spec.Network, "Network"); !res {
+		return err
 	}
 	return nil
 }
diff --git a/api/v1beta1/ibmpowervsmachine_types.go b/api/v1beta1/ibmpowervsmachine_types.go
@@ -37,6 +37,7 @@ type IBMPowerVSMachineSpec struct {
 	// Important: Run "make" to regenerate code after modifying this file
 
 	// ServiceInstanceID is the id of the power cloud instance where the vsi instance will get deployed
+	// +kubebuilder:validation:MinLength=1
 	ServiceInstanceID string `json:"serviceInstanceID"`
 
 	// SSHKey is the name of the SSH key pair provided to the vsi for authenticating users
@@ -74,10 +75,12 @@ type IBMPowerVSMachineSpec struct {
 // a validation error.
 type IBMPowerVSResourceReference struct {
 	// ID of resource
+	// +kubebuilder:validation:MinLength=1
 	// +optional
 	ID *string `json:"id,omitempty"`
 
 	// Name of resource
+	// +kubebuilder:validation:MinLength=1
 	// +optional
 	Name *string `json:"name,omitempty"`
 }
diff --git a/api/v1beta1/ibmpowervsmachine_webhook.go b/api/v1beta1/ibmpowervsmachine_webhook.go
@@ -78,6 +78,9 @@ func (r *IBMPowerVSMachine) validateIBMPowerVSMachine() error {
 	if err := r.validateIBMPowerVSMachineNetwork(); err != nil {
 		allErrs = append(allErrs, err)
 	}
+	if err := r.validateIBMPowerVSMachineImage(); err != nil {
+		allErrs = append(allErrs, err)
+	}
 	if len(allErrs) == 0 {
 		return nil
 	}
@@ -102,8 +105,15 @@ func (r *IBMPowerVSMachine) validateIBMPowerVSMachineProcType() *field.Error {
 }
 
 func (r *IBMPowerVSMachine) validateIBMPowerVSMachineNetwork() *field.Error {
-	if res, net := validateIBMPowerVSNetwork(r.Spec.Network); !res {
-		return field.Invalid(field.NewPath("spec", "network"), net, "Only one of Network - ID or Name may be specified")
+	if res, err := validateIBMPowerVSResourceReference(r.Spec.Network, "Network"); !res {
+		return err
+	}
+	return nil
+}
+
+func (r *IBMPowerVSMachine) validateIBMPowerVSMachineImage() *field.Error {
+	if res, err := validateIBMPowerVSResourceReference(r.Spec.Image, "Image"); !res {
+		return err
 	}
 	return nil
 }
diff --git a/api/v1beta1/ibmpowervsmachinetemplate_webhook.go b/api/v1beta1/ibmpowervsmachinetemplate_webhook.go
@@ -78,6 +78,9 @@ func (r *IBMPowerVSMachineTemplate) validateIBMPowerVSMachineTemplate() error {
 	if err := r.validateIBMPowerVSMachineTemplateNetwork(); err != nil {
 		allErrs = append(allErrs, err)
 	}
+	if err := r.validateIBMPowerVSMachineTemplateImage(); err != nil {
+		allErrs = append(allErrs, err)
+	}
 	if len(allErrs) == 0 {
 		return nil
 	}
@@ -102,8 +105,15 @@ func (r *IBMPowerVSMachineTemplate) validateIBMPowerVSMachineTemplateProcType()
 }
 
 func (r *IBMPowerVSMachineTemplate) validateIBMPowerVSMachineTemplateNetwork() *field.Error {
-	if res, net := validateIBMPowerVSNetwork(r.Spec.Template.Spec.Network); !res {
-		return field.Invalid(field.NewPath("spec", "template", "spec", "network"), net, "Only one of Network - ID or Name may be specified")
+	if res, err := validateIBMPowerVSResourceReference(r.Spec.Template.Spec.Network, "Network"); !res {
+		return err
+	}
+	return nil
+}
+
+func (r *IBMPowerVSMachineTemplate) validateIBMPowerVSMachineTemplateImage() *field.Error {
+	if res, err := validateIBMPowerVSResourceReference(r.Spec.Template.Spec.Image, "Image"); !res {
+		return err
 	}
 	return nil
 }
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_ibmpowervsclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_ibmpowervsclusters.yaml
@@ -126,14 +126,17 @@ spec:
                 properties:
                   id:
                     description: ID of resource
+                    minLength: 1
                     type: string
                   name:
                     description: Name of resource
+                    minLength: 1
                     type: string
                 type: object
               serviceInstanceID:
                 description: ServiceInstanceID is the id of the power cloud instance
                   where the vsi instance will get deployed
+                minLength: 1
                 type: string
             required:
             - network
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_ibmpowervsmachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_ibmpowervsmachines.yaml
@@ -193,9 +193,11 @@ spec:
                 properties:
                   id:
                     description: ID of resource
+                    minLength: 1
                     type: string
                   name:
                     description: Name of resource
+                    minLength: 1
                     type: string
                 type: object
               memory:
@@ -207,9 +209,11 @@ spec:
                 properties:
                   id:
                     description: ID of resource
+                    minLength: 1
                     type: string
                   name:
                     description: Name of resource
+                    minLength: 1
                     type: string
                 type: object
               procType:
@@ -226,6 +230,7 @@ spec:
               serviceInstanceID:
                 description: ServiceInstanceID is the id of the power cloud instance
                   where the vsi instance will get deployed
+                minLength: 1
                 type: string
               sshKey:
                 description: SSHKey is the name of the SSH key pair provided to the
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_ibmpowervsmachinetemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_ibmpowervsmachinetemplates.yaml
@@ -150,9 +150,11 @@ spec:
                         properties:
                           id:
                             description: ID of resource
+                            minLength: 1
                             type: string
                           name:
                             description: Name of resource
+                            minLength: 1
                             type: string
                         type: object
                       memory:
@@ -164,9 +166,11 @@ spec:
                         properties:
                           id:
                             description: ID of resource
+                            minLength: 1
                             type: string
                           name:
                             description: Name of resource
+                            minLength: 1
                             type: string
                         type: object
                       procType:
@@ -183,6 +187,7 @@ spec:
                       serviceInstanceID:
                         description: ServiceInstanceID is the id of the power cloud
                           instance where the vsi instance will get deployed
+                        minLength: 1
                         type: string
                       sshKey:
                         description: SSHKey is the name of the SSH key pair provided
diff --git a/controllers/ibmpowervscluster_controller_test.go b/controllers/ibmpowervscluster_controller_test.go
@@ -41,7 +41,7 @@ var _ = Describe("IBMPowerVSClusterReconciler", func() {
 				Log:    klogr.New(),
 			}
 
-			instance := &v1beta1.IBMPowerVSCluster{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}}
+			instance := &v1beta1.IBMPowerVSCluster{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, Spec: v1beta1.IBMPowerVSClusterSpec{ServiceInstanceID: "foo"}}
 
 			// Create the IBMPowerVSCluster object and expect the Reconcile to be created
 			Expect(k8sClient.Create(ctx, instance)).To(Succeed())

Original file line number	Diff line number	Diff line change
`@@ -16,6 +16,8 @@ limitations under the License.`
`16`	`16`
`17`	`17`	`package v1beta1`
`18`	`18`
	`19`	`+import "k8s.io/apimachinery/pkg/util/validation/field"`
	`20`	`+`
`19`	`21`	`func defaultIBMPowerVSMachineSpec(spec *IBMPowerVSMachineSpec) {`
`20`	`22`
`21`	`23`	`if spec.Memory == "" {`
`@@ -52,11 +54,11 @@ func validateIBMPowerVSProcType(spec IBMPowerVSMachineSpec) (bool, IBMPowerVSMac`
`52`	`54`	`return false, spec`
`53`	`55`	`}`
`54`	`56`
`55`		`-func validateIBMPowerVSNetwork(network IBMPowerVSResourceReference) (bool, IBMPowerVSResourceReference) {`
`56`		`- if network.ID != nil && network.Name != nil {`
`57`		`- return false, network`
	`57`	`+func validateIBMPowerVSResourceReference(res IBMPowerVSResourceReference, resType string) (bool, *field.Error) {`
	`58`	`+ if res.ID != nil && res.Name != nil {`
	`59`	`+ return false, field.Invalid(field.NewPath("spec", resType), res, "Only one of "+resType+" - ID or Name may be specified")`
`58`	`60`	`}`
`59`		`- return true, IBMPowerVSResourceReference{}`
	`61`	`+ return true, nil`
`60`	`62`	`}`
`61`	`63`
`62`	`64`	`func defaultIBMVPCMachineSpec(spec *IBMVPCMachineSpec) {`
Original file line number	Diff line number	Diff line change
`@@ -82,8 +82,8 @@ func (r *IBMPowerVSCluster) validateIBMPowerVSCluster() error {`
`82`	`82`	`}`
`83`	`83`
`84`	`84`	`func (r IBMPowerVSCluster) validateIBMPowerVSClusterNetwork() field.Error {`
`85`		`- if res, net := validateIBMPowerVSNetwork(r.Spec.Network); !res {`
`86`		`- return field.Invalid(field.NewPath("spec", "network"), net, "Only one of Network - ID or Name may be specified")`
	`85`	`+ if res, err := validateIBMPowerVSResourceReference(r.Spec.Network, "Network"); !res {`
	`86`	`+ return err`
`87`	`87`	`}`
`88`	`88`	`return nil`
`89`	`89`	`}`
Original file line number	Diff line number	Diff line change
`@@ -78,6 +78,9 @@ func (r *IBMPowerVSMachine) validateIBMPowerVSMachine() error {`
`78`	`78`	`if err := r.validateIBMPowerVSMachineNetwork(); err != nil {`
`79`	`79`	`allErrs = append(allErrs, err)`
`80`	`80`	`}`
	`81`	`+ if err := r.validateIBMPowerVSMachineImage(); err != nil {`
	`82`	`+ allErrs = append(allErrs, err)`
	`83`	`+ }`
`81`	`84`	`if len(allErrs) == 0 {`
`82`	`85`	`return nil`
`83`	`86`	`}`
`@@ -102,8 +105,15 @@ func (r IBMPowerVSMachine) validateIBMPowerVSMachineProcType() field.Error {`
`102`	`105`	`}`
`103`	`106`
`104`	`107`	`func (r IBMPowerVSMachine) validateIBMPowerVSMachineNetwork() field.Error {`
`105`		`- if res, net := validateIBMPowerVSNetwork(r.Spec.Network); !res {`
`106`		`- return field.Invalid(field.NewPath("spec", "network"), net, "Only one of Network - ID or Name may be specified")`
	`108`	`+ if res, err := validateIBMPowerVSResourceReference(r.Spec.Network, "Network"); !res {`
	`109`	`+ return err`
	`110`	`+ }`
	`111`	`+ return nil`
	`112`	`+}`
	`113`	`+`
	`114`	`+func (r IBMPowerVSMachine) validateIBMPowerVSMachineImage() field.Error {`
	`115`	`+ if res, err := validateIBMPowerVSResourceReference(r.Spec.Image, "Image"); !res {`
	`116`	`+ return err`
`107`	`117`	`}`
`108`	`118`	`return nil`
`109`	`119`	`}`
Original file line number	Diff line number	Diff line change
`@@ -41,7 +41,7 @@ var _ = Describe("IBMPowerVSClusterReconciler", func() {`
`41`	`41`	`Log: klogr.New(),`
`42`	`42`	`}`
`43`	`43`
`44`		`- instance := &v1beta1.IBMPowerVSCluster{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}}`
	`44`	`+ instance := &v1beta1.IBMPowerVSCluster{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, Spec: v1beta1.IBMPowerVSClusterSpec{ServiceInstanceID: "foo"}}`
`45`	`45`
`46`	`46`	`// Create the IBMPowerVSCluster object and expect the Reconcile to be created`
`47`	`47`	`Expect(k8sClient.Create(ctx, instance)).To(Succeed())`