Implemented namespace deletion #1938

mssola · 2018-08-10T08:02:48Z

This commit fixes one of the oldest requests we've had: being able to
delete a namespace. It also introduces an API endpoint for it.

Fixes #767

Signed-off-by: Miquel Sabaté Solà [email protected]

mssola · 2018-08-10T08:04:21Z

To do:

Fix weird delay when deleting a namespace with repositories. This looks like one of those connection limits errors we've had sometimes...
Add UI button to delete namespaces 🙃 (now it's only done through API).

mssola · 2018-08-10T10:15:16Z

Fixed by raising the default puma workers. This is really ugly, but it looks unavoidable on a raw puma setup.

In order to improve this for 2.5, I propose to leave the task of deleting manifests (which is the part of the code taking connections) to the garbage collector. This way, deleting namespaces/images/tags will work on a DB level, and then the garbage collector will delete the dangling manifests. What do you think @vitoravelino ?

vitoravelino · 2018-08-10T10:25:08Z

In order to improve this for 2.5, I propose to leave the task of deleting manifests (which is the part of the code taking connections) to the garbage collector. This way, deleting namespaces/images/tags will work on a DB level, and then the garbage collector will delete the dangling manifests. What do you think?

It totally makes sense. This could also be applied when deleting a repository too. 👍

vitoravelino · 2018-08-10T12:52:14Z

app/policies/namespace_policy.rb

@@ -52,6 +52,16 @@ def create?
    (APP_CONFIG.enabled?("user_permission.create_namespace") || user.admin?) && push?
  end

+  def destroy?


This is allowing any namespace to be destroyed if user is admin and delete is enabled. That is not true as described in destroy service object code.

We can further discuss this on Monday. I believe that those checks have to be performed on the service, rather than on the policy. This is because an error on a personal namespace is, an error semantically speaking, not something disallowed for some users (i.e. related to authentication or authorization).

We need to fix this in the authorization level somehow to avoid showing the delete button when deletion would be denied. That's why I suggested it here.

I'm not sure I'm following 😕 In order to show/hide the delete button, you need some code in the authorization level? Isn't it possible to get this right from a helper somehow?

I do use a helper for that but I usually reuse the logic from the policies. That's what I've been doing for other cases. We've been strict in other policies rules but not in this one. What's the line between having stuff being done inside policies or not?

Fair point. The distinction is on the semantics imho. It's not about "you are not authorized to perform this", rather "what you are trying to do is not possible because of reason X". In this case, it's not possible because we don't want to implement personal namespace removal until we don't implement user removal.

mssola · 2018-08-10T18:18:59Z

Squashed. I've added a Signed-Off in the first commit 😉

mssola · 2018-08-13T13:03:31Z

app/assets/javascripts/modules/namespaces/components/delete-btn.vue

+
+      // TODO: refactor bootstrap popover to a component
+      $(this.$el).on('inserted.bs.popover', DELETE_BTN, () => {
+        console.log('delete carai');


@vitoravelino 🤣

mssola · 2018-08-13T14:28:41Z

lib/api/helpers/namespaces.rb

@@ -8,6 +8,10 @@ def can_manage_namespace?(namespace, user)
        NamespacePolicy.new(user, namespace).update?
      end

+      def can_destroy_namespace?(namespace, user)
+        NamespacePolicy.new(user, namespace).destroy?


@vitoravelino all that would take is to update this method so it also checked personal namespaces, no?

mssola · 2018-08-13T14:51:03Z

@vitoravelino I just implemented what I mentioned here. Could you review again?

vitoravelino · 2018-08-13T16:39:32Z

lib/api/helpers/namespaces.rb

@@ -8,6 +8,10 @@ def can_manage_namespace?(namespace, user)
        NamespacePolicy.new(user, namespace).update?
      end

+      def can_destroy_namespace?(namespace, user)
+        NamespacePolicy.new(user, namespace).destroy? && !User.exists?(namespace: namespace)


It worked partially. Just need to check for global namespaces to deny that too.

That is true. We are not checking this in neither the policy nor the service. Where should we put this check 🤔 ? I'd say that you could argue for both in this case:

If you put it on the policy, it means that no one is authorized to delete a global namespace. It means that this will never happen (e.g. it's not like the personal namespace case because in that case we may eventually allow that).

If you put it on the service, it means that what you are trying to do is semantically wrong, regardless of your credentials.

On this I would favor option 1, but I'm not sure. @vitoravelino what do you think?

@mssola option 1 sounds better.

vitoravelino

Besides my last comment, everything working as expected. This is super nice! Kudos! 🎉

This commit fixes one of the oldest requests we've had: being able to delete a namespace. It also introduces an API endpoint for it. There are two exceptions when it comes to deleting a namespace: 1. Personal namespaces cannot be removed. In the future we might implement user removal, and then it will make sense to re-iterate on this. 2. Global namespaces cannot be removed, because they are meant to be guaranteed by everyone using the registry. Fixes SUSE#767 Signed-off-by: Miquel Sabaté Solà <[email protected]> Signed-off-by: Vítor Avelino <[email protected]>

This is needed to keep up with the requirements of deleting namespaces. Signed-off-by: Miquel Sabaté Solà <[email protected]>

mssola · 2018-08-14T09:53:31Z

@vitoravelino done! Let's wait for Travis now 😄

mssola · 2018-08-14T11:32:37Z

And with that, there are no more features left for 2.4. Let's start hunting bugs until the final release! 😋

mssola requested a review from vitoravelino August 10, 2018 08:02

mssola mentioned this pull request Aug 10, 2018

Move deleting manifests into the background process #1939

Open

vitoravelino reviewed Aug 10, 2018

View reviewed changes

mssola force-pushed the delete-namespaces branch from b6b1294 to c9f679c Compare August 10, 2018 18:18

mssola force-pushed the delete-namespaces branch 2 times, most recently from 83133d6 to bb864eb Compare August 13, 2018 12:12

mssola commented Aug 13, 2018

View reviewed changes

mssola force-pushed the delete-namespaces branch from bb864eb to 9eb99ed Compare August 13, 2018 13:04

mssola commented Aug 13, 2018

View reviewed changes

mssola force-pushed the delete-namespaces branch from 9eb99ed to e6e2115 Compare August 13, 2018 14:50

vitoravelino reviewed Aug 13, 2018

View reviewed changes

vitoravelino previously approved these changes Aug 13, 2018

View reviewed changes

mssola added 2 commits August 14, 2018 11:49

config: raise the default puma workers number

990a04e

This is needed to keep up with the requirements of deleting namespaces. Signed-off-by: Miquel Sabaté Solà <[email protected]>

mssola dismissed vitoravelino’s stale review via 990a04e August 14, 2018 09:52

mssola force-pushed the delete-namespaces branch from e6e2115 to 990a04e Compare August 14, 2018 09:52

vitoravelino approved these changes Aug 14, 2018

View reviewed changes

vitoravelino merged commit eb33db6 into SUSE:master Aug 14, 2018

mssola deleted the delete-namespaces branch August 14, 2018 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented namespace deletion #1938

Implemented namespace deletion #1938

mssola commented Aug 10, 2018 •

edited

Loading

mssola commented Aug 10, 2018 •

edited by vitoravelino

Loading

mssola commented Aug 10, 2018 •

edited

Loading

vitoravelino commented Aug 10, 2018

vitoravelino Aug 10, 2018

mssola Aug 10, 2018

vitoravelino Aug 13, 2018

mssola Aug 13, 2018

vitoravelino Aug 13, 2018

mssola Aug 13, 2018 •

edited

Loading

mssola commented Aug 10, 2018

mssola Aug 13, 2018

vitoravelino Aug 13, 2018

mssola Aug 13, 2018

mssola commented Aug 13, 2018

vitoravelino Aug 13, 2018

mssola Aug 14, 2018

vitoravelino Aug 14, 2018

vitoravelino left a comment

mssola commented Aug 14, 2018

mssola commented Aug 14, 2018

Implemented namespace deletion #1938

Implemented namespace deletion #1938

Conversation

mssola commented Aug 10, 2018 • edited Loading

mssola commented Aug 10, 2018 • edited by vitoravelino Loading

mssola commented Aug 10, 2018 • edited Loading

vitoravelino commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssola Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

mssola commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssola commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitoravelino left a comment

Choose a reason for hiding this comment

mssola commented Aug 14, 2018

mssola commented Aug 14, 2018

mssola commented Aug 10, 2018 •

edited

Loading

mssola commented Aug 10, 2018 •

edited by vitoravelino

Loading

mssola commented Aug 10, 2018 •

edited

Loading

mssola Aug 13, 2018 •

edited

Loading