Policy simplification #670

tgraf · 2017-05-08T22:59:00Z

Fixes: #674

rlenglet · 2017-05-08T23:01:21Z

pkg/policy/api/rule.go

+// accumulated additively
+//
+// The rule is split into an ingress section which contains all rules
+// applicable at ingress, and an egress section applicable at egress.


applicable -> enforced

rlenglet · 2017-05-08T23:02:38Z

pkg/policy/api/rule.go

+// accumulated additively
+//
+// The rule is split into an ingress section which contains all rules
+// applicable at ingress, and an egress section applicable at egress.


I would also specify that for a flow between two endpoints to be allowed by Cilium, it must be allowed by both the egress section of the source endpoint and by the ingress section of the destination endpoint.

That is only true to some extent. Endpoint to endpoint policy is only specified at Ingress. We only do egress specification for things where we do not control the receiving endpoint.

I've added a note to clarify this.

rlenglet · 2017-05-08T23:05:16Z

pkg/policy/api/rule.go

+	// rule. Rules cannot be identified by comment.
+	//
+	// +optional
+	Comment string `json:"comment,omitempty"`


Comment -> Description (?)

I inherited comment from iptables but I like Description better as well, fixed.

rlenglet · 2017-05-08T23:06:05Z

pkg/policy/api/rule.go

+	// If omitted, no rules will be applied at ingress via this rule.
+	//
+	// +optional
+	Ingress *IngressRule `json:"ingress,omitempty"`


This should be a slice, not a single rule.

All members of IngressRule are sliced which saves us to enforce oneOf at IngressRule level. What benefit do you see in one over the over? I'm fine with both actually.

Because the semantics is different.
I would like to be able to specify:

allow from 10/8 to port 80
and

allow from 11/8 to port 443

That's very different from:

allow from "either 10/8 or 11/8" to "either port 80 or port 443"

As mentioned in another comment as well: The datapath does support the coupling of L3 and L4 rules at this point and we have to support the decoupled version anyway for k8s policy.

I'm fine changing this as I said but we can't support the coupling right now.

Actually you could. It would just cost you the calculation of a cartesian product of the k8s L3 rules with the K8s L4 rules in the same k8s policy, to obtain those "coupled" rule versions. It may be costly only in case of complicated k8s policies, e.g. with contain many criteria. I don't think that will be common, nor very costly.

rlenglet · 2017-05-08T23:06:32Z

pkg/policy/api/rule.go

+	// If omitted, no rules will be applied at egress via this rule
+	//
+	// +optional
+	Egress *EgressRule `json:"egress,omitempty"`


This should be a slice, not a single rule.

rlenglet · 2017-05-08T23:15:01Z

pkg/policy/api/rule.go

+	// protocol which the endpoint subject to the rule is allowed to
+	// connect to.
+	//
+	// If omitted and no other IngressRule applies a ToPorts rule on the


Why mentioning "IngressRule" here? Did you mean "EgressRule"?

I meant EgressRule but I believe the paragraph is not required at all for understanding.

rlenglet · 2017-05-08T23:15:23Z

pkg/policy/api/rule.go

+	// protocol which the endpoint subject to the rule is allowed to
+	// connect to.
+	//
+	// If omitted and no other IngressRule applies a ToPorts rule on the


I find this confusing. This looks like a conjunction of rules.
I would prefer if all the rules form a disjunction, i.e. if a rule allows some flows, no addition or removal of any other rule can alter that.

Not sure which paragraph this comment refers to.

rlenglet · 2017-05-08T23:16:23Z

pkg/policy/api/rule.go

+	foo string
+}
+
+// PortProtocol specifices an L4 port with an optional protocol


specifices -> specifies
protocol -> transport protocol

rlenglet · 2017-05-08T23:20:59Z

pkg/policy/api/rule.go

+
+// PortProtocol specifices an L4 port with an optional protocol
+//
+// TODO: allow port range?


It's usually useful. Actually, a list combining individual ports and ranges seems common, e.g.:
["80", "443", "2000-2100"]
The only negative aspect about it is that it requires the data to be passed as strings, but that's a small price to pay.

I agree, I will change it to a string for now so we can easily add ranges later on.

What about:

type PortProtocol struct { Port PortRange } type PortRange stuct{ min uint16 max uint16 } func NewPort(p uint16) *PortRange{ return &PortRange{min: p, max: p} } func NewPortRange(min, max uint16) *PortRange{ return &PortRange{min: min, max: max} } func (p *PortRange) Contains(p uint16) bool { return p.min >= p && p <= p.max }

At least we don't deal with strings

rlenglet · 2017-05-08T23:22:32Z

pkg/policy/api/rule.go

+	Port uint16 `json:"port"`
+
+	// Protocol is the L4 protocol. If omitted, any protocol matches
+	// Accepted values: "tcp", "udp", ""/"any"


In case of ICMP, how can one match on the code / type?
If that's not supported, specify it.

Note added, we do not support matching on ICMP.

What about port 0 with proto any?

Such a rule would never match right now. I've added the case to the Validate() function.

ashwinp

Initial review comments.

ashwinp · 2017-05-08T23:17:54Z

pkg/policy/api/rule.go

+type Rule struct {
+	// EndpointSelector selects all endpoints which should be subject to
+	// this rule. Cannot be empty.
+	EndpointSelector EndpointSelector `json:"selector"`


Please consider renaming this to AppliedEndpoints.

// Mandatory field. Identifies the endpoints where this rule should be applied.
AppliedEndpoints EndpointSelector json:"selector"

I like selector because it is in line with the k8s API which uses the word selector for everything to select objects to apply rules onto.

I see. Thanks.

ashwinp · 2017-05-08T23:19:07Z

pkg/policy/api/rule.go

+	// If omitted, no rules will be applied at ingress via this rule.
+	//
+	// +optional
+	Ingress *IngressRule `json:"ingress,omitempty"`


The field is a pointer to IngressRule, and can hold only 1 ingress rule. The document needs to be fixed, or we need to have a collection of IngressRules here.

comment fixed

ashwinp · 2017-05-08T23:22:36Z

pkg/policy/api/rule.go

+//
+// Either ingres, egress, or both can be provided. If both ingress and egress
+// are omitted, the rule has no effect.
+type Rule struct {


Please consider renaming this to Policy. A Policy can contain multiple rules. Each rule can be of type Egress or Ingress.

That's what it is. The policy is not part of the API. Cilium owns a policy repository which consists of a list of rules which can be modified via the API by adding/removing/changing rules.

Got it. Thanks.

ashwinp · 2017-05-08T23:23:11Z

pkg/policy/api/rule.go

+	// If omitted, no rules will be applied at egress via this rule
+	//
+	// +optional
+	Egress *EgressRule `json:"egress,omitempty"`


Can we simplify this by having a collection of Rules (a new type that only contains match predicates)? A Rule can either be of type Egress or Ingress. The current type Rule can be renamed to Policy.

See the comment from @rlenglet. I'm trying to avoid oneOf as it can't be described and enforced with swagger.

OK, thanks.

ashwinp · 2017-05-08T23:24:08Z

pkg/policy/api/rule.go

+	// endpoints which are whitelisted in fromCIDR.
+	//
+	// +optional
+	FromEndpoints []EndpointSelector `json:"fromEndpoints,omitempty"`


Please consider renaming this to SourceEndpoints

Source has some implications as it could be confused for source addresses. "From" is clearer in that sense, "rule selects b and allows ingress from a"

ashwinp · 2017-05-08T23:26:02Z

pkg/policy/api/rule.go

+	// EndpointSelector which are allowed to communicate with the endpoint
+	// subject to the rule.
+	//
+	// If omitted and no other IngressRule applies a FromEndpoints rule on


Please remove this paragraph. We're referring to fromCIDR, which is defined later, and the comment is redundant as the params are tagged as optional.

Agreed, only obfuscates, removed both occurrences.

ashwinp · 2017-05-08T23:27:05Z

pkg/policy/api/rule.go

+	// +optional
+	FromEndpoints []EndpointSelector `json:"fromEndpoints,omitempty"`
+
+	// FromRequires is a selector which must match in combination with at


I did not understand the purpose of FromRequires. The example about group=A talks about labels, but isn't this field of type endpointselector?

I have reworded the paragraph to make it clearer

ashwinp · 2017-05-08T23:27:28Z

pkg/policy/api/rule.go

+	// ports.
+	//
+	// +optional
+	ToPorts []PortRule `json:"toPorts,omitempty"`


Please consider renaming to destinationPorts

ashwinp · 2017-05-08T23:27:56Z

pkg/policy/api/rule.go

+	// endpoints which are whitelisted in FromEndpoints.
+	//
+	// +optional
+	FromCIDR []CIDR `json:"toCIDR,omitempty"`


Please consider renaming to SourceCIDRs

SourceCIDRs would definitely be mistaken for source address CIDR

2nd thought, this is exactly what we want so I'm making this change.

ashwinp · 2017-05-08T23:29:34Z

pkg/policy/api/rule.go

+// TODO: allow port range?
+type PortProtocol struct {
+	// Port is an L4 port number
+	Port uint16 `json:"port"`


Can we make this:
Ports []uint16 ?
That way, one can specify TCP: [80, 8080], as an example.

See @rlenglet comment to move to strong to support ranges, I like that approach. We could eventually support all of "10", "10, 11", "10-15"

aanm · 2017-05-11T09:03:10Z

pkg/labels/array.go

+func ParseLabelArray(labels ...string) LabelArray {
+	array := make([]*Label, len(labels))
+	for i := range labels {
+		array[i] = ParseLabel(labels[i])


What happens if a label is not valid?

How can a label not be valid?

Good question, since it returns a pointer I was assuming it could be nil

aanm · 2017-05-11T09:38:45Z

pkg/policy/api/rule.go

+	EndpointSelector EndpointSelector `json:"endpointSelector"`
+
+	// Ingress is a list of IngressRule which are enforced at ingress.
+	// If omitted, this rule does not apply at ingress.


For the omitted ones please also add the case where they are empty. "If omitted, or empty,..."

Agreed, will do.

aanm · 2017-05-11T09:40:35Z

pkg/policy/api/rule.go

+	// unique, multiple rules can have overlapping or identical labels.
+	//
+	// +optional
+	Labels labels.LabelArray `json:"group,omitempty"`


I've been thinking about this. What about "Metadata" instead, seems wrong?
Also, why the json variable is called Group? I mean, I would like to see Group instead of Labels in the go struct.

aanm · 2017-05-11T09:43:18Z

pkg/policy/api/selector.go

+// ParseEndpointSelector parses a list of labels in the format of
+// strings and returns an EndpointSelector
+func ParseEndpointSelector(list ...string) EndpointSelector {
+	array := make([]*labels.Label, len(list))


[]*labels.Label -> labels.LabelArray

aanm · 2017-05-11T09:45:03Z

pkg/policy/api/rule.go

+// is entering the endpoint selected by the endpointSelector.
+//
+// - All members of this structure are optional.
+// - All rule types are considered independently


Does it this means all slices in this structure will be "ORed" or "ANDed"?

ORed. Rephrased.

aanm · 2017-05-11T12:54:49Z

pkg/policy/repository.go

+		switch r.canReach(ctx) {
+		// The rule contained a constraint which was not met, this
+		// connection is not allowed
+		case api.Denied:


I don't get this, if the policy has multiple rules, and one of the rules can't be reachable why does that means it is automatically denied?

Shouldn't you first check for all the rules that have its ingress matching the ctx.To and then check if one of them allows receiving from ctx.From?

api.Denied is only returned if it is denied for sure. This only happens for Require rules. If the requirement for a require rule is not met, the potential consumer is never allowed.

aanm · 2017-05-11T12:56:03Z

pkg/policy/repository.go

+}
+
+// AddList inserts a rule into the policy repository
+func (p *Repository) AddList(rules []*api.Rule) error {


Why a pointer to the rule itself?

I've masked it behind a new api.Rules type now so we can easily change this. I don't see a problem with a pointer.

aanm · 2017-05-11T12:56:35Z

pkg/policy/repository.go

+	defer p.Mutex.Unlock()
+
+	// Validate entire rule list first and only append array if
+	// all rules are valid


aanm · 2017-05-11T13:00:24Z

pkg/policy/repository.go

+
+// GetJSON returns all rules of the policy repository as string in JSON
+// representation
+func (p *Repository) GetJSON() string {


~~Why not implement the MarshalJSON itself?~~ because it returns byte and error

Why not ToString() or String() or even GoString()?

Because we return JSON :-) It's not meant to be human readable.

aanm · 2017-05-11T13:02:50Z

pkg/policy/rule.go

+}
+
+func mergeL4Port(r api.PortRule, p api.PortProtocol, proto string, resMap L4PolicyMap) int {
+	fmt := fmt.Sprintf("%s:%d", proto, p.Port)


why not %s/%d instead?

Either is fine, it doesn't matter. Never exposed. Will change this if you look this better.

tgraf · 2017-05-11T20:40:40Z

What about port 0 with proto any?

ashwinp · 2017-05-12T02:07:41Z

pkg/policy/api/rule.go, line 24 at r3 (raw file):

// labels contained in the endpointSelector
//
// Multiple rules are related to each with a logical OR.

related to each?

Comments from Reviewable

ashwinp · 2017-05-12T17:55:07Z

pkg/policy/api/rule.go, line 44 at r4 (raw file):

	// Egress is a list of EgressRule which are enforced at egress.
	// If omitted or empty, this rule does not apply at ingress.

typo: at egress.

Comments from Reviewable

tgraf · 2017-05-12T17:59:36Z

typo: at egress.

Fixed, thanks

ashwinp · 2017-05-12T18:05:13Z

pkg/policy/api/rule.go, line 183 at r4 (raw file):

// rules which must be met.
type PortRule struct {
	// Ports is a lst of L4 port/protocol

typo: list

Comments from Reviewable

ashwinp · 2017-05-12T19:32:57Z

pkg/policy/api/rule.go, line 193 at r4 (raw file):

	// RedirectPort is the L4 port which, if set, all traffic matching the
	// Ports ie being redirected to. Whatever listener behind that port

/ie/is/

Comments from Reviewable

ashwinp · 2017-05-12T21:38:02Z

pkg/policy/api/rule.go

+// - All members of this structure are optional. If omitted or empty, the
+//   member will have no effect on the rule.
+// - All members of this structure are evaluated independently, i.e. L4 ports
+//   allowed with ToPorts do not depend on a match of the FromEndpoints in the


Please update the comment; There is no FromEndpoints in the EgressRule.

ashwinp · 2017-05-12T21:50:17Z

pkg/policy/api/rule.go

+//
+// - All members of this structure are optional. If omitted or empty, the
+//   member will have no effect on the rule.
+// - All members of this structure are evaluated independently, i.e. L4 ports


"All members of this structure are evaluated independently". This would imply that as a user, I cannot enforce the following:

Allow Ingress only from container labeled A (FromEndpoint=A) to port 80.

Allow Ingress only from container labeled B (FromEndpoint=B) to port 8080.

B will be allowed to connect to port 80 in addition to port 8080. Similarly, A will be allowed to connect to port 8080 in addition to port 80. Right?

Correct. Two reasons:

The BPF datapath is currently not capable of enforcing this. L3 endpoints are whitelisted via BPF map. The per endpoint L4 map is another BPF map. Referring to a 2nd BPF map from a 1st BPF map value was only just recently added so we will start supporting this with more recent kernels.

k8s NetworkPolicy also defines this separate notion and we need to able to map k8s NetworkPolicy to our policy.

The idea is to add a field later on which introduces L4 policies which depend on a specific L3 rule. This could be done by adding a new ToPorts inside FromEndpoints or to add a flag to ToPorts which allows to tie the ToPorts to FromEndpoints and FromCIDR within the same IngressRule.

Signed-off-by: Thomas Graf <thomas@cilium.io>

- Replacement of tree structure with list of rules. Within the list, all rules enjoy the same priority - Separation of concerns will be addresses via a different concept introduced later. Users without an interest in separation of concern are not introduce to unnecessary complexity - Removal of deny rules. Require rules provide a great baseline and all use cases so far could be addressed with require rules without requiring implicit denies. - Removal of tree allows to drastically simplify label handling as labels are now always absolute. No need for a label to have a path. Signed-off-by: Thomas Graf <thomas@cilium.io>

Signed-off-by: Thomas Graf <thomas@cilium.io>

- Removes the {path} parameter from the API - Introduces search & delete by labels Signed-off-by: Thomas Graf <thomas@cilium.io>

Signed-off-by: Thomas Graf <thomas@cilium.io>

Fixes sporadic testsuite failures Signed-off-by: Thomas Graf <thomas@cilium.io>

Signed-off-by: Thomas Graf <thomas@cilium.io>

aanm

Really awesome work @tgraf !

tgraf added the wip label May 8, 2017

rlenglet reviewed May 8, 2017

View reviewed changes

ashwinp suggested changes May 8, 2017

View reviewed changes

tgraf force-pushed the policy-simplification branch 8 times, most recently from 8fb64f4 to eaf8c02 Compare May 11, 2017 01:04

aanm reviewed May 11, 2017

View reviewed changes

tgraf closed this May 11, 2017

tgraf reopened this May 11, 2017

tgraf force-pushed the policy-simplification branch 2 times, most recently from e1c1792 to 97013e2 Compare May 12, 2017 09:21

tgraf added kind/bug This is a bug in the Cilium logic. kind/enhancement This would improve or streamline existing functionality. pending-review and removed wip labels May 12, 2017

tgraf force-pushed the policy-simplification branch 4 times, most recently from 4ab023a to 95c3c8b Compare May 12, 2017 17:44

tgraf force-pushed the policy-simplification branch from 95c3c8b to 4346c2d Compare May 12, 2017 17:58

tgraf force-pushed the policy-simplification branch from 4346c2d to 6e3d8fd Compare May 12, 2017 20:50

ashwinp reviewed May 12, 2017

View reviewed changes

tgraf added 9 commits May 13, 2017 15:16

labels: New ParseLabelArray()

6daa040

Signed-off-by: Thomas Graf <thomas@cilium.io>

policy: Adjust code to new simplified policy API

59abde7

Signed-off-by: Thomas Graf <thomas@cilium.io>

policy: Adapt /policy API

60a7573

- Removes the {path} parameter from the API - Introduces search & delete by labels Signed-off-by: Thomas Graf <thomas@cilium.io>

tests: Adjust tests to new policy

a037ef7

Signed-off-by: Thomas Graf <thomas@cilium.io>

endpoint: Better debug messages during policy regeneration

41056a6

Signed-off-by: Thomas Graf <thomas@cilium.io>

Documentation: Remove outdated policy model

8646a7b

Signed-off-by: Thomas Graf <thomas@cilium.io>

tests/monitor: Longer wait times to gather notifications

a31eec4

Fixes sporadic testsuite failures Signed-off-by: Thomas Graf <thomas@cilium.io>

tests: Allow calling monitor_stop on stopped monitor

ec6a666

Signed-off-by: Thomas Graf <thomas@cilium.io>

tgraf force-pushed the policy-simplification branch 5 times, most recently from 337a293 to 9a466b0 Compare May 14, 2017 23:53

tgraf requested a review from aanm May 15, 2017 10:59

tgraf force-pushed the policy-simplification branch 2 times, most recently from b6b5b63 to 42c977e Compare May 15, 2017 11:46

aanm approved these changes May 15, 2017

View reviewed changes

tgraf force-pushed the policy-simplification branch from 42c977e to ec6a666 Compare May 15, 2017 12:23

tgraf merged commit 9eec5e9 into master May 15, 2017

tgraf deleted the policy-simplification branch May 15, 2017 12:39

Policy simplification #670

Policy simplification #670

Conversation

tgraf commented May 8, 2017 • edited

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

rlenglet May 9, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwinp 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

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

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

tgraf commented May 11, 2017

ashwinp commented May 12, 2017

ashwinp commented May 12, 2017

tgraf commented May 12, 2017

ashwinp commented May 12, 2017

ashwinp commented May 12, 2017

Choose a reason for hiding this comment

tgraf commented May 8, 2017 •

edited

rlenglet May 9, 2017 •

edited