Skip to content
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

feat: handle UID mismatch between incoming and existing resources #242

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: handle UID mismatch between incoming and existing resources #242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 49 additions & 2 deletions agent/inbound.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,36 @@ func (a *Agent) processIncomingApplication(ev *event.Event) error {
return err
}

sourceUIDMatch, err := a.appManager.CompareSourceUID(a.context, incomingApp)
if err != nil {
return fmt.Errorf("failed to compare the source UID of app: %w", err)
}

switch ev.Type() {
case event.Create:
if !sourceUIDMatch {
logCtx.Debug("An app already exists with a different source UID. Deleting the existing app")
if err := a.deleteApplication(incomingApp); err != nil {
return err
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should give more context to the returned error here.

The caller will expect an application to be created, and probably reflect that in any error message logged, but could potentially receive an error from the delete operation.

For example, the following case would be confusing (admittedly, the chance for this happening is very slim, but it should ):

  1. A create event is sent for an application where sourceUIDMatch is false
  2. The application is deleted on the cluster by a third party
  3. a.deleteApplication fails because the resource already is deleted. The error would be "the requested resource could not be found"
  4. That error is passed back to the caller
  5. The error log for the caller would look similar to "Could not create app foo: requested resource not found"

Or think about RBAC - agent might have RBAC to create applications, but not delete applications, but the create call would return a permission denied. Troubleshooting that would be fun :)

So it would make more sense to augment the returned error by more information, e.g. fmt.Errorf("could not delete existing resource prior to creation: %w", err) or something similar.

}
}

_, err = a.createApplication(incomingApp)
if err != nil {
logCtx.Errorf("Error creating application: %v", err)
}
case event.SpecUpdate:
if !sourceUIDMatch {
logCtx.Debug("Source UID mismatch between the incoming app and existing app. Deleting the existing app")
if err := a.deleteApplication(incomingApp); err != nil {
return err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same about augmenting the error as above.

}

logCtx.Debug("Creating the incoming app after deleting the existing app")
_, err := a.createApplication(incomingApp)
return err
}

_, err = a.updateApplication(incomingApp)
if err != nil {
logCtx.Errorf("Error updating application: %v", err)
Expand All @@ -87,13 +110,36 @@ func (a *Agent) processIncomingAppProject(ev *event.Event) error {
return err
}

sourceUIDMatch, err := a.projectManager.CompareSourceUID(a.context, incomingAppProject)
if err != nil {
return fmt.Errorf("failed to validate source UID of appProject: %w", err)
}

switch ev.Type() {
case event.Create:
if !sourceUIDMatch {
logCtx.Debug("An appProject already exists with a different source UID. Deleting the existing appProject")
if err := a.deleteAppProject(incomingAppProject); err != nil {
return err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same about augmenting the error as above.

}
}

_, err = a.createAppProject(incomingAppProject)
if err != nil {
logCtx.Errorf("Error creating appproject: %v", err)
}
case event.SpecUpdate:
if !sourceUIDMatch {
logCtx.Debug("Source UID mismatch between the incoming and existing appProject. Deleting the existing appProject")
if err := a.deleteAppProject(incomingAppProject); err != nil {
return err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same about augmenting the error as above.

}

logCtx.Debug("Creating the incoming appProject after deleting the existing appProject")
_, err := a.createAppProject(incomingAppProject)
return err
}

_, err = a.updateAppProject(incomingAppProject)
if err != nil {
logCtx.Errorf("Error updating appproject: %v", err)
Expand Down Expand Up @@ -153,13 +199,13 @@ func (a *Agent) createApplication(incoming *v1alpha1.Application) (*v1alpha1.App
}

func (a *Agent) updateApplication(incoming *v1alpha1.Application) (*v1alpha1.Application, error) {
//
incoming.ObjectMeta.SetNamespace(a.namespace)
logCtx := log().WithFields(logrus.Fields{
"method": "UpdateApplication",
"app": incoming.QualifiedName(),
"resourceVersion": incoming.ResourceVersion,
})

if a.appManager.IsChangeIgnored(incoming.QualifiedName(), incoming.ResourceVersion) {
logCtx.Tracef("Discarding this event, because agent has seen this version %s already", incoming.ResourceVersion)
return nil, event.NewEventDiscardedErr("the version %s has already been seen by this agent", incoming.ResourceVersion)
Expand Down Expand Up @@ -262,6 +308,7 @@ func (a *Agent) updateAppProject(incoming *v1alpha1.AppProject) (*v1alpha1.AppPr
"app": incoming.Name,
"resourceVersion": incoming.ResourceVersion,
})

if a.appManager.IsChangeIgnored(incoming.Name, incoming.ResourceVersion) {
logCtx.Tracef("Discarding this event, because agent has seen this version %s already", incoming.ResourceVersion)
return nil, event.NewEventDiscardedErr("the version %s has already been seen by this agent", incoming.ResourceVersion)
Expand All @@ -287,7 +334,7 @@ func (a *Agent) deleteAppProject(project *v1alpha1.AppProject) error {
// means that we're out-of-sync from the control plane.
//
// TODO(jannfis): Handle this situation properly instead of throwing an error.
if !a.appManager.IsManaged(project.Name) {
if !a.projectManager.IsManaged(project.Name) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

return fmt.Errorf("appProject %s is not managed", project.Name)
}

Expand Down
266 changes: 266 additions & 0 deletions agent/inbound_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@ import (
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"

backend_mocks "github.com/argoproj-labs/argocd-agent/internal/backend/mocks"
"github.com/argoproj-labs/argocd-agent/internal/event"
"github.com/argoproj-labs/argocd-agent/internal/manager"
"github.com/argoproj-labs/argocd-agent/internal/manager/application"
"github.com/argoproj-labs/argocd-agent/internal/manager/appproject"
"github.com/argoproj-labs/argocd-agent/pkg/types"
ktypes "k8s.io/apimachinery/pkg/types"
)

func Test_CreateApplication(t *testing.T) {
Expand Down Expand Up @@ -68,6 +70,270 @@ func Test_CreateApplication(t *testing.T) {

}

func Test_ProcessIncomingAppWithUIDMismatch(t *testing.T) {
a := newAgent(t)
a.mode = types.AgentModeManaged
evs := event.NewEventSource("test")
var be *backend_mocks.Application
var getMock, createMock, deleteMock *mock.Call

// incomingApp is the app sent by the principal
incomingApp := &v1alpha1.Application{ObjectMeta: v1.ObjectMeta{
Name: "test",
Namespace: "argocd",
}}

// oldApp is the app that is already present on the agent
oldApp := incomingApp.DeepCopy()
oldApp.Annotations = map[string]string{
manager.SourceUIDAnnotation: "old_uid",
}
a.appManager.Manage(oldApp.QualifiedName())
defer a.appManager.ClearManaged()

incomingApp.UID = ktypes.UID("new_uid")

// createdApp is the app that is finally created on the cluster.
createdApp := incomingApp.DeepCopy()
createdApp.UID = ktypes.UID("random_uid")
createdApp.Annotations = map[string]string{
// the final version of the app should have the source UID from the incomingApp
manager.SourceUIDAnnotation: string(incomingApp.UID),
}

configureManager := func(t *testing.T) {
t.Helper()
be = backend_mocks.NewApplication(t)
var err error
a.appManager, err = application.NewApplicationManager(be, "argocd", application.WithAllowUpsert(true))
require.NoError(t, err)
require.NotNil(t, a)

// Get is used to retrieve the oldApp and compare its UID annotation.
getMock = be.On("Get", mock.Anything, mock.Anything, mock.Anything).Return(oldApp, nil)

// Create is used to create the latest version of the app on the agent.
createMock = be.On("Create", mock.Anything, mock.Anything).Return(createdApp, nil)

// Delete is used to delete the oldApp from the agent.
deleteMock = be.On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
Return(nil)
}

unsetMocks := func(t *testing.T) {
t.Helper()
getMock.Unset()
createMock.Unset()
deleteMock.Unset()
}

t.Run("Create: Old app with diff UID must be deleted before creating the incoming app", func(t *testing.T) {
configureManager(t)
defer unsetMocks(t)
a.appManager.Manage(oldApp.QualifiedName())
defer a.appManager.ClearManaged()

ev := event.New(evs.ApplicationEvent(event.Create, incomingApp), event.TargetApplication)
err := a.processIncomingApplication(ev)
require.Nil(t, err)

// Check if the API calls were made in the same order:
// compare the UID, delete old app, and create a new app.
expectedCalls := []string{"Get", "Delete", "Create"}
gotCalls := []string{}
for _, call := range be.Calls {
gotCalls = append(gotCalls, call.Method)
}
require.Equal(t, expectedCalls, gotCalls)

// Check if the new app has the updated source UID annotation.
appInterface := be.Calls[2].ReturnArguments[0]
latestApp, ok := appInterface.(*v1alpha1.Application)
require.True(t, ok)
require.Equal(t, string(incomingApp.UID), latestApp.Annotations[manager.SourceUIDAnnotation])
})

t.Run("Update: Old app with diff UID must be deleted and a new app must be created", func(t *testing.T) {
configureManager(t)
defer unsetMocks(t)
a.appManager.Manage(oldApp.QualifiedName())
defer a.appManager.ClearManaged()

// Create an Update event for the new app
ev := event.New(evs.ApplicationEvent(event.SpecUpdate, incomingApp), event.TargetApplication)
err := a.processIncomingApplication(ev)
require.Nil(t, err)

// Check if the API calls were made in the same order:
// compare the UID, delete old app, and create a new app.
expectedCalls := []string{"Get", "Delete", "Create"}
gotCalls := []string{}
for _, call := range be.Calls {
gotCalls = append(gotCalls, call.Method)
}
require.Equal(t, expectedCalls, gotCalls)

// Check if the new app has the updated source UID annotation.
appInterface := be.Calls[2].ReturnArguments[0]

latestApp, ok := appInterface.(*v1alpha1.Application)
require.True(t, ok)
require.Equal(t, string(incomingApp.UID), latestApp.Annotations[manager.SourceUIDAnnotation])
})

t.Run("Delete: Old app with diff UID must be deleted", func(t *testing.T) {
configureManager(t)
defer unsetMocks(t)
a.appManager.Manage(oldApp.QualifiedName())
defer a.appManager.ClearManaged()

// Create a delete event for the new app
ev := event.New(evs.ApplicationEvent(event.Delete, incomingApp), event.TargetApplication)
err := a.processIncomingApplication(ev)
require.Nil(t, err)

// Check if the API calls were made in the same order:
// compare the UID and delete old app
expectedCalls := []string{"Get", "Delete"}
gotCalls := []string{}
for _, call := range be.Calls {
gotCalls = append(gotCalls, call.Method)
}
require.Equal(t, expectedCalls, gotCalls)
require.False(t, a.appManager.IsManaged(incomingApp.QualifiedName()))
})
}

func Test_ProcessIncomingAppProjectWithUIDMismatch(t *testing.T) {
a := newAgent(t)
a.mode = types.AgentModeManaged
evs := event.NewEventSource("test")
var be *backend_mocks.AppProject
var getMock, createMock, deleteMock *mock.Call

// incomingAppProject is the appProject sent by the principal
incomingAppProject := &v1alpha1.AppProject{ObjectMeta: v1.ObjectMeta{
Name: "test",
Namespace: "argocd",
}}

// oldAppProject is the appProject that is already present on the agent
oldAppProject := incomingAppProject.DeepCopy()
oldAppProject.Annotations = map[string]string{
manager.SourceUIDAnnotation: "old_uid",
}

incomingAppProject.UID = ktypes.UID("new_uid")

// createdAppProject is the final version of appProject created on the cluster.
createdAppProject := incomingAppProject.DeepCopy()
createdAppProject.UID = ktypes.UID("random_uid")
createdAppProject.Annotations = map[string]string{
manager.SourceUIDAnnotation: string(incomingAppProject.UID),
}

configureManager := func(t *testing.T) {
t.Helper()
be = backend_mocks.NewAppProject(t)
var err error
a.projectManager, err = appproject.NewAppProjectManager(be, "argocd", appproject.WithAllowUpsert(true))
require.NoError(t, err)
require.NotNil(t, a)

// Get is used to retrieve the oldAppProject and compare its UID annotation.
getMock = be.On("Get", mock.Anything, mock.Anything, mock.Anything).Return(oldAppProject, nil)

// Create is used to create the latest version of the appProject on the agent.
createMock = be.On("Create", mock.Anything, mock.Anything).Return(createdAppProject, nil)

// Delete is used to delete the oldAppProject from the agent.
deleteMock = be.On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
Return(nil)
}

unsetMocks := func(t *testing.T) {
t.Helper()
getMock.Unset()
createMock.Unset()
deleteMock.Unset()
}

t.Run("Create: Old appProject with diff UID must be deleted before creating the incoming appProject", func(t *testing.T) {
configureManager(t)
defer unsetMocks(t)
a.projectManager.Manage(oldAppProject.Name)
defer a.projectManager.ClearManaged()
ev := event.New(evs.AppProjectEvent(event.Create, incomingAppProject), event.TargetAppProject)
err := a.processIncomingAppProject(ev)
require.Nil(t, err)

// Check if the API calls were made in the same order:
// compare the UID, delete old appProject, and create a new appProject.
expectedCalls := []string{"Get", "Delete", "Create"}
gotCalls := []string{}
for _, call := range be.Calls {
gotCalls = append(gotCalls, call.Method)
}
require.Equal(t, expectedCalls, gotCalls)

// Check if the new app has the updated source UID annotation.
appInterface := be.Calls[2].ReturnArguments[0]
latestAppProject, ok := appInterface.(*v1alpha1.AppProject)
require.True(t, ok)
require.Equal(t, string(incomingAppProject.UID), latestAppProject.Annotations[manager.SourceUIDAnnotation])
})

t.Run("Update: Old appProject with diff UID must be deleted and a new appProject must be created", func(t *testing.T) {
configureManager(t)
defer unsetMocks(t)
a.projectManager.Manage(oldAppProject.Name)
defer a.projectManager.ClearManaged()

// Create an Update event for the new appProject
ev := event.New(evs.AppProjectEvent(event.SpecUpdate, incomingAppProject), event.TargetAppProject)
err := a.processIncomingAppProject(ev)
require.Nil(t, err)

// Check if the API calls were made in the same order:
// compare the UID, delete old appProject, and create a new appProject.
expectedCalls := []string{"Get", "Delete", "Create"}
gotCalls := []string{}
for _, call := range be.Calls {
gotCalls = append(gotCalls, call.Method)
}
require.Equal(t, expectedCalls, gotCalls)

// Check if the new appProject has the updated source UID annotation.
appInterface := be.Calls[2].ReturnArguments[0]

latestAppProject, ok := appInterface.(*v1alpha1.AppProject)
require.True(t, ok)
require.Equal(t, string(incomingAppProject.UID), latestAppProject.Annotations[manager.SourceUIDAnnotation])
})

t.Run("Delete: Old appProject with the same UID must be deleted", func(t *testing.T) {
configureManager(t)
defer unsetMocks(t)
a.projectManager.Manage(oldAppProject.Name)
defer a.projectManager.ClearManaged()

// Create a delete event for the new appProject
ev := event.New(evs.AppProjectEvent(event.Delete, incomingAppProject), event.TargetAppProject)
err := a.processIncomingAppProject(ev)
require.Nil(t, err)

// Check if the API calls were made in the same order:
// compare the UID and delete old appProject
expectedCalls := []string{"Get", "Delete"}
gotCalls := []string{}
for _, call := range be.Calls {
gotCalls = append(gotCalls, call.Method)
}
require.Equal(t, expectedCalls, gotCalls)
require.False(t, a.appManager.IsManaged(incomingAppProject.Name))
})
}

func Test_UpdateApplication(t *testing.T) {
a := newAgent(t)
be := backend_mocks.NewApplication(t)
Expand Down
Loading
Loading