gobgp icon indicating copy to clipboard operation
gobgp copied to clipboard

make config parsing strict when starting gobgpd

Open myaut opened this issue 1 year ago • 1 comments

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).

myaut avatar Sep 06 '24 16:09 myaut

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.

SkalaNetworks avatar Oct 20 '24 09:10 SkalaNetworks

@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 avatar Nov 07 '24 17:11 myaut

@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.

myaut avatar Nov 07 '24 17:11 myaut

Merged, thanks!

fujita avatar Dec 02 '24 14:12 fujita