Add network flows export support proposal

[rcarrillocruz]

No description provided.

[russellb]

/assign @russellb

[rcarrillocruz]

/retest

[russellb]

this is looking good. I just had a few minor comments

[rcarrillocruz]

/assign @danwinship

[danwinship]

This is basically good and maybe all of my comments are better addressed in the actual implementation PRs rather than in the enhancement PR?
/lgtm
/hold
if you want to merge it as-is just cancel the hold

[danwinship]

Not sure how much I should be arguing about the API details here rather than in the eventual API PR, but...

1. Not sure this belongs under ovnKubernetesConfig? Even though we only support it in ovn-kubernetes currently, these are generic protocols which could be supported by any network plugin. eg, like how we have kubeProxyConfig as a top-level rather than inside openshiftSDNConfig. Not totally sure about this one...

2. You probably don't need Config in the name? Just exportNetworkFlows explains what that section is doing

3. Get rid of the naming redundancy in the subfields. eg, consider what accessing this would look like from golang code:

    ip := defaultNetwork.exportNetworkFlowsConfig.exportNetworkFlowsCollectors[0].exportNetworkFlowsCollectorIP
   

   You can just have collectors and ip/port

4. Kubernetes API enums always have initial caps and then proper case. So, NetFlow, IPFIX, SFlow

5. Is there any reason we allow multiple collectors of a single type but not multiple collectors of different types? If you put the protocol, ip, and port together, then it would be more flexible, and also slightly simpler to write out in the case where you only have a single collector (which I think is the common case?):

    exportNetworkFlows:
      - protocol: NetFlow
        ip: 172.30.158.150
        port: 2056
   

6. The big feature of sFlow over NetFlow is that it lets you export only a sample of the traffic rather than all the traffic. Any plans to allow configuring that, now or later?

[rcarrillocruz]

1. Right, I thought about that. Thing is I saw IPSEC within ovnKubernetesConfig and conceptually it's not really an OVN-K related feature, it could also be leveraged by other plugin. Given that it's the latest thing added, I just cargo culted. I don't have a strong opinion.
2. I cargo culted HybridOverlayConfig and IPSecConfig
3. Yeah :D , I was actually looking for this name bikeshedding feedback. I see hybrid overlay options prepended with HybridOverlay, thus I just cargo culted. But I agree, probably sounds better not to prepend, since it's a dict it's clear those options apply to 'export network flows' .
4. I'd rather accept any case and just canonicalize as we do with other options. Note that sflow is usually written as "sFlow" per https://sflow.org/ .
5. Here you get me off-guard. The docs don't clarify if you can have multiple collector protocols defined, I'd need to test. I take you suggest having something like "endpoint: 172.30.158.140:2056"
6. Yes it does, however I was thinking of going light with exposing per-protocol options. In others proposals you were against having toggles on stuff to avoid end-user overconfigurability, e.g. the network diagnostics interval, so I thought of just hardcoding them and if there's end-user demand/RFE then expose . But I'm cool with exposing sflow sampling and polling params now.

[danwinship]

Not sure this belongs under ovnKubernetesConfig?

I guess another counter-argument is that even if we supported exactly the same flow collecting options in openshift-sdn, we could do that by just adding the same ExternalNetworkFlowsConfig struct to openshiftSDNConfig too. OK.

I cargo culted HybridOverlayConfig and IPSecConfig

which maybe we should have done differently...

exportNetworkFlowsConfig struck me as particularly awkward though. If you had said networkFlowExportConfig I probably wouldn't have said anything... I guess "verb phrase" = OK, "noun phrase + Config" = OK, "verb phrase + Config" = 🙁

I see hybrid overlay options prepended with HybridOverlay, thus I just cargo culted.

HybridOverlayVXLANPort is definitely wrong and I think I even complained about that

I'd rather accept any case and just canonicalize as we do with other options.

We mostly don't do that any more; we only originally did that for the network plugin type because the original capitalization was wrong (OpenshiftSDN rather than OpenShiftSDN) and we fixed it right before 4.1 code freeze and didn't want to break anything. But then this led to other bugs because while CNO handles either variant, stuff in other components didn't (eg, MCO deciding whether to enable system OVS) and so random things would break if you had used the wrong case. In the end, it's easier to just require everyone to get the string correct (and use validation to only accept the correct spelling/casing).

The docs don't clarify if you can have multiple collector protocols defined

oh, hm, I just assumed you could since they're totally separate tables...

In others proposals you were against having toggles on stuff to avoid end-user overconfigurability

It's true. I guess we can add more options later if we need them. My vague understanding though was that pretty much the main reason anyone used sFlow or IPFIX rather than NetFlow was to get sampling.

[rcarrillocruz]

It is, it's just I'm hardcoding the sampling/polling values, from my WIP branch:

https://github.com/openshift/cluster-network-operator/pull/982/files#diff-d1edeaf791eef7719efb5bb733f48f3a201cc1000ce6f8d09a917be2accc9cb4R86

 ovs-vsctl -- --id=@sflow create sflow agent=ovn-k8s-mp0 targets={{ .ExportNetworkFlowsCollectors }} header=128 sampling=64 polling=10 -- set bridge br-int sflow=@sflow

[rcarrillocruz]

@danwinship please take a look, I put up an alternative based slightly on your suggestions.
Since there can be either netflow, ipfix or sflow exports, have those as dicts and as values for those dicts a list of strings encompassing the IP and the port , i.e. no need to add yet another key.

[rcarrillocruz]

In the end, I'm making the exportNetworkFlows option top-level, I'll check on runtime OVN-K is used.
Cos otherwise, we will just keep piling on ovnKubernetesConfig and it won't be sustainable.

[rcarrillocruz]

/retest

[rcarrillocruz]

/hold cancel

[danwinship]

I was going to just merge this and say we can address the other issues in a later follow-up, but then I remembered that I'm not an approver in enhancements anyway

[danwinship]

netFlow, sFlow, ipfix. Sticking to the standard rules makes it easier for people to remember the right capitalization to use when creating/editing the struct

[rcarrillocruz]

sgtm

[danwinship]

Above you describe sflow as an "optional dict", but here it's an array, not a dict, and in particular, there's nowhere under sflow where you could add sampling/polling config. You'd need something like

sFlow:
  endpoints:
    - 172.30.158.150:2056
    - 172.30.54.103:2056

so you can add other fields later too. (I don't know if "endpoint" is the right terminology here.)

But then, would you necessarily want the same sampling config for every endpoint?

[rcarrillocruz]

yeah, the description needs fixing, it should be an array just like in the example.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, russellb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [russellb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[danwinship]

oh, Russell approved it.
/lgtm