OCIRepoReconciler: no-op reconcile improvements

Introduce contentConfigChecksum in the OCIRepository status to store a
checksum of the values that affect the source artifact. It is used to
detect when to rebuild an artifact when the spec changes.

The considerations for this are similar to the GitRepository
reconciler no-op clone implementation. Both reconcileSource and
reconcileArtifact need to consider the source configuration change
when deciding if the artifact in the storage is up-to-date.

Adds tests for reconcileSource and reconcileArtifact for the noop
cases.

Signed-off-by: Sunny <darkowlzz@protonmail.com>

api/v1beta2/ocirepository_types.go

@@ -203,6 +203,17 @@ type OCIRepositoryStatus struct {
 	// +optional
 	Artifact *Artifact `json:"artifact,omitempty"`
 
+	// ContentConfigChecksum is a checksum of all the configurations related to
+	// the content of the source artifact:
+	//  - .spec.ignore
+	//  - .spec.layerSelector
+	// observed in .status.observedGeneration version of the object. This can
+	// be used to determine if the content configuration has changed and the
+	// artifact needs to be rebuilt.
+	// It has the format of `<algo>:<checksum>`, for example: `sha256:<checksum>`.
+	// +optional
+	ContentConfigChecksum string `json:"contentConfigChecksum,omitempty"`
+
 	meta.ReconcileRequestStatus `json:",inline"`
 }
 

config/crd/bases/source.toolkit.fluxcd.io_ocirepositories.yaml

@@ -300,6 +300,14 @@ spec:
                   - type
                   type: object
                 type: array
+              contentConfigChecksum:
+                description: 'ContentConfigChecksum is a checksum of all the configurations
+                  related to the content of the source artifact: - .spec.ignore -
+                  .spec.layerSelector observed in .status.observedGeneration version
+                  of the object. This can be used to determine if the content configuration
+                  has changed and the artifact needs to be rebuilt. It has the format
+                  of `<algo>:<checksum>`, for example: `sha256:<checksum>`.'
+                type: string
               lastHandledReconcileAt:
                 description: LastHandledReconcileAt holds the value of the most recent
                   reconcile request value, so a change of the annotation value can

controllers/ocirepository_controller.go

@@ -18,6 +18,7 @@ package controllers
 
 import (
 	"context"
+	"crypto/sha256"
 	"crypto/tls"
 	"crypto/x509"
 	"errors"
@@ -418,8 +419,10 @@ func (r *OCIRepositoryReconciler) reconcileSource(ctx context.Context, obj *sour
 		conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, meta.SucceededReason, "verified signature of revision %s", revision)
 	}
 
-	// Skip pulling if the artifact revision hasn't changes
+	// Skip pulling if the artifact revision and the content config checksum has
-	if obj.GetArtifact().HasRevision(revision) {
+	// not changed.
+	if obj.GetArtifact().HasRevision(revision) &&
+		r.calculateContentConfigChecksum(obj) == obj.Status.ContentConfigChecksum {
 		conditions.Delete(obj, sourcev1.FetchFailedCondition)
 		return sreconcile.ResultSuccess, nil
 	}
@@ -922,9 +925,13 @@ func (r *OCIRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *so
 	artifact := r.Storage.NewArtifactFor(obj.Kind, obj, revision,
 		fmt.Sprintf("%s.tar.gz", r.digestFromRevision(revision)))
 
+	// Calculate the content config checksum.
+	ccc := r.calculateContentConfigChecksum(obj)
+
 	// Set the ArtifactInStorageCondition if there's no drift.
 	defer func() {
-		if obj.GetArtifact().HasRevision(artifact.Revision) {
+		if obj.GetArtifact().HasRevision(artifact.Revision) &&
+			obj.Status.ContentConfigChecksum == ccc {
 			conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition)
 			conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason,
 				"stored artifact for digest '%s'", artifact.Revision)
@@ -932,7 +939,8 @@ func (r *OCIRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *so
 	}()
 
 	// The artifact is up-to-date
-	if obj.GetArtifact().HasRevision(artifact.Revision) {
+	if obj.GetArtifact().HasRevision(artifact.Revision) &&
+		obj.Status.ContentConfigChecksum == ccc {
 		r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.ArtifactUpToDateReason,
 			"artifact up-to-date with remote revision: '%s'", artifact.Revision)
 		return sreconcile.ResultSuccess, nil
@@ -997,6 +1005,7 @@ func (r *OCIRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *so
 	// Record it on the object
 	obj.Status.Artifact = artifact.DeepCopy()
 	obj.Status.Artifact.Metadata = metadata.Metadata
+	obj.Status.ContentConfigChecksum = ccc
 
 	// Update symlink on a "best effort" basis
 	url, err := r.Storage.Symlink(artifact, "latest.tar.gz")
@@ -1125,3 +1134,21 @@ func (r *OCIRepositoryReconciler) notify(ctx context.Context, oldObj, newObj *so
 		}
 	}
 }
+
+// calculateContentConfigChecksum calculates a checksum of all the
+// configurations that result in a change in the source artifact. It can be used
+// to decide if further reconciliation is needed when an artifact already exists
+// for a set of configurations.
+func (r *OCIRepositoryReconciler) calculateContentConfigChecksum(obj *sourcev1.OCIRepository) string {
+	c := []byte{}
+	// Consider the ignore rules.
+	if obj.Spec.Ignore != nil {
+		c = append(c, []byte(*obj.Spec.Ignore)...)
+	}
+	// Consider the layer selector.
+	if obj.Spec.LayerSelector != nil {
+		c = append(c, []byte(obj.GetLayerMediaType()+obj.GetLayerOperation())...)
+	}
+
+	return fmt.Sprintf("sha256:%x", sha256.Sum256(c))
+}

controllers/ocirepository_controller_test.go

@@ -38,15 +38,6 @@ import (
 	"time"
 
 	"github.com/darkowlzz/controller-check/status"
-	"github.com/fluxcd/pkg/apis/meta"
-	"github.com/fluxcd/pkg/oci"
-	"github.com/fluxcd/pkg/runtime/conditions"
-	"github.com/fluxcd/pkg/runtime/patch"
-	"github.com/fluxcd/pkg/untar"
-	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
-	serror "github.com/fluxcd/source-controller/internal/error"
-	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
-	"github.com/fluxcd/source-controller/pkg/git"
 	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/google/go-containerregistry/pkg/crane"
 	"github.com/google/go-containerregistry/pkg/registry"
@@ -60,12 +51,26 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/record"
+	"k8s.io/utils/pointer"
 	kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+
+	"github.com/fluxcd/pkg/apis/meta"
+	"github.com/fluxcd/pkg/oci"
+	"github.com/fluxcd/pkg/runtime/conditions"
+	"github.com/fluxcd/pkg/runtime/patch"
+	"github.com/fluxcd/pkg/untar"
+
+	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
+	serror "github.com/fluxcd/source-controller/internal/error"
+	sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
+	"github.com/fluxcd/source-controller/pkg/git"
 )
 
+const ociRepoEmptyContentConfigChecksum = "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
+
 func TestOCIRepository_Reconcile(t *testing.T) {
 	g := NewWithT(t)
 
@@ -1237,9 +1242,131 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) {
 	}
 }
 
-func TestOCIRepository_reconcileArtifact(t *testing.T) {
+func TestOCIRepository_reconcileSource_noop(t *testing.T) {
 	g := NewWithT(t)
 
+	testRevision := "6.1.5/d1fc4595915714af2492dc4b66097de1e10f80150c8899907d8f8e61c6d6f67d"
+
+	tmpDir := t.TempDir()
+	server, err := setupRegistryServer(ctx, tmpDir, registryOptions{})
+	g.Expect(err).ToNot(HaveOccurred())
+
+	_, err = pushMultiplePodinfoImages(server.registryHost, "6.1.5")
+	g.Expect(err).ToNot(HaveOccurred())
+
+	// NOTE: The following verifies if it was a noop run by checking the
+	// artifact metadata which is unknown unless the image is pulled.
+
+	tests := []struct {
+		name       string
+		beforeFunc func(obj *sourcev1.OCIRepository)
+		afterFunc  func(g *WithT, artifact *sourcev1.Artifact)
+	}{
+		{
+			name: "full reconcile - no existing artifact",
+			afterFunc: func(g *WithT, artifact *sourcev1.Artifact) {
+				g.Expect(artifact.Metadata).ToNot(BeEmpty())
+			},
+		},
+		{
+			name: "noop - artifact revisions and ccc match",
+			beforeFunc: func(obj *sourcev1.OCIRepository) {
+				obj.Status.Artifact = &sourcev1.Artifact{
+					Revision: testRevision,
+				}
+				obj.Status.ContentConfigChecksum = ociRepoEmptyContentConfigChecksum
+			},
+			afterFunc: func(g *WithT, artifact *sourcev1.Artifact) {
+				g.Expect(artifact.Metadata).To(BeEmpty())
+			},
+		},
+		{
+			name: "full reconcile - same rev, different ccc",
+			beforeFunc: func(obj *sourcev1.OCIRepository) {
+				obj.Status.ContentConfigChecksum = "some-checksum"
+				obj.Status.Artifact = &sourcev1.Artifact{
+					Revision: testRevision,
+				}
+			},
+			afterFunc: func(g *WithT, artifact *sourcev1.Artifact) {
+				g.Expect(artifact.Metadata).ToNot(BeEmpty())
+			},
+		},
+		{
+			name: "noop - same rev, observed layer selector",
+			beforeFunc: func(obj *sourcev1.OCIRepository) {
+				obj.Spec.LayerSelector = &sourcev1.OCILayerSelector{
+					MediaType: "application/vnd.docker.image.rootfs.diff.tar.gzip",
+					Operation: sourcev1.OCILayerCopy,
+				}
+				obj.Status.Artifact = &sourcev1.Artifact{
+					Revision: testRevision,
+				}
+				obj.Status.ContentConfigChecksum = "sha256:fcfd705e10431a341f2df5b05ecee1fb54facd9a5e88b0be82276bdf533b6c64"
+			},
+			afterFunc: func(g *WithT, artifact *sourcev1.Artifact) {
+				g.Expect(artifact.Metadata).To(BeEmpty())
+			},
+		},
+		{
+			name: "full reconcile - same rev, observed layer selector changed",
+			beforeFunc: func(obj *sourcev1.OCIRepository) {
+				obj.Spec.LayerSelector = &sourcev1.OCILayerSelector{
+					MediaType: "application/vnd.docker.image.rootfs.diff.tar.gzip",
+					Operation: sourcev1.OCILayerExtract,
+				}
+				obj.Status.Artifact = &sourcev1.Artifact{
+					Revision: testRevision,
+				}
+				obj.Status.ContentConfigChecksum = "sha256:fcfd705e10431a341f2df5b05ecee1fb54facd9a5e88b0be82276bdf533b6c64"
+			},
+			afterFunc: func(g *WithT, artifact *sourcev1.Artifact) {
+				g.Expect(artifact.Metadata).ToNot(BeEmpty())
+			},
+		},
+	}
+
+	builder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme())
+	r := &OCIRepositoryReconciler{
+		Client:        builder.Build(),
+		EventRecorder: record.NewFakeRecorder(32),
+		Storage:       testStorage,
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			obj := &sourcev1.OCIRepository{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "noop-",
+				},
+				Spec: sourcev1.OCIRepositorySpec{
+					URL:       fmt.Sprintf("oci://%s/podinfo", server.registryHost),
+					Reference: &sourcev1.OCIRepositoryRef{Tag: "6.1.5"},
+					Interval:  metav1.Duration{Duration: interval},
+					Timeout:   &metav1.Duration{Duration: timeout},
+				},
+			}
+
+			if tt.beforeFunc != nil {
+				tt.beforeFunc(obj)
+			}
+
+			artifact := &sourcev1.Artifact{}
+			tmpDir := t.TempDir()
+			got, err := r.reconcileSource(ctx, obj, artifact, tmpDir)
+			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(got).To(Equal(sreconcile.ResultSuccess))
+
+			if tt.afterFunc != nil {
+				tt.afterFunc(g, artifact)
+			}
+		})
+	}
+}
+
+func TestOCIRepository_reconcileArtifact(t *testing.T) {
 	tests := []struct {
 		name             string
 		targetPath       string
@@ -1250,6 +1377,7 @@ func TestOCIRepository_reconcileArtifact(t *testing.T) {
 		assertArtifact   *sourcev1.Artifact
 		assertPaths      []string
 		assertConditions []metav1.Condition
+		afterFunc        func(g *WithT, obj *sourcev1.OCIRepository)
 	}{
 		{
 			name:       "Archiving Artifact creates correct files and condition",
@@ -1279,6 +1407,7 @@ func TestOCIRepository_reconcileArtifact(t *testing.T) {
 				obj.Status.Artifact = &sourcev1.Artifact{
 					Revision: "revision",
 				}
+				obj.Status.ContentConfigChecksum = ociRepoEmptyContentConfigChecksum
 			},
 			assertArtifact: &sourcev1.Artifact{
 				Revision: "revision",
@@ -1287,6 +1416,96 @@ func TestOCIRepository_reconcileArtifact(t *testing.T) {
 				*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for digest"),
 			},
 		},
+		{
+			name:       "Artifact already present, unobserved ignore, rebuild artifact",
+			targetPath: "testdata/oci/repository",
+			artifact: &sourcev1.Artifact{
+				Revision: "revision",
+			},
+			beforeFunc: func(obj *sourcev1.OCIRepository) {
+				obj.Status.Artifact = &sourcev1.Artifact{Revision: "revision"}
+				obj.Spec.Ignore = pointer.String("aaa")
+				obj.Status.ContentConfigChecksum = ociRepoEmptyContentConfigChecksum
+			},
+			want: sreconcile.ResultSuccess,
+			assertPaths: []string{
+				"latest.tar.gz",
+			},
+			afterFunc: func(g *WithT, obj *sourcev1.OCIRepository) {
+				g.Expect(obj.Status.ContentConfigChecksum).To(Equal("sha256:9834876dcfb05cb167a5c24953eba58c4ac89b1adf57f28f2f9d09af107ee8f0"))
+			},
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for digest"),
+			},
+		},
+		{
+			name:       "Artifact already present, unobserved layer selector, rebuild artifact",
+			targetPath: "testdata/oci/repository",
+			artifact: &sourcev1.Artifact{
+				Revision: "revision",
+			},
+			beforeFunc: func(obj *sourcev1.OCIRepository) {
+				obj.Spec.LayerSelector = &sourcev1.OCILayerSelector{MediaType: "foo"}
+				obj.Status.Artifact = &sourcev1.Artifact{Revision: "revision"}
+				obj.Status.ContentConfigChecksum = ociRepoEmptyContentConfigChecksum
+			},
+			want: sreconcile.ResultSuccess,
+			assertPaths: []string{
+				"latest.tar.gz",
+			},
+			afterFunc: func(g *WithT, obj *sourcev1.OCIRepository) {
+				g.Expect(obj.Status.ContentConfigChecksum).To(Equal("sha256:82410edf339ab2945d97e26b92b6499e57156db63b94c17654b6ab97fbf86dbb"))
+			},
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for digest"),
+			},
+		},
+		{
+			name:       "Artifact already present, observed layer selector changed, rebuild artifact",
+			targetPath: "testdata/oci/repository",
+			artifact: &sourcev1.Artifact{
+				Revision: "revision",
+				Path:     "foo.txt",
+			},
+			beforeFunc: func(obj *sourcev1.OCIRepository) {
+				obj.Spec.LayerSelector = &sourcev1.OCILayerSelector{
+					MediaType: "foo",
+					Operation: sourcev1.OCILayerCopy,
+				}
+				obj.Status.Artifact = &sourcev1.Artifact{Revision: "revision"}
+				obj.Status.ContentConfigChecksum = ociRepoEmptyContentConfigChecksum
+			},
+			want: sreconcile.ResultSuccess,
+			assertPaths: []string{
+				"latest.tar.gz",
+			},
+			afterFunc: func(g *WithT, obj *sourcev1.OCIRepository) {
+				g.Expect(obj.Status.ContentConfigChecksum).To(Equal("sha256:0e0e1c82f6403c8ee74fdf51349c8b5d98c508b5374c507c7ffb2e41dbc875df"))
+			},
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for digest"),
+			},
+		},
+		{
+			name:       "Artifact already present, observed ignore and layer selector, up-to-date",
+			targetPath: "testdata/oci/repository",
+			artifact: &sourcev1.Artifact{
+				Revision: "revision",
+			},
+			beforeFunc: func(obj *sourcev1.OCIRepository) {
+				obj.Spec.Ignore = pointer.String("aaa")
+				obj.Spec.LayerSelector = &sourcev1.OCILayerSelector{MediaType: "foo"}
+				obj.Status.Artifact = &sourcev1.Artifact{Revision: "revision"}
+				obj.Status.ContentConfigChecksum = "sha256:0b56187b81cab6c3485583a46bec631f5ea08a1f69b769457f0e4aafb47884e3"
+			},
+			want: sreconcile.ResultSuccess,
+			assertArtifact: &sourcev1.Artifact{
+				Revision: "revision",
+			},
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for digest"),
+			},
+		},
 		{
 			name:       "target path doesn't exist",
 			targetPath: "testdata/oci/non-existent",
@@ -1317,6 +1536,7 @@ func TestOCIRepository_reconcileArtifact(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
 
 			obj := &sourcev1.OCIRepository{
 				ObjectMeta: metav1.ObjectMeta{
@@ -1345,6 +1565,10 @@ func TestOCIRepository_reconcileArtifact(t *testing.T) {
 				g.Expect(obj.Status.Artifact).To(MatchArtifact(tt.artifact))
 			}
 
+			if tt.afterFunc != nil {
+				tt.afterFunc(g, obj)
+			}
+
 			for _, path := range tt.assertPaths {
 				localPath := testStorage.LocalPath(*obj.GetArtifact())
 				path = filepath.Join(filepath.Dir(localPath), path)
@@ -1978,3 +2202,27 @@ func createTLSServer() (*httptest.Server, []byte, []byte, []byte, tls.Certificat
 	clientTLSCert, err = tls.X509KeyPair(clientCertPEM, clientKeyPEM)
 	return srv, rootCertPEM, clientCertPEM, clientKeyPEM, clientTLSCert, err
 }
+
+func TestOCIRepository_calculateContentConfigChecksum(t *testing.T) {
+	g := NewWithT(t)
+	obj := &sourcev1.OCIRepository{}
+	r := &OCIRepositoryReconciler{}
+
+	emptyChecksum := r.calculateContentConfigChecksum(obj)
+	g.Expect(emptyChecksum).To(Equal(ociRepoEmptyContentConfigChecksum))
+
+	// Ignore modified.
+	obj.Spec.Ignore = pointer.String("some-rule")
+	ignoreModChecksum := r.calculateContentConfigChecksum(obj)
+	g.Expect(emptyChecksum).ToNot(Equal(ignoreModChecksum))
+
+	// LayerSelector modified.
+	obj.Spec.LayerSelector = &sourcev1.OCILayerSelector{
+		MediaType: "application/vnd.docker.image.rootfs.diff.tar.gzip",
+	}
+	mediaTypeChecksum := r.calculateContentConfigChecksum(obj)
+	g.Expect(ignoreModChecksum).ToNot(Equal(mediaTypeChecksum))
+	obj.Spec.LayerSelector.Operation = sourcev1.OCILayerCopy
+	layerCopyChecksum := r.calculateContentConfigChecksum(obj)
+	g.Expect(mediaTypeChecksum).ToNot(Equal(layerCopyChecksum))
+}

docs/api/source.md

@@ -2995,6 +2995,25 @@ Artifact
 </tr>
 <tr>
 <td>
+<code>contentConfigChecksum</code><br>
+<em>
+string
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>ContentConfigChecksum is a checksum of all the configurations related to
+the content of the source artifact:
+- .spec.ignore
+- .spec.layerSelector
+observed in .status.observedGeneration version of the object. This can
+be used to determine if the content configuration has changed and the
+artifact needs to be rebuilt.
+It has the format of <code>&lt;algo&gt;:&lt;checksum&gt;</code>, for example: <code>sha256:&lt;checksum&gt;</code>.</p>
+</td>
+</tr>
+<tr>
+<td>
 <code>ReconcileRequestStatus</code><br>
 <em>
 <a href="https://godoc.org/github.com/fluxcd/pkg/apis/meta#ReconcileRequestStatus">