status.lastUnpacked is being updated on every pull attempt

[m1kola]

It seems like .status.lastUnpacked is being updated on every poll attempt even when the content of the catalog hasn't changed.

Steps to reproduce

1. Create a catalog with frequent poll interval:

   apiVersion: olm.operatorframework.io/v1alpha1
   kind: ClusterCatalog
   metadata:
     name: operatorhubio
   spec:
     source:
       type: image
       image:
         ref: quay.io/operatorhubio/catalog:latest
         pollInterval: 5s

2. Watch the status fields:

   kubectl get clustercatalog operatorhubio -o json -w | jq '{".status.lastUnpacked": .status.lastUnpacked, ".status.resolvedSource.image.lastUnpacked": .status.resolvedSource.image.lastUnpacked, ".status.resolvedSource.image.lastPollAttempt": .status.resolvedSource.image.lastPollAttempt}'

Result

{
  ".status.lastUnpacked": "2024-09-10T13:03:18Z",
  ".status.resolvedSource.image.lastUnpacked": "2024-09-10T13:03:18Z",
  ".status.resolvedSource.image.lastPollAttempt": "2024-09-10T13:03:18Z"
}
{
  ".status.lastUnpacked": "2024-09-10T13:03:24Z",
  ".status.resolvedSource.image.lastUnpacked": "2024-09-10T13:03:24Z",
  ".status.resolvedSource.image.lastPollAttempt": "2024-09-10T13:03:24Z"
}

Expected result

.status.lastUnpacked and .status.resolvedSource.image.lastUnpacked should stay the same as long as the content of the catalog is the same.

[m1kola]

I think the issue is that we set LastUnpacked in Result struct to "now" here on line 86:

catalogd/internal/source/image_registry_client.go

Lines 80 to 88 in 4323184

	digest, isDigest := imgRef.(name.Digest)
	if isDigest {
	hexVal := strings.TrimPrefix(digest.DigestStr(), "sha256:")
	unpackPath := filepath.Join(i.BaseCachePath, catalog.Name, hexVal)
	if stat, err := os.Stat(unpackPath); err == nil && stat.IsDir() {
	l.V(1).Info("found image in filesystem cache", "digest", hexVal)
	return unpackedResult(os.DirFS(unpackPath), catalog, digest.String(), metav1.Time{Time: time.Now()}), nil
	}
	}

Despite the fact that we just return the content we already had. We probably should set it to "now" here only here.

[joelanford]

Possibly related: #379

[m1kola]

I don't think #379 is related: so far it does not seem to fix this bug.

---

In additional to my my previous I suspect that ImageRegistry's Unpack is not working as expected.

From the unit tests it seems like the expected behaviour is that when we the spec has an image such as quay.io/operatorhubio/catalog:latest (with a tag) we will be resolving a tag into a digest and checking if we already have a cache for that digest here. However we only seem to parse image reference and attempt to type cast into name.Digest without actually resolving image tag - this will always return isDigest==false for image refs with tags. So we will never be hitting this code when working with image refs with tags.

We have test cases for both digest and tag image refs, but the test is very complex with a lot of parameters. I suspect that something went wrong with the test because it shows green.

[m1kola]

Ok, the above seems to be a separate bug. Reported: #392

This probably needs to be fixed before this one.