Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block the Remote removal if it is referred by a Distribution #5802

Open
git-hyagi opened this issue Sep 16, 2024 · 10 comments
Open

Block the Remote removal if it is referred by a Distribution #5802

git-hyagi opened this issue Sep 16, 2024 · 10 comments
Labels

Comments

@git-hyagi
Copy link
Contributor

Is your feature request related to a problem? Please describe.
If we remove a remote that is linked to a distribution, trying to pull non-synced content may fail.

Describe the solution you'd like
We could define the on_delete of Distribution remote attribute as models.Protect

remote = models.ForeignKey(Remote, null=True, on_delete=models.SET_NULL)

as a safeguard to ensure that there is no associated Distribution before removing the Remote.

Additional context
This is related to pulp/pulp_container#1729

@ipanova
Copy link
Member

ipanova commented Sep 16, 2024

I don't think this would be that easy, we have already discussed this topic. See this PR for more #2836

@git-hyagi
Copy link
Contributor Author

😲 thank you for the reference!
Since we already have another open issue to handle a similar scenario and there is still no consensus on how to proceed, I am closing this issue.

@mdellweg
Copy link
Member

I can see the analogy. But honestly, I would say this is a totally different scope.
On demand content is kind of out of control of the user, but remotes and distributions are directly the users concern. I think we can safely add this removal protection here.

@lubosmj
Copy link
Member

lubosmj commented Sep 17, 2024

Yes, I agree with @mdellweg. These two issues are not related and do not concern the same use case. Please, reassess its importance once again, @git-hyagi.

@ipanova
Copy link
Member

ipanova commented Sep 17, 2024

Would not consumption from distribution also fail if it had a repo referenced and it got removed?
We have more objects in pulp that are related and if the relation is removed things might break. So what do we do, do we audit the rest of them or treat each case differently?

Also, tangentially related. How do you proceed in this case:
UserA creates remoteA.
UserB has read access to remoteA. UserB creates DistributionB and uses remoteA.
UserA wants to remove remoteA and has zero access to DistributionB.

@mdellweg
Copy link
Member

Would not consumption from distribution also fail if it had a repo referenced and it got removed? We have more objects in pulp that are related and if the relation is removed things might break. So what do we do, do we audit the rest of them or treat each case differently?

Sure. This one is a good start for one of the individual cases.

Also, tangentially related. How do you proceed in this case: UserA creates remoteA. UserB has read access to remoteA. UserB creates DistributionB and uses remoteA. UserA wants to remove remoteA and has zero access to DistributionB.

Well UserA deliberately shared their remote with UserB. If there's a conflict, they need to talk. After all remotes are first level objects to the user. Remote artifacts are deeply hidden within the internal architecture. That is what renders these cases different for me.

@mdellweg mdellweg reopened this Sep 17, 2024
@ipanova
Copy link
Member

ipanova commented Sep 18, 2024

Would not consumption from distribution also fail if it had a repo referenced and it got removed? We have more objects in pulp that are related and if the relation is removed things might break. So what do we do, do we audit the rest of them or treat each case differently?

Sure. This one is a good start for one of the individual cases.

To be honest I personally think we should not add these restrictions, as we make things less flexible and RBAC on top of that makes things even worse.

Also, tangentially related. How do you proceed in this case: UserA creates remoteA. UserB has read access to remoteA. UserB creates DistributionB and uses remoteA. UserA wants to remove remoteA and has zero access to DistributionB.

Well UserA deliberately shared their remote with UserB. If there's a conflict, they need to talk.

Just because someone has a read scope on my object I am going to lose the full control over my object? That sounds a bit too much.
In addition, given that UserA does not have any access to DistributionB he would be able even to figure out what user exactly 'blocks' the remote removal unless that poor owner one-by-one starts knocking on the door of each user he gave the read scope of the remote and ask 'Hey, is not you by any chance blocking my remote removal?' And what if the userB says I need it, that's kinda nonsense that owner of an object needs to negotiate this.

And what about those user who have read access to remote but use it solely at sync POST operation?
Also one could permanently assign remote to the repo, that case kinda falls into the same bucket as relation between distribution and remote.

After all remotes are first level objects to the user. Remote artifacts are deeply hidden within the internal architecture. That is what renders these cases different for me.

@mdellweg
Copy link
Member

Well at the moment UserA onwning the remote can break UserB's workflow by deleting the remote without even being notified. I would say that is worse. Specifically for userB coming back from a 2 weeks leave being completely astonished and having a hard time pulling all the post mortem information together.
What i hear is we'd need a way to either tell the userA about repositories linking to their remote and/or allow force removing it anyway.

@ipanova
Copy link
Member

ipanova commented Sep 18, 2024

You don't usually pass body that could contain force:true to the DELETE request.

@ipanova
Copy link
Member

ipanova commented Sep 18, 2024

What you're describing is more like shared and exclusive locking logic, which I don't think it should be applied here.
IMO we should follow the filesystem approach.

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

No branches or pull requests

5 participants