From 9b73062a06c05a9aeabfb75ee08eb23a02638ae2 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Sun, 9 Jul 2023 17:45:43 +0000 Subject: [PATCH] Add validations to CI --- .github/workflows/ci.yaml | 49 +++++++++++++++++++ charts/ingress-nginx/templates/_params.tpl | 3 ++ charts/ingress-nginx/values.yaml | 1 + docs/user-guide/cli-arguments.md | 2 +- .../ingress/annotations/alias/main_test.go | 4 +- internal/ingress/annotations/parser/main.go | 8 +-- .../ingress/annotations/parser/validators.go | 2 +- pkg/flags/flags.go | 6 +-- test/e2e/framework/exec.go | 7 ++- test/e2e/run-e2e-suite.sh | 1 + test/e2e/run-kind-e2e.sh | 1 + test/e2e/wait-for-nginx.sh | 2 + 12 files changed, 74 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 097e77276..453775379 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -319,6 +319,55 @@ jobs: name: e2e-test-reports-${{ matrix.k8s }} path: 'test/junitreports/report*.xml' + kubernetes-validations: + name: Kubernetes with Validations + runs-on: ubuntu-latest + needs: + - changes + - build + if: | + (needs.changes.outputs.go == 'true') || ${{ inputs.run_e2e }} + + strategy: + matrix: + k8s: [v1.27.1] + + steps: + - name: Checkout + uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3 + + - name: cache + uses: actions/download-artifact@9bc31d5ccc31df68ecc42ccf4149144866c47d8a # v3.0.2 + with: + name: docker.tar.gz + + - name: Create Kubernetes ${{ matrix.k8s }} cluster + id: kind + run: | + kind create cluster --image=kindest/node:${{ matrix.k8s }} --config test/e2e/kind.yaml + + - name: Load images from cache + run: | + echo "loading docker images..." + pigz -dc docker.tar.gz | docker load + + - name: Run e2e tests + env: + KIND_CLUSTER_NAME: kind + SKIP_CLUSTER_CREATION: true + SKIP_IMAGE_CREATION: true + ENABLE_VALIDATIONS: true + run: | + kind get kubeconfig > $HOME/.kube/kind-config-kind + make kind-e2e-test + + - name: Upload e2e junit-reports + uses: actions/upload-artifact@0b7f8abb1508181956e8e162db84b466c27e18ce # v3.1.2 + if: success() || failure() + with: + name: e2e-test-reports-${{ matrix.k8s }} + path: 'test/junitreports/report*.xml' + kubernetes-chroot: name: Kubernetes chroot diff --git a/charts/ingress-nginx/templates/_params.tpl b/charts/ingress-nginx/templates/_params.tpl index a1aef01ae..6358b49f7 100644 --- a/charts/ingress-nginx/templates/_params.tpl +++ b/charts/ingress-nginx/templates/_params.tpl @@ -1,5 +1,8 @@ {{- define "ingress-nginx.params" -}} - /nginx-ingress-controller +{{- if .Values.controller.enableValidations }} +- --enable-annotation-validation=true +{{- end }} {{- if .Values.defaultBackend.enabled }} - --default-backend-service=$(POD_NAMESPACE)/{{ include "ingress-nginx.defaultBackend.fullname" . }} {{- end }} diff --git a/charts/ingress-nginx/values.yaml b/charts/ingress-nginx/values.yaml index d091391a8..90edac84c 100644 --- a/charts/ingress-nginx/values.yaml +++ b/charts/ingress-nginx/values.yaml @@ -15,6 +15,7 @@ commonLabels: {} controller: name: controller + enableValidations: false image: ## Keep false as default for now! chroot: false diff --git a/docs/user-guide/cli-arguments.md b/docs/user-guide/cli-arguments.md index bbd69f359..be6e1dd50 100644 --- a/docs/user-guide/cli-arguments.md +++ b/docs/user-guide/cli-arguments.md @@ -15,7 +15,7 @@ They are set in the container spec of the `ingress-nginx-controller` Deployment | `--default-backend-service` | Service used to serve HTTP requests not matching any known server name (catch-all). Takes the form "namespace/name". The controller configures NGINX to forward requests to the first port of this Service. | | `--default-server-port` | Port to use for exposing the default server (catch-all). (default 8181) | | `--default-ssl-certificate` | Secret containing a SSL certificate to be used by the default HTTPS server (catch-all). Takes the form "namespace/name". | -| `--disable-annotation-validation` | If true, will disable the annotation validation feature. This value will be defaulted to false on a future release. | +| `--enable-annotation-validation` | If true, will enable the annotation validation feature. This value will be defaulted to true on a future release. | | `--disable-catch-all` | Disable support for catch-all Ingresses. (default false) | | `--disable-full-test` | Disable full test of all merged ingresses at the admission stage and tests the template of the ingress being created or updated (full test of all ingresses is enabled by default). | | `--disable-svc-external-name` | Disable support for Services of type ExternalName. (default false) | diff --git a/internal/ingress/annotations/alias/main_test.go b/internal/ingress/annotations/alias/main_test.go index e212f3cf4..1965f2630 100644 --- a/internal/ingress/annotations/alias/main_test.go +++ b/internal/ingress/annotations/alias/main_test.go @@ -63,10 +63,10 @@ func TestParse(t *testing.T) { for _, testCase := range testCases { ing.SetAnnotations(testCase.annotations) if testCase.skipValidation { - parser.DisableAnnotationValidation = true + parser.EnableAnnotationValidation = false } defer func() { - parser.DisableAnnotationValidation = false + parser.EnableAnnotationValidation = true }() result, err := ap.Parse(ing) if (err != nil) != testCase.wantErr { diff --git a/internal/ingress/annotations/parser/main.go b/internal/ingress/annotations/parser/main.go index d517cbc57..951970e27 100644 --- a/internal/ingress/annotations/parser/main.go +++ b/internal/ingress/annotations/parser/main.go @@ -30,15 +30,15 @@ import ( // DefaultAnnotationsPrefix defines the common prefix used in the nginx ingress controller const ( - DefaultAnnotationsPrefix = "nginx.ingress.kubernetes.io" - DefaultDisableAnnotationValidation = false + DefaultAnnotationsPrefix = "nginx.ingress.kubernetes.io" + DefaultEnableAnnotationValidation = true ) var ( // AnnotationsPrefix is the mutable attribute that the controller explicitly refers to AnnotationsPrefix = DefaultAnnotationsPrefix - // DisableAnnotationValidation is the mutable attribute for enabling or disabling the validation functions - DisableAnnotationValidation = DefaultDisableAnnotationValidation + // Enable is the mutable attribute for enabling or disabling the validation functions + EnableAnnotationValidation = DefaultEnableAnnotationValidation ) // AnnotationGroup defines the group that this annotation may belong diff --git a/internal/ingress/annotations/parser/validators.go b/internal/ingress/annotations/parser/validators.go index 8ecf18add..e14b486eb 100644 --- a/internal/ingress/annotations/parser/validators.go +++ b/internal/ingress/annotations/parser/validators.go @@ -216,7 +216,7 @@ func checkAnnotation(name string, ing *networking.Ingress, fields AnnotationFiel } } // We don't run validation against empty values - if !DisableAnnotationValidation && annotationValue != "" { + if EnableAnnotationValidation && annotationValue != "" { if err := validateFunc(annotationValue); err != nil { klog.Warningf("validation error on ingress %s/%s: annotation %s contains invalid value %s", ing.GetNamespace(), ing.GetName(), name, annotationValue) return "", ing_errors.NewValidationError(annotationFullName) diff --git a/pkg/flags/flags.go b/pkg/flags/flags.go index 7402218e3..2c926a35f 100644 --- a/pkg/flags/flags.go +++ b/pkg/flags/flags.go @@ -152,8 +152,8 @@ Requires the update-status parameter.`) annotationsPrefix = flags.String("annotations-prefix", parser.DefaultAnnotationsPrefix, `Prefix of the Ingress annotations specific to the NGINX controller.`) - disableAnnotationValidation = flags.Bool("disable-annotation-validation", true, - `If true, will disable the annotation validation feature. This value will be defaulted to false on a future release`) + enableAnnotationValidation = flags.Bool("enable-annotation-validation", false, + `If true, will enable the annotation validation feature. This value will be defaulted to true on a future release`) enableSSLChainCompletion = flags.Bool("enable-ssl-chain-completion", false, `Autocomplete SSL certificate chains with missing intermediate CA certificates. @@ -252,7 +252,7 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g } parser.AnnotationsPrefix = *annotationsPrefix - parser.DisableAnnotationValidation = *disableAnnotationValidation + parser.EnableAnnotationValidation = *enableAnnotationValidation // check port collisions if !ing_net.IsPortAvailable(*httpPort) { diff --git a/test/e2e/framework/exec.go b/test/e2e/framework/exec.go index d91d36551..84371e4a3 100644 --- a/test/e2e/framework/exec.go +++ b/test/e2e/framework/exec.go @@ -116,7 +116,12 @@ func (f *Framework) newIngressController(namespace string, namespaceOverlay stri if !ok { isChroot = "false" } - cmd := exec.Command("./wait-for-nginx.sh", namespace, namespaceOverlay, isChroot) + + enableValidations, ok := os.LookupEnv("ENABLE_VALIDATIONS") + if !ok { + enableValidations = "false" + } + cmd := exec.Command("./wait-for-nginx.sh", namespace, namespaceOverlay, isChroot, enableValidations) out, err := cmd.CombinedOutput() if err != nil { return fmt.Errorf("unexpected error waiting for ingress controller deployment: %v.\nLogs:\n%v", err, string(out)) diff --git a/test/e2e/run-e2e-suite.sh b/test/e2e/run-e2e-suite.sh index b56312afd..015895e56 100755 --- a/test/e2e/run-e2e-suite.sh +++ b/test/e2e/run-e2e-suite.sh @@ -78,6 +78,7 @@ kubectl run --rm \ --env="E2E_NODES=${E2E_NODES}" \ --env="FOCUS=${FOCUS}" \ --env="IS_CHROOT=${IS_CHROOT:-false}"\ + --env="ENABLE_VALIDATIONS=${ENABLE_VALIDATIONS:-false}"\ --env="E2E_CHECK_LEAKS=${E2E_CHECK_LEAKS}" \ --env="NGINX_BASE_IMAGE=${NGINX_BASE_IMAGE}" \ --env="HTTPBUN_IMAGE=${HTTPBUN_IMAGE}" \ diff --git a/test/e2e/run-kind-e2e.sh b/test/e2e/run-kind-e2e.sh index e6e4e086b..4dc1bddd0 100755 --- a/test/e2e/run-kind-e2e.sh +++ b/test/e2e/run-kind-e2e.sh @@ -39,6 +39,7 @@ fi KIND_LOG_LEVEL="1" IS_CHROOT="${IS_CHROOT:-false}" +ENABLE_VALIDATIONS="${ENABLE_VALIDATIONS:-false}" export KIND_CLUSTER_NAME=${KIND_CLUSTER_NAME:-ingress-nginx-dev} DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" # Use 1.0.0-dev to make sure we use the latest configuration in the helm template diff --git a/test/e2e/wait-for-nginx.sh b/test/e2e/wait-for-nginx.sh index 153d348c2..7ed97bb9c 100755 --- a/test/e2e/wait-for-nginx.sh +++ b/test/e2e/wait-for-nginx.sh @@ -24,6 +24,7 @@ DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" export NAMESPACE=$1 export NAMESPACE_OVERLAY=$2 export IS_CHROOT=$3 +export ENABLE_VALIDATIONS=$4 echo "deploying NGINX Ingress controller in namespace $NAMESPACE" @@ -68,6 +69,7 @@ else # TODO: remove the need to use fullnameOverride fullnameOverride: nginx-ingress controller: + enableValidations: ${ENABLE_VALIDATIONS} image: repository: ingress-controller/controller chroot: ${IS_CHROOT}