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

Fix NetworkPolicies allowing from all to *some* (not all) #12972

Merged
merged 1 commit into from
Feb 16, 2017

Conversation

danwinship
Copy link
Contributor

The NetworkPolicy flow-computing code had a short circuit where if you had a policy that allowed all traffic, it wouldn't bother outputting any other rules that might be present to only allow a subset of traffic, since those rules would be unnecessary. But (a) the short-circuit was buggy, in that it got triggered by any rule that accepted traffic from all sources, even if it wasn't supposed to allow traffic to all destinations, and (b) we weren't making any effort to filter out any other redundant rules (eg, if you had two identical copies of the same NetworkPolicy), and (c) there's no particular reason to think the kind of rule redundancy we were special-casing is going to be more likely than any other kind. So this just gets rid of the special case.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1421091

@openshift/networking PTAL

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 thanks

@knobunc
Copy link
Contributor

knobunc commented Feb 15, 2017

[merge]

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@smarterclayton
Copy link
Contributor

Two flakes:

openshift/origin-gce#17
Devicemapper failed to start container - this looks net new

@smarterclayton
Copy link
Contributor

The devicemapper failure looks scary and new, have not seen it before @stevekuznetsov

@stevekuznetsov
Copy link
Contributor

Yeah... @runcom have you seen this?

devmapper: Error activating devmapper device for 'e1b1d28bff425cd5c08b03a07d37ca63de6d88e4d947e51e200cbd61c62284ed-init': devicemapper: Error running deviceCreate (ActivateDevice) dm_task_run failed"

Context is a test like this

@runcom
Copy link
Member

runcom commented Feb 15, 2017

Nope, @rhvgoyal may know more about that devmapper error though

@knobunc
Copy link
Contributor

knobunc commented Feb 15, 2017

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to dcda31f

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/241/) (Base Commit: c8e60fe)

@danwinship
Copy link
Contributor Author

flake #12989, [merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to dcda31f

@danwinship danwinship mentioned this pull request Feb 16, 2017
@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 16, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/268/) (Base Commit: 19aa349) (Image: devenv-rhel7_5925)

@openshift-bot openshift-bot merged commit a42b43a into openshift:master Feb 16, 2017
@danwinship danwinship deleted the np-from-all-to-some branch February 18, 2017 20:01
@rhvgoyal
Copy link

Most likely something is wrong with the thin pool.

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.

7 participants