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

Inherit defaults from Fabric8 kubernetes client defaults #43475

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

metacosm
Copy link
Contributor

Fixes #43474

Signed-off-by: Chris Laprun claprun@redhat.com

Copy link

github-actions bot commented Sep 24, 2024

🎊 PR Preview 5050908 has been successfully built and deployed to https://quarkus-pr-main-43475-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

This comment has been minimized.

This comment has been minimized.

Fixes quarkusio#43474

Signed-off-by: Chris Laprun <claprun@redhat.com>
@metacosm
Copy link
Contributor Author

I haven't considered backwards compatibility, not sure this is something that needs to be taken into account here.

Copy link

quarkus-bot bot commented Sep 25, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 243b4d3.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Sep 25, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 243b4d3.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@gastaldi gastaldi merged commit 239eb5e into quarkusio:main Sep 25, 2024
26 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 25, 2024
@metacosm metacosm deleted the fix-kube-client-defaults branch September 25, 2024 12:40
@manusa
Copy link
Contributor

manusa commented Sep 25, 2024

I'd vote to backport this PR too since it's addressing a big issue.
Quarkus is overriding some of the Kubernetes Client defaults which might drastically change the expected client behavior.
This is especially notable for the retry functionality which is effectively disabled by the overridden default.

@gsmet
Copy link
Member

gsmet commented Oct 18, 2024

@metacosm @manusa could you prepare a note for the migration guide as to which default values were changed to what? Because while I understand the new defaults are better, people might still rely on the old defaults for some use case: we need to document the change.

You can post a comment here and I'll post it to the appropriate migration guide.

@gsmet gsmet modified the milestones: 3.16.0.CR1, 3.15.2 Oct 18, 2024
@metacosm
Copy link
Contributor Author

The previous defaults appeared in KubernetesClientBuildConfig in the @Default annotations. The new ones cannot really be documented once and for all since the whole point of this PR is to follow whatever the client does as it changes over time.

@manusa
Copy link
Contributor

manusa commented Oct 21, 2024

I can try to compile the values from before and after and create a table with the changed ones.
Maybe we can clarify in the note that starting from this version the values will default to whatever you find in https://github.com/fabric8io/kubernetes-client#configuring-the-client

@manusa
Copy link
Contributor

manusa commented Oct 21, 2024

Hi @gsmet

I've compiled all of the configuration options, their old defaults, and the new defaults.
The following snippet contains some markdown that can be adapted to fit in the migration guide:

Starting from the xxxx version, the default values for the Kubernetes Client extension configuration have changed.
In the past, Quarkus had explicit values for some of the Kubernetes Client configuration options.
This was not ideal since some of these default settings were different to those provided by the client library which led to some confusion.
To address this, we have removed the explicit default values from the Kubernetes Client extension configuration and now rely on the client library defaults.

The following table contains all the configuration option entries, the previous default value, and the new default value (note that many of the values remain unchanged and others don't have a default value):

| Configuration Option          | Previous Default Value | New Default Value |
|-------------------------------|------------------------|-------------------|
| `watchReconnectInterval`      | `PT1S`                 | `PT1S`            |
| `watchReconnectLimit`         | `-1` (no limit)        | `-1` (no limit)   |
| `connectionTimeout`           | `PT10S`                | `PT10S`           |
| `requestTimeout`              | `PT10S`                | `PT10S`           |
| `requestRetryBackoffLimit`    | `0` (no retry)         | `10`              |
| `requestRetryBackoffInterval` | `PT1S`                 | `PT0.1S`          |
| `trustCerts`                  | n/a                    | `false`           |
| `apiServerUrl`                | n/a                    | n/a               |
| `namespace`                   | n/a                    | n/a               |
| `caCertFile`                  | n/a                    | n/a               |
| `caCertData`                  | n/a                    | n/a               |
| `clientCertFile`              | n/a                    | n/a               |
| `clientCertData`              | n/a                    | n/a               |
| `clientKeyFile`               | n/a                    | n/a               |
| `clientKeyData`               | n/a                    | n/a               |
| `clientKeyAlgo`               | n/a                    | `RSA`             |
| `clientKeyPassphrase`         | n/a                    | n/a               |
| `username`                    | n/a                    | n/a               |
| `password`                    | n/a                    | n/a               |
| `token`                       | n/a                    | n/a               |
| `httpProxy`                   | n/a                    | n/a               |
| `httpsProxy`                  | n/a                    | n/a               |
| `proxyUsername`               | n/a                    | n/a               |
| `proxyPassword`               | n/a                    | n/a               |
| `noProxy`                     | n/a                    | n/a               |

@manusa
Copy link
Contributor

manusa commented Nov 5, 2024

Hi @gsmet
I'm not sure if you manged to see my previous comment or if there's anything else to do on my side.

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

Successfully merging this pull request may close these issues.

Improper defaults in KubernetesClientBuildConfig
5 participants