feat: restrict the membercluster name length to 63 #946
Conversation
jamyct
changed the title
config/crd/bases/cluster.kubernetes-fleet.io_memberclusters.yaml
Outdated
Show resolved
Hide resolved
config/crd/bases/cluster.kubernetes-fleet.io_memberclusters.yaml
Outdated
Show resolved
Hide resolved
|
Manual test for name length restriction:
Manual test for regex (as per RFC 1123 Label Names):
|
| Copyright (c) Microsoft Corporation. | ||
| Licensed under the MIT license. | ||
| */ | ||
| package e2e |
There was a problem hiding this comment.
please move this to integration test
There was a problem hiding this comment.
hi ryan, will the e2e tests be better here? The invalid MC is rejected by the CRD definition, while "The controller-runtime/pkg/envtest Go library helps write integration tests for your controllers by setting up and starting an instance of etcd and the Kubernetes API server, without kubelet, controller-manager or other components."
The ITs is better to test the controller behavior itself.
| "errors" | ||
| "fmt" | ||
|
|
||
| "k8s.io/apimachinery/pkg/types" | ||
|
|
||
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
|
|
||
| "reflect" | ||
|
|
||
| clusterv1beta1 "go.goms.io/fleet/apis/cluster/v1beta1" | ||
| rbacv1 "k8s.io/api/rbac/v1" | ||
| k8serrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" |
There was a problem hiding this comment.
please group the imports correctly
| }, | ||
| }, | ||
| } | ||
| Expect(impersonateHubClient.Create(ctx, memberClusterName)).Should(Succeed()) |
There was a problem hiding this comment.
| Expect(impersonateHubClient.Create(ctx, memberClusterName)).Should(Succeed()) | |
| Expect(hubClient.Create(ctx, memberClusterName)).Should(Succeed()) |
plese use hubClient instead and the imerpsonateHubClient is used for other test purpose.
| Expect(impersonateHubClient.Create(ctx, memberClusterName)).Should(Succeed()) | ||
| Expect(impersonateHubClient.Get(ctx, types.NamespacedName{Name: memberClusterName.Name}, memberClusterName)).Should(Succeed()) | ||
| Expect(impersonateHubClient.Delete(ctx, memberClusterName)).Should(Succeed()) | ||
| ensureMemberClusterAndRelatedResourcesDeletion(name) |
There was a problem hiding this comment.
This func will delete mc. Please remove line 66.
| }, | ||
| } | ||
| Expect(impersonateHubClient.Create(ctx, memberClusterName)).Should(Succeed()) | ||
| Expect(impersonateHubClient.Get(ctx, types.NamespacedName{Name: memberClusterName.Name}, memberClusterName)).Should(Succeed()) |
There was a problem hiding this comment.
you don't need to validate the get, since the reject should fail the create request.
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| var _ = Describe("Resource validation tests for Member Cluster", func() { |
There was a problem hiding this comment.
can you split the tests into two, one is to allow create and the other one is to deny to improve the readability.
| 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 must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')")) |
There was a problem hiding this comment.
no need to compare the full error strings and it's not reliable cause the package may change the error message.
Checking if have "a lowercase RFC 1123 subdomain" should be fine.
Description of your changes
restrict the membercluster name length to 63
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer