Egress firewall support #9227

danwinship · 2016-06-08T15:09:34Z

The origin side of openshift/openshift-sdn#319, for reference. The interesting bits are all over there though.

dcbw · 2016-06-09T17:59:42Z

Travis says you need a gofmt on pkg/sdn/api/v1beta3/types.go, pkg/sdn/api/v1/types.go and pkg/sdn/api/validation/validation_test.go

dcbw · 2016-06-09T17:59:59Z

Otherwise LGTM but somebody with more experience adding API to origin needs to check too.

danwinship · 2016-07-12T17:36:47Z

merged in the old openshift-sdn PR, and updated to add service endpoint filtering; we use the existing endpoint filtering infrastructure to also filter out external endpoints that conflict with firewall rules. (This is a stopgap; eventually we will fix things so that either service rewriting happens in OVS [when we have conntrack support there] or else egress firewalling happens in iptables [when we have a way to make iptables rules namespace-aware].)

danwinship · 2016-07-14T13:43:47Z

@openshift/networking PTAL?

The one customer who has looked at this so far was a bit concerned about having all of the rules on a single object, since they expect to have on the order of one rule per namespace (with lots of namespaces). (Basically, they plan to block all outgoing access by default, and project admins will have to use some [non-OpenShift] tool to indicate the external sites they need access to, and then some script will create the EgressFirewall rules from that.) So, we could change it so that the EgressFirewall object doesn't have a namespace field, and instead you just create objects in each namespace, and then say that the default namespace's rules apply to all namespaces, except that as discussed before in email, I'm dubious of that since nothing else treats the default namespace as being a parent of other namespaces. So maybe we should just ignore this for 3.3 and just make sure it gets dealt with better in the eventual upstream version.

danwinship · 2016-07-14T13:47:28Z

@openshift/api-review PTAL. This adds a new object type EgressFirewall containing namespaced firewall rules for the cluster as a whole. (It does not change any existing APIs.) See https://trello.com/c/iH5IjWI8 for the use cases.

smarterclayton · 2016-07-14T14:24:42Z

How does this differ from NetworkPolicy in intent?

On Jul 14, 2016, at 9:47 AM, Dan Winship notifications@github.com wrote:

@openshift/api-review https://github.com/orgs/openshift/teams/api-review
PTAL. This adds a new object type EgressFirewall containing namespaced
firewall rules for the cluster as a whole. (It does not change any existing
APIs.) See https://trello.com/c/iH5IjWI8 for the use cases.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#9227 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABG_py0ren0-4k6QxX9lJyfK4PKlOH1Iks5qVj3ygaJpZM4IxESt
.

danwinship · 2016-07-14T14:49:13Z

As of kubernetes 1.3, NetworkPolicy only covers ingress policy, not egress policy. There will eventually be upstream egress policy stuff as well, but not in time for OpenShift 1.3 (or probably even 1.4), and based on the conversation that has happened so far, it's impossible to guess what it will look like when it does happen. (Notably, the existing ingress NetworkPolicy stuff is something that project admins opt in to, whereas the functionality implemented in this PR is imposed by the cluster admin, and can't be overridden or modified by project admins, so it seems unlikely to end up being part of the same NetworkPolicy objects.) So the expectation is that, like NetworkNamespaces, this would eventually be deprecated in favor of the upstream solution, but we don't know what or when that will be.

smarterclayton · 2016-07-14T14:59:22Z

Why would cluster admins not be able to choose whether either ingress or
egress policy is applied? Can you describe the use cases where a project
admin would be allowed to set ingress but not egress and vice versa (I just
want to make sure that we need two objects, vs storing this as an
annotation on network policy and making it virtual).

On Thu, Jul 14, 2016 at 10:49 AM, Dan Winship notifications@github.com
wrote:

As of kubernetes 1.3, NetworkPolicy only covers ingress policy, not egress
policy. There will eventually be upstream egress policy stuff as well, but
not in time for OpenShift 1.3 (or probably even 1.4), and based on the
conversation that has happened so far, it's impossible to guess what it
will look like when it does happen. (Notably, the existing ingress
NetworkPolicy stuff is something that project admins opt in to, whereas the
functionality implemented in this PR is imposed by the cluster admin, and
can't be overridden or modified by project admins, so it seems unlikely to
end up being part of the same NetworkPolicy objects.) So the expectation is
that, like NetworkNamespaces, this would eventually be deprecated in favor
of the upstream solution, but we don't know what or when that will be.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#9227 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_pxT7zyw9IPv4BoYMJ6f5U7ogR51fks5qVkxrgaJpZM4IxESt
.

smarterclayton · 2016-07-14T15:25:02Z

pkg/sdn/api/types.go

+	unversioned.TypeMeta
+	kapi.ObjectMeta
+
+	Rules []EgressFirewallRule


Should be in a spec (same as network policy) and Rules should be called Egress []EgressFirewallRule

smarterclayton · 2016-07-14T15:29:13Z

Added some comments about structure assuming this is intended to be roughly in line with the upstream NetworkPolicy. Unless you plan on being wildly divergent, we should have similar patterns and structures.

danwinship · 2016-07-14T17:07:00Z

Why would cluster admins not be able to choose whether either ingress or egress policy is applied?

Because ingress filtering is about protecting your project from other people, but egress filtering is about protecting other people from your project. The entire premise of egress filtering (in this PR) is that the cluster admin doesn't fully trust the project admins. (Eg, in OpenShift Online, maybe there are some
monitoring/cleanup/whatever jobs that run in pods in the cluster but need to have access to some internal Red Hat logging servers, yum repos, etc. But we don't want customer pods to be able to access those servers. So, we create an egress firewall blocking access to those servers, and add an exception for the namespace that the special jobs run in.)

There are other egress filtering use cases where opt-in at the project level does make sense. (Eg, "I know my pod needs to contact github.com, but nothing else, so I want to firewall off everything else so that if hackers break into my pod they can't use it to attack people") But this PR isn't trying to handle that. (And really, that makes more sense as a Pod annotation than a Namespace-level policy...)

danwinship · 2016-07-14T17:23:03Z

Added some comments about structure assuming this is intended to be roughly in line with the upstream NetworkPolicy.

It's not. This isn't intended to handle arbitrary "egress policy". It's just supposed to handle this one use case (namespace-aware firewall rules) as a stop-gap until upstream finishes designing a full egress policy solution, at which point we should hopefully be able to to auto-translate EgressFirewall objects into corresponding upstream policy objects and deprecate EgressFirewall.

Also, I don't think it's safe to assume at this point that the eventual upstream egress filtering solution is going to look anything like (current) NetworkPolicy. (The initial attempt to add egress policy to NetworkPolicy fell apart when someone pointed out that the proposal didn't actually let you implement any of the proposed use cases.)

smarterclayton · 2016-07-14T18:42:52Z

Rather than invent new conventions, use the existing conventions. If
you're going to deprecate the object, that's fine, but we don't add APIs
that are inconsistent. I don't care whether eventual egress network policy
matches what we do here, but don't create something net new that doesn't
look like anything that has gone before.

On Thu, Jul 14, 2016 at 1:23 PM, Dan Winship notifications@github.com
wrote:

Added some comments about structure assuming this is intended to be
roughly in line with the upstream NetworkPolicy.

It's not. This isn't intended to handle arbitrary "egress policy". It's
just supposed to handle this one use case (namespace-aware firewall rules)
as a stop-gap until upstream finishes designing a full egress policy
solution, at which point we should hopefully be able to to auto-translate
EgressFirewall objects into corresponding upstream policy objects and
deprecate EgressFirewall.

Also, I don't think it's safe to assume at this point that the eventual
upstream egress filtering solution is going to look anything like (current)
NetworkPolicy. (The initial attempt to add egress policy to NetworkPolicy
fell apart when someone pointed out that the proposal didn't actually let
you implement any of the proposed use cases.)

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#9227 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_pwKBDeG7R-s0UubUEKUxHd8-dIHMks5qVnB6gaJpZM4IxESt
.

danwinship · 2016-07-21T16:15:12Z

[test]

danwinship · 2016-07-29T17:30:57Z

[test]

pravisankar · 2016-07-29T19:48:21Z

pkg/cmd/cli/describe/printer.go

+	if _, err := fmt.Fprintf(w, "%s\t", n.Namespace); err != nil {
+		return err
+	}
+	if _, err := fmt.Fprintf(w, "%s\n", n.Name); err != nil {


Simplify...if _, err := fmt.Fprintf(w, "%s\t%s\n", n.Namespace, n.Name); err != nil
egressNetworkPolicyColumns contains only one column 'NAME', do we want to add 'NAMESPACE' column?

Hm... somehow I thought this matched the style elsewhere, but I guess not really.
Namespace gets added automatically, although it looks like I didn't handle it quite correctly.

(Also, this was a fix-up that I squashed into the wrong commit, it should have gone into the earlier commit...)

…verse)

pravisankar · 2016-07-29T20:55:59Z

pkg/sdn/plugin/controller.go

+		// Temporarily drop all outgoing traffic, to avoid race conditions while modifying the other rules
+		otx.AddFlow("table=9, reg0=%d, cookie=1, priority=65535, actions=drop", vnid)
+		otx.DeleteFlows("table=9, reg0=%d, cookie=0/1", vnid)
+


Don't we need to drop all existing rules for vnid before adding new rules?
otx.DeleteFlows("table=9, reg0=%d", vnid) [Consider the case where policy is updated]

we do; the comment about dropping all traffic only applies to the AddFlow(); the DeleteFlows() then drops all existing flows for vnid (except for the "drop all traffic" flow which we just added with cookie=1 to mark it as being different from the the other flows)

pravisankar · 2016-07-29T21:14:17Z

LGTM, there will be follow up PR to provide feedback to the user when egress policy is not applied.

pravisankar · 2016-07-29T21:15:29Z

[test]

pravisankar · 2016-07-29T22:39:09Z

Flake #9959
[test]

pravisankar · 2016-07-29T22:39:21Z

[merge]

dcbw · 2016-07-30T15:05:14Z

LGTM re[merge]

openshift-bot · 2016-07-30T18:40:17Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7348/) (Image: devenv-rhel7_4720)

smarterclayton · 2016-07-30T22:43:11Z

[test] #9959

knobunc · 2016-08-01T13:29:52Z

re[merge]

openshift-bot · 2016-08-01T13:41:22Z

Evaluated for origin merge up to 5aaf3cb

stevekuznetsov · 2016-08-01T19:20:43Z

re[test]

openshift-bot · 2016-08-01T20:00:24Z

Evaluated for origin test up to 5aaf3cb

openshift-bot · 2016-08-01T21:25:01Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7316/)

danwinship mentioned this pull request Jun 9, 2016

fix up Endpoint filtering for multitenant isolation #9255

Closed

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2016

danwinship force-pushed the egress-firewall branch from 79b8a40 to 8a49b98 Compare July 12, 2016 17:28

danwinship mentioned this pull request Jul 12, 2016

Egress firewall support openshift/openshift-sdn#319

Closed

danwinship changed the title ~~Egress firewall support [DO NOT MERGE]~~ Egress firewall support Jul 12, 2016

danwinship force-pushed the egress-firewall branch from 8a49b98 to e5aa1a6 Compare July 12, 2016 17:31

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2016

danwinship force-pushed the egress-firewall branch 2 times, most recently from 46c5ba8 to 5a5b4e3 Compare July 13, 2016 17:14

smarterclayton reviewed Jul 14, 2016
View reviewed changes

danwinship force-pushed the egress-firewall branch from 5a5b4e3 to b25c1dd Compare July 21, 2016 15:42

danwinship force-pushed the egress-firewall branch 2 times, most recently from 973dee7 to 5e1e660 Compare July 21, 2016 17:44

danwinship mentioned this pull request Jul 21, 2016

Drop cluster-internal endpoint filtering / pod monitoring #9982

Merged

danwinship force-pushed the egress-firewall branch from 718e1d2 to 0b0dac9 Compare July 29, 2016 17:51

pravisankar reviewed Jul 29, 2016
View reviewed changes

danwinship added 4 commits July 29, 2016 16:52

Add EgressNetworkPolicy

4783dc9

Update generated code

ea0b38d

Make SDN plugin track vnid->Namespaces mapping (in addition to the re…

5aaf3cb

…verse)

Support EgressNetworkPolicy in SDN plugin

2f0255e

pravisankar reviewed Jul 29, 2016
View reviewed changes

danwinship force-pushed the egress-firewall branch from 0b0dac9 to 2f0255e Compare July 29, 2016 21:01

danwinship mentioned this pull request Aug 1, 2016

SDN docs and stuff #10143

Merged

openshift-bot merged commit b26dd1e into openshift:master Aug 2, 2016

danwinship deleted the egress-firewall branch August 2, 2016 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Egress firewall support #9227

Egress firewall support #9227

danwinship commented Jun 8, 2016

dcbw commented Jun 9, 2016

dcbw commented Jun 9, 2016

danwinship commented Jul 12, 2016

danwinship commented Jul 14, 2016

danwinship commented Jul 14, 2016

smarterclayton commented Jul 14, 2016

danwinship commented Jul 14, 2016

smarterclayton commented Jul 14, 2016

smarterclayton Jul 14, 2016

smarterclayton commented Jul 14, 2016

danwinship commented Jul 14, 2016

danwinship commented Jul 14, 2016

smarterclayton commented Jul 14, 2016

danwinship commented Jul 21, 2016

danwinship commented Jul 29, 2016

pravisankar Jul 29, 2016

danwinship Jul 29, 2016

pravisankar Jul 29, 2016

danwinship Jul 29, 2016

pravisankar commented Jul 29, 2016

pravisankar commented Jul 29, 2016

pravisankar commented Jul 29, 2016

pravisankar commented Jul 29, 2016

dcbw commented Jul 30, 2016

openshift-bot commented Jul 30, 2016 •

edited

Loading

smarterclayton commented Jul 30, 2016 via email

knobunc commented Aug 1, 2016

openshift-bot commented Aug 1, 2016

stevekuznetsov commented Aug 1, 2016

openshift-bot commented Aug 1, 2016

openshift-bot commented Aug 1, 2016

Egress firewall support #9227

Egress firewall support #9227

Conversation

danwinship commented Jun 8, 2016

dcbw commented Jun 9, 2016

dcbw commented Jun 9, 2016

danwinship commented Jul 12, 2016

danwinship commented Jul 14, 2016

danwinship commented Jul 14, 2016

smarterclayton commented Jul 14, 2016

danwinship commented Jul 14, 2016

smarterclayton commented Jul 14, 2016

smarterclayton Jul 14, 2016

Choose a reason for hiding this comment

smarterclayton commented Jul 14, 2016

danwinship commented Jul 14, 2016

danwinship commented Jul 14, 2016

smarterclayton commented Jul 14, 2016

danwinship commented Jul 21, 2016

danwinship commented Jul 29, 2016

pravisankar Jul 29, 2016

Choose a reason for hiding this comment

danwinship Jul 29, 2016

Choose a reason for hiding this comment

pravisankar Jul 29, 2016

Choose a reason for hiding this comment

danwinship Jul 29, 2016

Choose a reason for hiding this comment

pravisankar commented Jul 29, 2016

pravisankar commented Jul 29, 2016

pravisankar commented Jul 29, 2016

pravisankar commented Jul 29, 2016

dcbw commented Jul 30, 2016

openshift-bot commented Jul 30, 2016 • edited Loading

smarterclayton commented Jul 30, 2016 via email

knobunc commented Aug 1, 2016

openshift-bot commented Aug 1, 2016

stevekuznetsov commented Aug 1, 2016

openshift-bot commented Aug 1, 2016

openshift-bot commented Aug 1, 2016

openshift-bot commented Jul 30, 2016 •

edited

Loading