Skip to content

Commit

Permalink
Add predicates for GitRepo and ImagePolicy watches
Browse files Browse the repository at this point in the history
ImageUpdateAutomationReconciler watches GitRepository and ImagePolicy
kinds for every event. This leads to unnecessary extra reconciliations
at times. For example when the controller starts with existing
resources, the same ImageUpdateAutomation object gets reconciled at
least twice, once due to the watch on ImageUpdateAutomation startup
and again due to the watches on GitRepository and ImagePolicy for create
event, as they get registered in the cache.

Add predicates to filter the ImagePolicy to only allow events for latest
image update, and GitRepository to only allow events for change in the
source configuration.

Signed-off-by: Sunny <[email protected]>
  • Loading branch information
darkowlzz committed Feb 26, 2024
1 parent f1b8d33 commit b424607
Show file tree
Hide file tree
Showing 4 changed files with 295 additions and 2 deletions.
12 changes: 10 additions & 2 deletions internal/controller/imageupdateautomation_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -478,8 +478,16 @@ func (r *ImageUpdateAutomationReconciler) SetupWithManager(ctx context.Context,
return ctrl.NewControllerManagedBy(mgr).
For(&imagev1.ImageUpdateAutomation{}, builder.WithPredicates(
predicate.Or(predicate.GenerationChangedPredicate{}, predicates.ReconcileRequestedPredicate{}))).
Watches(&sourcev1.GitRepository{}, handler.EnqueueRequestsFromMapFunc(r.automationsForGitRepo)).
Watches(&imagev1_reflect.ImagePolicy{}, handler.EnqueueRequestsFromMapFunc(r.automationsForImagePolicy)).
Watches(
&sourcev1.GitRepository{},
handler.EnqueueRequestsFromMapFunc(r.automationsForGitRepo),
builder.WithPredicates(sourceConfigChangePredicate{}),
).
Watches(
&imagev1_reflect.ImagePolicy{},
handler.EnqueueRequestsFromMapFunc(r.automationsForImagePolicy),
builder.WithPredicates(latestImageChangePredicate{}),
).
WithOptions(controller.Options{
RateLimiter: opts.RateLimiter,
}).
Expand Down
84 changes: 84 additions & 0 deletions internal/controller/predicate.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
Copyright 2024 The Flux authors
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package controller

import (
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/predicate"

imagev1_reflect "github.com/fluxcd/image-reflector-controller/api/v1beta2"
)

// latestImageChangePredicate implements a predicate for latest image change.
// This can be used to filter events from ImagePolicies for change in the latest
// image.
type latestImageChangePredicate struct {
predicate.Funcs
}

func (latestImageChangePredicate) Create(e event.CreateEvent) bool {
return false
}

func (latestImageChangePredicate) Delete(e event.DeleteEvent) bool {
return false
}

func (latestImageChangePredicate) Update(e event.UpdateEvent) bool {
if e.ObjectOld == nil || e.ObjectNew == nil {
return false
}

oldSource, ok := e.ObjectOld.(*imagev1_reflect.ImagePolicy)
if !ok {
return false
}

newSource, ok := e.ObjectNew.(*imagev1_reflect.ImagePolicy)
if !ok {
return false
}

if oldSource.Status.LatestImage != newSource.Status.LatestImage {
return true
}

return false
}

// sourceConfigChangePredicate implements a predicate for source configuration
// change. This can be used to filter events from source objects for change in
// source configuration.
type sourceConfigChangePredicate struct {
predicate.Funcs
}

func (sourceConfigChangePredicate) Create(e event.CreateEvent) bool {
return false
}

func (sourceConfigChangePredicate) Delete(e event.DeleteEvent) bool {
return false
}

func (sourceConfigChangePredicate) Update(e event.UpdateEvent) bool {
if e.ObjectOld == nil || e.ObjectNew == nil {
return false
}

return e.ObjectOld.GetGeneration() != e.ObjectNew.GetGeneration()
}
120 changes: 120 additions & 0 deletions internal/controller/predicate_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/*
Copyright 2024 The Flux authors
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package controller

import (
"testing"

. "github.com/onsi/gomega"
"sigs.k8s.io/controller-runtime/pkg/event"

imagev1_reflect "github.com/fluxcd/image-reflector-controller/api/v1beta2"
sourcev1 "github.com/fluxcd/source-controller/api/v1"
)

func Test_latestImageChangePredicate_Update(t *testing.T) {
tests := []struct {
name string
beforeFunc func(oldObj, newObj *imagev1_reflect.ImagePolicy)
want bool
}{
{
name: "no latest image",
beforeFunc: func(oldObj, newObj *imagev1_reflect.ImagePolicy) {
oldObj.Status.LatestImage = ""
newObj.Status.LatestImage = ""
},
want: false,
},
{
name: "new image, no old image",
beforeFunc: func(oldObj, newObj *imagev1_reflect.ImagePolicy) {
oldObj.Status.LatestImage = ""
newObj.Status.LatestImage = "foo"
},
want: true,
},
{
name: "different old and new image",
beforeFunc: func(oldObj, newObj *imagev1_reflect.ImagePolicy) {
oldObj.Status.LatestImage = "bar"
newObj.Status.LatestImage = "foo"
},
want: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

oldObj := &imagev1_reflect.ImagePolicy{}
newObj := oldObj.DeepCopy()
if tt.beforeFunc != nil {
tt.beforeFunc(oldObj, newObj)
}
e := event.UpdateEvent{
ObjectOld: oldObj,
ObjectNew: newObj,
}
p := latestImageChangePredicate{}
g.Expect(p.Update(e)).To(Equal(tt.want))
})
}
}

func Test_sourceConfigChangePredicate_Update(t *testing.T) {
tests := []struct {
name string
beforeFunc func(oldObj, newObj *sourcev1.GitRepository)
want bool
}{
{
name: "no generation change, same config",
beforeFunc: func(oldObj, newObj *sourcev1.GitRepository) {
oldObj.Generation = 0
newObj.Generation = 0
},
want: false,
},
{
name: "new generation, config change",
beforeFunc: func(oldObj, newObj *sourcev1.GitRepository) {
oldObj.Generation = 1
newObj.Generation = 2
},
want: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

oldObj := &sourcev1.GitRepository{}
newObj := oldObj.DeepCopy()
if tt.beforeFunc != nil {
tt.beforeFunc(oldObj, newObj)
}
e := event.UpdateEvent{
ObjectOld: oldObj,
ObjectNew: newObj,
}
p := sourceConfigChangePredicate{}
g.Expect(p.Update(e)).To(Equal(tt.want))
})
}

}
81 changes: 81 additions & 0 deletions internal/controller/update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import (
"github.com/fluxcd/pkg/git"
"github.com/fluxcd/pkg/git/gogit"
"github.com/fluxcd/pkg/gittestserver"
"github.com/fluxcd/pkg/runtime/conditions"
"github.com/fluxcd/pkg/ssh"
sourcev1 "github.com/fluxcd/source-controller/api/v1"

Expand Down Expand Up @@ -166,6 +167,86 @@ func TestImageUpdateAutomationReconciler_deleteBeforeFinalizer(t *testing.T) {
}, timeout).Should(Succeed())
}

func TestImageAutomationReconciler_watchSourceAndLatestImage(t *testing.T) {
policySpec := imagev1_reflect.ImagePolicySpec{
ImageRepositoryRef: meta.NamespacedObjectReference{
Name: "not-expected-to-exist",
},
Policy: imagev1_reflect.ImagePolicyChoice{
SemVer: &imagev1_reflect.SemVerPolicy{
Range: "1.x",
},
},
}
fixture := "testdata/appconfig"
latest := "helloworld:v1.0.0"

testWithRepoAndImagePolicy(NewWithT(t), testEnv, fixture, policySpec, latest, gogit.ClientName, func(g *WithT, s repoAndPolicyArgs, repoURL string, localRepo *extgogit.Repository) {
// Update the setter marker in the repo.
policyKey := types.NamespacedName{
Name: s.imagePolicyName,
Namespace: s.namespace,
}
commitInRepo(g, repoURL, s.branch, "Install setter marker", func(tmp string) {
g.Expect(replaceMarker(tmp, policyKey)).To(Succeed())
})

// Create the automation object.
updateStrategy := &imagev1.UpdateStrategy{
Strategy: imagev1.UpdateStrategySetters,
}
err := createImageUpdateAutomation(testEnv, "update-test", s.namespace, s.gitRepoName, s.gitRepoNamespace, s.branch, s.branch, "", testCommitTemplate, "", updateStrategy)
g.Expect(err).ToNot(HaveOccurred())

var imageUpdate imagev1.ImageUpdateAutomation
imageUpdateKey := types.NamespacedName{
Namespace: s.namespace,
Name: "update-test",
}

// Let the image update be ready.
g.Eventually(func() bool {
if err := testEnv.Get(ctx, imageUpdateKey, &imageUpdate); err != nil {
return false
}
return conditions.IsReady(&imageUpdate)
}, timeout).Should(BeTrue())
readyMsg := conditions.Get(&imageUpdate, meta.ReadyCondition).Message

// Update ImagePolicy with new latest and wait for image update to
// trigger.
latest = "helloworld:v1.1.0"
err = updateImagePolicyWithLatestImage(testEnv, s.imagePolicyName, s.namespace, latest)
g.Expect(err).ToNot(HaveOccurred())

g.Eventually(func() bool {
if err := testEnv.Get(ctx, imageUpdateKey, &imageUpdate); err != nil {
return false
}
ready := conditions.Get(&imageUpdate, meta.ReadyCondition)
return ready.Status == metav1.ConditionTrue && ready.Message != readyMsg
}, timeout).Should(BeTrue())

// Update GitRepo with bad config and wait for image update to fail.
var gitRepo sourcev1.GitRepository
gitRepoKey := types.NamespacedName{
Name: s.gitRepoName,
Namespace: s.gitRepoNamespace,
}
g.Expect(testEnv.Get(ctx, gitRepoKey, &gitRepo)).To(Succeed())
patch := client.MergeFrom(gitRepo.DeepCopy())
gitRepo.Spec.SecretRef = &meta.LocalObjectReference{Name: "non-existing-secret"}
g.Expect(testEnv.Patch(ctx, &gitRepo, patch)).To(Succeed())

g.Eventually(func() bool {
if err := testEnv.Get(ctx, imageUpdateKey, &imageUpdate); err != nil {
return false
}
return conditions.IsFalse(&imageUpdate, meta.ReadyCondition)
}, timeout).Should(BeTrue())
})
}

func TestImageAutomationReconciler_commitMessage(t *testing.T) {
policySpec := imagev1_reflect.ImagePolicySpec{
ImageRepositoryRef: meta.NamespacedObjectReference{
Expand Down

0 comments on commit b424607

Please sign in to comment.