Skip to content

NO-ISSUE: C2CC IPSec test: tcpdump robustness#6845

Open
pmtk wants to merge 3 commits into
openshift:mainfrom
pmtk:c2cc-ipsec-tcpdump-robustness
Open

NO-ISSUE: C2CC IPSec test: tcpdump robustness#6845
pmtk wants to merge 3 commits into
openshift:mainfrom
pmtk:c2cc-ipsec-tcpdump-robustness

Conversation

@pmtk

@pmtk pmtk commented Jun 9, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • Tests
    • Improved IPSec ESP packet capture and verification for cluster tests: simplified timeout handling, per-cluster capture management over SSH, streamlined startup/teardown with shorter waits and a graceful interrupt to end captures, and capture output is now collected and logged before verification to make retrieval more reliable and reduce flakiness.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 9, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@pmtk: This pull request explicitly references no jira issue.

Details

In 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.

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 5fdfd293-78c3-4f1b-848c-517cb47e3aca

📥 Commits

Reviewing files that changed from the base of the PR and between 505d29c and ca26921.

📒 Files selected for processing (1)
  • test/resources/ipsec.resource
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/resources/ipsec.resource

Walkthrough

Two Robot Framework keywords switch tcpdump execution to per-alias SSH connections: Start Tcpdump For ESP On Cluster drops the timeout and launches tcpdump via SSHLibrary.Start Command (sudo) returning the pcap path; Wait For Tcpdump And Verify ESP stops tcpdump with SIGINT and reads command output via SSHLibrary before pcap verification.

Changes

Tcpdump SSH Integration

Layer / File(s) Summary
Tcpdump Process Lifecycle via SSH
test/resources/ipsec.resource
Start Tcpdump For ESP On Cluster removes the timeout argument and starts tcpdump via SSHLibrary.Switch Connection + SSHLibrary.Start Command on the per-alias SSH connection (C2CC_SSH_IDS), deletes any existing pcap, sleeps briefly, and returns the pcap path. Wait For Tcpdump And Verify ESP shortens the capture wait, stops tcpdump with pkill -INT tcpdump, switches to the per-alias SSH connection, reads/logs stdout and stderr via SSHLibrary.Read Command Output, then continues existing pcap-based ESP verification.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • jerpeter1
  • kasturinarra
🚥 Pre-merge checks | ✅ 15
✅ Passed checks (15 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: improving tcpdump robustness in C2CC IPSec tests through modifications to how tcpdump is launched and managed.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed PR contains no Ginkgo tests, only Robot Framework tests in ipsec.resource and ipsec.robot files. The check for stable Ginkgo test names is not applicable.
Test Structure And Quality ✅ Passed PR modifies only Robot Framework resource file (test/resources/ipsec.resource), not Ginkgo test code. The custom check is specific to Ginkgo tests and does not apply.
Microshift Test Compatibility ✅ Passed PR modifies only Robot Framework keywords in test/resources/ipsec.resource; no new Ginkgo e2e tests are added, so MicroShift API compatibility check does not apply.
Single Node Openshift (Sno) Test Compatibility ✅ Passed PR modifies only Robot Framework resource file (ipsec.resource), not Ginkgo e2e tests. Check applies only to new Ginkgo tests.
Topology-Aware Scheduling Compatibility ✅ Passed Changes modify only test resource file (test/resources/ipsec.resource) with Robot Framework keyword definitions. Check requires deployment manifests, operator code, or controllers—not test utilities.
Ote Binary Stdout Contract ✅ Passed PR modifies Robot Framework test resources only. OTE Binary Stdout Contract applies to Go/Ginkgo code, not Robot Framework files.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed This PR modifies Robot Framework resource files, not Ginkgo e2e tests. The check applies only to new Ginkgo tests (It/Describe/etc), which are not present in this PR.
No-Weak-Crypto ✅ Passed PR adds test resource file with no weak crypto, custom implementations, or insecure comparisons of secrets.
Container-Privileges ✅ Passed PR adds K8s manifests with justified privileged settings: network components require kernel access; c2cc needs hostPath access; router references BZ#2007246.
No-Sensitive-Data-In-Logs ✅ Passed tcpdump logs contain only statistics (packet counts/drops). The -w flag writes packets to file, so stderr shows only summary stats without passwords, tokens, or PII.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci Bot requested review from agullon and copejon June 9, 2026 17:40
@openshift-ci

openshift-ci Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pmtk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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 `@test/resources/ipsec.resource`:
- Around line 84-86: Capture and surface tcpdump's stdout/stderr when running
over SSH so failures are visible: after obtaining ${conn_id} from
${C2CC_SSH_IDS}/${alias} and calling SSHLibrary.Switch Connection, read and
store the command output from SSHLibrary.Read Command Output into a variable
(both stdout and stderr if available), log that content (or write to a debug
file) and assert/raise an error if tcpdump reported problems (permission denied,
interface not found, etc.) before continuing to the "no ESP packets" checks;
update the block that uses ${conn_id}, SSHLibrary.Switch Connection, and
SSHLibrary.Read Command Output to capture, log, and fail fast on tcpdump errors.
- Line 75: Update the tcpdump invocation used by SSHLibrary.Start Command so it
includes the packet-buffered flag; specifically, modify the command string
passed to SSHLibrary.Start Command (the line containing "tcpdump -i ${iface} -w
${pcap_file} esp 2>/dev/null sudo=True") to add the "-U" (or
"--packet-buffered") option immediately after "tcpdump" so the pcap file is
flushed per-packet and avoids racy reads.
🪄 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 YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 4df3c100-5c79-40c7-bc6b-60cad2387690

📥 Commits

Reviewing files that changed from the base of the PR and between e84e492 and 522529c.

📒 Files selected for processing (1)
  • test/resources/ipsec.resource

Comment thread test/resources/ipsec.resource Outdated
Comment thread test/resources/ipsec.resource Outdated
@pmtk pmtk force-pushed the c2cc-ipsec-tcpdump-robustness branch from 522529c to 505d29c Compare June 10, 2026 08:49
@pmtk pmtk force-pushed the c2cc-ipsec-tcpdump-robustness branch from 505d29c to ca26921 Compare June 10, 2026 13:09
@openshift-ci

openshift-ci Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

@pmtk: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-tests-bootc-el10 ca26921 link true /test e2e-aws-tests-bootc-el10
ci/prow/e2e-aws-tests-bootc-el9 ca26921 link true /test e2e-aws-tests-bootc-el9
ci/prow/e2e-aws-tests ca26921 link true /test e2e-aws-tests

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants