👍

I think it makes sense to also add a comment which says that if multiple members are set, all of them need to match in order for the rule to take effect. And also add something like: The exception to this rule is the FromRequires field, the effects of any Requires field in any rule will apply to all other rules as well.

Done

This may need further thought. Imagine the case where we have the following rules:

  ingress:
  - fromEndpoints:
    - matchLabels:
        id: app2
    toPorts:
    - ports:
      - port: "80"
        protocol: TCP
      rules:
        http:
        - method: "GET"
          path: "/public"
  - toPorts:
    - ports:
      - port: "80"
        protocol: TCP
      rules:
        http:
        - method: "GET"
          path: "/public"

I think we would agree that if we have two rules which both cover port 80/tcp. One is restricted to fromEndpoints, one is not. The one without fromEndpoints should take precedence as it also includes the more restrictive rule.

I thik we need to clear existing fromEndpoints conditions if we encounter a unrestricted toPorts and we need to skip conditional toPorts if we already have an unrestricted L4 rule.

L7 rule will potentially need to be stored per fromEndpoints as the rules should not apply to other endpoints. So while we want to potentially skip/clear fromEndpoints restrictions on pure L4 allow/disallow. We always need to store the L7 rules per fromEndpoints. We will need to encode logic in the proxy which allows matching on the source identity.

What do you think?

Thanks for the quick review!

I think we would agree that if we have two rules which both cover port 80/tcp. One is restricted to fromEndpoints, one is not. The one without fromEndpoints should take precedence as it also includes the more restrictive rule.

Hrm, to me this is not as obvious. For some reason, I'd expect the opposite actually. Or, perhaps, "unexpected behavior"? :)

We should have a disjunction between rules ("or"). So if rule A is less restrictive than rule B, then rule A should have precedence. But I wouldn't talk about "precedence" because there's no order or priority.

personally i'd like to have an option to disallow overlapping rules to avoid being overly permissive unintentionally, but in general i agree with @rlenglet .

I'm fine with that, in particular if used in combination with the Cilium REST API. We can reject rules there. As for k8s, the best we could do is report ignored/rejected rules in the Status field though.

L7 rule will potentially need to be stored per fromEndpoints as the rules should not apply to other endpoints. So while we want to potentially skip/clear fromEndpoints restrictions on pure L4 allow/disallow. We always need to store the L7 rules per fromEndpoints. We will need to encode logic in the proxy which allows matching on the source identity.

@tgraf just to make sure I understand this correctly. If the rule has a number of rules with explicit FromEps lists, we should make sure L7 rules are not merged. However, do we also clear all existing FromEps->L7 entries if there is a rule without explicit set of FromEps?

I think the logic should be:

1. Any L7 rule with 1..n combined FromEndpoints must be appended to a list which is maintained for each endpoint in the FromEndpoints, e.g. if we have the independent rules: r1 from {a,b} and r2 from {b,c}, then b must be able to do {r1, r2}.
2. Any L7 rule without an explicit combined FromEndpoints must be appended to all lists maintained and also to a wildcard list. We may also maintain the wildcard list only and then append the wildcard list to all maintained lists at the end. E.g. if we have an additional, unconstrained r3 then {a, b, c and d} must be able to do r3.

comment on exported type L7DataMap should be of the form "L7DataMap ..." (with optional leading article)

exported const WildcardEndpointSelector should have comment (or a comment on this block) or be unexported

exported type L7Rules should have comment or be unexported

L7 rules

Can you add documentation for this type?

Update the documentation to include context for the parameter you added, as well as for direction / protocol strings.

Please name the import for logrus to "log" for consistency.

Name the import to "log" so that logging statements are "log.Debugf..." for consistency.

L7

Even though this function is not exported, I think it needs documentation.

EP -> endpoint for consistency

ep -> endpoint for consistency

Similar to above - even though this function is not visible outside of this package, some documentation would be very helpful.

When the users print cilium endpoint get XXX they will see this in the output. It might be confusing:

"policy": {
    "allowed-consumers": [
      1,
      268,
      269
    ],
    "build": 60,
    "id": 269,
    "l4": {
      "egress": [],
      "ingress": [
        "{\n  \"Port\": 9080,\n  \"Protocol\": \"TCP\",\n \"FromEndpoints\":\"There will be duplicated and lots of verbose that here\"  \"L7Parser\": \"\",\n  \"L7RedirectPort\": 0,\n  \"L7Rules\": null,\n  \"Ingress\": true\n}"
      ]
    }
  },

I suggest you append json:"-" at the end to omit this field from the output

Done

small nit, why not?

		wildcardEp := L7Rules{}
		for _, existingL7Rules := range v.L7RulesPerEp {
			wildcardEp = append(wildcardEp, existingL7Rules...)
		}
                v.L7RulesPerEp[WildcardEndpointSelector] = wildcardEp

Done

This doesn't work as you're expecting, replace with
v.L7RulesPerEp[hash] = append(ep, newL7Rules...)

Done

Done

Done

@amreshakim Why is this for ingress only?

Discussed offline, seems like it may just be because egress L7Filters were not supported when this was written.