diff --git a/go.mod b/go.mod index 6a4b14f55..c036a0e1e 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ go 1.20 require ( github.com/go-logr/logr v1.2.4 github.com/spf13/pflag v1.0.5 + github.com/stretchr/testify v1.8.1 golang.org/x/sync v0.3.0 k8s.io/api v0.27.4 k8s.io/apimachinery v0.27.4 @@ -25,6 +26,7 @@ require ( github.com/cespare/xxhash/v2 v2.2.0 // indirect github.com/davecgh/go-spew v1.1.1 // indirect github.com/emicklei/go-restful/v3 v3.9.0 // indirect + github.com/evanphx/json-patch v5.6.0+incompatible // indirect github.com/evanphx/json-patch/v5 v5.6.0 // indirect github.com/fsnotify/fsnotify v1.6.0 // indirect github.com/go-openapi/jsonpointer v0.19.6 // indirect @@ -49,6 +51,7 @@ require ( github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/onsi/gomega v1.27.8 // indirect github.com/pkg/errors v0.9.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/client_golang v1.16.0 // indirect github.com/prometheus/client_model v0.4.0 // indirect github.com/prometheus/common v0.42.0 // indirect diff --git a/go.sum b/go.sum index a084406cc..a8f27be4b 100644 --- a/go.sum +++ b/go.sum @@ -42,6 +42,7 @@ github.com/envoyproxy/go-control-plane v0.9.9-0.20201210154907-fd9021fe5dad/go.m github.com/envoyproxy/go-control-plane v0.9.9-0.20210512163311-63b5d3c536b0/go.mod h1:hliV/p42l8fGbc6Y9bQ70uLwIvmJyVE5k4iMKlh8wCQ= github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c= github.com/evanphx/json-patch v5.6.0+incompatible h1:jBYDEEiFBPxA0v50tFdvOzQQTCvpL6mnFh5mB2/l16U= +github.com/evanphx/json-patch v5.6.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= github.com/evanphx/json-patch/v5 v5.6.0 h1:b91NhWfaz02IuVxO9faSllyAtNXHMPkC5J8sJCLunww= github.com/evanphx/json-patch/v5 v5.6.0/go.mod h1:G79N1coSVB93tBe7j6PhzjmR3/2VvlbKOFpnXhI9Bw4= github.com/flowstack/go-jsonschema v0.1.1/go.mod h1:yL7fNggx1o8rm9RlgXv7hTBWxdBM0rVwpMwimd3F3N0= diff --git a/make/go.mk b/make/go.mk index 8afa13815..46cb701cd 100644 --- a/make/go.mk +++ b/make/go.mk @@ -170,7 +170,7 @@ endif .PHONY: go-fix.% go-fix.%: ## Runs golangci-lint for a specific module -go-fix.%: ; $(info $(M) linting $* module) +go-fix.%: ; $(info $(M) go fixing $* module) $(if $(filter-out root,$*),cd $* && )go fix ./... .PHONY: go-generate diff --git a/pkg/handlers/servicelbgc/deleter.go b/pkg/handlers/servicelbgc/deleter.go index 94ba8f8ca..721c9cf4d 100644 --- a/pkg/handlers/servicelbgc/deleter.go +++ b/pkg/handlers/servicelbgc/deleter.go @@ -105,7 +105,7 @@ func failedToDeleteServicesError(svcsFailedToBeDeleted []client.ObjectKey) error return fmt.Errorf("%w: the following Services could not be deleted "+ "and must cleaned up manually before deleting the cluster: %s", ErrFailedToDeleteService, - strings.Join(toStringSlice(svcsFailedToBeDeleted), ","), + strings.Join(toStringSlice(svcsFailedToBeDeleted), ", "), ) } diff --git a/pkg/handlers/servicelbgc/deleter_test.go b/pkg/handlers/servicelbgc/deleter_test.go new file mode 100644 index 000000000..ea0dd0416 --- /dev/null +++ b/pkg/handlers/servicelbgc/deleter_test.go @@ -0,0 +1,301 @@ +// Copyright 2023 D2iQ, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package servicelbgc + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +//nolint:funlen // Long tests are OK +func Test_shouldDeleteServicesWithLoadBalancer(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + cluster *v1beta1.Cluster + shouldDelete bool + }{{ + name: "should delete", + cluster: &v1beta1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-should-delete", + }, + Status: v1beta1.ClusterStatus{ + Conditions: v1beta1.Conditions{{ + Type: v1beta1.ControlPlaneInitializedCondition, + Status: corev1.ConditionTrue, + }}, + Phase: string(v1beta1.ClusterPhaseProvisioned), + }, + }, + shouldDelete: true, + }, { + name: "should delete: annotation is set to true", + cluster: &v1beta1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-should-delete", + Annotations: map[string]string{ + LoadBalancerGCAnnotation: "true", + }, + }, + Status: v1beta1.ClusterStatus{ + Conditions: v1beta1.Conditions{{ + Type: v1beta1.ControlPlaneInitializedCondition, + Status: corev1.ConditionTrue, + }}, + Phase: string(v1beta1.ClusterPhaseProvisioned), + }, + }, + shouldDelete: true, + }, { + name: "should not delete: annotation is set to false", + cluster: &v1beta1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-should-delete", + Annotations: map[string]string{ + LoadBalancerGCAnnotation: "false", + }, + }, + Status: v1beta1.ClusterStatus{ + Phase: string(v1beta1.ClusterPhaseProvisioned), + }, + }, + shouldDelete: false, + }, { + name: "should not delete: phase is Pending", + cluster: &v1beta1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-should-delete", + }, + Status: v1beta1.ClusterStatus{ + Phase: string(v1beta1.ClusterPhasePending), + }, + }, + shouldDelete: false, + }, { + name: "should not delete: ControlPlaneInitialized condition is False", + cluster: &v1beta1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-should-delete", + }, + Status: v1beta1.ClusterStatus{ + Conditions: v1beta1.Conditions{{ + Type: v1beta1.ControlPlaneInitializedCondition, + Status: corev1.ConditionFalse, + }}, + }, + }, + shouldDelete: false, + }} + for idx := range tests { + tt := tests[idx] + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + shouldDelete, err := shouldDeleteServicesWithLoadBalancer(tt.cluster) + assert.NoError(t, err) + assert.Equal(t, tt.shouldDelete, shouldDelete) + }) + } +} + +//nolint:funlen // Long tests are OK +func Test_deleteServicesWithLoadBalancer(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + startServices []corev1.Service + endServices []corev1.Service + }{{ + name: "no services", + startServices: []corev1.Service(nil), + endServices: []corev1.Service(nil), + }, { + name: "should not delete, all services with ClusterIP", + startServices: []corev1.Service{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-1", + Namespace: "ns-1", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-2", + Namespace: "ns-2", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + }, + }}, + endServices: []corev1.Service{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-1", + Namespace: "ns-1", + ResourceVersion: "1", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-2", + Namespace: "ns-2", + ResourceVersion: "1", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + }, + }}, + }, { + name: "should delete 1 service with LoadBalancer", + startServices: []corev1.Service{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-1", + Namespace: "ns-1", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-2", + Namespace: "ns-2", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{{Hostname: "lb-123.com"}}, + }, + }, + }}, + endServices: []corev1.Service{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-1", + Namespace: "ns-1", + ResourceVersion: "1", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + }, + }}, + }} + + for idx := range tests { + tt := tests[idx] // Capture range variable + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + fakeClient := fake.NewClientBuilder().Build() + for i := range tt.startServices { + svc := &tt.startServices[i] + require.NoError( + t, + fakeClient.Create(context.Background(), svc), + "error creating Service", + ) + } + + for { + err := deleteServicesWithLoadBalancer(context.TODO(), fakeClient, logr.Discard()) + if err == nil { + break + } + require.ErrorIs( + t, + err, + ErrServicesStillExist, + "only allowed a retry-able error here", + ) + } + + services := &corev1.ServiceList{} + require.NoError( + t, + fakeClient.List(context.Background(), services), + "error listing Services", + ) + assert.Equal(t, tt.endServices, services.Items) + }) + } +} + +func Test_needsDelete(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + service *corev1.Service + shouldDelete bool + }{{ + name: "shouldDelete", + service: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{{Hostname: "lb-123.com"}}, + }, + }, + }, + shouldDelete: true, + }, { + name: "false: ServiceTypeNodePort", + service: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{{Hostname: "lb-123.com"}}, + }, + }, + }, + shouldDelete: false, + }, { + name: "false: LoadBalancer is empty", + service: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + Status: corev1.ServiceStatus{}, + }, + shouldDelete: false, + }} + for idx := range tests { + tt := tests[idx] + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + del := needsDelete(tt.service) + assert.Equal(t, tt.shouldDelete, del) + }) + } +} + +// this test is mainly here to visually show what the error will look like. +func Test_failedToDeleteServicesError(t *testing.T) { + svcs := []client.ObjectKey{ + {Namespace: "ns-1", Name: "svc-1"}, + {Namespace: "ns-2", Name: "svc-2"}, + {Namespace: "ns-3", Name: "svc-3"}, + {Namespace: "ns-4", Name: "svc-4"}, + {Namespace: "ns-5", Name: "svc-5"}, + } + //nolint:lll // want to show the full error in one line + expectedErrString := "failed to delete kubernetes services: the following Services could not be deleted and must cleaned up manually before deleting the cluster: ns-1/svc-1, ns-2/svc-2, ns-3/svc-3, ns-4/svc-4, ns-5/svc-5" + assert.EqualError(t, failedToDeleteServicesError(svcs), expectedErrString) +} diff --git a/pkg/handlers/servicelbgc/handler.go b/pkg/handlers/servicelbgc/handler.go index 328a5f5f4..96b2fd480 100644 --- a/pkg/handlers/servicelbgc/handler.go +++ b/pkg/handlers/servicelbgc/handler.go @@ -78,13 +78,14 @@ func (s *ServiceLoadBalancerGC) BeforeClusterDelete( err = deleteServicesWithLoadBalancer(ctx, remoteClient, log) switch { case errors.Is(err, ErrFailedToDeleteService): - resp.Status = runtimehooksv1.ResponseStatusFailure - resp.Message = err.Error() + resp.SetStatus(runtimehooksv1.ResponseStatusFailure) + resp.SetMessage(err.Error()) + resp.SetRetryAfterSeconds(5) case errors.Is(err, ErrServicesStillExist): - resp.Status = runtimehooksv1.ResponseStatusSuccess - resp.Message = err.Error() - resp.RetryAfterSeconds = 5 + resp.SetStatus(runtimehooksv1.ResponseStatusSuccess) + resp.SetMessage(err.Error()) + resp.SetRetryAfterSeconds(5) default: - resp.Status = runtimehooksv1.ResponseStatusSuccess + resp.SetStatus(runtimehooksv1.ResponseStatusSuccess) } }