Refactor tests for blocking HTTP connections

- Use noopstore to disable throttling behaviour.
- Fake k8s client to remove need of interacting with an envtest
apiserver.
- Replace HTTP Status Code magic numbers, with their respective
constants.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>

controllers/event_handling_test.go

@@ -7,13 +7,12 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
-	"net/url"
 	"testing"
 	"time"
 
 	"github.com/fluxcd/pkg/ssa"
 	. "github.com/onsi/gomega"
-	"github.com/sethvargo/go-limiter/memorystore"
+	"github.com/sethvargo/go-limiter/noopstore"
 	prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
 	"github.com/slok/go-http-metrics/middleware"
 	corev1 "k8s.io/api/core/v1"
@@ -46,82 +45,17 @@ func TestEventHandler(t *testing.T) {
 		}),
 	})
 
-	store, err := memorystore.New(&memorystore.Config{
-		Interval: 5 * time.Minute,
-	})
-	if err != nil {
-		t.Fatalf("failed to create memory storage")
-	}
-
-	httpScheme := "http"
-
-	eventServerTests := []struct {
-		name          string
-		isHttpEnabled bool
-		url           string
-	}{
-		{
-			name:          "http scheme is enabled",
-			isHttpEnabled: true,
-		}, {
-			name:          "http scheme is disabled",
-			isHttpEnabled: false,
-		},
-	}
-	for _, eventServerTest := range eventServerTests {
-		t.Run(eventServerTest.name, func(t *testing.T) {
-
-			eventServer := server.NewEventServer("127.0.0.1:56789", logf.Log, k8sClient, true, eventServerTest.isHttpEnabled)
-
-			stopCh := make(chan struct{})
-			go eventServer.ListenAndServe(stopCh, eventMdlw, store)
-			requestsReceived := 0
-			rcvServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-				requestsReceived = requestsReceived + 1
-				req = r
-				w.WriteHeader(200)
-			}))
-			defer rcvServer.Close()
-			defer close(stopCh)
-
-			providerKey := types.NamespacedName{
-				Name:      fmt.Sprintf("provider-%s", randStringRunes(5)),
-				Namespace: namespace,
-			}
-			provider = &notifyv1.Provider{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      providerKey.Name,
-					Namespace: providerKey.Namespace,
-				},
-				Spec: notifyv1.ProviderSpec{
-					Type:    "generic",
-					Address: rcvServer.URL,
-				},
-			}
-
-			webhook_url, err := url.Parse(provider.Spec.Address)
-
-			g.Expect(err).ToNot(HaveOccurred())
-
-			if eventServerTest.isHttpEnabled {
-				g.Expect(webhook_url.Scheme).To(Equal(httpScheme))
-				g.Expect(requestsReceived).To(Equal(1))
-			} else {
-				g.Expect(webhook_url.Scheme).ToNot(Equal(httpScheme))
-				g.Expect(requestsReceived).To(Equal(0))
-			}
-
-		})
-	}
-
-	eventServer := server.NewEventServer("127.0.0.1:56789", logf.Log, k8sClient, true, true)
+	store, err := noopstore.New()
+	g.Expect(err).ToNot(HaveOccurred())
 
+	serverEndpoint := "127.0.0.1:56789"
+	eventServer := server.NewEventServer(serverEndpoint, logf.Log, k8sClient, true, true)
 	stopCh := make(chan struct{})
 	go eventServer.ListenAndServe(stopCh, eventMdlw, store)
 
 	rcvServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		req = r
-		w.WriteHeader(200)
+		w.WriteHeader(http.StatusOK)
 	}))
 	defer rcvServer.Close()
 	defer close(stopCh)
@@ -236,9 +170,9 @@ func TestEventHandler(t *testing.T) {
 	testSent := func() {
 		buf := &bytes.Buffer{}
 		g.Expect(json.NewEncoder(buf).Encode(&event)).To(Succeed())
-		res, err := http.Post("http://localhost:56789/", "application/json", buf)
+		res, err := http.Post("http://"+serverEndpoint, "application/json", buf)
 		g.Expect(err).ToNot(HaveOccurred())
-		g.Expect(res.StatusCode).To(Equal(202)) // event_server responds with 202 Accepted
+		g.Expect(res.StatusCode).To(Equal(http.StatusAccepted))
 
 	}
 
@@ -361,5 +295,4 @@ func TestEventHandler(t *testing.T) {
 			req = nil
 		})
 	}
-
 }

go.mod

@@ -31,6 +31,7 @@ require (
 	k8s.io/api v0.25.3
 	k8s.io/apimachinery v0.25.3
 	k8s.io/client-go v0.25.3
+	k8s.io/kubectl v0.24.0
 	sigs.k8s.io/cli-utils v0.33.0
 	sigs.k8s.io/controller-runtime v0.13.0
 	sigs.k8s.io/yaml v1.3.0
@@ -151,7 +152,6 @@ require (
 	k8s.io/component-base v0.25.2 // indirect
 	k8s.io/klog/v2 v2.80.1 // indirect
 	k8s.io/kube-openapi v0.0.0-20220803162953-67bda5d908f1 // indirect
-	k8s.io/kubectl v0.24.0 // indirect
 	k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed // indirect
 	sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect
 	sigs.k8s.io/kustomize/api v0.12.1 // indirect

internal/server/event_handlers.go

@@ -45,7 +45,6 @@ import (
 
 func (s *EventServer) handleEvent() func(w http.ResponseWriter, r *http.Request) {
 	return func(w http.ResponseWriter, r *http.Request) {
-		r.Context()
 		body, err := io.ReadAll(r.Body)
 		if err != nil {
 			s.logger.Error(err, "reading the request body failed")

internal/server/event_server.go

@@ -106,7 +106,7 @@ func (s *EventServer) logRateLimitMiddleware(h http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		recorder := &statusRecorder{
 			ResponseWriter: w,
-			Status:         200,
+			Status:         http.StatusOK,
 		}
 		h.ServeHTTP(recorder, r)
 

internal/server/event_server_test.go

@@ -20,29 +20,39 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"testing"
 	"time"
 
-	"github.com/onsi/gomega"
+	. "github.com/onsi/gomega"
 	"github.com/sethvargo/go-limiter/httplimit"
 	"github.com/sethvargo/go-limiter/memorystore"
+	"github.com/sethvargo/go-limiter/noopstore"
+	"github.com/slok/go-http-metrics/middleware"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/kubectl/pkg/scheme"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
+	notifyv1 "github.com/fluxcd/notification-controller/api/v1beta1"
+	"github.com/fluxcd/pkg/apis/meta"
 	"github.com/fluxcd/pkg/runtime/events"
 )
 
 func TestEventKeyFunc(t *testing.T) {
-	g := gomega.NewGomegaWithT(t)
+	g := NewWithT(t)
 
 	// Setup middleware
 	store, err := memorystore.New(&memorystore.Config{
 		Interval: 10 * time.Minute,
 	})
-	g.Expect(err).ShouldNot(gomega.HaveOccurred())
+	g.Expect(err).ShouldNot(HaveOccurred())
 	middleware, err := httplimit.NewMiddleware(store, eventKeyFunc)
-	g.Expect(err).ShouldNot(gomega.HaveOccurred())
+	g.Expect(err).ShouldNot(HaveOccurred())
 	handler := middleware.Handle(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(http.StatusOK)
 	}))
@@ -129,19 +139,135 @@ func TestEventKeyFunc(t *testing.T) {
 				Message:        tt.message,
 			}
 			eventData, err := json.Marshal(event)
-			g.Expect(err).ShouldNot(gomega.HaveOccurred())
+			g.Expect(err).ShouldNot(HaveOccurred())
 
 			req := httptest.NewRequest("POST", "/", bytes.NewBuffer(eventData))
-			g.Expect(err).ShouldNot(gomega.HaveOccurred())
+			g.Expect(err).ShouldNot(HaveOccurred())
 			res := httptest.NewRecorder()
 			handler.ServeHTTP(res, req)
 
 			if tt.rateLimit {
-				g.Expect(res.Code).Should(gomega.Equal(429))
-				g.Expect(res.Header().Get("X-Ratelimit-Remaining")).Should(gomega.Equal("0"))
+				g.Expect(res.Code).Should(Equal(http.StatusTooManyRequests))
+				g.Expect(res.Header().Get("X-Ratelimit-Remaining")).Should(Equal("0"))
 			} else {
-				g.Expect(res.Code).Should(gomega.Equal(200))
+				g.Expect(res.Code).Should(Equal(http.StatusOK))
 			}
 		})
 	}
 }
+
+func TestBlockInsecureHTTP(t *testing.T) {
+	g := NewWithT(t)
+
+	var requestsReceived int
+	rcvServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		requestsReceived++
+		io.Copy(io.Discard, r.Body)
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer rcvServer.Close()
+
+	utilruntime.Must(notifyv1.AddToScheme(scheme.Scheme))
+
+	testNamespace := "test-ns"
+	providerKey := "provider"
+	client := fake.NewFakeClientWithScheme(scheme.Scheme,
+		&notifyv1.Provider{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      providerKey,
+				Namespace: testNamespace,
+			},
+			Spec: notifyv1.ProviderSpec{
+				Type:    "generic",
+				Address: rcvServer.URL,
+			},
+		},
+		&notifyv1.Alert{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "some-alert-name",
+				Namespace: testNamespace,
+			},
+			Spec: notifyv1.AlertSpec{
+				ProviderRef: meta.LocalObjectReference{
+					Name: providerKey,
+				},
+				EventSeverity: "info",
+				EventSources: []notifyv1.CrossNamespaceObjectReference{
+					{
+						Kind:      "Bucket",
+						Name:      "hyacinth",
+						Namespace: testNamespace,
+					},
+				},
+			},
+			Status: notifyv1.AlertStatus{
+				Conditions: []metav1.Condition{
+					{Type: meta.ReadyCondition, Status: metav1.ConditionTrue},
+				},
+			},
+		},
+	)
+
+	eventMdlw := middleware.New(middleware.Config{})
+
+	store, err := noopstore.New()
+	g.Expect(err).ToNot(HaveOccurred())
+
+	serverEndpoint := "127.0.0.1:56789"
+	eventServer := NewEventServer(serverEndpoint, logf.Log, client, true, true)
+	stopCh := make(chan struct{})
+	go eventServer.ListenAndServe(stopCh, eventMdlw, store)
+	defer close(stopCh)
+
+	event := events.Event{
+		InvolvedObject: corev1.ObjectReference{
+			Kind:      "Bucket",
+			Name:      "hyacinth",
+			Namespace: testNamespace,
+		},
+		Severity:            "info",
+		Timestamp:           metav1.Now(),
+		Message:             "well that happened",
+		Reason:              "event-happened",
+		ReportingController: "source-controller",
+	}
+
+	eventServerTests := []struct {
+		name          string
+		isHttpEnabled bool
+		url           string
+		wantRequest   int
+	}{
+		{
+			name:          "http scheme is disabled",
+			isHttpEnabled: false,
+			wantRequest:   0,
+		},
+		{
+			name:          "http scheme is enabled",
+			isHttpEnabled: true,
+			wantRequest:   1,
+		},
+	}
+	for _, tt := range eventServerTests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+			requestsReceived = 0 // reset counter
+
+			// Change the internal state instead of creating a new server.
+			eventServer.supportHttpScheme = tt.isHttpEnabled
+
+			buf := &bytes.Buffer{}
+			g.Expect(json.NewEncoder(buf).Encode(&event)).To(Succeed())
+			res, err := http.Post("http://"+serverEndpoint, "application/json", buf)
+
+			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(res.StatusCode).To(Equal(http.StatusAccepted))
+
+			// Requests happens async, so should the assertion.
+			g.Eventually(func() bool {
+				return requestsReceived == tt.wantRequest
+			}, 5*time.Second).Should(BeTrue())
+		})
+	}
+}