Merge pull request #79 from fdfzcq/fix-deletion-bug

Do not block deletion if NEG status is empty

controllers/controller_test.go

@@ -0,0 +1,102 @@
+package controllers
+
+import (
+	"context"
+	"reflect"
+	"time"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+var _ = Describe("Run autoneg Controller", func() {
+
+	ctx := context.Background()
+
+	serviceKey := client.ObjectKey{
+		Name:      "old-service",
+		Namespace: "namespace",
+	}
+
+	Context("Create a service resource with autoneg annotations", func() {
+
+		It("should succeed", func() {
+
+			namespace := &corev1.Namespace{
+				ObjectMeta: v1.ObjectMeta{
+					Name: "namespace",
+				},
+			}
+
+			err := k8sClient.Create(ctx, namespace)
+			Expect(err).NotTo(HaveOccurred())
+
+			annotations := make(map[string]string)
+			annotations[negAnnotation] = "{\"exposed_ports\":{\"4242\":{}}}"
+			annotations[autonegAnnotation] = "{\"backend_services\":{\"4242\":[{\"max_rate_per_endpoint\":4242}]}}"
+
+			ports := make([]corev1.ServicePort, 1)
+			ports[0] = corev1.ServicePort{
+				Port:     4242,
+				Protocol: corev1.ProtocolTCP,
+			}
+
+			service := &corev1.Service{
+				ObjectMeta: v1.ObjectMeta{
+					Name:        "old-service",
+					Namespace:   "namespace",
+					Annotations: annotations,
+				},
+				Spec: corev1.ServiceSpec{
+					Ports: ports,
+				},
+			}
+
+			err = k8sClient.Create(ctx, service)
+			Expect(err).NotTo(HaveOccurred(), "failed to create service resource")
+
+			createdService := &corev1.Service{}
+
+			Eventually(func() string {
+				err = k8sClient.Get(ctx, serviceKey, createdService)
+				Expect(err).NotTo(HaveOccurred(), "failed to retrieve service resource")
+				annos := createdService.Annotations
+				autonegStatus := annos[autonegStatusAnnotation]
+				return autonegStatus
+			}, time.Second*5, time.Second).ShouldNot(BeEmpty())
+
+			updatedAnnos := createdService.Annotations
+
+			Expect(updatedAnnos[autonegStatusAnnotation]).To(Equal(
+				"{\"backend_services\":{\"4242\":{\"namespace-old-service-4242-de64ba2d\":" +
+					"{\"name\":\"namespace-old-service-4242-de64ba2d\",\"max_rate_per_endpoint\":4242}}}," +
+					"\"network_endpoint_groups\":null,\"zones\":null}"))
+			Expect(updatedAnnos[negStatusAnnotation]).To(BeEmpty())
+		})
+
+		Context("Remove the service", func() {
+
+			It("should succeed", func() {
+				createdService := &corev1.Service{}
+				err := k8sClient.Get(ctx, serviceKey, createdService)
+				Expect(err).NotTo(HaveOccurred(), "failed to retrieve service resource")
+
+				err = k8sClient.Delete(ctx, createdService)
+				Expect(err).NotTo(HaveOccurred(), "failed to delete service resource")
+
+				Eventually(func() error {
+					err = k8sClient.Get(ctx, serviceKey, &corev1.Service{})
+					return err
+				}, time.Second*5, time.Second).ShouldNot(BeNil())
+
+				var e *errNotFound
+				Expect(err).To(HaveOccurred())
+				Expect(reflect.TypeOf(err).Kind()).To(Equal(reflect.TypeOf(e).Kind()))
+			})
+
+		})
+	})
+})

controllers/service_controller.go

@@ -100,9 +100,7 @@ func (r *ServiceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 
 	if deleting {
 		intendedStatus.NEGStatus = NEGStatus{}
-	}
-
-	if reflect.DeepEqual(status.status, intendedStatus) {
+	} else if reflect.DeepEqual(status.status, intendedStatus) {
 		// Equal, no reconciliation necessary
 		return reconcile.Result{}, nil
 	}

controllers/suite_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package controllers
 
 import (
+	"context"
 	"path/filepath"
 	"testing"
 
@@ -25,6 +26,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
+	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	"sigs.k8s.io/controller-runtime/pkg/envtest/printer"
@@ -38,7 +40,9 @@ import (
 
 var cfg *rest.Config
 var k8sClient client.Client
+var k8sManager ctrl.Manager
 var testEnv *envtest.Environment
+var cancel context.CancelFunc
 
 func TestAPIs(t *testing.T) {
 	RegisterFailHandler(Fail)
@@ -51,6 +55,9 @@ func TestAPIs(t *testing.T) {
 var _ = BeforeSuite(func() {
 	logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
 
+	var ctx context.Context
+	ctx, cancel = context.WithCancel(context.TODO())
+
 	By("bootstrapping test environment")
 	testEnv = &envtest.Environment{
 		CRDDirectoryPaths:     []string{filepath.Join("..", "config", "crd", "bases")},
@@ -70,9 +77,29 @@ var _ = BeforeSuite(func() {
 	Expect(err).NotTo(HaveOccurred())
 	Expect(k8sClient).NotTo(BeNil())
 
+	k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
+		Scheme: scheme.Scheme,
+	})
+	Expect(err).ToNot(HaveOccurred())
+
+	err = (&ServiceReconciler{
+		Client:              k8sManager.GetClient(),
+		BackendController:   &BackendController{},
+		Recorder:            k8sManager.GetEventRecorderFor("autoneg-controller"),
+		ServiceNameTemplate: serviceNameTemplate,
+		AllowServiceName:    true,
+	}).SetupWithManager(k8sManager)
+	Expect(err).ToNot(HaveOccurred())
+
+	go func() {
+		defer GinkgoRecover()
+		err = k8sManager.Start(ctx)
+		Expect(err).ToNot(HaveOccurred())
+	}()
 }, 60)
 
 var _ = AfterSuite(func() {
+	cancel()
 	By("tearing down the test environment")
 	err := testEnv.Stop()
 	Expect(err).NotTo(HaveOccurred())