cc @caseydavenport @thockin @danwinship @bboreham

I guess real fields are on the table, but we could also consider a jump straight to GA. It's an option for things of this scope.

s/CIDRS/CIDRs/

Do we need CIDRs to be plural? It's part of Peer which is already a slice.

Is there any design proposal?

/cc

Selects IP range(s) that a pod can accept (ingrees) or send (egress) traffic.

How to specify ingress IP range(s)? based on node? I think it's not very straightforward to specify container/pod IP ranges in real world

•   // Selects IP range(s) that a pod can accept (ingrees) or send (egress) traffic.
  

don't talk about egress yet

Do we need CIDRs to be plural? It's part of Peer which is already a slice.

And Peer is part of Ingress which is also a slice... NetworkPolicy is a little plural-happy. But I guess having this be singular is more consistent with PodSelector and NamespaceSelector.

Should it be called CIDRSelector for consistency?

As discussed some in #26720, we need more than just an array of allowed CIDR ranges, because using only allows makes it too difficult to specify "allow connections from everywhere except X.X.X.X/X".

IIRC the best solution we came up with for that was to be able to specify a combined allows+denies block, so that you can specify an "allow" rule that has holes cut out of it. Eg, something like:

type CIDRSelectorRuleType string
const (
        CIDRSelectorRuleAllow CIDRSelectorRuleType = "Allow"
        CIDRSelectorRuleDeny  CIDRSelectorRuleType = "Deny"
)

type CIDRSelectorRule struct {
        Type CIDRSelectorRuleType
        CIDR string
}

type NetworkPolicyPeer struct {
        ...

        CIDRSelector []CIDRSelectorRule
}

And then you could say things like:

kind: NetworkPolicy
apiVersion: networking.k8s.io/v1
metadata:
  name: allow-except-from-private
spec:
  ingress:
  - cidrSelector:
    - type: Deny
      cidr: 10.0.0.0/8
    - type: Deny
      cidr: 192.168.0.0/16
    - type: Deny
      cidr: 172.16.0.0/12
    - type: Allow
      cidr: 0.0.0.0/0

I had suggested before that we should say that NetworkPolicy CIDR selectors are only tested against cluster-external IPs, NOT Pod or Service IPs. Although this is more important in the egress case: you want to be able to say "deny 0.0.0.0/0" (to deny all external network access) without that also causing the pod to be unable to talk to other pods.

Also, we need to specify exactly how this is expected to interact with different traffic ingress mechanisms:

• HostPort/NodePort
• Ingress
• LoadBalancers
• ... ?

Also, we have backward compatibility issues, since the current behavior is "allow from any external IP" when there are no CIDR rules present...

don't talk about egress yet

Oops, left this in from the egress patch

I like the allow, deny flexibility with CIDR rules. I will add this to the patchset.

how this is expected to interact with different traffic ingress mechanisms:

Solid point, I'm not sure if this ever came up in the mailing list/original proposal

I like the allow, deny flexibility with CIDR rules. I will add this to the patchset.

I don't think we should be looking to add DENY action to NetworkPolicy. It's adds a lot of complexity. We're simply not going to get this in if we try to tackle that now.

we need more than just an array of allowed CIDR ranges, because using only allows makes it too difficult to specify "allow connections from everywhere except X.X.X.X/X".

This is a big part of what killed the previous attempt at adding CIDR support. If we want to add DENY functionality, that's something we need to tackle as its own feature - let's not bog down CIDRs with this.

I had suggested before that we should say that NetworkPolicy CIDR selectors are only tested against cluster-external IPs, NOT Pod or Service IPs

I think this is also adding more complexity than is necessary - why does this help users?

Also, we have backward compatibility issues, since the current behavior is "allow from any external IP" when there are no CIDR rules present..

Not sure I understand. The current behavior is not "allow from any external IP when there are no CIDR rules present" - applying a NetworkPolicy to a selection of pods today that includes a podSelector will prevent traffic from external IPs. With this change, that would still be the case.

How to specify ingress IP range(s)? based on node? I think it's not very straightforward to specify container/pod IP ranges in real world

IIUC this is what podSelector is for.

I don't think we should be looking to add DENY action to NetworkPolicy. It's adds a lot of complexity.

It's not adding a deny action, it's adding "allow X-but-not-Y". The end result is always still an allow action, it's just that that set of IPs allowed may not be easily expressible as a single CIDR range.

I had suggested before that we should say that NetworkPolicy CIDR selectors are only tested against cluster-external IPs, NOT Pod or Service IPs

I think this is also adding more complexity than is necessary - why does this help users?

Because pod and service IPs are essentially random and transient, and so you wouldn't ever want to intentionally use CIDR rules to control pod-to-pod traffic. And if there's no good use case for doing it intentionally, then it helps users if we make it so that they can't do it accidentally either.

Eg, people want to be able to say "block egress to all cluster-external IPs" without also necessarily blocking access to all other pods. If CIDR matches are allowed to match pod IPs, then they would need to specify a rule with the semantics of "allow egress traffic to [cluster CIDR range] but deny traffic to all other IPs" (and they'd need to update that rule if the cluster CIDR range changed). But if we say that CIDR rules never match pod IPs, then they can just say "deny traffic to all IPs" (or rather, "allow traffic to an empty array of CIDR ranges").

Maybe it could be named "ExternalCIDR" rather than "CIDR" to clarify this.

The end result is always still an allow action

well, my example syntax didn't actually enforce this, but that was the intent

Because pod and service IPs are essentially random and transient, and so you wouldn't ever want to intentionally use CIDR rules to control pod-to-pod traffic

In most scenarios they are random and transient within a specific well-known range. However, there's no mandate that a particular IPAM scheme be used with Kubernetes for either Pods or Services. I know of customers who use strict IPAM schemes where the ability to do this might be useful for them. I think we could just as easily document this as "in most cases you really shouldn't write CIDR rules that include the pod or service CIDR" and be done with it without writing any code.

Speaking of code, I'm not sure how we'd expect implementations to do this - neither the Pod nor Service CIDR is something that's available in the API.

Speaking of code, I'm not sure how we'd expect implementations to do this - neither the Pod nor Service CIDR is something that's available in the API.

Was just noting this too. xref: #46508 #25533

It seems that a config-map for kube-proxy was the goal for exposing cluster-cidr. This would force consumers to utilize RBAC.

Similar to #46254

- type: Deny
 cidr: 10.0.0.0/8

That syntax looks very much like a deny. I feel very strongly we should not add deny capability right now, and if we ever do it shouldn't be a sub-struct of CIDR.

difficult to specify "allow connections from everywhere except X.X.X.X/X".

This feels too much like trying to squeeze deny rules in through a loophole. If someone wants to express this they'll need to do so with multiple CIDRs, and yeah sometimes that will be a lot.

We should be trying to address most people's needs with something simple and easy to use, and a NetworkPolicyPeer.CIDR field gives us that.

We should be trying to address most people's needs with something simple and easy to use, and a NetworkPolicyPeer.CIDR field gives us that.

It addresses the needs of people who want "allow from X", but is really unhelpful for people who want "allow from anywhere except X". I'm not sure what "most people's needs" are though.

Note that you can currently specify policies like "allow traffic from anywhere except pods that have label X", or even "allow traffic from pods that have label X and don't have label Y or Z".

@danwinship @cmluciano @thockin Ok, what about something like this?

from:
  - cidr: "!10.0.0.0/24"

Or

from:
  - cidrSelector: {"cidr": "10.0.0.0/24", "operation": "NotIn"}
  - cidrSelector: {"cidr": "10.0.0.5/32", "operation": "In"}

^ Default would be "operation: In"

While I think the flexibility is great, there is no such flexibility for pod selector and namespace selector. Is there a way to say "allow connections from everywhere except ns-1"? I don't have a specific use case, but just wondering if this is intentionally different.

I like the allow, deny flexibility with CIDR rules. I will add this to the patchset.

@caseydavenport The "In", "NotIn" syntax and what it does is not immediately obvious to me.

In place of Deny, do we expect that one would create a "deny to everywhere, except these ranges" ?

While I think the flexibility is great, there is no such flexibility for pod selector and namespace selector

Yep, that's right.

Yeah, to be clear my strong preference is still not to add support for either of those methods I suggested above in v1.8, I was just trying to find a syntax I thought was palatable and didn't have the word "deny" in it.

cidrSelector: {"cidr": "10.0.0.0/24", "operation": "NotIn"}

@cmluciano the idea with this was that this rule would match any source IP addresses which are not in "10.0.0.0/24". I think this addresses the case that @danwinship raised re: some users who want to allow from everywhere except a certain range of IPs.

@cmluciano the idea with this was that this rule would match any source IP addresses which are not in "10.0.0.0/24". I think this addresses the case that @danwinship raised re: some users who want to allow from everywhere except a certain range of IPs.

Alas, it's not just "everywhere except X". You want to be able to say "allow from 192.168.0.0/16 except for 192.168.8.0/24".

The problem is that while you can assign labels to pods and namespaces in ways that make it easy to describe the set of pods/namespaces you want to allow traffic from/to, you don't get to rearrange the rest of the internet to make it easy to describe the IP ranges you want to allow traffic from/to. So to provide the same level of functionality, you need a more complicated system.

Well, except that as mentioned above, it's not actually "more complicated" because you can already do "X except not Y" in LabelSelectors. So we could just make CIDRSelectors work the same way:

from:
- podSelector:
    matchExpressions:
    - { key: "type", operator: In, values: [ "webserver" ] }
    - { key: "status", operator: NotIn, values: [ "testing" ] }
- cidrSelector:
  - { operator: In, cidr: "192.168.0.0/16" }
  - { operator: NotIn, cidr: "192.168.8.0/24" }
- cidrSelector:
  - { cidr: "192.168.8.1/32" }

As with LabelSelectors, the elements within a single selector are ANDed together, so the first cidrSelector says "allow 192.168.0.0/16 except for 192.168.8.0/24" (and it would still say the same thing if the two elements were in the opposite order) and the second cidrSelector says "allow 192.168.8.1" (taking advantage of the fact that "In" is the default value for "operator").

Talked with Casey and Dan and we generally like the operator pattern since it matches labelSelector. I will update the PoC while we gather more feedback.

Would appreciate @thockin's input to the above suggestion for CIDRs.

From an implementation standpoint, the above would be really easy in Calico, and it's nice to have the apparent consistency with how matchExpressions is defined.

Ugh.

If we have to solve this problem, could we make it simpler and more structural?

from:
- cidr:
  allow: 192.168.0.0/16
  except:
    - 192.168.8.0/24
    - 192.168.9.0/24

Or, if we REALLY think we have concrete use cases (please)

from:
- cidr:
  allow: 192.168.0.0/16
  except:
  - deny: 192.168.8.0/24
    except:
    - allow:  192.168.8.128/25
      except:
      - deny: 192.168.8.99/32
      - deny: 192.168.8.199/32
  - deny: 192.168.9.0/24

e.g.

type CIDRAllow struct {
	Allow  string
	Except []CIDRDeny
}

type CIDRDeny struct {
	Deny   string
	Except []CIDRAllow
}

https://play.golang.org/p/oMx0Ic2mwg

Benefit here is that we can enforce that "inner" rules are subsets of "outer" rules.

It's still confusing because "deny" doesn't mean deny. It means "not under this rule", but another rule could allow it.

Or, if we REALLY think we have concrete use cases (please)

You don't have to allow nesting "allow"s inside "deny"s, because you can always just hoist the inner "allow" into a separate NetworkPolicyPeer. Your second example could be expressed in the syntax of the first example as:

from:
- cidr:
  allow: 192.168.0.0/16
  except:
  - 192.168.8.0/24
  - 192.168.9.0/24
- cidr:
  allow: 192.168.8.128/25
  except:
  - 192.168.8.99/32
  - 192.168.8.199/32

Benefit here is that we can enforce that "inner" rules are subsets of "outer" rules.

Oh. Indeed, the rewritten example doesn't allow that. But is that really that much of a benefit? (Answer: maybe yes since there was a bug in your example: "192.168.8.99/32" is not a subset of "192.168.8.128/25". :-)

It's still confusing because "deny" doesn't mean deny.

Yes. "Except" is much better.

A few thoughts on the allow except suggestion:

• I like the more structural approach.
• I prefer keeping it simple: so the allow except without the denyclauses.
• However, imagine a future where we decide to add deny as a general attribute of a policy rule (overriding the implicit allow of rules today) supporting the full set of selectors in the rule (pod, namespace, CIDRs). i.e. The ability to specify policies like"deny from podSelector" or "deny from namespaceSelector" or "deny from CIDRs".
  • In this potential future we might regret choosing allow except for the CIDR structure, rather than in notInt or similar.
  • We might also regret the choice of allow being single-value and except being multi-value, rather than symmetrical both being multi-value.

Thoughts?

I should add, I don't know if we every will decide to add deny, but it has come up in discussions many times, so I wouldn't want to rule it out!

Dan, good point. Much simpler.

Alex, Are you just arguing for more generic names than "allow" and "except" ?

Aside from that, we could repeat?

from:
- ipAddresses:
  - cidr: 192.168.0.0/16
    except:
    - 192.168.8.0/24
    - 192.168.9.0/24
  - cidr: 192.168.8.128/25
    except:
    - 192.168.8.99/32
    - 192.168.8.199/32

Better? Worth the extra nesting?

I'm not sure what the alternative options are for ipAddresses: . Is this more of a clarification type of thing?

Ah. Can we perhaps use a synonym for allow like admit or accept?

from:
- ipAddresses:
 - cidr: 192.168.0.0/16
   except:
   - 192.168.8.0/24
   - 192.168.9.0/24
 - cidr: 192.168.8.128/25
   except:
   - 192.168.8.99/32
   - 192.168.8.199/32

Yes, that's the kind of thing I was thinking for the potential deny rule future proofing.

But if people don't like the ipAddresses then ipBlock that Tim suggested later also sounds fine to me.

I'm happy with the ipBlock syntax.

I'd like to avoid including "action" words (allow, deny, etc) as keys, since like Alex says it ties our hands if we want to add additional actions later.

I'm cool with ipBlock if other CNI implementors are. I will update the PoC.