fix(pendingactions): refactor pending actions [EE-7011] (#11780)

api/pendingactions/actions/actions.go

@@ -1,7 +1,7 @@
 package actions
 
 const (
-	CleanNAPWithOverridePolicies      = "CleanNAPWithOverridePolicies"
-	DeletePortainerK8sRegistrySecrets = "DeletePortainerK8sRegistrySecrets"
-	PostInitMigrateEnvironment        = "PostInitMigrateEnvironment"
+	CleanNAPWithOverridePolicies = "CleanNAPWithOverridePolicies"
+	DeleteK8sRegistrySecrets     = "DeleteK8sRegistrySecrets"
+	PostInitMigrateEnvironment   = "PostInitMigrateEnvironment"
 )

api/pendingactions/actions/converters.go

@@ -1,44 +0,0 @@
-package actions
-
-import (
-	"fmt"
-
-	portainer "github.com/portainer/portainer/api"
-)
-
-type (
-	CleanNAPWithOverridePoliciesPayload struct {
-		EndpointGroupID portainer.EndpointGroupID
-	}
-)
-
-func ConvertCleanNAPWithOverridePoliciesPayload(actionData interface{}) (*CleanNAPWithOverridePoliciesPayload, error) {
-	var payload CleanNAPWithOverridePoliciesPayload
-
-	if actionData == nil {
-		return nil, nil
-	}
-
-	// backward compatible with old data format
-	if endpointGroupId, ok := actionData.(float64); ok {
-		payload.EndpointGroupID = portainer.EndpointGroupID(endpointGroupId)
-		return &payload, nil
-	}
-
-	data, ok := actionData.(map[string]interface{})
-	if !ok {
-		return nil, fmt.Errorf("failed to convert actionData to map[string]interface{}")
-
-	}
-
-	for key, value := range data {
-		switch key {
-		case "EndpointGroupID":
-			if endpointGroupID, ok := value.(float64); ok {
-				payload.EndpointGroupID = portainer.EndpointGroupID(endpointGroupID)
-			}
-		}
-	}
-
-	return &payload, nil
-}

api/pendingactions/delete_registry_secrets.go

@@ -1,76 +0,0 @@
-package pendingactions
-
-import (
-	"fmt"
-
-	portainer "github.com/portainer/portainer/api"
-	"github.com/rs/zerolog/log"
-)
-
-type DeletePortainerK8sRegistrySecretsData struct {
-	RegistryID portainer.RegistryID `json:"RegistryID"`
-	Namespaces []string             `json:"Namespaces"`
-}
-
-func (service *PendingActionsService) DeleteKubernetesRegistrySecrets(endpoint *portainer.Endpoint, registryData *DeletePortainerK8sRegistrySecretsData) error {
-	if endpoint == nil || registryData == nil {
-		return nil
-	}
-
-	kubeClient, err := service.kubeFactory.GetKubeClient(endpoint)
-	if err != nil {
-		return err
-	}
-
-	for _, namespace := range registryData.Namespaces {
-		err = kubeClient.DeleteRegistrySecret(registryData.RegistryID, namespace)
-		if err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
-// Failure in this code is basically a bug.  So if we get one we should log it and continue.
-func convertToDeletePortainerK8sRegistrySecretsData(actionData interface{}) (*DeletePortainerK8sRegistrySecretsData, error) {
-	var registryData DeletePortainerK8sRegistrySecretsData
-
-	// Due to the way data is stored and subsequently read from the database, we can't directly type assert the actionData to
-	// the type DeletePortainerK8sRegistrySecretsData.  It's stored as a map[string]interface{} and we need to extract the
-	// data from that map.
-	if data, ok := actionData.(map[string]interface{}); ok {
-		for key, value := range data {
-			switch key {
-			case "Namespaces":
-				if namespaces, ok := value.([]interface{}); ok {
-					registryData.Namespaces = make([]string, len(namespaces))
-					for i, ns := range namespaces {
-						if namespace, ok := ns.(string); ok {
-							registryData.Namespaces[i] = namespace
-						}
-					}
-				} else {
-					// we shouldn't ever see this.  It's a bug if we do.
-					log.Debug().Msgf("DeletePortainerK8sRegistrySecrets: Failed to convert Namespaces to []interface{}")
-				}
-			case "RegistryID":
-				if registryID, ok := value.(float64); ok {
-					registryData.RegistryID = portainer.RegistryID(registryID)
-				} else {
-					// we shouldn't ever see this.  It's a bug if we do.
-					log.Debug().Msgf("DeletePortainerK8sRegistrySecrets: Failed to convert RegistryID to float64")
-				}
-			}
-		}
-
-		log.Debug().Msgf("DeletePortainerK8sRegistrySecrets: %+v", registryData)
-	} else {
-		// this should not happen.  It's a bug if it does. As the actionData is defined
-		// by what portainer puts in it.  It never comes from a user or external source so it shouldn't fail.
-		// Nevertheless we should check it in case of db corruption or developer mistake down the road
-		return nil, fmt.Errorf("type assertion failed in convertToDeletePortainerK8sRegistrySecretsData")
-	}
-
-	return &registryData, nil
-}

api/pendingactions/handlers/clean_nap_with_override_policies.go

@@ -0,0 +1,82 @@
+package handlers
+
+import (
+	"fmt"
+
+	portainer "github.com/portainer/portainer/api"
+	"github.com/portainer/portainer/api/dataservices"
+	"github.com/portainer/portainer/api/internal/authorization"
+	"github.com/portainer/portainer/api/pendingactions/actions"
+	"github.com/rs/zerolog/log"
+)
+
+type (
+	cleanNAPWithOverridePolicies struct {
+		EndpointGroupID portainer.EndpointGroupID
+	}
+
+	HandlerCleanNAPWithOverridePolicies struct {
+		authorizationService *authorization.Service
+		dataStore            dataservices.DataStore
+	}
+)
+
+// NewCleanNAPWithOverridePolicies creates a new CleanNAPWithOverridePolicies pending action
+func NewCleanNAPWithOverridePolicies(endpointID portainer.EndpointID, gid *portainer.EndpointGroupID) portainer.PendingAction {
+	pendingAction := portainer.PendingAction{
+		EndpointID: endpointID,
+		Action:     actions.CleanNAPWithOverridePolicies,
+	}
+
+	if gid != nil {
+		pendingAction.ActionData = cleanNAPWithOverridePolicies{
+			EndpointGroupID: *gid,
+		}
+	}
+
+	return pendingAction
+}
+
+// NewHandlerCleanNAPWithOverridePolicies creates a new handler to execute CleanNAPWithOverridePolicies pending action
+func NewHandlerCleanNAPWithOverridePolicies(
+	authorizationService *authorization.Service,
+	dataStore dataservices.DataStore,
+) *HandlerCleanNAPWithOverridePolicies {
+	return &HandlerCleanNAPWithOverridePolicies{
+		authorizationService: authorizationService,
+		dataStore:            dataStore,
+	}
+}
+
+func (h *HandlerCleanNAPWithOverridePolicies) Execute(pendingAction portainer.PendingAction, endpoint *portainer.Endpoint) error {
+	if pendingAction.ActionData == nil {
+		h.authorizationService.CleanNAPWithOverridePolicies(h.dataStore, endpoint, nil)
+		return nil
+	}
+
+	var payload cleanNAPWithOverridePolicies
+	err := pendingAction.UnmarshallActionData(&payload)
+	if err != nil {
+		log.Error().Err(err).Msgf("Error unmarshalling endpoint group ID for cleaning NAP with override policies for environment %d", endpoint.ID)
+		return fmt.Errorf("failed to unmarshal endpoint group ID for cleaning NAP with override policies for environment %d: %w", endpoint.ID, err)
+	}
+
+	if payload.EndpointGroupID == 0 {
+		h.authorizationService.CleanNAPWithOverridePolicies(h.dataStore, endpoint, nil)
+		return nil
+	}
+
+	endpointGroup, err := h.dataStore.EndpointGroup().Read(portainer.EndpointGroupID(payload.EndpointGroupID))
+	if err != nil {
+		log.Error().Err(err).Msgf("Error reading environment group to clean NAP with override policies for environment %d and environment group %d", endpoint.ID, endpointGroup.ID)
+		return fmt.Errorf("failed to retrieve environment group %d: %w", payload.EndpointGroupID, err)
+	}
+
+	err = h.authorizationService.CleanNAPWithOverridePolicies(h.dataStore, endpoint, endpointGroup)
+	if err != nil {
+		log.Error().Err(err).Msgf("Error cleaning NAP with override policies for environment %d and environment group %d", endpoint.ID, endpointGroup.ID)
+		return fmt.Errorf("failed to clean NAP with override policies for environment %d and environment group %d: %w", endpoint.ID, endpointGroup.ID, err)
+	}
+
+	return nil
+}

api/pendingactions/handlers/delete_k8s_registry_secrets.go

@@ -0,0 +1,73 @@
+package handlers
+
+import (
+	portainer "github.com/portainer/portainer/api"
+	"github.com/portainer/portainer/api/dataservices"
+	"github.com/portainer/portainer/api/internal/authorization"
+	kubecli "github.com/portainer/portainer/api/kubernetes/cli"
+	"github.com/portainer/portainer/api/pendingactions/actions"
+)
+
+type (
+	HandlerDeleteK8sRegistrySecrets struct {
+		authorizationService *authorization.Service
+		dataStore            dataservices.DataStore
+		kubeFactory          *kubecli.ClientFactory
+	}
+
+	deleteK8sRegistrySecretsData struct {
+		RegistryID portainer.RegistryID `json:"RegistryID"`
+		Namespaces []string             `json:"Namespaces"`
+	}
+)
+
+// NewDeleteK8sRegistrySecrets creates a new DeleteK8sRegistrySecrets pending action
+func NewDeleteK8sRegistrySecrets(endpointID portainer.EndpointID, registryID portainer.RegistryID, namespaces []string) portainer.PendingAction {
+	return portainer.PendingAction{
+		EndpointID: endpointID,
+		Action:     actions.DeleteK8sRegistrySecrets,
+		ActionData: &deleteK8sRegistrySecretsData{
+			RegistryID: registryID,
+			Namespaces: namespaces,
+		},
+	}
+}
+
+// NewHandlerDeleteRegistrySecrets creates a new handler to execute DeleteK8sRegistrySecrets pending action
+func NewHandlerDeleteRegistrySecrets(
+	authorizationService *authorization.Service,
+	dataStore dataservices.DataStore,
+	kubeFactory *kubecli.ClientFactory,
+) *HandlerDeleteK8sRegistrySecrets {
+	return &HandlerDeleteK8sRegistrySecrets{
+		authorizationService: authorizationService,
+		dataStore:            dataStore,
+		kubeFactory:          kubeFactory,
+	}
+}
+
+func (h *HandlerDeleteK8sRegistrySecrets) Execute(pa portainer.PendingAction, endpoint *portainer.Endpoint) error {
+	if endpoint == nil || pa.ActionData == nil {
+		return nil
+	}
+
+	var registryData deleteK8sRegistrySecretsData
+	err := pa.UnmarshallActionData(&registryData)
+	if err != nil {
+		return err
+	}
+
+	kubeClient, err := h.kubeFactory.GetKubeClient(endpoint)
+	if err != nil {
+		return err
+	}
+
+	for _, namespace := range registryData.Namespaces {
+		err = kubeClient.DeleteRegistrySecret(registryData.RegistryID, namespace)
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}

api/pendingactions/handlers/post_init_migrate_environment.go

@@ -0,0 +1,58 @@
+package handlers
+
+import (
+	"fmt"
+
+	portainer "github.com/portainer/portainer/api"
+	"github.com/portainer/portainer/api/dataservices"
+	"github.com/portainer/portainer/api/datastore/postinit"
+	dockerClient "github.com/portainer/portainer/api/docker/client"
+	"github.com/portainer/portainer/api/internal/authorization"
+	kubecli "github.com/portainer/portainer/api/kubernetes/cli"
+	"github.com/rs/zerolog/log"
+)
+
+type HandlerPostInitMigrateEnvironment struct {
+	authorizationService *authorization.Service
+	dataStore            dataservices.DataStore
+	kubeFactory          *kubecli.ClientFactory
+	dockerFactory        *dockerClient.ClientFactory
+	assetsPath           string
+	kubernetesDeployer   portainer.KubernetesDeployer
+}
+
+// NewPostInitMigrateEnvironment creates a new PostInitMigrateEnvironment pending action
+func NewHandlerPostInitMigrateEnvironment(
+	authorizationService *authorization.Service,
+	dataStore dataservices.DataStore,
+	kubeFactory *kubecli.ClientFactory,
+	dockerFactory *dockerClient.ClientFactory,
+	assetsPath string,
+	kubernetesDeployer portainer.KubernetesDeployer,
+) *HandlerPostInitMigrateEnvironment {
+	return &HandlerPostInitMigrateEnvironment{
+		authorizationService: authorizationService,
+		dataStore:            dataStore,
+		kubeFactory:          kubeFactory,
+		dockerFactory:        dockerFactory,
+		assetsPath:           assetsPath,
+		kubernetesDeployer:   kubernetesDeployer,
+	}
+}
+
+func (h *HandlerPostInitMigrateEnvironment) Execute(_ portainer.PendingAction, endpoint *portainer.Endpoint) error {
+	postInitMigrator := postinit.NewPostInitMigrator(
+		h.kubeFactory,
+		h.dockerFactory,
+		h.dataStore,
+		h.assetsPath,
+		h.kubernetesDeployer,
+	)
+	err := postInitMigrator.MigrateEnvironment(endpoint)
+	if err != nil {
+		log.Error().Err(err).Msgf("Error running post-init migrations for edge environment %d", endpoint.ID)
+		return fmt.Errorf("failed running post-init migrations for edge environment %d: %w", endpoint.ID, err)
+	}
+
+	return nil
+}

api/pendingactions/pendingactions.go

@@ -1,62 +1,44 @@
 package pendingactions
 
 import (
-	"context"
 	"fmt"
 	"sync"
 
 	portainer "github.com/portainer/portainer/api"
 	"github.com/portainer/portainer/api/dataservices"
-	"github.com/portainer/portainer/api/datastore/postinit"
-	dockerClient "github.com/portainer/portainer/api/docker/client"
-	"github.com/portainer/portainer/api/internal/authorization"
 	"github.com/portainer/portainer/api/internal/endpointutils"
 	kubecli "github.com/portainer/portainer/api/kubernetes/cli"
-	"github.com/portainer/portainer/api/pendingactions/actions"
 	"github.com/rs/zerolog/log"
 )
 
-type (
-	PendingActionsService struct {
-		authorizationService *authorization.Service
-		kubeFactory          *kubecli.ClientFactory
-		dockerFactory        *dockerClient.ClientFactory
-		dataStore            dataservices.DataStore
-		shutdownCtx          context.Context
-		assetsPath           string
-		kubernetesDeployer   portainer.KubernetesDeployer
+type PendingActionsService struct {
+	kubeFactory *kubecli.ClientFactory
+	dataStore   dataservices.DataStore
+	mu          sync.Mutex
+}
 
-		mu sync.Mutex
-	}
-)
+var handlers = make(map[string]portainer.PendingActionHandler)
 
 func NewService(
 	dataStore dataservices.DataStore,
 	kubeFactory *kubecli.ClientFactory,
-	dockerFactory *dockerClient.ClientFactory,
-	authorizationService *authorization.Service,
-	shutdownCtx context.Context,
-	assetsPath string,
-	kubernetesDeployer portainer.KubernetesDeployer,
 ) *PendingActionsService {
 	return &PendingActionsService{
-		dataStore:            dataStore,
-		shutdownCtx:          shutdownCtx,
-		authorizationService: authorizationService,
-		kubeFactory:          kubeFactory,
-		dockerFactory:        dockerFactory,
-		assetsPath:           assetsPath,
-		kubernetesDeployer:   kubernetesDeployer,
-		mu:                   sync.Mutex{},
+		dataStore:   dataStore,
+		kubeFactory: kubeFactory,
+		mu:          sync.Mutex{},
 	}
 }
 
-func (service *PendingActionsService) Create(r portainer.PendingActions) error {
+func (service *PendingActionsService) RegisterHandler(name string, handler portainer.PendingActionHandler) {
+	handlers[name] = handler
+}
+
+func (service *PendingActionsService) Create(r portainer.PendingAction) error {
 	return service.dataStore.PendingActions().Create(&r)
 }
 
 func (service *PendingActionsService) Execute(id portainer.EndpointID) error {
-
 	service.mu.Lock()
 	defer service.mu.Unlock()
 
@@ -108,73 +90,18 @@ func (service *PendingActionsService) Execute(id portainer.EndpointID) error {
 	return nil
 }
 
-func (service *PendingActionsService) executePendingAction(pendingAction portainer.PendingActions, endpoint *portainer.Endpoint) error {
+func (service *PendingActionsService) executePendingAction(pendingAction portainer.PendingAction, endpoint *portainer.Endpoint) error {
 	log.Debug().Msgf("Executing pending action %s for environment %d", pendingAction.Action, pendingAction.EndpointID)
 
 	defer func() {
 		log.Debug().Msgf("End executing pending action %s for environment %d", pendingAction.Action, pendingAction.EndpointID)
 	}()
 
-	switch pendingAction.Action {
-	case actions.CleanNAPWithOverridePolicies:
-		pendingActionData, err := actions.ConvertCleanNAPWithOverridePoliciesPayload(pendingAction.ActionData)
-		if err != nil {
-			return fmt.Errorf("failed to parse pendingActionData for CleanNAPWithOverridePoliciesPayload")
-		}
-
-		if pendingActionData == nil || pendingActionData.EndpointGroupID == 0 {
-			service.authorizationService.CleanNAPWithOverridePolicies(service.dataStore, endpoint, nil)
-			return nil
-		}
-
-		endpointGroupID := pendingActionData.EndpointGroupID
-
-		endpointGroup, err := service.dataStore.EndpointGroup().Read(portainer.EndpointGroupID(endpointGroupID))
-		if err != nil {
-			log.Error().Err(err).Msgf("Error reading environment group to clean NAP with override policies for environment %d and environment group %d", endpoint.ID, endpointGroup.ID)
-			return fmt.Errorf("failed to retrieve environment group %d: %w", endpointGroupID, err)
-		}
-		err = service.authorizationService.CleanNAPWithOverridePolicies(service.dataStore, endpoint, endpointGroup)
-		if err != nil {
-			log.Error().Err(err).Msgf("Error cleaning NAP with override policies for environment %d and environment group %d", endpoint.ID, endpointGroup.ID)
-			return fmt.Errorf("failed to clean NAP with override policies for environment %d and environment group %d: %w", endpoint.ID, endpointGroup.ID, err)
-		}
-
-		return nil
-	case actions.DeletePortainerK8sRegistrySecrets:
-		if pendingAction.ActionData == nil {
-			return nil
-		}
-
-		registryData, err := convertToDeletePortainerK8sRegistrySecretsData(pendingAction.ActionData)
-		if err != nil {
-			return fmt.Errorf("failed to parse pendingActionData: %w", err)
-		}
-
-		err = service.DeleteKubernetesRegistrySecrets(endpoint, registryData)
-		if err != nil {
-			log.Warn().Err(err).Int("endpoint_id", int(endpoint.ID)).Msgf("Unable to delete kubernetes registry secrets")
-			return fmt.Errorf("failed to delete kubernetes registry secrets for environment %d: %w", endpoint.ID, err)
-		}
-
-		return nil
-
-	case actions.PostInitMigrateEnvironment:
-		postInitMigrator := postinit.NewPostInitMigrator(
-			service.kubeFactory,
-			service.dockerFactory,
-			service.dataStore,
-			service.assetsPath,
-			service.kubernetesDeployer,
-		)
-		err := postInitMigrator.MigrateEnvironment(endpoint)
-		if err != nil {
-			log.Error().Err(err).Msgf("Error running post-init migrations for edge environment %d", endpoint.ID)
-			return fmt.Errorf("failed running post-init migrations for edge environment %d: %w", endpoint.ID, err)
-		}
-
+	handler, ok := handlers[pendingAction.Action]
+	if !ok {
+		log.Warn().Msgf("No handler found for pending action %s", pendingAction.Action)
 		return nil
 	}
 
-	return nil
+	return handler.Execute(pendingAction, endpoint)
 }