From e764b976aab2a72b43316c46bcc9160a12cc513c Mon Sep 17 00:00:00 2001 From: Dario Tranchitella Date: Sat, 31 Oct 2020 19:43:46 +0100 Subject: [PATCH] Allowing dash on Tenant namespace (#118) * Allowing dashes in the Tenant name as DNS RFC-1123 * Allowing force tenant prefix with Namespaces with dash --- e2e/custom_capsule_group_test.go | 2 +- e2e/force_tenant_prefix_test.go | 69 +++++++++++--------- e2e/ingress_class_test.go | 2 +- e2e/namespace_metadata_test.go | 2 +- e2e/new_namespace_test.go | 2 +- e2e/overquota_namespace_test.go | 2 +- e2e/owner_webhooks_test.go | 2 +- e2e/protected_namespace_regex_test.go | 2 +- e2e/resource_quota_exceeded_test.go | 2 +- e2e/selecting_non_owned_tenant_test.go | 2 +- e2e/selecting_tenant_fail_test.go | 8 +-- e2e/selecting_tenant_with_label_test.go | 4 +- e2e/service_metadata_test.go | 2 +- e2e/storage_class_test.go | 2 +- e2e/tenant_name_webhook_test.go | 2 +- e2e/tenant_owner_group_test.go | 2 +- e2e/tenant_resources_changes_test.go | 2 +- e2e/tenant_resources_test.go | 2 +- pkg/webhook/owner_reference/patching.go | 86 ++++++++++--------------- pkg/webhook/tenant/validating.go | 2 +- 20 files changed, 94 insertions(+), 105 deletions(-) diff --git a/e2e/custom_capsule_group_test.go b/e2e/custom_capsule_group_test.go index c3716964..73893625 100644 --- a/e2e/custom_capsule_group_test.go +++ b/e2e/custom_capsule_group_test.go @@ -32,7 +32,7 @@ import ( var _ = Describe("creating a Namespace as Tenant owner with custom --capsule-group", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantassignedcustomgroup", + Name: "tenant-assigned-custom-group", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/force_tenant_prefix_test.go b/e2e/force_tenant_prefix_test.go index 73b547da..e1dfcd76 100644 --- a/e2e/force_tenant_prefix_test.go +++ b/e2e/force_tenant_prefix_test.go @@ -49,52 +49,57 @@ var _ = Describe("creating a Namespace with --force-tenant-name flag", func() { ResourceQuota: []corev1.ResourceQuotaSpec{}, }, } - t2 := &v1alpha1.Tenant{ - ObjectMeta: metav1.ObjectMeta{ - Name: "second", - }, - Spec: v1alpha1.TenantSpec{ - Owner: v1alpha1.OwnerSpec{ - Name: "john", - Kind: "User", - }, - NamespacesMetadata: v1alpha1.AdditionalMetadata{}, - ServicesMetadata: v1alpha1.AdditionalMetadata{}, - IngressClasses: v1alpha1.IngressClassesSpec{}, - StorageClasses: v1alpha1.StorageClassesSpec{}, - LimitRanges: []corev1.LimitRangeSpec{}, - NamespaceQuota: 10, - NodeSelector: map[string]string{}, - ResourceQuota: []corev1.ResourceQuotaSpec{}, - }, - } JustBeforeEach(func() { EventuallyCreation(func() error { t1.ResourceVersion = "" return k8sClient.Create(context.TODO(), t1) }).Should(Succeed()) - EventuallyCreation(func() error { - t2.ResourceVersion = "" - return k8sClient.Create(context.TODO(), t2) - }).Should(Succeed()) + ModifyCapsuleManagerPodArgs(append(defaulManagerPodArgs, []string{"--force-tenant-prefix"}...)) }) JustAfterEach(func() { Expect(k8sClient.Delete(context.TODO(), t1)).Should(Succeed()) - Expect(k8sClient.Delete(context.TODO(), t2)).Should(Succeed()) + ModifyCapsuleManagerPodArgs(defaulManagerPodArgs) }) - It("should fail", func() { - args := append(defaulManagerPodArgs, []string{"--force-tenant-prefix"}...) - ModifyCapsuleManagerPodArgs(args) + It("should fail with missing prefix", func() { ns := NewNamespace("test") NamespaceCreation(ns, t1, podRecreationTimeoutInterval).ShouldNot(Succeed()) }) - It("should be assigned to the second Tenant", func() { + It("should succeed using prefix", func() { + ns := NewNamespace("first-test") + NamespaceCreation(ns, t1, podRecreationTimeoutInterval).Should(Succeed()) + }) + It("should fail with multiple Tenant assigned to user", func() { + t2 := &v1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "second", + }, + Spec: v1alpha1.TenantSpec{ + Owner: v1alpha1.OwnerSpec{ + Name: "john", + Kind: "User", + }, + NamespacesMetadata: v1alpha1.AdditionalMetadata{}, + ServicesMetadata: v1alpha1.AdditionalMetadata{}, + IngressClasses: v1alpha1.IngressClassesSpec{}, + StorageClasses: v1alpha1.StorageClassesSpec{}, + LimitRanges: []corev1.LimitRangeSpec{}, + NamespaceQuota: 10, + NodeSelector: map[string]string{}, + ResourceQuota: []corev1.ResourceQuotaSpec{}, + }, + } + + EventuallyCreation(func() error { + return k8sClient.Create(context.TODO(), t2) + }).Should(Succeed()) + + defer func() { + Expect(k8sClient.Delete(context.TODO(), t2)).Should(Succeed()) + }() + ns := NewNamespace("second-test") ns2 := NewNamespace("second-test2") - NamespaceCreation(ns, t2, podRecreationTimeoutInterval).Should(Succeed()) - TenantNamespaceList(t2, podRecreationTimeoutInterval).Should(ContainElement(ns.GetName())) + NamespaceCreation(ns, t2, podRecreationTimeoutInterval).ShouldNot(Succeed()) NamespaceCreation(ns2, t1, podRecreationTimeoutInterval).ShouldNot(Succeed()) - args := defaulManagerPodArgs - ModifyCapsuleManagerPodArgs(args) }) }) diff --git a/e2e/ingress_class_test.go b/e2e/ingress_class_test.go index 659da8ea..2b5dfe4b 100644 --- a/e2e/ingress_class_test.go +++ b/e2e/ingress_class_test.go @@ -33,7 +33,7 @@ import ( var _ = Describe("when Tenant handles Ingress classes", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "ingressclass", + Name: "ingress-class", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/namespace_metadata_test.go b/e2e/namespace_metadata_test.go index 249735e3..f89614d6 100644 --- a/e2e/namespace_metadata_test.go +++ b/e2e/namespace_metadata_test.go @@ -33,7 +33,7 @@ import ( var _ = Describe("creating a Namespace for a Tenant with additional metadata", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantmetadata", + Name: "tenant-metadata", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/new_namespace_test.go b/e2e/new_namespace_test.go index ad4f8baa..1d436a46 100644 --- a/e2e/new_namespace_test.go +++ b/e2e/new_namespace_test.go @@ -32,7 +32,7 @@ import ( var _ = Describe("creating a Namespace as Tenant owner", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantassigned", + Name: "tenant-assigned", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/overquota_namespace_test.go b/e2e/overquota_namespace_test.go index ab435ad1..cef66078 100644 --- a/e2e/overquota_namespace_test.go +++ b/e2e/overquota_namespace_test.go @@ -32,7 +32,7 @@ import ( var _ = Describe("creating a Namespace over-quota", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "overquotatenant", + Name: "over-quota-tenant", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/owner_webhooks_test.go b/e2e/owner_webhooks_test.go index 5c980e2e..c55376ed 100644 --- a/e2e/owner_webhooks_test.go +++ b/e2e/owner_webhooks_test.go @@ -36,7 +36,7 @@ import ( var _ = Describe("when Tenant owner interacts with the webhooks", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantowner", + Name: "tenant-owner", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/protected_namespace_regex_test.go b/e2e/protected_namespace_regex_test.go index 3d64d1e6..0631f35c 100644 --- a/e2e/protected_namespace_regex_test.go +++ b/e2e/protected_namespace_regex_test.go @@ -32,7 +32,7 @@ import ( var _ = Describe("creating a Namespace with --protected-namespace-regex enabled", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantprotectednamespace", + Name: "tenant-protected-namespace", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/resource_quota_exceeded_test.go b/e2e/resource_quota_exceeded_test.go index 31889681..9acbd78a 100644 --- a/e2e/resource_quota_exceeded_test.go +++ b/e2e/resource_quota_exceeded_test.go @@ -39,7 +39,7 @@ import ( var _ = Describe("exceeding Tenant resource quota", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantresourceschanges", + Name: "tenant-resources-changes", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/selecting_non_owned_tenant_test.go b/e2e/selecting_non_owned_tenant_test.go index 7751b66c..e32c3822 100644 --- a/e2e/selecting_non_owned_tenant_test.go +++ b/e2e/selecting_non_owned_tenant_test.go @@ -32,7 +32,7 @@ import ( var _ = Describe("creating a Namespace trying to select a third Tenant", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantnonowned", + Name: "tenant-non-owned", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/selecting_tenant_fail_test.go b/e2e/selecting_tenant_fail_test.go index 855ca38c..4682cdfa 100644 --- a/e2e/selecting_tenant_fail_test.go +++ b/e2e/selecting_tenant_fail_test.go @@ -32,7 +32,7 @@ import ( var _ = Describe("creating a Namespace without a Tenant selector when user owns multiple Tenants", func() { t1 := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantone", + Name: "tenant-one", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ @@ -51,7 +51,7 @@ var _ = Describe("creating a Namespace without a Tenant selector when user owns } t2 := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenanttwo", + Name: "tenant-two", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ @@ -70,7 +70,7 @@ var _ = Describe("creating a Namespace without a Tenant selector when user owns } t3 := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantthree", + Name: "tenant-three", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ @@ -89,7 +89,7 @@ var _ = Describe("creating a Namespace without a Tenant selector when user owns } t4 := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantfour", + Name: "tenant-four", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/selecting_tenant_with_label_test.go b/e2e/selecting_tenant_with_label_test.go index 7c88c068..94ce4afa 100644 --- a/e2e/selecting_tenant_with_label_test.go +++ b/e2e/selecting_tenant_with_label_test.go @@ -32,7 +32,7 @@ import ( var _ = Describe("creating a Namespace with Tenant selector when user owns multiple tenants", func() { t1 := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantone", + Name: "tenant-one", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ @@ -51,7 +51,7 @@ var _ = Describe("creating a Namespace with Tenant selector when user owns multi } t2 := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenanttwo", + Name: "tenant-two", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/service_metadata_test.go b/e2e/service_metadata_test.go index f9584951..51d299b3 100644 --- a/e2e/service_metadata_test.go +++ b/e2e/service_metadata_test.go @@ -34,7 +34,7 @@ import ( var _ = Describe("creating a Service/Endpoint/EndpointSlice for a Tenant with additional metadata", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "servicemetadata", + Name: "service-metadata", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/storage_class_test.go b/e2e/storage_class_test.go index 82ef3d15..30d8144c 100644 --- a/e2e/storage_class_test.go +++ b/e2e/storage_class_test.go @@ -32,7 +32,7 @@ import ( var _ = Describe("when Tenant handles Storage classes", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "storageclass", + Name: "storage-class", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/tenant_name_webhook_test.go b/e2e/tenant_name_webhook_test.go index a23a7e56..0d300123 100644 --- a/e2e/tenant_name_webhook_test.go +++ b/e2e/tenant_name_webhook_test.go @@ -32,7 +32,7 @@ import ( var _ = Describe("creating a Tenant with wrong name", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "wrong-name", + Name: "non_rfc_dns_1123", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/tenant_owner_group_test.go b/e2e/tenant_owner_group_test.go index a41cad63..7095cda0 100644 --- a/e2e/tenant_owner_group_test.go +++ b/e2e/tenant_owner_group_test.go @@ -32,7 +32,7 @@ import ( var _ = Describe("creating a Namespace with group Tenant owner", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantgroupowner", + Name: "tenant-group-owner", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/tenant_resources_changes_test.go b/e2e/tenant_resources_changes_test.go index d5a12cd3..464cd4f8 100644 --- a/e2e/tenant_resources_changes_test.go +++ b/e2e/tenant_resources_changes_test.go @@ -37,7 +37,7 @@ import ( var _ = Describe("changing Tenant managed Kubernetes resources", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantresourceschanges", + Name: "tenant-resources-changes", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/e2e/tenant_resources_test.go b/e2e/tenant_resources_test.go index 42b19b2c..cfd4f214 100644 --- a/e2e/tenant_resources_test.go +++ b/e2e/tenant_resources_test.go @@ -38,7 +38,7 @@ import ( var _ = Describe("creating namespaces within a Tenant with resources", func() { tnt := &v1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "tenantresources", + Name: "tenant-resources", }, Spec: v1alpha1.TenantSpec{ Owner: v1alpha1.OwnerSpec{ diff --git a/pkg/webhook/owner_reference/patching.go b/pkg/webhook/owner_reference/patching.go index 2ebc228e..902a01e9 100644 --- a/pkg/webhook/owner_reference/patching.go +++ b/pkg/webhook/owner_reference/patching.go @@ -79,73 +79,57 @@ func (h *handler) OnCreate(clt client.Client, decoder *admission.Decoder) capsul return admission.Errored(http.StatusBadRequest, err) } // If we already had TenantName label on NS -> assign to it - if len(ns.ObjectMeta.Labels) > 0 { - l, ok := ns.ObjectMeta.Labels[ln] - // assigning namespace to Tenant in case of label - if ok { - // retrieving the selected Tenant - t := &capsulev1alpha1.Tenant{} - if err := clt.Get(ctx, types.NamespacedName{Name: l}, t); err != nil { - return admission.Errored(http.StatusBadRequest, err) - } - // Tenant owner must adhere to user that asked for NS creation - if !h.isTenantOwner(t.Spec.Owner, req.UserInfo) { - return admission.Denied("Cannot assign the desired namespace to a non-owned Tenant") - } - // Patching the response - return h.patchResponseForOwnerRef(t, ns) - } - - } - // If we forceTenantPrefix -> find Tenant from NS name - if h.forceTenantPrefix { - t := &v1alpha1.Tenant{} - tenantName := strings.Split(ns.GetName(), "-")[0] - if err := clt.Get(ctx, types.NamespacedName{Name: tenantName}, t); err != nil { + if l, ok := ns.ObjectMeta.Labels[ln]; ok { + // retrieving the selected Tenant + t := &capsulev1alpha1.Tenant{} + if err := clt.Get(ctx, types.NamespacedName{Name: l}, t); err != nil { return admission.Errored(http.StatusBadRequest, err) } + // Tenant owner must adhere to user that asked for NS creation + if !h.isTenantOwner(t.Spec.Owner, req.UserInfo) { + return admission.Denied("Cannot assign the desired namespace to a non-owned Tenant") + } + // Patching the response return h.patchResponseForOwnerRef(t, ns) } - tenants := []*capsulev1alpha1.Tenant{} + // If we forceTenantPrefix -> find Tenant from NS name + var tenants []*capsulev1alpha1.Tenant // Find tenants belonging to user - tlu, err := h.listTenantsForOwnerKind(ctx, "User", req.UserInfo.Username, clt) - if err != nil { - return admission.Errored(http.StatusBadRequest, err) - } - // No groups single tenant short-circuit - if len(req.UserInfo.Groups) == 0 && len(tlu.Items) == 1 { - return h.patchResponseForOwnerRef(&tlu.Items[0], ns) - } - - switch userTenants := len(tlu.Items); { - case userTenants > 1: - return admission.Denied("Unable to assign namespace to tenant. Please use " + ln + " label when creating a namespace") - case userTenants == 1: - tenants = append(tenants, &tlu.Items[0]) - } - - for _, group := range req.UserInfo.Groups { - tl, err := h.listTenantsForOwnerKind(ctx, "Group", group, clt) + { + tl, err := h.listTenantsForOwnerKind(ctx, "User", req.UserInfo.Username, clt) if err != nil { return admission.Errored(http.StatusBadRequest, err) } - for _, item := range tl.Items { - tenants = append(tenants, &item) + for _, tnt := range tl.Items { + tenants = append(tenants, &tnt) } - // more than one tenant found, returning error - if len(tenants) > 1 { - return admission.Denied("Unable to assign namespace to tenant. Please use " + ln + " label when creating a namespace") + } + // Find tenants belonging to user groups + { + for _, group := range req.UserInfo.Groups { + tl, err := h.listTenantsForOwnerKind(ctx, "Group", group, clt) + if err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + for _, tnt := range tl.Items { + tenants = append(tenants, &tnt) + } } } - // Single tenant found for group - if len(tenants) == 1 { + switch len(tenants) { + case 0: + return admission.Denied("You do not have any Tenant assigned: please, reach out the system administrators") + case 1: + if h.forceTenantPrefix && !strings.HasPrefix(ns.GetName(), tenants[0].GetName()) { + return admission.Denied("The namespace doesn't match any assigned tenants prefix. Please, use the desired tenant name as prefix followed by a dash when creating a namespace") + } return h.patchResponseForOwnerRef(tenants[0], ns) + default: + return admission.Denied("Unable to assign namespace to tenant. Please use " + ln + " label when creating a namespace") } - - return admission.Denied("You do not have any Tenant assigned: please, reach out the system administrators") } } diff --git a/pkg/webhook/tenant/validating.go b/pkg/webhook/tenant/validating.go index 672df2bf..f7c8e20f 100644 --- a/pkg/webhook/tenant/validating.go +++ b/pkg/webhook/tenant/validating.go @@ -64,7 +64,7 @@ func (r *handler) OnCreate(client client.Client, decoder *admission.Decoder) cap return admission.Errored(http.StatusBadRequest, err) } - matched, _ := regexp.MatchString(`^[a-z0-9]([a-z0-9]*[a-z0-9])?$`, tnt.GetName()) + matched, _ := regexp.MatchString(`[a-z0-9]([-a-z0-9]*[a-z0-9])?`, tnt.GetName()) if !matched { return admission.Denied("Tenant name has forbidden characters") }