pierky / arouteserver

A tool to automatically build (and test) feature-rich configurations for BGP route servers.
https://arouteserver.readthedocs.org/
GNU General Public License v3.0
284 stars 46 forks source link

Configurable BIRD logging #75

Closed bluikko closed 3 years ago

bluikko commented 3 years ago

I propose the logging classes in templates/bird/header.j2 be configurable, with the default value of all:

log "/var/log/bird.log" all;
log syslog all;

Perhaps something like:

I can volunteer to make a PR for it if other people consider it useful.

If not, I guess I will just use a custom header.j2.

Edit: I feel that value of all for syslog is actually redundant since the file already catches trace level messages. Trace level messages can have a large volume.

pierky commented 3 years ago

Hello @bluikko,

would the usage of Site-specific custom configuration files help to customize the logging configuration of the BGP speaker?

The header file is placed right after the part of the template where the default log settings are configured, so custom snippets of configuration containing the desired logging levels can be added there and used to override the default ones.

Implementing platform specific knobs like logging levels and methods into the "core" of ARouteServer could be a bit tricky. The "core" is supposed to be platform agnostic (as far as it's possible), and focused on the actual behaviour of the route server, and not on the platform specific settings. A solution in that direction should be abstract and flexible enough to be implemented on both BIRD and OpenBGPD, which might be hard to achieve. That's why I originally introduced those "customizable include files" in the tool.

nicko170 commented 3 years ago

This was something on my ever growing list of things to look at, as the default log location in arouteserver is annoying with logrotate and would prefer it in a separate directory.

I think @pierky has hit the nail on the head with leaving it out of the core and placing it in an override file, so I'll scratch that off my list and use a custom file now!

bluikko commented 3 years ago

would the usage of Site-specific custom configuration files help to customize the logging configuration of the BGP speaker?

I looked at that when trying to find out how to accomplish this. But it was not totally clear to me how it worked.

I agree it would probably be a better way. But the documentation could use improving then. For example it could refer to the .j2 files in the repository to give a better understanding which piece goes where and what are the defaults.

Edit: after re-reading the documentation I still think it would be nice if the locations were documented. For example, is header include inserted after the header.j2 template?

bluikko commented 3 years ago

The Docker image does not currently support --use-local-files.

I see a few different ways to add that support:

  1. Expect the files in some fixed paths and if they exist then add automatically the --use-local-files switch with parameters depending on which files exist.
  2. Check if some environment variable exists and if it does pass its contents to --use-local-files.

Opinions on how it should be implemented?

pierky commented 3 years ago

Hello @bluikko, I'm implementing it via env variables.

Having the files available to the Docker container at a fixed path in my opinion makes things even more confusing: those files are not "consumed" by ARouteServer at all, it only adds an include statement into the configuration files, pointing to them, so that the BGP daemon will eventually be able to pull them and make them part of the final config.

bluikko commented 3 years ago

Hello @bluikko, I'm implementing it via env variables.

That sounds great!

Edit: The documentation about it is now much better after https://github.com/pierky/arouteserver/commit/35f95ccd6cd271ca896dcffaa247a769c221344d

bluikko commented 3 years ago

I will close this since https://github.com/pierky/arouteserver/commit/b0d1db813d41ddf6629b1f6c0e121af284af2da3 seems to work (will test to actually load the config later today).

bluikko commented 3 years ago

I see the tag v1.7.0 does not include this change yet, may I inquire about your timeline for releasing this change?

pierky commented 3 years ago

I see the tag v1.7.0 does not include this change yet, may I inquire about your timeline for releasing this change?

I hope to push it by the end of the day.

pierky commented 3 years ago

Hello @bluikko, 1.7.0-post1 is out on DockerHub (referenced by :latest too).

bluikko commented 3 years ago

It works fine. This is a useful feature, now I can set logging options properly.

Edit: the Docker image works, not the logging configuration.

bluikko commented 3 years ago

@pierky So after all this all I can say that it doesn't work, unless there is some undocumented trick to remove the "TRACE" and "INFO" classes later from the first log syslog line.

If the main BIRD 2.0.8 configuration file has the line

log syslog all;

with the "local include"

include "/etc/bird/header.local";

which contains

log syslog { warning, error, fatal, remote, auth, bug };

nothing changes. The first log syslog all line in the main configuration file overrides the included more specific logging line. All, including TRACE level messages continue to be syslogged.

If I as a test copy the line from header.local to main configuration file, to overwrite the original log syslog all then syslog is not flooded anymore.


Edit: I have been thinking how to implement this ("log only >INFO level messages to syslog") and following options are obvious:

I guess I would go with the first option.

I have always wondered about the usefulness of logging with full TRACE level to both a file and syslog. I would've expected only a minority wishes to send what is very easily tens of GBs of logs to their centralized logging. If they even ship logs - if not then there are just duplicate logs in the system. I would have expected most people would not care to ship TRACE or possibly even INFO level messages.

pierky commented 3 years ago

Thanks @bluikko,

I'm going to implement something very similar to option 1.

My plan is to introduce a new option for the --use-local-files argument: logging. When logging is passed to --use-local-files, the lines of configuration related to logging will be omitted from the template, and the logging.local file included in place of them: so, when something like ... --use-local-files logging is used, the following 3 lines...

log "/var/log/bird.log" all;
log syslog all;
debug protocols { states, routes, filters, interfaces, events };

... will be replaced with this:

include "/etc/bgpd/header.local"

This seems to me the best backward-compatible and flexible approach. People who are relying on the 3 lines above will continue to have them in their configuration (unless they start passing logging to --use-local-files). Also, this allows a flexible and custom configuration of the logging. It can also be used from the container as all the other --use-local-files options are already. And it can be extended to both BIRD and OpenBGPD.

P.S.: yeah, probably it's not very close to your option 1 :-)

pierky commented 3 years ago

@bluikko, I've just pushed a new branch, "dev", with the code that implements the solution above.

To test it, you can fetch the repo, checkout the branch and build the Docker image yourself:

# from the root of the repository:
docker build \
            -t pierky/arouteserver:latest \
            -f docker/Dockerfile \
            .

The updated doc is available at the "dev" instance of readthedocs.io: https://arouteserver.readthedocs.io/en/dev/CONFIG.html#site-specific-custom-configuration-files

bluikko commented 3 years ago

... will be replaced with this:

include "/etc/bgpd/header.local"

This was probably a typo and you meant logging.local.

I have built the dev image and it works. I also think the solution is beautiful.

I will try the configuration later today when I can reconfigure BIRD but I see no reason why it wouldn't work.

Additionally, I think the documentation is good but it might be beneficial to refer the reader to the default logging configuration. I have created a quick PR for this. But I do feel that it is a bit repeating the links from the previous part of the documentation where the links to header.j2 exist when talking about position of the includes etc.

Edit: BIRD is using the new configuration and it looks good.

pierky commented 3 years ago

I've just merged everything into master, the CI/CD pipeline is running, if everything goes well 1.80 should be released in about 1 hour.

pierky commented 3 years ago

Cool, 1.8.0 is out, also on DockerHub. Thanks for your contribution, you helped me a lot to find a pretty good solution to the logging issue!