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

Fixes #37552 - container push repo content view support #11028

Merged
merged 6 commits into from
Jul 9, 2024

Conversation

ianballou
Copy link
Member

What are the changes introduced in this pull request?

  • Allows publishing container push repositories in content views
  • Allows deletion of container push repositories
  • Adds authorization checks for container push
  • Skips metadata generation for container push repositories
  • Adds translations for container push errors

Considerations taken when implementing this change?

Other update actions on container push repositories will cause weirdness. I considered fixing that here too, but I think it'll be better for another PR. We'll need to change up the repo edit UI for container push repos.

What are the testing steps for this pull request?

  1. Push some container repos to Katello and publish them in a content view
  2. Try filtering content with CV publishes
  3. Try deleting container push repos
  4. Try pushing repositories as a user that shouldn't be able to (perhaps only has ability to create personal access tokens)
  5. Try regenerating metadata for container push repositories -- see that nothing fails

@ianballou
Copy link
Member Author

I realize I need to add some tests.

@sjha4 sjha4 self-requested a review June 25, 2024 15:43
@ianballou ianballou force-pushed the 37552-container-push-cv branch 3 times, most recently from aba03a1 to c301503 Compare June 26, 2024 19:48
@ianballou ianballou force-pushed the 37552-container-push-cv branch from 2cd4d6d to d49002b Compare June 26, 2024 21:51
@ianballou
Copy link
Member Author

One test should be fixed, a few more to go in registry proxies controller test. There are quite a few mocks to deal with :)

@ianballou ianballou force-pushed the 37552-container-push-cv branch from d49002b to e530777 Compare June 27, 2024 19:48
@ianballou
Copy link
Member Author

I made a bunch of test updates. I also made the decision that unauthenticated_pull should not allow unauthenticated pushing of repositories -- that sounds unsafe. For now we can just require authentication for any pushing of content. If people request it, we could add an unauthenticated push setting on LCEs.

@ianballou ianballou requested a review from sjha4 June 27, 2024 19:49
@ianballou ianballou force-pushed the 37552-container-push-cv branch from e530777 to 35a6448 Compare June 28, 2024 20:20
@ianballou ianballou force-pushed the 37552-container-push-cv branch from 35a6448 to 17d1606 Compare July 1, 2024 16:39
@ianballou ianballou force-pushed the 37552-container-push-cv branch from 17d1606 to 7e02baa Compare July 1, 2024 18:39
@sjha4
Copy link
Member

sjha4 commented Jul 1, 2024

Everything looks good here..Was able to go through the general CV workflows with container push repos.

One note above which is because after deleting the product, I was left with a container repo in pulp untracked by katello.

@ianballou
Copy link
Member Author

One note above which is because after deleting the product, I was left with a container repo in pulp untracked by katello.

Interesting, was the DeleteDistributions step skipped? If it is, then the Pulp repository won't get deleted. I'm curious if you can consistently reproduce the issue.

@sjha4
Copy link
Member

sjha4 commented Jul 2, 2024

Will retrace the steps but I think it wa the library push repo without any CVs that I tried deleting first and it left the library repo in pulp.

@sjha4
Copy link
Member

sjha4 commented Jul 3, 2024

So I tried reproducing and it happens only when the repo push actually fails. For ex: I tried a push with a v1 manifest podman pull quay.io/aptible/hello-world . That created the repo in katello/pulp and then deleting the katello repo left a pulp repo.

@ianballou ianballou force-pushed the 37552-container-push-cv branch from 98b2d5d to 36b4afb Compare July 5, 2024 17:11
@ianballou
Copy link
Member Author

@sjha4 I fixed the issue by allowing distributions to get looked up even if the manifest push part fails. Previously, the blob upload would succeed but the manifest upload would fail. This caused a Pulp repo to get created that Katello wasn't keeping proper track of.

@ianballou ianballou requested a review from sjha4 July 5, 2024 17:28
@sjha4
Copy link
Member

sjha4 commented Jul 9, 2024

I think there is a workflow that leads to an orphaned pulp repo. If you push a container repo, publish it in a CV version and then delete the repository from the repo details page while selecting remove from all CV versions, there is still a repo left in pulp db after the task. All the distributions get deleted though.

@ianballou
Copy link
Member Author

I think there is a workflow that leads to an orphaned pulp repo. If you push a container repo, publish it in a CV version and then delete the repository from the repo details page while selecting remove from all CV versions, there is still a repo left in pulp db after the task. All the distributions get deleted though.

I did a bit of digging and found out this is the normal behavior for published repositories in general. I was able to reproduce the same thing on a yum repository with filtered content. If actual repository copies are made, some empty shell of a repository is always left behind. When orphaned cleanup happens, all of its repository versions get deleted and just the repository is left.

Take a look at https://github.com/Katello/katello/blob/master/app/lib/actions/pulp3/orchestration/repository/delete.rb
Inside you'll see that a repository only gets deleted if repository.content_view.default?.
If repository.environment.nil?, we only delete the repository version, not the repository itself.

This is explained a bit in here: #8594

It seems that logic came from the fact that, if you deleted the CV version and it deleted the entire repository, it would break other CV versions.


The next question is then, what should we do about this bug(?). The repo shells left behind don't take up any space, but they do take up database entries and potential backend_object_name resources.

We might be able to delete these orphaned repositories if they have no repository versions left. Or, we could improve the repo deletion logic to determine that we are trying to purge this repository and thus delete all cloned repository versions.

I'm inclined to make this a separate issue and deal with it outside the scope of this PR so it's not rushed. Touching our repo deletion logic will need some special attention since it's a bit complicated.

Copy link
Member

@sjha4 sjha4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack..let's revisit the orphaned repo issue separately.

This PR looks good and works well..Able to do the usual push, repository and CV operations. 🚢

@ianballou ianballou merged commit 7246740 into Katello:master Jul 9, 2024
27 of 28 checks passed
@ianballou ianballou deleted the 37552-container-push-cv branch July 9, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants