Skip to content

Commit

Permalink
fix: ApplicationSet deletes Application status (#15514)
Browse files Browse the repository at this point in the history 
* fix: ApplicationSet deletes Application status

Signed-off-by: Alexander Matyushentsev <[email protected]>

* apply reviewer notes

Signed-off-by: Alexander Matyushentsev <[email protected]>

---------

Signed-off-by: Alexander Matyushentsev <[email protected]>
  • Loading branch information
@alexmt
alexmt authored Sep 15, 2023
1 parent c30f0cc commit 21672a2
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 9 deletions.
38 changes: 31 additions & 7 deletions applicationset/controllers/applicationset_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,11 @@ import (
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/client-go/kubernetes"
k8scache "k8s.io/client-go/tools/cache"
"k8s.io/client-go/tools/record"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
"sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
Expand Down Expand Up @@ -85,6 +87,7 @@ type ApplicationSetReconciler struct {
SCMRootCAPath string
GlobalPreservedAnnotations []string
GlobalPreservedLabels []string
Cache cache.Cache
}

// +kubebuilder:rbac:groups=argoproj.io,resources=applicationsets,verbs=get;list;watch;create;update;patch;delete
Expand Down Expand Up @@ -582,6 +585,25 @@ func (r *ApplicationSetReconciler) SetupWithManager(mgr ctrl.Manager, enableProg
Complete(r)
}

func (r *ApplicationSetReconciler) updateCache(ctx context.Context, obj client.Object, logger *log.Entry) {
informer, err := r.Cache.GetInformer(ctx, obj)
if err != nil {
logger.Errorf("failed to get informer: %v", err)
return
}
// The controller runtime abstract away informers creation
// so unfortunately could not find any other way to access informer store.
k8sInformer, ok := informer.(k8scache.SharedInformer)
if !ok {
logger.Error("informer is not a kubernetes informer")
return
}
if err := k8sInformer.GetStore().Update(obj); err != nil {
logger.Errorf("failed to update cache: %v", err)
return
}
}

// createOrUpdateInCluster will create / update application resources in the cluster.
// - For new applications, it will call create
// - For existing application, it will call update
Expand Down Expand Up @@ -671,7 +693,7 @@ func (r *ApplicationSetReconciler) createOrUpdateInCluster(ctx context.Context,
}
continue
}

r.updateCache(ctx, found, appLog)
r.Recorder.Eventf(&applicationSet, corev1.EventTypeNormal, fmt.Sprint(action), "%s Application %q", action, generatedApp.Name)
appLog.Logf(log.InfoLevel, "%s Application", action)
}
Expand Down Expand Up @@ -829,15 +851,17 @@ func (r *ApplicationSetReconciler) removeFinalizerOnInvalidDestination(ctx conte

// If the finalizer length changed (due to filtering out an Argo finalizer), update the finalizer list on the app
if len(newFinalizers) != len(app.Finalizers) {
app.Finalizers = newFinalizers
updated := app.DeepCopy()
updated.Finalizers = newFinalizers
if err := r.Client.Patch(ctx, updated, client.MergeFrom(app)); err != nil {
return fmt.Errorf("error updating finalizers: %w", err)
}
r.updateCache(ctx, updated, appLog)
// Application must have updated list of finalizers
updated.DeepCopyInto(app)

r.Recorder.Eventf(&applicationSet, corev1.EventTypeNormal, "Updated", "Updated Application %q finalizer before deletion, because application has an invalid destination", app.Name)
appLog.Log(log.InfoLevel, "Updating application finalizer before deletion, because application has an invalid destination")

err := r.Client.Update(ctx, app, &client.UpdateOptions{})
if err != nil {
return fmt.Errorf("error updating finalizers: %w", err)
}
}
}

Expand Down
48 changes: 48 additions & 0 deletions applicationset/controllers/applicationset_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ import (
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/intstr"
kubefake "k8s.io/client-go/kubernetes/fake"
k8scache "k8s.io/client-go/tools/cache"
"k8s.io/client-go/tools/record"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/cache"
crtclient "sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
Expand All @@ -40,6 +42,34 @@ import (
"github.com/argoproj/argo-cd/v2/pkg/apis/application"
)

type fakeStore struct {
k8scache.Store
}

func (f *fakeStore) Update(obj interface{}) error {
return nil
}

type fakeInformer struct {
k8scache.SharedInformer
}

func (f *fakeInformer) AddIndexers(indexers k8scache.Indexers) error {
return nil
}

func (f *fakeInformer) GetStore() k8scache.Store {
return &fakeStore{}
}

type fakeCache struct {
cache.Cache
}

func (f *fakeCache) GetInformer(ctx context.Context, obj crtclient.Object) (cache.Informer, error) {
return &fakeInformer{}, nil
}

type generatorMock struct {
mock.Mock
}
Expand Down Expand Up @@ -185,6 +215,7 @@ func TestExtractApplications(t *testing.T) {
},
Renderer: &rendererMock,
KubeClientset: kubefake.NewSimpleClientset(),
Cache: &fakeCache{},
}

got, reason, err := r.generateApplications(v1alpha1.ApplicationSet{
Expand Down Expand Up @@ -967,6 +998,7 @@ func TestCreateOrUpdateInCluster(t *testing.T) {
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(len(initObjs) + len(c.expected)),
Cache: &fakeCache{},
}

err = r.createOrUpdateInCluster(context.TODO(), c.appSet, c.desiredApps)
Expand Down Expand Up @@ -1080,6 +1112,7 @@ func TestRemoveFinalizerOnInvalidDestination_FinalizerTypes(t *testing.T) {
Scheme: scheme,
Recorder: record.NewFakeRecorder(10),
KubeClientset: kubeclientset,
Cache: &fakeCache{},
}
//settingsMgr := settings.NewSettingsManager(context.TODO(), kubeclientset, "namespace")
//argoDB := db.NewDB("namespace", settingsMgr, r.KubeClientset)
Expand Down Expand Up @@ -1241,6 +1274,7 @@ func TestRemoveFinalizerOnInvalidDestination_DestinationTypes(t *testing.T) {
Scheme: scheme,
Recorder: record.NewFakeRecorder(10),
KubeClientset: kubeclientset,
Cache: &fakeCache{},
}
// settingsMgr := settings.NewSettingsManager(context.TODO(), kubeclientset, "argocd")
// argoDB := db.NewDB("argocd", settingsMgr, r.KubeClientset)
Expand Down Expand Up @@ -1452,6 +1486,7 @@ func TestCreateApplications(t *testing.T) {
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(len(initObjs) + len(c.expected)),
Cache: &fakeCache{},
}

err = r.createInCluster(context.TODO(), c.appSet, c.apps)
Expand Down Expand Up @@ -1659,6 +1694,7 @@ func TestGetMinRequeueAfter(t *testing.T) {
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(0),
Cache: &fakeCache{},
Generators: map[string]generators.Generator{
"List": &generatorMock10,
"Git": &generatorMock1,
Expand Down Expand Up @@ -1872,6 +1908,7 @@ func TestValidateGeneratedApplications(t *testing.T) {
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Cache: &fakeCache{},
Generators: map[string]generators.Generator{},
ArgoDB: &argoDBMock,
ArgoCDNamespace: "namespace",
Expand Down Expand Up @@ -1976,6 +2013,7 @@ func TestReconcilerValidationErrorBehaviour(t *testing.T) {
Scheme: scheme,
Renderer: &utils.Render{},
Recorder: record.NewFakeRecorder(1),
Cache: &fakeCache{},
Generators: map[string]generators.Generator{
"List": generators.NewListGenerator(),
},
Expand Down Expand Up @@ -2052,6 +2090,7 @@ func TestSetApplicationSetStatusCondition(t *testing.T) {
Scheme: scheme,
Renderer: &utils.Render{},
Recorder: record.NewFakeRecorder(1),
Cache: &fakeCache{},
Generators: map[string]generators.Generator{
"List": generators.NewListGenerator(),
},
Expand Down Expand Up @@ -2126,6 +2165,7 @@ func applicationsUpdateSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp
Scheme: scheme,
Renderer: &utils.Render{},
Recorder: record.NewFakeRecorder(recordBuffer),
Cache: &fakeCache{},
Generators: map[string]generators.Generator{
"List": generators.NewListGenerator(),
},
Expand Down Expand Up @@ -2295,6 +2335,7 @@ func applicationsDeleteSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp
Scheme: scheme,
Renderer: &utils.Render{},
Recorder: record.NewFakeRecorder(recordBuffer),
Cache: &fakeCache{},
Generators: map[string]generators.Generator{
"List": generators.NewListGenerator(),
},
Expand Down Expand Up @@ -2475,6 +2516,7 @@ func TestGenerateAppsUsingPullRequestGenerator(t *testing.T) {
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Cache: &fakeCache{},
Generators: map[string]generators.Generator{
"PullRequest": &generatorMock,
},
Expand Down Expand Up @@ -2599,6 +2641,7 @@ func TestPolicies(t *testing.T) {
Scheme: scheme,
Renderer: &utils.Render{},
Recorder: record.NewFakeRecorder(10),
Cache: &fakeCache{},
Generators: map[string]generators.Generator{
"List": generators.NewListGenerator(),
},
Expand Down Expand Up @@ -2761,6 +2804,7 @@ func TestSetApplicationSetApplicationStatus(t *testing.T) {
Scheme: scheme,
Renderer: &utils.Render{},
Recorder: record.NewFakeRecorder(1),
Cache: &fakeCache{},
Generators: map[string]generators.Generator{
"List": generators.NewListGenerator(),
},
Expand Down Expand Up @@ -3525,6 +3569,7 @@ func TestBuildAppDependencyList(t *testing.T) {
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Cache: &fakeCache{},
Generators: map[string]generators.Generator{},
ArgoDB: &argoDBMock,
ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...),
Expand Down Expand Up @@ -4118,6 +4163,7 @@ func TestBuildAppSyncMap(t *testing.T) {
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Cache: &fakeCache{},
Generators: map[string]generators.Generator{},
ArgoDB: &argoDBMock,
ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...),
Expand Down Expand Up @@ -4777,6 +4823,7 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Cache: &fakeCache{},
Generators: map[string]generators.Generator{},
ArgoDB: &argoDBMock,
ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...),
Expand Down Expand Up @@ -5530,6 +5577,7 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) {
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Cache: &fakeCache{},
Generators: map[string]generators.Generator{},
ArgoDB: &argoDBMock,
ArgoAppClientset: appclientset.NewSimpleClientset(argoObjs...),
Expand Down
8 changes: 6 additions & 2 deletions applicationset/utils/createOrUpdate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,11 @@ func CreateOrUpdate(ctx context.Context, c client.Client, obj client.Object, f c
return controllerutil.OperationResultCreated, nil
}

existing := obj.DeepCopyObject()
existingObj := obj.DeepCopyObject()
existing, ok := existingObj.(client.Object)
if !ok {
panic(fmt.Errorf("existing object is not a client.Object"))
}
if err := mutate(f, key, obj); err != nil {
return controllerutil.OperationResultNone, err
}
Expand Down Expand Up @@ -79,7 +83,7 @@ func CreateOrUpdate(ctx context.Context, c client.Client, obj client.Object, f c
return controllerutil.OperationResultNone, nil
}

if err := c.Update(ctx, obj); err != nil {
if err := c.Patch(ctx, obj, client.MergeFrom(existing)); err != nil {
return controllerutil.OperationResultNone, err
}
return controllerutil.OperationResultUpdated, nil
Expand Down
1 change: 1 addition & 0 deletions cmd/argocd-applicationset-controller/commands/applicationset_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ func NewCommand() *cobra.Command {
SCMRootCAPath: scmRootCAPath,
GlobalPreservedAnnotations: globalPreservedAnnotations,
GlobalPreservedLabels: globalPreservedLabels,
Cache: mgr.GetCache(),
}).SetupWithManager(mgr, enableProgressiveSyncs, maxConcurrentReconciliations); err != nil {
log.Error(err, "unable to create controller", "controller", "ApplicationSet")
os.Exit(1)
Expand Down

0 comments on commit 21672a2

Please sign in to comment.