Fix nuget package file update

David Fernandezrequested to merge
10io-fix-nuget-package-file-update-from-metadata into master

What does this MR do?

Nuget packages are uploaded to GitLab as a opaque request. (see !21493 (merged))

In order to have a package name and version, a worker will download the package archive and read the .nuspec file to extract those information. This has been done !21561 (merged).

In !21561 (merged), the linked Packages::PackageFile is updated with a new file_name. This will effectively remove the way how the package files are retrieved from the object storage, see . As a consequence, the package file can't be downloaded anymore, the request will end up in a 404.

Details

Here is a detailed sequence of actions:

  1. A nuget package is uploaded
  2. The API controller will create a Packages::Package linked with a single Packages::PackageFile that points the uploaded file. Since the upload request doesn't have any info on the package, package.name, package.version and package_file.file_name are all constants (this is to allow those objects to pass the rails validations).
  3. An metadata extraction worker is enqueued for the given package.
  4. The extraction worker will download the package and its file and update package.name, package.version and package_file.file_name.

The issue we have is updating package_file.file_name. This field is used to generate the key used by the object storage system (see https://gitlab.com/gitlab-org/gitlab/blob/master/ee/app/uploaders/packages/package_file_uploader.rb#L13). In other words, when the worker read the package file, the key is packages/6b/86/6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b/packages/10/files/13/package.nupkg. Once the worker has done its job, the key will be /packages/6b/86/6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b/packages/10/files/13/dummypackage.1.0.0.nupkg. Updating the key will not update the file in the object storage. Thus, the file can't be downloaded with the new key (since it's still stored under the old key).

Solution

Here are the possible solutions:

  1. Don't update the file_name attribute
  2. Update the file_name attribute and move the file in the object storage
  3. Create a new Packages::PackageFile with the proper file_name attribute and destroy the old one.

(1.) is possible but the UI will use the file_name to display the files linked to a Packages::Package. Since the file_name sent in the upload is a constant package.nupkg, they will all look the same = bad UX. (2.) would be ideal but it doesn't seem possible to do.

(3.) is what this MR implements. Since the file is download for the metadata extraction, we will use to create a new Packages::PackageFile with the proper attributes (namely file_name and size). The old package_file will be deleted. In a sense, this is doing a move operation by doing two steps: copy and then delete.

A downside to this approach: it uses a throwaway Packages::PackageFile. For each nuget package upload, we will use two package file primary ids.

This MR does the following:

  • Packages::Nuget::MetadataExtractionService will now work with a package_file_path
  • Packages::Nuget::UpdatePackageFromMetadataService will now download the package archive, extract the metadata, create a new package file (linked to the proper package) and delete the old package file.
  • Update the relevant specs

Thanks to @engwan's comment (!24064 (comment 283321807)), (2.) is possible and is the cleanest solution. CarrierWave takes care of moving the file key in the object storage.

This MR is gated behind this feature flag (scoped by project): nuget_package_registry as this MR is part of the nuget MVC (See the epic: &2271 (closed)).

This MR doesn't need documentation or a change log entry.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by David Fernandez

Merge request reports