diff --git a/watcher/registryhandler.go b/watcher/registryhandler.go index 3160979..52d3091 100644 --- a/watcher/registryhandler.go +++ b/watcher/registryhandler.go @@ -25,6 +25,7 @@ import ( "golang.org/x/sync/errgroup" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -283,30 +284,39 @@ func (ch *RegistryCommandsHandler) getECRRepositoriesWithSDK(registry armotypes. return allRepositories, nil } +// deleteRegistry removes what upsertRegistry created for the registry: its secret, its +// request ConfigMap, the schedule when it had one and the job of an immediate scan. A +// registry without a scan frequency never had a CronJob, so an object that is not there is +// not an error; everything is attempted and the first other error is returned. The jobs +// are deleted with background propagation, since the API's default for a Job leaves its +// pods behind. func (ch *RegistryCommandsHandler) deleteRegistry(cmd v1alpha1.OperatorCommand) error { registry, err := armotypes.UnmarshalRegistry(cmd.Spec.Body) if err != nil { logger.L().Error("deleteRegistry - failed to unmarshal command payload", helpers.Error(err)) return err } - resourceName := registry.GetBase().ResourceName - err = ch.k8sAPI.KubernetesClient.BatchV1().CronJobs(ch.config.Namespace()).Delete(context.Background(), resourceName, metav1.DeleteOptions{}) - if err != nil { - logger.L().Error("deleteRegistry - failed to delete cronjob resource", helpers.Error(err)) - return err - } - err = ch.k8sAPI.KubernetesClient.CoreV1().Secrets(ch.config.Namespace()).Delete(context.Background(), resourceName, metav1.DeleteOptions{}) - if err != nil { - logger.L().Error("deleteRegistry - failed to delete secret resource", helpers.Error(err)) - return err + name, namespace := registry.GetBase().ResourceName, ch.config.Namespace() + var firstErr error + attempt := func(what string, err error) { + if err == nil || apierrors.IsNotFound(err) { + return + } + logger.L().Error("deleteRegistry - failed to delete "+what, helpers.String("name", name), helpers.Error(err)) + if firstErr == nil { + firstErr = fmt.Errorf("failed to delete %s %s: %w", what, name, err) + } } - err = ch.k8sAPI.KubernetesClient.CoreV1().ConfigMaps(ch.config.Namespace()).Delete(context.Background(), resourceName, metav1.DeleteOptions{}) - if err != nil { - logger.L().Error("deleteRegistry - failed to delete configmap resource", helpers.Error(err)) - return err + background := metav1.DeleteOptions{PropagationPolicy: ptr.To(metav1.DeletePropagationBackground)} + attempt("cronjob", ch.k8sAPI.KubernetesClient.BatchV1().CronJobs(namespace).Delete(context.Background(), name, background)) + attempt("job", ch.k8sAPI.KubernetesClient.BatchV1().Jobs(namespace).Delete(context.Background(), name, background)) + attempt("secret", ch.k8sAPI.KubernetesClient.CoreV1().Secrets(namespace).Delete(context.Background(), name, metav1.DeleteOptions{})) + attempt("configmap", ch.k8sAPI.KubernetesClient.CoreV1().ConfigMaps(namespace).Delete(context.Background(), name, metav1.DeleteOptions{})) + if firstErr != nil { + return firstErr } - logger.L().Info("deleteRegistry: registry deleted successfully") + logger.L().Info("deleteRegistry: registry deleted successfully", helpers.String("name", name)) return nil } diff --git a/watcher/registryhandler_test.go b/watcher/registryhandler_test.go new file mode 100644 index 0000000..eca6b05 --- /dev/null +++ b/watcher/registryhandler_test.go @@ -0,0 +1,121 @@ +package watcher + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/armosec/armoapi-go/armotypes" + utilsmetadata "github.com/armosec/utils-k8s-go/armometadata" + "github.com/kubescape/backend/pkg/command" + "github.com/kubescape/backend/pkg/command/types/v1alpha1" + beUtils "github.com/kubescape/backend/pkg/utils" + "github.com/kubescape/k8s-interface/k8sinterface" + "github.com/kubescape/operator/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" +) + +func deleteRegistryCommand(t *testing.T, name string) v1alpha1.OperatorCommand { + t.Helper() + registry := armotypes.HarborImageRegistry{InstanceURL: "https://harbor.example", Username: "scanner", Password: "secret"} + registry.Provider = armotypes.Harbor + registry.ResourceName = name + body, err := json.Marshal(registry) + require.NoError(t, err) + return v1alpha1.OperatorCommand{Spec: v1alpha1.OperatorCommandSpec{CommandType: string(command.OperatorCommandTypeDeleteRegistry), Body: body}} +} + +// registryObjects is what upsertRegistry creates for a registry: the secret, the request +// ConfigMap, the job of the immediate scan and, when the registry has a scan frequency, +// the CronJob. +func registryObjects(name string, withSchedule bool) []runtime.Object { + meta := metav1.ObjectMeta{Namespace: "kubescape", Name: name} + objects := []runtime.Object{&corev1.Secret{ObjectMeta: meta}, &corev1.ConfigMap{ObjectMeta: meta}, &batchv1.Job{ObjectMeta: meta}} + if withSchedule { + objects = append(objects, &batchv1.CronJob{ObjectMeta: meta}) + } + return objects +} + +func newRegistryHandler(t *testing.T, objects ...runtime.Object) (*RegistryCommandsHandler, *fake.Clientset) { + t.Helper() + cfg, err := config.NewOperatorConfig(config.CapabilitiesConfig{}, utilsmetadata.ClusterConfig{}, &beUtils.Credentials{}, config.Config{Namespace: "kubescape"}) + require.NoError(t, err) + client := fake.NewSimpleClientset(objects...) + return &RegistryCommandsHandler{ctx: context.Background(), k8sAPI: &k8sinterface.KubernetesApi{KubernetesClient: client}, config: cfg}, client +} + +// remaining reports which of a registry's objects still exist. +func remaining(t *testing.T, client *fake.Clientset, name string) map[string]bool { + t.Helper() + ctx := context.Background() + exists := func(err error) bool { + if err == nil { + return true + } + require.True(t, apierrors.IsNotFound(err), err) + return false + } + _, secretErr := client.CoreV1().Secrets("kubescape").Get(ctx, name, metav1.GetOptions{}) + _, configMapErr := client.CoreV1().ConfigMaps("kubescape").Get(ctx, name, metav1.GetOptions{}) + _, jobErr := client.BatchV1().Jobs("kubescape").Get(ctx, name, metav1.GetOptions{}) + _, cronJobErr := client.BatchV1().CronJobs("kubescape").Get(ctx, name, metav1.GetOptions{}) + return map[string]bool{"secret": exists(secretErr), "configmap": exists(configMapErr), "job": exists(jobErr), "cronjob": exists(cronJobErr)} +} + +func TestDeleteRegistryRemovesEverythingTheRegistryHad(t *testing.T) { + for _, withSchedule := range []bool{true, false} { + name := "without a schedule" + if withSchedule { + name = "with a schedule" + } + t.Run(name, func(t *testing.T) { + objects := append(registryObjects("registry-a", withSchedule), registryObjects("registry-b", true)...) + h, client := newRegistryHandler(t, objects...) + + require.NoError(t, h.deleteRegistry(deleteRegistryCommand(t, "registry-a"))) + + assert.Equal(t, map[string]bool{"secret": false, "configmap": false, "job": false, "cronjob": false}, remaining(t, client, "registry-a")) + assert.Equal(t, map[string]bool{"secret": true, "configmap": true, "job": true, "cronjob": true}, remaining(t, client, "registry-b"), "another registry's objects stay") + }) + } +} + +func TestDeleteRegistryDeletesJobsWithTheirPods(t *testing.T) { + h, client := newRegistryHandler(t, registryObjects("registry-a", true)...) + + require.NoError(t, h.deleteRegistry(deleteRegistryCommand(t, "registry-a"))) + + policies := map[string]*metav1.DeletionPropagation{} + for _, action := range client.Actions() { + if del, ok := action.(k8stesting.DeleteActionImpl); ok { + policies[del.GetResource().Resource] = del.DeleteOptions.PropagationPolicy + } + } + for _, resource := range []string{"jobs", "cronjobs"} { + require.NotNil(t, policies[resource], resource) + assert.Equal(t, metav1.DeletePropagationBackground, *policies[resource], resource) + } +} + +func TestDeleteRegistryReportsTheFirstFailureAfterTryingEverything(t *testing.T) { + h, client := newRegistryHandler(t, registryObjects("registry-a", true)...) + boom := errors.New("secrets is forbidden") + client.PrependReactor("delete", "secrets", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, boom + }) + + err := h.deleteRegistry(deleteRegistryCommand(t, "registry-a")) + + require.ErrorIs(t, err, boom) + assert.Equal(t, map[string]bool{"secret": true, "configmap": false, "job": false, "cronjob": false}, remaining(t, client, "registry-a"), "the rest is still removed") +}