From 3bf84e8b0ccd68aba9c17c061ce1d4774071cc28 Mon Sep 17 00:00:00 2001 From: Viktor Pettersson Date: Thu, 10 Jul 2025 10:52:12 +1200 Subject: [PATCH] fix(tags): reconcile edge relations prior to deletion [BE-11969] (#867) --- api/http/handler/tags/tag_delete.go | 44 ++++++-- api/http/handler/tags/tag_delete_test.go | 124 ++++++++++++++++++++--- 2 files changed, 147 insertions(+), 21 deletions(-) diff --git a/api/http/handler/tags/tag_delete.go b/api/http/handler/tags/tag_delete.go index 5e67c4f4c..f8f1b7786 100644 --- a/api/http/handler/tags/tag_delete.go +++ b/api/http/handler/tags/tag_delete.go @@ -107,14 +107,10 @@ func deleteTag(tx dataservices.DataStoreTx, tagID portainer.TagID) error { return httperror.InternalServerError("Unable to retrieve edge stacks from the database", err) } - for _, endpoint := range endpoints { - if (tag.Endpoints[endpoint.ID] || tag.EndpointGroups[endpoint.GroupID]) && endpointutils.IsEdgeEndpoint(&endpoint) { - if err := updateEndpointRelations(tx, endpoint, edgeGroups, edgeStacks); err != nil { - return httperror.InternalServerError("Unable to update environment relations in the database", err) - } - } + edgeJobs, err := tx.EdgeJob().ReadAll() + if err != nil { + return httperror.InternalServerError("Unable to retrieve edge job configurations from the database", err) } - for _, edgeGroup := range edgeGroups { edgeGroup.TagIDs = slices.DeleteFunc(edgeGroup.TagIDs, func(t portainer.TagID) bool { return t == tagID @@ -126,6 +122,16 @@ func deleteTag(tx dataservices.DataStoreTx, tagID portainer.TagID) error { } } + for _, endpoint := range endpoints { + if (!tag.Endpoints[endpoint.ID] && !tag.EndpointGroups[endpoint.GroupID]) || !endpointutils.IsEdgeEndpoint(&endpoint) { + continue + } + + if err := updateEndpointRelations(tx, endpoint, edgeGroups, edgeStacks, edgeJobs); err != nil { + return httperror.InternalServerError("Unable to update environment relations in the database", err) + } + } + err = tx.Tag().Delete(tagID) if err != nil { return httperror.InternalServerError("Unable to remove the tag from the database", err) @@ -134,7 +140,7 @@ func deleteTag(tx dataservices.DataStoreTx, tagID portainer.TagID) error { return nil } -func updateEndpointRelations(tx dataservices.DataStoreTx, endpoint portainer.Endpoint, edgeGroups []portainer.EdgeGroup, edgeStacks []portainer.EdgeStack) error { +func updateEndpointRelations(tx dataservices.DataStoreTx, endpoint portainer.Endpoint, edgeGroups []portainer.EdgeGroup, edgeStacks []portainer.EdgeStack, edgeJobs []portainer.EdgeJob) error { endpointRelation, err := tx.EndpointRelation().EndpointRelation(endpoint.ID) if err != nil { return err @@ -153,5 +159,25 @@ func updateEndpointRelations(tx dataservices.DataStoreTx, endpoint portainer.End endpointRelation.EdgeStacks = stacksSet - return tx.EndpointRelation().UpdateEndpointRelation(endpoint.ID, endpointRelation) + if err := tx.EndpointRelation().UpdateEndpointRelation(endpoint.ID, endpointRelation); err != nil { + return err + } + + for _, edgeJob := range edgeJobs { + endpoints, err := edge.GetEndpointsFromEdgeGroups(edgeJob.EdgeGroups, tx) + if err != nil { + return err + } + if slices.Contains(endpoints, endpoint.ID) { + continue + } + + delete(edgeJob.GroupLogsCollection, endpoint.ID) + + if err := tx.EdgeJob().Update(edgeJob.ID, &edgeJob); err != nil { + return err + } + } + + return nil } diff --git a/api/http/handler/tags/tag_delete_test.go b/api/http/handler/tags/tag_delete_test.go index a215e7edd..ecced7c0d 100644 --- a/api/http/handler/tags/tag_delete_test.go +++ b/api/http/handler/tags/tag_delete_test.go @@ -1,6 +1,7 @@ package tags import ( + "github.com/portainer/portainer/api/dataservices" "net/http" "net/http/httptest" "strconv" @@ -8,23 +9,18 @@ import ( "testing" portainer "github.com/portainer/portainer/api" + portainerDsErrors "github.com/portainer/portainer/api/dataservices/errors" "github.com/portainer/portainer/api/datastore" "github.com/portainer/portainer/api/internal/testhelpers" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestTagDeleteEdgeGroupsConcurrently(t *testing.T) { const tagsCount = 100 - _, store := datastore.MustNewTestStore(t, true, false) - - user := &portainer.User{ID: 2, Username: "admin", Role: portainer.AdministratorRole} - if err := store.User().Create(user); err != nil { - t.Fatal("could not create admin user:", err) - } - - handler := NewHandler(testhelpers.NewTestRequestBouncer()) - handler.DataStore = store - + handler, store := setUpHandler(t) // Create all the tags and add them to the same edge group var tagIDs []portainer.TagID @@ -85,11 +81,101 @@ func TestTagDeleteEdgeGroupsConcurrently(t *testing.T) { } } -func TestDeleteTag(t *testing.T) { - _, store := datastore.MustNewTestStore(t, true, false) +func TestHandler_tagDelete(t *testing.T) { + t.Run("should delete tag and update related endpoints and edge groups", func(t *testing.T) { + handler, store := setUpHandler(t) + + tag := &portainer.Tag{ + ID: 1, + Name: "tag-1", + Endpoints: make(map[portainer.EndpointID]bool), + EndpointGroups: make(map[portainer.EndpointGroupID]bool), + } + require.NoError(t, store.Tag().Create(tag)) + + endpointGroup := &portainer.EndpointGroup{ + ID: 2, + Name: "endpoint-group-1", + TagIDs: []portainer.TagID{tag.ID}, + } + require.NoError(t, store.EndpointGroup().Create(endpointGroup)) + + endpoint1 := &portainer.Endpoint{ + ID: 1, + Name: "endpoint-1", + GroupID: endpointGroup.ID, + } + require.NoError(t, store.Endpoint().Create(endpoint1)) + + endpoint2 := &portainer.Endpoint{ + ID: 2, + Name: "endpoint-2", + TagIDs: []portainer.TagID{tag.ID}, + } + require.NoError(t, store.Endpoint().Create(endpoint2)) + + tag.Endpoints[endpoint2.ID] = true + tag.EndpointGroups[endpointGroup.ID] = true + require.NoError(t, store.Tag().Update(tag.ID, tag)) + + dynamicEdgeGroup := &portainer.EdgeGroup{ + ID: 1, + Name: "edgegroup-1", + TagIDs: []portainer.TagID{tag.ID}, + Dynamic: true, + } + require.NoError(t, store.EdgeGroup().Create(dynamicEdgeGroup)) + + staticEdgeGroup := &portainer.EdgeGroup{ + ID: 2, + Name: "edgegroup-2", + Endpoints: []portainer.EndpointID{endpoint2.ID}, + } + require.NoError(t, store.EdgeGroup().Create(staticEdgeGroup)) + + req, err := http.NewRequest(http.MethodDelete, "/tags/"+strconv.Itoa(int(tag.ID)), nil) + if err != nil { + t.Fail() + + return + } + + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + require.Equal(t, http.StatusNoContent, rec.Code) + + // Check that the tag is deleted + _, err = store.Tag().Read(tag.ID) + require.ErrorIs(t, err, portainerDsErrors.ErrObjectNotFound) + + // Check that the endpoints are updated + endpoint1, err = store.Endpoint().Endpoint(endpoint1.ID) + require.NoError(t, err) + assert.Len(t, endpoint1.TagIDs, 0, "endpoint-1 should not have any tags") + assert.Equal(t, endpoint1.GroupID, endpointGroup.ID, "endpoint-1 should still belong to the endpoint group") + + endpoint2, err = store.Endpoint().Endpoint(endpoint2.ID) + require.NoError(t, err) + assert.Len(t, endpoint2.TagIDs, 0, "endpoint-2 should not have any tags") + + // Check that the dynamic edge group is updated + dynamicEdgeGroup, err = store.EdgeGroup().Read(dynamicEdgeGroup.ID) + require.NoError(t, err) + assert.Len(t, dynamicEdgeGroup.TagIDs, 0, "dynamic edge group should not have any tags") + assert.Len(t, dynamicEdgeGroup.Endpoints, 0, "dynamic edge group should not have any endpoints") + + // Check that the static edge group is not updated + staticEdgeGroup, err = store.EdgeGroup().Read(staticEdgeGroup.ID) + require.NoError(t, err) + assert.Len(t, staticEdgeGroup.TagIDs, 0, "static edge group should not have any tags") + assert.Len(t, staticEdgeGroup.Endpoints, 1, "static edge group should have one endpoint") + assert.Equal(t, endpoint2.ID, staticEdgeGroup.Endpoints[0], "static edge group should have the endpoint-2") + }) // Test the tx.IsErrObjectNotFound logic when endpoint is not found during cleanup t.Run("should continue gracefully when endpoint not found during cleanup", func(t *testing.T) { + _, store := setUpHandler(t) // Create a tag with a reference to a non-existent endpoint tag := &portainer.Tag{ ID: 1, @@ -109,3 +195,17 @@ func TestDeleteTag(t *testing.T) { } }) } + +func setUpHandler(t *testing.T) (*Handler, dataservices.DataStore) { + _, store := datastore.MustNewTestStore(t, true, false) + + user := &portainer.User{ID: 2, Username: "admin", Role: portainer.AdministratorRole} + if err := store.User().Create(user); err != nil { + t.Fatal("could not create admin user:", err) + } + + handler := NewHandler(testhelpers.NewTestRequestBouncer()) + handler.DataStore = store + + return handler, store +}