Add support for romana policy spec V2 to romana-policy-agent

[flashvoid]

Implemented

• Support for new policy spec defined last week.
• RESTish requests. No more method in a request - POST to create, DELETE to delete, GET/PUT is not supported.
• Discover network config from network policy object, no more hardcoding.

  Current limitations and questions.

• Can't express network isolation policy with default spec, special rule { "isolated" : True } added for now.
• Don't know how to handle ICMP rules. ICMP types are defined in rules section while some types need to be applied on emitter and others on a receiver.

[jbrendel]

I don't know why isolation has to be a special case?

You already have a chain per tenant. At the end of this is a default DROP. So, there you have default isolation for all tenants/namespaces. If isolation==off then our k8slistener should propagate a 'allow-all' policy for the tenant.

We can easily have tenant-only policies (without segment) by inserting them into the tenant chain. We are currently adding the per-segment rules (of a tenant) into that tenant's chain. But at the end of that chain (and just before the default DROP) we can have any tenant-only policies.

[jbrendel]

Also, could you please explain the concern with ICMP? Since we currently do a default-accept for all egress, which rules would have to be defined on the emitter?

[flashvoid]

@jbrendel on isolation. At the end of this is a default DROP. So, there you have default isolation for all tenants/namespaces. If isolation==off then our k8slistener should propagate a 'allow-all' policy for the tenant. And that is exactly what this code is doing. The question is - how policy definition should look like to express a desire to create tenant wide allow policy.

The problem is not architectural, we can and we will have per-tenant policies. There is a problem with implementation, though. Function make_rules assumes segment id is defined, we would have to introduce another function or another execution flow in existing function to handle non-defined segment IDs. So, it converges to - "check for segment id, if present then do usual stuff else do some special stuff". That is what I was referring to talking about the special case.

The second topic is how to express isolation policy using current policy spec. To be specific we want to get -A ROMANA-T1-WIDE -j ACCEPT rule. In this rule, we can work out tenant id because it's provided and WIDE because no segment id provided. How to express -j ACCEPT using peers : [] and rules : []? The current spec clearly has no way to express such rule so, I've introduced a new term in rules called isolation. Open to any suggestions.

[flashvoid]

@jbrendel Also, could you please explain the concern with ICMP? Since we currently do a default-accept for all egress, which rules would have to be defined on the emitter? Receiver need permissions to receive requests and emitter need permissions to receive replays.

[jbrendel]

About ICMP: What the sender does or doesn't allow is none of our concern, though. If someone wishes to receive responses to their pings then they better allow ICMP Echo reply. Just because we allow incoming Echo requests doesn't mean we have to worry about whether the sender of those requests is configured to receive responses.

[flashvoid]

@jbrendel

Just because we allow incoming Echo requests doesn't mean we have to worry about whether the sender of those requests is configured to receive responses.

This is not how we treat TCP and UDP. For each port we open on receiver we also create a rule on emitter that allows requests from this port to reach it.

[jbrendel]

TCP: Hm. Maybe we are talking past each other? When you talk about 'emitter', you mean the initiator of the TCP connection? If we are a server, we need to allow incoming port 80. We send our response (no egress filtering at the moment). It's up to the one starting the connection to ensure that our response packets come back. We certainly do NOT set anything on the emitter. We can't anyway, since in some cases the emitter may not even be under our control.

IF the initiator of the TCP connection is also under our control then we might have to do something. However, that is a slightly separate problem. I don't think that just because in one tenant/segment we allow incoming port 80 that at that moment we need to automatically ensure that the responses can reach back to the sender. There should be an explicit policy that allows that, if they want to receive such connections. Something like "ingress->TCP->src-port:80". If this can be made smarter with the correct application of 'related' then even better. Maybe with something like this: "-p tcp -m conntrack --ctstate RELATED -j ACCEPT" ? That should work even without port and therefore should allow return packets for any outgoing TCP connection.

And maybe that's exactly what we should allow.

We can do the same for UDP and I think even for ICMP.

[jbrendel]

Just want to re-iterate: Creating ingress rules for tenant/segment "A" (for traffic coming from tenant/segment "B") does NOT mean that we are obliged to create any symmetric rules for tenant/segment "B".

The 'cstate related' one above is more like a general rule that we may wish to implement by default.

[jbrendel]

About isolation: You can express "from anyone" in "peers" via "0.0.0.0/0". Even if we don't have full support for CIDRs yet, you can already add support for that one, specific CIDR.

As far as the rules is concerned: Ok, good point. I would suggest we add the protocol "all", which does not require any ports or anything.

[flashvoid]

Ok, last commit implements default chains per tenant and default -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT everywhere.

This probably a good thing for now but it will bite us hard during egress implementation.

[jbrendel]

Yeah, about the egress. I think you just need to put the egress filter BEFORE the conntrack rule and the outgoing traffic would be properly filtered... But we'll cross that bridge when we come to it.

[flashvoid]

Proposal 1: change policy spec

Goal is to introduce semantics that will allow us to express -A ROMANA-T1 -j ACCEPT rule.

As any other policy semantics, we need to be able to express 4 different components in each rule.

                        Source                        Action
+-------------------vvvvvvvvvvvvv-------------------vvvvvvvvvv-+
|                                                              |
|    -A ROMANA-T1-S2 -s 10.1.17.5 -p tcp --dport 80 -j ACCEPT  |
|                                                              |
+----^^^^^^^^^^^^^^^--------------^^^^^^^^^^^^^^^^^------------+
       Destination                  Protocol spec

Our current spec uses:

• applied_to section for destinations
• peer section for sources
• rule section for protocol spec
• and action is always ACCEPT

however, the peer and the rule section are lacking the capability to express the desire to omit Source and Protocol spec sections in result rule.

I propose to extend policy spec by adding following terms:

• Source selector which would not be rendered.

{
    "peers" : [
        { "peer" : "any" }
    ]
}

• Protocol spec that would not be rendered.

{
    "rule" : [
        { "protocol" : "any" }
    ]
}

[flashvoid]

Proposal 2: Support for multiple applied_to, peers, rules.

Goal get rid of the special case when processing namespace isolation policy.

Current implementation consists of one function that

• Takes the first item from applied_to, peers and rules lists.
• Works out chain names for tenant, segment and a policy.
• Inserts default rules and jumps from tenant chain to segment and policy chains.
• Generates policy rules and inserts them into the policy chains.

What need to happen:

for target in applied_to:
    for rule in rules:
        for peer in peers:
            iptables_rules.append(_make_rule(target, rule, peer))

and some default rules need to be processed outside of this loop ...

[jbrendel]

Proposal 1: I agree and support that proposal. Only one issue: A peer definition with CIDR "0.0.0.0/0" would have the same effect, so once we support CIDR then we would have two ways to express "any" as peer. Not sure if this is so great.

On the other hand, I like the clarity of "peer" == "any", so I would agree that this should be supported.

[jbrendel]

Proposal 2: Yes, that sounds about right.

[debedb]

This actually reminds me of a question I had for myself - if no "appliedTo" is specified it's probably an error, but if no "peer" is specified, what does it mean? If no rules are specified is it also an error? Or it just means "all traffic allowed"? What's the difference, in other words, between "any" and omission?

[flashvoid]

@debedb https://docs.google.com/spreadsheets/d/1vN-EnZqJnIp8krY1cRf6VzRPkO9_KNYLW0QOfw2k1Mo/edit?usp=sharing

[debedb]

Nice - then we should add all this stuff to Validate() method of policy that I have in the last review...

[jbrendel]

I like explicit better than implicit. Why allow omission if we have "any", which is a perfectly readable and clear way of saying things?

[jbrendel]

I think the table is mostly correct. As I understand it, when you say '' you mean "defined in any/some way", right? So, this would cover "protocol=any" as well as "protocol=!any" so you could collapse rows 7 and 8 into a single "cidr / " row, right?

I think another way of saying it would be:

"peer can be 'any', a CIDR, a whole tenant or a segment of a tenant". "applied-to can be the same, except 'any'." "rules can be 'any' or protocol specific (in all circumstances)"

[debedb]

No, appliedTo right now cannot be CIDR I believe

[jbrendel]

@debedb Yes, currently 'appliedTo' cannot be a CIDR, you're correct.

[flashvoid]

UPD

Proposal # 1 implemented.

Current view of namespace isolation policy is

{
    "datacenter": {
    "endpoint_bits": 8,
    "endpoint_space_bits": 0,
    "segment_bits": 4,
    "tenant_bits": 4,
    "host_bits": 8,
    "cidr": "10.0.0.0/8",
    "ip_version": 4
  },
  "rules": [
    {
      "protocol": "any"
    }
  ],
  "peers": [
    {
      "peer": "any"
    }
  ],
  "name": "ns1",
  "applied_to": [
    {
      "tenant_network_id": 1
    }
  ]
}

note: isolation policy not treated in any special way so it requires a name and a datacenter spec

Proposal # 2 implemented.

• There are few problems with policy spec discovered.

1) is_stateful makes no sense right now since stateful is the default, so I prohibited it.

2) applied_to allows multiple tenants but tenant id is a part of a policy name and so, loop over applied_to implicitly produces multiple policies.

3) Implementation is one big function right now, we should either spend some time on another refactoring run or pay the price next time when we need to put any changes into this mess. Or rewrite it in Go already ...

[flashvoid]

@jbrendel

In the case of line 313 (target_segment_forward_chain = tenant_wide_policy_vector_chain), should this entry here not be skipped? Since it would be essentially the same as what's added in 321?

The code on 330 is responsible for adding a jump from vector chain to policy chain, it never directly processes tenant_wide_policy_vector_chain and relays on code at 313 for setting variable right.

This is done to limit amount of code under if in 307, otherwise this function would be twice as big.

[flashvoid]

@jbrendel

Since this is not a loop, I guess you could drop line 327 and just write: rules[target_segment_forward_chain] = [ _make_rule(.....) ]

Then the code at 334 will override data written by 326 if target_segment_forward_chain == tenant_wide_policy_vector_chain

[jbrendel]

I know you had a long conversation about packages yesterday, and I don't know what the outcome was. But could you explain to me why "pkg/util/policy/agent/agent.py" is under util? It seems to me that the policy agent is a pretty major component, so it should be more ... prominent?

[flashvoid]

@jbrendel

could you explain to me why "pkg/util/policy/agent/agent.py" is under util

Well, as much as I don't like to say this, but this is temporary, really we shouldn't have python code in the core. In my vision, we would have a go package that handles policies under /pkg/util/policy/agent and binary under /cmd/policy/agent.

[jbrendel]

@flashvoid You wrote:

we would have a go package that handles policies under /pkg/util/policy/agent

I guess my question would be why the policy agent would be in a 'util' package? It's a major component?

[flashvoid]

@jbrendel

we would have a go package that handles policies under /pkg/util/policy/agent

I guess my question would be why the policy agent would be in a 'util' package? It's a major component?

Package that provides policy managing tools can be under util i think. Policy agent as a service should be under /cmd or under /pkg/policy/agent if it's too big to fit under /cmd.