bgp / stayrtr

RPKI-To-Router server implementation in Go
BSD 3-Clause "New" or "Revised" License
91 stars 13 forks source link

Add syslog flag to log to syslog #11

Closed aknit444 closed 1 year ago

aknit444 commented 3 years ago

Greetings.

Grateful if the code can be updated to send logs to Syslog rather than to stdout, in cases where no logging options have been specified upon start-up of StayRTR. Thanks.

Mark.

randomthingsandstuff commented 3 years ago

This is not normal practice in modern era. The usual practice is to use systemd + journald. I'm not okay with this as this as default. There should be some syslog capabilities, however.

aknit444 commented 3 years ago

In my case, I am running stayrtr on FreeBSD. AFAIK, FreeBSD does not support systemd or journald. So this wouldn't work.

lspgn commented 3 years ago

I am not familiar with FreeBSD but I believe daemon should work and allow you to redirect stdout to syslog.

aknit444 commented 3 years ago

I am not familiar with FreeBSD but I believe daemon should work and allow you to redirect stdout to syslog.

Seems overly onerous for what might be a simple requirement. I understand Linux takes a different approach, but I would also anticipate that including Syslog support for non-Linux systems would be in keeping with basic needs.

lspgn commented 3 years ago

This is my take as previous maintainer of Cloudflare's GoRTR: I don't think adding a syslog library is a simple requirement. This would add a handful of CLI args and usage would be redundant for many (?) users and delegating the implementation of syslog to the logrus library instead of using OS provided tools.

It would help me if you could provide me the use case where you would not use daemon which also handles restarts : running in foreground and having syslog requirement. Eventually, that would restrain from: minimal Linux, Windows and possibly Mac OS. This said, I am not sure if a StayRTR packaged onto routers (Juniper/Arista) would have its stdout sent into the device logs.

Overall I agree with @randomthingsandstuff saying this is not a normal practice in a modern era. I would add from my experience on server telemetry that I mostly see file logging + fluentbit/logstash to NewRelic/DataDog/Splunk/Loki these days.

randomthingsandstuff commented 3 years ago

My only objection is syslog as a default. Syslog appears simple to implement in this case.

I support a flag to enable logrus to do local syslog (along WITH stdout.. no option to turn that off). So, no syslogging to wherever.. do it locally and sort it out there if you want to forward.

Logrus' syslog handler thingy seems reasonable. It maps severities as such:

    switch entry.Level {
    case logrus.PanicLevel:
        return hook.Writer.Crit(line)
    case logrus.FatalLevel:
        return hook.Writer.Crit(line)
    case logrus.ErrorLevel:
        return hook.Writer.Err(line)
    case logrus.WarnLevel:
        return hook.Writer.Warning(line)
    case logrus.InfoLevel:
        return hook.Writer.Info(line)
    case logrus.DebugLevel, logrus.TraceLevel:
        return hook.Writer.Debug(line)
    default:
        return nil
    }
aknit444 commented 3 years ago

This is my take as previous maintainer of Cloudflare's GoRTR: I don't think adding a syslog library is a simple requirement. This would add a handful of CLI args and usage would be redundant for many users and delegating the implementation of syslog to the logrus library instead of using OS provided tools.

It would help me if you could provide me the use case where you would not use daemon which also handles restarts : running in foreground and having syslog requirement.

Overall I agree with @randomthingsandstuff saying this is not a normal practice in a modern era. I would add from my experience on server telemetry that I mostly see file logging + fluentbit/logstash to NewRelic/DataDog/Splunk/Loki these days.

Well, just trying to keep the system as simple as possible, and not needing to run more services than is necessary to keep the system alive.

stayrtr is the only service on our FreeBSD systems that don't natively log to the base OS, and clog the foreground with logs. FreeBSD-13.0 is reasonably modern, so not sure how we address this without directing the discussion into philosophical OS preferences...

aknit444 commented 3 years ago

My only objection is syslog as a default. Syslog appears simple to implement in this case.

I support a flag to enable logrus to do local syslog (along WITH stdout.. no option to turn that off). So, no syslogging to wherever.. do it locally and sort it out there if you want to forward.

Logrus' syslog handler thingy seems reasonable. It maps severities as such:

  switch entry.Level {
  case logrus.PanicLevel:
      return hook.Writer.Crit(line)
  case logrus.FatalLevel:
      return hook.Writer.Crit(line)
  case logrus.ErrorLevel:
      return hook.Writer.Err(line)
  case logrus.WarnLevel:
      return hook.Writer.Warning(line)
  case logrus.InfoLevel:
      return hook.Writer.Info(line)
  case logrus.DebugLevel, logrus.TraceLevel:
      return hook.Writer.Debug(line)
  default:
      return nil
  }

I'm fine with Syslog not being default, but grateful if those of us on FreeBSD would have the option to use it.

That said, I would also request that if Syslog can be supported, we be given the option to turn off stdout, as the box we may be using for stayrtr is not dedicated to the service, can may be in use for other functions.

job commented 3 years ago

Perhaps a -daemon option should be introduced that launches StayRTR as daemon and returns the prompt to the operator.

If the -daemon option is set, all log messages should go to the Syslog facility.

aknit444 commented 3 years ago

Perhaps a -daemon option should be introduced that launches StayRTR as daemon and returns the prompt to the operator.

If the -daemon option is set, all log messages should go to the Syslog facility.

Perhaps, sure... :-).

job commented 3 years ago

This library can maybe help https://github.com/sevlyar/go-daemon

lspgn commented 3 years ago

As much as I am trying to welcome new features and improve my original design, I will say that: I understand from a UNIX and C-world that the norm was to provide daemons out of the box but Golang is the wrong tool for this. Golang and GoRTR were developed around 12-factors methodology. The reason is to make applications more modular and code more pleasant to maintain and operate in a devops perspective. The RPKI framework gave me a lot of headaches when implementing it (rsync, configs, monolithic, unobservable, to interpret) and as a result made operating at scale difficult. This is why I would like to warn against doing this for StayRTR.

The logs part:

A twelve-factor app never concerns itself with routing or storage of its output stream. It should not attempt to write to or manage logfiles. Instead, each running process writes its event stream, unbuffered, to stdout. During local development, the developer will view this stream in the foreground of their terminal to observe the app’s behavior.

I could consider to make an exception due to heavily used non-standard routing devices but not for BSD nor Windows.

The concurrency factor explicitly says:

Twelve-factor app processes should never daemonize or write PID files. Instead, rely on the operating system’s process manager (such as systemd, a distributed process manager on a cloud platform, or a tool like Foreman in development) to manage output streams, respond to crashed processes, and handle user-initiated restarts and shutdowns.

Please see this StackOverflow answer for additional details. Due to a language limitation to threads, I view adding daemon option is more of a hack which would add non-core features to maintain and more external dependencies.

This goes in contradiction with Mark statement. Yes, there is a philosophical context to it and I am opinionated on how I think programs should be written and operated, but not which OS to pick. The OS purpose is to schedule programs, it is not the choice of the program to daemonize itself nor decide on the logging. This shift the complexity onto N software maintainers (and therefore results in N different interpretations, implementations and bugs). The side discussion on should it be a default or muting stdout is an example of it. Whereas it allows me to have very similar and portable configuration files that I do not need to update if I need to change the syslog destination (which task can be delegated to another SRE team for instance).

Well, just trying to keep the system as simple as possible, and not needing to run more services than is necessary to keep the system alive. stayrtr is the only service on our FreeBSD systems that don't natively log to the base OS, and clog the foreground with logs. FreeBSD-13.0 is reasonably modern, so not sure how we address this without directing the discussion into philosophical OS preferences...

I won't push more on saying this should not be implemented as I believe I provided enough arguments. This was my mindset for GoRTR. I would hope it remains similar for StayRTR.

job commented 3 years ago

Upon more investigation, indeed modifying StayRTR to daemonize itself appears non-trivial.

Still, it seems harmless to make it possible to use the syslog facility natively.

randomthingsandstuff commented 3 years ago

Chatted with job. We're gonna add a syslog flag and call it a day for now. Additional experience will dictate things in the future.

dutchshark commented 2 years ago

I would follow @lspgn to NOT include the daemon option, my workable workaround for the syslog stuff would be easy as follows: stayrtr 2>&1 | logger -s -t $(basename $0) grep stayrtr /var/log/messages

Wouldn't this help you out @aknit444? as you did redirect your logs to syslog this way and your not using any extra binaries or adding fuzzy code into our yet so clean code base.

With stdout redirection to syslog the objective will be easily reached and none extra lines of code nor packages on the basesystem would be needed for this.

benjojo commented 1 year ago

daemon and direct syslog support is not planned at this time