Rework Ginkgo usage (#9522)

* Rework Ginkgo usage

Currently Ginkgo is launched multiple times with different options to
accomodate various use-cases. In particular, some specs needs to be run
sequentially because non-namespaced objects are created that conflicts
with concurent Helm deployments.
However Ginkgo is able to handle such cases natively, in particular
specs that needs to be run sequentially are supported (Serial spec).

This commit marks the specs that needs to be run sequentially as Serial
specs and runs the whole test suite from a single Ginkgo invocation. As
a result, a single JUnit report is now generated.

Signed-off-by: Hervé Werner <dud225@hotmail.com>

* Fix controller error in test

Error getting ConfigMap "$NAMESPACE/tcp-services": no object matching key "$NAMESPACE/tcp-services" in local store

Signed-off-by: Hervé Werner <dud225@hotmail.com>

* Replace "go get" invocations by "go install"

Executing "go get" changes the go.mod & go.sum files which is not the
case of "go install".

Signed-off-by: Hervé Werner <dud225@hotmail.com>

* Always clean out the Helm deployment

Signed-off-by: Hervé Werner <dud225@hotmail.com>

* Add E2E test to verify that changes to one or more configmap trigger an update

Signed-off-by: Hervé Werner <dud225@hotmail.com>

---------

Signed-off-by: Hervé Werner <dud225@hotmail.com>

Makefile

@@ -31,7 +31,7 @@ TAG ?= $(shell cat TAG)
 
 # e2e settings
 # Allow limiting the scope of the e2e tests. By default run everything
-FOCUS ?= .*
+FOCUS ?=
 # number of parallel test
 E2E_NODES ?= 7
 # run e2e test suite with tests that check for memory leaks? (default is false)

internal/ingress/controller/store/store.go

@@ -701,7 +701,6 @@ func New(
 		},
 	}
 
-	// TODO: add e2e test to verify that changes to one or more configmap trigger an update
 	changeTriggerUpdate := func(name string) bool {
 		return name == configmap || name == tcp || name == udp
 	}

internal/k8s/main.go

@@ -78,7 +78,7 @@ func GetNodeIPOrName(kubeClient clientset.Interface, name string, useInternalIP
 var (
 	// IngressPodDetails hold information about the ingress-nginx pod
 	IngressPodDetails *PodInfo
-	// IngressNodeDetails old information about the node running ingress-nginx pod
+	// IngressNodeDetails hold information about the node running ingress-nginx pod
 	IngressNodeDetails *NodeInfo
 )
 

test/e2e-image/e2e.sh

@@ -14,70 +14,39 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-set -e
+set -eu
 
 NC='\e[0m'
 BGREEN='\e[32m'
 
-#SLOW_E2E_THRESHOLD=${SLOW_E2E_THRESHOLD:-"5s"}
-FOCUS=${FOCUS:-.*}
 E2E_NODES=${E2E_NODES:-5}
 E2E_CHECK_LEAKS=${E2E_CHECK_LEAKS:-""}
 
+reportFile="report-e2e-test-suite.xml"
 ginkgo_args=(
-  "-randomize-all"
-  "-flake-attempts=2"
-  "-fail-fast"
-  "--show-node-events"
+  "--fail-fast"
+  "--flake-attempts=2"
+  "--junit-report=${reportFile}"
+  "--nodes=${E2E_NODES}"
   "--poll-progress-after=180s"
-#  "-slow-spec-threshold=${SLOW_E2E_THRESHOLD}"
-  "-succinct"
-  "-timeout=75m"
+  "--randomize-all"
+  "--show-node-events"
+  "--succinct"
+  "--timeout=75m"
 )
 
-# Variable for the prefix of report filenames
-reportFileNamePrefix="report-e2e-test-suite"
-
-echo -e "${BGREEN}Running e2e test suite (FOCUS=${FOCUS})...${NC}"
-ginkgo "${ginkgo_args[@]}"               \
-  -focus="${FOCUS}"                  \
-  -skip="\[Serial\]|\[MemoryLeak\]|\[TopologyHints\]"  \
-  -nodes="${E2E_NODES}" \
-  --junit-report=$reportFileNamePrefix.xml \
-  /e2e.test
-# Create configMap out of a compressed report file for extraction later
+if [ -n "${FOCUS}" ]; then
+  ginkgo_args+=("--focus=${FOCUS}")
+fi
 
-# Must be isolated, there is a collision if multiple helms tries to install same clusterRole at same time
-echo -e "${BGREEN}Running e2e test for topology aware hints...${NC}"
-ginkgo "${ginkgo_args[@]}" \
-  -focus="\[TopologyHints\]" \
-  -skip="\[Serial\]|\[MemoryLeak\]]" \
-  -nodes="${E2E_NODES}" \
-  --junit-report=$reportFileNamePrefix-topology.xml \
-  /e2e.test
-# Create configMap out of a compressed report file for extraction later
+if [ -z "${E2E_CHECK_LEAKS}" ]; then
+  ginkgo_args+=("--skip=\[Memory Leak\]")
+fi
 
-echo -e "${BGREEN}Running e2e test suite with tests that require serial execution...${NC}"
-ginkgo "${ginkgo_args[@]}"   \
-  -focus="\[Serial\]"    \
-  -skip="\[MemoryLeak\]" \
-  --junit-report=$reportFileNamePrefix-serial.xml \
-  /e2e.test
-# Create configMap out of a compressed report file for extraction later
+echo -e "${BGREEN}Running e2e test suite...${NC}"
+(set -x; ginkgo "${ginkgo_args[@]}" /e2e.test)
 
-if [[ ${E2E_CHECK_LEAKS} != "" ]]; then
-  echo -e "${BGREEN}Running e2e test suite with tests that check for memory leaks...${NC}"
-  ginkgo "${ginkgo_args[@]}"    \
-    -focus="\[MemoryLeak\]" \
-    -skip="\[Serial\]" \
-    --junit-report=$reportFileNamePrefix-memleak.xml \
-    /e2e.test
 # Create configMap out of a compressed report file for extraction later
-fi
-
-for rFile in `ls $reportFileNamePrefix*` 
-do
-  gzip -k $rFile
-  kubectl create cm $rFile.gz --from-file $rFile.gz
-  kubectl label cm $rFile.gz junitreport=true
-done
+gzip -k ${reportFile}
+kubectl create cm ${reportFile}.gz --from-file ${reportFile}.gz
+kubectl label cm ${reportFile}.gz junitreport=true

test/e2e-image/namespace-overlays/topology/values.yaml

@@ -8,6 +8,7 @@ controller:
     digest:
     digestChroot:
   scope:
+    # Necessary to allow the ingress controller to get the topology information from the nodes
     enabled: false
   config:
     worker-processes: "1"
@@ -19,12 +20,7 @@ controller:
     periodSeconds: 1
   service:
     type: NodePort
-  electionID: ingress-controller-leader
-  ingressClassResource:
-    # We will create and remove each IC/ClusterRole/ClusterRoleBinding per test so there's no conflict
-    enabled: false
   extraArgs:
-    tcp-services-configmap: $NAMESPACE/tcp-services
     # e2e tests do not require information about ingress status
     update-status: "false"
   terminationGracePeriodSeconds: 1
@@ -33,19 +29,6 @@ controller:
 
   enableTopologyAwareRouting: true
 
-  # ulimit -c unlimited
-  # mkdir -p /tmp/coredump
-  # chmod a+rwx /tmp/coredump
-  # echo "/tmp/coredump/core.%e.%p.%h.%t" > /proc/sys/kernel/core_pattern
-  extraVolumeMounts:
-    - name: coredump
-      mountPath: /tmp/coredump
-
-  extraVolumes:
-    - name: coredump
-      hostPath:
-        path: /tmp/coredump
-
 rbac:
   create: true
   scope: false

test/e2e/admission/admission.go

@@ -32,19 +32,14 @@ import (
 	"k8s.io/ingress-nginx/test/e2e/framework"
 )
 
-var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() {
+var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller", func() {
 	f := framework.NewDefaultFramework("admission")
 
 	ginkgo.BeforeEach(func() {
 		f.NewEchoDeployment()
 		f.NewSlowEchoDeployment()
 	})
 
-	ginkgo.AfterEach(func() {
-		err := uninstallChart(f)
-		assert.Nil(ginkgo.GinkgoT(), err, "uninstalling helm chart")
-	})
-
 	ginkgo.It("reject ingress with global-rate-limit annotations when memcached is not configured", func() {
 		host := "admission-test"
 
@@ -216,16 +211,6 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() {
 	})
 })
 
-func uninstallChart(f *framework.Framework) error {
-	cmd := exec.Command("helm", "uninstall", "--namespace", f.Namespace, "nginx-ingress")
-	_, err := cmd.CombinedOutput()
-	if err != nil {
-		return fmt.Errorf("unexpected error uninstalling ingress-nginx release: %v", err)
-	}
-
-	return nil
-}
-
 const (
 	validV1Ingress = `
 apiVersion: networking.k8s.io/v1

test/e2e/annotations/fastcgi.go

@@ -21,7 +21,6 @@ import (
 	"strings"
 
 	"github.com/onsi/ginkgo/v2"
-	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -81,9 +80,7 @@ var _ = framework.DescribeAnnotation("backend-protocol - FastCGI", func() {
 			},
 		}
 
-		cm, err := f.EnsureConfigMap(configuration)
-		assert.Nil(ginkgo.GinkgoT(), err, "creating configmap")
-		assert.NotNil(ginkgo.GinkgoT(), cm, "expected a configmap but none returned")
+		f.EnsureConfigMap(configuration)
 
 		host := "fastcgi-params-configmap"
 

test/e2e/endpointslices/topology.go

@@ -21,7 +21,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
-	"os/exec"
 	"strings"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -33,20 +32,14 @@ import (
 	"k8s.io/ingress-nginx/test/e2e/framework"
 )
 
-var _ = framework.IngressNginxDescribe("[TopologyHints] topology aware routing", func() {
+var _ = framework.IngressNginxDescribeSerial("[TopologyHints] topology aware routing", func() {
 	f := framework.NewDefaultFramework("topology")
 	host := "topology-svc.foo.com"
 
 	ginkgo.BeforeEach(func() {
 		f.NewEchoDeployment(framework.WithDeploymentReplicas(2), framework.WithSvcTopologyAnnotations())
 	})
 
-	ginkgo.AfterEach(func() {
-		// we need to uninstall chart because of clusterRole which is not destroyed with namespace
-		err := uninstallChart(f)
-		assert.Nil(ginkgo.GinkgoT(), err, "uninstalling helm chart")
-	})
-
 	ginkgo.It("should return 200 when service has topology hints", func() {
 
 		annotations := make(map[string]string)
@@ -100,13 +93,3 @@ var _ = framework.IngressNginxDescribe("[TopologyHints] topology aware routing",
 		}
 	})
 })
-
-func uninstallChart(f *framework.Framework) error {
-	cmd := exec.Command("helm", "uninstall", "--namespace", f.Namespace, "nginx-ingress")
-	_, err := cmd.CombinedOutput()
-	if err != nil {
-		return fmt.Errorf("unexpected error uninstalling ingress-nginx release: %v", err)
-	}
-
-	return nil
-}

test/e2e/framework/exec.go

@@ -152,6 +152,16 @@ func (f *Framework) KubectlProxy(port int) (int, *exec.Cmd, error) {
 	return -1, cmd, fmt.Errorf("failed to parse port from proxy stdout: %s", output)
 }
 
+func (f *Framework) UninstallChart() error {
+	cmd := exec.Command("helm", "uninstall", "--namespace", f.Namespace, "nginx-ingress")
+	_, err := cmd.CombinedOutput()
+	if err != nil {
+		return fmt.Errorf("unexpected error uninstalling ingress-nginx release: %v", err)
+	}
+
+	return nil
+}
+
 func startCmdAndStreamOutput(cmd *exec.Cmd) (stdout, stderr io.ReadCloser, err error) {
 	stdout, err = cmd.StdoutPipe()
 	if err != nil {

test/e2e/framework/framework.go

@@ -150,7 +150,11 @@ func (f *Framework) AfterEach() {
 
 	defer func(kubeClient kubernetes.Interface, ingressclass string) {
 		defer ginkgo.GinkgoRecover()
-		err := deleteIngressClass(kubeClient, ingressclass)
+
+		err := f.UninstallChart()
+		assert.Nil(ginkgo.GinkgoT(), err, "uninstalling helm chart")
+
+		err = deleteIngressClass(kubeClient, ingressclass)
 		assert.Nil(ginkgo.GinkgoT(), err, "deleting IngressClass")
 	}(f.KubeClientSet, f.IngressClass)
 
@@ -192,6 +196,11 @@ func IngressNginxDescribe(text string, body func()) bool {
 	return ginkgo.Describe(text, body)
 }
 
+// IngressNginxDescribeSerial wrapper function for ginkgo describe. Adds namespacing.
+func IngressNginxDescribeSerial(text string, body func()) bool {
+	return ginkgo.Describe(text, ginkgo.Serial, body)
+}
+
 // DescribeAnnotation wrapper function for ginkgo describe. Adds namespacing.
 func DescribeAnnotation(text string, body func()) bool {
 	return ginkgo.Describe("[Annotations] "+text, body)
@@ -202,11 +211,6 @@ func DescribeSetting(text string, body func()) bool {
 	return ginkgo.Describe("[Setting] "+text, body)
 }
 
-// MemoryLeakIt is wrapper function for ginkgo It.  Adds "[MemoryLeak]" tag and makes static analysis easier.
-func MemoryLeakIt(text string, body interface{}) bool {
-	return ginkgo.It(text+" [MemoryLeak]", body)
-}
-
 // GetNginxIP returns the number of TCP port where NGINX is running
 func (f *Framework) GetNginxIP() string {
 	s, err := f.KubeClientSet.
@@ -387,7 +391,7 @@ func (f *Framework) UpdateNginxConfigMapData(key string, value string) {
 }
 
 // WaitForReload calls the passed function and
-// asser it has caused at least 1 reload.
+// asserts it has caused at least 1 reload.
 func (f *Framework) WaitForReload(fn func()) {
 	initialReloadCount := getReloadCount(f.pod, f.Namespace, f.KubeClientSet)
 

test/e2e/framework/influxdb.go

@@ -68,9 +68,7 @@ func (f *Framework) NewInfluxDBDeployment() {
 		},
 	}
 
-	cm, err := f.EnsureConfigMap(configuration)
-	assert.Nil(ginkgo.GinkgoT(), err, "creating an Influxdb deployment")
-	assert.NotNil(ginkgo.GinkgoT(), cm, "expected a configmap but none returned")
+	f.EnsureConfigMap(configuration)
 
 	deployment := &appsv1.Deployment{
 		ObjectMeta: metav1.ObjectMeta{
@@ -136,7 +134,7 @@ func (f *Framework) NewInfluxDBDeployment() {
 
 	d := f.EnsureDeployment(deployment)
 
-	err = waitForPodsReady(f.KubeClientSet, DefaultTimeout, 1, f.Namespace, metav1.ListOptions{
+	err := waitForPodsReady(f.KubeClientSet, DefaultTimeout, 1, f.Namespace, metav1.ListOptions{
 		LabelSelector: fields.SelectorFromSet(fields.Set(d.Spec.Template.ObjectMeta.Labels)).String(),
 	})
 	assert.Nil(ginkgo.GinkgoT(), err, "waiting for influxdb pod to become ready")