Repository: kurator-dev/kurator
Pull request #345 (2023-06-26 03:35:23)

[release-0.4] attachedcluster: init webhook
This is an automated cherry-pick of #344

```release-note
add validation for attachedcluster
```


Patch:
diff --git a/cmd/cluster-operator/attachedcluster/attachedcluster.go b/cmd/cluster-operator/attachedcluster/attachedcluster.go
index 4eb6250..9fa6333 100644
--- a/cmd/cluster-operator/attachedcluster/attachedcluster.go
+++ b/cmd/cluster-operator/attachedcluster/attachedcluster.go
@@ -24,6 +24,7 @@ import (
 
 	"kurator.dev/kurator/cmd/cluster-operator/options"
 	clusteroperator "kurator.dev/kurator/pkg/cluster-operator"
+	"kurator.dev/kurator/pkg/webhooks"
 )
 
 var log = ctrl.Log.WithName("attached cluster")
@@ -38,5 +39,12 @@ func InitControllers(ctx context.Context, opts *options.Options, mgr ctrl.Manage
 		return err
 	}
 
+	if err := (&webhooks.AttachedClusterWebhook{
+		Client: mgr.GetClient(),
+	}).SetupWebhookWithManager(mgr); err != nil {
+		log.Error(err, "unable to create AttachedCluster webhook", "Webhook", "AttachedCluster")
+		return err
+	}
+
 	return nil
 }
diff --git a/manifests/charts/cluster-operator/templates/webhooks.yaml b/manifests/charts/cluster-operator/templates/webhooks.yaml
index 1fb0710..04928a0 100644
--- a/manifests/charts/cluster-operator/templates/webhooks.yaml
+++ b/manifests/charts/cluster-operator/templates/webhooks.yaml
@@ -50,3 +50,25 @@ webhooks:
     resources:
     - customclusters
   sideEffects: None
+- admissionReviewVersions:
+    - v1
+    - v1beta1
+  clientConfig:
+    service:
+      name: kurator-webhook-service
+      namespace: {{ .Release.Namespace }}
+      path: /validate-cluster-kurator-dev-v1alpha1-attachedcluster # get this path from webhook service log
+  failurePolicy: Fail
+  matchPolicy: Equivalent
+  name: validation.attachedcluster.cluster.kurator.dev
+  rules:
+    - apiGroups:
+        - cluster.kurator.dev
+      apiVersions:
+        - v1alpha1
+      operations:
+        - CREATE
+        - UPDATE
+      resources:
+        - attachedclusters
+  sideEffects: None
diff --git a/pkg/webhooks/attachedcluster_webhook.go b/pkg/webhooks/attachedcluster_webhook.go
new file mode 100644
index 0000000..2a21cf2
--- /dev/null
+++ b/pkg/webhooks/attachedcluster_webhook.go
@@ -0,0 +1,101 @@
+/*
+Copyright Kurator Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package webhooks
+
+import (
+	"context"
+	"fmt"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
+
+	"kurator.dev/kurator/pkg/apis/cluster/v1alpha1"
+)
+
+var _ webhook.CustomValidator = &AttachedClusterWebhook{}
+
+type AttachedClusterWebhook struct {
+	Client client.Reader
+}
+
+func (wh *AttachedClusterWebhook) SetupWebhookWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewWebhookManagedBy(mgr).
+		For(&v1alpha1.AttachedCluster{}).
+		WithValidator(wh).
+		Complete()
+}
+
+func (wh *AttachedClusterWebhook) ValidateCreate(_ context.Context, obj runtime.Object) error {
+	in, ok := obj.(*v1alpha1.AttachedCluster)
+	if !ok {
+		return apierrors.NewBadRequest(fmt.Sprintf("expected a AttachedCluster but got a %T", obj))
+	}
+
+	return wh.validate(in)
+}
+
+func (wh *AttachedClusterWebhook) validate(in *v1alpha1.AttachedCluster) error {
+	var allErrs field.ErrorList
+
+	allErrs = append(allErrs, validateSecretKeyRef(in.Spec.Kubeconfig)...)
+
+	if len(allErrs) > 0 {
+		return apierrors.NewInvalid(v1alpha1.SchemeGroupVersion.WithKind("AttachedCluster").GroupKind(), in.Name, allErrs)
+	}
+
+	return nil
+}
+
+func validateSecretKeyRef(kubeconfig v1alpha1.SecretKeyRef) field.ErrorList {
+	var allErrs field.ErrorList
+
+	if kubeconfig.Name == "" {
+		allErrs = append(allErrs, field.Required(field.NewPath("spec", "kubeconfig", "name"), "must be set"))
+	} else {
+		allErrs = append(allErrs, validateDNS1123Domain(kubeconfig.Name, field.NewPath("spec", "kubeconfig", "name"))...)
+	}
+	if kubeconfig.Key == "" {
+		allErrs = append(allErrs, field.Required(field.NewPath("spec", "kubeconfig", "key"), "must be set"))
+	} else {
+		// The IsDNS1123Subdomain is used to validate the keys due to the portability and utility of this format across various contexts.
+		allErrs = append(allErrs, validateDNS1123Domain(kubeconfig.Key, field.NewPath("spec", "kubeconfig", "key"))...)
+	}
+
+	return allErrs
+}
+
+func (wh *AttachedClusterWebhook) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) error {
+	_, ok := oldObj.(*v1alpha1.AttachedCluster)
+	if !ok {
+		return apierrors.NewBadRequest(fmt.Sprintf("expected a AttachedCluster but got a %T", oldObj))
+	}
+
+	newAttachedCluster, ok := newObj.(*v1alpha1.AttachedCluster)
+	if !ok {
+		return apierrors.NewBadRequest(fmt.Sprintf("expected a AttachedCluster but got a %T", newObj))
+	}
+
+	return wh.validate(newAttachedCluster)
+}
+
+func (wh *AttachedClusterWebhook) ValidateDelete(_ context.Context, obj runtime.Object) error {
+	return nil
+}
diff --git a/pkg/webhooks/validation_utils.go b/pkg/webhooks/validation_utils.go
index 3e59271..ccab75d 100644
--- a/pkg/webhooks/validation_utils.go
+++ b/pkg/webhooks/validation_utils.go
@@ -46,7 +46,7 @@ func validateDNS1123Label(label string, fldPath *field.Path) field.ErrorList {
 }
 
 // validateDNS1123Domain checks if a string is a valid DNS1123 domain.
-// A lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-',
+// A lowercase RFC 1123 domain must consist of lower case alphanumeric characters, '-' or '.'
 // must start and end with an alphanumeric character,
 // and must have a maximum length of 253 characters.
 func validateDNS1123Domain(domain string, fldPath *field.Path) field.ErrorList {


Test patch:
diff --git a/pkg/webhooks/attachedcluster_webhook_test.go b/pkg/webhooks/attachedcluster_webhook_test.go
new file mode 100644
index 0000000..6b59b5b
--- /dev/null
+++ b/pkg/webhooks/attachedcluster_webhook_test.go
@@ -0,0 +1,73 @@
+/*
+Copyright Kurator Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package webhooks
+
+import (
+	"io/fs"
+	"os"
+	"path"
+	"path/filepath"
+	"testing"
+
+	. "github.com/onsi/gomega"
+	"github.com/stretchr/testify/assert"
+	"sigs.k8s.io/yaml"
+
+	"kurator.dev/kurator/pkg/apis/cluster/v1alpha1"
+)
+
+func TestInvalidAttachedClusterValidation(t *testing.T) {
+	r := path.Join("testdata", "attachedcluster")
+	caseNames := make([]string, 0)
+	err := filepath.WalkDir(r, func(path string, d fs.DirEntry, err error) error {
+		if d.IsDir() {
+			return nil
+		}
+
+		caseNames = append(caseNames, path)
+
+		return nil
+	})
+	assert.NoError(t, err)
+
+	wh := &AttachedClusterWebhook{}
+	for _, tt := range caseNames {
+		t.Run(tt, func(t *testing.T) {
+			g := NewWithT(t)
+			c, err := readAttachedCluster(tt)
+			g.Expect(err).NotTo(HaveOccurred())
+
+			err = wh.validate(c)
+			g.Expect(err).To(HaveOccurred())
+			t.Logf("%v", err)
+		})
+	}
+}
+
+func readAttachedCluster(filename string) (*v1alpha1.AttachedCluster, error) {
+	b, err := os.ReadFile(filename)
+	if err != nil {
+		return nil, err
+	}
+
+	c := &v1alpha1.AttachedCluster{}
+	if err := yaml.Unmarshal(b, c); err != nil {
+		return nil, err
+	}
+
+	return c, nil
+}
diff --git a/pkg/webhooks/testdata/attachedcluster/invalid-kubeconfig-name.yaml b/pkg/webhooks/testdata/attachedcluster/invalid-kubeconfig-name.yaml
new file mode 100644
index 0000000..e8dd6cd
--- /dev/null
+++ b/pkg/webhooks/testdata/attachedcluster/invalid-kubeconfig-name.yaml
@@ -0,0 +1,9 @@
+apiVersion: cluster.kurator.dev/v1alpha1
+kind: AttachedCluster
+metadata:
+  name: kurator-member1
+  namespace: default
+spec:
+  kubeconfig:
+    name: kurator-member1/a
+    key: kurator-member1.config
diff --git a/pkg/webhooks/testdata/attachedcluster/miss-kubeconfig-key.yaml b/pkg/webhooks/testdata/attachedcluster/miss-kubeconfig-key.yaml
new file mode 100644
index 0000000..00f3cb7
--- /dev/null
+++ b/pkg/webhooks/testdata/attachedcluster/miss-kubeconfig-key.yaml
@@ -0,0 +1,8 @@
+apiVersion: cluster.kurator.dev/v1alpha1
+kind: AttachedCluster
+metadata:
+  name: kurator-member1
+  namespace: default
+spec:
+  kubeconfig:
+    name: kurator-member1