Add e2e network policy tests. #35660

ozdanborne · 2016-10-27T00:20:10Z

What this PR does / why we need it:
Add set of e2e tests for Network Policy. This has succesfully run againast a deployment using Calico as the network policy provider.

Specifically, adds a new e2e test file (/test/e2e/network_policy.go) which tests TCP connectivity between pods with isolation.

See #25638 for PR that added NetworkPolicy resource.

This PR is a replacement for #27447 .

This change is

k8s-ci-robot · 2016-10-27T00:20:11Z

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot 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 will still work. Regular contributors should join the org to skip this step.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

googlebot · 2016-10-27T00:20:12Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

caseydavenport

@djosborne Thanks for doing this!

A few nitty comments from me, but overall this looks like a nice set of basic e2es for NetworkPolicy.

caseydavenport · 2016-10-27T16:52:01Z

test/e2e/network_policy.go

+a simple netcat server,
+
+We test the following:
+	-  Namespace ingress isolation set to DefaultDeny


extra leading space on the first bullet.

caseydavenport · 2016-10-27T16:54:25Z

test/e2e/network_policy.go

+	- Policy rules to allow mono-directional TCP connectivity
+	- Policy rules to allow bi-directional TCP connectivity
+
+	TODO:


Not a big fan of leaving TODOs in the code, but not too fussed.

caseydavenport · 2016-10-27T16:56:42Z

test/e2e/network_policy.go

+			}
+		}()
+
+		framework.Logf("Waiting for client-b to run.")


This log doesn't correspond with the equivalent one for client-a above?

framework.Logf("Waiting for client-a to to come up.")

caseydavenport · 2016-10-27T16:59:05Z

test/e2e/network_policy.go

+		setNamespaceIsolation(f, ns, "DefaultDeny")
+
+		By("Creating a simple server.")
+		podServer, service := createServerPod(f, ns, serverName, []int{listeningPort1})


Naming nit: podServer is actually a serverPod

caseydavenport · 2016-10-27T17:02:39Z

test/e2e/network_policy.go

+		By("Creating a network policy for the server which allows traffic from the pod 'client-a'.")
+		policy, err := f.Client.NetworkPolicies(ns.Name).Create(&extensions.NetworkPolicy{
+			ObjectMeta: api.ObjectMeta{
+				Name: "test-policy",


A more descriptive name here would be nice - something like Name: "allow-client-a" maybe?

caseydavenport · 2016-10-27T17:38:34Z

test/e2e/network_policy.go

+		}()
+
+		// Create a pod with name 'client-b', which will attempt to comunicate with the server,
+		//  but should not be able to do to the label-based policy.


extra space at beginning of comment.

s/do to/due to

caseydavenport · 2016-10-27T17:40:05Z

test/e2e/network_policy.go

+			}
+		}()
+
+		// Create a pod with name 'client-a', which should be able to communicate with server.


comment doesn't match the test.

I don't see how that's true? This is the PodSelector test, which allows connections from pods with label "pod-name": "client-a"

D'oh, I somehow put a "not" in the comment when reading it. Sorry...

No, Casey's original comment was correct; line 347 says:

// Create a pod with name 'client-a', which should be able to communicate with server.

and 348 says:

By("Creating client-a in ns-a which should *not* be able to contact the server.")

(or maybe the comment got moved around after new commits were pushed...)

caseydavenport · 2016-10-27T17:41:16Z

test/e2e/network_policy.go

+	for i, port := range ports {
+		// Create the Container
+		containers = append(containers, api.Container{
+			Name:  fmt.Sprintf("%s-container-%d", podName, i),


Instead of naming this after i, could you name it after the port it is listening on?

caseydavenport · 2016-10-27T17:42:15Z

test/e2e/network_policy.go

+			},
+			Ports: []api.ContainerPort{{ContainerPort: int32(port)}},
+		})
+		// Create the Service.


comment doesn't match the code is describes.

caseydavenport · 2016-10-27T17:44:11Z

test/e2e/network_policy.go

+// Create a server pod with a listening container for each port in ports[].
+// Will also assign a pod label with key: "pod-name" and label set to the given podname for later use by the network
+// policy.
+func createServerPod(f *framework.Framework, namespace *api.Namespace, podName string, ports []int) (*api.Pod, *api.Service) {


naming nit: createServerPodAndService

caseydavenport · 2016-10-27T17:51:14Z

@djosborne You need to sign the linuxfoundation CLA.

ozdanborne · 2016-10-28T16:26:16Z

I signed it!

ozdanborne · 2016-10-28T18:14:22Z

Worth noting, I used the etcd container as it was the only one that had latest netcat installed, and doesn't suffer from the dreaded search domain issue with alpine.

Might be worth adding a busybox alternative / fix for e2e tests to use so I don't have to hijack etcd.

caseydavenport · 2016-10-28T19:58:07Z

@kubernetes/sig-network

caseydavenport · 2016-10-28T20:01:28Z

@k8s-bot ok to test

caseydavenport · 2016-10-28T20:11:46Z

@djosborne Looks like you've got some build failures.

W1028 13:10:07.906] test/e2e/network_policy.go:86: f.Client undefined (type *framework.Framework has no field or method Client)
W1028 13:10:07.906] test/e2e/network_policy.go:415: f.Client undefined (type *framework.Framework has no field or method Client)
W1028 13:10:07.906] test/e2e/network_policy.go:432: f.Client undefined (type *framework.Framework has no field or method Client)
W1028 13:10:07.907] test/e2e/network_policy.go:453: f.Client undefined (type *framework.Framework has no field or method Client)
W1028 13:10:07.907] test/e2e/network_policy.go:500: f.Client undefined (type *framework.Framework has no field or method Client)

danwinship · 2016-10-31T21:25:54Z

Worth noting, I used the etcd container as it was the only one that had latest netcat installed, and doesn't suffer from the dreaded search domain issue with alpine.

Might be worth adding a busybox alternative / fix for e2e tests to use so I don't have to hijack etcd.

We also have LaunchWebserverPod() / CheckConnectivityToHost() in test/e2e/framework/util.go for this... (Well, it looks like they're currently unused, but this is what they were intended for; they originally came from the OpenShift network isolation tests.)

danwinship · 2016-10-31T21:30:05Z

test/e2e/network_policy.go

+		err = framework.WaitForPodSuccessInNamespace(f.Client, podClientA.Name, ns.Name)
+		Expect(err).NotTo(HaveOccurred(), "checking Client-A could communicate with server.")
+
+		By("Creating a client-b that should not succesfully connect to the closed port.")


"closed" sounds like it means "has had close() called on it". Something like "blocked" or "isolated" would be better. (And "unblocked"/"unisolated" rather than "opened" before)

danwinship · 2016-10-31T21:46:49Z

test/e2e/network_policy.go

+
+		// Create Server with Service
+		By("Creating a simple server.")
+		serverPod, service := createServerPodAndService(f, ns, serverName, []int{listeningPort1, listeningPort2})


This is the only test case that passes multiple ports to createServerPodAndService, but nothing tests that createServerPodAndService actually works correctly in that case. (If there is a bug in createServerPodAndService such that it only opens the first port, this test would still pass.) And then if that happened, then the test would always pass, even if the NetworkPolicy implementation was broken, because it doesn't distinguish client-b failing because of isolation vs client-b failing because of connection refused.

Simple fix: flip the policy around so port2 is allowed and port1 is denied, and test that instead (and assume that createServerPodAndService isn't likely to be buggy in a way that would let this pass erroneously)
Better fix: test that both listeningPort1 and listeningPort2 work before applying the NetworkPolicy

danwinship · 2016-10-31T21:53:55Z

test/e2e/network_policy.go

+			}
+		}()
+
+		// Create a pod with name 'client-a', which should be able to communicate with server.


No, Casey's original comment was correct; line 347 says:

// Create a pod with name 'client-a', which should be able to communicate with server.

and 348 says:

By("Creating client-a in ns-a which should *not* be able to contact the server.")

(or maybe the comment got moved around after new commits were pushed...)

danwinship · 2016-10-31T21:55:08Z

test/e2e/network_policy.go

+
+		// Create a pod with name 'client-b', which will attempt to comunicate with the server,
+		// but should not be able to do to the label-based policy.
+		By("Creating client-b in ns-b which should be able to contact the server.")


and this comment is backwards too

danwinship · 2016-10-31T22:01:38Z

Additional test possibilities:

"allow all" (eg, last example in https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/network-policy.md)
multiple NetworkPolicies (eg, adding a second NetworkPolicy to a namespace allows additional connections, as opposed to being ignored, or replacing the first NetworkPolicy)
adding a NetworkPolicy object to a namespace without the isolation annotation has no effect (ie, does not implicitly cause a DefaultDeny)?

ozdanborne · 2016-11-02T16:04:28Z

Sorry for the delay, thanks for the review guys. Will revisit and clean up next week.

ozdanborne · 2017-03-14T23:07:43Z

hey @deads2k I've finally done it - all green. Ready to merge?

cc: @caseydavenport

deads2k · 2017-03-15T12:39:30Z

hey @deads2k I've finally done it - all green. Ready to merge?

cc: @caseydavenport

My name got caught during the initial approver storm. I have approved, so when @caseydavenport lgtm's, you'll enter the queue. We're about to fork for 1.6, so I'm inclined to leave this in 1.7.

caseydavenport · 2017-03-15T16:26:42Z

/approve

caseydavenport · 2017-04-03T00:50:25Z

@thockin can we get a lgtm label on this guy so the merge bot will let it through?

thockin · 2017-04-05T17:14:16Z

/lgtm

caseydavenport · 2017-04-05T18:57:08Z

@k8s-bot ok to test

caseydavenport · 2017-04-28T23:24:18Z

@djosborne looks like the rebase broke things:

I0410 16:49:18.147] k8s.io/kubernetes/test/e2e/network_policy.go:22: can't find import: "k8s.io/kubernetes/pkg/apis/meta/v1"

deads2k · 2017-05-01T12:15:22Z

can't find import: "k8s.io/kubernetes/pkg/apis/meta/v1"

metav1 k8s.io/apimachinery/pkg/apis/meta/v1

k8s-ci-robot · 2017-05-01T21:02:32Z

@djosborne: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins GKE smoke e2e	b78057cf8a83a0363f4945659b2ce1140711c206	link	`@k8s-bot cvm gke e2e test this`
Jenkins GCI GKE smoke e2e	b78057cf8a83a0363f4945659b2ce1140711c206	link	`@k8s-bot gci gke e2e test this`
Jenkins GCE e2e	1d05a44701889beaf6b52952212246afce45244f	link	`@k8s-bot cvm gce e2e test this`
Jenkins GCI GCE e2e	1d05a44701889beaf6b52952212246afce45244f	link	`@k8s-bot gci gce e2e test this`
Jenkins non-CRI GCE e2e	1d05a44701889beaf6b52952212246afce45244f	link	`@k8s-bot non-cri e2e test this`

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.

caseydavenport · 2017-05-01T23:25:26Z

/lgtm

k8s-github-robot · 2017-05-01T23:25:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caseydavenport, deads2k, djosborne, thockin

Needs approval from an approver in each of these OWNERS Files:

~~test/OWNERS~~ [deads2k,thockin]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-05-02T00:40:49Z

Automatic merge from submit-queue

fejta · 2017-05-02T15:30:31Z

FYI @caseydavenport @djosborne @deads2k this merge broke the submit queue! Looks like a bad rebase of test_owners.csv

kubernetes/test-infra#2630

Automatic merge from submit-queue NetworkPolicy tests This pulls in the NetworkPolicy tests from kubernetes/kubernetes#35660, and then makes a few changes to get it to work with our extended networking test, and to get it to actually pass given the current state of our NetworkPolicy support; hopefully the last commit will be able to be reverted at some point.

googlebot added the cla: no label Oct 27, 2016

k8s-github-robot assigned spxtr Oct 27, 2016

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Oct 27, 2016

spxtr assigned caseydavenport and unassigned spxtr Oct 27, 2016

caseydavenport mentioned this pull request Oct 27, 2016

Add e2e tests for network policy #27447

Closed

caseydavenport suggested changes Oct 27, 2016

View reviewed changes

ozdanborne force-pushed the e2e-network-policy branch from 764ea65 to 9e36e59 Compare October 27, 2016 23:38

danwinship reviewed Oct 31, 2016

View reviewed changes

ozdanborne force-pushed the e2e-network-policy branch 2 times, most recently from 4fd1006 to 338c9b8 Compare November 10, 2016 01:14

ozdanborne force-pushed the e2e-network-policy branch 2 times, most recently from 6992f2c to fe48e86 Compare November 11, 2016 16:28

apelisse removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016

ozdanborne force-pushed the e2e-network-policy branch from fe48e86 to b405fbc Compare November 11, 2016 18:53

ozdanborne force-pushed the e2e-network-policy branch from 0ffd7f9 to e1706ad Compare March 14, 2017 21:42

deads2k removed their assignment Mar 15, 2017

k8s-ci-robot assigned thockin Apr 5, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2017

grodrigues3 added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 5, 2017

ozdanborne force-pushed the e2e-network-policy branch from e1706ad to 1d05a44 Compare April 10, 2017 16:46

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2017

ozdanborne force-pushed the e2e-network-policy branch 2 times, most recently from bd6d843 to 0a49451 Compare May 1, 2017 20:41

Add e2e network policy tests.

e3762c6

ozdanborne force-pushed the e2e-network-policy branch from 0a49451 to e3762c6 Compare May 1, 2017 21:02

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 1, 2017

k8s-github-robot merged commit 11c5d12 into kubernetes:master May 2, 2017

ozdanborne deleted the e2e-network-policy branch May 2, 2017 00:46

fejta mentioned this pull request May 2, 2017

Revert test_owners.csv after bad rebase #45232

Merged

dcbw added the sig/network Categorizes an issue or PR as relevant to SIG Network. label May 23, 2017

Add e2e network policy tests. #35660

Add e2e network policy tests. #35660

Conversation

ozdanborne commented Oct 27, 2016 • edited by k8s-oncall Loading

k8s-ci-robot commented Oct 27, 2016

googlebot commented Oct 27, 2016

caseydavenport left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caseydavenport commented Oct 27, 2016

ozdanborne commented Oct 28, 2016

ozdanborne commented Oct 28, 2016

caseydavenport commented Oct 28, 2016

caseydavenport commented Oct 28, 2016

caseydavenport commented Oct 28, 2016 • edited Loading

danwinship commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship commented Oct 31, 2016 • edited Loading

ozdanborne commented Nov 2, 2016

ozdanborne commented Mar 14, 2017

deads2k commented Mar 15, 2017

caseydavenport commented Mar 15, 2017

caseydavenport commented Apr 3, 2017

thockin commented Apr 5, 2017

caseydavenport commented Apr 5, 2017

caseydavenport commented Apr 28, 2017

deads2k commented May 1, 2017

k8s-ci-robot commented May 1, 2017 • edited Loading

caseydavenport commented May 1, 2017

k8s-github-robot commented May 1, 2017

k8s-github-robot commented May 2, 2017

fejta commented May 2, 2017

ozdanborne commented Oct 27, 2016 •

edited by k8s-oncall

Loading

caseydavenport commented Oct 28, 2016 •

edited

Loading

danwinship commented Oct 31, 2016 •

edited

Loading

k8s-ci-robot commented May 1, 2017 •

edited

Loading