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

Add a kubernetes-log4j module #5718

Merged
merged 13 commits into from
Mar 25, 2024
Merged

Add a kubernetes-log4j module #5718

merged 13 commits into from
Mar 25, 2024

Conversation

ppkarwasz
Copy link
Contributor

Description

The kubernetes-log4j module adds the ability to Log4j Core to use Kubernetes attributes in a configuration file.

It is a cleaned-up version of the org.apache.logging.log4j:log4j-kubernetes, which is also released under ASL 2.0.

As explained in #5682, it does make more sense to host is here since:

  • it only depends on a very stable StrLookup dependency from log4j-core,
  • the number and kind of properties available through kubernetes-client depend on its version.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@ppkarwasz
Copy link
Contributor Author

For compatibility with org.apache.logging.log4j:log4j-kubernetes the lookup allows to override the automatic Kubernetes Client configuration using Log4j property sources. If this does not make sense in this project, I can remove the ClientBuilder and ClientProperties classes.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions

23.7% Coverage on New Code (required ≥ 80%)
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

@ppkarwasz ppkarwasz marked this pull request as draft January 22, 2024 09:11
@ppkarwasz
Copy link
Contributor Author

I am converting this to draft, since I need to add tests for most of the code.

Do you have an util to replace the ContainerUtil from the original log4j-kubernetes? The class tries to determine the id of the Kubernetes container, by parsing /proc/self/cgroup on Linux, which is error prone and not easy to test.

log4j/pom.xml Outdated Show resolved Hide resolved
log4j/pom.xml Outdated Show resolved Hide resolved
@manusa
Copy link
Member

manusa commented Jan 25, 2024

If this does not make sense in this project, I can remove the ClientBuilder and ClientProperties classes.

I'm not sure how does this play with our own property inference logic:

public static void configFromSysPropsOrEnvVars(Config config) {

I assume that for backwards-compatibility this is necessary, so I'm fine keeping it.

@manusa
Copy link
Member

manusa commented Jan 25, 2024

Do you have an util to replace the ContainerUtil from the original log4j-kubernetes?

Not that I know of.

pom.xml Outdated Show resolved Hide resolved
pom.xml Show resolved Hide resolved
@ppkarwasz ppkarwasz marked this pull request as ready for review February 18, 2024 21:20
@ppkarwasz
Copy link
Contributor Author

@manusa,

Thank you for your review. I finally had some time to work on this PR.

Apart from addressing your remarks above:

  • I introduced a system property kubernetes.log4j.useProperties. When set to false (default) Log4j properties are not used to configure the Kubernetes client. I think that this is a good compromise to maintain compatibility with log4j-kubernetes, but require users to set a special property to enable it.
  • I increased the test coverage.

rgoers and others added 8 commits March 19, 2024 13:13
This module adds the ability to Log4j Core to use Kubernetes attributes
in a configuration file.

It is a cleaned-up version of the
`org.apache.logging.log4j:log4j-kubernetes`.

As explained in fabric8io#5682, it does make more sense to host is here since:

 * it only depends on a very stable `StrLookup` dependency from
   `log4j-core`,
 * the number and kind of properties available through
   `kubernetes-client` depend on its version.
Adds a `kubernetes.log4j.useProperties` Java system property to disable
the usage of Log4j properties.

Increases test coverage.
@ppkarwasz
Copy link
Contributor Author

The OpenShift failures seem unrelated to my PR.

@ppkarwasz ppkarwasz requested a review from manusa March 21, 2024 17:52
@rohanKanojia
Copy link
Member

@ppkarwasz You're right. These have been failing for a while now #5545

@rohanKanojia
Copy link
Member

@ppkarwasz : Is it possible to add some README or user documentation in doc folder regarding how to use this?

@ppkarwasz
Copy link
Contributor Author

@rohanKanojia,

I have added a doc/KubernetesLog4j.md file in the /doc folder.

Copy link

Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thx!

@manusa manusa added this to the 6.11.0 milestone Mar 25, 2024
@manusa manusa merged commit f1b105b into fabric8io:main Mar 25, 2024
17 of 19 checks passed
@ppkarwasz ppkarwasz deleted the log4j-lookup branch March 25, 2024 11:50
ppkarwasz added a commit to apache/logging-log4j2 that referenced this pull request Mar 25, 2024
Due to differences in the lifecycle of Log4j Core and Kubernetes Client,
we remove `log4j-kubernetes` from the 3.x release and redirect users to
Fabric8's own `kubernetes-log4j` artifact introduced in fabric8io/kubernetes-client#5718.

The `log4j-kubernetes` lookup depends on:

* the very stable `StrLookup` interface from Log4j Core,
* the evolving set of Kubernetes metadata provided by Kubernetes Client.

Therefore it makes more sense to distribute the lookup together with
Kubernetes Client.
@ppkarwasz
Copy link
Contributor Author

@manusa,

Thank you for offering to maintain this component. If some Log4j-specific issues were to appear with it, feel free to @-mention me.

@manusa
Copy link
Member

manusa commented Mar 25, 2024

@manusa,

Thank you for offering to maintain this component. If some Log4j-specific issues were to appear with it, feel free to @-mention me.

Will do :) Thanks for the contribution!

ppkarwasz added a commit to apache/logging-log4j2 that referenced this pull request Mar 26, 2024
Due to differences in the lifecycle of Log4j Core and Kubernetes Client,
we remove `log4j-kubernetes` from the 2.x release and redirect users to
Fabric8's own `kubernetes-log4j` artifact introduced in fabric8io/kubernetes-client#5718.

The `log4j-kubernetes` lookup depends on:

* the very stable `StrLookup` interface from Log4j Core,
* the evolving set of Kubernetes metadata provided by Kubernetes Client.

Therefore it makes more sense to distribute the lookup together with
Kubernetes Client.
ppkarwasz added a commit to apache/logging-log4j2 that referenced this pull request Mar 27, 2024
Due to differences in the lifecycle of Log4j Core and Kubernetes Client,
we remove `log4j-kubernetes` from the 2.x release and redirect users to
Fabric8's own `kubernetes-log4j` artifact introduced in fabric8io/kubernetes-client#5718.

The `log4j-kubernetes` lookup depends on:

* the very stable `StrLookup` interface from Log4j Core,
* the evolving set of Kubernetes metadata provided by Kubernetes Client.

Therefore it makes more sense to distribute the lookup together with
Kubernetes Client.
ppkarwasz added a commit to apache/logging-log4j2 that referenced this pull request Mar 27, 2024
Due to differences in the lifecycle of Log4j Core and Kubernetes Client,
we remove `log4j-kubernetes` from the 3.x release and redirect users to
Fabric8's own `kubernetes-log4j` artifact introduced in fabric8io/kubernetes-client#5718.

The `log4j-kubernetes` lookup depends on:

* the very stable `StrLookup` interface from Log4j Core,
* the evolving set of Kubernetes metadata provided by Kubernetes Client.

Therefore it makes more sense to distribute the lookup together with
Kubernetes Client.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants