Integrate components from Goflow-Kube #120
Conversation
mariomac
requested review from
KalmanMeth,
OlivierCazade,
eranra,
jotak,
jpinsonneau and
ronensc
| Port int `yaml:"port" doc:"the port number to listen on"` | ||
| HostName string `yaml:"hostName" doc:"the hostname to listen on"` | ||
| Port int `yaml:"port" doc:"the port number to listen on"` | ||
| BatchMaxLen int `yaml:"batchMaxLen" doc:"the number of accumulated flows before being forwarded for processing"` |
There was a problem hiding this comment.
Good point. We can do this or we can try what was discussed in a previous PR: to forward flows one by one via channels. In that case, we don't need neither BatchMaxLen nor BatchTimeout.
There was a problem hiding this comment.
@mariomac I like the one-by-one + channels approach ... getting more performance by "multi-thread/core" is what we need.
pkg/config/config.go
Outdated
| type KubeEnrich struct { | ||
| KubeConfigPath string `json:"kubeConfigPath"` | ||
| IPFields map[string]string `json:"ipFields"` |
There was a problem hiding this comment.
Should this perhaps go into the api directory and get included in the api.md?
Codecov Report
@@ Coverage Diff @@
## main #120 +/- ##
==========================================
+ Coverage 54.63% 58.32% +3.69%
==========================================
Files 47 52 +5
Lines 2709 3141 +432
==========================================
+ Hits 1480 1832 +352
- Misses 1129 1184 +55
- Partials 100 125 +25
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
pkg/pipeline/pipeline_netobs_test.go
Outdated
| @@ -0,0 +1,201 @@ | |||
| package pipeline | |||
There was a problem hiding this comment.
@mariomac if this is "specific" for some "use-case" or "scenario" ... maybe the right thing is to open a folder called scenarios and put the test file inside this folder. We might have more "use-cases" tests in the future. Also, can you think of some other name for "netobs" it is not informative? Maybe something like a redhat-ocp scenario or something like that. We need to think on this repo as being an upstream repo.
There was a problem hiding this comment.
This was located here because tests we often rely on private APIs. But you are right when you say that this is only a scenario-addressed test.
I think that, for the moment, I will remove this test, as it relies on other components that are already tested.
In a future, I think it would be nice to enable integration tests that do not rely on any private API:
- Enabling tests that are run via docker-compose, as it is actually being done.
- Changing the components' constructors to not be constructed directly from a configuration file but allowing dependency injection.
|
|
||
| // Values taken from https://www.iana.org/assignments/ipfix/ipfix.xhtml | ||
| var ( | ||
| sourceIPv4Address = entities.NewInfoElement("sourceIPv4Address", 8, entities.Ipv4Address, 0, 4) |
There was a problem hiding this comment.
This "feels" like end-to-end test ... and very specific to IPFIX ... do we need this ... what component is being tested here ???
There was a problem hiding this comment.
This is used to test the Ingest Collector (ingest_collector_test.go), as it remained untested.
There was a problem hiding this comment.
@mariomac ok ... this looks good to me ... the only comment small comment I have is that maybe it makes sense to put this file, and maybe also the Loki mock file under some "internal" folder underneath "test" and not directly in test.
|
|
||
| // The IPFIX client might send information before the Ingester is actually listening, | ||
| // so we might need to repeat the submission | ||
| test.Eventually(t, timeout, func(t require.TestingT) { |
There was a problem hiding this comment.
I really do not like "inconsistent" tests ... can we make this test more robust and drop the Eventually concept ... the entire Eventually concept feels like hiding from creating a consistent and deterministic test.
There was a problem hiding this comment.
In this case, the "Eventually" is not hiding an inconsistent test but addressing a synchronization issue. Goflow2 is initializing in an internal, background goroutine, and there is no way that we can know when it has started to accept flows (as UDP connections are uni-directional).
There are two ways to address this:
- Adding a Timeout of e.g. 1-2 seconds, but that would cause that this test is slow.
- Keep sending flows until we observe that they are being forwarded.
In the next commit, I have rewritten the test to be more explicit that we follow case 2, without giving the impression of flakyness.
6 participants
It integrates the following components from the old Goflow-Kube repository: