Skip to content

Port 5 router cookie tests from openshift-tests-private #31157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dgoodwin wants to merge 3 commits into from
Closed

Port 5 router cookie tests from openshift-tests-private #31157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6166637
Port 5 router cookie tests from openshift-tests-private
dgoodwin May 11, 2026
98db4e4
gofmt
dgoodwin May 11, 2026
be507ab
bindata
dgoodwin May 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
253 changes: 253 additions & 0 deletions test/extended/router/cookies_otp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,253 @@
package router

import (
"fmt"
"os"
"os/exec"
"path/filepath"

g "github.com/onsi/ginkgo/v2"
o "github.com/onsi/gomega"

exutil "github.com/openshift/origin/test/extended/util"
admissionapi "k8s.io/pod-security-admission/api"
)

var _ = g.Describe("[sig-network-edge] Network_Edge Component_Router [OTP]", func() {
defer g.GinkgoRecover()

var oc = exutil.NewCLIWithPodSecurityLevel("route-cookies", admissionapi.LevelBaseline)

g.It("[OTP] [OCP-11903] haproxy cookies based sticky session for unsecure routes [apigroup:route.openshift.io]", func() {
var (
buildPruningBaseDir = exutil.FixturePath("testdata", "router")
customTemp = filepath.Join(buildPruningBaseDir, "ingresscontroller-np.yaml")
clientPod = filepath.Join(buildPruningBaseDir, "test-client-pod.yaml")
clientPodName = "hello-pod"
clientPodLabel = "app=hello-pod"
testPodSvc = filepath.Join(buildPruningBaseDir, "web-server-deploy.yaml")
srvrcInfo = "web-server-deploy"
unsecSvcName = "service-unsecure"
cookie = "/data/OCP-11903-cookie"

ingctrl = ingressControllerDescription{
name: "otp11903",
namespace: "openshift-ingress-operator",
domain: "",
template: customTemp,
}
)

g.By("1.0 Create a custom ingresscontroller")
baseDomain := getBaseDomain(oc)
ingctrl.domain = ingctrl.name + "." + baseDomain
defer ingctrl.delete(oc)
ingctrl.create(oc)
ensureRouterDeployGenerationIs(oc, ingctrl.name, "1")

g.By("2.0: Prepare file for testing")
updateFilebySedCmd(testPodSvc, "replicas: 1", "replicas: 2")
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Mutating shared fixture files can cause test flakiness or pollution.

updateFilebySedCmd modifies web-server-deploy.yaml on disk. Multiple tests (OCP-11903, OCP-12566, OCP-15872) call this on the same file. If tests run in parallel or the file is already modified from a previous run, the sed replacement becomes a no-op (the pattern won't match). Consider either:

  1. Creating a temporary copy of the fixture per test, or
  2. Using oc process with parameter overrides instead of file mutation
🛠️ Suggested approach: copy fixture to temp file 
// Create a temp copy of the fixture
tmpFile, err := os.CreateTemp("", "web-server-deploy-*.yaml")
o.Expect(err).NotTo(o.HaveOccurred())
defer os.Remove(tmpFile.Name())

content, err := os.ReadFile(testPodSvc)
o.Expect(err).NotTo(o.HaveOccurred())

modifiedContent := strings.ReplaceAll(string(content), "replicas: 1", "replicas: 2")
err = os.WriteFile(tmpFile.Name(), []byte(modifiedContent), 0644)
o.Expect(err).NotTo(o.HaveOccurred())

// Use tmpFile.Name() instead of testPodSvc
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/extended/router/cookies_otp.go` at line 49, The test mutates a shared
fixture by calling updateFilebySedCmd on testPodSvc which can cause flakiness;
instead create a temp copy of the fixture or avoid in-place edits (e.g., use oc
process with parameter overrides). Implement by reading the file referenced by
testPodSvc, writing a modified copy to a temporary file (os.CreateTemp /
os.WriteFile), replace "replicas: 1" → "replicas: 2" in that temp content, use
the temp file path for subsequent calls instead of testPodSvc, and ensure the
temp file is removed in a defer; alternatively refactor the apply step to call
oc process with parameter overrides rather than mutating the file.


g.By("3.0: Create a client pod and two server pods and the service")
ns := oc.Namespace()
err := oc.AsAdmin().WithoutNamespace().Run("create").Args("-n", ns, "-f", clientPod).Execute()
o.Expect(err).NotTo(o.HaveOccurred())
ensurePodWithLabelReady(oc, ns, clientPodLabel)
srvPodList := createResourceFromWebServer(oc, ns, testPodSvc, srvrcInfo)

g.By("4.0: Create a plain HTTP route")
routehost := "unsecure11903" + "." + ingctrl.domain
createRouteOTP(oc, ns, "http", unsecSvcName, unsecSvcName, []string{"--hostname=" + routehost})
ensureRouteIsAdmittedByIngressController(oc, ns, unsecSvcName, ingctrl.name)

g.By("5.0: Curl the http route, make sure the second server is hit")
routerpod := getOneNewRouterPodFromRollingUpdate(oc, ingctrl.name)
podIP := getPodv4Address(oc, routerpod, "openshift-ingress")
toDst := routehost + ":80:" + podIP
curlCmd := []string{"-n", ns, clientPodName, "--", "curl", "http://" + routehost, "-s", "-c", cookie, "--resolve", toDst, "--connect-timeout", "10"}
expectOutput := []string{"Hello-OpenShift " + srvPodList[1] + " http-8080"}
repeatCmdOnClient(oc, curlCmd, expectOutput, 60, 1)

g.By("6.0: Curl the http route again, make sure the first server is hit")
curlCmd = []string{"-n", ns, clientPodName, "--", "curl", "http://" + routehost, "-s", "--resolve", toDst, "--connect-timeout", "10"}
expectOutput = []string{"Hello-OpenShift " + srvPodList[0] + " http-8080"}
repeatCmdOnClient(oc, curlCmd, expectOutput, 60, 1)

g.By("7.0: Curl the http route with the specified cookie for 10 times, expect all are forwarded to the second server")
curlCmd = []string{"-n", ns, clientPodName, "--", "curl", "http://" + routehost, "-s", "-b", cookie, "--resolve", toDst, "--connect-timeout", "10"}
expectOutput = []string{"Hello-OpenShift " + srvPodList[0] + " http-8080", "Hello-OpenShift " + srvPodList[1] + " http-8080"}
_, result := repeatCmdOnClient(oc, curlCmd, expectOutput, 120, 10)
o.Expect(result[1]).To(o.Equal(10))

g.By(`8.0: Disable haproxy hash based sticky session for the route by adding 'disable cookies' annotation to it`)
setAnnotation(oc, ns, "route/"+unsecSvcName, "haproxy.router.openshift.io/disable_cookies=true")

g.By("9.0: Check the disable cookies configuration in haproxy")
backendStart := fmt.Sprintf(`backend be_http:%s:%s`, ns, unsecSvcName)
ensureHaproxyBlockConfigNotMatchRegexp(oc, routerpod, backendStart, []string{`cookie .+httponly`})

g.By("10.0: Curl the http route with the specified cookie for 10 times again, expect the requests are forwarded to the two servers")
_, result = repeatCmdOnClient(oc, curlCmd, expectOutput, 120, 10)
o.Expect(result[0] + result[1]).To(o.Equal(10))
o.Expect(result[0]*result[1] > 0).To(o.BeTrue())
})

g.It("[OTP] [OCP-12566] Cookie name should not use openshift prefix [apigroup:route.openshift.io]", func() {
if !isCanaryRouteAvailable(oc) {
g.Skip("Skip for the ingress canary route could not be available to the outside")
}

var (
buildPruningBaseDir = exutil.FixturePath("testdata", "router")
testPodSvc = filepath.Join(buildPruningBaseDir, "web-server-deploy.yaml")
srvrcInfo = "web-server-deploy"
unsecSvcName = "service-unsecure"
fileDir = "/tmp/OCP-12566-cookie"
cookie = fileDir + "/cookie12566"
routeName = "unsecureroute2"
)

g.By("1.0: Prepare file folder and file for testing")
defer os.RemoveAll(fileDir)
err := os.MkdirAll(fileDir, 0755)
o.Expect(err).NotTo(o.HaveOccurred())

g.By("2.0: Create two server pods and the service")
ns := oc.Namespace()
updateFilebySedCmd(testPodSvc, "replicas: 1", "replicas: 2")
createResourceFromWebServer(oc, ns, testPodSvc, srvrcInfo)

g.By("3.0: Create a plain HTTP route")
routehost := "unsecure12566" + ".apps." + getBaseDomain(oc)
createRouteOTP(oc, ns, "http", routeName, unsecSvcName, []string{"--hostname=" + routehost})
ensureRouteIsAdmittedByIngressController(oc, ns, routeName, "default")

g.By("4.0: Curl the http route, make sure one server is hit")
curlCmd := fmt.Sprintf(`curl http://%s -s -c %s --connect-timeout 10`, routehost, cookie)
expectOutput := []string{"Hello-OpenShift"}
repeatCmdOnClient(oc, curlCmd, expectOutput, 60, 1)

g.By("5.0: Check the cookies which should not contain a OPENSHIFT prefix or a md5 hash")
cmd := fmt.Sprintf(`cat %s`, cookie)
cookiesOutput, err := exec.Command("bash", "-c", cmd).Output()
o.Expect(err).NotTo(o.HaveOccurred())
Comment on lines +131 to +133
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify there are no remaining shell-wrapped exec.Command usages in router extended tests
rg -nP 'exec\.Command\("bash",\s*"-c"' --type go test/extended/router

Repository: openshift/origin

Length of output: 362

🏁 Script executed:

#!/bin/bash
# Get the context around lines 131-133 in cookies_otp.go
sed -n '125,140p' test/extended/router/cookies_otp.go | cat -n

Repository: openshift/origin

Length of output: 1059

🏁 Script executed:

#!/bin/bash
# Check the util_otp.go instances to see what they're doing
echo "=== util_otp.go:391 ===" 
sed -n '385,395p' test/extended/router/util_otp.go | cat -n
echo ""
echo "=== util_otp.go:448 ===" 
sed -n '442,452p' test/extended/router/util_otp.go | cat -n

Repository: openshift/origin

Length of output: 907

🏁 Script executed:

#!/bin/bash
# Verify what imports are available in cookies_otp.go
head -30 test/extended/router/cookies_otp.go | cat -n

Repository: openshift/origin

Length of output: 1332

Replace shell-wrapped cat with os.ReadFile() for cleaner, safer file reading.

Line 132 uses exec.Command("bash", "-c", cmd) to read a file with cat. Use os.ReadFile(cookie) instead, which returns []byte and error matching the existing error handling pattern.

Safer direct read 
- cmd := fmt.Sprintf(`cat %s`, cookie)
- cookiesOutput, err := exec.Command("bash", "-c", cmd).Output()
+ cookiesOutput, err := os.ReadFile(cookie)
  o.Expect(err).NotTo(o.HaveOccurred())
🧰 Tools
🪛 OpenGrep (1.20.0)

[ERROR] 132-132: Dynamic command passed to exec.Command with a shell invocation. Pass arguments directly to exec.Command without a shell wrapper.

(coderabbit.command-injection.go-exec-command)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/extended/router/cookies_otp.go` around lines 131 - 133, Replace the
shell-backed cat call that builds cmd and runs exec.Command("bash", "-c", cmd)
with a direct file read using os.ReadFile(cookie); assign the returned []byte to
cookiesOutput and handle the error the same way
(o.Expect(err).NotTo(o.HaveOccurred())). Update the references to cmd and
exec.Command to use os.ReadFile(cookie) so file contents are read safely without
spawning a shell.

o.Expect(string(cookiesOutput)).NotTo(o.ContainSubstring("OPENSHIFT"))
o.Expect(string(cookiesOutput)).NotTo(o.MatchRegexp(`\d+\.\d+\.\d+\.\d+`))
})

g.It("[OTP] [OCP-15872] can set cookie name for unsecure routes by annotation [apigroup:route.openshift.io]", func() {
if !isCanaryRouteAvailable(oc) {
g.Skip("Skip for the ingress canary route could not be available to the outside")
}

var (
buildPruningBaseDir = exutil.FixturePath("testdata", "router")
testPodSvc = filepath.Join(buildPruningBaseDir, "web-server-deploy.yaml")
srvrcInfo = "web-server-deploy"
unsecSvcName = "service-unsecure"
)

g.By("1.0: Create two server pods and the service")
updateFilebySedCmd(testPodSvc, "replicas: 1", "replicas: 2")
ns := oc.Namespace()
srvPodList := createResourceFromWebServer(oc, ns, testPodSvc, srvrcInfo)

g.By("2.0: Create a plain HTTP route")
routehost := "unsecure15872" + ".apps." + getBaseDomain(oc)
createRouteOTP(oc, ns, "http", unsecSvcName, unsecSvcName, []string{"--hostname=" + routehost})
ensureRouteIsAdmittedByIngressController(oc, ns, unsecSvcName, "default")

g.By("3.0: Set a cookie name to the route by the annotation, and then ensure the change in haproxy")
setAnnotation(oc, ns, "route/"+unsecSvcName, "router.openshift.io/cookie_name=unsecure-cookie_1")
routerPod := getOneRouterPodNameByIC(oc, "default")
ensureHaproxyBlockConfigContains(oc, routerPod, "be_http:"+ns+":"+unsecSvcName, []string{"unsecure-cookie_1"})

g.By("4.0: Curl the http route, make sure the second server is hit and the cookie is set in the client side")
curlCmd := fmt.Sprintf(`curl http://%s -sv --connect-timeout 10`, routehost)
expectOutput := []string{"Hello-OpenShift " + srvPodList[1] + " http-8080"}
output, _ := repeatCmdOnClient(oc, curlCmd, expectOutput, 60, 1)
o.Expect(output).To(o.MatchRegexp(`(s|S)et-(c|C)ookie: unsecure-cookie_1=[0-9a-zA-Z]+`))
})

g.It("[OTP] [OCP-35547] cookie-same-site route annotation accepts None Lax or Strict attribute for edge routes [apigroup:route.openshift.io]", func() {
if !isCanaryRouteAvailable(oc) {
g.Skip("Skip for the ingress canary route could not be available to the outside")
}

var (
buildPruningBaseDir = exutil.FixturePath("testdata", "router")
testPodSvc = filepath.Join(buildPruningBaseDir, "web-server-deploy.yaml")
srvrcInfo = "web-server-deploy"
unsecSvcName = "service-unsecure"
)

g.By("1.0: Create two server pods and the service")
ns := oc.Namespace()
createResourceFromWebServer(oc, ns, testPodSvc, srvrcInfo)

g.By("2.0: Create an edge route")
routehost := "edge35547" + ".apps." + getBaseDomain(oc)
createRouteOTP(oc, ns, "edge", "edge35547", unsecSvcName, []string{"--hostname=" + routehost})
ensureRouteIsAdmittedByIngressController(oc, ns, "edge35547", "default")

g.By("3.0: Curl the edge route, and check set-cookie header, expect getting SameSite=None")
curlCmd := fmt.Sprintf(`curl https://%s -sSkI --connect-timeout 10`, routehost)
expectOutput := []string{"set-cookie:"}
result, _ := repeatCmdOnClient(oc, curlCmd, expectOutput, 60, 1)
o.Expect(result).To(o.ContainSubstring(`Secure; SameSite=None`))

g.By("4.0: Add Strict annotation to the edge route, and then ensure the change in haproxy")
setAnnotation(oc, ns, "route/edge35547", "router.openshift.io/cookie-same-site=Strict")
routerPod := getOneRouterPodNameByIC(oc, "default")
ensureHaproxyBlockConfigContains(oc, routerPod, "be_edge_http:"+ns+":edge35547", []string{"SameSite=Strict"})

g.By("5.0: Curl the edge route again, and check set-cookie header, expect getting SameSite=Strict")
result, _ = repeatCmdOnClient(oc, curlCmd, expectOutput, 60, 1)
o.Expect(result).To(o.ContainSubstring(`Secure; SameSite=Strict`))
})

g.It("[OTP] [OCP-35548] cookie-same-site route annotation accepts None Lax or Strict attribute for Reencrypt routes [apigroup:route.openshift.io]", func() {
if !isCanaryRouteAvailable(oc) {
g.Skip("Skip for the ingress canary route could not be available to the outside")
}

var (
buildPruningBaseDir = exutil.FixturePath("testdata", "router")
testPodSvc = filepath.Join(buildPruningBaseDir, "web-server-signed-deploy.yaml")
srvrcInfo = "web-server-deploy"
secsvcName = "service-secure"
)

g.By("1.0: Create two server pods and the service")
ns := oc.Namespace()
createResourceFromWebServer(oc, ns, testPodSvc, srvrcInfo)

g.By("2.0: Create a reencrypt route")
routehost := "reen35548" + ".apps." + getBaseDomain(oc)
createRouteOTP(oc, ns, "reencrypt", "reen35548", secsvcName, []string{"--hostname=" + routehost})
ensureRouteIsAdmittedByIngressController(oc, ns, "reen35548", "default")

g.By("3.0: Curl the reencrypt route, and check set-cookie header, expect getting SameSite=None")
curlCmd := fmt.Sprintf(`curl https://%s -sSkI --connect-timeout 10`, routehost)
expectOutput := []string{"set-cookie:"}
result, _ := repeatCmdOnClient(oc, curlCmd, expectOutput, 60, 1)
o.Expect(result).To(o.ContainSubstring(`Secure; SameSite=None`))

g.By("4.0: Add Lax annotation to the reencrypt route, and then ensure the change in haproxy")
setAnnotation(oc, ns, "route/reen35548", "router.openshift.io/cookie-same-site=Lax")
routerPod := getOneRouterPodNameByIC(oc, "default")
ensureHaproxyBlockConfigContains(oc, routerPod, "be_secure:"+ns+":reen35548", []string{"SameSite=Lax"})

g.By("5.0: Curl the reencrypt route again, and check set-cookie header, expect getting SameSite=Lax")
result, _ = repeatCmdOnClient(oc, curlCmd, expectOutput, 60, 1)
o.Expect(result).To(o.ContainSubstring(`Secure; SameSite=Lax`))

g.By("6.0: Add Strict annotation to the reencrypt route, and then ensure the change in haproxy")
setAnnotation(oc, ns, "route/reen35548", "router.openshift.io/cookie-same-site=Strict")
ensureHaproxyBlockConfigContains(oc, routerPod, "be_secure:"+ns+":reen35548", []string{"SameSite=Strict"})

g.By("7.0: Curl the reencrypt route for the 3rd time, and check set-cookie header, expect getting SameSite=Strict")
result, _ = repeatCmdOnClient(oc, curlCmd, expectOutput, 60, 1)
o.Expect(result).To(o.ContainSubstring(`Secure; SameSite=Strict`))
})
})
Loading