Skip to content

Update artifact tarball when spec.ignore changes #712

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Update artifact tarball when spec.ignore changes #712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9009c39
Update artifact tarball when `spec.ignore` changes
angelokurtis May 9, 2022
6208eff
Testing spec ignore update scenarios
angelokurtis May 13, 2022
6426893
Doc adjustments
angelokurtis May 13, 2022
92a666b
Sorting imports
angelokurtis May 13, 2022
1e8980a
Documenting new ignoreChecksum status fields
angelokurtis May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions api/v1beta2/bucket_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,12 @@ type BucketStatus struct {
// +optional
Artifact *Artifact `json:"artifact,omitempty"`

// IgnoreChecksum is a checksum of .spec.ignore in .status.observedGeneration
// version of the object.
// It has the format of `<algo>:<checksum>`, for example: `sha256:<checksum>`.
// +optional
IgnoreChecksum string `json:"ignoreChecksum,omitempty"`

meta.ReconcileRequestStatus `json:",inline"`
}

Expand Down
6 changes: 6 additions & 0 deletions api/v1beta2/gitrepository_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,12 @@ type GitRepositoryStatus struct {
// +optional
IncludedArtifacts []*Artifact `json:"includedArtifacts,omitempty"`

// IgnoreChecksum is a checksum of .spec.ignore in .status.observedGeneration
// version of the object.
// It has the format of `<algo>:<checksum>`, for example: `sha256:<checksum>`.
// +optional
IgnoreChecksum string `json:"ignoreChecksum,omitempty"`

meta.ReconcileRequestStatus `json:",inline"`
}

Expand Down
5 changes: 5 additions & 0 deletions config/crd/bases/source.toolkit.fluxcd.io_buckets.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,11 @@ spec:
- type
type: object
type: array
ignoreChecksum:
description: 'IgnoreChecksum is a checksum of .spec.ignore in .status.observedGeneration
version of the object. 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
Expand Down
5 changes: 5 additions & 0 deletions config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,11 @@ spec:
- type
type: object
type: array
ignoreChecksum:
description: 'IgnoreChecksum is a checksum of .spec.ignore in .status.observedGeneration
version of the object. It has the format of `<algo>:<checksum>`,
for example: `sha256:<checksum>`.'
type: string
includedArtifacts:
description: IncludedArtifacts contains a list of the last successfully
included Artifacts as instructed by GitRepositorySpec.Include.
Expand Down
16 changes: 13 additions & 3 deletions controllers/bucket_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"sync"
"time"

"github.com/fluxcd/source-controller/pkg/azure"
"golang.org/x/sync/errgroup"
"golang.org/x/sync/semaphore"
corev1 "k8s.io/api/core/v1"
Expand All @@ -53,6 +52,7 @@ import (
serror "github.com/fluxcd/source-controller/internal/error"
sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
"github.com/fluxcd/source-controller/internal/reconcile/summarize"
"github.com/fluxcd/source-controller/pkg/azure"
"github.com/fluxcd/source-controller/pkg/gcp"
"github.com/fluxcd/source-controller/pkg/minio"
"github.com/fluxcd/source-controller/pkg/sourceignore"
Expand Down Expand Up @@ -562,21 +562,31 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.
// Create artifact
artifact := r.Storage.NewArtifactFor(obj.Kind, obj, revision, fmt.Sprintf("%s.tar.gz", revision))

// Calculates checksum of current .spec.ignore
var ignoreChecksum string
if obj.Spec.Ignore != nil {
ignore := []byte(*obj.Spec.Ignore)
ignoreChecksum = fmt.Sprintf("sha256:%x", sha256.Sum256(ignore))
}

// Set the ArtifactInStorageCondition if there's no drift.
defer func() {
if obj.GetArtifact().HasRevision(artifact.Revision) {
if obj.GetArtifact().HasRevision(artifact.Revision) && obj.Status.IgnoreChecksum == ignoreChecksum {
conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition)
conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason,
"stored artifact for revision '%s'", artifact.Revision)
}
}()

// The artifact is up-to-date
if obj.GetArtifact().HasRevision(artifact.Revision) {
if obj.GetArtifact().HasRevision(artifact.Revision) && obj.Status.IgnoreChecksum == ignoreChecksum {
r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.ArtifactUpToDateReason, "artifact up-to-date with remote revision: '%s'", artifact.Revision)
return sreconcile.ResultSuccess, nil
}

// Ensure .status.ignoreChecksum is up-to-date with .spec.ignore.
obj.Status.IgnoreChecksum = ignoreChecksum

// Ensure target path exists and is a directory
if f, err := os.Stat(dir); err != nil {
e := &serror.Event{
Expand Down
44 changes: 44 additions & 0 deletions controllers/bucket_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ 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"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
Expand Down Expand Up @@ -977,6 +978,49 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) {
*conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.InvalidPathReason, "is not a directory"),
},
},
{
name: "Ignore checksum should be set when spec ignore is in use",
beforeFunc: func(t *WithT, obj *sourcev1.Bucket, index *etagIndex, dir string) {
obj.Spec.Interval = metav1.Duration{Duration: interval}
obj.Spec.Ignore = pointer.StringPtr("!**.txt\n")
},
afterFunc: func(t *WithT, obj *sourcev1.Bucket, dir string) {
t.Expect(obj.Status.IgnoreChecksum).To(Equal("sha256:539530fbd29c252e357e36ddc065dc7923265e4ea795f09cada03287288f5e7a"))
},
want: sreconcile.ResultSuccess,
assertConditions: []metav1.Condition{
*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"),
},
},
{
name: "Ignore checksum must be removed when spec ignore is empty",
beforeFunc: func(t *WithT, obj *sourcev1.Bucket, index *etagIndex, dir string) {
obj.Spec.Interval = metav1.Duration{Duration: interval}
obj.Status.IgnoreChecksum = "sha256:539530fbd29c252e357e36ddc065dc7923265e4ea795f09cada03287288f5e7a"
},
afterFunc: func(t *WithT, obj *sourcev1.Bucket, dir string) {
t.Expect(obj.Status.IgnoreChecksum).To(BeEmpty())
},
want: sreconcile.ResultSuccess,
assertConditions: []metav1.Condition{
*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"),
},
},
Copy link
Contributor

@darkowlzz darkowlzz May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests.
In the above, we test for cases where ignore is added and removed. Let's also test for the case where the existing value of ignore is updated, would result in a different checksum, not empty.

Same for the GitRepo tests.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

{
name: "Ignore checksum should be updated when spec ignore changes",
beforeFunc: func(t *WithT, obj *sourcev1.Bucket, index *etagIndex, dir string) {
obj.Spec.Interval = metav1.Duration{Duration: interval}
obj.Spec.Ignore = pointer.StringPtr("**.yaml\n")
obj.Status.IgnoreChecksum = "sha256:539530fbd29c252e357e36ddc065dc7923265e4ea795f09cada03287288f5e7a"
},
afterFunc: func(t *WithT, obj *sourcev1.Bucket, dir string) {
t.Expect(obj.Status.IgnoreChecksum).To(Equal("sha256:f4c4021b0b39597f03baee5f45fa6ef3d57c9fae57115ee3a54781b02e3a6cd8"))
},
want: sreconcile.ResultSuccess,
assertConditions: []metav1.Condition{
*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'"),
},
},
}

for _, tt := range tests {
Expand Down
15 changes: 13 additions & 2 deletions controllers/gitrepository_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package controllers

import (
"context"
"crypto/sha256"
"errors"
"fmt"
"os"
Expand Down Expand Up @@ -514,21 +515,31 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context,
// Create potential new artifact with current available metadata
artifact := r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), commit.String(), fmt.Sprintf("%s.tar.gz", commit.Hash.String()))

// Calculates checksum of current .spec.ignore
var ignoreChecksum string
if obj.Spec.Ignore != nil {
ignore := []byte(*obj.Spec.Ignore)
ignoreChecksum = fmt.Sprintf("sha256:%x", sha256.Sum256(ignore))
}

// Set the ArtifactInStorageCondition if there's no drift.
defer func() {
if obj.GetArtifact().HasRevision(artifact.Revision) && !includes.Diff(obj.Status.IncludedArtifacts) {
if obj.GetArtifact().HasRevision(artifact.Revision) && !includes.Diff(obj.Status.IncludedArtifacts) && obj.Status.IgnoreChecksum == ignoreChecksum {
conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition)
conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason,
"stored artifact for revision '%s'", artifact.Revision)
}
}()

// The artifact is up-to-date
if obj.GetArtifact().HasRevision(artifact.Revision) && !includes.Diff(obj.Status.IncludedArtifacts) {
if obj.GetArtifact().HasRevision(artifact.Revision) && !includes.Diff(obj.Status.IncludedArtifacts) && obj.Status.IgnoreChecksum == ignoreChecksum {
r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.ArtifactUpToDateReason, "artifact up-to-date with remote revision: '%s'", artifact.Revision)
return sreconcile.ResultSuccess, nil
}

// Ensure .status.ignoreChecksum is up-to-date with .spec.ignore.
obj.Status.IgnoreChecksum = ignoreChecksum

// Ensure target path exists and is a directory
if f, err := os.Stat(dir); err != nil {
e := &serror.Event{
Expand Down
46 changes: 46 additions & 0 deletions controllers/gitrepository_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,6 +838,52 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) {
*conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, sourcev1.InvalidPathReason, "invalid target path"),
},
},
{
name: "Ignore checksum should be set when spec ignore is in use",
dir: "testdata/git/repository",
beforeFunc: func(obj *sourcev1.GitRepository) {
obj.Spec.Interval = metav1.Duration{Duration: interval}
obj.Spec.Ignore = pointer.StringPtr("!**.txt\n")
},
afterFunc: func(t *WithT, obj *sourcev1.GitRepository) {
t.Expect(obj.Status.IgnoreChecksum).To(Equal("sha256:539530fbd29c252e357e36ddc065dc7923265e4ea795f09cada03287288f5e7a"))
},
want: sreconcile.ResultSuccess,
assertConditions: []metav1.Condition{
*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"),
},
},
{
name: "Ignore checksum must be removed when spec ignore is empty",
dir: "testdata/git/repository",
beforeFunc: func(obj *sourcev1.GitRepository) {
obj.Spec.Interval = metav1.Duration{Duration: interval}
obj.Status.IgnoreChecksum = "sha256:539530fbd29c252e357e36ddc065dc7923265e4ea795f09cada03287288f5e7a"
},
afterFunc: func(t *WithT, obj *sourcev1.GitRepository) {
t.Expect(obj.Status.IgnoreChecksum).To(BeEmpty())
},
want: sreconcile.ResultSuccess,
assertConditions: []metav1.Condition{
*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"),
},
},
{
name: "Ignore checksum should be updated when spec ignore changes",
dir: "testdata/git/repository",
beforeFunc: func(obj *sourcev1.GitRepository) {
obj.Spec.Interval = metav1.Duration{Duration: interval}
obj.Spec.Ignore = pointer.StringPtr("**.yaml\n")
obj.Status.IgnoreChecksum = "sha256:539530fbd29c252e357e36ddc065dc7923265e4ea795f09cada03287288f5e7a"
},
afterFunc: func(t *WithT, obj *sourcev1.GitRepository) {
t.Expect(obj.Status.IgnoreChecksum).To(Equal("sha256:f4c4021b0b39597f03baee5f45fa6ef3d57c9fae57115ee3a54781b02e3a6cd8"))
},
want: sreconcile.ResultSuccess,
assertConditions: []metav1.Condition{
*conditions.TrueCondition(sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision 'main/revision'"),
},
},
}
artifactSize := func(g *WithT, artifactURL string) *int64 {
if artifactURL == "" {
Expand Down
28 changes: 28 additions & 0 deletions docs/api/source.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1208,6 +1208,20 @@ Artifact
</tr>
<tr>
<td>
<code>ignoreChecksum</code><br>
<em>
string
</em>
</td>
<td>
<em>(Optional)</em>
<p>IgnoreChecksum is a checksum of .spec.ignore in .status.observedGeneration
version of the object.
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">
Expand Down Expand Up @@ -1643,6 +1657,20 @@ Artifacts as instructed by GitRepositorySpec.Include.</p>
</tr>
<tr>
<td>
<code>ignoreChecksum</code><br>
<em>
string
</em>
</td>
<td>
<em>(Optional)</em>
<p>IgnoreChecksum is a checksum of .spec.ignore in .status.observedGeneration
version of the object.
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">
Expand Down
6 changes: 6 additions & 0 deletions docs/spec/v1beta2/buckets.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,12 @@ Note that a Bucket can be [reconciling](#reconciling-bucket) while failing at
the same time, for example due to a newly introduced configuration issue in the
Bucket spec.

### Ignore Checksum

The source-controller calculates the SHA256 checksum of the last Bucket's
[`.spec.ignore` field](#ignore) and record it in the `.status.ignoreChecksum` field. This field
is used to detect `.spec.ignore` updates to allow artifact changes.
Comment on lines +949 to +951
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The source-controller calculates the SHA256 checksum of the last Bucket's
[`.spec.ignore` field](#ignore) and record it in the `.status.ignoreChecksum` field. This field
is used to detect `.spec.ignore` updates to allow artifact changes.
The source-controller calculates the SHA256 checksum Bucket's [`.spec.ignore` field](#ignore),
and records it in `.status.ignoreChecksum`. This field is used to detect `.spec.ignore` changes to
trigger an Artifact update.


### Observed Generation

The source-controller reports an
Expand Down
6 changes: 6 additions & 0 deletions docs/spec/v1beta2/gitrepositories.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,6 +838,12 @@ Note that a GitRepository can be [reconciling](#reconciling-gitrepository)
while failing at the same time, for example due to a newly introduced
configuration issue in the GitRepository spec.

### Ignore Checksum

The source-controller calculates the SHA256 checksum of the last GitRepository's
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same suggestion applies here.

[`.spec.ignore` field](#ignore) and record it in the `.status.ignoreChecksum` field. This field
is used to detect `.spec.ignore` updates to allow artifact changes.

### Observed Generation

The source-controller reports an [observed generation][typical-status-properties]
Expand Down