USHIFT-6801: C2CC: IPSec test#6792
Open
pmtk wants to merge 5 commits into
Open
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pmtk: This pull request references USHIFT-6801 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 story 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds RHEL9/10 Libreswan-enabled boot images, extracts connectivity/DNS keywords to a shared c2cc resource, adds an ipsec Robot resource with tunnel/XFRM/tcpdump/enforcement helpers, introduces a c2cc-ipsec Robot test suite, and adds EL9/EL10 scenario scripts to provision a three-host Libreswan full-mesh and run the tests. ChangesC2CC IPsec E2E
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/scenarios-bootc/el10/presubmits/el102-src@c2cc-ipsec.sh (1)
157-159: ⚡ Quick winPSK exposure risk is via caller tracing, not
run_command_on_vm
test/bin/scenario.sh’srun_command_on_vmjust forwards the provided command tossh ... "${command}"and does not echo/log the command itself. The PSK embedded insecrets_contentcould still end up in CI logs if the caller runs withset -x/bash tracing around therun_command_on_vm "${host}" "... ${secrets_content} ..."call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/scenarios-bootc/el10/presubmits/el102-src`@c2cc-ipsec.sh around lines 157 - 159, The current call embeds the PSK in the SSH command string (secrets_content) which can leak via caller shell tracing; instead send the secret via SSH stdin or a secure transfer so it never appears in the command line. Update the call that uses run_command_on_vm and secrets_content to stream the secret to the remote file (e.g., use ssh remote 'cat > /etc/ipsec.d/c2cc.secrets' and pipe secrets_content into stdin or use an scp/sftp/temporary file upload), then run run_command_on_vm to set secure perms with sudo chmod 600 /etc/ipsec.d/c2cc.secrets; ensure run_command_on_vm remains unchanged or extend it to accept stdin payloads so the secret is never interpolated into the command string.
🤖 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 125-132: Update the test documentation for "Curl From Host Should
Fail" to accurately describe the setup as IPsec tunnel mode (not Geneve); change
the explanatory lines that currently state "bypasses the Geneve tunnel" and "not
protected by transport-mode IPsec on port 6081" to explain that the scenario
uses type=tunnel (no Geneve between hosts) and that host-originated traffic is
not matched by selectors scoped to local pod/service CIDRs, so host-sourced
traffic to ${pod_ip}:${port} should still fail; keep the test invocation using
Disruptive Command On Cluster and the assertion Should Not Contain ${stdout}
Hello from unchanged.
- Around line 51-56: The keyword "Get XFRM Byte Counters" currently sums the
second awk field (packets) because lines like "84(bytes), 2(packets)" make $2
the packet count; update the Command On Cluster invocation that runs `ip -s xfrm
state | awk ...` to extract and sum the numeric value that precedes "(bytes)"
instead of using $2 — e.g. change the awk program to use a regex match like
/([0-9]+)\(bytes\)/ and add m[1] to sum (or otherwise strip non-digits from the
bytes token) so the returned value is the actual byte total rather than packets.
- Around line 67-76: The keyword Start Tcpdump For ESP On Cluster currently
hardcodes ${iface}=enp1s0 causing captures to miss ESP when the VM's uplink
differs; update the keyword to accept ${iface} from the suite/vars (no hard
default) or implement auto-detection (e.g., determine the primary uplink
interface inside the keyword) and use that value for tcpdump -i; update
references to ${iface}, ${alias}, and ${pcap_file} in the keyword (and callers)
so the test suite can pass a configured interface or rely on the detected uplink
interface.
In `@test/suites/c2cc-ipsec/ipsec.robot`:
- Around line 3-4: The suite docstring incorrectly describes a "Libreswan
transport-mode IPsec mesh (Geneve UDP/6081)" while the implementation
(configure_ipsec_host using type=tunnel and subnet selectors, and its comment
noting no Geneve tunnel) configures tunnel-mode; update the suite
description/docstring to state that this validates a Libreswan tunnel-mode IPsec
mesh (no Geneve) so the docs match the implementation and references to
Geneve/transport-mode are removed or corrected; ensure mentions of
transport-mode or Geneve UDP/6081 are replaced with "tunnel-mode IPsec (subnet
selectors, no Geneve)" and/or other accurate wording near the top-level suite
description.
---
Nitpick comments:
In `@test/scenarios-bootc/el10/presubmits/el102-src`@c2cc-ipsec.sh:
- Around line 157-159: The current call embeds the PSK in the SSH command string
(secrets_content) which can leak via caller shell tracing; instead send the
secret via SSH stdin or a secure transfer so it never appears in the command
line. Update the call that uses run_command_on_vm and secrets_content to stream
the secret to the remote file (e.g., use ssh remote 'cat >
/etc/ipsec.d/c2cc.secrets' and pipe secrets_content into stdin or use an
scp/sftp/temporary file upload), then run run_command_on_vm to set secure perms
with sudo chmod 600 /etc/ipsec.d/c2cc.secrets; ensure run_command_on_vm remains
unchanged or extend it to accept stdin payloads so the secret is never
interpolated into the command string.
🪄 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: 23b820f6-db19-498c-831f-2b7fd9a65af6
📒 Files selected for processing (9)
test/image-blueprints-bootc/el10/layer2-presubmit/group2/rhel102-bootc-source-ipsec.containerfiletest/image-blueprints-bootc/el9/layer2-presubmit/group2/rhel98-bootc-source-ipsec.containerfiletest/resources/c2cc.resourcetest/resources/ipsec.resourcetest/scenarios-bootc/el10/presubmits/el102-src@c2cc-ipsec.shtest/scenarios-bootc/el9/presubmits/el98-src@c2cc-ipsec.shtest/suites/c2cc-ipsec/ipsec.robottest/suites/c2cc/connectivity.robottest/suites/c2cc/dns.robot
💤 Files with no reviewable changes (2)
- test/suites/c2cc/dns.robot
- test/suites/c2cc/connectivity.robot
Member
Author
|
Re: CodeRabbit nitpick — PSK exposure via shell tracing in configure_ipsec_host Won't fix — PSK is ephemeral (openssl rand -hex 32 per test run), VMs are disposable CI infrastructure, and run_command_on_vm does not echo commands. Only explicit set -x around the call would leak it. |
Provides reusable keywords for Libreswan/XFRM operations: - Tunnel verification (single tunnel + mesh count check) - XFRM byte counter baselines and increment assertions - ESP packet capture via background tcpdump - SA flush and verification - IPsec service lifecycle (start/stop/restart/wait) - nftables enforcement rule management (meta ipsec missing) - Negative curl assertions (pod-to-pod and host-to-pod)
Seven test cases validating C2CC connectivity through Libreswan transport-mode IPsec across a 3-cluster mesh: 1. Tunnel establishment — all hosts have 2 tunnels each 2. ESP encapsulation — tcpdump + XFRM byte counter verification 3. Connectivity — pod-to-pod and pod-to-service across all pairs 4. Source IP preservation — no SNAT through the tunnel 5. Policy enforcement — SA flush blocks traffic, restart restores it 6. Plaintext rejection — stopped IPsec + nftables enforcement drops 7. MTU validation — near-MTU payloads through Geneve + ESP Uses the same test workloads (hello-microshift + curl-pod) and multi-cluster keywords (Deploy Test Workloads, etc.) as the existing c2cc suite.
Member
Author
|
/test ? |
Member
Author
|
/test e2e-aws-tests-bootc-arm-el9 e2e-aws-tests-bootc-el10 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/scenarios-bootc/el9/presubmits/el98-src`@c2cc-ipsec.sh:
- Around line 85-87: The command substitutions for host1_ip, host2_ip, and
host3_ip using get_vm_property can hide failures when used directly in local
assignments; change each to first declare the readonly variable (e.g., local -r
host1_ip; or local host1_ip and then mark readonly) and then assign with a
separate command and immediate status check such as host1_ip=$(get_vm_property
host1 ip) || { echo "failed to get host1 ip" >&2; return 1; }, repeating for
host2_ip and host3_ip (use the same pattern for get_vm_property host2 ip and
get_vm_property host3 ip) so any failure causes the script to fail fast.
🪄 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: a5848435-5b6e-4fa1-9eaa-865629a703e7
📒 Files selected for processing (9)
test/image-blueprints-bootc/el10/layer2-presubmit/group2/rhel102-bootc-source-ipsec.containerfiletest/image-blueprints-bootc/el9/layer2-presubmit/group2/rhel98-bootc-source-ipsec.containerfiletest/resources/c2cc.resourcetest/resources/ipsec.resourcetest/scenarios-bootc/el10/presubmits/el102-src@c2cc-ipsec.shtest/scenarios-bootc/el9/presubmits/el98-src@c2cc-ipsec.shtest/suites/c2cc-ipsec/ipsec.robottest/suites/c2cc/connectivity.robottest/suites/c2cc/dns.robot
💤 Files with no reviewable changes (2)
- test/suites/c2cc/connectivity.robot
- test/suites/c2cc/dns.robot
🚧 Files skipped from review as they are similar to previous changes (6)
- test/image-blueprints-bootc/el9/layer2-presubmit/group2/rhel98-bootc-source-ipsec.containerfile
- test/image-blueprints-bootc/el10/layer2-presubmit/group2/rhel102-bootc-source-ipsec.containerfile
- test/resources/ipsec.resource
- test/resources/c2cc.resource
- test/scenarios-bootc/el10/presubmits/el102-src@c2cc-ipsec.sh
- test/suites/c2cc-ipsec/ipsec.robot
Set up a Libreswan transport-mode IPsec mesh across 3 MicroShift clusters. The scenario reuses the C2CC CIDR layout and VM creation from el98-src@c2cc.sh, then installs libreswan/tcpdump via bootc usr-overlay, distributes a shared PSK, writes per-host connection configs scoped to Geneve UDP/6081, and waits for all 6 tunnels (2 per host) to establish. failureshunt=drop / negotiationshunt=drop ensure traffic is never sent in plaintext, which the Robot Framework tests will assert.
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
coderabbitai
Bot
added
the
ready-for-human-review
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/user/howto_c2cc_ipsec.md`:
- Around line 55-57: Add a fenced code language tag (e.g., conf) to each
unlabeled code block to satisfy MD040: update the block containing "192.168.1.10
192.168.1.20 : PSK \"<your-hex-key>\"" to start with ```conf, and likewise label
the blocks beginning with "conn c2cc-to-host-b" and "conn c2cc-to-host-a" by
changing their opening ``` to ```conf so all three blocks are language-tagged.
🪄 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: 6ac57367-453f-4e74-aafc-e5db41019318
📒 Files selected for processing (1)
docs/user/howto_c2cc_ipsec.md
coderabbitai
Bot
removed
the
ready-for-human-review
Member
Author
|
/test e2e-aws-tests-bootc-arm-el10 |
Contributor
|
@pmtk: all tests passed! 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Documentation
Refactor