Bug 2107079

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.

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.

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.

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.

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.

Bulk setting Target Milestone = 6.15.0 where sat-6.15.0+ is set.

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?

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.

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

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 ***