[#7231] Add support for multi flow exporter targets via CRDs. #7641
Conversation
|
Reopening as new PR since the other one closed and I can't reopen/edit it. For reference to older comments: #7444 |
andrew-su
added
area/flow-visibility
| FLOW_VISIBILITY_HELM_VALUES="$THIS_DIR/values-flow-exporter.yml" | ||
| CH_OPERATOR_YML="$THIS_DIR/../../build/yamls/clickhouse-operator-install-bundle.yml" | ||
| FLOW_VISIBILITY_CHART="$THIS_DIR/../../test/e2e/charts/flow-visibility" | ||
| KUSTOMIZATION_DIR="$THIS_DIR/../../test/e2e/kustomize" |
There was a problem hiding this comment.
Instead of using kustomize, can we stick to a single tool (Helm) and update the chart so that it can support multiple FA installs in different Namespaces? I think this is a pretty common problem for helm charts in general, and I would actually recommend doing it as a separate PR, yet again :)
I would suggest looking at https://github.com/kubernetes-sigs/external-dns/blob/031b6e4aedf212fe24a0865b2cea4ac628f712b0/charts/external-dns/templates/clusterrole.yaml#L5 for an example, I think it's done pretty well there (there may be other good examples).
We don't have to worry too much about CRDs for now ... since the FA chart currently doesn't have CRDs, but that will come up in the future. We can also follow the external-dns approach for CRDs in the future (or whatever we did for Antrea CRDs, even though Antrea always has a single chart installation per cluster).
There was a problem hiding this comment.
In that case, let's do that as a follow-up PR. I'll open a issue for supporting FA in different namespaces.
| conntrackConnStore *connections.ConntrackConnectionStore | ||
| conntrackPriorityQueue *priorityqueue.ExpirePriorityQueue | ||
|
|
||
| denyConnStore *connections.DenyConnectionStore | ||
| denyPriorityQueue *priorityqueue.ExpirePriorityQueue |
There was a problem hiding this comment.
what currently prevents each Destination to have a unified connection "store"?
it's really strange to have:
poller / packetin -> subscription -> destination -> conntrackConnectionStore + denyConnnetionStore -> exporter
instead of just:
poller / packetin -> subscription -> destination with connection map -> exporter
even if we keep conntrackConnectionStore and denyConnnectionStore, currently there is just too much complexity and layering with channels. For example, is there a good reason to keep a Run method and have an incoming channel in conntrackConnectionStore?
Connection should come in to the Destination via a callback, they can then be added synchronously to the unified connection store (or we can keep separate stores for now if it is more convenient).
There was a problem hiding this comment.
I had a combined store, but it also involved a lot more changes. I kept it as two separate stores now to keep implementation of store closer to what they were doing before (in terms of storage/exporting)
There was a problem hiding this comment.
I do plan on merging the two stores later though, as a follow up PR.
andrew-su
force-pushed
the
suan/fef
branch
4 times, most recently
from
26bca97 to
8ccbc7a
Compare
This change uses the `FlowExporterDestination` CRD to determine which Flow Aggregator to send flows to. The logic for polling inside the ConntrackConnectionStore is pulled out and the connections are forwarded to the different "destinations". The destinations if using gRPC or TLS with IPFIX will no longer translate the <namespace>/<service>:<port> into the dns address. It is the responsibility of whoever creates the resource to properly set the server name. For the static destination it will be populated automatically. Additionally, a static destination will be created based on the Antrea Agent ConfigMap. The `flowExporter.enable` key in the Antrea Agent config is used to turn on/off the static destination. From the config, only the `pollInterval` and `staleConnectionTimeout` are shared between all the destinations. Signed-off-by: Andrew Su <[email protected]>
2 participants
This change uses the
FlowExporterDestinationCRD to determine which Flow Aggregator to send flows to. The logic for polling inside the ConntrackConnectionStore is pulled out and the connections are forwarded to the different "destinations". The destinations if using gRPC or TLS with IPFIX will no longer translate the /: into the dns address. It is the responsibility of whoever creates the resource to properly set the server name. For the static destination it will be populated automatically.Additionally, a static destination will be created based on the Antrea Agent ConfigMap. The
flowExporter.enablekey in the Antrea Agent config is used to turn on/off the static destination. From the config, only thepollIntervalandstaleConnectionTimeoutare shared between all the destinations.Fixes #7231