if block ends with a return statement, so drop this else and outdent its block

comment on exported method L4Policy.IngressCoversContext should be of the form "IngressCoversContext ..."

receiver name filter should be consistent with previous receiver name l4 for L4Filter

exported method PolicyMap.DeleteL4 should have comment or be unexported

exported method PolicyMap.L4Exists should have comment or be unexported

exported method PolicyMap.AllowL4 should have comment or be unexported

if block ends with a return statement, so drop this else and outdent its block

if block ends with a return statement, so drop this else and outdent its block

comment on exported method L4Policy.IngressCoversContext should be of the form "IngressCoversContext ..."

receiver name filter should be consistent with previous receiver name l4 for L4Filter

exported method PolicyMap.DeleteL4 should have comment or be unexported

exported method PolicyMap.L4Exists should have comment or be unexported

exported method PolicyMap.AllowL4 should have comment or be unexported

if block ends with a return statement, so drop this else and outdent its block

if block ends with a return statement, so drop this else and outdent its block

comment on exported method L4Policy.IngressCoversContext should be of the form "IngressCoversContext ..."

receiver name filter should be consistent with previous receiver name l4 for L4Filter

exported method PolicyMap.DeleteL4 should have comment or be unexported

exported method PolicyMap.L4Exists should have comment or be unexported

exported method PolicyMap.AllowL4 should have comment or be unexported

if block ends with a return statement, so drop this else and outdent its block

Can we just drop this commit? I think you will undo this anyway in a later commit.

Are you undoing all of these later on again? I did this commit to stop showing L3/L4 combined policies in the guides when we don't support them. We never merged it though. Given that we now support it, this commit is no longer required. So we can either just skip it here and rebase or undo it at the end.

Let me look at skipping it here and see whether anything falls out.

Should we consider tracing either the positive / negative match cases for L4 policy?

I think what you have is fine to get started.

This comment can be dropped.

@tgraf I wanted to double-check, if we have a policy like:

  [{                                                                            
    "endpointSelector": {"matchLabels":{"id.bar":""}},                          
    "ingress": [{                                                               
      "fromEndpoints": [                                                        
          {"matchLabels":{"reserved:host":""}},                                     
          {"matchLabels":{"id.foo":""}
      }]                                                                           
    }]                                                                    
  }]

Then this means that we should apply the policy at id.bar that the fromEndpoint must include either the label reserved:host or id.foo. It does not require both labels. Correct?

Correct, fromEndpoints is a slice and each member is an allowed combination of labels. matchLabels can include multiple labels if more than label must match.

Any preferences on how this trace log should indicate that the label stage cannot select this rule because it includes L4 requirements? In practice I think this will end up with the trace output stating later on that "Label verdict: undecided", but I don't know if the above line clearly describes why this decision can't be made yet.

Related: #1610

maybe specify how you cannot make a purely L3 / label-specific decision whether policy allows or denies? E.g.:

L3 traffic restricted to specific L4 destinations; cannot determine whether policy allows traffic purely based off of L3

How about this, a little more terse:
Rule restricts traffic to specific L4 destinations; deferring policy decision to L4 policy stage

Why isn't it this logic: iRule.From != nil && len(iRule.From) > 0?

Good catch, will fix.

The key size is not the same, what are the consequences for running instances in case we upgrade?

Looks not great right now. I believe that after the new Cilium instance starts, it will attempt to regenerate the BPF (including PolicyMap) for existing endpoints, which will call down to OpenOrCreateMap(). If that latter function finds a map in the correct path, it won't validate that the map parameters are the same, so the existing map with a smaller valueSize will remain. Subsequently, when attempting to load the BPF program into the kernel, verification will fail due to access beyond the valueSize of the map. That will leave the container in a bad state.

I think that OpenOrCreateMap() should validate that all of the passed parameters are the same for the existing map, or delete it and attempt to create a new map to mitigate the above issue.

Looks like there's already some validation in an alternative, OpenOrCreate() but not all of the maps are using this. Should be a minor extra patch.

Commit 7c8f502 attempts to address this, would be keen to get some feedback on that.

Move the "fmt" to line 18

done.

Move the "fmt" to line 20.

done.

use the GetExtendedKeyFrom: matchLabels[labels.GetExtendedKeyFrom(k)] = v.Value

use the GetExtendedKeyFrom: matchLabels[labels.GetExtendedKeyFrom(k)] = v.Value

What's the difference between this line and line 80?

Line 80 is saying that if the policy as resolved so far has an empty "FromEndpoints" list then it already covers the endpoints specified by the L4 filter that is currently being processed. Nil FromEndpoints would mean any endpoint can be used.

This line 85 is stating that if there are currently constraints on which endpoints this rule must receive traffic from in the policy, but the L4 Filter being processed allows contact from any Endpoint, then we need to loosen the current policy's restrictions on endpoints.

Was this imported into vendor/?

I believe so, here's the commit which updates vendor.conf and imports the code:
1ae6a2c

Can you add a comment why you are doing this?

At the bottom of the NEWS.rst file, add the links that these PR's refer to.

Ah, I thought that was automagic. OK.

Ah, I thought that was automagic. OK.

I have a vim script to this automatically for links that are not complete yet.

... FromRequires field; the effects ...

Fixed

incomplete documentation

Fixed

"this" is vague for a log message; which L4 Policy are we skipping? The log message should reflect which policy is being skipped.

I'll update it to print the policy and the fromEndpoints.

Thanks for adding this!

Ah, I'll actually drop this change out from this series because it's unrelated, but I do plan on following up on this separately.

Please bring back this file

Yeah, I'll drop the patch that deleted this when I push my latest changes.

This was an extra testing patch; I'll drop it.