Fix flow validation breakage

[jrajahalme]

DNS exchange happens first, it's useful to have it validated even if
the actual test traffic fails to validate.

Retry flow matching again if all flow requirements can not be
met. Start the new round from the flow following the previous first
match. This effectively retries flow matching from different L4
connections, as the ephemeral source port found from the first match
is fixed for the remaining requirements. By skipping the previous
first match the match can succeed on a different L4 connection.

For example, if successful DNS reply from proxy is required, it may be
on a different UDP or TCP port than the first DNS resolution exchange
that may have failed.

This capability was inadvertently removed in #319.

Merge flow validation results for each flow requirement before returning.
Without this flow validation was reported successful when at least one
flow validation requirement was met, hiding failures in later requirements.

Fixes: #319

[ti-mo]

I never understood the code touched here, so not sure if I can give a comprehensive review. The problem this PR is trying to solve is also not clear. I'm not sure why an offset is still being used, I thought we would remove it completely.

Given a list of flows on the left and a list of requirements on the right, the flow matcher should return a single verdict. If not all of the requirements are met, the error returned should state which one. If DNS resolution was successful, the matcher should make it past the DNS resolution and fail to match establishing e.g. a TCP connection. Why does this need special treatment?

[jrajahalme]

I never understood the code touched here, so not sure if I can give a comprehensive review. The problem this PR is trying to solve is also not clear. I'm not sure why an offset is still being used, I thought we would remove it completely.

typically the 1st DNS flow requirement (First) is a DNS request sent by the source. This request has some ephemeral source port. So when matching for the First requirement (e.g., TCP SYN) we do not know what the source port of the successful connection exchange is, so we are happy to a TCP SYN from any port. All other flow requirements in the same "set" (i.e., Last and Middle) also require the same ephemeral port. This was implemented to match the flow requirements on a single transport connection so that an RST from another TCP connection would not break the flow validation.

See this commit for details on ephemeral port tracking: 6819144

The same applies to UDP exchanges as well. When the flow requirement expects to see the successful DNS response with the final answer, this practically never exists on the first DNS exchange. Without sliding the offset past the last successfully matched First flow filter the matcher would never see any of the later DNS exchanges (with different ephemeral port numbers) so the final answer would never be matched. This is why it is essential to try the flow filters on successive L4 (UDP or TCP) exchanges, implemented by incrementing the offset past the last First match.

Given a list of flows on the left and a list of requirements on the right, the flow matcher should return a single verdict. If not all of the requirements are met, the error returned should state which one. If DNS resolution was successful, the matcher should make it past the DNS resolution and fail to match establishing e.g. a TCP connection. Why does this need special treatment?

Currently the code does not indicate which requirement fails. Prior to this PR the flow matcher ignored failing requirements after the 1st successful one. This was hiding failing DNS requirements after successful TCP requirements. Making DNS requirements the 1st one reveled this. This is fixed by merging the validation results earlier.

[jrajahalme]

I'm not sure why an offset is still being used, I thought we would remove it completely.

The offset is an internal detail of the flow matcher that was previously spilled to the caller. This PR removes the offset parameter from the flow matcher prototype. Internally, offset is needed to find the L4 connection successfully matching all the requirements, given the ephemeral port tracking in 6819144.

[jrajahalme]

Given a list of flows on the left and a list of requirements on the right, the flow matcher should return a single verdict.

matchAllFlowRequirements() does this by calling matchFlowRequirements() successively for each requirement.

If not all of the requirements are met, the error returned should state which one. If DNS resolution was successful, the matcher should make it past the DNS resolution and fail to match establishing e.g. a TCP connection. Why does this need special treatment?

Not sure what you mean with "special treatment" here. matchFlowRequirements() internally uses an index variable we call offset to implement its function, given the presence of ephemeral port tracking done by the L4 filters. matchAllFlowRequirements() needs to merge validation results returned by matchFlowRequirements() to form the single verdict it needs to return.

[jrajahalme]

kind test failed with the same error as cilium/cilium#12141:

kind-chart-testing] Waiting for Service cilium-test/echo-same-node to become ready...
🐛 Error waiting for service cilium-test/echo-same-node: command terminated with exit code 1:
;; reply from unexpected source: 10.244.1.178#53, expected 10.245.0.10#53

;; reply from unexpected source: 10.244.1.178#53, expected 10.245.0.10#53

;; reply from unexpected source: 10.244.1.178#53, expected 10.245.0.10#53

;; connection timed out; no servers could be reached

[jrajahalme]

Multicluster failed with #361

[jrajahalme]

This is a bug fix and the flakes are unrelated, marking as ready-to-merge