Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to disable flow logging for network instance #62

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

milan-zededa
Copy link
Contributor

@milan-zededa milan-zededa commented Jul 22, 2024

Flow-logging is an EVE feature that allows to collect information
about every application network flow. This includes attributes like
src/dst IP, src/dst port, ACL rule applied, packet/byte counters, etc.

However, this feature is always enabled for every network instance and
can produce lot of data which are then uploaded to the cloud.
We have seen cases where this amounted to hundreds of GB each week,
which was a burden for the controller's database.

Another drawback of flow-logging is that the iptables rules that EVE
installs for network instances are considerably more complicated because
of this feature and thus introduce additional packet processing overhead.

This API change introduced a new boolean option to disable flow logging
for a given Network Instance.
It is recommended that the controller disables flow logging unless it is
explicitly enabled by the user.

// By default, this feature is disabled. This is because it may potentially produce
// a large amount of data, which is then uploaded to the controller, and depending on
// the implementation, it may also introduce additional packet processing overhead.
bool enable_flowlog = 50;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This being an "enable" means that the default will be off since the controller(s) would not set it initially. I wonder if it is better making it a "disable" knob and then we can add a properly documented and notified default change to disable it in the controller.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can offer a configitem for this, if user do want this, they can explicitly enable this per device

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, changed to disable_flowlog.
Originally, I intended to make this a global configitem, but Daniel asked if this could be a NI option because configitems are not UX friendly.

Copy link
Contributor

@zed-rishabh zed-rishabh Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any difference between having an enableFlowlog variable set to False by default and a disableFlowlog variable set to True. Both approaches achieve the same result: disabling flow logs by default. The key point is that the default state of flow logs is determined by the initial value of the variable. Whether we use enableFlowlog = False or disableFlowlog = True, the outcome remains the same.
@eriknordmark Or do you mean that flow logs will be enabled by default from the controller, and at a later point, we'll disable them by default after properly communicating this change to the users?

Flow-logging is an EVE feature that allows to collect information
about every application network flow. This includes attributes like
src/dst IP, src/dst port, ACL rule applied, packet/byte counters, etc.

However, this feature is always enabled for every network instance and
can produce lot of data which are then uploaded to the cloud.
We have seen cases where this amounted to hundreds of GB each week,
which was a burden for the controller's database.

Another drawback of flow-logging is that the iptables rules that EVE
installs for network instances are considerably more complicated because
of this feature and thus introduce additional packet processing overhead.

This API change introduced a new boolean option to disable flow logging
for a given Network Instance.
It is recommended that the controller disables flow logging unless it is
explicitly enabled by the user.

Signed-off-by: Milan Lenco <milan@zededa.com>
Signed-off-by: Milan Lenco <milan@zededa.com>
@milan-zededa milan-zededa changed the title Add option to enable/disable flow logging for network instance Add option to disable flow logging for network instance Jul 23, 2024
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eriknordmark eriknordmark merged commit 106b1d6 into lf-edge:main Jul 23, 2024
4 checks passed
milan-zededa added a commit to milan-zededa/eden that referenced this pull request Aug 8, 2024
Soon EVE will allow to disable flow logging, which reduces the amount of
data published to the controller and simplifies iptables rules installed
by EVE.
See: lf-edge/eve-api#62

However, the ACL implementation is quite different between enabled and
disabled flow logging. Therefore, it makes sense to run the ACL test
twice to test both cases.

Signed-off-by: Milan Lenco <milan@zededa.com>
milan-zededa added a commit to milan-zededa/eden that referenced this pull request Aug 8, 2024
Soon EVE will allow to disable flow logging, which reduces the amount of
data published to the controller and simplifies iptables rules installed
by EVE.
See: lf-edge/eve-api#62

However, the ACL implementation is quite different between enabled and
disabled flow logging. Therefore, it makes sense to run the ACL test
twice to test both cases.

Signed-off-by: Milan Lenco <milan@zededa.com>
milan-zededa added a commit to milan-zededa/eden that referenced this pull request Aug 8, 2024
Soon EVE will allow to disable flow logging, which reduces the amount of
data published to the controller and simplifies iptables rules installed
by EVE.
See: lf-edge/eve-api#62

However, the ACL implementation is quite different between enabled and
disabled flow logging. Therefore, it makes sense to run the ACL test
twice to test both cases.

Signed-off-by: Milan Lenco <milan@zededa.com>
milan-zededa added a commit to milan-zededa/eden that referenced this pull request Aug 8, 2024
Soon EVE will allow to disable flow logging, which reduces the amount of
data published to the controller and simplifies iptables rules installed
by EVE.
See: lf-edge/eve-api#62

However, the ACL implementation is quite different between enabled and
disabled flow logging. Therefore, it makes sense to run the ACL test
twice to test both cases.

Signed-off-by: Milan Lenco <milan@zededa.com>
milan-zededa added a commit to milan-zededa/eden that referenced this pull request Aug 9, 2024
Soon EVE will allow to disable flow logging, which reduces the amount of
data published to the controller and simplifies iptables rules installed
by EVE.
See: lf-edge/eve-api#62

However, the ACL implementation is quite different between enabled and
disabled flow logging. Therefore, it makes sense to run the ACL test
twice to test both cases.

Signed-off-by: Milan Lenco <milan@zededa.com>
milan-zededa added a commit to lf-edge/eden that referenced this pull request Aug 15, 2024
Soon EVE will allow to disable flow logging, which reduces the amount of
data published to the controller and simplifies iptables rules installed
by EVE.
See: lf-edge/eve-api#62

However, the ACL implementation is quite different between enabled and
disabled flow logging. Therefore, it makes sense to run the ACL test
twice to test both cases.

Signed-off-by: Milan Lenco <milan@zededa.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants