Skip to content

Commit c893cb2

Browse files
ncr38ncr38
authored
fix: Drop owner references from autonomous agent events (#524)
Signed-off-by: navin <[email protected]>
1 parent 88966cc commit c893cb2

File tree

4 files changed

+110
-1
lines changed

4 files changed

+110
-1
lines changed

‎agent/inbound.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,12 +104,15 @@ func (a *Agent) processIncomingApplication(ev *event.Event) error {
104104

105105
var exists, sourceUIDMatch bool
106106

107-
// Source UID annotation is not present for apps on the autonomous agent since it is the source of truth.
108107
if a.mode == types.AgentModeManaged {
108+
// Source UID annotation is not present for apps on the autonomous agent since it is the source of truth.
109109
exists, sourceUIDMatch, err = a.appManager.CompareSourceUID(a.context, incomingApp)
110110
if err != nil {
111111
return fmt.Errorf("failed to compare the source UID of app: %w", err)
112112
}
113+
// In managed mode, Drop ownerReferences from the incoming resource
114+
// This can lead to garbage-collection of the resource on the agent cluster, if referenced owner is missing. For example, AppSet
115+
incomingApp.OwnerReferences = nil
113116
}
114117

115118
switch ev.Type() {
@@ -195,6 +198,12 @@ func (a *Agent) processIncomingAppProject(ev *event.Event) error {
195198
return fmt.Errorf("failed to validate source UID of appProject: %w", err)
196199
}
197200

201+
if a.mode == types.AgentModeManaged {
202+
// In managed mode, Drop ownerReferences from the incoming resource
203+
// This can lead to garbage-collection of the resource on the agent cluster, if referenced owner is missing. For example, AppSet
204+
incomingAppProject.OwnerReferences = nil
205+
}
206+
198207
switch ev.Type() {
199208
case event.Create:
200209
if exists {

‎agent/inbound_test.go‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,14 @@ func Test_CreateApplication(t *testing.T) {
4848
app := &v1alpha1.Application{ObjectMeta: v1.ObjectMeta{
4949
Name: "test",
5050
Namespace: "default",
51+
OwnerReferences: []v1.OwnerReference{
52+
{
53+
APIVersion: "argoproj.io/v1alpha1",
54+
Kind: "ApplicationSet",
55+
Name: "owner",
56+
UID: "uid-1",
57+
},
58+
},
5159
}}
5260
t.Run("Discard event in unmanaged mode", func(t *testing.T) {
5361
napp, err := a.createApplication(app)
@@ -72,6 +80,7 @@ func Test_CreateApplication(t *testing.T) {
7280
napp, err := a.createApplication(app)
7381
require.NoError(t, err)
7482
require.NotNil(t, napp)
83+
require.Empty(t, napp.OwnerReferences, "OwnerReferences should not be applied on managed app")
7584
})
7685

7786
}
@@ -87,6 +96,14 @@ func Test_ProcessIncomingAppWithUIDMismatch(t *testing.T) {
8796
incomingApp := &v1alpha1.Application{ObjectMeta: v1.ObjectMeta{
8897
Name: "test",
8998
Namespace: "argocd",
99+
OwnerReferences: []v1.OwnerReference{
100+
{
101+
APIVersion: "argoproj.io/v1alpha1",
102+
Kind: "ApplicationSet",
103+
Name: "owner",
104+
UID: "uid-1",
105+
},
106+
},
90107
}}
91108

92109
// oldApp is the app that is already present on the agent
@@ -540,6 +557,14 @@ func Test_UpdateApplication(t *testing.T) {
540557
Name: "test",
541558
Namespace: "default",
542559
ResourceVersion: "12345",
560+
OwnerReferences: []v1.OwnerReference{
561+
{
562+
APIVersion: "argoproj.io/v1alpha1",
563+
Kind: "ApplicationSet",
564+
Name: "owner",
565+
UID: "uid-1",
566+
},
567+
},
543568
}}
544569
t.Run("Discard event because version has been seen already", func(t *testing.T) {
545570
defer a.appManager.ClearIgnored()
@@ -561,6 +586,7 @@ func Test_UpdateApplication(t *testing.T) {
561586
napp, err := a.updateApplication(app)
562587
require.NoError(t, err)
563588
require.NotNil(t, napp)
589+
require.Empty(t, napp.OwnerReferences, "OwnerReferences should not be applied on managed app")
564590
})
565591

566592
t.Run("Update application using update in managed mode", func(t *testing.T) {
@@ -575,6 +601,7 @@ func Test_UpdateApplication(t *testing.T) {
575601
napp, err := a.updateApplication(app)
576602
require.NoError(t, err)
577603
require.NotNil(t, napp)
604+
require.Empty(t, napp.OwnerReferences, "OwnerReferences should not be applied on managed app")
578605
})
579606

580607
t.Run("Update application using patch in autonomous mode", func(t *testing.T) {
@@ -620,6 +647,14 @@ func Test_CreateAppProject(t *testing.T) {
620647
ObjectMeta: v1.ObjectMeta{
621648
Name: "test",
622649
Namespace: "default",
650+
OwnerReferences: []v1.OwnerReference{
651+
{
652+
APIVersion: "argoproj.io/v1alpha1",
653+
Kind: "Application",
654+
Name: "owner",
655+
UID: "uid-1",
656+
},
657+
},
623658
}, Spec: v1alpha1.AppProjectSpec{
624659
SourceNamespaces: []string{"default", "argocd"},
625660
},
@@ -648,6 +683,7 @@ func Test_CreateAppProject(t *testing.T) {
648683
napp, err := a.createAppProject(project)
649684
require.NoError(t, err)
650685
require.NotNil(t, napp)
686+
require.Empty(t, napp.OwnerReferences, "OwnerReferences should not be applied on managed app project")
651687
})
652688

653689
t.Run("Create appproject", func(t *testing.T) {
@@ -658,6 +694,7 @@ func Test_CreateAppProject(t *testing.T) {
658694
napp, err := a.createAppProject(project)
659695
require.NoError(t, err)
660696
require.NotNil(t, napp)
697+
require.Empty(t, napp.OwnerReferences, "OwnerReferences should not be applied on managed app project")
661698
})
662699

663700
}
@@ -674,6 +711,14 @@ func Test_UpdateAppProject(t *testing.T) {
674711
Name: "test",
675712
Namespace: "argocd",
676713
ResourceVersion: "12345",
714+
OwnerReferences: []v1.OwnerReference{
715+
{
716+
APIVersion: "argoproj.io/v1alpha1",
717+
Kind: "Application",
718+
Name: "owner",
719+
UID: "uid-1",
720+
},
721+
},
677722
},
678723
Spec: v1alpha1.AppProjectSpec{
679724
SourceNamespaces: []string{"default", "argocd"},
@@ -693,6 +738,7 @@ func Test_UpdateAppProject(t *testing.T) {
693738
napp, err := a.updateAppProject(project)
694739
require.NoError(t, err)
695740
require.NotNil(t, napp)
741+
require.Empty(t, napp.OwnerReferences, "OwnerReferences should not be applied on managed app project")
696742
})
697743

698744
t.Run("Create the appproject if it doesn't exist", func(t *testing.T) {
@@ -703,6 +749,7 @@ func Test_UpdateAppProject(t *testing.T) {
703749
napp, err := a.updateAppProject(project)
704750
require.NoError(t, err)
705751
require.NotNil(t, napp)
752+
require.Empty(t, napp.OwnerReferences, "OwnerReferences should not be applied on managed app project")
706753
})
707754

708755
}

‎principal/event.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,10 @@ func (s *Server) processApplicationEvent(ctx context.Context, agentName string,
154154
return fmt.Errorf("could not prefix project name: %w", err)
155155
}
156156

157+
// In autonomous mode, Drop ownerReferences from the agent-side resource
158+
// Having ownerReferences can lead to garbage-collection of the resource on the control plane, if owner is missing
159+
incoming.OwnerReferences = nil
160+
157161
// Set the destination name to the cluster mapping for the agent
158162
cluster := s.clusterMgr.Mapping(agentName)
159163
if cluster == nil {
@@ -297,6 +301,11 @@ func (s *Server) processAppProjectEvent(ctx context.Context, agentName string, e
297301
}
298302
// Set the source namespaces to allow the agent's namespace on the principal
299303
incoming.Spec.SourceNamespaces = []string{agentName}
304+
305+
// In autonomous mode, Drop ownerReferences from the incoming resource
306+
// This can lead to garbage-collection of the resource on the control plane, if owner is missing For example, AppSet
307+
incoming.OwnerReferences = nil
308+
300309
// Set all destinations to point to the agent cluster
301310
for i := range incoming.Spec.Destinations {
302311
incoming.Spec.Destinations[i].Name = agentName

‎principal/event_test.go‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,14 @@ func Test_CreateEvents(t *testing.T) {
146146
ObjectMeta: v1.ObjectMeta{
147147
Name: "test",
148148
Namespace: "argocd",
149+
OwnerReferences: []v1.OwnerReference{
150+
{
151+
APIVersion: "argoproj.io/v1alpha1",
152+
Kind: "ApplicationSet",
153+
Name: "test",
154+
UID: "test",
155+
},
156+
},
149157
},
150158
Spec: v1alpha1.ApplicationSpec{
151159
Project: "test",
@@ -183,6 +191,8 @@ func Test_CreateEvents(t *testing.T) {
183191
assert.NoError(t, err)
184192
require.NotNil(t, napp)
185193
assert.Equal(t, "HEAD", napp.Spec.Source.TargetRevision)
194+
// OwnerReferences should be dropped on the control-plane application
195+
assert.Empty(t, napp.OwnerReferences)
186196
// Check that the destination is set to the cluster mapping
187197
assert.Equal(t, "foo", napp.Spec.Destination.Name)
188198
assert.Equal(t, "", napp.Spec.Destination.Server)
@@ -200,6 +210,14 @@ func Test_CreateEvents(t *testing.T) {
200210
ObjectMeta: v1.ObjectMeta{
201211
Name: "test",
202212
Namespace: "argocd",
213+
OwnerReferences: []v1.OwnerReference{
214+
{
215+
APIVersion: "argoproj.io/v1alpha1",
216+
Kind: "ApplicationSet",
217+
Name: "test",
218+
UID: "test",
219+
},
220+
},
203221
},
204222
Spec: v1alpha1.ApplicationSpec{
205223
Source: &v1alpha1.ApplicationSource{
@@ -219,6 +237,7 @@ func Test_CreateEvents(t *testing.T) {
219237
}
220238
exapp := app.DeepCopy()
221239
exapp.Namespace = "foo"
240+
exapp.OwnerReferences = nil
222241
fac := kube.NewKubernetesFakeClientWithApps(exapp)
223242
ev := cloudevents.NewEvent()
224243
ev.SetDataSchema("application")
@@ -237,6 +256,7 @@ func Test_CreateEvents(t *testing.T) {
237256
napp, err := fac.ApplicationsClientset.ArgoprojV1alpha1().Applications("foo").Get(context.TODO(), "test", v1.GetOptions{})
238257
assert.NoError(t, err)
239258
require.NotNil(t, napp)
259+
assert.Empty(t, napp.OwnerReferences)
240260
// Check that the destination is set to the cluster mapping
241261
assert.Equal(t, "foo", napp.Spec.Destination.Name)
242262
assert.Equal(t, "", napp.Spec.Destination.Server)
@@ -250,6 +270,14 @@ func Test_UpdateEvents(t *testing.T) {
250270
ObjectMeta: v1.ObjectMeta{
251271
Name: "test",
252272
Namespace: "argocd",
273+
OwnerReferences: []v1.OwnerReference{
274+
{
275+
APIVersion: "argoproj.io/v1alpha1",
276+
Kind: "ApplicationSet",
277+
Name: "test",
278+
UID: "test",
279+
},
280+
},
253281
},
254282
Spec: v1alpha1.ApplicationSpec{
255283
Project: "default",
@@ -308,6 +336,8 @@ func Test_UpdateEvents(t *testing.T) {
308336
assert.NoError(t, err)
309337
require.NotNil(t, napp)
310338
assert.Equal(t, "HEAD", napp.Spec.Source.TargetRevision)
339+
// OwnerReferences should be dropped on spec update in autonomous mode
340+
assert.Empty(t, napp.OwnerReferences)
311341
// Check that the destination is set to the cluster mapping
312342
assert.Equal(t, "foo", napp.Spec.Destination.Name)
313343
assert.Equal(t, "", napp.Spec.Destination.Server)
@@ -504,6 +534,14 @@ func Test_processAppProjectEvent(t *testing.T) {
504534
ObjectMeta: v1.ObjectMeta{
505535
Name: "test",
506536
Namespace: "argocd",
537+
OwnerReferences: []v1.OwnerReference{
538+
{
539+
APIVersion: "argoproj.io/v1alpha1",
540+
Kind: "ApplicationSet",
541+
Name: "owner",
542+
UID: "uid-1",
543+
},
544+
},
507545
},
508546
Spec: v1alpha1.AppProjectSpec{
509547
SourceRepos: []string{"foo"},
@@ -544,6 +582,8 @@ func Test_processAppProjectEvent(t *testing.T) {
544582
createdProject, err := fac.ApplicationsClientset.ArgoprojV1alpha1().AppProjects("argocd").Get(context.TODO(), projName, v1.GetOptions{})
545583
assert.NoError(t, err)
546584
assert.Equal(t, projName, createdProject.Name)
585+
// OwnerReferences should be dropped on the control-plane appProject
586+
assert.Empty(t, createdProject.OwnerReferences)
547587

548588
// Check that SourceNamespaces is set to the agent name
549589
assert.Equal(t, []string{"foo"}, createdProject.Spec.SourceNamespaces)
@@ -579,6 +619,8 @@ func Test_processAppProjectEvent(t *testing.T) {
579619
ev.SetType(event.SpecUpdate.String())
580620

581621
updatedProject := project.DeepCopy()
622+
// include owner refs in update payload and ensure they are dropped
623+
updatedProject.OwnerReferences = []v1.OwnerReference{{APIVersion: "argoproj.io/v1alpha1", Kind: "ApplicationSet", Name: "owner2", UID: "uid-2"}}
582624
updatedProject.Spec.Description = "updated"
583625
updatedProject.Name = "test" // Use original name (will be prefixed)
584626
ev.SetData(cloudevents.ApplicationJSON, updatedProject)
@@ -601,6 +643,8 @@ func Test_processAppProjectEvent(t *testing.T) {
601643
got, err := fac.ApplicationsClientset.ArgoprojV1alpha1().AppProjects("argocd").Get(context.TODO(), projName, v1.GetOptions{})
602644
assert.NoError(t, err)
603645
assert.Equal(t, updatedProject.Spec.Description, got.Spec.Description)
646+
// OwnerReferences should be dropped on update as well
647+
assert.Empty(t, got.OwnerReferences)
604648

605649
// Check that SourceNamespaces is set to the agent name
606650
assert.Equal(t, []string{"foo"}, got.Spec.SourceNamespaces)

0 commit comments

Comments
 (0)