grails / grails-core

The Grails Web Application Framework
http://grails.org
Apache License 2.0
2.78k stars 951 forks source link

new logback situation hasn't been thought through #12399

Open xpusostomos opened 2 years ago

xpusostomos commented 2 years ago

Feature description

Back in the good old days, when we had logback.groovy, the standard logback.groovy that create-app gave you, had some conditionals based on being in development mode.

The new logback.xml in grails 5.1 doesn't. Is this because the grails team had a change of heart about what the default logging should be? Probably not.

So it raises the question, how do you do conditional login setup based on grails 5?

According to the logback documentation, it encourages you to have a logback-test.xml which it says you should put in src/test/resources . This doesn't work, as far as I can see, it isn't picked up when you run grails as a development run. So that option seems ruled out, and besides which, it doesn't seem to follow the grails philosophy to have a test setup that isn't in the war, and isn't based on the development/test/production trilogy.

So the other other option alluded to in the logback documentation is to use janino:

implementation "org.codehaus.janino:janino:3.1.6"

However this isn't included in the standard grails setup.

Then you can put this in logback.xml:

    <if condition='property("grails.env").equals("development")'>
        <then>
                blah
       </then
    </if>

Since supposedly logback.groovy is too dangerous (How? If you have access to the server, then you're screwed anyway)... but when I look at what Janino is... it appears to be some kind of a Java interpreter. Does swapping one pseudo java/groovy interpreter for another java interpreter save you from anything? I don't know.

Anyway, it seems to me that grails needs to give users guidance on the official way to do environment based log setup in this new world. If it's Janino, then I think it should be in standard grails... or at least documented.

lifeweaver commented 2 years ago

I miss Logback configuration via Groovy too, but I switched over to configuring in a Groovy class, and it's not too bad. It's certainly better than XML in my opinion.

xpusostomos commented 2 years ago

@lifeweaver What do you mean by configuring in a groovy class?

lifeweaver commented 2 years ago

https://logback.qos.ch/manual/configuration.html, see # 3 about the 'service-provider loading facility'. Then you can just configure your logging via a class implementing the logback Configurator class. Note this link was useful(Part about ContextAwareBase): https://stackoverflow.com/a/65117149/2137125. I would have preferred to implement ApplicationContextInitializer, but I couldn't figure out how to make it work with Grails.

xpusostomos commented 2 years ago

@lifeweaver The thing is... you kinda want to be able to configure logging at runtime, after it's deployed if necessary. Now one could load one of these classes dynamically I suppose, but then you have a poor man's logback.groovy.

I still don't get the rationale for removing logback.groovy. Maybe there are some scenarios, like a desktop app, where it would be dangerous, but on a server app, you control the server's files, you control the server period. I think grails should take over the logback.groovy interface for use in grails. Let people decide if this option is right for them.

lifeweaver commented 2 years ago

I understand runtime log configuration is a feature many use, in my situation I couldn't wait. As for the rationale, the Logback team made the decision, I don't like it and agree with your thoughts. If you feel strongly about it, I suggest checking out: https://jira.qos.ch/browse/LOGBACK-1606, someone has already suggested putting it as a separate module.

davidkron commented 2 years ago

As Grails effectively is a Spring Boot application, there is this option, without adding additional dependencies: https://docs.spring.io/spring-boot/docs/2.6.x/reference/htmlsingle/#features.logging.logback-extensions.profile-specific

I can't remember why, but for this to work you have to name the file logback-spring.xml.

xpusostomos commented 2 years ago

In my opinion, Grails should refrain from using this new logback, until the groovy configuration is fixed, whether by a new logback module or whatever. There's no benefit here for the grails community, even if it benefits some other usage scenarios. To my mind, logback have [made a mistake with] their product.

osscontributor commented 2 years ago

@xpusostomos Thank you for all of the feedback. It really is appreciated. I edited your previous comment to use more inclusive language without changing the intent of the message.

Thank you again.

osscontributor commented 2 years ago

So it raises the question, how do you do conditional login setup based on grails 5?

There are a few options. One is described at https://github.com/grails/grails-core/releases/tag/v5.1.2 which includes setting logback.version=1.2.7.

humberthardy commented 2 years ago

Another option can be use a different XML via configuration. Let's say:

You just have to set the logging.config property in application.yaml:

environments:
  development:
    logging:
      config: "grails-app/conf/logback-development.xml"
osscontributor commented 2 years ago

Since supposedly logback.groovy is too dangerous

I don't consider logback.groovy to be particularly dangerous. The kinds of problems one could create there could be created lots of places.

Trebla7th commented 2 years ago

I don't consider logback.groovy to be particularly dangerous. The kinds of problems one could create there could be created lots of places.

It seems that most of the community (myself included) agrees with this and disagrees with the decision to remove groovy configuration support. The reason provided by Logback in the release notes (version 1.2.9 - https://logback.qos.ch/news.html) is:

Removed Groovy configuration support. As logging is so pervasive and configuration with Groovy is probably too powerful, this feature is unlikely to be reinstated for security reasons.

and this change "...is intended to prevent an escalation of an existing flaw to a higher threat level."

chrisbitmead commented 2 years ago

@Trebla7th with all due respect, nobody in "the community" has justified this decision. Logback.groovy is generally inside the war, and EVERYTHING inside the war is executable dangerous stuff.... including for example, the ability to put in an application.groovy. And if you decide to make the effort to put it outside the war, then it will be in the tomcat folder, typically, which is also chock full of executable dangerous stuff. If somebody has access to changing your server's config, then you are completely toast already. And also, you can put executable code in logback.xml, so what are we even discussing?

This situation might be different for a client side app... but surely 95% of people using logback are doing server stuff.

Trebla7th commented 2 years ago

@chrisbitmead I think you misunderstood (or perhaps I misunderstand you). I was saying that, by and large, "the community" agrees with Jeff's comment that logback.groovy isn't particularly dangerous, and removal of support for groovy configuration doesn't really prevent anything.

brdbry commented 1 year ago

If you want to keep your old logback.groovy file, you can now do it by installing this library: https://github.com/virtualdogbert/logback-groovy-config

It's also mentioned in the logback docs (https://logback.qos.ch/manual/configuration.html) but adding here in case helpful to others.

brdbry commented 1 year ago

@lifeweaver, thanks for this:

Then you can just configure your logging via a class implementing the logback Configurator class

I got Configurator code working, but then during bootstrap spring-boot-starter-logging ignored it and set up its own default logback config. As far as I can see Spring docs only refer to customisation using config files: https://docs.spring.io/spring-boot/docs/2.1.8.RELEASE/reference/html/howto-logging.html

How did you get Spring to use your code config?

xpusostomos commented 1 year ago

@brdbry I welcome any work in this area, and continue to use logback.groovy via the suggestion above. It would be helpful if you could explain how what you've done differs from the above solution of setting logback version, and also in your repository you mention security, but nobody has ever explained to me why or when I should care about security regarding this.

brdbry commented 1 year ago

@xpusostomos It's not my library, I just used it because setting logback.version=1.2.7 for me caused a bunch of errors on startup. I'd have much preferred Logback to have kept the groovy support themselves.

lifeweaver commented 1 year ago

@lifeweaver, thanks for this:

Then you can just configure your logging via a class implementing the logback Configurator class

I got Configurator code working, but then during bootstrap spring-boot-starter-logging ignored it and set up its own default logback config. As far as I can see Spring docs only refer to customisation using config files: https://docs.spring.io/spring-boot/docs/2.1.8.RELEASE/reference/html/howto-logging.html

How did you get Spring to use your code config?

Check out https://stackoverflow.com/a/29146141/2137125

It sounds like that Groovy library is better tbh, but after going from XML to Groovy to Java, I don't want to have to redo it a fourth time.

yuri1969 commented 1 year ago

Another option can be use a different XML via configuration. Let's say:

* `logback-development.xml` -> ConsoleAppender

* `logback.xml` -> FileAppender

You just have to set the logging.config property in application.yaml:

environments:
  development:
    logging:
      config: "grails-app/conf/logback-development.xml"

The logging.config is intended for external configs. It won't resolve the classpath resource when deployed as JAR/WAR.

chrisbitmead commented 1 year ago

@yuri1969 I'm not sure what you mean by "external configs". If you mean that its intended that you define logging.config externally, that's problematic for a few reasons... (1) If you load application config externally (either via the external-config plugin, or just by rolling your own, in various ways described in grails books), logging has already commenced prior to that code being run, so you don't get to tell it where it is externally. (2) there's no way I can think of to set it in Tomcat externally. Obviously you can put it on the command line, but that then applies to all apps deployed, not just to your one.

In your example you seem to be setting it to an internal location.

The ability to define logging externally is problematic and not thought out in grails.

yuri1969 commented 1 year ago

@chrisbitmead Thanks for reaction. Actually, I don't need external configuration at all. I was just trying to solve the issue of having a different Logback config for each grails.env - using XML config. For some reason the Spring Boot's approach of using a single logback-spring.xml file did not work using Grails 5.1.8.

The next approach was splitting the config file to per-env files referenced from application.yml using environment's logging.config. That approach was the quoted one. It fails after you bulid a JAR file and run it since it naturally lacks the grails-app dir.

puneetbehl commented 1 year ago

@yuri1969 Can you please try using environments block with Grails 6?

yuri1969 commented 1 year ago

@yuri1969 Can you please try using environments block with Grails 6?

I'm sorry, using the environments bock seems to work even with 5.1.8 - on a freshly generated project.

When declaring a custom config for the production env, building via ./gradlew clean bootJar followed by java -jar build/libs/*.jar the app uses the custom config.

environments:
    production:
        logging:
            config: "grails-app/conf/logback-foo.xml"

Does it mean the logigng.config property is intended to be used even with non-external config files? If so, it would be great to mention that in the documentation section.

xpusostomos commented 1 year ago

It would be interesting to see if it works in tomcat too. I kind of doubt it. You might have found a quirk only.

1442acb42ee0e3a727d7330e9755eb94 commented 11 months ago

I would not change the location of logging.config, because it does not honor the convention of a leading/missing slash(/) in front of the file location very well and if you include WEB-INF/classes in the path, the grails app crashes. It's buggy and makes you rip out your hair.

But placing this code snippet into logback.xml works quite well.

`<?xml version="1.0" encoding="UTF-8"?>

UTF-8 %clr(%d{yyyy-MM-dd HH:mm:ss.SSS}){faint} %clr(%5p) %clr(---){faint} %clr([%15.15t]){faint} %clr(%-40.40logger{39}){cyan} %clr(:){faint} %m%n%wex ${HOMEDIR}${PROJECTNAME}-production-%d{yyyy-MM-dd}.log 30 2GB UTF-8 %clr(%d{dd.MM.yyyy HH:mm:ss.SSS}) %level %logger - %msg%xThrowable%n ${HOMEDIR}${PROJECTNAME}-production-stacktrace-%d{yyyy-MM-dd}.log 30 2GB UTF-8 %clr(%d{dd.MM.yyyy HH:mm:ss.SSS}) %level %logger - %msg%n ${HOMEDIR}${PROJECTNAME}-development-%d{yyyy-MM-dd}.log 30 2GB UTF-8 %clr(%d{dd.MM.yyyy HH:mm:ss.SSS}) %level %logger - %msg%xThrowable%n ${HOMEDIR}${PROJECTNAME}-development-stacktrace-%d{yyyy-MM-dd}.log 30 2GB UTF-8 %clr(%d{dd.MM.yyyy HH:mm:ss.SSS}) %level %logger - %msg%n

`

jamesfredley commented 1 month ago

To get the above xml example working on Grails 6.2.0 and Logback 1.2.13 I had to name the config file grails-app/config/logback-spring.xml and wrap it with the <configuration> root xml element.

<?xml version="1.0" encoding="UTF-8"?>
<configuration>
   ...
</configuration>

https://docs.spring.io/spring-boot/docs/2.1.13.RELEASE/reference/html/boot-features-logging.html#boot-features-logback-extensions

1442acb42ee0e3a727d7330e9755eb94 commented 1 month ago

There is a spelling error in my code above It should be StackTrace not StrackTrace

1442acb42ee0e3a727d7330e9755eb94 commented 1 month ago

Not sure if when you use logback-spring.xml instead of logback.xml you run into the location problem of the config file being not found in different environments (dev vs prod) and tomcat vs embedded.

jamesfredley commented 1 month ago

In Grails 6.2.0 (should be the same 5 also) with Logback 1.2.13 you can get back to having environment specific (development/test/production) configuration by placing your logback config in grails-app/config/logback-spring.xml. The following is pretty close to the old default logging config for a new grails application in logback.groovy, except it writes everything to CONSOLE, which is the default in Spring Boot 2.

https://docs.spring.io/spring-boot/docs/2.7.18/reference/html/howto.html#howto.logging.logback

https://docs.spring.io/spring-boot/docs/2.7.18/reference/html/features.html#features.logging.logback-extensions

<?xml version="1.0" encoding="UTF-8"?>
<configuration>
    <include resource="org/springframework/boot/logging/logback/defaults.xml"/>
    <include resource="org/springframework/boot/logging/logback/console-appender.xml"/>
    <root level="ERROR">
        <appender-ref ref="CONSOLE"/>
    </root>

    <springProfile name="development">
        <logger name="StackTrace" level="ERROR" additivity="false"/>
<!--        <logger name="com.mypackage" level="DEBUG"/>-->
<!--        <root level="WARN"/>-->
    </springProfile>
</configuration>

If you would rather write the logs to a file: https://docs.spring.io/spring-boot/docs/2.7.18/reference/html/howto.html#howto.logging.logback.file-only-output