Prevent mixing cached and uploaded content in repositories - #50
Conversation
65e9452 to
79b100b
Compare
|
There is still a case were both uploaded and cached content can coexist in the same Repo, it can happen after a user decides to change repo_type value after the repo is already linked to a distributions. Should repo_type field be immutable? This would cover such cases. |
I need to think about it and ask around to see if others have a similar use case. I had considered in the past that perhaps we could expose different types of repositories like this without two different models entirely, but I haven't thought through all of the implications. Probably this is the right choice for the first version of this feature though. |
aKlimau
changed the title
| ) | ||
|
|
||
| # A repository is the unit of exclusivity: it may be used for pull-through caching | ||
| # OR for uploads, but never both. |
| # OR for uploads, but never both. | ||
| if repository: | ||
| repo = repository.cast() | ||
| sibling_distributions = models.RustDistribution.objects.filter(repository=repo) |
There was a problem hiding this comment.
This seems fragile, at the very least the Distribution could probably be pointing to a repository version - "frozen" so to speak.
Which probably ought to also disable uploads?
There was a problem hiding this comment.
But why would anyone try to upload something to a repository version? We are trying to prevent mixing of cached and uploaded content and uploading to a repo version doesn't lead to it.
I'm open to other solutions, but I don't see an issue with this one.
There was a problem hiding this comment.
They're not uploading to the repository version, they're uploading to the distribution, which has a repo version set. And it shouldn't work. I think right now (not under this PR, I haven't checked, but master branch) it would upload to the repository and make new versions even though the distribution & distributed content doesn't change.
There was a problem hiding this comment.
If I understood the code correctly, uploading via cargo publish won't work if a distribution has repo_version set, since it relies on distro.repository having a value: https://github.com/pulp/pulp_rust/blob/main/pulp_rust/app/views.py#L347
And base distribution class doesn't allow for a distro to have both repo and repo_version set.
https://github.com/pulp/pulpcore/blob/main/pulpcore/app/serializers/publication.py#L315
Was that your concern or am I missing smt?
There was a problem hiding this comment.
I suppose that covers it, although it's not a very good error message for this specific case.
|
Go ahead and squash the commits |
| return self._error_response( | ||
| "This distribution serves a fixed repository version and cannot accept uploads.", | ||
| status=400, | ||
| ) |
There was a problem hiding this comment.
Factor this into a re-usable check. Actually the whole block possibly could be since the bottom line (353, 496, 543) is also basically the same in all cases.
|
|
||
|
|
||
| def repository_write_error(distro): | ||
| """Return a Cargo error response if the distribution can't be written to, else ``None``. |
There was a problem hiding this comment.
ditch the double-backticks
| "This distribution serves a fixed repository version and cannot be modified.", | ||
| status=400, | ||
| ) | ||
| return _cargo_error("no repository associated with this distribution", status=404) |
| @@ -434,11 +449,7 @@ def get_permissions(self): | |||
|
|
|||
| @staticmethod | |||
| def _error_response(detail, status=400): | |||
There was a problem hiding this comment.
Feels like _error_response can just be replaced by _cargo_error yes? Or just rename it in the first place, I don't see what is being added.
Closes pulp#28 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Prevent mixing cached and uploaded content in repositories.
When a distribution is created or updated, the serializer checks the repo's existing relationships - its own remote, and the remote/allow_uploads of any sibling distributions pointing at the same repo. If the operation results in mixing caching and uploads - it will be rejected.
Closes #28.
📜 Checklist
See: Pull Request Walkthrough