[OCPERT-264][hold]Test OTE migration in monorepo mode with Dockerfile integration#751
[OCPERT-264][hold]Test OTE migration in monorepo mode with Dockerfile integration#751ming1013 wants to merge 3 commits intoopenshift:masterfrom
Conversation
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe pull request introduces a comprehensive test extension framework for OpenShift router e2e testing, including build infrastructure updates (Makefile, Docker multi-stage build), new Go test suites covering network edge components (AWS Load Balancer, DNS, ExternalDNS, Gateway API, IP failover, logging, cookies), AWS utilities for STS cluster integration, and extensive test data fixtures. Dependencies are updated significantly with AWS SDKs and OpenShift modules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (35)
test/e2e/extension/testdata/router/egressfirewall-wildcard.yaml-9-9 (1)
9-9:⚠️ Potential issue | 🟠 MajorAvoid coupling this fixture to a public internet domain
Using
dnsName: "*.google.com"makes test behavior depend on external DNS/egress availability and public IP churn, which can cause flaky e2e runs (especially in restricted or disconnected CI). Prefer a wildcard domain controlled by test infrastructure (or another deterministic internal target) for stable test outcomes.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/router/egressfirewall-wildcard.yaml` at line 9, The fixture is using an external wildcard domain via the dnsName key ("*.google.com"), which couples tests to public DNS/egress; update the dnsName value to a deterministic, test-controlled wildcard (e.g., a local/test domain like "*.example.test" or a CI-provisioned hostname variable) or reference the test harness's configurable domain variable so e2e runs don't depend on external DNS or internet access; ensure any related README/tests use the same variable/name so the fixture remains self-contained and stable.test/e2e/extension/testdata/router/subdomain-routes/alpha-shard-route.yaml-6-12 (1)
6-12:⚠️ Potential issue | 🟠 MajorYAML indentation error:
namespaceis outsidemetadatablock.Same issue as in
route.yaml— thenamespacefield should be indented undermetadata, not at thespeclevel.🔧 Proposed fix
metadata: annotations: labels: shard: alpha name: ${SUBDOMAIN_NAME}-unsecure - namespace: ${NAMESPACE} + namespace: ${NAMESPACE} spec:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/router/subdomain-routes/alpha-shard-route.yaml` around lines 6 - 12, The YAML has the namespace key placed at the top level instead of under metadata; move the namespace entry so it is indented beneath the metadata block (with the existing labels/shard/name entries) rather than alongside spec; update the block containing metadata, name: ${SUBDOMAIN_NAME}-unsecure, and labels (shard: alpha) to include namespace: ${NAMESPACE} nested under metadata so spec remains a separate top-level key.test/e2e/extension/testdata/router/subdomain-routes/ocp51148-route.yaml-6-55 (1)
6-55:⚠️ Potential issue | 🟠 MajorYAML indentation error:
namespaceis outsidemetadatablock in all four routes.Lines 11, 24, 36, and 50 all have
namespaceat the wrong indentation level. Each should be nested under its respectivemetadatablock.🔧 Proposed fix (showing first route as example, apply same pattern to all)
metadata: annotations: labels: name: service-unsecure name: ${SUBDOMAIN_NAME}-unsecure1 - namespace: ${NAMESPACE} + namespace: ${NAMESPACE} spec:Apply the same 2-space indentation increase to
namespaceon lines 24, 36, and 50.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/router/subdomain-routes/ocp51148-route.yaml` around lines 6 - 55, The YAML has namespace keys mis-indented outside each Route's metadata block for the Route resources named ${SUBDOMAIN_NAME}-unsecure1, ${SUBDOMAIN_NAME}-unsecure2, ${SUBDOMAIN_NAME}-unsecure3, and ${SUBDOMAIN_NAME}-unsecure4; fix by moving each namespace key two spaces right so it is nested under its corresponding metadata mapping (i.e., align namespace under metadata for each Route spec), and apply the same indentation correction for all four Route entries.test/e2e/testdata/49802-route.yaml-10-10 (1)
10-10:⚠️ Potential issue | 🟠 MajorMove
namespaceundermetadatafor both Route objectsThe
namespacefield is currently at the top level of eachRouteobject (lines 10, 24), but Kubernetes/OpenShift schemas require it undermetadata.namespace. This placement is schema-invalid and will cause manifest validation to fail.Proposed fix
- apiVersion: route.openshift.io/v1 kind: Route metadata: labels: name: service-unsecure name: hello-pod - namespace: ${NAMESPACE} + namespace: ${NAMESPACE} spec: tls: termination: edge insecureEdgeTerminationPolicy: Redirect @@ - apiVersion: route.openshift.io/v1 kind: Route metadata: labels: name: service-unsecure name: hello-pod-http - namespace: ${NAMESPACE} + namespace: ${NAMESPACE} spec: path: "/test"Also applies to: 24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/testdata/49802-route.yaml` at line 10, The top-level "namespace: ${NAMESPACE}" entries in the Route manifests are invalid; move each "namespace: ${NAMESPACE}" value into the corresponding Route object's metadata block as "metadata.namespace: ${NAMESPACE}" and remove the top-level namespace keys so each Route (Kind: Route) has its namespace defined under metadata; update both occurrences that currently read "namespace: ${NAMESPACE}" to be nested under the Route's "metadata" section.test/e2e/extension/testdata/router/ocp73771-role.yaml-3-5 (1)
3-5:⚠️ Potential issue | 🟠 MajorPin both RBAC resources to
openshift-ingressexplicitly.The Role and RoleBinding lack explicit
metadata.namespace, causing them to be created in whatever namespace the test applies them to (via-nflag). However, the RoleBinding's subject references a ServiceAccount inopenshift-ingress. This creates a cross-namespace mismatch that violates Kubernetes RBAC semantics and can cause permission binding to fail silently or behave unpredictably across different test environments.Proposed fix
apiVersion: rbac.authorization.k8s.io/v1 kind: Role metadata: name: secret-reader + namespace: openshift-ingress rules: - apiGroups: [""] resources: ["secrets"] verbs: ["get", "list", "watch"] resourceNames: ["mytls"] --- apiVersion: rbac.authorization.k8s.io/v1 kind: RoleBinding metadata: name: secret-reader-binding + namespace: openshift-ingress subjects: - kind: ServiceAccount name: router namespace: openshift-ingress roleRef: kind: Role name: secret-reader apiGroup: rbac.authorization.k8s.ioAlso applies to: lines 12-22 (RoleBinding resource)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/router/ocp73771-role.yaml` around lines 3 - 5, The Role named "secret-reader" and its corresponding RoleBinding must be pinned to the openshift-ingress namespace to avoid cross-namespace RBAC mismatches: add metadata.namespace: openshift-ingress to the Role resource (symbol: metadata.name: secret-reader) and to the RoleBinding resource, and verify the RoleBinding.subjects[].namespace matches openshift-ingress and the roleRef references the "secret-reader" Role in that namespace; update both resources so they are explicitly namespaced to openshift-ingress.test/e2e/extension/testdata/router/ingress-destCA.yaml-6-12 (1)
6-12:⚠️ Potential issue | 🟠 Major
namespacefield is misplaced outsidemetadatablock.The
namespacefield at line 11 is at the wrong indentation level. It should be nested undermetadata:alongsidenameandannotations, not at the same level asspec.Proposed fix
metadata: name: ${NAME} + namespace: ${NAMESPACE} annotations: route.openshift.io/destination-ca-certificate-secret: service-secret route.openshift.io/termination: reencrypt - namespace: ${NAMESPACE} spec:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/router/ingress-destCA.yaml` around lines 6 - 12, Move the misplaced namespace field into the metadata block so it is nested alongside name and annotations; update the YAML in this file by indenting the namespace key under metadata (same level as name and annotations) instead of placing it at the same level as spec, ensuring keys metadata, name, annotations, namespace, and spec reflect correct hierarchy.test/e2e/testdata/awslb/podsvc.yaml-7-13 (1)
7-13:⚠️ Potential issue | 🟠 MajorHarden Pod/container
securityContextto match baseline security standards.
Lines 7-13 lack security context definitions that are present in similar test fixtures (e.g.,coreDNS-pod.yaml). Add pod-level seccomp and container-level hardening to prevent policy failures in stricter clusters and establish consistent baseline security across test assets.Proposed patch
spec: + securityContext: + seccompProfile: + type: RuntimeDefault containers: - image: quay.io/openshifttest/nginx-alpine@sha256:cee6930776b92dc1e93b73f9e5965925d49cff3d2e91e1d071c2f0ff72cbca29 name: nginx + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + capabilities: + drop: + - ALL ports: - containerPort: 8080 - containerPort: 8443🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/testdata/awslb/podsvc.yaml` around lines 7 - 13, Add a pod-level securityContext and container-level securityContext for the nginx container to match baseline hardening (similar to coreDNS-pod.yaml): set pod.spec.securityContext.seccompProfile.type to RuntimeDefault (or Legacy if needed), ensure pod.spec.securityContext.runAsNonRoot and runAsUser are set; for the container named "nginx" add securityContext with runAsNonRoot: true, runAsUser (non-zero), allowPrivilegeEscalation: false, privileged: false, readOnlyRootFilesystem: true, and drop all capabilities (e.g., capabilities.drop: ["ALL"]); update the YAML under spec and the nginx container entry to include these fields so the fixture passes stricter cluster policies.test/e2e/extension/testdata/router/awslb/podsvc.yaml-7-13 (1)
7-13:⚠️ Potential issue | 🟠 MajorCorrect the security context placement and configuration to match the existing pattern in test fixtures.
The security hardening goal is valid—other test fixtures like
test-client-pod.yamlinclude hardened security contexts—but the proposed patch has two issues: (1) it placesrunAsNonRootat the container level instead of the spec level as established in similar fixtures, and (2) it introducesreadOnlyRootFilesystemwhich is not part of the existing pattern. Additionally, the root copy (./test/e2e/testdata/awslb/podsvc.yaml) currently lacks this hardening as well, so both files need correction to follow the established pattern:
securityContextwithrunAsNonRootandseccompProfileshould be at the spec level (not container level)- Container-level
securityContextshould haveallowPrivilegeEscalation: falseandcapabilities.drop: [ALL]withoutreadOnlyRootFilesystem🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/router/awslb/podsvc.yaml` around lines 7 - 13, Move the pod securityContext keys to the pod spec level and align container securityContext with the fixture pattern: remove runAsNonRoot and seccompProfile from the container block and add them under the top-level spec.securityContext (e.g., spec.securityContext.runAsNonRoot and spec.securityContext.seccompProfile), and in the container named "nginx" ensure the container-level securityContext only contains allowPrivilegeEscalation: false and capabilities.drop: [ALL]; also remove readOnlyRootFilesystem from the container-level securityContext so both this file and the root fixture follow the same hardening pattern.bindata.mk-5-8 (1)
5-8:⚠️ Potential issue | 🟠 MajorPin
go-bindataversion instead of installing@latestUsing
@latestmakes bindata generation non-reproducible and can break CI unexpectedly when upstream releases change. Pin an explicit version.Proposed fix
TESTDATA_PATH := test/e2e/extension/testdata GOPATH ?= $(shell go env GOPATH) GO_BINDATA := $(GOPATH)/bin/go-bindata +GO_BINDATA_VERSION ?= v3.1.3 $(GO_BINDATA): `@echo` "Installing go-bindata..." - `@GOFLAGS`= go install github.com/go-bindata/go-bindata/v3/go-bindata@latest + `@GOFLAGS`= GOBIN=$(dir $(GO_BINDATA)) go install github.com/go-bindata/go-bindata/v3/go-bindata@$(GO_BINDATA_VERSION)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindata.mk` around lines 5 - 8, The make target $(GO_BINDATA) currently installs go-bindata with `@latest` which breaks reproducibility; update the install command in the $(GO_BINDATA) recipe to pin an explicit tagged release (e.g., replace `@latest` with a specific version string) or use a MAKE variable like BINDATA_VERSION and reference it in the go install invocation so CI always installs the same go-bindata version. Ensure the chosen tag is committed (or the variable defaulted) so builds remain reproducible.go.mod-232-234 (1)
232-234:⚠️ Potential issue | 🟠 MajorSecurity: High-severity vulnerabilities in indirect dependencies require attention.
Verified vulnerabilities:
github.com/opencontainers/runcv1.2.5: Three container escape issues (CVE-2025-31133, CVE-2025-52565, CVE-2025-52881) — fixed in v1.2.8+github.com/opencontainers/selinuxv1.11.1: Race condition enabling procfs writes and container escape (CVE-2025-52881) — fixed in v1.13.0+go.opentelemetry.io/otel/sdkv1.37.0: PATH hijacking leading to arbitrary code execution on macOS/Darwin (CVE-2026-24051) — fixed in v1.40.0+As indirect dependencies, these may be pulled transitively. Upgrade to patched versions and verify whether the vulnerable code paths are exercised in your usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 232 - 234, The go.mod currently references vulnerable indirect modules; update the transitive versions to safe releases by bumping github.com/opencontainers/runc to v1.2.8 or later, github.com/opencontainers/selinux to v1.13.0 or later, and go.opentelemetry.io/otel/sdk to v1.40.0 or later (e.g., run `go get github.com/opencontainers/runc@v1.2.8 github.com/opencontainers/selinux@v1.13.0 go.opentelemetry.io/otel/sdk@v1.40.0` and then `go mod tidy`), verify the upgraded versions are present in go.sum and the build passes, and re-run your dependency/vulnerability scanner to ensure the CVEs (runc CVE-2025-31133/CVE-2025-52565/CVE-2025-52881 and otel CVE-2026-24051) are no longer reported.test/e2e/extension/gatewayapi.go-70-78 (1)
70-78:⚠️ Potential issue | 🟠 MajorPoll the exact condition instead of any
Truesubstring.Both waits ignore CLI errors and succeed on the first
Truethey find in the status blob. For the Gateway check, that can return beforeProgrammed=True.Suggested fix
- waitErr := wait.PollImmediate(2*time.Second, 300*time.Second, func() (bool, error) { - output, _ := oc.AsAdmin().WithoutNamespace().Run("get").Args("gatewayclass", gwcName, "-ojsonpath={.status.conditions}").Output() - if strings.Contains(output, "True") { - e2e.Logf("the gatewayclass is accepted") - return true, nil - } - e2e.Logf("Waiting for the GatewayClass to be accepted") - return false, nil - }) + waitErr := wait.PollImmediate(2*time.Second, 300*time.Second, func() (bool, error) { + output, err := oc.AsAdmin().WithoutNamespace().Run("get").Args( + "gatewayclass", gwcName, + `-o=jsonpath={.status.conditions[?(@.type=="Accepted")].status}`, + ).Output() + if err != nil { + return false, err + } + if output == "True" { + e2e.Logf("the gatewayclass is accepted") + return true, nil + } + e2e.Logf("Waiting for the GatewayClass to be accepted") + return false, nil + })Apply the same pattern to the Gateway poll below, but target
Programmed.Also applies to: 87-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/gatewayapi.go` around lines 70 - 78, The current wait.PollImmediate calls use a substring check for "True" and ignore CLI errors; update both waits (the GatewayClass check using oc.AsAdmin().WithoutNamespace().Run("get").Args("gatewayclass", gwcName, "-ojsonpath={.status.conditions}").Output() and the Gateway check at lines 87-95) to parse the JSON status or use a jsonpath that targets the specific condition (e.g., check for the condition with type "Programmed" and status "True"), return any CLI errors instead of discarding them, and only return true when the exact condition equals "True".test/e2e/extension/ipfailover.go-315-322 (1)
315-322:⚠️ Potential issue | 🟠 MajorDon't capture
futurePrimaryPodbefore the replacement pod exists.This pod list is read immediately after deleting
new_master. On a slow rollout,futurePrimaryPod[0]can still be the terminating pod or the slice can be empty, and the fixed 125s sleep just hides the race.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/ipfailover.go` around lines 315 - 322, The test currently captures futurePrimaryPod from getPodListByLabel immediately after deleting new_master and then sleeps a fixed 125s, which can race on slow rollouts; replace the fixed sleep and pre-capture with a short polling loop that repeatedly calls getPodListByLabel(oc, ns, "ipfailover=hello-openshift") and waits until you see a pod that is not the terminating/deleted pod (i.e. different from new_backup/new_master) and the slice returned by slicingElement(new_backup, latestpods) is non-empty, then pass that stable pod (futurePrimaryPod[0]) into waitForPrimaryPod(oc, ns, futurePrimaryPod[0], virtualIP). Ensure the loop times out after a reasonable duration and fails the test if no replacement pod appears.test/e2e/gatewayapi.go-69-77 (1)
69-77:⚠️ Potential issue | 🟠 MajorPoll the exact condition instead of any
Truesubstring.Both waits ignore CLI errors and succeed on the first
Truethey find in the status blob. For the Gateway check, that can return beforeProgrammed=True.Suggested fix
- waitErr := wait.PollImmediate(2*time.Second, 300*time.Second, func() (bool, error) { - output, _ := oc.AsAdmin().WithoutNamespace().Run("get").Args("gatewayclass", gwcName, "-ojsonpath={.status.conditions}").Output() - if strings.Contains(output, "True") { - e2e.Logf("the gatewayclass is accepted") - return true, nil - } - e2e.Logf("Waiting for the GatewayClass to be accepted") - return false, nil - }) + waitErr := wait.PollImmediate(2*time.Second, 300*time.Second, func() (bool, error) { + output, err := oc.AsAdmin().WithoutNamespace().Run("get").Args( + "gatewayclass", gwcName, + `-o=jsonpath={.status.conditions[?(@.type=="Accepted")].status}`, + ).Output() + if err != nil { + return false, err + } + if output == "True" { + e2e.Logf("the gatewayclass is accepted") + return true, nil + } + e2e.Logf("Waiting for the GatewayClass to be accepted") + return false, nil + })Apply the same pattern to the Gateway poll below, but target
Programmed.Also applies to: 86-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/gatewayapi.go` around lines 69 - 77, The wait loops currently check for any "True" substring in the status JSON and ignore CLI errors; update both the GatewayClass poll (the wait.PollImmediate call that runs oc.AsAdmin().WithoutNamespace().Run("get").Args("gatewayclass", gwcName, ...).Output()) and the Gateway poll to parse the specific condition field and match the exact condition name (e.g., "Programmed") instead of any "True" token, and propagate/handle the CLI error from Output() instead of discarding it; specifically, extract the status.conditions array or use a jsonpath that targets the named condition for "Programmed" and only return true when that condition's status == "True", returning the CLI error if Output() fails.test/e2e/ipfailover.go-314-321 (1)
314-321:⚠️ Potential issue | 🟠 MajorDon't capture
futurePrimaryPodbefore the replacement pod exists.This pod list is read immediately after deleting
new_master. On a slow rollout,futurePrimaryPod[0]can still be the terminating pod or the slice can be empty, and the fixed 125s sleep just hides the race.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/ipfailover.go` around lines 314 - 321, The test captures futurePrimaryPod too early which can pick up a terminating pod or be empty; instead poll for a non-terminating replacement pod after deleting new_master by repeatedly calling getPodListByLabel(oc, ns, "ipfailover=hello-openshift") and selecting a pod different from the old master (use slicingElement/new_backup logic) until a new ready/non-terminating pod appears or a timeout elapses, then pass that pod to waitForPrimaryPod(oc, ns, futurePrimaryPod[0], virtualIP); remove the fixed time.Sleep(125 * time.Second) and replace it with this polling loop to avoid the race.test/e2e/route-weight.go-557-558 (1)
557-558:⚠️ Potential issue | 🟠 Major
srvPod2Nameis never checked in this precondition.Both grep patterns use
srvPod3Nametwice, so pod 2 can be missing or have the wrong default weight and this assertion still passes.Suggested fix
- selectedSrvNum := fmt.Sprintf("cat haproxy.config | grep -E \"server pod:ingress-canary|server pod:%s|server pod:%s|server pod:%s\"| wc -l", srvPod1Name, srvPod3Name, srvPod3Name) - selectedWeight1Num := fmt.Sprintf("cat haproxy.config | grep -E \"server pod:ingress-canary|server pod:%s|server pod:%s|server pod:%s\"| grep \"weight 1\" |wc -l", srvPod1Name, srvPod3Name, srvPod3Name) + selectedSrvNum := fmt.Sprintf("cat haproxy.config | grep -E \"server pod:ingress-canary|server pod:%s|server pod:%s|server pod:%s\"| wc -l", srvPod1Name, srvPod2Name, srvPod3Name) + selectedWeight1Num := fmt.Sprintf("cat haproxy.config | grep -E \"server pod:ingress-canary|server pod:%s|server pod:%s|server pod:%s\"| grep \"weight 1\" |wc -l", srvPod1Name, srvPod2Name, srvPod3Name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/route-weight.go` around lines 557 - 558, The grep patterns building selectedSrvNum and selectedWeight1Num mistakenly include srvPod3Name twice and never include srvPod2Name; update the fmt.Sprintf calls that define selectedSrvNum and selectedWeight1Num to include srvPod1Name, srvPod2Name, and srvPod3Name (replace the duplicate srvPod3Name occurrences with srvPod2Name) so the precondition actually checks all three pods and their weights (refer to the variables selectedSrvNum, selectedWeight1Num, srvPod1Name, srvPod2Name, srvPod3Name in route-weight.go).test/e2e/dns-operator.go-63-69 (1)
63-69:⚠️ Potential issue | 🟠 MajorFail the cleanup if DNS/CVO recovery does not succeed.
This spec deliberately scales CVO and the DNS operator to zero. Swallowing restore failures here can leave the cluster degraded and poison the rest of the suite.
Suggested fix
defer func() { exutil.By("Recover the CVO and DNS") - oc.AsAdmin().WithoutNamespace().Run("scale").Args("deployment/cluster-version-operator", "--replicas=1", "-n", "openshift-cluster-version").Output() - oc.AsAdmin().WithoutNamespace().Run("scale").Args("deployment/dns-operator", "--replicas=1", "-n", "openshift-dns-operator").Output() + _, err := oc.AsAdmin().WithoutNamespace().Run("scale").Args("deployment/cluster-version-operator", "--replicas=1", "-n", "openshift-cluster-version").Output() + o.Expect(err).NotTo(o.HaveOccurred()) + _, err = oc.AsAdmin().WithoutNamespace().Run("scale").Args("deployment/dns-operator", "--replicas=1", "-n", "openshift-dns-operator").Output() + o.Expect(err).NotTo(o.HaveOccurred()) // if the dns-default svc didn't came up in given time, dns operator restoration will help deleteDnsOperatorToRestore(oc) }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/dns-operator.go` around lines 63 - 69, The deferred cleanup currently swallows failures when scaling back the cluster-version-operator and dns-operator and when calling deleteDnsOperatorToRestore(oc); update the defer to capture and check errors from oc.AsAdmin().WithoutNamespace().Run("scale").Args(...).Output() for both "deployment/cluster-version-operator" and "deployment/dns-operator" and also check the result of deleteDnsOperatorToRestore(oc), and if any of these operations fail, cause the test to fail (e.g., call the test failure helper used in this file such as t.Fatalf or Ginkgo's Fail) so cleanup failures are not silently ignored. Ensure you reference the existing calls to oc.AsAdmin().WithoutNamespace().Run(...).Output() and deleteDnsOperatorToRestore(oc) when implementing the checks.test/e2e/extension/ipfailover.go-64-66 (1)
64-66:⚠️ Potential issue | 🟠 MajorUse Ginkgo's
g.Serialdecorator instead of text tokens to ensure these specs actually serialize.This file uses
[Serial]only as text in spec titles. Without Ginkgo's nativeg.Serialdecorator, the OTE runner may execute these specs in parallel, causing race conditions when they mutate shared ipfailover resources.Suggested change
-g.It("Author:hongli-NonHyperShiftHOST-ConnectedOnly-Critical-41025-support to deploy ipfailover [Serial]", func() { +g.It("Author:hongli-NonHyperShiftHOST-ConnectedOnly-Critical-41025-support to deploy ipfailover [Serial]", g.Serial, func() {Apply the same pattern to the four specs using
[Serial]in this file (lines 66, 144, 210, 327).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/ipfailover.go` around lines 64 - 66, Several specs in ipfailover.go use the literal "[Serial]" in their g.It titles but lack Ginkgo's g.Serial decorator; find each g.It call whose title contains "[Serial]" (the four specs currently using that token) and add g.Serial as an option argument (e.g. change g.It("... [Serial]", func() {...}) to g.It("... (remove [Serial] token)", g.Serial, func() {...})), leaving the test body unchanged so Ginkgo will actually run them serially.test/e2e/ipfailover.go-63-66 (1)
63-66: 🛠️ Refactor suggestion | 🟠 MajorAdd
g.Serialdecorator to these conflicting specs.These four specs have comments stating they must be serial to avoid conflicts, but they rely only on
[Serial]text in the title. Ginkgo v2 does not enforce serialization based on title tokens—you must use theg.Serialdecorator.Suggested changes
-g.It("Author:hongli-NonHyperShiftHOST-ConnectedOnly-Critical-41025-support to deploy ipfailover [Serial]", func() { +g.It("Author:hongli-NonHyperShiftHOST-ConnectedOnly-Critical-41025-support to deploy ipfailover [Serial]", g.Serial, func() {Apply to all four specs at lines 65, 143, 209, and 326.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/ipfailover.go` around lines 63 - 66, The four test specs in test/e2e/ipfailover.go currently use g.It(...) with “[Serial]” in their titles (e.g., the Author:hongli-NonHyperShiftHOST-ConnectedOnly-Critical-41025-support to deploy ipfailover spec) but rely on title tokens for serialization; update each of those g.It calls (the ones at the four locations noted) to use the Ginkgo v2 serial decorator by prefixing them with g.Serial so they become g.Serial(..., func() { ... }) (i.e., replace g.It with g.Serial for those specific tests to ensure proper serialization).test/e2e/extension/testdata/fixtures.go-91-99 (1)
91-99:⚠️ Potential issue | 🟠 Major
ListFixtures()is filtering out the actual embedded asset names.The extension callers already resolve fixtures from
router/..., so filteringAssetNames()totestdata/makes this new public API return an empty slice with the current bindata layout.Suggested fix
func ListFixtures() []string { names := AssetNames() fixtures := make([]string, 0, len(names)) for _, name := range names { - if strings.HasPrefix(name, "testdata/") { - fixtures = append(fixtures, strings.TrimPrefix(name, "testdata/")) - } + fixtures = append(fixtures, strings.TrimPrefix(name, "testdata/")) } sort.Strings(fixtures) return fixtures }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/fixtures.go` around lines 91 - 99, ListFixtures currently filters AssetNames() by the "testdata/" prefix which causes it to return an empty slice with the current bindata layout; change ListFixtures (the function using AssetNames()) to stop filtering by "testdata/" and instead include all AssetNames (or simply return/sort the full AssetNames slice) so callers that expect router/... names receive the embedded asset names unchanged.test/e2e/extension/route-hsts.go-157-159 (1)
157-159:⚠️ Potential issue | 🟠 MajorAdd
g.Serialdecorator to all HSTS policy specs that modify cluster-global configuration.These specs have
[Serial]in their titles but lack Ginkgo's nativeg.Serialdecorator. Under standard Ginkgo v2, title text does not enforce serialization—the specs will run in parallel and race on the cluster-globalrequiredHSTSPoliciesand ingresscontroller state.Six specs need this fix: lines 159, 224, 281, 346, 403, and 551.
Example fix
-g.It("Author:aiyengar-NonHyperShiftHOST-Critical-43474-The includeSubDomainsPolicy parameter can configure subdomain policy to inherit the HSTS policy of parent domain [Serial]", func() { +g.It("Author:aiyengar-NonHyperShiftHOST-Critical-43474-The includeSubDomainsPolicy parameter can configure subdomain policy to inherit the HSTS policy of parent domain [Serial]", g.Serial, func() {Apply to all
[Serial]specs in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/route-hsts.go` around lines 157 - 159, Several HSTS tests that modify cluster-global state use titles with "[Serial]" but are missing the Ginkgo g.Serial decorator; locate the g.It blocks for the HSTS specs (e.g., the test whose description starts "Author:aiyengar-NonHyperShiftHOST-Critical-43474-The includeSubDomainsPolicy..." and the other five specs at the indicated locations) and prepend g.Serial() to each spec definition (i.e., change the g.It(...) invocation to use the g.Serial decorator so the spec is declared with g.Serial(..., func() {...}) or wrap the spec with g.Serial where your test harness expects it) so these tests run serially and no longer race on cluster-global requiredHSTSPolicies/ingresscontroller state.test/e2e/cloud_load_balancer.go-86-88 (1)
86-88:⚠️ Potential issue | 🟠 MajorCopy fixture YAMLs to a temp file before rewriting them.
These specs mutate
ingresscontroller-external.yaml/ingresscontroller-clb.yamlin place after resolving them throughFixturePath(...). Under the new extracted-fixture flow, that makes the suite order-dependent: if an NLB case runs before a CLB case, later specs—and parallel Ginkgo nodes—can see the already-mutated file.Representative fix
- customTemp := filepath.Join(buildPruningBaseDir, "ingresscontroller-clb.yaml") - sedCmd := fmt.Sprintf(`sed -i'' -e 's|Classic|NLB|g' %s`, customTemp) - _, err := exec.Command("bash", "-c", sedCmd).Output() - o.Expect(err).NotTo(o.HaveOccurred()) + source := filepath.Join(buildPruningBaseDir, "ingresscontroller-clb.yaml") + customTemp := filepath.Join(g.GinkgoT().TempDir(), filepath.Base(source)) + data, err := os.ReadFile(source) + o.Expect(err).NotTo(o.HaveOccurred()) + data = []byte(strings.ReplaceAll(string(data), "Classic", "NLB")) + err = os.WriteFile(customTemp, data, 0600) + o.Expect(err).NotTo(o.HaveOccurred())A small helper that copies the source fixture into
g.GinkgoT().TempDir()before applying the replacement would fix all four sites.Also applies to: 312-314, 374-376, 461-463
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/cloud_load_balancer.go` around lines 86 - 88, The test mutates fixture files in place via the sed command (see sedCmd / exec.Command("bash", "-c", sedCmd) using customTemp), making tests order-dependent; instead, copy the original fixture resolved by FixturePath(...) into a fresh file in g.GinkgoT().TempDir() (e.g., read the source with os.ReadFile and write to a new temp path or use exec.Command("cp", ...)), assign that temp path to customTemp, then run the sed replacement against that temp copy so the original fixture is not modified; apply the same pattern to the other occurrences (lines referenced: 312-314, 374-376, 461-463).test/e2e/testdata/httpbin-deploy.yaml-20-28 (1)
20-28:⚠️ Potential issue | 🟠 MajorHarden container security context to avoid restricted-policy failures.
Both containers run without explicit hardening (
allowPrivilegeEscalation/ non-root posture). In restricted clusters this fixture may be rejected, and it weakens baseline security.Proposed hardening patch
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - name: httpbin-http image: quay.io/openshifttest/httpbin@sha256:cc44fbd857f4148d8aad8359acc03efa719517e01d390b152e4f3830ad871c9f + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL ports: - containerPort: 8080 - name: httpbin-https image: quay.io/openshifttest/httpbin@sha256:f57f4e682e05bcdadb103c93ae5ab9be166f79bcbbccaf45d92a2cad18da8d64 + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL ports: - containerPort: 8443🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/testdata/httpbin-deploy.yaml` around lines 20 - 28, Add a hardened securityContext to both container specs (httpbin-http and httpbin-https) to comply with restricted policies: set allowPrivilegeEscalation: false, runAsNonRoot: true, readOnlyRootFilesystem: true, and drop all capabilities (capabilities.drop: ["ALL"]); ensure any required fsGroup/runAsUser are set at the Pod or container level if needed by the image. Locate the two container entries named httpbin-http and httpbin-https in the Deployment manifest and add the securityContext block under each container to enforce non-root, no privilege escalation, read-only root filesystem, and dropped capabilities.test/e2e/extension/testdata/router/awslb/subscription-src-qe.yaml-7-10 (1)
7-10:⚠️ Potential issue | 🟠 MajorAvoid a moving AWS Load Balancer Operator target in e2e fixtures.
The subscription on lines 7–10 uses
installPlanApproval: Automaticwith thestable-v1channel, which resolves whatever version is current when the test runs. This breaks test reproducibility and bisectability. Pin the installed bundle explicitly by addingstartingCSVto the subscription spec or by pinning the catalog source to a specific image digest instead of following the channel head.Current subscription configuration
channel: stable-v1 installPlanApproval: Automatic name: aws-load-balancer-operator source: qe-app-registry🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/router/awslb/subscription-src-qe.yaml` around lines 7 - 10, The subscription currently uses a moving target by setting channel: stable-v1 and installPlanApproval: Automatic for the aws-load-balancer-operator (name: aws-load-balancer-operator, source: qe-app-registry); fix this by pinning the bundle—either add a startingCSV field in the subscription spec with the exact CSV name to lock the installed version, or pin the catalog (qe-app-registry) to a specific image digest so the channel head is stable; update the subscription spec to include startingCSV (or replace the source reference with a digest-pinned catalog) and keep installPlanApproval as needed.test/e2e/extension/externaldns.go-69-70 (1)
69-70:⚠️ Potential issue | 🟠 MajorDon't edit
FixturePath()assets in place.
FixturePath()returns a cached extracted path (test/e2e/extension/testdata/fixtures.go:23-54). After thesesed/updateFilebySedCmdcalls, later specs and retries reuse already-mutated YAML, so placeholders likebasedomainandprivatezonedisappear and the suite becomes order-dependent. Copy each fixture to a per-spec temp file before rewriting it.Also applies to: 243-244, 323-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/externaldns.go` around lines 69 - 70, The tests are mutating shared fixture files returned by FixturePath(), causing order-dependent failures; before calling sedCmd/exec.Command or updateFilebySedCmd on fixtures like sampleAWS (and the other occurrences at the later calls), copy the fixture returned by FixturePath() to a per-spec temporary file (e.g., os.CreateTemp/os.MkdirTemp + io.Copy) and run the sed/update on that temp copy, then use the temp file path in the rest of the spec; update the code around sedCmd/exec.Command and the helper updateFilebySedCmd invocations to operate on the temp copy rather than the original FixturePath() asset so fixtures remain immutable across specs.test/e2e/extension/tuning.go-594-600 (1)
594-600:⚠️ Potential issue | 🟠 MajorThis null-patch check isn't observing the router you patched.
Line 597 reads
deployment.apps/router-default, even though the patch targets the custom ingresscontroller. Line 600 also usesContainSubstringon generation numbers, so"10"would satisfy"1". That makes the no-op assertion effectively meaningless.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/tuning.go` around lines 594 - 600, The test is patching the custom ingresscontroller but checks generation on "deployment.apps/router-default" and then uses a substring assertion that allows false positives; change the resource checked by getByJsonPath to the actual ingresscontroller resource (use ingctrl.name or ingctrlResource/ingctrl.namespace as appropriate) and keep jpath = "{.status.observedGeneration}", then after patchResourceAsAdmin(...) fetch observedGen1 and observedGen2 from that same ingresscontroller and assert they are equal using an exact comparison (or convert to integers and assert equality/unchanged) instead of using ContainSubstring so the no-op/null-patch is validated correctly.test/e2e/extension/testdata/router/httpbin-deploy.yaml-20-28 (1)
20-28:⚠️ Potential issue | 🟠 MajorLock down the default httpbin containers.
This fixture relies on permissive defaults. Without an explicit
securityContext, both containers keep privilege escalation enabled by default and may be rejected under restricted admission. That is unnecessary for the non-privileged httpbin path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/testdata/router/httpbin-deploy.yaml` around lines 20 - 28, The httpbin container specs (containers named "httpbin-http" and "httpbin-https") are using permissive defaults; add a securityContext for each container to explicitly lock them down: set allowPrivilegeEscalation: false, runAsNonRoot: true (or an appropriate non-root uid if required), readOnlyRootFilesystem: true, and drop all capabilities (e.g., capabilities.drop: ["ALL"]); apply these fields under each container's spec so the pods will be accepted by restricted admission without changing runtime behavior.test/e2e/extension/tuning.go-610-621 (1)
610-621:⚠️ Potential issue | 🟠 MajorPatch value and expected
maxconndiverge.Line 610 says
50000, Line 611 patches500000, Line 618 expects500000in env, and Line 621 still expects50000in haproxy. This can't all be true, and it contradicts the earlier maxConnections coverage in this file.Proposed fix
- maxConnections = "500000" + maxConnections = "50000" patchResourceAsAdmin(oc, ingctrl.namespace, ingctrlResource, "{\"spec\": {\"tuningOptions\": {\"maxConnections\": "+maxConnections+"}}}") ensureRouterDeployGenerationIs(oc, ingctrl.name, "2") @@ - ensureHaproxyBlockConfigContains(oc, podname, "maxconn", []string{"maxconn 50000"}) + ensureHaproxyBlockConfigContains(oc, podname, "maxconn", []string{"maxconn " + maxConnections})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/tuning.go` around lines 610 - 621, The test mixes two different values for max connections (50000 vs 500000); make them consistent by using the intended value (use "50000" to match earlier coverage): set maxConnections = "50000", update the JSON passed to patchResourceAsAdmin to use that variable, keep the readRouterPodEnv expectation using "ROUTER_MAX_CONNECTIONS="+maxConnections, and change the ensureHaproxyBlockConfigContains call to expect "maxconn 50000" so both the env and haproxy checks align with patchResourceAsAdmin, targeting the variables and functions maxConnections, patchResourceAsAdmin, readRouterPodEnv, and ensureHaproxyBlockConfigContains.test/e2e/extension/awslb-operator.go-116-121 (1)
116-121:⚠️ Potential issue | 🟠 MajorCount discovered subnets, not characters.
Line 120 uses
len(internalSubnets) > 1, butinternalSubnetsis raw jsonpath output.[]and[subnet-123]both satisfy that condition, so the test can wait for ALB provisioning even when fewer than two subnets were discovered.Proposed fix
- internalSubnets, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("awsloadbalancercontroller/cluster", "-ojsonpath={.status.subnets.internal}").Output() + internalSubnets, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("awsloadbalancercontroller/cluster", "-ojsonpath={.status.subnets.internal[*]}").Output() o.Expect(err).NotTo(o.HaveOccurred()) e2e.Logf("The discovered subnets are: %v", internalSubnets) - if len(internalSubnets) > 1 { + if len(strings.Fields(internalSubnets)) >= 2 { waitForLoadBalancerProvision(oc, oc.Namespace(), "ingress-test") } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/awslb-operator.go` around lines 116 - 121, The code checks len(internalSubnets) on raw jsonpath output (internalSubnets) which counts characters, not items; instead parse the output as JSON into a slice and check its length before calling waitForLoadBalancerProvision. Concretely, after obtaining internalSubnets from oc.AsAdmin().WithoutNamespace().Run(...).Output(), unmarshal that string into a []string (or use a JSON decoder) and replace the condition len(internalSubnets) > 1 with len(parsedSubnets) > 1; keep references to internalSubnets and waitForLoadBalancerProvision so the conditional still gates calling waitForLoadBalancerProvision(oc, oc.Namespace(), "ingress-test").test/e2e/route-hsts.go-216-216 (1)
216-216:⚠️ Potential issue | 🟠 MajorRemove shell quoting from these
annotateargs.
Run(...).Args(...)forwards literal argv items—it does not invoke a shell. The single quotes become part of the annotation value, so these cases test'max-age=...'instead ofmax-age=...and hit the wrong validation path.Example fix
- msg2, err := oc.Run("annotate").WithoutNamespace().Args("route/route-edge", "haproxy.router.openshift.io/hsts_header='max-age=50000'", "--overwrite").Output() + msg2, err := oc.Run("annotate").WithoutNamespace().Args("route/route-edge", "haproxy.router.openshift.io/hsts_header=max-age=50000", "--overwrite").Output()Also applies to: 338, 449–454, 537–542.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/route-hsts.go` at line 216, The annotate call is passing shell-quoted values as a single argv element, so the annotation value includes literal single quotes; update calls like oc.Run("annotate").WithoutNamespace().Args("route/route-edge", "haproxy.router.openshift.io/hsts_header='max-age=50000'", "--overwrite").Output() to remove the shell quotes and pass the annotation as haproxy.router.openshift.io/hsts_header=max-age=50000 (and similarly for the other annotate invocations referenced around lines 338, 449–454, 537–542) so Args receives unquoted key=value strings.test/e2e/externaldns.go-68-69 (1)
68-69:⚠️ Potential issue | 🟠 MajorStop rewriting shared fixture files.
These manifests are reusable inputs. Editing them in place with
sedleaks cluster-specific values into later retries/specs and makes the suite order-dependent. Create a temp copy per test before substitution.Also applies to: 242-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/externaldns.go` around lines 68 - 69, The test currently modifies the shared fixture file in place by building sedCmd and running exec.Command("bash", "-c", sedCmd) against sampleAWS (see sedCmd and sampleAWS in test/e2e/externaldns.go), which leaks state; instead copy the fixture to a temp file per test, perform substitutions on that temp copy, and use the temp path in subsequent operations; update the logic around the sedCmd/exec.Command invocation to operate on the temp file and ensure the same change is applied where sampleAWS is edited again (lines referenced near 242-243) so the original fixture remains untouched and each test gets an isolated file copy.test/e2e/extension/cloud_load_balancer.go-87-89 (1)
87-89:⚠️ Potential issue | 🟠 MajorThese
sededits make the ingresscontroller fixtures order-dependent.
ingresscontroller-external.yamlandingresscontroller-clb.yamlare shared extracted fixtures. Mutating them from External→Internal and Classic→NLB in place means later specs can start from the wrong baseline depending on execution order.Also applies to: 313-315, 375-377, 462-464
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/cloud_load_balancer.go` around lines 87 - 89, The tests mutate shared fixture files in-place via sed (see sedCmd, exec.Command usage and customTemp), making test order-dependent; fix by avoiding in-place edits—read the original fixture (e.g., ingresscontroller-external.yaml / ingresscontroller-clb.yaml) into memory, perform string replacements (External→Internal, Classic→NLB) on the copied content, write the result to a new uniquely named temp file per test (do not overwrite the source fixture), and update the code that passes customTemp to use that temp file so each spec works from a fresh, non-shared baseline.test/e2e/extension/ingress.go-65-66 (1)
65-66:⚠️ Potential issue | 🟠 MajorUse a temp copy for the ingress template.
testdata.FixturePath("router")returns a shared extracted asset. Replacing placeholders directly iningress-resource.yamlmutates the baseline file for retries and any later spec that reuses it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/ingress.go` around lines 65 - 66, The test is mutating the shared fixture returned by testdata.FixturePath("router") by running sed directly on ingress-resource.yaml; instead, create a temporary copy of the fixture (e.g., read the fixture file at testIngress, write its contents to a temp file via os.CreateTemp/os.WriteFile) and run the sedCmd (or perform replacements in-memory and write the temp file) against that temp file so the original ingress-resource.yaml remains unchanged; update references from testIngress to the temp file path when invoking exec.Command("bash", "-c", sedCmd) or applying the manifest.test/e2e/extension/dns.go-1065-1065 (1)
1065-1065:⚠️ Potential issue | 🟠 MajorDon't mutate
egressfirewall-wildcard.yamlin place.The same fixture is already loaded earlier in this file. Rewriting it to
www.amazon.comhere makes later specs and reruns inherit a modified manifest instead of the baseline wildcard rule.Also applies to: 1111-1111, 1172-1175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/extension/dns.go` at line 1065, The test is mutating the fixture file referenced by egressFirewall (egressfirewall-wildcard.yaml) which breaks later tests; instead of writing changes back to that path, load the original fixture and write the modified manifest to a new temporary file or separate variable (e.g., egressFirewallModified) before using it in the spec. Concretely: keep egressFirewall pointing to the original fixture, create a copy in a temp directory or an in-memory modified manifest, apply the www.amazon.com change to that copy, and use the copied path/variable in the existing apply/load calls so the baseline wildcard file remains unchanged (also apply the same change for the other occurrences flagged around the block that mutates fixtures).test/e2e/awslb-operator.go-116-124 (1)
116-124:⚠️ Potential issue | 🟠 MajorCount discovered subnets instead of string bytes.
Line 119 checks
len(internalSubnets) > 1, butinternalSubnetsis the raw jsonpath output string. A single subnet like[subnet-123]still has length> 1, so this can wait for ALB provisioning on clusters that do not meet the>= 2subnet requirement.Suggested fix
- internalSubnets, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("awsloadbalancercontroller/cluster", "-ojsonpath={.status.subnets.internal}").Output() + internalSubnetsRaw, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("awsloadbalancercontroller/cluster", "-ojsonpath={.status.subnets.internal}").Output() o.Expect(err).NotTo(o.HaveOccurred()) - e2e.Logf("The discovered subnets are: %v", internalSubnets) - if len(internalSubnets) > 1 { + e2e.Logf("The discovered subnets are: %v", internalSubnetsRaw) + internalSubnetCount := len(strings.Fields(strings.Trim(internalSubnetsRaw, "[]"))) + if internalSubnetCount >= 2 { waitForLoadBalancerProvision(oc, oc.Namespace(), "ingress-test") } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/awslb-operator.go` around lines 116 - 124, The code currently checks len(internalSubnets) on the raw jsonpath string; instead parse the json output into a slice and check the slice length. After calling oc.AsAdmin().WithoutNamespace().Run(...).Output() for the awsloadbalancercontroller status, unmarshal the returned JSON string (internalSubnets) into a []string (or split a JSON array) and then use len(subnets) to decide whether to call waitForLoadBalancerProvision(oc, oc.Namespace(), "ingress-test") or to run oc.Run("describe").Args("ingress", "ingress-test").Output() and log the events; reference the variables/internalSubnets, the oc.AsAdmin().WithoutNamespace().Run(...).Args(...) call, and the waitForLoadBalancerProvision function to locate the change.test/e2e/aws_util.go-235-247 (1)
235-247:⚠️ Potential issue | 🟠 MajorDon't proceed with an unchecked
infraIDwhen touching account-global IAM roles.Lines 235 and 253 discard the error from
exutil.GetInfraID(oc)even thoughinfraIDis used directly to construct role names likeinfraID + "-albo-role"andinfraID + "-albc-role". If that lookup fails,infraIDbecomes an empty string, resulting in generic role names that operate on the shared AWS account. The same issue exists intest/e2e/extension/aws_util.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/aws_util.go` around lines 235 - 247, The code calls exutil.GetInfraID(oc) and ignores its error, which can leave infraID empty and cause account-global IAM roles to be created; update the call to check the returned error and abort the test or return the error if infraID is empty or the call fails (e.g., fail the test with o.Expect(err).NotTo(o.HaveOccurred()) or propagate the error) before using infraID in createAlboRolePolicy and createAlbcRolePolicy, ensuring you do not construct role names when infraID is missing.
| exutil.By("Ensure the dnsrecord with new LB IP/hostname are published") | ||
| publishStatus, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress-operator", "dnsrecord", dnsRecordName, `-o=jsonpath={.status.zones[*].conditions[?(@.type == "Published")].status})`).Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(publishStatus).NotTo(o.ContainSubstring("False")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
path = Path("test/e2e/cloud_load_balancer.go")
text = path.read_text()
needle = '-o=jsonpath={.status.zones[*].conditions[?(@.type == "Published")].status})'
print("malformed_jsonpath_present =", needle in text)
for i, line in enumerate(text.splitlines(), 1):
if 'dnsrecord", dnsRecordName' in line:
print(f"{i}: {line}")
PYRepository: openshift/router
Length of output: 322
🏁 Script executed:
#!/bin/bash
# Get the exact lines 129-135 to see full context
sed -n '129,135p' test/e2e/cloud_load_balancer.goRepository: openshift/router
Length of output: 493
🏁 Script executed:
#!/bin/bash
# Check imports in the file to verify strings package availability
head -50 test/e2e/cloud_load_balancer.go | grep -E "^import|^ \"(strings|github)"Repository: openshift/router
Length of output: 79
Fix the malformed jsonpath and require a published value.
Line 130 has an extra ) inside the -o=jsonpath=... argument, which malforms the JSONPath syntax and can cause oc get to fail. Additionally, NotTo(ContainSubstring("False")) passes on empty output, which is a logic flaw—the check must verify a non-empty published status.
Suggested fix
- publishStatus, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress-operator", "dnsrecord", dnsRecordName, `-o=jsonpath={.status.zones[*].conditions[?(@.type == "Published")].status})`).Output()
+ publishStatus, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress-operator", "dnsrecord", dnsRecordName, `-o=jsonpath={.status.zones[*].conditions[?(@.type == "Published")].status}`).Output()
o.Expect(err).NotTo(o.HaveOccurred())
+ o.Expect(strings.TrimSpace(publishStatus)).NotTo(o.BeEmpty())
o.Expect(publishStatus).NotTo(o.ContainSubstring("False"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exutil.By("Ensure the dnsrecord with new LB IP/hostname are published") | |
| publishStatus, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress-operator", "dnsrecord", dnsRecordName, `-o=jsonpath={.status.zones[*].conditions[?(@.type == "Published")].status})`).Output() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| o.Expect(publishStatus).NotTo(o.ContainSubstring("False")) | |
| exutil.By("Ensure the dnsrecord with new LB IP/hostname are published") | |
| publishStatus, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress-operator", "dnsrecord", dnsRecordName, `-o=jsonpath={.status.zones[*].conditions[?(@.type == "Published")].status}`).Output() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| o.Expect(strings.TrimSpace(publishStatus)).NotTo(o.BeEmpty()) | |
| o.Expect(publishStatus).NotTo(o.ContainSubstring("False")) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/cloud_load_balancer.go` around lines 129 - 132, The jsonpath in the
oc get call is malformed (extra ')') and the test allows empty output; fix the
Args call in oc.AsAdmin().WithoutNamespace().Run("get").Args(...) to remove the
stray closing parenthesis in `-o=jsonpath={.status.zones[*].conditions[?(@.type
== "Published")].status}` and then tighten the assertions on publishStatus (the
variable returned) so you first assert it is not empty and then assert it
indicates published (e.g., o.Expect(publishStatus).NotTo(o.BeEmpty()) and
o.Expect(publishStatus).To(o.ContainSubstring("True")) or equivalent)
referencing publishStatus and dnsRecordName to validate the published state.
| exutil.By("Ensure the dnsrecord with new LB IP/hostname are published") | ||
| publishStatus, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress-operator", "dnsrecord", dnsRecordName, `-o=jsonpath={.status.zones[*].conditions[?(@.type == "Published")].status})`).Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(publishStatus).NotTo(o.ContainSubstring("False")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/e2e/extension/cloud_load_balancer.go | sed -n '125,140p'Repository: openshift/router
Length of output: 1049
Fix the malformed jsonpath in the dnsrecord query.
Line 131 contains an extra ) at the end of the jsonpath argument: {.status.zones[*].conditions[?(@.type == "Published")].status}) should be {.status.zones[*].conditions[?(@.type == "Published")].status}. This syntax error will cause the oc get command to fail.
Suggested fix
- publishStatus, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress-operator", "dnsrecord", dnsRecordName, `-o=jsonpath={.status.zones[*].conditions[?(@.type == "Published")].status})`).Output()
+ publishStatus, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress-operator", "dnsrecord", dnsRecordName, `-o=jsonpath={.status.zones[*].conditions[?(@.type == "Published")].status}`).Output()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exutil.By("Ensure the dnsrecord with new LB IP/hostname are published") | |
| publishStatus, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress-operator", "dnsrecord", dnsRecordName, `-o=jsonpath={.status.zones[*].conditions[?(@.type == "Published")].status})`).Output() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| o.Expect(publishStatus).NotTo(o.ContainSubstring("False")) | |
| exutil.By("Ensure the dnsrecord with new LB IP/hostname are published") | |
| publishStatus, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress-operator", "dnsrecord", dnsRecordName, `-o=jsonpath={.status.zones[*].conditions[?(@.type == "Published")].status}`).Output() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| o.Expect(publishStatus).NotTo(o.ContainSubstring("False")) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/extension/cloud_load_balancer.go` around lines 130 - 133, The
jsonpath argument passed to oc.AsAdmin().WithoutNamespace().Run("get").Args(...)
is malformed: `{.status.zones[*].conditions[?(@.type == "Published")].status})`
has an extra closing parenthesis; update the Args call that builds the dnsrecord
query (the call that uses dnsRecordName and `-o=jsonpath=...`) to use a correct
jsonpath `{.status.zones[*].conditions[?(@.type == "Published")].status}` so the
oc get command succeeds and the subsequent publishStatus check works.
test/e2e/extension/ingress.go
Outdated
| removeRoute := fmt.Sprintf("[{\"op\":\"remove\", \"path\":\"/spec/componentRoutes\", \"value\":[{\"hostname\": \"1digit9.apps.%s\", \"name\": \"downloads\", \"namespace\": \"openshift-console\"}]}]}]", getBaseDomain(oc)) | ||
| addRoute := fmt.Sprintf("[{\"op\":\"add\", \"path\":\"/spec/componentRoutes\", \"value\":[{\"hostname\": \"1digit9.apps.%s\", \"name\": \"downloads\", \"namespace\": \"openshift-console\"}]}]}]", getBaseDomain(oc)) | ||
| defer patchGlobalResourceAsAdmin(oc, resourceName, removeRoute) | ||
| patchGlobalResourceAsAdmin(oc, resourceName, addRoute) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/e2e/extension/ingress.go | sed -n '487,490p'Repository: openshift/router
Length of output: 650
🌐 Web query:
RFC 6902 JSON Patch remove operation value field specification
💡 Result:
In RFC 6902 (JSON Patch), a remove operation does not have (and does not require) a value member—it only uses:
op(must be"remove")path(JSON Pointer to the target location)
Example from the RFC: { "op": "remove", "path": "/a/b/c" }. No value is defined for remove. [1]
Also, RFC 6902 explicitly says that members not defined for a given operation MUST be ignored—so if a client sends a value alongside a remove, a compliant implementation should ignore it. [1]
Sources:
[1] RFC 6902, Sections 4 (Operations) and 4.2 (remove) (IETF datatracker)
Fix malformed JSON patches in componentRoutes operations.
Lines 487–488 have two critical issues:
- Extra closing brackets (
}]) at the end make the JSON invalid - The
removeoperation includes avaluefield, which violates RFC 6902 (JSON Patch spec requires onlyopandpathfor remove operations)
These malformed patches will fail, and since cleanup is deferred, the ingress configuration will be left modified for subsequent tests.
Suggested fix
- removeRoute := fmt.Sprintf("[{\"op\":\"remove\", \"path\":\"/spec/componentRoutes\", \"value\":[{\"hostname\": \"1digit9.apps.%s\", \"name\": \"downloads\", \"namespace\": \"openshift-console\"}]}]}]", getBaseDomain(oc))
- addRoute := fmt.Sprintf("[{\"op\":\"add\", \"path\":\"/spec/componentRoutes\", \"value\":[{\"hostname\": \"1digit9.apps.%s\", \"name\": \"downloads\", \"namespace\": \"openshift-console\"}]}]}]", getBaseDomain(oc))
+ removeRoute := `[{"op":"remove","path":"/spec/componentRoutes"}]`
+ addRoute := fmt.Sprintf(
+ `[{"op":"add","path":"/spec/componentRoutes","value":[{"hostname":"1digit9.apps.%s","name":"downloads","namespace":"openshift-console"}]}]`,
+ getBaseDomain(oc),
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/extension/ingress.go` around lines 487 - 490, The JSON patch strings
constructed in removeRoute and addRoute are malformed: they include extra
closing brackets and the removeOperation incorrectly includes a "value" field;
update removeRoute to use a JSON Patch with only op "remove" and the correct
path (no "value"), and update addRoute to a valid add operation JSON (remove the
stray extra brackets at the end). Use the same helpers shown (getBaseDomain(oc)
to build the hostname) and call patchGlobalResourceAsAdmin(resourceName, ...)
the same way so that removeRoute is a proper remove patch and addRoute is a
proper add patch to /spec/componentRoutes.
| labels: | ||
| name: service-unsecure | ||
| name: hello-pod | ||
| namespace: ${NAMESPACE} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether Route template objects still use top-level namespace (should return no matches after fix)
rg -n '^\s{2}namespace:\s*\$\{NAMESPACE\}\s*$' test/e2e/extension/testdata/router/49802-route.yamlRepository: openshift/router
Length of output: 117
🏁 Script executed:
cat -n test/e2e/extension/testdata/router/49802-route.yamlRepository: openshift/router
Length of output: 897
🌐 Web query:
OpenShift Route YAML namespace specification metadata structure
💡 Result:
In an OpenShift Route manifest, the “namespace specification” is simply the standard Kubernetes ObjectMeta field metadata.namespace (a string) on the Route object.
Minimal example:
apiVersion: route.openshift.io/v1
kind: Route
metadata:
name: hello-openshift-edge
namespace: hello-openshift
spec:
to:
kind: Service
name: hello-openshiftRed Hat’s Route examples show metadata.name and metadata.namespace exactly like this. [1]
Notes:
metadata.namespaceis the project/namespace the Route belongs to; it must match where the target Service exists (Routes are namespaced resources). [1]- Ensure the Route
apiVersionisroute.openshift.io/v1(not plainv1), or creation can fail with “no matches for kind ‘Route’ in version ‘v1’”. [2]
Sources: [1] [2]
Move namespace into metadata for both Route objects.
Lines 10 and 24 incorrectly place namespace at the top level of the Route object. According to Kubernetes/OpenShift specification, namespace must be nested under metadata. The current structure risks invalid YAML parsing and namespace scoping being ignored.
Proposed fix
- apiVersion: route.openshift.io/v1
kind: Route
metadata:
labels:
name: service-unsecure
name: hello-pod
- namespace: ${NAMESPACE}
+ namespace: ${NAMESPACE}
spec:
@@
- apiVersion: route.openshift.io/v1
kind: Route
metadata:
labels:
name: service-unsecure
name: hello-pod-http
- namespace: ${NAMESPACE}
+ namespace: ${NAMESPACE}
spec:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| namespace: ${NAMESPACE} | |
| - apiVersion: route.openshift.io/v1 | |
| kind: Route | |
| metadata: | |
| labels: | |
| name: service-unsecure | |
| name: hello-pod | |
| namespace: ${NAMESPACE} | |
| spec: | |
| - apiVersion: route.openshift.io/v1 | |
| kind: Route | |
| metadata: | |
| labels: | |
| name: service-unsecure | |
| name: hello-pod-http | |
| namespace: ${NAMESPACE} | |
| spec: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/extension/testdata/router/49802-route.yaml` at line 10, The Route
manifests have namespace: ${NAMESPACE} placed at the top level instead of under
metadata; move each top-level namespace entry into the corresponding Route
object's metadata block (i.e., ensure each Route has metadata: name: ... and
metadata: namespace: ${NAMESPACE}), removing the top-level namespace keys so
both Route resources conform to Kubernetes/OpenShift schema and namespace
scoping.
| "containers": [{ | ||
| "name": "httpbin-http", | ||
| "image": "quay.io/openshifttest/httpbin@sha256:cc44fbd857f4148d8aad8359acc03efa719517e01d390b152e4f3830ad871c9f", | ||
| "ports": [ | ||
| { | ||
| "containerPort": 8080, | ||
| "securityContext": { | ||
| "privileged": "true" | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "name": "httpbin-https", | ||
| "image": "quay.io/openshifttest/httpbin@sha256:f57f4e682e05bcdadb103c93ae5ab9be166f79bcbbccaf45d92a2cad18da8d64", | ||
| "ports": [ | ||
| { | ||
| "containerPort": 8443, | ||
| "securityContext": { | ||
| "privileged": "true" | ||
| } | ||
| } | ||
| ] | ||
| }] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import json
from pathlib import Path
path = Path("test/e2e/extension/testdata/router/httpbin-pod-withprivilege.json")
doc = json.loads(path.read_text())
for container in doc["spec"]["containers"]:
print(container["name"], "container_securityContext =", container.get("securityContext"))
for port in container.get("ports", []):
sc = port.get("securityContext")
print(" port keys =", sorted(port.keys()))
print(" port.securityContext =", sc, "privileged_type =", type(sc.get("privileged")).__name__ if sc else None)
PYRepository: openshift/router
Length of output: 394
🏁 Script executed:
# Check the purpose of this fixture by looking at related test files
fd -t f "\.json$" test/e2e/extension/testdata/router/ | head -20Repository: openshift/router
Length of output: 449
🏁 Script executed:
# Search for references to this fixture file to understand its intended use
rg "httpbin-pod-withprivilege" --type json --type sh --type goRepository: openshift/router
Length of output: 1080
🌐 Web query:
Kubernetes Container securityContext schema - is it valid under ports?
💡 Result:
No. securityContext is a field of the Container object (spec.containers[*].securityContext), not of ports. In the Kubernetes API schema, spec.containers[*].ports is an array of ContainerPort objects, and ContainerPort only allows containerPort, hostIP, hostPort, name, and protocol—there is no securityContext under ports[*]. If you put it there, you’ll get an “unknown field …” validation/strict-decoding error. [1], [2]
Valid placement:
spec:
containers:
- name: app
securityContext:
runAsNonRoot: true
ports:
- containerPort: 8080Sources:
[1] (kubernetes.io)
[2] (kubernetes.io)
Citations:
- 1: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.26/
- 2: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.26/
🏁 Script executed:
# Look at how the fixture is used in headers.go test file
rg -B 3 -A 5 "httpbin-pod-withprivilege" test/e2e/extension/headers.goRepository: openshift/router
Length of output: 1154
🏁 Script executed:
# Check if there's a test that expects this pod to fail or succeed
rg -B 5 -A 10 "serverPod.*=.*buildPruningBaseDir" test/e2e/extension/headers.go | head -80Repository: openshift/router
Length of output: 1997
Move securityContext out of ports[] and use booleans.
securityContext is not valid under ports[] (the Kubernetes Container API only allows containerPort, hostIP, hostPort, name, and protocol on ContainerPort objects). Additionally, "privileged": "true" is the wrong type—it must be a boolean. The API server will reject or prune this field, leaving the pod unprivileged.
Schema-correct fix
{
"name": "httpbin-http",
"image": "quay.io/openshifttest/httpbin@sha256:cc44fbd857f4148d8aad8359acc03efa719517e01d390b152e4f3830ad871c9f",
+ "securityContext": {
+ "privileged": true
+ },
"ports": [
{
- "containerPort": 8080,
- "securityContext": {
- "privileged": "true"
- }
+ "containerPort": 8080
}
]
},
{
"name": "httpbin-https",
"image": "quay.io/openshifttest/httpbin@sha256:f57f4e682e05bcdadb103c93ae5ab9be166f79bcbbccaf45d92a2cad18da8d64",
+ "securityContext": {
+ "privileged": true
+ },
"ports": [
{
- "containerPort": 8443,
- "securityContext": {
- "privileged": "true"
- }
+ "containerPort": 8443
}
]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "containers": [{ | |
| "name": "httpbin-http", | |
| "image": "quay.io/openshifttest/httpbin@sha256:cc44fbd857f4148d8aad8359acc03efa719517e01d390b152e4f3830ad871c9f", | |
| "ports": [ | |
| { | |
| "containerPort": 8080, | |
| "securityContext": { | |
| "privileged": "true" | |
| } | |
| } | |
| ] | |
| }, | |
| { | |
| "name": "httpbin-https", | |
| "image": "quay.io/openshifttest/httpbin@sha256:f57f4e682e05bcdadb103c93ae5ab9be166f79bcbbccaf45d92a2cad18da8d64", | |
| "ports": [ | |
| { | |
| "containerPort": 8443, | |
| "securityContext": { | |
| "privileged": "true" | |
| } | |
| } | |
| ] | |
| }] | |
| "containers": [{ | |
| "name": "httpbin-http", | |
| "image": "quay.io/openshifttest/httpbin@sha256:cc44fbd857f4148d8aad8359acc03efa719517e01d390b152e4f3830ad871c9f", | |
| "securityContext": { | |
| "privileged": true | |
| }, | |
| "ports": [ | |
| { | |
| "containerPort": 8080 | |
| } | |
| ] | |
| }, | |
| { | |
| "name": "httpbin-https", | |
| "image": "quay.io/openshifttest/httpbin@sha256:f57f4e682e05bcdadb103c93ae5ab9be166f79bcbbccaf45d92a2cad18da8d64", | |
| "securityContext": { | |
| "privileged": true | |
| }, | |
| "ports": [ | |
| { | |
| "containerPort": 8443 | |
| } | |
| ] | |
| }] |
🧰 Tools
🪛 Trivy (0.69.3)
[error] 23-34: Root file system is not read-only
Container 'httpbin-https' of Pod 'httpbin-pod' should set 'securityContext.readOnlyRootFilesystem' to true
Rule: KSV-0014
(IaC/Kubernetes)
[error] 23-34: Default security context configured
container httpbin-pod in default namespace is using the default security context
Rule: KSV-0118
(IaC/Kubernetes)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/extension/testdata/router/httpbin-pod-withprivilege.json` around
lines 11 - 34, The securityContext blocks currently live under ports[], which is
invalid; move each securityContext (and its privileged field) up into the
corresponding container object (e.g., the containers named "httpbin-http" and
"httpbin-https") as the container-level securityContext, and change
"privileged": "true" to privileged: true (a boolean) so the
Container.securityContext.privileged field is correctly typed and accepted by
the API.
| kind: List | ||
| items: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/e2e/extension/testdata/router/ingress-resource.yaml | head -20Repository: openshift/router
Length of output: 613
🏁 Script executed:
# Verify Kubernetes List schema requirements
web_search_query="kubernetes api list kind apiversion required schema"Repository: openshift/router
Length of output: 42
🌐 Web query:
Kubernetes List kind apiVersion schema requirements
💡 Result:
In the Kubernetes API, a “list response” is not a generic kind: List. Instead, each resource has its own concrete list kind, e.g. PodList, ServiceList, NodeList, etc. (kind is <ResourceKind>List). These list objects include:
apiVersion: the API version of that resource (example:v1)kind:<ResourceKind>List(example:ServiceList)metadata: ListMeta (list-level metadata such asresourceVersion, and optionallycontinue/remainingItemCountfor chunking)items: array of the listed objects (each item is the single-object schema for that resource) [1] [2]
Example (API list response shape):
apiVersion: v1
kind: ServiceList
metadata:
resourceVersion: "2947301"
# continue: "..." # if chunking
# remainingItemCount: 123 # if provided
items:
- apiVersion: v1
kind: Service
metadata:
name: kubernetes
namespace: default
spec: {}About kind: List
kubectl may output mixed collections using:
apiVersion: v1
kind: List
items: [...]…but the Kubernetes API does not have a kind named List; this is a client-side (kubectl) representation detail and shouldn’t be depended on for automation/code [1].
Sources: [1] Kubernetes “API Concepts” (Collections / kind: List note); [2] Kubernetes API reference (common list fields: apiVersion, kind, metadata (ListMeta), items).
Add missing apiVersion on the top-level List object.
Line 1 defines kind: List without apiVersion; this violates Kubernetes manifest schema and can fail validation in strict clients.
Proposed patch
+apiVersion: v1
kind: List
items:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kind: List | |
| items: | |
| apiVersion: v1 | |
| kind: List | |
| items: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/extension/testdata/router/ingress-resource.yaml` around lines 1 - 2,
The top-level Kubernetes List manifest is missing an apiVersion; update the
top-level List object (the document where kind: List appears) to include
apiVersion: v1 so the resource validates against the Kubernetes schema (add
apiVersion: v1 alongside kind: List at the top of the file).
| spec: | ||
| selector: | ||
| matchLabels: | ||
| app: echoenv-sts | ||
| replicas: 2 | ||
| template: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import sys, subprocess, pkgutil, pathlib
if pkgutil.find_loader("yaml") is None:
subprocess.check_call([sys.executable, "-m", "pip", "install", "--quiet", "pyyaml"])
import yaml
path = pathlib.Path("test/e2e/extension/testdata/router/ocp57404-stateful-set.yaml")
doc = yaml.safe_load(path.read_text())
items = doc.get("items", [])
services = {
i.get("metadata", {}).get("name"): i
for i in items if i.get("kind") == "Service"
}
errors = []
for idx, item in enumerate(items):
if item.get("kind") != "StatefulSet":
continue
spec = item.get("spec", {})
svc_name = spec.get("serviceName")
if not svc_name:
errors.append(f"items[{idx}] StatefulSet missing spec.serviceName")
continue
svc = services.get(svc_name)
if not svc:
errors.append(f"items[{idx}] serviceName '{svc_name}' does not reference a Service in this List")
elif svc.get("spec", {}).get("clusterIP") != "None":
errors.append(f"Service '{svc_name}' is not headless (clusterIP must be None)")
if errors:
print("\n".join(errors))
sys.exit(1)
print("OK: StatefulSet serviceName is present and references a headless Service.")
PYRepository: openshift/router
Length of output: 105
Add required spec.serviceName to StatefulSet
The StatefulSet is missing the mandatory spec.serviceName field. In Kubernetes apps/v1, this field must reference the governing headless Service; without it, the fixture is invalid and cannot be applied.
Fix
spec:
+ serviceName: echoenv-statefulset-service
selector:
matchLabels:
app: echoenv-sts📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| spec: | |
| selector: | |
| matchLabels: | |
| app: echoenv-sts | |
| replicas: 2 | |
| template: | |
| spec: | |
| serviceName: echoenv-statefulset-service | |
| selector: | |
| matchLabels: | |
| app: echoenv-sts | |
| replicas: 2 | |
| template: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/extension/testdata/router/ocp57404-stateful-set.yaml` around lines 8
- 13, The StatefulSet is missing the required spec.serviceName; update the
StatefulSet manifest for the resource (the StatefulSet object under "spec") to
add a spec.serviceName field that references the governing headless Service name
(the Service that has clusterIP: None and matches the StatefulSet's selector),
e.g., set spec.serviceName to the headless Service's metadata.name so the
StatefulSet's serviceName matches its governing Service.
| patchStr := "[{\"op\":\"replace\",\"path\":\"/spec/zones/0\",\"value\":" + zoneID + "}]" | ||
| patchGlobalResourceAsAdmin(oc, "externaldnses.externaldns.olm.openshift.io/"+crName, patchStr) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l test/e2e/externaldns.goRepository: openshift/router
Length of output: 87
🏁 Script executed:
sed -n '120,140p' test/e2e/externaldns.goRepository: openshift/router
Length of output: 1617
🏁 Script executed:
sed -n '175,190p' test/e2e/externaldns.goRepository: openshift/router
Length of output: 1357
Fix JSON patches to use proper string quoting and valid JSON syntax.
Line 127 concatenates zoneID without JSON string quoting, producing invalid JSON like {"value":somevalue}. Line 182 uses single quotes (not valid in JSON) and concatenates zoneName unquoted. Both payloads are syntactically invalid and will cause the patch operation to fail before pod-rollout assertions run.
Use fmt.Sprintf with %q for proper JSON string escaping:
Suggested fixes
- patchStr := "[{\"op\":\"replace\",\"path\":\"/spec/zones/0\",\"value\":" + zoneID + "}]"
+ patchStr := fmt.Sprintf(`[{"op":"replace","path":"/spec/zones/0","value":%q}]`, zoneID)
patchGlobalResourceAsAdmin(oc, "externaldnses.externaldns.olm.openshift.io/"+crName, patchStr)- patchStr := "[{\"op\":\"replace\",\"path\":\"/spec/source/fqdnTemplate/0\",\"value\":'{{.Name}}." + baseDomain + "'},{\"op\":\"replace\",\"path\":\"/spec/zones/0\",\"value\":" + zoneName + "}]"
+ patchStr := fmt.Sprintf(
+ `[{"op":"replace","path":"/spec/source/fqdnTemplate/0","value":%q},{"op":"replace","path":"/spec/zones/0","value":%q}]`,
+ "{{.Name}}."+baseDomain,
+ zoneName,
+ )
patchGlobalResourceAsAdmin(oc, "externaldnses.externaldns.olm.openshift.io/"+crName, patchStr)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| patchStr := "[{\"op\":\"replace\",\"path\":\"/spec/zones/0\",\"value\":" + zoneID + "}]" | |
| patchGlobalResourceAsAdmin(oc, "externaldnses.externaldns.olm.openshift.io/"+crName, patchStr) | |
| patchStr := fmt.Sprintf(`[{"op":"replace","path":"/spec/zones/0","value":%q}]`, zoneID) | |
| patchGlobalResourceAsAdmin(oc, "externaldnses.externaldns.olm.openshift.io/"+crName, patchStr) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/externaldns.go` around lines 127 - 128, The JSON patch strings are
being built by concatenation without proper JSON string quoting (patchStr using
zoneID and another payload using zoneName), producing invalid JSON; update the
patch construction to use fmt.Sprintf with the %q verb to safely quote/escape
zoneID and zoneName (e.g. build patchStr via
fmt.Sprintf(`[{"op":"replace","path":"/spec/zones/0","value":%q}]`, zoneID)) and
similarly replace the single-quoted/concatenated payload at the other location
with fmt.Sprintf and %q, then pass those valid JSON strings into the existing
patchGlobalResourceAsAdmin call with crName unchanged.
| "containers": [{ | ||
| "name": "httpbin-http", | ||
| "image": "quay.io/openshifttest/httpbin@sha256:cc44fbd857f4148d8aad8359acc03efa719517e01d390b152e4f3830ad871c9f", | ||
| "ports": [ | ||
| { | ||
| "containerPort": 8080, | ||
| "securityContext": { | ||
| "privileged": "true" | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "name": "httpbin-https", | ||
| "image": "quay.io/openshifttest/httpbin@sha256:f57f4e682e05bcdadb103c93ae5ab9be166f79bcbbccaf45d92a2cad18da8d64", | ||
| "ports": [ | ||
| { | ||
| "containerPort": 8443, | ||
| "securityContext": { | ||
| "privileged": "true" | ||
| } | ||
| } | ||
| ] | ||
| }] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json
from pathlib import Path
targets = list(Path(".").rglob("httpbin-pod-withprivilege.json"))
if not targets:
print("Target file not found")
raise SystemExit(1)
for p in targets:
data = json.loads(p.read_text())
print(f"\nChecking: {p}")
containers = data.get("spec", {}).get("containers", [])
for idx, c in enumerate(containers, start=1):
sc = c.get("securityContext")
ports = c.get("ports", [])
port_sc = any(isinstance(port, dict) and "securityContext" in port for port in ports)
priv_type = type(sc.get("privileged")).__name__ if isinstance(sc, dict) and "privileged" in sc else None
print(f" container[{idx}]={c.get('name')}")
print(f" container.securityContext present: {sc is not None}")
print(f" container.securityContext.privileged type: {priv_type}")
print(f" securityContext under ports (invalid): {port_sc}")
PYRepository: openshift/router
Length of output: 890
Move securityContext from ports to container level and change privileged to boolean
Both container definitions have securityContext incorrectly nested under each ports entry, and privileged is a string ("true") instead of a boolean (true). Kubernetes expects securityContext at the container level. This prevents the intended privileged setting from being applied and will cause validation failures.
The same issue exists in two files:
test/e2e/testdata/httpbin-pod-withprivilege.jsontest/e2e/extension/testdata/router/httpbin-pod-withprivilege.json
Proposed fix (both files)
"spec": {
"containers": [{
"name": "httpbin-http",
"image": "quay.io/openshifttest/httpbin@sha256:cc44fbd857f4148d8aad8359acc03efa719517e01d390b152e4f3830ad871c9f",
+ "securityContext": {
+ "privileged": true
+ },
"ports": [
{
- "containerPort": 8080,
- "securityContext": {
- "privileged": "true"
- }
+ "containerPort": 8080
}
]
},
{
"name": "httpbin-https",
"image": "quay.io/openshifttest/httpbin@sha256:f57f4e682e05bcdadb103c93ae5ab9be166f79bcbbccaf45d92a2cad18da8d64",
+ "securityContext": {
+ "privileged": true
+ },
"ports": [
{
- "containerPort": 8443,
- "securityContext": {
- "privileged": "true"
- }
+ "containerPort": 8443
}
]
}]
}🧰 Tools
🪛 Trivy (0.69.3)
[error] 23-34: Root file system is not read-only
Container 'httpbin-https' of Pod 'httpbin-pod' should set 'securityContext.readOnlyRootFilesystem' to true
Rule: KSV-0014
(IaC/Kubernetes)
[error] 23-34: Default security context configured
container httpbin-pod in default namespace is using the default security context
Rule: KSV-0118
(IaC/Kubernetes)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/testdata/httpbin-pod-withprivilege.json` around lines 11 - 34, The
securityContext blocks are incorrectly nested under each ports entry and use
string values; move each securityContext up to the container level for the
httpbin-http and httpbin-https containers (i.e., directly inside the container
object, not inside ports) and change "privileged": "true" to privileged: true (a
boolean); remove the securityContext entries from each ports array so Kubernetes
validation applies the privileged setting at the container level.
…onally append the SHARD parameter only when it's not empty, preventing the fail to process,error caused by passing SHARD= with an empty value to oc process.
|
$ ./bin/router-tests-ext list | grep sig - $ ./bin/router-tests-ext list | grep sig - | wc -l $ ./bin/router-tests-ext run-test "[sig-network-edge] Network_Edge Component_Router Author:shudili-LEVEL0-High-55367-Default HAProxy maxconn value to 50000 for OCP 4.12 and later" Random Seed: 1773802962 - will randomize all specs Will run 1 of 1 specs [sig-network-edge] Network_Edge Component_Router Author:shudili-LEVEL0-High-55367-Default HAProxy maxconn value to 50000 for OCP 4.12 and later Ran 1 of 1 Specs in 155.890 seconds $ podman run --rm --entrypoint ls router:test -lh /usr/bin/router-test-extension.tar.gz |
|
@ming1013: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
========================================
🎉 OTE Migration Complete!
Summary
Successfully migrated router to OTE framework using monorepo strategy with single module approach.
Created Structure
Directory tree:
Key Features
cmd/extension/main.go(at root, not under test/)vendor/at repository root (447M)Test Migration Summary
Dependency Resolution
Next Steps
1. Verify Build
2. List Tests
3. Run Tests
4. Build Docker Image
5. Verify Test Annotations
Files Created/Modified
Created Files:
cmd/extension/main.go- OTE entry pointtest/e2e/testdata/fixtures.go- Testdata helper functionstest/e2e/testdata/bindata.go- Generated bindata (can be in .gitignore)bindata.mk- Bindata generation makefile (at root)vendor/- Vendored dependencies (447M)bin/router-tests-ext- Built extension binary (342M)Modified Files:
go.mod- Added OTE dependencies, updated k8s.io versionsgo.sum- Updated checksumsMakefile- Added OTE targets (tests-ext-build, extension, clean-extension)test/e2e/*.go- 23 test files (annotations, imports)images/router/haproxy/Dockerfile.rhel- Added test extension stagesBackups Created:
go.mod.backup.k8s-version-fix- Backup before k8s.io version updateimages/router/haproxy/Dockerfile.rhel.pre-ote-migration- Dockerfile backupFiles to Commit (IMPORTANT!)
Before creating a PR, ensure these files are committed to git:
Required for reproducible builds:
go.mod- Single module with all dependenciesgo.sum- Single go.sum for reproducible buildscmd/extension/main.go- OTE entry pointtest/e2e/testdata/fixtures.go- Testdata helper functionsbindata.mk- Bindata generation makefile (at root)test/e2e/*.go- Migrated test files (23 files)Makefile- Updated with OTE targetsimages/router/haproxy/Dockerfile.rhel- Updated DockerfileCan be in .gitignore:
test/e2e/testdata/bindata.go- Generated file (regenerated during build)bin/router-tests-ext- Binary (build artifact)vendor/- Optional (some repos commit, others don't)Why go.sum is critical:
Without committed go.sum, Docker builds will:
With committed go.sum, Docker builds will:
Verify file is tracked:
Troubleshooting
If you encounter issues during build or test execution, refer to the troubleshooting guide in the migration skill documentation.
Common issues:
go mod tidy && go mod vendormake -f bindata.mk update-bindatamake clean-extension && make extension========================================
Migration completed successfully! 🎉