Merge pull request #440 from stuggi/8063-18.0.2

[18.0.0-proposed] [validation] check cinderVolume names are valid

api/go.mod

@@ -3,8 +3,9 @@ module github.com/openstack-k8s-operators/cinder-operator/api
 go 1.20
 
 require (
-	github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240730154700-e526dc22c2bf
+	github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240913084932-eb5ec1aa13b5
 	github.com/openstack-k8s-operators/lib-common/modules/storage v0.3.1-0.20240730154700-e526dc22c2bf
+	golang.org/x/exp v0.0.0-20240213143201-ec583247a57a
 	k8s.io/api v0.28.11
 	k8s.io/apimachinery v0.28.11
 	sigs.k8s.io/controller-runtime v0.16.6
@@ -43,7 +44,6 @@ require (
 	github.com/prometheus/procfs v0.12.0 // indirect
 	github.com/spf13/pflag v1.0.5 // indirect
 	go.uber.org/goleak v1.3.0 // indirect
-	golang.org/x/exp v0.0.0-20240213143201-ec583247a57a // indirect
 	golang.org/x/net v0.25.0 // indirect
 	golang.org/x/oauth2 v0.16.0 // indirect
 	golang.org/x/sys v0.20.0 // indirect
@@ -61,7 +61,7 @@ require (
 	k8s.io/component-base v0.28.11 //indirect
 	k8s.io/klog/v2 v2.120.1 // indirect
 	k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect
-	k8s.io/utils v0.0.0-20240502163921-fe8a2dddb1d0 // indirect
+	k8s.io/utils v0.0.0-20240711033017-18e509b52bc8 // indirect
 	sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect
 	sigs.k8s.io/yaml v1.4.0 // indirect

api/go.sum

@@ -65,8 +65,8 @@ github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq
 github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ=
 github.com/onsi/ginkgo/v2 v2.19.0 h1:9Cnnf7UHo57Hy3k6/m5k3dRfGTMXGvxhHFvkDTCTpvA=
 github.com/onsi/gomega v1.33.1 h1:dsYjIxxSR755MDmKVsaFQTE22ChNBcuuTWgkUDSubOk=
-github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240730154700-e526dc22c2bf h1:VDxaGely5T2NBt7HtCMYMMd7yR6w7tKrHFdQgAus/wY=
-github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240730154700-e526dc22c2bf/go.mod h1:k9KuWN2LBtLbKHgcyh/0lrwk3Kr0cOAhiR3hi/mrwOQ=
+github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240913084932-eb5ec1aa13b5 h1:1PCgFxZUaIlkAqBNgl18xtvnX0CdGZm1SYISKawu+oU=
+github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240913084932-eb5ec1aa13b5/go.mod h1:k9KuWN2LBtLbKHgcyh/0lrwk3Kr0cOAhiR3hi/mrwOQ=
 github.com/openstack-k8s-operators/lib-common/modules/storage v0.3.1-0.20240730154700-e526dc22c2bf h1:TdE6ccP5WWxv2UAWjOpugiGnwyeXk0NszqVdHmyZqXs=
 github.com/openstack-k8s-operators/lib-common/modules/storage v0.3.1-0.20240730154700-e526dc22c2bf/go.mod h1:v9iFrR8J5fZACS9W5pZau/4lwyWs/YmO4ezpDeoEFKU=
 github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
@@ -179,8 +179,8 @@ k8s.io/klog/v2 v2.120.1 h1:QXU6cPEOIslTGvZaXvFWiP9VKyeet3sawzTOvdXb4Vw=
 k8s.io/klog/v2 v2.120.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE=
 k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 h1:BZqlfIlq5YbRMFko6/PM7FjZpUb45WallggurYhKGag=
 k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340/go.mod h1:yD4MZYeKMBwQKVht279WycxKyM84kkAx2DPrTXaeb98=
-k8s.io/utils v0.0.0-20240502163921-fe8a2dddb1d0 h1:jgGTlFYnhF1PM1Ax/lAlxUPE+KfCIXHaathvJg1C3ak=
-k8s.io/utils v0.0.0-20240502163921-fe8a2dddb1d0/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
+k8s.io/utils v0.0.0-20240711033017-18e509b52bc8 h1:pUdcCO1Lk/tbT5ztQWOBi5HBgbBP1J8+AsQnQCKsi8A=
+k8s.io/utils v0.0.0-20240711033017-18e509b52bc8/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
 sigs.k8s.io/controller-runtime v0.16.6 h1:FiXwTuFF5ZJKmozfP2Z0j7dh6kmxP4Ou1KLfxgKKC3I=
 sigs.k8s.io/controller-runtime v0.16.6/go.mod h1:+dQzkZxnylD0u49e0a+7AR+vlibEBaThmPca7lTyUsI=
 sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd h1:EDPBXCAspyGV4jQlpZSudPeMmr1bNJefnuqLsRAsHZo=

api/v1beta1/cinder_webhook.go

@@ -24,6 +24,8 @@ package v1beta1
 import (
 	"fmt"
 
+	"golang.org/x/exp/maps"
+
 	"github.com/openstack-k8s-operators/lib-common/modules/common/service"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/util"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -34,6 +36,8 @@ import (
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+
+	common_webhook "github.com/openstack-k8s-operators/lib-common/modules/common/webhook"
 )
 
 // CinderDefaults -
@@ -134,6 +138,24 @@ func (r *Cinder) ValidateCreate() (admission.Warnings, error) {
 
 	var allErrs field.ErrorList
 	basePath := field.NewPath("spec")
+
+	// Validate cinderVolume name is valid
+	// CinderVolume name is <cinder name>-volume-<volume name>
+	// The CinderVolume controller creates StatefulSet for volume service to run.
+	// This adds a StatefulSet pod's label
+	// "controller-revision-hash": "<statefulset_name>-<hash>"
+	// to the pod.
+	// The kubernetes label is restricted under 63 char and the revision
+	// hash is an int32, 10 chars + the hyphen. Which results in a default
+	// statefulset max len of 52 chars. The statefulset name also
+	// contain the cinder name and -volume-. So the
+	// max len also need to be reduced bye the length of those.
+	err := common_webhook.ValidateDNS1123Label(
+		basePath.Child("cinderVolumes"),
+		maps.Keys(r.Spec.CinderVolumes),
+		GetCrMaxLengthCorrection(r.Name)) // omit issue with statefulset pod label "controller-revision-hash": "<statefulset_name>-<hash>"
+	allErrs = append(allErrs, err...)
+
 	if err := r.Spec.ValidateCreate(basePath); err != nil {
 		allErrs = append(allErrs, err...)
 	}
@@ -183,6 +205,23 @@ func (r *Cinder) ValidateUpdate(old runtime.Object) (admission.Warnings, error)
 	var allErrs field.ErrorList
 	basePath := field.NewPath("spec")
 
+	// Validate cinderVolume name is valid
+	// CinderVolume name is <cinder name>-volume-<volume name>
+	// The CinderVolume controller creates StatefulSet for volume service to run.
+	// This adds a StatefulSet pod's label
+	// "controller-revision-hash": "<statefulset_name>-<hash>"
+	// to the pod.
+	// The kubernetes label is restricted under 63 char and the revision
+	// hash is an int32, 10 chars + the hyphen. Which results in a default
+	// statefulset max len of 52 chars. The statefulset name also
+	// contain the cinder name and -volume-. So the
+	// max len also need to be reduced bye the length of those.
+	err := common_webhook.ValidateDNS1123Label(
+		basePath.Child("cinderVolumes"),
+		maps.Keys(r.Spec.CinderVolumes),
+		GetCrMaxLengthCorrection(r.Name)) // omit issue with statefulset pod label "controller-revision-hash": "<statefulset_name>-<hash>"
+	allErrs = append(allErrs, err...)
+
 	if err := r.Spec.ValidateUpdate(oldCinder.Spec, basePath); err != nil {
 		allErrs = append(allErrs, err...)
 	}
@@ -239,12 +278,12 @@ func (spec *CinderSpecCore) SetDefaultRouteAnnotations(annotations map[string]st
 	valHAProxy, okHAProxy := annotations[haProxyAnno]
 
 	// Human operator set the HAProxy timeout manually
-	if (!okCinder && okHAProxy) {
+	if !okCinder && okHAProxy {
 		return
 	}
 
 	// Human operator modified the HAProxy timeout manually without removing the Cinder flag
-	if (okCinder && okHAProxy && valCinder != valHAProxy) {
+	if okCinder && okHAProxy && valCinder != valHAProxy {
 		delete(annotations, cinderAnno)
 		return
 	}
@@ -253,3 +292,16 @@ func (spec *CinderSpecCore) SetDefaultRouteAnnotations(annotations map[string]st
 	annotations[cinderAnno] = timeout
 	annotations[haProxyAnno] = timeout
 }
+
+// GetCrMaxLengthCorrection - get correction for ValidateDNS1123Label to get the real max string len of the cinder volume key
+func GetCrMaxLengthCorrection(name string) int {
+	// defaultCrMaxLengthCorrection - DNS1123LabelMaxLength (63) - CrMaxLengthCorrection used in validation to
+	// omit issue with statefulset pod label "controller-revision-hash": "<statefulset_name>-<hash>"
+	// Int32 is a 10 character + hyphen = 11
+	defaultCrMaxLengthCorrection := 11
+
+	// cinder volume name is <cinder name>-volume-<volume name> with this
+	// crMaxLengthCorrection = defaultCrMaxLengthCorrection + len(<cinder name>) + "-volume-"
+
+	return (defaultCrMaxLengthCorrection + len(name) + 8)
+}

go.mod

@@ -10,15 +10,15 @@ require (
 	github.com/onsi/gomega v1.33.1
 	github.com/openstack-k8s-operators/infra-operator/apis v0.3.1-0.20240701051058-e23cdfd81161
 	github.com/openstack-k8s-operators/keystone-operator/api v0.3.1-0.20240703053916-a2bf14020ef4
-	github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240730154700-e526dc22c2bf
+	github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240913084932-eb5ec1aa13b5
 	github.com/openstack-k8s-operators/lib-common/modules/storage v0.3.1-0.20240730154700-e526dc22c2bf
 	github.com/openstack-k8s-operators/lib-common/modules/test v0.3.1-0.20240624132705-6c8da3c0bbfd
 	github.com/openstack-k8s-operators/mariadb-operator/api v0.3.1-0.20240626205513-5c175a95f408
 	golang.org/x/exp v0.0.0-20240213143201-ec583247a57a
 	k8s.io/api v0.28.11
 	k8s.io/apimachinery v0.28.11
 	k8s.io/client-go v0.28.11
-	k8s.io/utils v0.0.0-20240502163921-fe8a2dddb1d0
+	k8s.io/utils v0.0.0-20240711033017-18e509b52bc8
 	sigs.k8s.io/controller-runtime v0.16.6
 )
 

go.sum

@@ -78,8 +78,8 @@ github.com/openstack-k8s-operators/infra-operator/apis v0.3.1-0.20240701051058-e
 github.com/openstack-k8s-operators/infra-operator/apis v0.3.1-0.20240701051058-e23cdfd81161/go.mod h1:gdc2zBX7iz+6vWjD2dhi+rtEmTulb91aybjU6bJyFVQ=
 github.com/openstack-k8s-operators/keystone-operator/api v0.3.1-0.20240703053916-a2bf14020ef4 h1:MZqkT7o29H2zrCdWFo7ZgmpipCRO0HUvIgUTv9jus2s=
 github.com/openstack-k8s-operators/keystone-operator/api v0.3.1-0.20240703053916-a2bf14020ef4/go.mod h1:Pg+s5VIUvZNec600X7GtlGTAUD2vafi9GZ7V0guaujI=
-github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240730154700-e526dc22c2bf h1:VDxaGely5T2NBt7HtCMYMMd7yR6w7tKrHFdQgAus/wY=
-github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240730154700-e526dc22c2bf/go.mod h1:k9KuWN2LBtLbKHgcyh/0lrwk3Kr0cOAhiR3hi/mrwOQ=
+github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240913084932-eb5ec1aa13b5 h1:1PCgFxZUaIlkAqBNgl18xtvnX0CdGZm1SYISKawu+oU=
+github.com/openstack-k8s-operators/lib-common/modules/common v0.3.1-0.20240913084932-eb5ec1aa13b5/go.mod h1:k9KuWN2LBtLbKHgcyh/0lrwk3Kr0cOAhiR3hi/mrwOQ=
 github.com/openstack-k8s-operators/lib-common/modules/openstack v0.3.1-0.20240730154700-e526dc22c2bf h1:5IxujAEi+Fk3DyOY83CegcNe+Lcqg/RwEPaPEhQiQ3E=
 github.com/openstack-k8s-operators/lib-common/modules/openstack v0.3.1-0.20240730154700-e526dc22c2bf/go.mod h1:zuPcZ5Kopr15AdfxvA0xqKIIGCZ0XbSe/0VHNKuvbEE=
 github.com/openstack-k8s-operators/lib-common/modules/storage v0.3.1-0.20240730154700-e526dc22c2bf h1:TdE6ccP5WWxv2UAWjOpugiGnwyeXk0NszqVdHmyZqXs=
@@ -206,8 +206,8 @@ k8s.io/klog/v2 v2.120.1 h1:QXU6cPEOIslTGvZaXvFWiP9VKyeet3sawzTOvdXb4Vw=
 k8s.io/klog/v2 v2.120.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE=
 k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 h1:BZqlfIlq5YbRMFko6/PM7FjZpUb45WallggurYhKGag=
 k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340/go.mod h1:yD4MZYeKMBwQKVht279WycxKyM84kkAx2DPrTXaeb98=
-k8s.io/utils v0.0.0-20240502163921-fe8a2dddb1d0 h1:jgGTlFYnhF1PM1Ax/lAlxUPE+KfCIXHaathvJg1C3ak=
-k8s.io/utils v0.0.0-20240502163921-fe8a2dddb1d0/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
+k8s.io/utils v0.0.0-20240711033017-18e509b52bc8 h1:pUdcCO1Lk/tbT5ztQWOBi5HBgbBP1J8+AsQnQCKsi8A=
+k8s.io/utils v0.0.0-20240711033017-18e509b52bc8/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
 sigs.k8s.io/controller-runtime v0.16.6 h1:FiXwTuFF5ZJKmozfP2Z0j7dh6kmxP4Ou1KLfxgKKC3I=
 sigs.k8s.io/controller-runtime v0.16.6/go.mod h1:+dQzkZxnylD0u49e0a+7AR+vlibEBaThmPca7lTyUsI=
 sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd h1:EDPBXCAspyGV4jQlpZSudPeMmr1bNJefnuqLsRAsHZo=

test/functional/cinder_controller_test.go

@@ -16,6 +16,7 @@ limitations under the License.
 package functional
 
 import (
+	"errors"
 	"fmt"
 	"os"
 
@@ -27,6 +28,7 @@ import (
 	. "github.com/openstack-k8s-operators/lib-common/modules/common/test/helpers"
 
 	corev1 "k8s.io/api/core/v1"
+	k8s_errors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/ptr"
@@ -790,4 +792,64 @@ var _ = Describe("Cinder Webhook", func() {
 					"Invalid value: \"wrooong\": invalid endpoint type: wrooong"),
 		)
 	})
+
+	It("webhooks reject the request - cinderVolume key too long", func() {
+		spec := GetDefaultCinderSpec()
+		raw := map[string]interface{}{
+			"apiVersion": "cinder.openstack.org/v1beta1",
+			"kind":       "Cinder",
+			"metadata": map[string]interface{}{
+				"name":      cinderTest.Instance.Name,
+				"namespace": cinderTest.Instance.Namespace,
+			},
+			"spec": spec,
+		}
+
+		volumeList := map[string]interface{}{
+			"foo-1234567890-1234567890-1234567890-1234567890-1234567890": GetDefaultCinderVolumeSpec(),
+		}
+		spec["cinderVolumes"] = volumeList
+
+		unstructuredObj := &unstructured.Unstructured{Object: raw}
+		_, err := controllerutil.CreateOrPatch(
+			ctx, k8sClient, unstructuredObj, func() error { return nil })
+
+		Expect(err).Should(HaveOccurred())
+		var statusError *k8s_errors.StatusError
+		Expect(errors.As(err, &statusError)).To(BeTrue())
+		Expect(statusError.ErrStatus.Message).To(
+			ContainSubstring(
+				"Invalid value: \"foo-1234567890-1234567890-1234567890-1234567890-1234567890\": must be no more than 32 characters"),
+		)
+	})
+
+	It("webhooks reject the request - cinderVolume wrong key/name", func() {
+		spec := GetDefaultCinderSpec()
+		raw := map[string]interface{}{
+			"apiVersion": "cinder.openstack.org/v1beta1",
+			"kind":       "Cinder",
+			"metadata": map[string]interface{}{
+				"name":      cinderTest.Instance.Name,
+				"namespace": cinderTest.Instance.Namespace,
+			},
+			"spec": spec,
+		}
+
+		volumeList := map[string]interface{}{
+			"foo_bar": GetDefaultCinderVolumeSpec(),
+		}
+		spec["cinderVolumes"] = volumeList
+
+		unstructuredObj := &unstructured.Unstructured{Object: raw}
+		_, err := controllerutil.CreateOrPatch(
+			ctx, k8sClient, unstructuredObj, func() error { return nil })
+
+		Expect(err).Should(HaveOccurred())
+		var statusError *k8s_errors.StatusError
+		Expect(errors.As(err, &statusError)).To(BeTrue())
+		Expect(statusError.ErrStatus.Message).To(
+			ContainSubstring(
+				"Invalid value: \"foo_bar\": a lowercase RFC 1123 label must consist of lower case alphanumeric characters"),
+		)
+	})
 })