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

make config parsing strict when starting gobgpd #2834

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

myaut
Copy link

@myaut myaut commented Sep 6, 2024

The default policy for gobgp if config is invalid is to start with default policy ACCEPT. This is harmful for our Route Reflectors setup in which we can get spurous routes after unlucky release.

This commit adds --config-strict option that makes gobgp do the fatal exit if config is invalid. If option is not specified, gobgpd will still log warning and continue working.

It also introduces special facilty logging field similar to the one in syslog. This allows to implement said backward compatibility with Fatal() calls with facility "config" being downgraded to Warn().

Reload behavior is not changed as it is not as harmful (policies are retained in such case).

Sergey Klyaus added 3 commits September 6, 2024 18:13
The default policy for gobgp if config is invalid is to start with
default policy ACCEPT. This is harmful for our Route Reflectors
setup in which we can get spurous routes after unlucky release.

This commit adds --config-strict option that makes gobgp do the
fatal exit if config is invalid. If option is not specified,
gobgpd will still log warning and continue working.

It also introduces special facilty logging field similar to the
one in syslog. This allows to implement said backward compatibility
with Fatal() calls with facility "config" being downgraded to Warn().

Reload behavior is not changed as it is not as harmful (policies
are retained in such case).
@SkalaNetworks
Copy link
Contributor

Hi, I think this is an extremely useful feature. It is easy to mess up a config and end up with full tables because your filters are not working for example (happened to me). But is using the logger to dismiss the configurations the best way to go through it?

In my opinion, there should be a way to read a config file using gobgp(d) and validating the policies before enforcing them. Error handling should be handled there.

@myaut
Copy link
Author

myaut commented Nov 7, 2024

@fujita I think I fixed the test. As I understood, the problem arises if BGP session is not yet established when we start waiting for hold timer expiry, so g2.wait_for(expected_state=BGP_FSM_ACTIVE, peer=g1) exits early catching initial session state. This is partially proven that I got test failures early:

Ran 5 tests in 43.884s

(less than 90s hold timer :) )

@myaut
Copy link
Author

myaut commented Nov 7, 2024

@SkalaNetworks thanks for interest in this patch! We can use config-strict freely because we have testing environment where we can tolerate total outage due to invalid config before we deploy to production. We weren't sure that everyone has such luxury, so it is an option.

As using log.Fatal for aborting daemon, this is somewhat standard technique, although I understand that it creates unwanted side effects nested deeply into code. But that's a simplest patch, so we decided to use Fatal.

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.

2 participants