NE-2126: Migrating Ipfailover test cases to images repo - #245
NE-2126: Migrating Ipfailover test cases to images repo#245melvinjoseph86 wants to merge 1 commit into
Conversation
|
@melvinjoseph86: This pull request references NE-2126 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Go-based ipfailover OTE test extension with cluster utilities, six E2E scenarios, suite registration, and Docker image packaging. The Docker build produces a gzip-compressed extension binary and copies it into the final image. ChangesIpfailover test extension
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TestExtension
participant GinkgoSuite
participant OpenShiftCluster
participant KeepalivedPods
TestExtension->>GinkgoSuite: build and filter test specs
GinkgoSuite->>OpenShiftCluster: create ipfailover deployment
OpenShiftCluster->>KeepalivedPods: start keepalived replicas
KeepalivedPods-->>GinkgoSuite: return roles, VIPs, and logs
GinkgoSuite-->>TestExtension: report test results
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 5 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/test tests-extension |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with 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.
Inline comments:
In `@ipfailover/keepalived/Dockerfile`:
- Line 4: Replace the broad COPY . . instruction in the Dockerfile with explicit
COPY directives for only the test module’s required files and directories,
ensuring unrelated files and secrets are excluded from the builder context.
- Around line 24-25: Update the final base-rhel9 stage in the Dockerfile to run
with a non-root USER, not only the builder stage. Verify keepalived still
functions with that identity; if elevated privileges are required, document the
exception and restrict execution to the minimum necessary capabilities.
- Around line 24-25: The final image lacks a healthcheck for the keepalived
service. Add a Docker HEALTHCHECK to the keepalived Dockerfile that verifies
both monitor.sh and the keepalived process are running successfully, using an
appropriate command and interval, timeout, retries, and startup grace period.
In `@ipfailover/keepalived/tests/e2e/util.go`:
- Around line 107-123: Update the temporary directory creation in
getImagePullSpecFromPayload to use owner-only permissions instead of 0755,
ensuring the extracted pull-secret and its parent directory are inaccessible to
other local users while preserving the existing extraction and cleanup flow.
- Around line 414-437: Update getSpecificPodLogs to remove the bash-based
exec.Command filtering and extra cat process. Read podLogs directly in Go, apply
the optional case-insensitive filter in memory, and return the matching log
content while preserving the existing error logging and return behavior.
In `@ipfailover/keepalived/tests/README.md`:
- Line 49: Update the fenced directory-tree code block in the README to specify
the text language, changing the untyped fence to a text fence while preserving
its contents.
In `@tests-extension/README.md`:
- Line 38: Update the documented CI command in the execution instructions to
include --max-concurrency=1 when invoking run-suite for ipfailover/all,
preserving serial test execution required by the hostNetwork-based tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 10b4db7b-9654-4cb0-94fa-6e7281579f0b
⛔ Files ignored due to path filters (1)
ipfailover/keepalived/tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
ipfailover/keepalived/.dockerignoreipfailover/keepalived/Dockerfileipfailover/keepalived/tests/.gitignoreipfailover/keepalived/tests/Makefileipfailover/keepalived/tests/README.mdipfailover/keepalived/tests/cmd/main.goipfailover/keepalived/tests/e2e/ipfailover.goipfailover/keepalived/tests/e2e/testdata/router/ipfailover.yamlipfailover/keepalived/tests/e2e/util.goipfailover/keepalived/tests/go.modtests-extension/.gitignoretests-extension/Makefiletests-extension/README.md
💤 Files with no reviewable changes (1)
- ipfailover/keepalived/.dockerignore
1bed137 to
2c8ac73
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
/test tests-extension |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with 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.
Inline comments:
In `@ipfailover/keepalived/Dockerfile`:
- Around line 8-9: Update the Dockerfile artifact packaging commands to
gzip-compress the built ipfailover-tests-ext executable with gzip rather than
creating a tar archive, and publish the resulting raw gzip artifact at the
registered /usr/bin/ipfailover-test-extension.tar.gz path while preserving the
expected executable build flow.
In `@ipfailover/keepalived/tests/e2e/testdata/router/ipfailover.yaml`:
- Around line 69-74: Add a readinessProbe alongside the existing livenessProbe
in the Deployment container configuration, using a check that confirms
keepalived initialization and VRRP/VIP readiness (or the VRRP controller port)
rather than merely checking that the process is running. Keep the existing
livenessProbe unchanged.
- Around line 28-69: Add a resources block to the openshift-ipfailover container
definition, specifying both CPU and memory requests and limits. Keep the
existing container configuration unchanged and use the repository’s established
resource values or conventions if available.
- Around line 14-27: Update the ipfailover e2e setup around the deployment
replica configuration and OPENSHIFT_HA_REPLICA_COUNT to derive the count from
eligible worker nodes rather than assuming two workers. Inspect
infrastructure.Status.ControlPlaneTopology before running these tests, and
explicitly skip SingleReplica, HyperShift, arbiter, and External topologies;
keep the node-role.kubernetes.io/worker selection aligned with the computed
placement.
- Around line 22-47: Harden the ipfailover test PodSpec by removing the invalid
PodSpec-level privileged setting, hostNetwork, and host-slash host-path mount
unless each is demonstrably required; retain only required host integrations.
For the remaining container configuration, remove unnecessary privileges and add
allowPrivilegeEscalation: false, runAsNonRoot: true, and readOnlyRootFilesystem:
true where supported, using only verified capabilities if host networking must
remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
2c8ac73 to
cc23f7e
Compare
|
/test tests-extension |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: melvinjoseph86 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
ipfailover/keepalived/tests/e2e/ipfailover.go (2)
388-390: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the pod list call with no effect.
Line 389 calls
getPodListByLabeland discards the result. The same pattern exists inipfailover/keepalived/tests/e2e/util.goline 282. Delete both calls, or capture and use the result.🤖 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 `@ipfailover/keepalived/tests/e2e/ipfailover.go` around lines 388 - 390, Remove the unused getPodListByLabel call in the “Exclude VIP '9' from the ipfailover group” test flow, and remove the matching discarded call in the related utility flow. Keep the surrounding setEnvVariable behavior unchanged.
244-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the
checkPlatformhelper.
util.goalready providescheckPlatform(oc), which runs the sameoc get infrastructurequery and lowercases the result. Lines 43-47 and this block duplicate that logic.♻️ Proposed refactor
- // Get platform type using oc command instead of compat_otp - infraOutput, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("infrastructure", "cluster", "-o=jsonpath={.status.platformStatus.type}").Output() - if err == nil { - platformtype := strings.ToLower(strings.TrimSpace(infraOutput)) - if platformtype == "nutanix" { - g.Skip("This test will not works for Nutanix") - } - } + if checkPlatform(oc) == "nutanix" { + g.Skip("This test does not work on Nutanix") + }🤖 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 `@ipfailover/keepalived/tests/e2e/ipfailover.go` around lines 244 - 251, Replace the duplicated platform query and normalization in the test setup with the existing checkPlatform helper, passing the current oc client. Preserve the Nutanix-specific skip behavior based on the helper’s returned platform value and remove the local infrastructure query logic.ipfailover/keepalived/tests/e2e/util.go (1)
110-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
rm -rfsubprocess withos.RemoveAll.Line 112 spawns an external process and discards its error.
os.RemoveAllremoves the directory in-process and returns an error you can log. This also removes theos/execdependency from this function.♻️ Proposed refactor
dockerconfigjsonpath := filepath.Join(indexTmpPath, ".dockerconfigjson") - defer exec.Command("rm", "-rf", indexTmpPath).Output() + defer func() { + if rmErr := os.RemoveAll(indexTmpPath); rmErr != nil { + e2e.Logf("failed to remove temporary directory %s: %v", indexTmpPath, rmErr) + } + }() err := os.MkdirAll(indexTmpPath, 0700)🤖 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 `@ipfailover/keepalived/tests/e2e/util.go` around lines 110 - 113, Update the cleanup in the test utility around indexTmpPath to call os.RemoveAll instead of spawning rm -rf through exec.Command. Handle or log the returned cleanup error, and remove the now-unused os/exec dependency if it is only needed by this call.ipfailover/keepalived/tests/Makefile (1)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose the test command through the Makefile.
This new test extension has no
testtarget. Add a target that runs the supported E2E and vet commands. Ifcheckmakeis enforced, add its requiredalltarget or configure the check for this project.Based on learnings, run the full test suite and vet checks before submitting a pull request.
🤖 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 `@ipfailover/keepalived/tests/Makefile` around lines 14 - 18, Add a test target to the Makefile that runs the supported end-to-end test and vet commands for the extension. Also provide the required all target if checkmake enforces it, or configure the project’s check accordingly, while preserving the existing help, build, and clean targets.Sources: Learnings, Linters/SAST tools
🤖 Prompt for all review comments with 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.
Inline comments:
In `@ipfailover/keepalived/tests/e2e/ipfailover.go`:
- Around line 346-358: Update the step 10 flow around getPodListByLabel and
futurePrimaryPod to wait until the replacement pod is ready and the deleted
new_master pod no longer appears, then compute futurePrimaryPod using the
refreshed pod list while excluding new_master before calling waitForPrimaryPod.
In `@ipfailover/keepalived/tests/e2e/util.go`:
- Around line 180-187: In ipfailover/keepalived/tests/e2e/util.go lines 180-187,
update getPodListByLabel to use whitespace-aware field parsing and assert the
returned pod list is non-empty; in lines 125-143, update
ensureIpfailoverMasterBackup to assert len(podList) is at least 2 before
accessing podList[0] and podList[1].
- Around line 109-110: The e2e extension resolves testdata from relative paths
that are unavailable outside the source tree. Update the testdata path setup in
util.go around baseDir and the ipfailover.go loading paths at lines 96-97,
121-122, 180-181, 252-253, 288-289, and 362-363 to use one embedded-filesystem
or executable-location-based resolution mechanism; replace relative
buildPruningBaseDir usage while preserving the existing file-loading behavior.
- Around line 215-225: Update slicingElement to remove the matching element
without modifying podList’s backing array: construct and return a separate slice
containing all non-matching pods, while preserving the existing logging and
behavior when no element matches.
- Around line 189-213: Update getVipOwnerPod to assert that primaryNode is
non-empty after the pod search completes and before returning it, so failure
occurs when no pod reports the VIP while preserving the existing
successful-owner return path.
- Around line 296-314: Update getPodIP to assert that the returned podIp slice
is non-empty before returning it, including a clear failure message for an
unknown or unsupported IP stack type. Preserve the existing single-stack and
dual-stack collection behavior.
---
Nitpick comments:
In `@ipfailover/keepalived/tests/e2e/ipfailover.go`:
- Around line 388-390: Remove the unused getPodListByLabel call in the “Exclude
VIP '9' from the ipfailover group” test flow, and remove the matching discarded
call in the related utility flow. Keep the surrounding setEnvVariable behavior
unchanged.
- Around line 244-251: Replace the duplicated platform query and normalization
in the test setup with the existing checkPlatform helper, passing the current oc
client. Preserve the Nutanix-specific skip behavior based on the helper’s
returned platform value and remove the local infrastructure query logic.
In `@ipfailover/keepalived/tests/e2e/util.go`:
- Around line 110-113: Update the cleanup in the test utility around
indexTmpPath to call os.RemoveAll instead of spawning rm -rf through
exec.Command. Handle or log the returned cleanup error, and remove the
now-unused os/exec dependency if it is only needed by this call.
In `@ipfailover/keepalived/tests/Makefile`:
- Around line 14-18: Add a test target to the Makefile that runs the supported
end-to-end test and vet commands for the extension. Also provide the required
all target if checkmake enforces it, or configure the project’s check
accordingly, while preserving the existing help, build, and clean targets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| baseDir := "e2e/testdata/router" | ||
| indexTmpPath := filepath.Join(baseDir, getRandomString()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Testdata files are resolved by relative path but are not shipped with the extension binary. Both files build paths from the relative directory e2e/testdata/router. The image contains only /usr/bin/ipfailover-tests-ext.gz, and the extracted binary runs with an arbitrary working directory, so these lookups fail outside the source tree.
ipfailover/keepalived/tests/e2e/util.go#L109-L110: resolvebaseDirfrom an embedded filesystem or from a path derived from the binary location.ipfailover/keepalived/tests/e2e/ipfailover.go#L96-L97: loadipfailover.yamlthrough the same resolution mechanism instead of the relativebuildPruningBaseDir. The same pattern repeats at lines 121-122, 180-181, 252-253, 288-289, and 362-363.
📍 Affects 2 files
ipfailover/keepalived/tests/e2e/util.go#L109-L110(this comment)ipfailover/keepalived/tests/e2e/ipfailover.go#L96-L97
🤖 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 `@ipfailover/keepalived/tests/e2e/util.go` around lines 109 - 110, The e2e
extension resolves testdata from relative paths that are unavailable outside the
source tree. Update the testdata path setup in util.go around baseDir and the
ipfailover.go loading paths at lines 96-97, 121-122, 180-181, 252-253, 288-289,
and 362-363 to use one embedded-filesystem or executable-location-based
resolution mechanism; replace relative buildPruningBaseDir usage while
preserving the existing file-loading behavior.
cc23f7e to
3a24f5b
Compare
|
/test tests-extension |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
3a24f5b to
6de87e3
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
/test tests-extension |
|
@rikatz Kindly review... |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
6de87e3 to
611cd05
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
/test tests-extension |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
ipfailover/keepalived/tests/Makefile (1)
7-7: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winKeep the compiler selection deterministic.
GOTOOLCHAIN=autocan switch to and download another Go toolchain. The Docker builder andgo.modalready target Go 1.25. UseGOTOOLCHAIN=localor an explicitly pinned toolchain unless automatic upgrades are required. Go documents this automatic switching behavior. (go.dev)Proposed fix
- GOTOOLCHAIN=auto GOSUMDB=sum.golang.org go build -o $(BINARY) ./cmd + GOTOOLCHAIN=local GOSUMDB=sum.golang.org go build -o $(BINARY) ./cmd🤖 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 `@ipfailover/keepalived/tests/Makefile` at line 7, Update the go build command in the Makefile to use deterministic compiler selection by replacing GOTOOLCHAIN=auto with GOTOOLCHAIN=local, preserving the existing Go 1.25 target and build flags.Source: MCP tools
🤖 Prompt for all review comments with 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.
Inline comments:
In `@ipfailover/keepalived/tests/e2e/testdata_embed.go`:
- Around line 14-23: The testdata initialization around the package init flow
must materialize ipfailoverTemplate at a stable writable absolute location
rather than relative e2e paths. Use the explicit writable location expected by
the OTE environment, assign that concrete path to
ipfailoverDescription.template, and propagate wrapped errors from MkdirAll,
WriteFile, and both fmt.Fprintf calls instead of warning and continuing; ensure
initialization fails when materialization cannot complete.
- Around line 20-24: Update Init() to validate the destination before use:
inspect it with os.Lstat or equivalent, reject symlinks and non-regular files,
propagate non-ENOENT errors, and compare existing ipfailover.yaml contents with
ipfailoverTemplate, failing on mismatches. For a missing destination, create the
template atomically and report creation errors instead of accepting partial or
stale content.
In `@ipfailover/keepalived/tests/go.mod`:
- Line 48: Update the dependency resolution for the ipfailover/keepalived/tests
module by running govulncheck ./... and addressing the reported vulnerable
transitive packages, including github.com/moby/spdystream,
go.opentelemetry.io/otel/sdk, golang.org/x/crypto, and google.golang.org/grpc.
If the affected Docker, SPDY, SSH-agent, or gRPC-server paths are unreachable,
document the reachable-code exemption explicitly rather than relying on the //
indirect annotation in go.mod.
In `@ipfailover/keepalived/tests/Makefile`:
- Around line 3-8: Add a .PHONY test target to the Makefile that depends on
build, then execute the specified local OTE/Ginkgo coverage command with its
existing arguments. Do not add an all target; keep the current build, clean, and
help targets unchanged.
---
Nitpick comments:
In `@ipfailover/keepalived/tests/Makefile`:
- Line 7: Update the go build command in the Makefile to use deterministic
compiler selection by replacing GOTOOLCHAIN=auto with GOTOOLCHAIN=local,
preserving the existing Go 1.25 target and build flags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| dir := filepath.Join("e2e", "testdata", "router") | ||
| if err := os.MkdirAll(dir, 0755); err != nil { | ||
| fmt.Fprintf(os.Stderr, "warning: could not create testdata dir: %v\n", err) | ||
| return | ||
| } | ||
| dest := filepath.Join(dir, "ipfailover.yaml") | ||
| if _, err := os.Stat(dest); os.IsNotExist(err) { | ||
| if err := os.WriteFile(dest, ipfailoverTemplate, 0644); err != nil { | ||
| fmt.Fprintf(os.Stderr, "warning: could not write embedded testdata: %v\n", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 6 \
'ipfailoverDescription|template[[:space:]]*[:=]|e2e/testdata/router|os\.Chdir|WORKDIR|ENTRYPOINT|CMD' \
ipfailover/keepalived/tests \
ipfailover/keepalived/Dockerfile \
.ci-operator.yamlRepository: openshift/images
Length of output: 50373
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '--- target file exists ---\n'
git ls-files | grep -F 'ipfailover/keepalived/tests/e2e/testdata_embed.go' || true
printf '\n--- relevant non-vendor files ---\n'
git ls-files 'ipfailover/keepalived/tests/e2e/*.go' 'ipfailover/keepalived/Dockerfile' '.ci-operator.yaml' | sed 's#^`#-` #'
printf '\n--- target file ---\n'
if git ls-files | grep -F 'ipfailover/keepalived/tests/e2e/testdata_embed.go' >/dev/null; then
cat -n ipfailover/keepalived/tests/e2e/testdata_embed.go
fi
printf '\n--- util context ---\n'
if git ls-files | grep -F 'ipfailover/keepalived/tests/e2e/util.go' >/dev/null; then
nl -ba ipfailover/keepalived/tests/e2e/util.go | sed -n '260,355p'
fi
printf '\n--- focused workspace/test related patterns ---\n'
rg -n --glob '!ipfailover/keepalived/tests/vendor/**' \
'pwd|working directory|WORKDIR|ENTRYPOINT|CMD|os\.Chdir|RunInWorkspace|go test|TestMain|TestE2E|ipfailoverDescription|ipf\.template|testdata_embed|e2e/testdata/router' \
ipfailover/keepalived/tests \
ipfailover/keepalived/Dockerfile \
.ci-operator.yaml || trueRepository: openshift/images
Length of output: 245
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- target file exists ---'
git ls-files | grep -F 'ipfailover/keepalived/tests/e2e/testdata_embed.go' || true
printf '%s\n' ''
printf '%s\n' '--- relevant non-vendor files ---'
git ls-files 'ipfailover/keepalived/tests/e2e/*.go' 'ipfailover/keepalived/Dockerfile' '.ci-operator.yaml' | sed 's#^`#-` #'
printf '%s\n' ''
printf '%s\n' '--- target file ---'
if git ls-files | grep -F 'ipfailover/keepalived/tests/e2e/testdata_embed.go' >/dev/null; then
cat -n ipfailover/keepalived/tests/e2e/testdata_embed.go
fi
printf '%s\n' ''
printf '%s\n' '--- util context ---'
if git ls-files | grep -F 'ipfailover/keepalived/tests/e2e/util.go' >/dev/null; then
nl -ba ipfailover/keepalived/tests/e2e/util.go | sed -n '260,355p'
fi
printf '%s\n' ''
printf '%s\n' '--- focused workspace/test related patterns ---'
rg -n --glob '!ipfailover/keepalived/tests/vendor/**' \
'pwd|working directory|WORKDIR|ENTRYPOINT|CMD|os\.Chdir|RunInWorkspace|go test|TestMain|TestE2E|ipfailoverDescription|ipf\.template|testdata_embed|e2e/testdata/router' \
ipfailover/keepalived/tests \
ipfailover/keepalived/Dockerfile \
.ci-operator.yaml || trueRepository: openshift/images
Length of output: 1328
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- util.go relevant lines ---'
awk '{printf "%5d %s\n", NR, $0}' ipfailover/keepalived/tests/e2e/util.go | sed -n '260,355p'
printf '%s\n' ''
printf '%s\n' '--- other e2e files outline-ish relevant definitions using grep ---'
rg -n --glob '!ipfailover/keepalived/tests/vendor/**' \
'type ipfailoverDescription|template|func create|createResourceFromTemplate|testdata|router/ipfailover\.yaml' \
ipfailover/keepalived/tests/e2e/ipfailover.go \
ipfailover/keepalived/tests/e2e/util.go \
.ci-operator.yaml \
ipfailover/keepalived/Dockerfile \
|| true
printf '%s\n' ''
printf '%s\n' '--- focused repo paths ---'
awk '{printf "%5d %s\n", NR, $0}' ipfailover/keepalived/tests/e2e/ipfailover.go | sed -n '1,260p'Repository: openshift/images
Length of output: 20308
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- util.go ipfailoverDescription and helper lines ---'
awk '{printf "%5d %s\n", NR, $0}' ipfailover/keepalived/tests/e2e/util.go | sed -n '1,130p'
printf '%s\n' ''
printf '%s\n' '--- ci-operator and docker runner/workdir ---'
awk '{printf "%5d %s\n", NR, $0}' .ci-operator.yaml | sed -n '1,260p'
printf '%s\n' ''
awk '{printf "%5d %s\n", NR, $0}' ipfailover/keepalived/Dockerfile | sed -n '1,220p'Repository: openshift/images
Length of output: 8071
Materialize the embedded template into a stable writable path and fail on write errors.
dir and dest are relative to the process working directory, while the ci-operator build runs make build from tests and the OTE image uses /var/lib/ipfailover as its WORKDIR. If MkdirAll or the first WriteFile fails, init() continues and later tests pass a non-existent relative path to createResourceFromTemplate. Use an explicit writable location, set ipfailoverDescription.template to that concrete path, and return a wrapped error instead of logging and returning. Also propagate errors from the fmt.Fprintf calls.
🤖 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 `@ipfailover/keepalived/tests/e2e/testdata_embed.go` around lines 14 - 23, The
testdata initialization around the package init flow must materialize
ipfailoverTemplate at a stable writable absolute location rather than relative
e2e paths. Use the explicit writable location expected by the OTE environment,
assign that concrete path to ipfailoverDescription.template, and propagate
wrapped errors from MkdirAll, WriteFile, and both fmt.Fprintf calls instead of
warning and continuing; ensure initialization fails when materialization cannot
complete.
Sources: Path instructions, MCP tools
| if _, err := os.Stat(dest); os.IsNotExist(err) { | ||
| if err := os.WriteFile(dest, ipfailoverTemplate, 0644); err != nil { | ||
| fmt.Fprintf(os.Stderr, "warning: could not write embedded testdata: %v\n", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'testdata_embed\.go|util\.go' . || true
echo "== outline target =="
ast-grep outline ipfailover/keepalived/tests/e2e/testdata_embed.go --view compact || true
echo "== stat relevant util section =="
wc -l ipfailover/keepalived/tests/e2e/util.go
sed -n '280,345p' ipfailover/keepalived/tests/e2e/util.go
echo "== target file =="
wc -l ipfailover/keepalived/tests/e2e/testdata_embed.go
cat -n ipfailover/keepalived/tests/e2e/testdata_embed.go
echo "== search ipf.template usage =="
rg -n 'ipf\.template|testdata|generateEmbed|os\.Stat|WriteFile|ReadFile' ipfailover/keepalived/tests/e2e -SRepository: openshift/images
Length of output: 16226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== outline util.go around ipfailoverDescription =="
ast-grep outline ipfailover/keepalived/tests/e2e/util.go --view expanded || true
echo "== util.go relevant section =="
sed -n '80,130p' ipfailover/keepalived/tests/e2e/util.go
echo "== ipfailover.go template and file discovery context =="
sed -n '88,128p' ipfailover/keepalived/tests/e2e/ipfailover.go
sed -n '172,190p' ipfailover/keepalived/tests/e2e/ipfailover.go
sed -n '246,295p' ipfailover/keepalived/tests/e2e/ipfailover.go
sed -n '356,368p' ipfailover/keepalived/tests/e2e/ipfailover.go
echo "== Go os.Stat/WriteFile behavior probe in temp dir =="
tmpdir="$(mktemp -d)"
cat > "$tmpdir/statwriteprobe.go" <<'GO'
package main
import (
"fmt"
"io/fs"
"os"
"path/filepath"
)
func probe(name string, dest string, expectNotExists bool, fn func() error) {
err := fn()
fmt.Println(name)
fmt.Println("error:", errToString(err))
fmt.Println("isNotExist:", err != nil && os.IsNotExist(err))
if expectNotExists {
st, statErr := os.Stat(dest)
fmt.Println("statAfter:", statErrToString(statErr), "isRegular:", isRegular(st), "mode:", st.Mode().String())
content, readErr := os.ReadFile(dest)
fmt.Println("readContent:", content, "readErr:", readErrToString(readErr))
}
}
func errToString(err error) string {
if err == nil {
return "<nil>"
}
return err.Error()
}
func statErrToString(err error) string {
if err == nil {
return "<nil>"
}
return err.Error()
}
func readErrToString(err error) string {
if err == nil {
return "<nil>"
}
return err.Error()
}
func isRegular(info fs.FileInfo) bool {
return info != nil && info.Mode().IsRegular()
}
func main() {
base := filepath.Join(os.TempDir(), "probe-"+fmt.Sprint(os.Getpid()))
os.RemoveAll(base)
os.MkdirAll(base, 0755)
dest := filepath.Join(base, "ipfailover.yaml")
content := []byte("generated-template\n")
probe("missing-file", dest, true, func() error {
if _, err := os.Stat(dest); os.IsNotExist(err) {
return os.WriteFile(dest, content, 0644)
}
return nil
})
probe("existing-regular-file", dest, true, func() error {
if _, err := os.Stat(dest); os.IsNotExist(err) {
return os.WriteFile(dest, content, 0644)
}
return nil
})
probe("permission-errors-ignored", dest, true, func() error {
permfile := filepath.Join(base, "noperm")
if err := os.WriteFile(permfile, []byte("x"), 0); err != nil {
panic(errToString(err))
}
if _, err := os.Stat(permfile); os.IsNotExist(err) {
return os.WriteFile(permfile, content, 0644)
}
return nil
})
proc := filepath.Join(base, "child")
if err := os.WriteFile(proc, []byte("child"), 0644); err != nil {
panic(errToString(err))
}
destSym := filepath.Join(base, "ipfailover.yaml")
if err := os.Symlink(proc, destSym); err != nil {
panic(errToString(err))
}
probe("symlink-exists-path", destSym, true, func() error {
if _, err := os.Stat(destSym); os.IsNotExist(err) {
return os.WriteFile(destSym, content, 0644)
}
return nil
})
st, _ := os.Stat(destSym)
file, _ := os.OpenFile(destSym, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644)
if file != nil {
file.WriteString("symlink-partial\n")
file.Close()
}
newcontent, _ := os.ReadFile(destSym)
fmt.Println("symlink-partial-content-after-init:", string(newcontent))
}
GO
go run "$tmpdir/statwriteprobe.go"Repository: openshift/images
Length of output: 9910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
cat > "$tmpdir/statwriteprobe.go" <<'GO'
package main
import (
"fmt"
"io/fs"
"os"
"path/filepath"
)
func main() {
base := filepath.Join(os.TempDir(), "probe-"+fmt.Sprint(os.Getpid()))
os.RemoveAll(base)
os.MkdirAll(base, 0755)
dest := filepath.Join(base, "ipfailover.yaml")
content := []byte("generated-template\n")
probe := func(name string, dest string, fn func() error) {
fmt.Println(name)
err := fn()
fmt.Println("init_error:", errToString(err))
fmt.Println("isNotExist:", err != nil && os.IsNotExist(err))
st, statErr := os.Stat(dest)
fmt.Println("statAfter:", statErrToString(statErr), "isRegular:", isRegular(st), "mode:", st.Mode().String())
fileContent, readErr := os.ReadFile(dest)
fmt.Println("readContent:", string(fileContent), "readErr:", readErrToString(readErr))
if err == nil && os.IsNotExist(statErr) {
fmt.Println("ERROR path did not create file")
}
}
probe("missing-file", dest, func() error {
if _, err := os.Stat(dest); os.IsNotExist(err) {
return os.WriteFile(dest, content, 0644)
}
return nil
})
probe("existing-regular-file", dest, func() error {
if _, err := os.Stat(dest); os.IsNotExist(err) {
return os.WriteFile(dest, content, 0644)
}
return nil
})
permfile := filepath.Join(base, "noperm")
if err := os.WriteFile(permfile, []byte("x"), 0000); err != nil {
panic(errToString(err))
}
defer os.Chmod(permfile, 0644)
probe("permission-errors-ignored", permfile, func() error {
if _, err := os.Stat(permfile); os.IsNotExist(err) {
return os.WriteFile(permfile, content, 0644)
}
return nil
})
proc := filepath.Join(base, "child")
if err := os.WriteFile(proc, []byte("child"), 0644); err != nil {
panic(errToString(err))
}
destSym := filepath.Join(base, "ipfailover.yaml")
if err := os.Symlink(proc, destSym); err != nil {
panic(errToString(err))
}
probe("symlink-exists-path", destSym, func() error {
if _, err := os.Stat(destSym); os.IsNotExist(err) {
return os.WriteFile(destSym, content, 0644)
}
return nil
})
file, _ := os.OpenFile(destSym, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644)
if file != nil {
if _, err := file.WriteString("symlink-partial\n"); err != nil {
panic(errToString(err))
}
file.Close()
}
pr, ph := os.ReadFile(destSym)
fmt.Println("symlink-partial-content-after-init:", string(pr))
fmt.Println("symlink-partial-readErr:", readErrToString(ph))
}
func errToString(err error) string {
if err == nil {
return "<nil>"
}
return err.Error()
}
func statErrToString(err error) string {
if err == nil {
return "<nil>"
}
return err.Error()
}
func readErrToString(err error) string {
if err == nil {
return "<nil>"
}
return err.Error()
}
func isRegular(info fs.FileInfo) bool {
return info != nil && info.Mode().IsRegular()
}
GO
go run "$tmpdir/statwriteprobe.go"Repository: openshift/images
Length of output: 814
Validate ipfailover.yaml contents before using it.
Init() only writes when os.Stat("e2e/testdata/router/ipfailover.yaml") returns a non-existence error. Existing files, permission errors from previous state, and symlinks are accepted as-is, and no content comparison is done. Tests pass "-f" directly to oc create, so stale, partial, or invalid YAML can be used. Read or os.Lstat the destination first, fail on mismatches, non-regular files, symlinks, and non-ENOENT errors, then create the missing file atomically.
🤖 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 `@ipfailover/keepalived/tests/e2e/testdata_embed.go` around lines 20 - 24,
Update Init() to validate the destination before use: inspect it with os.Lstat
or equivalent, reject symlinks and non-regular files, propagate non-ENOENT
errors, and compare existing ipfailover.yaml contents with ipfailoverTemplate,
failing on mismatches. For a missing destination, create the template atomically
and report creation errors instead of accepting partial or stale content.
Source: Path instructions
| github.com/cyphar/filepath-securejoin v0.6.0 // indirect | ||
| github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect | ||
| github.com/distribution/reference v0.6.0 // indirect | ||
| github.com/docker/docker v28.5.2+incompatible // indirect |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
module_dir="ipfailover/keepalived/tests"
rg -n \
'github.com/docker/docker|github.com/moby/spdystream|go.opentelemetry.io/otel|golang.org/x/crypto|google.golang.org/grpc' \
"$module_dir" --glob '*.go' --glob 'go.mod' || true
echo "Run in a Go-enabled environment:"
echo " (cd $module_dir && govulncheck ./...)"Repository: openshift/images
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Module summary:"
sed -n '1,190p' ipfailover/keepalived/tests/go.mod
echo
echo "Imports of affected modules in non-vendor Go source:"
grep -R -n --include='*.go' --exclude-dir=vendor \
'["/]' ipfailover/keepalived/tests --glob '!ipfailover/keepalived/tests/vendor/**' \
| grep -E 'github\.com/docker/docker|github\.com/moby/spdystream|go\.opentelemetry\.io/otel|golang\.org/x/crypto|google\.golang\.org/grpc|go\.opentelemetry\.io/otel/sdk' || true
echo
echo "Affected module versions in go.mod:"
grep -E 'github.com/docker/docker|github.com/moby/spdystream|golang.org/x/crypto|go\.opentelemetry\.io/otel/sdk|google\.golang\.org/grpc' ipfailover/keepalived/tests/go.mod || trueRepository: openshift/images
Length of output: 10524
🌐 Web query:
OSV GO-2026-4958 go.opentelemetry.io/otel/sdk path hijacking
💡 Result:
The identifier GO-2026-4958 does not correspond to a vulnerability in go.opentelemetry.io/otel/sdk, but rather to an uncontrolled resource consumption vulnerability in github.com/moby/spdystream [1][2]. The vulnerability in go.opentelemetry.io/otel/sdk related to path hijacking is tracked under different identifiers: 1. macOS/Darwin Path Hijacking: This was tracked as CVE-2026-24051 (GHSA-9h8m-3fm2-qjrq) and affected go.opentelemetry.io/otel/sdk versions 1.20.0 through 1.39.0 [3][4]. It was addressed in version 1.40.0 [3]. 2. BSD/Solaris Path Hijacking (Incomplete Fix): This was an incomplete fix for the previous issue, tracked as GO-2026-5426 (CVE-2026-39883, GHSA-hfvc-g4fc-pqhx) [5][6][7][8]. It affected versions 1.15.0 through 1.42.0 and was fixed in version 1.43.0 [6][8][9]. This issue involved the use of a relative path for the kenv command, which could be exploited by an attacker with local access to perform arbitrary code execution [6][8]. If you are looking for information on GO-2026-4958 specifically, it refers to an out-of-memory vulnerability in the SPDY/3 frame parser within github.com/moby/spdystream, which was fixed in version 0.5.1 [1][2][10].
Citations:
- 1: https://pkg.go.dev/vuln/GO-2026-4958
- 2: https://osv.dev/vulnerability/GO-2026-4958
- 3: GHSA-9h8m-3fm2-qjrq
- 4: https://nvd.nist.gov/vuln/detail/CVE-2026-24051
- 5: https://osv.dev/vulnerability/GO-2026-5426
- 6: GHSA-hfvc-g4fc-pqhx
- 7: https://pkg.go.dev/vuln/GO-2026-5426
- 8: https://osv.dev/vulnerability/GHSA-hfvc-g4fc-pqhx
- 9: https://osv.dev/vulnerability/CVE-2026-39883
- 10: Dependency update: govulncheck dapr/dapr#10020
Resolve the vulnerable transitive dependencies or document reachable-code exemptions.
ipfailover/keepalived/tests depends transititively on fixed vulnerable packages: github.com/moby/spdystream v0.5.0, go.opentelemetry.io/otel/sdk v1.37.0, golang.org/x/crypto v0.50.0, and google.golang.org/grpc v1.75.1. Run govulncheck ./... in a Go-enabled environment and, if the affected Docker/SPDY/SSH-agent/gRPC-server code paths are not reachable, document that exemption. Do not rely on // indirect as evidence of safety. (osv.dev)
🤖 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 `@ipfailover/keepalived/tests/go.mod` at line 48, Update the dependency
resolution for the ipfailover/keepalived/tests module by running govulncheck
./... and addressing the reported vulnerable transitive packages, including
github.com/moby/spdystream, go.opentelemetry.io/otel/sdk, golang.org/x/crypto,
and google.golang.org/grpc. If the affected Docker, SPDY, SSH-agent, or
gRPC-server paths are unreachable, document the reachable-code exemption
explicitly rather than relying on the // indirect annotation in go.mod.
Sources: Path instructions, MCP tools, Linters/SAST tools
| .PHONY: build | ||
| build: | ||
| @echo "Building extension binary..." | ||
| @mkdir -p bin | ||
| GOTOOLCHAIN=auto GOSUMDB=sum.golang.org go build -o $(BINARY) ./cmd | ||
| @echo "Binary built: $(BINARY)" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '^(all|test):|go test|go vet|ginkgo|tests-extension|e2e-' \
ipfailover/keepalived/tests/Makefile \
ipfailover/keepalived/tests \
.ci-operator.yaml 2>/dev/null || trueRepository: openshift/images
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Makefile =="
cat -n ipfailover/keepalived/tests/Makefile
echo
echo "== CI/operator files in repo =="
git ls-files | rg '(^|/)\.ci-operator\.ya?ml$|ci|workflow|github/workflows|Makefile|e2e|Ginkgo|tests-extension'
echo
echo "== Target definitions and test commands outside vendor =="
rg -n --glob '!ipfailover/keepalived/tests/vendor/**' '^(all|test|build|clean|help):|go test|go vet|onsi/ginkgo|ginkgo run|ginkgo \.|e2e-|tests-extension|E2E|kind|oc|openshift' .
echo
echo "== .ci-operator.yaml if present =="
if [ -f .ci-operator.yaml ]; then cat -n .ci-operator.yaml; else echo "no .ci-operator.yaml"; fiRepository: openshift/images
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Root .ci-operator.yaml =="
cat -n .ci-operator.yaml
echo
echo "== Non-vendor test commands/targets =="
rg -n --glob '!ipfailover/keepalived/tests/vendor/**' \
--glob '!ipfailover/keepalived/tests/vendor/**' \
'^(all|test|build|clean|help):|go test|go vet|onsi/ginkgo|ginkgo run|ginkgo \.|e2e-|tests-extension|E2E|kind|oc|openshift|keepalived|ipfailover' . \
| head -n 500
echo
echo "== Makefile target list =="
python3 - <<'PY'
import re
from pathlib import Path
p = Path("ipfailover/keepalived/tests/Makefile")
for i, line in enumerate(p.read_text().splitlines(), 1):
if re.match(r'^[A-Za-z0-9_/.-]+\s*:', line) and not line.startswith("\t"):
print(f"{i}: {line.strip()}")
PYRepository: openshift/images
Length of output: 50372
Add a test target for the local E2E path.
ipfailover/keepalived/tests/Makefile only exposes build, clean, and help, so developers cannot run the OTE/Ginkgo suite from this directory. Add a test target that builds the extension and runs coverage via ./bin/ipfailover-tests-ext run-suite ipfailover/all --max-concurrency=-1 -cover -covermode=atomic. Do not add an all target only to satisfy the linter.
🤖 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 `@ipfailover/keepalived/tests/Makefile` around lines 3 - 8, Add a .PHONY test
target to the Makefile that depends on build, then execute the specified local
OTE/Ginkgo coverage command with its existing arguments. Do not add an all
target; keep the current build, clean, and help targets unchanged.
Sources: Learnings, Linters/SAST tools
611cd05 to
6caa66a
Compare
|
/test tests-extension |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ipfailover/keepalived/tests/e2e/testdata_embed.go (1)
19-23:⚠️ Potential issue | 🟠 MajorValidate the destination before using it.
When
os.Statreturns an error other thanos.IsNotExist(err), the code discards that error. Whendestexists, the code accepts its type and contents without checkingipfailoverTemplate. A stale, partial, invalid, or symlinked file can become the deployment template consumed byipfailover.go. Useos.Lstat, handle every error, reject symlinks and non-regular files, compare existing bytes, and atomically create a missing file.As per path instructions, Go code must not ignore error returns.
🤖 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 `@ipfailover/keepalived/tests/e2e/testdata_embed.go` around lines 19 - 23, Update the destination setup around os.Stat to use os.Lstat and handle every returned error explicitly. Reject symlinks and non-regular files, read existing regular files and compare their bytes with ipfailoverTemplate, and atomically create the file when it is missing. Preserve the existing panic context while ensuring all filesystem error returns are checked.Source: Path instructions
🤖 Prompt for all review comments with 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.
Inline comments:
In `@ipfailover/keepalived/tests/go.mod`:
- Around line 6-10: Align the dependency declarations for
github.com/onsi/ginkgo/v2 with the vendored module resolution: either require
the forked github.com/openshift/onsi-ginkgo/v2 version used by vendor, or remove
the fork replacement so direct imports consistently use upstream Ginkgo v2.
Ensure the go.mod require/replace entries and vendored module agree.
---
Duplicate comments:
In `@ipfailover/keepalived/tests/e2e/testdata_embed.go`:
- Around line 19-23: Update the destination setup around os.Stat to use os.Lstat
and handle every returned error explicitly. Reject symlinks and non-regular
files, read existing regular files and compare their bytes with
ipfailoverTemplate, and atomically create the file when it is missing. Preserve
the existing panic context while ensuring all filesystem error returns are
checked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
6caa66a to
94367c1
Compare
|
/test tests-extension |
|
/test e2e-metal-ipi-tests-ext |
|
@melvinjoseph86: The following test 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. |
Summary by CodeRabbit
New Features
Documentation
Chores