feat: restrict the membercluster name length to 63 (#946)

* Restrict membercluster name to a maximum of 63 characters

* remove validation from v1alpha1 API

* add validation to APIs

* fix validation

* fix validation

* remove new type

* switch restrictions to metav1.ObjectMeta identities

* latest version

* Use self.metadata.name instead of self.name

* update CEL

* fix CEL line

* create e2e test file for name validation

* fix issues

* add regex e2e tests

* remove testing example

* fix k8serrors capital

* move from e2e to integration

* run goimports

* move back to e2e for timebeing

* add positive e2e cases

* run goimports

* fix import order & replace impersonateHubClient

* remove line to delete mc

* separate tests in two types: deny or allow

* shorten error string

* remove unnecessary get validation

* test v1 not v1beta1

* move tests to apis placement directory

* Add context

* removed unused variables from utils test file

* run goimports

* run goimports on suite test

* fix import order arrangement

* fix issues

* remove unused file

* remove unused constants

* run goimports

* switch import from placement to cluster directory

* add copyright banner to suit_test.go

apis/cluster/v1/membercluster_types.go

@@ -25,6 +25,7 @@ import (
 // +kubebuilder:printcolumn:JSONPath=`.status.resourceUsage.allocatable.memory`,name="Allocatable-Memory", priority=1, type=string
 
 // MemberCluster is a resource created in the hub cluster to represent a member cluster within a fleet.
+// +kubebuilder:validation:XValidation:rule="size(self.metadata.name) < 64",message="metadata.name max length is 63"
 type MemberCluster struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`

apis/cluster/v1beta1/membercluster_types.go

@@ -26,6 +26,7 @@ import (
 // +kubebuilder:printcolumn:JSONPath=`.status.resourceUsage.allocatable.memory`,name="Allocatable-Memory", priority=1, type=string
 
 // MemberCluster is a resource created in the hub cluster to represent a member cluster within a fleet.
+// +kubebuilder:validation:XValidation:rule="size(self.metadata.name) < 64",message="metadata.name max length is 63"
 type MemberCluster struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`

config/crd/bases/cluster.kubernetes-fleet.io_memberclusters.yaml

@@ -408,6 +408,9 @@ spec:
         required:
         - spec
         type: object
+        x-kubernetes-validations:
+        - message: metadata.name max length is 63
+          rule: size(self.metadata.name) < 64
     served: true
     storage: false
     subresources:
@@ -801,6 +804,9 @@ spec:
         required:
         - spec
         type: object
+        x-kubernetes-validations:
+        - message: metadata.name max length is 63
+          rule: size(self.metadata.name) < 64
     served: true
     storage: true
     subresources:

test/apis/cluster/v1/api_validation_integration_test.go

@@ -0,0 +1,217 @@
+/*
+Copyright (c) Microsoft Corporation.
+Licensed under the MIT license.
+*/
+package v1
+
+import (
+	"errors"
+	"fmt"
+	"reflect"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	rbacv1 "k8s.io/api/rbac/v1"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	clusterv1 "go.goms.io/fleet/apis/cluster/v1"
+)
+
+var _ = Describe("Test cluster v1 API validation", func() {
+	Context("Test MemberCluster API validation - invalid cases", func() {
+		It("should deny creating API with invalid name size", func() {
+			var name = "abcdef-123456789-123456789-123456789-123456789-123456789-123456789-123456789"
+			// Create the API.
+			memberClusterName := &clusterv1.MemberCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: name,
+				},
+				Spec: clusterv1.MemberClusterSpec{
+					Identity: rbacv1.Subject{
+						Name:      "fleet-member-agent-cluster-1",
+						Kind:      "ServiceAccount",
+						Namespace: "fleet-system",
+						APIGroup:  "",
+					},
+				},
+			}
+			By(fmt.Sprintf("expecting denial of CREATE API %s", name))
+			var err = hubClient.Create(ctx, memberClusterName)
+			var statusErr *k8serrors.StatusError
+			Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create API call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8serrors.StatusError{})))
+			Expect(statusErr.Status().Message).Should(ContainSubstring("metadata.name max length is 63"))
+		})
+
+		It("should deny creating API with invalid name starting with non-alphanumeric character", func() {
+			var name = "-abcdef-123456789-123456789-123456789-123456789-123456789"
+			// Create the API.
+			memberClusterName := &clusterv1.MemberCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: name,
+				},
+				Spec: clusterv1.MemberClusterSpec{
+					Identity: rbacv1.Subject{
+						Name:      "fleet-member-agent-cluster-1",
+						Kind:      "ServiceAccount",
+						Namespace: "fleet-system",
+						APIGroup:  "",
+					},
+				},
+			}
+			By(fmt.Sprintf("expecting denial of CREATE API %s", name))
+			err := hubClient.Create(ctx, memberClusterName)
+			var statusErr *k8serrors.StatusError
+			Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create API call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8serrors.StatusError{})))
+			Expect(statusErr.Status().Message).Should(ContainSubstring("a lowercase RFC 1123 subdomain"))
+		})
+
+		It("should deny creating API with invalid name ending with non-alphanumeric character", func() {
+			var name = "abcdef-123456789-123456789-123456789-123456789-123456789-"
+			// Create the API.
+			memberClusterName := &clusterv1.MemberCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: name,
+				},
+				Spec: clusterv1.MemberClusterSpec{
+					Identity: rbacv1.Subject{
+						Name:      "fleet-member-agent-cluster-1",
+						Kind:      "ServiceAccount",
+						Namespace: "fleet-system",
+						APIGroup:  "",
+					},
+				},
+			}
+			By(fmt.Sprintf("expecting denial of CREATE API %s", name))
+			err := hubClient.Create(ctx, memberClusterName)
+			var statusErr *k8serrors.StatusError
+			Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create API call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8serrors.StatusError{})))
+			Expect(statusErr.Status().Message).Should(ContainSubstring("a lowercase RFC 1123 subdomain"))
+		})
+
+		It("should deny creating API with invalid name containing character that is not alphanumeric and not -", func() {
+			var name = "a_bcdef-123456789-123456789-123456789-123456789-123456789-123456789-123456789"
+			// Create the API.
+			memberClusterName := &clusterv1.MemberCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: name,
+				},
+				Spec: clusterv1.MemberClusterSpec{
+					Identity: rbacv1.Subject{
+						Name:      "fleet-member-agent-cluster-1",
+						Kind:      "ServiceAccount",
+						Namespace: "fleet-system",
+						APIGroup:  "",
+					},
+				},
+			}
+			By(fmt.Sprintf("expecting denial of CREATE API %s", name))
+			err := hubClient.Create(ctx, memberClusterName)
+			var statusErr *k8serrors.StatusError
+			Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create API call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8serrors.StatusError{})))
+			Expect(statusErr.Status().Message).Should(ContainSubstring("a lowercase RFC 1123 subdomain"))
+		})
+	})
+
+	Context("Test Member Cluster creation API validation - valid cases", func() {
+		It("should allow creating API with valid name size", func() {
+			var name = "abc-123456789-123456789-123456789-123456789-123456789-123456789"
+			// Create the API.
+			memberClusterName := &clusterv1.MemberCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: name,
+				},
+				Spec: clusterv1.MemberClusterSpec{
+					Identity: rbacv1.Subject{
+						Name:      "fleet-member-agent-cluster-1",
+						Kind:      "ServiceAccount",
+						Namespace: "fleet-system",
+						APIGroup:  "",
+					},
+				},
+			}
+			Expect(hubClient.Create(ctx, memberClusterName)).Should(Succeed())
+			Expect(hubClient.Delete(ctx, memberClusterName)).Should(Succeed())
+		})
+
+		It("should allow creating API with valid name starting with alphabet character", func() {
+			var name = "abc-123456789"
+			// Create the API.
+			memberClusterName := &clusterv1.MemberCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: name,
+				},
+				Spec: clusterv1.MemberClusterSpec{
+					Identity: rbacv1.Subject{
+						Name:      "fleet-member-agent-cluster-1",
+						Kind:      "ServiceAccount",
+						Namespace: "fleet-system",
+						APIGroup:  "",
+					},
+				},
+			}
+			Expect(hubClient.Create(ctx, memberClusterName)).Should(Succeed())
+			Expect(hubClient.Delete(ctx, memberClusterName)).Should(Succeed())
+		})
+
+		It("should allow creating API with valid name starting with numeric character", func() {
+			var name = "123-123456789"
+			// Create the API.
+			memberClusterName := &clusterv1.MemberCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: name,
+				},
+				Spec: clusterv1.MemberClusterSpec{
+					Identity: rbacv1.Subject{
+						Name:      "fleet-member-agent-cluster-1",
+						Kind:      "ServiceAccount",
+						Namespace: "fleet-system",
+						APIGroup:  "",
+					},
+				},
+			}
+			Expect(hubClient.Create(ctx, memberClusterName)).Should(Succeed())
+			Expect(hubClient.Delete(ctx, memberClusterName)).Should(Succeed())
+		})
+
+		It("should allow creating API with valid name ending with alphabet character", func() {
+			var name = "123456789-abc"
+			// Create the API.
+			memberClusterName := &clusterv1.MemberCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: name,
+				},
+				Spec: clusterv1.MemberClusterSpec{
+					Identity: rbacv1.Subject{
+						Name:      "fleet-member-agent-cluster-1",
+						Kind:      "ServiceAccount",
+						Namespace: "fleet-system",
+						APIGroup:  "",
+					},
+				},
+			}
+			Expect(hubClient.Create(ctx, memberClusterName)).Should(Succeed())
+			Expect(hubClient.Delete(ctx, memberClusterName)).Should(Succeed())
+		})
+
+		It("should allow creating API with valid name ending with numeric character", func() {
+			var name = "123456789-123"
+			// Create the API.
+			memberClusterName := &clusterv1.MemberCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: name,
+				},
+				Spec: clusterv1.MemberClusterSpec{
+					Identity: rbacv1.Subject{
+						Name:      "fleet-member-agent-cluster-1",
+						Kind:      "ServiceAccount",
+						Namespace: "fleet-system",
+						APIGroup:  "",
+					},
+				},
+			}
+			Expect(hubClient.Create(ctx, memberClusterName)).Should(Succeed())
+			Expect(hubClient.Delete(ctx, memberClusterName)).Should(Succeed())
+		})
+	})
+})

test/apis/cluster/v1/suite_test.go

@@ -0,0 +1,92 @@
+/*
+Copyright (c) Microsoft Corporation.
+Licensed under the MIT license.
+*/
+package v1
+
+import (
+	"context"
+	"flag"
+	"path/filepath"
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/klog/v2"
+	"k8s.io/klog/v2/textlogger"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/envtest"
+	metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
+
+	clusterv1 "go.goms.io/fleet/apis/cluster/v1"
+)
+
+var (
+	hubTestEnv *envtest.Environment
+	hubClient  client.Client
+	ctx        context.Context
+	cancel     context.CancelFunc
+)
+
+func TestAPIs(t *testing.T) {
+	RegisterFailHandler(Fail)
+
+	RunSpecs(t, "ClusterResourcePlacement Controller Suite")
+}
+
+var _ = BeforeSuite(func() {
+	By("Setup klog")
+	fs := flag.NewFlagSet("klog", flag.ContinueOnError)
+	klog.InitFlags(fs)
+	Expect(fs.Parse([]string{"--v", "5", "-add_dir_header", "true"})).Should(Succeed())
+
+	ctx, cancel = context.WithCancel(context.TODO())
+
+	By("bootstrap the test environment")
+	// Start the cluster.
+	hubTestEnv = &envtest.Environment{
+		CRDDirectoryPaths: []string{
+			filepath.Join("..", "..", "..", "..", "config", "crd", "bases"),
+		},
+		ErrorIfCRDPathMissing: true,
+	}
+	hubCfg, err := hubTestEnv.Start()
+	Expect(err).NotTo(HaveOccurred())
+	Expect(hubCfg).NotTo(BeNil())
+
+	Expect(clusterv1.AddToScheme(scheme.Scheme)).Should(Succeed())
+
+	klog.InitFlags(flag.CommandLine)
+	flag.Parse()
+	// Create the hub controller manager.
+	hubCtrlMgr, err := ctrl.NewManager(hubCfg, ctrl.Options{
+		Scheme: scheme.Scheme,
+		Metrics: metricsserver.Options{
+			BindAddress: "0",
+		},
+		Logger: textlogger.NewLogger(textlogger.NewConfig(textlogger.Verbosity(4))),
+	})
+	Expect(err).NotTo(HaveOccurred())
+
+	// Set up the client.
+	// The client must be one with cache (i.e. configured by the controller manager) to make
+	// use of the cache indexes.
+	hubClient = hubCtrlMgr.GetClient()
+	Expect(hubClient).NotTo(BeNil())
+
+	go func() {
+		defer GinkgoRecover()
+		err = hubCtrlMgr.Start(ctx)
+		Expect(err).ToNot(HaveOccurred(), "failed to start manager for hub")
+	}()
+})
+
+var _ = AfterSuite(func() {
+	defer klog.Flush()
+	cancel()
+
+	By("tearing down the test environment")
+	Expect(hubTestEnv.Stop()).Should(Succeed())
+})