Test traefik with kubernetes.ingress.localhost-only #5151

Nino-K · 2023-07-12T00:05:57Z

Adds additional tests to test the kubernetes.ingress.localhost-only setting.

bats/tests/k8s/traefik.bats

+    try --max 30 --delay 10 curl_exe --head --insecure "https://localhost:443"
+    assert_success
+    assert_output --regexp 'HTTP/[0-9.]* 404'
+    # traefik should not be accessible on other interfacec


jandubois

First round of feedback

bats/tests/helpers/paths.bash

bats/tests/k8s/traefik.bats

jandubois · 2023-07-21T20:15:52Z

bats/tests/k8s/traefik.bats

+curl_exe_http_traefik() {
+    local HTTP_URL=$1
+    try --max 30 --delay 10 curl.exe --head "$HTTP_URL"
+    assert_success
+    assert_output --regexp 'HTTP/[0-9.]* 404'
+}


You could generalize this slightly and get rid of curl_exe_https_traefik:

Suggested change

curl_exe_http_traefik() {

local HTTP_URL=$1

try --max 30 --delay 10 curl.exe --head "$HTTP_URL"

assert_success

assert_output --regexp 'HTTP/[0-9.]* 404'

}

curl_exe_traefik() {

try --max 30 --delay 10 curl.exe --head "$@"

assert_success

assert_output --regexp 'HTTP/[0-9.]* 404'

}

And then call it:

curl_exe_traefik "http://$(wsl_ip):80" curl_exe_traefik --insecure "https://$(wsl_ip):443"

jandubois · 2023-07-21T20:18:51Z

bats/tests/k8s/traefik.bats

+    run try --max 30 --delay 10 curl --head "http://localhost:80"
    assert_success
    assert_output --regexp 'HTTP/[0-9.]* 404'
-    run try --max 30 --delay 10 curl --head --insecure "https://$(get_host):443"
+    run try --max 30 --delay 10 curl --head --insecure "https://localhost:443"
    assert_success
    assert_output --regexp 'HTTP/[0-9.]* 404'


Wouldn't all this turn into:

curl_exe_traefik "http://localhost:80" curl_exe_traefik --insecure "https://localhost:443"

That one is calling curl, basically, curl from inside the distro.

jandubois · 2023-07-21T20:19:49Z

bats/tests/k8s/traefik.bats

+    wait_for_apiserver
+    # Check if the traefik pods come up
+    try --max 30 --delay 10 assert_traefik_pods_are_up
+    assert_success


You don't need assert_success because you did't use run.

jandubois · 2023-07-21T20:22:30Z

bats/tests/k8s/traefik.bats

+
+    # traefik should not be accessible on other interface
+    run curl.exe --head "http://$(wsl_ip):80"
+    assert_output --partial "Couldn't connect to server"


I can't make up my mind if there should be an assert_success before testing $output, but I guess it doesn't matter, as we don't test how curl itself behaves.

jandubois · 2023-07-21T23:18:01Z

bats/tests/k8s/traefik.bats

+    try --max 30 --delay 10 assert_traefik_pods_are_up
+}
+
+@test 'curl traefik endpoints from Windows Host while kubernetes.ingress.localhost-only is true' {


This test needs a if ! is_windows check

jandubois

Just saw that I still had a pending comment I never sent

jandubois · 2023-07-21T23:26:52Z

bats/tests/k8s/traefik.bats

@@ -45,18 +41,15 @@ assert_traefik_pods_are_down() {
 }

 assert_traefik_pods_are_up() {
-    run kubectl get --all-namespaces pods
+    ip_regex="^([0-9]{1,3}\.){3}[0-9]{1,3}$"
+    run kubectl -n kube-system get service traefik -o jsonpath="{.status.loadBalancer.ingress[0].ip}"


Probably should be followed by assert_success. The regex match would fail anyways, but I think it would be clearer to show that kubectl itself failed.

jandubois

The test seems to hang indefinitely on Windows with RD_USE_NETWORKING_TUNNEL=1 RD_USE_WINDOWS_EXE=0:

 ✗ start k8s
   (from function `assert' in file bats-assert/src/assert.bash, line 40,
    from function `wait_for_apiserver' in file tests/helpers/kubernetes.bash, line 6,
    in test file tests/k8s/traefik.bats, line 83)
     `wait_for_apiserver' failed
   time="2023-08-01T15:26:36-07:00" level=info msg="About to launch C:\\Program Files\\Rancher Desktop\\Rancher Desktop.exe --application.debug=true --application.updater.enabled=false --containerEngine.name containerd --kubernetes.version 1.22.7 --kubernetes.enabled=true --kubernetes.options.traefik=true --experimental.virtualMachine.networkingTunnel=true ...\n"
   (node:4652) [DEP0123] DeprecationWarning: Setting the TLS ServerName to an IP address is not permitted by RFC 6066. This will be ignored in a future version.
   (Use `Rancher Desktop --trace-deprecation ...` to show where the warning was created)

   -- assertion failed --
   expression : [ 1690929396 -lt 1690929396 ]
   --

If the tests are not (yet) expected to work in this combination, then they should be skipped.

Other than this, everything looks fine.

Nino-K · 2023-08-03T16:23:10Z

RD_USE_NETWORKING_TUNNEL=1

This combination needs to be skipped or cannot run right now due to the wsl integration. The wait_for_apiserver is running from the Ubuntu distro which makes an assumption that the API is accessible via 127.0.0.1:6443.
However, since we have not implemented the wsl integration in the new network stack this is not going to work.

Signed-off-by: Nino Kodabande <nkodabande@suse.com>

ericpromislow

Tests passed on all three platforms. Code looks fine. Let's merge

The test now skips when RD_USE_NETWORKING_TUNNEL=1 and RD_USE_WINDOWS_EXE=0

github-advanced-security bot found potential problems Jul 12, 2023

View reviewed changes

Nino-K force-pushed the add-bats-traefik-test branch 2 times, most recently from ae0c61c to fc8ca34 Compare July 12, 2023 00:09

gaktive requested a review from jandubois July 12, 2023 18:11

Nino-K force-pushed the add-bats-traefik-test branch 4 times, most recently from 07d669c to 02a6509 Compare July 21, 2023 20:02

jandubois requested changes Jul 21, 2023

View reviewed changes

Nino-K force-pushed the add-bats-traefik-test branch from 02a6509 to 1486a30 Compare July 21, 2023 21:18

jandubois requested changes Jul 21, 2023

View reviewed changes

Nino-K force-pushed the add-bats-traefik-test branch 3 times, most recently from a784566 to 6a628f1 Compare July 28, 2023 17:50

jandubois reviewed Jul 28, 2023

View reviewed changes

Nino-K force-pushed the add-bats-traefik-test branch from 6a628f1 to 55190b6 Compare August 1, 2023 16:49

Nino-K requested a review from jandubois August 1, 2023 17:34

jandubois previously requested changes Aug 1, 2023

View reviewed changes

Test traefik with kubernetes.ingress.localhost-only

6062685

Signed-off-by: Nino Kodabande <nkodabande@suse.com>

Nino-K force-pushed the add-bats-traefik-test branch from 55190b6 to 6062685 Compare August 3, 2023 17:01

ericpromislow approved these changes Aug 3, 2023

View reviewed changes

ericpromislow merged commit e55dc89 into main Aug 3, 2023
14 checks passed

ericpromislow deleted the add-bats-traefik-test branch August 3, 2023 21:05

Nino-K mentioned this pull request Aug 23, 2023

Epic: Write bats tests that target networking scenarios #5185

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test traefik with kubernetes.ingress.localhost-only #5151

Test traefik with kubernetes.ingress.localhost-only #5151

Nino-K commented Jul 12, 2023

jandubois left a comment

jandubois Jul 21, 2023

jandubois Jul 21, 2023

Nino-K Jul 21, 2023

jandubois Jul 21, 2023

jandubois Jul 21, 2023

jandubois Jul 21, 2023

jandubois left a comment

jandubois Jul 21, 2023

jandubois left a comment •

edited

Loading

Nino-K commented Aug 3, 2023

ericpromislow left a comment

Test traefik with kubernetes.ingress.localhost-only #5151

Test traefik with kubernetes.ingress.localhost-only #5151

Conversation

Nino-K commented Jul 12, 2023

jandubois left a comment

Choose a reason for hiding this comment

jandubois Jul 21, 2023

Choose a reason for hiding this comment

jandubois Jul 21, 2023

Choose a reason for hiding this comment

Nino-K Jul 21, 2023

Choose a reason for hiding this comment

jandubois Jul 21, 2023

Choose a reason for hiding this comment

jandubois Jul 21, 2023

Choose a reason for hiding this comment

jandubois Jul 21, 2023

Choose a reason for hiding this comment

jandubois left a comment

Choose a reason for hiding this comment

jandubois Jul 21, 2023

Choose a reason for hiding this comment

jandubois left a comment • edited Loading

Choose a reason for hiding this comment

Nino-K commented Aug 3, 2023

ericpromislow left a comment

Choose a reason for hiding this comment

jandubois left a comment •

edited

Loading