USHIFT-7271: Treat all C2CC clusters as remote#7010
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pmtk: This pull request references USHIFT-7271 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. |
WalkthroughThe test runner now fetches host1's kubeconfig and waits for readiness on all three hosts. A new ChangesC2CC shared keyword refactor and suite updates
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Suite as Robot Suite
participant Resource as c2cc.resource
participant Runner as c2cc_common.sh
Runner->>Runner: fetch kubeconfig-a/b/c, wait for readiness
Suite->>Resource: Register All C2CC Clusters
Resource->>Resource: Register cluster-a/b/c as remote clusters
Suite->>Suite: Deploy/Run tests
Suite->>Resource: Teardown All Remote Clusters
Resource->>Resource: Close SSH connections, clear alias list
Suggested labels: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
the
ready-for-human-review
|
[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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/bin/c2cc_common.sh (1)
282-287: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePredictable
/tmpkubeconfig paths flagged by static analysis.Hardcoded
/tmp/kubeconfig-{a,b,c}paths on the remote VM are susceptible to symlink/TOCTOU pre-planting (CWE-377). Usemktempon the remote host instead of a fixed name.🔒 Proposed fix using mktemp
- run_command_on_vm host1 "sudo cp /var/lib/microshift/resources/kubeadmin/${host1_ip}/kubeconfig /tmp/kubeconfig-a && sudo chmod 644 /tmp/kubeconfig-a" - run_command_on_vm host2 "sudo cp /var/lib/microshift/resources/kubeadmin/${host2_ip}/kubeconfig /tmp/kubeconfig-b && sudo chmod 644 /tmp/kubeconfig-b" - run_command_on_vm host3 "sudo cp /var/lib/microshift/resources/kubeadmin/${host3_ip}/kubeconfig /tmp/kubeconfig-c && sudo chmod 644 /tmp/kubeconfig-c" - copy_file_from_vm host1 "/tmp/kubeconfig-a" "${kubeconfig_a}" - copy_file_from_vm host2 "/tmp/kubeconfig-b" "${kubeconfig_b}" - copy_file_from_vm host3 "/tmp/kubeconfig-c" "${kubeconfig_c}" + local tmp_a tmp_b tmp_c + tmp_a=$(run_command_on_vm host1 "mktemp") + tmp_b=$(run_command_on_vm host2 "mktemp") + tmp_c=$(run_command_on_vm host3 "mktemp") + run_command_on_vm host1 "sudo cp /var/lib/microshift/resources/kubeadmin/${host1_ip}/kubeconfig ${tmp_a} && sudo chmod 644 ${tmp_a}" + run_command_on_vm host2 "sudo cp /var/lib/microshift/resources/kubeadmin/${host2_ip}/kubeconfig ${tmp_b} && sudo chmod 644 ${tmp_b}" + run_command_on_vm host3 "sudo cp /var/lib/microshift/resources/kubeadmin/${host3_ip}/kubeconfig ${tmp_c} && sudo chmod 644 ${tmp_c}" + copy_file_from_vm host1 "${tmp_a}" "${kubeconfig_a}" + copy_file_from_vm host2 "${tmp_b}" "${kubeconfig_b}" + copy_file_from_vm host3 "${tmp_c}" "${kubeconfig_c}"🤖 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/bin/c2cc_common.sh` around lines 282 - 287, The kubeconfig copy steps in the c2cc_common.sh flow use predictable /tmp/kubeconfig-a/b/c remote filenames, which should be replaced to avoid symlink/TOCTOU issues. Update the logic around run_command_on_vm and copy_file_from_vm to create each remote kubeconfig path with mktemp on the target VM, then copy from that generated path back to the local kubeconfig variables. Keep the same host1/host2/host3 handling, but remove all fixed /tmp/kubeconfig-* names.Source: Linters/SAST tools
test/suites/c2cc/healthcheck.robot (1)
66-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale Teardown docstring.
Says "clean up kubeconfigs" but
Teardown All Remote Clusters(pertest/resources/c2cc.resource) only closes SSH connections and clears the alias list—no kubeconfig cleanup happens anymore.📝 Proposed fix
Teardown - [Documentation] Close all connections and clean up kubeconfigs. + [Documentation] Close all connections. Teardown All Remote Clusters🤖 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/suites/c2cc/healthcheck.robot` around lines 66 - 68, The `Teardown` documentation in `healthcheck.robot` is stale because `Teardown All Remote Clusters` no longer cleans up kubeconfigs. Update the `[Documentation]` text to match the actual behavior implemented by `Teardown All Remote Clusters` in `c2cc.resource`, which only closes SSH connections and clears the alias list, and remove any mention of kubeconfig cleanup.
🤖 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.
Nitpick comments:
In `@test/bin/c2cc_common.sh`:
- Around line 282-287: The kubeconfig copy steps in the c2cc_common.sh flow use
predictable /tmp/kubeconfig-a/b/c remote filenames, which should be replaced to
avoid symlink/TOCTOU issues. Update the logic around run_command_on_vm and
copy_file_from_vm to create each remote kubeconfig path with mktemp on the
target VM, then copy from that generated path back to the local kubeconfig
variables. Keep the same host1/host2/host3 handling, but remove all fixed
/tmp/kubeconfig-* names.
In `@test/suites/c2cc/healthcheck.robot`:
- Around line 66-68: The `Teardown` documentation in `healthcheck.robot` is
stale because `Teardown All Remote Clusters` no longer cleans up kubeconfigs.
Update the `[Documentation]` text to match the actual behavior implemented by
`Teardown All Remote Clusters` in `c2cc.resource`, which only closes SSH
connections and clears the alias list, and remove any mention of kubeconfig
cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 43ef42f6-30d4-49e5-81eb-ecaa5f893ba4
📒 Files selected for processing (13)
test/bin/c2cc_common.shtest/resources/c2cc.resourcetest/suites/c2cc-ipsec/ipsec.robottest/suites/c2cc/cleanup.robottest/suites/c2cc/connectivity.robottest/suites/c2cc/custom-route-tables.robottest/suites/c2cc/disruptive.robottest/suites/c2cc/dns.robottest/suites/c2cc/healthcheck.robottest/suites/c2cc/infrastructure.robottest/suites/c2cc/probe.robottest/suites/c2cc/reconciliation.robottest/suites/c2cc/sanity.robot
|
/override ci/prow/e2e-aws-tests-bootc-periodic-arm-el10 |
|
/lgtm |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@agullon: This PR has been marked as verified by 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. |
|
@agullon: Overrode contexts on behalf of agullon: ci/prow/e2e-aws-tests-bootc-periodic-arm-el10, ci/prow/e2e-aws-tests-bootc-periodic-el10, ci/prow/e2e-aws-tests-periodic 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 kubernetes-sigs/prow repository. |
|
@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. |
3 participants
Summary by CodeRabbit
New Features
Bug Fixes