From f4aed8baf83b6e30ccf40422f91fbe1d84028b49 Mon Sep 17 00:00:00 2001 From: Sunny Date: Tue, 27 Sep 2022 06:31:58 +0530 Subject: [PATCH] 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 --- api/v1beta2/ocirepository_types.go | 11 + ...rce.toolkit.fluxcd.io_ocirepositories.yaml | 8 + controllers/ocirepository_controller.go | 35 ++- controllers/ocirepository_controller_test.go | 268 +++++++++++++++++- docs/api/source.md | 19 ++ 5 files changed, 327 insertions(+), 14 deletions(-) diff --git a/api/v1beta2/ocirepository_types.go b/api/v1beta2/ocirepository_types.go index b1a13508..91ca7f85 100644 --- a/api/v1beta2/ocirepository_types.go +++ b/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 `:`, for example: `sha256:`. + // +optional + ContentConfigChecksum string `json:"contentConfigChecksum,omitempty"` + meta.ReconcileRequestStatus `json:",inline"` } diff --git a/config/crd/bases/source.toolkit.fluxcd.io_ocirepositories.yaml b/config/crd/bases/source.toolkit.fluxcd.io_ocirepositories.yaml index b64a339f..2d236ec9 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_ocirepositories.yaml +++ b/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 `:`, for example: `sha256:`.' + type: string lastHandledReconcileAt: description: LastHandledReconcileAt holds the value of the most recent reconcile request value, so a change of the annotation value can diff --git a/controllers/ocirepository_controller.go b/controllers/ocirepository_controller.go index 95ec1ec6..ecec4891 100644 --- a/controllers/ocirepository_controller.go +++ b/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 - if obj.GetArtifact().HasRevision(revision) { + // Skip pulling if the artifact revision and the content config checksum has + // 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)) +} diff --git a/controllers/ocirepository_controller_test.go b/controllers/ocirepository_controller_test.go index 7449531c..01f5bbde 100644 --- a/controllers/ocirepository_controller_test.go +++ b/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)) +} diff --git a/docs/api/source.md b/docs/api/source.md index 96b26b3e..8c4eda2e 100644 --- a/docs/api/source.md +++ b/docs/api/source.md @@ -2995,6 +2995,25 @@ Artifact +contentConfigChecksum
+ +string + + + +(Optional) +

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

+ + + + ReconcileRequestStatus