Red Hat Satellite engineering is moving the tracking of its product development work on Satellite to Red Hat Jira (issues.redhat.com). If you're a Red Hat customer, please continue to file support cases via the Red Hat customer portal. If you're not, please head to the "Satellite project" in Red Hat Jira and file new tickets here. Individual Bugzilla bugs will be migrated starting at the end of May. If you cannot log in to RH Jira, please consult article #7032570. That failing, please send an e-mail to the RH Jira admins at rh-issues@redhat.com to troubleshoot your issue as a user management inquiry. The email creates a ServiceNow ticket with Red Hat. Individual Bugzilla bugs that are migrated will be moved to status "CLOSED", resolution "MIGRATED", and set with "MigratedToJIRA" in "Keywords". The link to the successor Jira issue will be found under "Links", have a little "two-footprint" icon next to it, and direct you to the "Satellite project" in Red Hat Jira (issue links are of type "https://issues.redhat.com/browse/SAT-XXXX", where "X" is a digit). This same link will be available in a blue banner at the top of the page informing you that that bug has been migrated.
Bug 2107079 - Receiving error while uploading RPMs (multiple RPMs) using hammer command
Summary: Receiving error while uploading RPMs (multiple RPMs) using hammer command
Keywords:
Status: CLOSED DUPLICATE of bug 1313502
Alias: None
Product: Red Hat Satellite
Classification: Red Hat
Component: Repositories
Version: 6.10.0
Hardware: Unspecified
OS: Unspecified
unspecified
medium
Target Milestone: 6.15.0
Assignee: Samir Jha
QA Contact: Satellite QE Team
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2022-07-14 09:43 UTC by Shekhar Raut
Modified: 2023-12-19 14:48 UTC (History)
8 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2023-12-19 14:48:56 UTC
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Issue Tracker SAT-18296 0 None None None 2023-06-12 18:35:43 UTC

Description Shekhar Raut 2022-07-14 09:43:12 UTC

Description of problem:

Receiving error while uploading RPMs (multiple RPMs) using hammer command

Or rpm will upload to random repositories.

Version-Release number of selected component (if applicable):

~~~
satellite-6.10.5.1-1.el7sat.noarch

tfm-rubygem-hammer_cli-2.5.1-1.el7sat.noarch
tfm-rubygem-hammer_cli_foreman-2.5.1.1-1.el7sat.noarch
~~~

How reproducible:

~~~
$ hammer repository upload-content --content-type rpm --path ~/test-package-1-3.el8.noarch.rpm --product-id 32 --organization-id 3 --id 52268

Successfully uploaded file 'test-package-1-3.el8.noarch.rpm'.

--> test-package-1-3.el8.noarch.rpm is now in the RHEL8_MAX_CustomPkgs repo and not the RHEL7_MAX_CustomPkgs repo.

$ hammer repository upload-content --content-type rpm --path ~/test-package-1-4.el7.noarch.rpm --product-id 32 --organization-id 3 --id 105
Failed to upload file 'test-package-1-4.el7.noarch.rpm' to repository.  Please check the file and try again.

--> test-package-1-3.el8.noarch.rpm is now in both the RHEL8_MAX_CustomPkgs and RHEL7_MAX_CustomPkgs repos.  test-package-1-4.el7.noarch.rpm is not in either repo.
~~~

Actual results:

Task was failed with error:

~~~

PulpcoreClient::ApiError

Error message: the server returns an error HTTP status code: 404 Response headers: {"Date"=>"Wed, 15 Jun 2022 13:16:35 GMT", "Server"=>"gunicorn", "Content-Type"=>"application/json", "Vary"=>"Accept,Cookie", "Allow"=>"GET, HEAD, PUT, DELETE", "X-Frame-Options"=>"SAMEORIGIN", "Content-Length"=>"23", "Correlation-ID"=>"c3b83150-e915-4d92-a48c-6fe4b532b3c1", "Access-Control-Expose-Headers"=>"Correlation-ID", "Via"=>"1.1 satellite.example.com"} Response body: {"detail":"Not found."}

~~~

Expected results:

RPM should upload without any error.

Comment 2 Paul Donohue 2022-07-14 14:59:05 UTC
Some clarification and additional details:

We actually ran into this bug while using an old version of the hammer CLI with Satellite 6.10, and upgrading to the newer version of the hammer CLI that is listed above fixed it.  However, we also have some home-grown scripts that use the Satellite API directly to upload files, and our scripts were affected by the same issue (our scripts worked fine on Satellite 6.9, but then ran into intermittent problems on Satellite 6.10).

The underlying problem is that the import_upload API does not function properly if the file checksum is not provided.  This can lead to import_upload either failing or importing the wrong upload into the specified repository.
The API (and associated documentation) considers the file checksum to be optional, so I consider the fact that the API fails when the checksum is not provided to be a bug in the API.

The reason both the old version of hammer and our home-grown scripts were affected is that they were not including the file checksum in calls to import_upload, and therefore were running into this bug in the API.  The newer version of hammer always includes the file checksum.


More specifics on the API bug:
* https://github.com/Katello/katello/blob/master/app/lib/actions/pulp3/orchestration/repository/import_upload.rb#L25 plans a CommitUpload action, then plans a SaveArtifact action and passes it commit_output[:pulp_tasks].
* commit_output[:pulp_tasks][0][:created_resources][0] contains the href for the artifact created by CommitUpload.
* However, https://github.com/Katello/katello/blob/master/app/lib/actions/pulp3/repository/save_artifact.rb#L14 doesn't use that href returned by CommitUpload.  Instead, it checks input[:options][:artifact_href] (which is always missing when this code is called via the import_uploads API), then calls fetch_artifact_href() to get the href.
* fetch_artifact_href() calls artifacts_api.list("sha256": input[:options][:sha256]).  If the checksum was not included in the API call then input[:options][:sha256] is nil.
* artifacts_api.list("sha256": nil) returns *all* of the uploaded artifacts.
* fetch_artifact_href() then selects the first artifact from the returned list and uses the href from that artifact.
* Thus, if the checksum is not provided to the import_uploads API call (which is permitted according to the docs), then the SaveArtifact action may select the wrong artifact, resulting in either an error or the wrong uploaded file being imported into the repository.

Comment 3 Paul Donohue 2022-07-14 15:09:46 UTC
I suspect a simple fix here is to:
1) Adjust https://github.com/Katello/katello/blob/master/app/lib/actions/pulp3/repository/save_artifact.rb#L14 to use commit_output[:pulp_tasks][0][:created_resources][0] as the artifact href whenever possible.
2) Adjust fetch_artifact_href() so that it will never retrieve the full list of artifacts and select the first.  At a minimum, it should probably throw an error if doesn't have enough information to correctly select an artifact (if input[:options][:sha256] is nil).  It would probably make sense to have it be able to search by name or checksum or both, and throw an error if neither the name nor checksum is available.

Comment 4 Paul Dudley 2022-10-21 18:46:17 UTC
Just to note; this looks similar if not the same as https://bugzilla.redhat.com/show_bug.cgi?id=2068454 and looks like it should be solved in 6.12.

Comment 5 Paul Donohue 2023-06-03 00:48:12 UTC
https://bugzilla.redhat.com/show_bug.cgi?id=2068454 fixes this for everything but docker uploads (by requiring the checksum, which prevents anyone from following the broken code path that is used when the checksum is not provided).

However, the broken code path is still present, and can still be hit if a docker upload is performed without providing a checksum.

Comment 7 Ian Ballou 2023-06-12 18:26:58 UTC
To recap, the closing criteria for this BZ is now to ensure that the situation in Comment 2 cannot be hit by uploading a docker manifest without specifying a checksum.

Comment 8 Brad Buckingham 2023-10-30 11:29:29 UTC
Bulk setting Target Milestone = 6.15.0 where sat-6.15.0+ is set.

Comment 10 Samir Jha 2023-11-17 15:53:06 UTC
We have the updated docs for API uploads here: https://access.redhat.com/documentation/en-us/red_hat_satellite/6.10/html/api_guide/chap-red_hat_satellite-api_guide-using_the_red_hat_satellite_api#sect-API_Guide-Uploading_Content_to_the_Satellite_Server

The use of checksum is new from pulp2->pulp3 and helps pulp/katello validate the file being uploaded. Can the home-grown scripts be updated to pass along the checksum of files being uploaded as require by the API?

Comment 11 Paul Donohue 2023-11-19 15:27:13 UTC
The generated API docs still say that the checksum is optional: https://satellite/apidoc/v2/repositories/import_uploads.en.html
And from a quick scan through the code, it looks like like the checksum is still not required when uploading a docker manifest, and this broken code path is still present, so it is still possible to hit this broken code when uploading a docker manifest.

Of course, I updated my scripts to unconditionally include the checksum (around the time I discovered this broken code path back in 2022).  However, that doesn't mean that other users/scripts won't hit this broken code path in certain conditions.

I would argue that even if the checksum is required, it is worth removing or fixing the broken code, if only to ensure that future Katello developers will not hit this bug again or use it as an example for other code without realizing it is buggy.
However, if the Katello developers would prefer to "fix" it by preventing uploads without a checksum rather than actually fixing the broken code, then so be it ... But in that case, the checksum really needs to be required in all cases.

Comment 12 Samir Jha 2023-12-04 15:47:08 UTC
Upon further discussions on this, we figured this workflow of uploading docker content is currently not supported. We have an epic to support Docker push with Satellite in the next release. I'll propose closing this as dup of that :https://bugzilla.redhat.com/show_bug.cgi?id=1313502

Comment 13 Samir Jha 2023-12-19 14:48:56 UTC
Closing this. This will be addressed as part of the RFE in the comment above which implements docker push.

*** This bug has been marked as a duplicate of bug 1313502 ***


Note You need to log in before you can comment on or make changes to this bug.