e2e Network util update: rewrite TestUnderTemporaryNetworkFailure to use nftables #119476
Conversation
k8s-ci-robot
added
kind/failing-test
k8s-ci-robot
added
the
do-not-merge/needs-sig
|
Welcome @mansi1597! |
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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/test-infra repository. |
k8s-ci-robot
added
needs-triage
|
Hi @mansi1597. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
k8s-ci-robot
added
needs-priority
|
Adding label 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/test-infra repository. |
k8s-ci-robot
added
do-not-merge/contains-merge-commits
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mansi1597 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/release-note-none |
k8s-ci-robot
added
release-note-none
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/retest |
|
@danwinship Can you please review this PR related to the issue #119047 |
|
/test pull-kubernetes-e2e-autoscaling-hpa-cpu |
|
/test pull-kubernetes-e2e-gci-gce-autoscaling |
|
/test ci-kubernetes-e2e-gci-gce-autoscaling-migs |
|
@mansi1597: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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 kubernetes/test-infra repository. |
You need to fix your branch to not include a merge commit. If you do |
| e2essh.LogResult(result) | ||
| framework.Failf("Unexpected error: %v", err) | ||
| nftablesRule := fmt.Sprintf("OUTPUT ip daddr %s reject", to) | ||
| dropCmdNftablesRule := fmt.Sprintf("sudo nft add rule ip nat %s", nftablesRule) |
There was a problem hiding this comment.
nftables doesn't have guaranteed pre-existing tables and chains like iptables does. You will need to nft add your own table and base chain (with type filter hook output priority filter)
Also, the all-caps OUTPUT is normal for iptables chain names, but lowercase is more normal with nftables. (But I'm not sure if it will let you use output as a chain name since that's also a keyword? Try and see.)
Also, I have no idea why the existing iptables code is written the way it is, where the command is split up into two pieces (iptablesRule and dropCmd), but it's weird there and it makes even less sense in the nftables case. (Or at least, if you're going to do it that way, then the chain name ("output") should be part of the command, not part of the "rule".)
And you don't need separate dropCmdNftablesRule and dropCmdIptablesRule. Just a single dropCmd that gets overwritten is fine.
There was a problem hiding this comment.
nftables doesn't have guaranteed pre-existing tables and chains like iptables does.
Oh, you have a table named nat with a chain named OUTPUT because of iptables-nft; that's the translation of the iptables OUTPUT chain. But not all hosts will have that (and it's bad style in nftables to use someone else's table anyway).
| nftablesRule := fmt.Sprintf("OUTPUT ip daddr %s reject", to) | ||
| dropCmdNftablesRule := fmt.Sprintf("sudo nft add rule ip nat %s", nftablesRule) | ||
| if result, err := e2essh.SSH(ctx, dropCmdNftablesRule, from, framework.TestContext.Provider); result.Code != 0 || err != nil { | ||
| framework.Logf("Nft table rule failed. Blocking traffic using iptables rule") |
There was a problem hiding this comment.
"nft" is the name of the command-line tool, but say "nftables" when referring to the subsystem.
also, you should log the error. Actually, probably call e2essh.LogResult.
|
|
||
| var undropCmd string | ||
|
|
||
| fetchHandleCmd := fmt.Sprintf("sudo nft -a list ruleset | grep 'ip daddr %s reject' | awk -F'# handle' '{print $2}'", to) |
There was a problem hiding this comment.
The simpler thing would be to just delete the whole table (which can be done by name rather than needing to use a handle.)
Any test using this function would have to be marked [Serial] / [Disruptive] anyway, so you don't have to worry that there might be another parallel caller to BlockNetwork.
| framework.Logf("Nft table rule not found. Unblocking traffic using iptables rule") | ||
| iptablesRule := fmt.Sprintf("OUTPUT --destination %s --jump REJECT", to) | ||
| undropCmd = fmt.Sprintf("sudo iptables --delete %s", iptablesRule) | ||
| } |
There was a problem hiding this comment.
Rather than doing one or the other, it might turn out to be simpler to just always do both? (treating the removal as successful if either command succeeds)
| framework.Failf("Failed to remove the iptable REJECT rule. Manual intervention is "+ | ||
| "required on host %s: remove rule %s, if exists", from, iptablesRule) | ||
| framework.Failf("Failed to remove the REJECT rule. Manual intervention is "+ | ||
| "required on host %s: remove rule using command %s, if exists", from, undropCmd) |
There was a problem hiding this comment.
"if exists" doesn't quite work there now that you switched from the rule to the command
|
@mansi1597: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. 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 kubernetes/test-infra repository. |
This PR updates the Block and Unblock function in e2e network framework util to use Nftable rule for blocking and unblocking traffic and fallback on iptable rule.
What type of PR is this?
/kind failing-test
What this PR does / why we need it:
TestUnderTemporaryNetworkFailure currently uses iptables rules to simulate the network failure, but in the future, nftables will be more widely-supported/widely-available than iptables.
Which issue(s) this PR fixes:
Fixes #119047
Special notes for your reviewer:
Does this PR introduce a user-facing change?
NONE
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: