update NetworkPolicy e2e test for v1 semantics #46630
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
size/L
|
@danwinship yeah we've got an implementation in the works but not 100% complete yet. Will try this PR out this week hopefully. CC @gunjan5 |
| testCannotConnect(f, ns, "client-a", service, 80) | ||
| testCanConnect(f, ns, "client-b", service, 81) | ||
| }) | ||
|
|
||
| It("shouldn't enforce policy when isolation is off [Feature:NetworkPolicy]", func() { |
There was a problem hiding this comment.
Should we rehash this and test that It("shouldn't enforce policy when no networkpolicy is defined")? At that point it's sort of a basic network test, but could still serve as a useful base case.
There was a problem hiding this comment.
I think that's covered well enough by other tests? Eg, the first test checks that it can connect to the server before adding a default-deny policy, and can't connect after.
There was a problem hiding this comment.
Ok, fair enough. This LGTM, looking forward to trying this with a testable plugin.
|
/approve |
k8s-github-robot
added
the
approved
|
We have cross version e2e tests -- will this work with the previous release? |
|
May want to skip the test cases if they are running against a <1.7 cluster? |
|
The tests aren't run automatically as part of any standard test run (they all still have "[Feature:NetworkPolicy]"), because they require a third-party network plugin. So I don't think the cross-version concerns matter? (At any rate, it depends on the plugin version as much as the kubernetes version, so we wouldn't be able to automatically determine what to do anyway.) |
|
What needs to happen 1.7-wise on this PR? I think we need the milestone set, and maybe cherrypick-candidate? (I am told that because it only affects tests it's not blocked by the code freeze.) |
|
@caseydavenport @ozdanborne FWIW I've tested this against my v1-semantics OpenShift branch now, and it passes, but that doesn't necessarily mean a lot since I could have just implemented the wrong behavior in both places :) |
|
@danwinship we're investigating some e2e failures we're seeing with Calico using this PR - not clear yet if it's on our side or something with the tests :) |
|
@danwinship the tests worked with Calico implementation as well (they were failing due to a DNS error and lack of coffee) :) |
test/e2e/network_policy.go
Outdated
| framework.Failf("unable to cleanup svc %v: %v", service.Name, err) | ||
| } | ||
| }() | ||
| defer cleanupServerPodAndService(f, podServer, service) | ||
| framework.Logf("Waiting for Server to come up.") | ||
| err := framework.WaitForPodRunningInNamespace(f.ClientSet, podServer) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| // Create a pod with name 'client-a', which should be able to communicate with server. |
There was a problem hiding this comment.
nit: the comment says pod 'client-a' but it's 'client-can-connect' in reality (same for a few other comments)
danwinship
force-pushed
the
networkpolicy-test-v1
branch
from
6df07d6
to
a2cb516
Compare
|
fixed comments (I only found two incorrect ones) and repushed |
|
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@thockin can we add this to the v1.7 milestone? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: caseydavenport, danwinship, mikedanese Associated issue: 46625 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue (batch tested with PRs 45877, 46846, 46630, 46087, 47003) |
This makes the NetworkPolicy test at least correct for v1, although ideally we'll eventually add a few more tests... (So this covers about half of #46625.)
I've tested that this compiles, but not that it passes, since I don't have a v1-compatible NetworkPolicy implementation yet...
@caseydavenport @ozdanborne, maybe you're closer to having a testable plugin than I am?
Release note: