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

add NetworkPolicy plugin #12448

Merged
merged 6 commits into from
Jan 21, 2017
Merged

Conversation

danwinship
Copy link
Contributor

This adds a new plugin, redhat/openshift-ovs-networkpolicy, which implements NetworkPolicy (and does not implement traditional openshift-sdn multitenancy). This is considered "tech preview" for 3.5 so it doesn't have to be completely flawless (eg, the pod watching code is currently not very scalable; there's a trello card about that).

Still to do:

  • docs
  • integrate with existing tests/extended/networking tests
  • pull in the upstream NetworkPolicy test case

@openshift/networking PTAL

@knobunc
Copy link
Contributor

knobunc commented Jan 12, 2017

Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

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

LGTM. Really nice, clean, code. Thanks!

@knobunc
Copy link
Contributor

knobunc commented Jan 13, 2017

@dcbw can you find some time to look at this soon please?

@knobunc
Copy link
Contributor

knobunc commented Jan 13, 2017

[test]

@danwinship
Copy link
Contributor Author

Repushed to make the UPSTREAM commit include the entire corresponding upstream commit, and to remove (incorrect) support for string-valued NetworkPolicyPort values.

Note that currently no tests get run with the networkpolicy plugin, so running tests doesn't actually tell you anything (except that the handful of changes outside of networkpolicy.go didn't break anything).

}

func (np *networkPolicyPlugin) watchNetworkPolicies() {
RunEventQueue(np.node.kClient.ExtensionsClient.RESTClient(), NetworkPolicies, func(delta cache.Delta) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have said we need to stop using event queue. Adding more uses of it isn't helping :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "EventQueue" used in pkg/sdn/plugin is a wrapper around DeltaFIFO (#10070) not the old pkg/client/cache/eventqueue.go.

np.lock.Lock()
defer np.lock.Unlock()

namespaces, err := np.node.kClient.Namespaces().List(kapi.ListOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the only network policy that reads namespaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. With the multitenant policy, everything it needs to track is on the NetNamespace object, but here we need to combine Namespace, NetNamespace, and NetworkPolicies.

@danwinship
Copy link
Contributor Author

pushed a fixup to get Services at least minimally working

Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

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

The service changes look good. Do you expect to have the tests implemented soon?

@dcbw
Copy link
Contributor

dcbw commented Jan 18, 2017

LGTM

@knobunc
Copy link
Contributor

knobunc commented Jan 19, 2017

Since this is tech preview and you have to opt in by manually creating the network policy objects, this feels okay to merge to get some mileage on it before the testing and docs stabilize. But we need to get those in ASAP.

@knobunc
Copy link
Contributor

knobunc commented Jan 19, 2017

Test change LGTM

@knobunc
Copy link
Contributor

knobunc commented Jan 19, 2017

[merge]

@knobunc
Copy link
Contributor

knobunc commented Jan 20, 2017

pkg/util/ovs/ovs_test.go:138: not enough arguments in call to New

That was a pain to dig out...

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 0988c04

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13085/) (Base Commit: b46ecc5)

@eparis
Copy link
Member

eparis commented Jan 20, 2017

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 0988c04

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13127/) (Base Commit: 766ada8) (Image: devenv-rhel7_5740)

@openshift-bot openshift-bot merged commit 2cc89a9 into openshift:master Jan 21, 2017
@danwinship danwinship deleted the network-policy branch January 21, 2017 12:31
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.

6 participants