Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

HNC: consider making leaf subnamespace deletion easier #716

Closed
adrianludwin opened this issue May 13, 2020 · 5 comments · Fixed by #788
Closed

HNC: consider making leaf subnamespace deletion easier #716

adrianludwin opened this issue May 13, 2020 · 5 comments · Fixed by #788
Assignees
Milestone

Comments

@adrianludwin
Copy link
Contributor

Right now, to delete a subnamespace, you first have to set allowCascadingDelete on the subnamespace itself (or its parent). This makes subnamespaces harder to delete than regular namespaces, which have no such protection.

The goal of allowCascadingDelete was really to prevent cascading deletions - that is, unintentionally deleting multiple namespaces. I wonder if we could skip this check unless the namespace being deleted actually has subnamespaces itself. So for example, I could say:

k hns create sub -n parent
k delete subns sub -n parent

But not:

k hns create sub -n parent
k hns create subsub -n sub
k delete subns sub -n parent # fails because of subsub
@adrianludwin adrianludwin added this to the hnc-backlog milestone May 13, 2020
@adrianludwin
Copy link
Contributor Author

/assign @yiqigao217

This is on the HNC backlog so it's not immediately urgent (e.g. it's not for v0.4)

@yiqigao217
Copy link
Contributor

@adrianludwin WDYT if we also add webhook rules on denying delete namespace when it has children, since otherwise the children will be orphaned and they will have "parentMissing" condition, which is a condition that's easy to get since we dont have any webhook on that.

@rcolline
Copy link

rcolline commented Jun 4, 2020

(edited)

The issue here is that namespaces are cluster level resources so deleting and creating them requires cluster level permissions.

We use a proxy resource in the parent namespace (an "anchor", I think) to represent the child namespaces.

One option I thought of is to create a couple of resource bound roles for each namespace created by HNC. HNC can maintain the role bindings to these role based on RBAC permissions to the anchor resource.

With this, now deleting, updating and reading namespaces is fully natural.

Thoughts?

@adrianludwin
Copy link
Contributor Author

adrianludwin commented Jun 5, 2020 via email

GinnyJI added a commit to GinnyJI/multi-tenancy that referenced this issue Jul 27, 2020
Change the mustNotRun() function to use Eventually() so that we get
stable test result. Add sleep time after create subnamespace so that the
anchor is ready before test. Change the default wait time in mustRun()
and mustNotRun() to 5 seconds because deletign a subnamespace takes a
while.

Tested: make e2e-test
GinnyJI added a commit to GinnyJI/multi-tenancy that referenced this issue Jul 28, 2020
Change the mustNotRun() function to use Eventually() so that we get
stable test result. Add sleep time after create subnamespace so that the
anchor is ready before test. Change the default wait time in mustRun()
and mustNotRun() to 5 seconds because deleting a subnamespace takes a
while.

Tested: make e2e-test
GinnyJI added a commit to GinnyJI/multi-tenancy that referenced this issue Jul 28, 2020
Change the mustNotRun() function to use Eventually() so that we get
stable test result. Add sleep time after create subnamespace so that the
anchor is ready before test. Change the default wait time in mustRun()
and mustNotRun() to 5 seconds because deleting a subnamespace takes a
while.

Tested: make e2e-test
k8s-ci-robot added a commit that referenced this issue Jul 28, 2020
GinnyJI added a commit to GinnyJI/multi-tenancy that referenced this issue Nov 19, 2020
When deleting a parent namespace, we should allow it if none of its
children are sub-namespace.

In the E2E test, I moved the webhook deletion out of AfterEach, so that
if the user doesn't have HNC_REPAIR path set, it won't delete the
webhook.

Since we changed the deletion validator, one of the tests for issue kubernetes-retired#716
does not apply anymore. I deleted it in this PR.

Tested: make test-e2e
GinnyJI added a commit to GinnyJI/multi-tenancy that referenced this issue Nov 20, 2020
When deleting a parent namespace, we should allow it if none of its
children are sub-namespace.

In the E2E test, I moved the webhook deletion out of AfterEach, so that
if the user doesn't have HNC_REPAIR path set, it won't delete the
webhook.

Since we changed the deletion validator, one of the tests for issue kubernetes-retired#716
does not apply anymore. I deleted it in this PR.

Tested: make test-e2e
GinnyJI added a commit to GinnyJI/multi-tenancy that referenced this issue Nov 20, 2020
When deleting a parent namespace, we should allow it if none of its
children are sub-namespace.

In the E2E test, I moved the webhook deletion out of AfterEach, so that
if the user doesn't have HNC_REPAIR path set, it won't delete the
webhook.

Since we changed the deletion validator, one of the tests for issue kubernetes-retired#716
does not apply anymore. I deleted it in this PR.

Tested: make test-e2e
GinnyJI added a commit to GinnyJI/multi-tenancy that referenced this issue Nov 20, 2020
When deleting a parent namespace, we should allow it if none of its
children are sub-namespace.

In the E2E test, I moved the webhook deletion out of AfterEach, so that
if the user doesn't have HNC_REPAIR path set, it won't delete the
webhook.

Since we changed the deletion validator, one of the tests for issue kubernetes-retired#716
does not apply anymore. I deleted it in this PR.

Tested: make test-e2e
@akessner
Copy link

akessner commented Mar 8, 2023

ubnamespaces offline. We considered an alternative where the "SubnamespaceAnchor" becomes vestigial after the namespace is first created, and HNC create ClusterRoles and ClusterRoleBindings for the newly created child namespace (basically copying permissions from the SubnamespaceAnchor to the namespace). This means that the namespace can then be deleted via k delete ns child instead of k delete subns child -n parent. I hadn't considered this before so it's an interesting decision to write down. We agreed that this change isn't essential today (even if we agreed to make it) and we'll watch for signs that we should switch to this kind of scheme in the future.

We are having trouble with ArgoCD removing namespaces.
Was this new approach ever used?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants