During sync, skip package download for packages with unchanged NEVRAs

Currently, we start synchronizing packages even if the package is unchanged
from a previous sync.  This means that we end up re-signing packages each
time we sync when signing during sync.

This commit fixes that problem by re-using the already synced version of
the package for packages that already exist in the latest version of the
repository with the same NEVRA.  Note that this means that a package that
has been changed without changing the NEVRA will *not* be resynced anymore.
This may be seen as a security feature where packages can't be silently
changed.  To sync a package with the same NEVRA, the original package must
first be removed from the repository and then the repository must be synced
again.

Signed-off-by: Jonathan Dieter <jdieter@ciq.com>

Author: jdieter

pulp_rpm/app/tasks/synchronizing.py

@@ -91,6 +91,7 @@
     is_previous_version,
     get_sha256,
     urlpath_sanitize,
+    format_nevra,
 )
 from pulp_rpm.app.rpm_version import RpmVersion
 
@@ -1239,6 +1240,27 @@ async def parse_packages(self, primary_xml, filelists_xml, other_xml, modulemd_l
         # we might want to pick the latest based on the build time.
         latest_build_time_by_nevra = {}
 
+        # Cache packages already in the repository to later check for duplicates
+        # This needs to be @sync_to_async because self.repository.latest_version() is synchronous
+        @sync_to_async
+        def get_existing_package_nevras():
+            contents = self.repository.latest_version().content.filter(
+                pulp_type=Package.get_pulp_type()
+            )
+            nevra_to_declarative = {}
+
+            for content in contents:
+                # Get package object from content
+                pkg = content.rpm_package
+                nevra = format_nevra(pkg.name, pkg.epoch, pkg.version, pkg.release, pkg.arch)
+                # Create a DeclarativeContent object for the existing package
+                dc = DeclarativeContent(content=pkg)
+                nevra_to_declarative[nevra] = dc
+
+            return nevra_to_declarative
+
+        existing_package_nevras = await get_existing_package_nevras()
+
         # Perform various checks and potentially filter out unwanted packages
         # We parse all of primary.xml first and fail fast if something is wrong.
         # Collect a list of any package nevras() we don't want to include.
@@ -1357,33 +1379,48 @@ def verification_and_skip_callback(pkg):
                 # entries with the same NEVRA, pick the one with the larger build time
                 elif pkg.time_build != latest_build_time_by_nevra[pkg_nevra]:
                     continue
-                # Implicit: There can be multiple package entries that are completely identical
-                # (same NEVRA, same build time, same checksum / pkgid) and the same or different
-                # location_href. We're not explicitly handling this, the pipeline will deduplicate.
-                package = Package(**Package.createrepo_to_dict(pkg))
-                base_url = pkg.location_base or self.remote_url
-                url = urlpath_sanitize(base_url, package.location_href)
-                del pkg  # delete it as soon as we're done with it
-
-                # Location_href is not a property of the Package in isolation [0], and Pulp has
-                # a well defined way of generating the layout/locations on publication time.
-                # We only need to use the original location_href for metadata mirroring
-                # [0] https://github.com/pulp/pulp_rpm/issues/2580
-                original_location_href = package.location_href
-                package.location_href = package.filename
-                store_package_for_mirroring(self.repository, package.pkgId, original_location_href)
-
-                artifact = Artifact(size=package.size_package)
-                checksum_type = getattr(CHECKSUM_TYPES, package.checksum_type.upper())
-                setattr(artifact, checksum_type, package.pkgId)
-                da = DeclarativeArtifact(
-                    artifact=artifact,
-                    url=url,
-                    relative_path=package.location_href,
-                    remote=self.remote,
-                    deferred_download=self.deferred_download,
-                )
-                dc = DeclarativeContent(content=package, d_artifacts=[da])
+
+                # Check for identical NEVRA to what's in the repo right now.  For these, we don't
+                # want to "skip" them, but we also don't want to re-download them, so let's just
+                # make the declarative content point to the existing package with no artifact.
+                if pkg_nevra in existing_package_nevras:
+                    log.debug(
+                        f"Substituting already existing package {pkg_nevra} into repo to avoid "
+                        f"re-download"
+                    )
+                    dc = existing_package_nevras[pkg_nevra]
+                else:
+                    # Implicit: There can be multiple package entries that are completely identical
+                    # (same NEVRA, same build time, same checksum / pkgid) and the same or different
+                    # location_href. We're not explicitly handling this, the pipeline will
+                    # deduplicate.
+                    package = Package(**Package.createrepo_to_dict(pkg))
+                    base_url = pkg.location_base or self.remote_url
+                    url = urlpath_sanitize(base_url, package.location_href)
+                    del pkg  # delete it as soon as we're done with it
+
+                    # Location_href is not a property of the Package in isolation [0], and Pulp has
+                    # a well defined way of generating the layout/locations on publication time.
+                    # We only need to use the original location_href for metadata mirroring
+                    # [0] https://github.com/pulp/pulp_rpm/issues/2580
+                    original_location_href = package.location_href
+                    package.location_href = package.filename
+                    store_package_for_mirroring(
+                        self.repository, package.pkgId, original_location_href
+                    )
+
+                    artifact = Artifact(size=package.size_package)
+                    checksum_type = getattr(CHECKSUM_TYPES, package.checksum_type.upper())
+                    setattr(artifact, checksum_type, package.pkgId)
+                    da = DeclarativeArtifact(
+                        artifact=artifact,
+                        url=url,
+                        relative_path=package.location_href,
+                        remote=self.remote,
+                        deferred_download=self.deferred_download,
+                    )
+                    dc = DeclarativeContent(content=package, d_artifacts=[da])
+
                 dc.extra_data = defaultdict(list)
 
                 # find if a package relates to a modulemd
@@ -1644,7 +1681,7 @@ async def run(self):
 
         async for batch in self.batches():
             for d_content in batch:
-                if isinstance(d_content.content, Package):
+                if isinstance(d_content.content, Package) and len(d_content.d_artifacts) > 0:
                     await self._sign_rpm_content(d_content)
 
                 await self.put(d_content)

pulp_rpm/tests/functional/api/test_package_signing.py

@@ -10,7 +10,12 @@
 from pulpcore.exceptions.validation import InvalidSignatureError
 
 from pulp_rpm.app.shared_utils import RpmTool
-from pulp_rpm.tests.functional.constants import RPM_PACKAGE_FILENAME, RPM_UNSIGNED_URL
+from pulp_rpm.tests.functional.constants import (
+    RPM_PACKAGE_FILENAME,
+    RPM_UNSIGNED_URL,
+    RPM_UNSIGNED_MODIFIED_FIXTURE_URL,
+    RPM_PACKAGE_CONTENT_NAME,
+)
 from pulp_rpm.tests.functional.utils import get_package_repo_path
 
 
@@ -260,7 +265,6 @@ def test_no_sign_packages_on_sync_if_no_signing_service(
 
     # Setup GPG and RPM tool
     gpg_a, _ = signing_gpg_extra
-    fingerprint = gpg_a.fingerprint
 
     rpm_tool = RpmTool(tmp_path)
     rpm_tool.import_pubkey_string(gpg_a.pubkey)
@@ -414,3 +418,94 @@ def test_error_signing_with_mirror_complete_sync(
     assert json.loads(e.value.body) == [
         "Cannot use 'mirror_complete' sync policy when repository has a package signing service."
     ]
+
+
+def test_no_resync_of_packages_on_second_sync(
+    init_and_sync,
+    tmp_path,
+    gen_object_with_cleanup,
+    download_content_unit,
+    signing_gpg_extra,
+    rpm_package_signing_service,
+    rpm_package_api,
+    rpm_repository_api,
+    rpm_repository_factory,
+    rpm_publication_factory,
+    rpm_distribution_factory,
+    delete_orphans_pre,
+    get_content,
+):
+    """
+    Ensure that a second sync doesn't re-download and re-sign packages that haven't changed
+    """
+    from pulp_rpm.app.shared_utils import RpmTool
+
+    # Setup GPG and RPM tool
+    gpg_a, _ = signing_gpg_extra
+
+    rpm_tool = RpmTool(tmp_path)
+    rpm_tool.import_pubkey_string(gpg_a.pubkey)
+
+    # Create repository with package signing service configured
+    repository = rpm_repository_factory(
+        package_signing_service=rpm_package_signing_service.pulp_href,
+        package_signing_fingerprint=gpg_a.fingerprint,
+    )
+    init_and_sync(
+        repository=repository,
+        url=RPM_UNSIGNED_MODIFIED_FIXTURE_URL,
+        sync_policy="mirror_content_only",
+    )
+
+    # Get synced packages - refresh repository to get latest version
+    updated_repository = rpm_repository_api.read(repository.pulp_href)
+    packages = rpm_package_api.list(repository_version=updated_repository.latest_version_href)
+    assert packages.count > 0, "No packages were synced"
+
+    # Test the first package to verify it was signed during sync
+    test_package = packages.results[0]
+
+    # Verify that the final served package is signed
+    publication = rpm_publication_factory(repository=repository.pulp_href)
+    distribution = rpm_distribution_factory(publication=publication.pulp_href)
+    downloaded_package = tmp_path / "package.rpm"
+    downloaded_package.write_bytes(
+        download_content_unit(
+            distribution.base_path, get_package_repo_path(test_package.location_href)
+        )
+    )
+    assert rpm_tool.verify_signature(downloaded_package)
+
+    # Save the content information of the packages from the original sync
+    original_packages = {
+        content["name"]: content
+        for content in get_content(updated_repository)["present"][RPM_PACKAGE_CONTENT_NAME]
+    }
+
+    init_and_sync(repository=repository, sync_policy="mirror_content_only")
+    # Get synced packages - refresh repository to get latest version
+    updated_repository = rpm_repository_api.read(repository.pulp_href)
+    packages = rpm_package_api.list(repository_version=updated_repository.latest_version_href)
+    assert packages.count > 0, "No packages were synced"
+
+    # Test the first package to verify it was signed during sync
+    test_package = packages.results[0]
+
+    # Verify that the final served package is signed
+    publication = rpm_publication_factory(repository=repository.pulp_href)
+    distribution = rpm_distribution_factory(publication=publication.pulp_href)
+    downloaded_package = tmp_path / "package.rpm"
+    downloaded_package.write_bytes(
+        download_content_unit(
+            distribution.base_path, get_package_repo_path(test_package.location_href)
+        )
+    )
+    assert rpm_tool.verify_signature(downloaded_package)
+
+    # Test that the zebra package wasn't re-synced
+    mutated_packages = {
+        content["name"]: content
+        for content in get_content(updated_repository)["present"][RPM_PACKAGE_CONTENT_NAME]
+    }
+
+    assert original_packages["zebra"] == mutated_packages["zebra"]

pulp_rpm/tests/functional/constants.py

@@ -96,6 +96,8 @@
 
 RPM_UNSIGNED_FIXTURE_URL = urljoin(PULP_FIXTURES_BASE_URL, "rpm-unsigned/")
 
+RPM_UNSIGNED_MODIFIED_FIXTURE_URL = urljoin(PULP_FIXTURES_BASE_URL, "rpm-unsigned-modified/")
+
 RPM_ZSTD_METADATA_FIXTURE_URL = urljoin(PULP_FIXTURES_BASE_URL, "rpm-zstd-metadata/")
 
 RPM_INVALID_FIXTURE_URL = urljoin(PULP_FIXTURES_BASE_URL, "rpm-missing-filelists/")