SDN-4168: Add IPsec e2e tests #28658
Merged
Conversation
This file contains 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
openshift-ci
bot
added
the
do-not-merge/work-in-progress
pperiyasamy
force-pushed
the
ipsec-e2e-tests
branch
from
cf28de5
to
571f86e
Compare
|
Job Failure Risk Analysis for sha: 571f86e
|
pperiyasamy
force-pushed
the
ipsec-e2e-tests
branch
from
571f86e
to
8f2cb8c
Compare
|
Job Failure Risk Analysis for sha: 8f2cb8c
|
pperiyasamy
force-pushed
the
ipsec-e2e-tests
branch
from
8f2cb8c
to
119a4ed
Compare
|
Job Failure Risk Analysis for sha: 119a4ed
|
pperiyasamy
force-pushed
the
ipsec-e2e-tests
branch
from
119a4ed
to
d10b3a1
Compare
|
Job Failure Risk Analysis for sha: d10b3a1
|
pperiyasamy
force-pushed
the
ipsec-e2e-tests
branch
3 times, most recently
from
2583612
to
e079638
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Job Failure Risk Analysis for sha: e079638
|
pperiyasamy
force-pushed
the
ipsec-e2e-tests
branch
from
e079638
to
8a66229
Compare
|
Job Failure Risk Analysis for sha: 587ab3c
|
pperiyasamy
force-pushed
the
ipsec-e2e-tests
branch
2 times, most recently
from
d043f01
to
8a66229
Compare
|
There are two issues to be addressed with latest change.
|
|
Job Failure Risk Analysis for sha: 8a66229
|
openshift-ci-robot
added
the
jira/valid-reference
|
@pperiyasamy: This pull request references SDN-4168 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 "4.16.0" version, but no target version was set. 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. |
pperiyasamy
force-pushed
the
ipsec-e2e-tests
branch
from
8c389e3
to
d0e4f09
Compare
|
Job Failure Risk Analysis for sha: d0e4f09
|
jcaamano
reviewed
May 3, 2024
| @@ -862,6 +864,20 @@ func areMachineConfigPoolsReadyWithIPsec(oc *exutil.CLI) (bool, error) { | |||
| return masterWithIPsec && workerWithIPsec, nil | |||
| } | |||
|
|
|||
| func areMachineConfigPoolsReadyWithoutIPsec(oc *exutil.CLI) (bool, error) { | |||
There was a problem hiding this comment.
There is something in this code that makes it very difficult to read. Some suggestions:
- Remove method
areWorkerMachineConfigPoolsReady, just as you don't have aareMasterMachineConfigPoolsReady - s/
areMachineConfigPoolReadyWithoutMachineConfig/areMachineConfigPoolsReadyWithoutMachineConfig - s/
isMachineConfigReadyInPools/areMachineConfigPoolsReadyWithMachineConfig - Avoid the
mustExistflag. Have agetMachineConfigPoolsByLabel(oc *exutil.CLI, mcpSelectorLabel string), correctly handle MMatchExpressionthere as well and do somethign like
func areMasterMachineConfigPoolsWithIPsec(oc *exutil.CLI) (bool, error) {
pools , err := getMachineConfigPoolsByLabel(...)
if err != nil {
return false, err
}
return areMachineConfigPoolsReadyWithMachineConfig(pools, ...)
}
and so on...
There was a problem hiding this comment.
This looks perfect and clean. Done.
Thanks @jcaamano .
test/extended/networking/ipsec.go
Outdated
| @@ -193,22 +193,19 @@ func ensureIPsecDisabled(oc *exutil.CLI) error { | |||
| return false, err | |||
| } | |||
| } | |||
| if done { | |||
There was a problem hiding this comment.
Would be good to follow the same order here and in ensureIPsecEnabled on the machine config and cluster operator checks.
Also, I wouldn't reuse ensureIPsecMachineConfigRolloutComplete in ensureIPsecEnabled. Just call areMachineConfigPoolsReadyWithIPsec directly.
| @@ -46,6 +46,7 @@ require ( | |||
| golang.org/x/crypto v0.16.0 | |||
| golang.org/x/net v0.19.0 | |||
| golang.org/x/oauth2 v0.10.0 | |||
| golang.org/x/sync v0.5.0 | |||
There was a problem hiding this comment.
I am still confused. Why is go mod vendor bringing in the machineconfig dependency if it is not referred from go.mod? Ideally, with no changes in go.mod, go mod vendor should not bring anything new? I am probably missing something.
I would keep doing the go mod tidyp afterward however.
pperiyasamy
force-pushed
the
ipsec-e2e-tests
branch
2 times, most recently
from
3201250
to
125282d
Compare
|
Job Failure Risk Analysis for sha: 125282d
|
This is needed to consume machine config APIs for IPsec related e2e tests. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
pperiyasamy
force-pushed
the
ipsec-e2e-tests
branch
from
125282d
to
accf68f
Compare
|
/lgtm |
|
/retest-required |
|
Job Failure Risk Analysis for sha: accf68f
|
|
Job Failure Risk Analysis for sha: accf68f
|
This adds relevant ipsec e2e tests to validate both control plane and dataplane for east west traffic scenario. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
This test applies ipsec configuration on two chosen worker nodes via IPsec North South mechanisam and ensure traffic between those two nodes are ESP encrypted. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
It is appropriate to generate certificates offline with certutil and butane, Then import those certficates into worker nodes via machine config. Hence this commit makes use of such a machine config to import certficates instead of doing it from a pod. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
This commit adds and updates relevant tests for validating node traffic for both east west and north south IPsec configurations. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
This commit has fixes for issues found with ipsec tests while testing with a 4.16 cluster running in AWS. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
pperiyasamy
force-pushed
the
ipsec-e2e-tests
branch
from
accf68f
to
06e240d
Compare
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, jcaamano, pperiyasamy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-aws-ovn-serial |
|
@pperiyasamy: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
openshift-merge-bot
bot
merged commit 539491f
into
openshift:master
22 of 23 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This adds relevant ipsec e2e tests to validate both control plane and dataplane for both east west and north south traffic scenarios.
Run IPsec tests with command:
./openshift-tests run openshift/network/ipsec