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

Breaking API changes in minor releases #5682

Closed
ppkarwasz opened this issue Jan 3, 2024 · 9 comments
Closed

Breaking API changes in minor releases #5682

ppkarwasz opened this issue Jan 3, 2024 · 9 comments

Comments

@ppkarwasz
Copy link
Contributor

Describe the bug

As reported by one of our users in apache/logging-log4j2#2151, the kubernetes-model-core and kubernetes-client contain some breaking API changes in versions 6.2.0 and 6.5.0:

  • in version 6.2.0 the public method io.fabric8.kubernetes.api.model.ObjectMeta#getClusterName was removed,
  • in version 6.5.0 the methods io.fabric8.kubernetes.client.Config#getRollingTimeout and io.fabric8.kubernetes.client.ConfigBuilder#withRollingTimeout were removed.

As a result log4j-kubernetes compiles and works with version 6.1.1 of kubernetes-client, but stop working with version 6.2.0.

Fabric8 Kubernetes Client version

6.2.0

Steps to reproduce

  • Checkout https://github.com/apache/logging-log4j2;
  • Run
    ./mvnw verify -pl log4j-kubernetes -Dkubernetes-client.version=6.9.2

Expected behavior

We would expect kubernetes-client to follow semantic versioning, where public methods can be deprecated in a minor release, but they are only removed in the next major release.

At Log4j we use bnd-baseline-maven-plugin to automatically check for major API changes, but plenty of other tools can also be used.

Runtime

other (please specify in additional context)

Kubernetes API Server version

other (please specify in additional context)

Environment

Windows, Linux, macOS

Fabric8 Kubernetes Client Logs

No response

Additional context

No response

@manusa
Copy link
Member

manusa commented Jan 3, 2024

Hello @ppkarwasz,

We don't follow semantic versioning but we try to comply as much as possible.
The Kubernetes Model introduces many breaking changes despite Kubernetes being at 1.x.x and its core API being at v1.
Since we generate Java model types for the matching Go structs, and their upstream equivalents don't respect semver (despite claiming to), we're forced to introduce these breakages between minor releases.
However, we do make a good effort to document them and introduce Notices in the release notes.

I'm happy to switch to a strict semantic versioning policy. However, if we want to keep a short cycled release loop this can imply releasing a major version every 2 months. On the other hand, we can switch to a predictable release schedule, but this would very likely imply elongating the release cycle.

@ppkarwasz
Copy link
Contributor Author

@manusa,

Thanks a lot for the explanation.

In version 2.x of Log4j I will add the required try ... catch to prevent users from getting linkage errors with newer kubernetes-client versions (we try to work with the largest possible range of versions).

However for version 3.x of Log4j it would make more sense to have separate versions for each kubernetes-client release.
Would you be willing to include log4j-kubernetes in you project? log4j-kubernetes:

  • is a small (4 classes) module that allows users to use Kubernetes container attributes in their Log4j Core configuration (cf. documentation),
  • from Log4j's perspective, log4j-kubernetes only uses the StrLookup interface that has never changed, so an artifact provided by kubernetes-client will work with all Log4j Core versions.
  • it is low maintenance: the bug I mentioned above is the probably the first one since the module was introduced,
  • I can perform the initial submission, if you suggest me the new artifact name.

@manusa
Copy link
Member

manusa commented Jan 5, 2024

Hi, @ppkarwasz
Sorry for the late reply. We're discussing these matters internally. We will keep you posted.

@manusa
Copy link
Member

manusa commented Jan 18, 2024

Hi @ppkarwasz,

Regarding the switch to a different versioning policy, I'm afraid that we will need to keep with a similar approach to the one we're following now. Bumping a major version every other release will probably force us to keep maintenance branches for a few of them.

We'll try to integrate tooling to make sure that we only break what we consider non-critical (such as upstream Kubernetes API changes) and detect those more critical breakages that should be delayed to the next major or be approached with a compatibility layer.

Would you be willing to include log4j-kubernetes in you project? log4j-kubernetes:

  • is a small (4 classes) module that allows users to use Kubernetes container attributes in their Log4j Core configuration (cf. documentation),
  • from Log4j's perspective, log4j-kubernetes only uses the StrLookup interface that has never changed, so an artifact provided by kubernetes-client will work with all Log4j Core versions.
  • it is low maintenance: the bug I mentioned above is the probably the first one since the module was introduced,
  • I can perform the initial submission, if you suggest me the new artifact name.

I've been reviewing the module and this sounds good. I can also see it makes sense to have the lookup hosted here.

Let us know how we can help you.

@ppkarwasz
Copy link
Contributor Author

@manusa,

I've been reviewing the module and this sounds good. I can also see it makes sense to have the lookup hosted here.

Let us know how we can help you.

That is great news. As I mentioned above I can perform the initial submission. What would be the best place for such a component? I was thinking about adding an artifact id kubernetes-log4j-lookup in a log4j-lookup folder of this repo.

ppkarwasz pushed a commit to ppkarwasz/kubernetes-client that referenced this issue Jan 19, 2024
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.
ppkarwasz pushed a commit to ppkarwasz/kubernetes-client that referenced this issue Jan 19, 2024
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.
ppkarwasz pushed a commit to ppkarwasz/kubernetes-client that referenced this issue Jan 19, 2024
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.
ppkarwasz pushed a commit to ppkarwasz/kubernetes-client that referenced this issue Jan 19, 2024
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.
@wind57
Copy link
Contributor

wind57 commented Jan 30, 2024

@manusa thank you for the explanation. I was wondering why sometimes we get updates in the fabric8 client and there were breaking changes. Now it makes a lot of sense.

ppkarwasz pushed a commit to ppkarwasz/kubernetes-client that referenced this issue Feb 18, 2024
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.
ppkarwasz pushed a commit to ppkarwasz/kubernetes-client that referenced this issue Mar 7, 2024
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.
ppkarwasz pushed a commit to ppkarwasz/kubernetes-client that referenced this issue Mar 19, 2024
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.
manusa pushed a commit that referenced this issue Mar 25, 2024
* Add `kubernetes-log4j` module

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 #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.

* Fix license header

* Add killswitch for Log4j properties

Adds a `kubernetes.log4j.useProperties` Java system property to disable
the usage of Log4j properties.

Increases test coverage.

* Fix dependencies and packaging

* Use `NamespaceBuilder` in test

* Add tests with mock client

* Add tests for `ContainerUtil`

* Split data-gathering code into methods

* Apply Sonarqube suggestions

* Fix license formatting

* Add missing JavaDoc

* Reach 80% test coverage

* Add documentation

---------

Co-authored-by: Ralph Goers <[email protected]>
@erathorus
Copy link

It seems that the new io.fabric8:kubernetes-log4j package doesn't include the Log4j2Plugins.dat file, I've described the details of this issue in #5847.

Copy link

stale bot commented Jun 29, 2024

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!

@stale stale bot added the status/stale label Jun 29, 2024
@ppkarwasz
Copy link
Contributor Author

I am closing this, since the reason for breaking changes has been explained.

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

No branches or pull requests

4 participants