e2e test cases for basic SCTP testing #88196

janosi · 2020-02-15T21:43:08Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:
This PR is to implement the basic e2e test cases for the SCTP support feature as defined at https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/0015-20180614-SCTP-support.md#basic-tests

Which issue(s) this PR fixes:

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.:

k8s-ci-robot · 2020-02-15T21:43:16Z

Hi @janosi. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

danwinship · 2020-02-20T22:09:43Z

/assign

aojea · 2020-03-01T17:44:01Z

test/e2e/network/util.go

 		if err != nil {
 			framework.Logf("sctp module is not loaded or error occured while executing command %s on node: %v", cmd, err)
 			return false
 		} else {
-			framework.Logf("the sctp module is loaded on node: %v", node.Name)
-			return true
+			for _, line := range strings.Split(result, "\n") {


do this block needs to be inside the else?
it seems that if err != nil it will return so no need to use it

aojea · 2020-03-01T17:49:54Z

test/e2e/network/util.go

-			framework.Logf("the sctp module is loaded on node: %v", node.Name)
-			return true
+			for _, line := range strings.Split(result, "\n") {
+				if !strings.Contains(line, "xt_sctp") && !!strings.Contains(line, "nf_conntrack_proto_sctp") {


sorry if this is a dumb question but if I do execute in my host I have

linux-6my5:~ # lsmod | grep sctp sctp_diag 16384 0 sctp 352256 39 sctp_diag inet_diag 20480 4 raw_diag,tcp_diag,sctp_diag,udp_diag libcrc32c 16384 5 ip_vs,nf_conntrack,xfs,nf_nat,sctp

is this if looking for a line that does not contain xt_sctp and nf_conntrack_proto_sctp?

Well, that if tries to filter out those modules that are not harmful from userspace SCTP stack perspective. But the best would be to check if the sctp module is loaded or not.

Coded a more sophisticated one with regex.

danwinship

Sorry for the delay in getting back to this. This looks ready to go; I just have nitpicks.

danwinship · 2020-04-21T13:47:27Z

test/e2e/network/network_policy.go

@@ -1482,9 +1484,48 @@ var _ = SIGDescribe("NetworkPolicy [LinuxOnly]", func() {
 			})
 			cleanupServerPodAndService(f, podA, serviceA)
 		})
+		ginkgo.It("should allow acces only for SCTP on port 80 [Feature:NetworkPolicy] [Feature:SCTP]", func() {


typo "acces"
but this test name doesn't describe what the test is doing well.
"It should not allow access by TCP when a policy specifies only SCTP" or something like that

danwinship · 2020-04-21T13:50:49Z

test/e2e/network/service.go

+		cs = f.ClientSet
+	})
+
+	ginkgo.It("should serve a basic SCTP service with pod and endpoints", func() {


"serve" makes it sound like this is testing that network traffic actually works, which it is not
"should allow creating a basic SCTP service with pod and endpoints"

danwinship · 2020-04-21T14:10:09Z

test/e2e/network/service.go

+			framework.ExpectNoError(err, "failed to delete service: %s in namespace: %s", serviceName, ns)
+		}()
+
+		err = framework.WaitForService(f.ClientSet, ns, serviceName, true, 5*time.Second, 45*time.Second)


meh. test cases shouldn't have hard-coded time values, they should use constants defined in e2e/framework... But the tests are kind of a mess and there are no obvious constants you could use here... I guess replace 45*time.Second with e2eservice.TestTimeout.

danwinship · 2020-04-21T14:17:58Z

test/e2e/network/service.go

+			framework.ExpectNoError(err, "failed to delete service: %s in namespace: %s", serviceName, ns)
+		}()
+
+		err = framework.WaitForService(f.ClientSet, ns, serviceName, true, 5*time.Second, 45*time.Second)


e2eservice.TestTimeout again

danwinship · 2020-04-21T14:20:48Z

test/e2e/network/service.go

+			framework.Failf("The state of the sctp module has changed due to the test case")
+		}
+	})
+	ginkgo.It("should create a NodePort Service with SCTP ports", func() {


Hm... I'm not sure this is really testing much more than the non-NodePort test. May not be worth having both.

Fixed. Deleted.

danwinship · 2020-04-21T14:22:01Z

test/e2e/network/util.go

+
+// VerifySCTPModuleLoadedOnNodes checks whether any node on the list has the
+// sctp.ko module loaded
+func VerifySCTPModuleLoadedOnNodes(f *framework.Framework, nodes *v1.NodeList) bool {


"Verify" makes it sound like the goal of the function is to ensure that SCTP is loaded, when in fact we want the opposite. Maybe rename to CheckSCTPModuleLoadedOnNodes? Or else VerifySCTPModuleNotLoadedOnNodes but then you'd have to flip the sense of the return value too.

danwinship · 2020-04-21T14:26:30Z

test/e2e/network/util.go

@@ -76,3 +79,27 @@ func newAgnhostPod(name string, args ...string) *v1.Pod {
 		},
 	}
 }
+
+// VerifySCTPModuleLoadedOnNodes checks whether any node on the list has the
+// sctp.ko module loaded


This is probably also a good place to document why we are doing this:

// For security reasons, and also to allow clusters to use userspace SCTP implementations, // we require that just creating an SCTP Pod/Service/NetworkPolicy must not do anything // that would cause the sctp kernel module to be loaded.

janosi · 2020-04-22T11:37:40Z

/retest

Add SCTP NetworkPolicy test.

danwinship · 2020-05-28T20:29:12Z

/lgtm

danwinship · 2020-05-28T22:02:58Z

oops, and
/approved

aojea · 2020-05-28T22:14:47Z

/retest
something odd is happening with kinds jobs and are timing out

janosi · 2020-05-29T07:05:28Z

/retest

janosi · 2020-05-29T12:59:09Z

/retest

danwinship · 2020-05-29T13:50:56Z

sigh
/approve

k8s-ci-robot · 2020-05-29T13:51:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, janosi

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:

~~test/e2e/network/OWNERS~~ [danwinship]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

danwinship · 2020-05-29T13:58:46Z

/hold
@janosi I think that you should clean the git history and squash all commits before merge.

that happened a while back
/hold cancel

janosi · 2020-05-29T16:25:58Z

/retest

janosi · 2020-05-29T17:08:24Z

/retest

janosi · 2020-05-29T17:55:19Z

/retest

janosi · 2020-05-29T20:42:20Z

/retest

fejta-bot · 2020-05-29T23:42:46Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.