Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extended Post-installation test framework to support Network Policies #6367

Merged
merged 6 commits into from
Jun 5, 2024

Conversation

kanha-gupta
Copy link
Contributor

It supports All ingress deny policy and All egress deny policy to conduct testing.
Ingress gets applied on echoSameNode and echoOtherNode deployments and Egress is applied on client deployment in order to conduct testing.

@kanha-gupta
Copy link
Contributor Author

kanha-gupta commented May 23, 2024

The framework currently utilises runagnhostcommand function which needs some discussion because networkpolicy tests requires receiving error in order to ensure its working fine and therefore runagnhostcommand gives logs such as :

[kind-kind] Running test: all-ingress-deny-connectivity
[kind-kind] -------------------------------------------------------------------------------------------
[kind-kind] Waiting for Network policy all-ingress-deny to get applied successfully...
[kind-kind] Network policy all-ingress-deny is ready.
[kind-kind] /agnhost command '/agnhost connect echo-same-node:80 --timeout=5s' failed: command terminated with exit code 1
[kind-kind] /agnhost stderr: TIMEOUT

[kind-kind] Network policy is working as expected with Pod test-client-7f76ddc687-s5gc7 and Service echo-same-node
[kind-kind] /agnhost command '/agnhost connect echo-other-node:80 --timeout=5s' failed: command terminated with exit code 1
[kind-kind] /agnhost stderr: TIMEOUT

[kind-kind] Network policy is working as expected with Pod test-client-7f76ddc687-s5gc7 and Service echo-other-node
[kind-kind] Network policy deletion successful

type AllEgressDenyConnectivityTest struct{}

func init() {
RegisterTest("all-egress-deny-connectivity", &AllEgressDenyConnectivityTest{})
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that testsRegistry is map[string]Test, how to guarantee the registered test cases' execution sequences? I suppose this depends on the basic Pod connectivity test?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't reviewed the PR yet, but all tests should be independent and we should not mandate any specific execution order.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense, @kanha-gupta could you check and make sure there is no dependency between each test? In your current implementation, I think your NP tests depends on Pod connectivity tests deployment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @luolanzone, Currently NP tests utilises Pod connectivity deployments that is client pod, echo same node pod and echo other node pod. Each NP test that is conducted first applies NP to required deployment and at the end, it deletes before proceeding to next test. This makes sure No error occurs and no test is executed with NP still applied to deployments. This would also help when we expand the number of NP test cases.
Should we create new deployments for NP ? I believe it would follow the same format of 2 deployments on first node and 1 deployment on second node ?

pkg/antctl/raw/check/policy.go Outdated Show resolved Hide resolved
pkg/antctl/raw/check/policy.go Outdated Show resolved Hide resolved
pkg/antctl/raw/check/policy.go Outdated Show resolved Hide resolved
pkg/antctl/raw/check/policy.go Outdated Show resolved Hide resolved
pkg/antctl/raw/check/policy.go Outdated Show resolved Hide resolved
pkg/antctl/raw/check/policy.go Outdated Show resolved Hide resolved
pkg/antctl/raw/check/installation/test_allegressdeny.go Outdated Show resolved Hide resolved
pkg/antctl/raw/check/installation/test_allegressdeny.go Outdated Show resolved Hide resolved
Signed-off-by: Kanha gupta <[email protected]>
Comment on lines 91 to 93
if err != nil {
return fmt.Errorf("error creating NetworkPolicy: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if err != nil {
return fmt.Errorf("error creating NetworkPolicy: %w", err)
}
if err != nil {
// Handle the case where the NetworkPolicy already exists
if errors.IsAlreadyExists(err) {
fmt.Fprintf(os.Stdout, "NetworkPolicy %s already exists in namespace %s\n", networkPolicy.Name, namespace)
return nil
}

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's worth checking this specific error as this is in a random namespace, having such check doesn't seem useful. Besides, none of other creations have performed such check, there is no reason to make NetworkPolicy special.

pkg/antctl/raw/check/installation/policy.go Outdated Show resolved Hide resolved
pkg/antctl/raw/check/installation/policy.go Outdated Show resolved Hide resolved
pkg/antctl/raw/check/installation/policy.go Outdated Show resolved Hide resolved
pkg/antctl/raw/check/installation/policy.go Outdated Show resolved Hide resolved
Comment on lines 91 to 93
if err != nil {
return fmt.Errorf("error creating NetworkPolicy: %w", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's worth checking this specific error as this is in a random namespace, having such check doesn't seem useful. Besides, none of other creations have performed such check, there is no reason to make NetworkPolicy special.

pkg/antctl/raw/check/installation/test_egressdenyall.go Outdated Show resolved Hide resolved
pkg/antctl/raw/check/installation/test_egressdenyall.go Outdated Show resolved Hide resolved
Signed-off-by: Kanha gupta <[email protected]>
@kanha-gupta
Copy link
Contributor Author

Hey mentors,
Changes have been pushed.

  • removed policy.go as some functions are removed and some are put in test files

Signed-off-by: Kanha gupta <[email protected]>
@kanha-gupta
Copy link
Contributor Author

Hey mentors, Changes have been pushed :)

Signed-off-by: Kanha gupta <[email protected]>
@kanha-gupta
Copy link
Contributor Author

Changes are pushed :)
also when tests are being executed, runAgnhostcommand function is used to execinpod and currently It takes around 12 seconds in my local environment to complete the test as we need error to determine the NP tests are successful.I believe it is because of the timeout period ?
is there a way to improve this ?

@antoninbas
Copy link
Contributor

Changes are pushed :) also when tests are being executed, runAgnhostcommand function is used to execinpod and currently It takes around 12 seconds in my local environment to complete the test as we need error to determine the NP tests are successful.I believe it is because of the timeout period ? is there a way to improve this ?

Yes, please use the --timeout flag to set a smaller timeout, e.g., 3s
See documentation for the connect command: https://pkg.go.dev/k8s.io/kubernetes/test/images/agnhost#section-readme

@kanha-gupta
Copy link
Contributor Author

kanha-gupta commented Jun 4, 2024

Changes are pushed :) also when tests are being executed, runAgnhostcommand function is used to execinpod and currently It takes around 12 seconds in my local environment to complete the test as we need error to determine the NP tests are successful.I believe it is because of the timeout period ? is there a way to improve this ?

Yes, please use the --timeout flag to set a smaller timeout, e.g., 3s See documentation for the connect command: https://pkg.go.dev/k8s.io/kubernetes/test/images/agnhost#section-readme

Thanks a lot :)
Changes are pushed

@antoninbas
Copy link
Contributor

/skip-all

@antoninbas antoninbas merged commit 5af9311 into antrea-io:main Jun 5, 2024
51 of 54 checks passed
@kanha-gupta kanha-gupta deleted the network-policies-support branch June 5, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants