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

Added whitelist for CLI log channels to include #2859

Merged
merged 2 commits into from
May 23, 2022
Merged

Conversation

nvoxland
Copy link
Contributor

@nvoxland nvoxland commented May 19, 2022

Impact

  • Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Marked as a breaking change because 3rd party (non-liquibase) logging will no longer be logged without also specifying the --log-channels flag.

Description

While we can't control the log configuration in most environments liquibase runs in, we do control the log setup in the CLI.

Liquibase does not log any sensitive information, but we have no control over what other libraries will log and want to make sure nothing unexpected gets logged in user environments.

Therefore, when applying the "logLevel" setting in the CLI, we are switching to a whitelisting of channels to include. The whitelist defaults to only "liquibase", but it can be controlled via a new liquibase.logChannel setting. For example, --log-channels=liquibase,com.microsoft.sqlserver.jdbc. We also support --log-channels=all

This replaces the previous reflection-based blacklisting of net.sun.www.protocol.http.HttpURLConnection which caused a warning in Java 11 and an ignored error in 17

Things to be aware of

  • Impacts the logging to both stdout and the log file
  • Only impacts in the CLI

Things to worry about

  • Nothing

Additional Context

I was not able to control the log level of net.sun.www.protocol.http.HttpURLConnection with the setting. I thing their use of the internal PlatformLogger is getting in the way. If anyone ever needs that, we can look at options for that. I was able to see it work with org.mariadb after adding ?log=true to the mariadb url

Testing

Setup: Configure liquibase to point to a mssql database

  • Running liquibase update with no logLevel should print no logs
  • Running liquibase --log-level=FINE update should log only messages with [liquibase ...] as the channel. No [com.microsoft...] channels
  • Running liquibase --log-level=FINE --log-channels=liquibase,com.microsoft update should log messages with both the liquibase and com.microsoft channels
  • Running the above update calls with and without --log-channels works as expected in Java 8 and 17
  • Running the update with and without --log-channels but with --log-file should have the same messages present/missing in the file as was in the console.

- Removed reflection-based blacklisting of `sun.net.www.protocol.http.HttpURLConnection` in favor of the new logChannels white list
@nvoxland nvoxland added this to the v4.11.0 milestone May 19, 2022
@nvoxland nvoxland requested a review from suryaaki2 May 19, 2022 21:56
@github-actions
Copy link

github-actions bot commented May 19, 2022

Unit Test Results

  4 512 files    4 512 suites   37m 3s ⏱️
  4 419 tests   4 205 ✔️    214 💤 0
52 308 runs  47 300 ✔️ 5 008 💤 0

Results for commit edb548a.

♻️ This comment has been updated with latest results.

@nvoxland
Copy link
Contributor Author

In testing --log-channels, we had issues with the sqlserver driver not logging with --log-channels=liquibase,com.microsoft on java 8 on a system with a non-EN_US locale on windows.

It logged correctly on that system with --log-channels=all and it logged correctly on java 8 with an english locale.
It logged correctly on the non-EN_US system with java 11 and 17

Our theory is that there is either a bug in java 8 or a bug in the mssql driver not handling the fact that java 8 (unlike 11 and 17) translates the log level. Either way, it's not something to change the implementation over given that 8 is old at this point and it does work with --log-channels=all.

If you run into this behavior and it causes problems for you, feel free to open a separate issue.

@nvoxland nvoxland merged commit 0a9d6d8 into master May 23, 2022
@nvoxland nvoxland deleted the add-log-channels branch May 23, 2022 15:16
@FBurguer
Copy link

For testing, i checked the points Nathan listed above:

  • Running liquibase update with no logLevel should print no logs PASS
  • Running liquibase --log-level=FINE update should log only messages with [liquibase ...] as the channel. No [com.microsoft...] channels PASS
  • Running liquibase --log-level=FINE --log-channels=liquibase,com.microsoft update should log messages with both the liquibase and com.microsoft channels PASS
  • Running the above update calls with and without --log-channels works as expected in Java 8 and 17 PASS
    (Aparently, when your env its not in english, Java 8 level names are not in english and this cause some issues when choosing the channel, further investigation is needed)
  • Running the update with and without --log-channels but with --log-file should have the same messages present/missing in the file as was in the console. PASS
  • Running the update with and with --log-channles=all, shows all the messages. PASS

Testing Environment:
Windows 10
Java 8,11,17
mssql-2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants