Allow interception of port-forwarded traffic by making the proxy target the pod IP

[roobre]

Description

Currently, the disruptor does not support disrupting local traffic, such as that generated through kubectl port-forward.

The main reason for this is that the agent running in the target pods redirects the traffic coming from the eth0 interface, which is the expected path for the traffic coming from other pods or from outside the cluster.

If we try to address this limitation by redirecting traffic from the lo interface, we would also be re-redirecting the egress traffic from the agrent back to itself.

This PR tackles the problem of differentiating the proxy traffic from port-forwarded traffic by not using the network interface as a way of identifying the source of the traffic. Instead, the proxy will differentiate the traffic using the source and destination IPs and will use the pod's IP as a target for the redirected traffic.

This is an alternative fix for #214.

Checklist:

• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works.
• I have run linter locally (make lint) and all checks pass.
• I have run tests locally (make test) and all tests pass.
• I have run relevant e2e test locally (make e2e-xxx for agent, disruptors, kubernetes or cluster related changes)
• Any dependent changes have been merged and published in downstream modules
  

[roobre]

Still pending some more manual testing, but e2e tests pass. It should be ready for a first review.
Changeset is on the medium-large side, I'd suggest to review commit-by-commit as they should be pretty much self-contained.

[pablochacin]

@roobre I would update the description of the PR. I don't think that explaining the differences with the previous PR adds value and I found it distracting.

[pablochacin]

@roobre Another general comment. I've seen you change the type of ports from unit to string. This is in parameters and structs. Ports are expected to be uints. Changing the type removes domain information and I don't see how benefits the code, beyond maybe saving a type conversion. Could you please elaborate on the rationale for this change?

[roobre]

Changing the type removes domain information and I don't see how benefits the code, beyond maybe saving a type conversion. Could you please elaborate on the rationale for this change?

I didn't see the point of storing something as integer when it is used as an string everywhere. Moreover, I think ports are better modeled as strings rather than numbers, as applications should tolerate using IANA-defined port names (although this is rarely used in practice.

That being said, I agree the change is not very relevant for this PR, so I have reverted it.

[pablochacin]

Moreover, I think ports are better modeled as strings rather than numbers, as applications should tolerate using IANA-defined port names (although this is rarely used in practice.

Even if this were valid in a general sense (I won't enter in discussing this) I don't see how this is relevant to the use cases of the disruptor and in particular, of the agent.

I think keeping code aligned to the use cases and not over-generalizing it decreases the cognitive load needed for understanding it.

[roobre]

Manual testing done, everything seems to be working pretty well. The only thing I noticed is #231 (comment), I'll give it a swing as it should be fairly easy to restore that behavior.

Script used for manual testing

import  http from 'k6/http';
import { PodDisruptor } from 'k6/x/disruptor';
import { check } from 'k6';

export const options = {
    scenarios: {
        load: {
            executor: 'constant-arrival-rate',
            rate: 100,
            preAllocatedVUs: 10,
            maxVUs: 100,
            exec: "default",
            startTime: "10s", // Delay load for a few seconds to give disruptor time to start.
            duration: "20s",
        },
        disrupt: {
            executor: 'shared-iterations',
            iterations: 1,
            vus: 1,
            exec: "disrupt",
            startTime: "0s",
        }
    }
}

export default function(data) {
    const resp = http.get(`http://${__ENV.SVC_IP}/`);
    check(resp.status, {
        'Injection succeeds': (status) => status === 418,
        'Injection passes through': (status) => status === 200
    })
}

export function disrupt(data) {
    if (__ENV.SKIP_FAULTS == "1") {
        return
    }

    const selector = {
        namespace: "default",
        select: {
            labels: {
                app: "nginx"
            }
        }
    }
    const podDisruptor = new PodDisruptor(selector, {
        injectTimeout: "5s"
    })

    if (podDisruptor.targets().length === 0) {
        throw new Error("Disruptor has no targets")
    }

    // delay traffic from one random replica of the deployment
    const fault = {
        averageDelay: "500ms",
        errorCode: 418,
        errorRate: 0.5
    }

    podDisruptor.injectHTTPFaults(fault, "30s")
}

[pablochacin]

Looking good. Some minor comments and a couple of questions. I still have to run tests locally.

[roobre]

I think we're past the point where fixup! commits are useful, as the original commits are out of my terminal's viewport. I'll manually squash/fixup commits once the PR gets approved.

[pablochacin]

I think we are repeating here the same problem as when the tests were in the disruptor e2e test, putting them as a sub-test of the fault injection instead of an independent test. However, this is easy to change in a follow up PR so I will not ask to change this.

[roobre]

My main goal was to reuse the setup, as it is largely identical, but I don't have a strong opinion on it. We can split-and-copypaste if that's preferred.

[pablochacin]

In general. LGTM. Two minor comments.

Code needs to be squashed before accepting PR

[pablochacin]

Please squash commits to clean history.

[roobre]

I've squashed the commits into the original set, which I think make sense and are pretty disjoint.

[pablochacin]

LGTM.