k8snetworkplumbingwg / multi-networkpolicy-iptables

MultiNetworkPolicy iptable based implementation
Apache License 2.0
13 stars 19 forks source link

End-to-End testing with KinD #21

Closed zeeke closed 1 year ago

zeeke commented 2 years ago

This PR adds some end2end testing with a KinD cluster.

The suite is intended to cover high-level features, while edge cases and special case errofor are demanded to unit tests.

I think they are helpful to work on new features or bug fixing without breaking what is already working.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 2824401035


Totals Coverage Status
Change from base Build 2820429741: 0.0%
Covered Lines: 862
Relevant Lines: 1717

💛 - Coveralls
s1061123 commented 2 years ago

First, thank you for your PR for e2e.

I agree with you about to have e2e testing, but I wonder whether we should have golang e2e test or not. Golang based e2e test is pretty good to test about Kubernetes API or some controller which utilize Kubernetes API, for example, consuming some CRDs and creating/modifying other CRDs dynamically.

Our testing focuses to check the traffic, so I suppose shell script based e2e is pretty enough because to testing the traffic, shell script can express the tests more simpler.

I've checked LoC (line of code) for each implementation (golang e2e and shell script based e2e, of your repo).

Your shell script based e2e: https://github.com/zeeke/multi-networkpolicy-iptables/tree/multinetpolicy-bugs/e2e

[tohayash@sakaki e2e]$ wc -l $(find . -name  "*")
wc: .: Is a directory
      0 .
     16 ./README.md
     64 ./cni-install.yml
    122 ./multi-network-policy-iptables-e2e.yml
     43 ./oneway-test.sh
    204 ./oneway.yml
     97 ./setup_cluster.sh
     30 ./stacked-test.sh
    278 ./stacked.yml
     10 ./teardown.sh
     15 ./get_tools.sh
    879 total

Your PR, golang based e2e code: https://github.com/zeeke/multi-networkpolicy-iptables/tree/e2e-testing/e2e

[tohayash@sakaki e2e]$ wc -l $(find . -name  "*")
wc: .: Is a directory
      0 .
     27 ./README.md
     64 ./cni-install.yml
     18 ./get_tools.sh
    123 ./multi-network-policy-iptables-e2e.yml
     85 ./setup_cluster.sh
     10 ./teardown.sh
wc: ./test: Is a directory
      0 ./test
     51 ./test/e2e_suite_test.go
wc: ./test/model: Is a directory
      0 ./test/model
     94 ./test/model/multinetworkpolicy.go
     54 ./test/model/namespace.go
     57 ./test/model/networkattachmentdefinition.go
     51 ./test/model/pod.go
    516 ./test/multinetworkpolicy_test.go
wc: ./test/reachmatcher: Is a directory
      0 ./test/reachmatcher
    429 ./test/reachmatcher/reachmatcher.go
     13 ./update_image_on_cluster.sh
   1592 total

As a result, golang based e2e has twice lines of code from shell script based. Unfortunately our community is pretty small and we don't have a big resource to maintain the lots of code, hence I prefer small lines of code implementation.

In addition, if we write networkpolicy as pure YAML (not golang), then it might help users to understand how to write multi-networkpolicy code.

So I believe that shell script based e2e could be feasible to implement our test cases. What do you think about it?

zeeke commented 2 years ago

@s1061123 thank you for your feedback.

I definitely that the test suite should be as simple as possible, and sample YAML testable files are an excellent way to document features.

BTW, here are my thoughts about the comparison you did between "script-version" and "go-version" of tests:

  1. "script-version" contains only two test cases (oneway-test.sh and stacked-test.sh), while the "go-version" has 12 test cases. We may talk about how many tests of this kind we want to implement, as they are much slower than unit tests, but I'm at least going to add some others for the IPv6 work, for instance.
  2. Adding a test case with the "go-version" is about 30 LoC, while the "script-version" is about copy-pasting hundreds of YAML lines.
  3. The "script-version" is missing any test framework to make it executable by a CI lane (e.g. shunit2 would do the work). Tests that are not scheduled in CI are likely to decay.

Can we find a tradeoff solution between maintainability and simplicity? Or maybe it's ok to add the shell test fwk and have redundancy in scenarios.

zeeke commented 1 year ago

I think I can refactor these tests to have resources created by *.yaml files, but still using ginkgo as test runner.

In each test we can render pods, NADs and policies from YAML files, so they can be referred by README as examples, and we can still use go to make assertions.

@s1061123 WDYT about it? Can it be a good middle-way solution?

s1061123 commented 1 year ago

Hi,

This repo's E2E test has following requirements:

As far as I know, ginkgo is to test golang code, so to me I still unsure why we need to rewrite in golang/ginkgo, because E2E test is to test end-to-end (create multi-networkpolicy and check the connectivity, in our repo's context).

BTW, this topic (idea to rewrite e2e in golang) is no longer about the PR's code, so how about to file a new issue in the repo and continue to discuss. Let me close PR for that. So let's make new issue for that and continue to discuss.