test-me-please

For egress path in future, it would be nice to instead map down the wildcard selector as {identity: 0, dport...} because then we can still appropriately apply L4-only policies even if IP->ID mapping fails.

I think this fix is not wrong but incomplete and in order to complete we will run into a problem.

First of all, it's contrary to what we have documented right now:

// - All members of this structure are optional. If omitted or empty, the
//   member will have no effect on the rule.

However, I agree that going this route is correct but I think we need to do it differently, because what you fix here must also apply to FromCIDR and FromEntities. We can't apply the logic to just FromEndpoints. A rule which only has ToPorts 80 set should also allow from any external client and it should also allow from the host. This means that if we have rule such as:

[{
    "labels": [{"key": "name", "value": "l4-rule"}],
    "endpointSelector": {"matchLabels":{"app":"myService"}},
    "ingress": [{
        "toPorts": [
            {"ports":[ {"port": "80", "protocol": "TCP"}]}
        ]
    }]
}]

Then this would have to translate into something like this:

[{
    "labels": [{"key": "name", "value": "l4-rule"}],
    "endpointSelector": {"matchLabels":{"app":"myService"}},
    "ingress": [{
        "toPorts": [
            {"ports":[ {"port": "80", "protocol": "TCP"}]}
        ],
        "fromEndpoints": [{}],
        "fromCIDR": ["0/0"],
        "fromEntities": [ "all" ],
    }]
}]

Assuming the above model, it then raises the question, how do to define a rule which only allows ingress from label "foo" without any further privileges granted? This is why we currently have a difference between empty selector to wildcard and nil.

I agree that it is confusing and that we have to change it. The good news is that we can change it while preserving backwards compatibility by bumping to v3 and converting from v2 to v3.

I suggest we do the following:

1. Embed toPorts into the From* and To* structures to make the L3 dependant L4 part obvious and explicitly require the user to specify the wildcard if wanted. This will make it super obvious what is going on at all times.
2. Declare the IngressRule and EgressRule a union which means that only one of FromEndpoints, FromRequires, FromCIDR and FromEntities can be specified. This makes it super obvious that there is no point in combing label based source selector and CIDR based source rules.
3. Only add ToPorts to the From* and To* fields that support L3 dependant L4. This means we would currently exclude FromCIDR and ToCIDR until we have added support for it.

Example:

  endpointSelector:
    matchLabels:
      role: backend
  ingress:
  - fromEndpoints:
    - toPorts:
      - ports:
        - port: "80"
          protocol: TCP

@joestringer

After further discussion: I will open an issue to propose the above proposal for a policy v3 and we fix this in v2 in the following way:

• Wildcard all L3 selectors (FromEndpoints, FromCIDR, and FromEntities) if none of the L3 selectors are specified. If any of them are specified in the rule, no wildcard expansion happens.

Sounds good, I'll get to that this week.

@tgraf I looked into it, we don't have such information at the L4Filter at the moment. The two paths forward I see are either:

1. Restrict other combinations at the API layer (Reject policies that contain rules with more than one L3 match in a single rule #3015).
   • We only actually support two combinations with ToPorts: Either with FromEndpoints specified, or not at all.
   • If we restrict this at the API layer, then the current approach in this PR will solve the problem for now.
   • In future we'll need to fix it up, but that's OK. We'll do so when the support is expanded beyond the current support.
2. Plumb CIDRs through the policy down to L4Filter.
   • We'll need to do this anyway for Support egress CIDR-dependent L4 policies #1684
   • Will just take a bit longer and bind this issue + egress labels policy support to that work

@joestringer OK, I would pick 1) then and solve this properly via #3005

test-me-please

Addressed the comments and rebased, ready for review. The comment in the patch assumes merging of #3015, but I believe this should fix the issue as-is.