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

[bitnami/clickhouse] Fix: bind ipv6 and ipv4 by default #31200

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JulesdeCube
Copy link

Note

This PR is re-open of #31168 (see #31168 (comment))

Motivation

The current clickhouse helm chart don't support ipv6 only cluster.

Context

By default clickhouse bind localhost on ipv4 and ipv6 (see https://github.com/ClickHouse/ClickHouse/blob/master/programs/server/config.xml#L253).

The bitnami container override this behaviors and bind to any ipv4 by passing -- --listen_host=0.0.0.0 to clickhouse-server via the CMD (see https://github.com/bitnami/containers/blob/main/bitnami/clickhouse/24/debian-12/Dockerfile#L60).
this is easily override to listen on ivp6 by changing the container command to /opt/bitnami/scripts/clickhouse/run.sh -- --listen_host="::".

The helm chart use an hard coded script as it's entry with no possibility to pass/override argument to clickhouse-server. (see https://github.com/bitnami/charts/blob/main/bitnami/clickhouse/templates/scripts-configmap.yaml)

Description of the change

this PR:

  • remove the default clickhouse-server arguments.
  • allow to specify clickhouse-server arguments.
  • use the defaultConfigurationOverrides to listen on ipv4 and ipv6

Benefits

Add Support for ipv6 only cluster

Possible drawbacks

It's harder to change listen_host in the xml config file.

Applicable issues

related with OneUptime/oneuptime#1348

Additional information

Checklist

  • Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
  • Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
  • Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
  • All commits signed off and in agreement of Developer Certificate of Origin (DCO)

@JulesdeCube

This comment was marked as resolved.

@carrodher
Copy link
Member

carrodher commented Jan 2, 2025

I just accepted the invitation on behalf of @bitnami-bot and the job is running again. If everything is ok, a new commit should be added authored by the bot

@carrodher
Copy link
Member

It seems there is still something wrong with the permission:

remote: Permission to JulesdeCube/bitnami-charts.git denied to bitnami-bot.
fatal: unable to access 'https://github.com/JulesdeCube/bitnami-charts/': The requested URL returned error: 403

@JulesdeCube JulesdeCube changed the title [bitnami/clickhouse] Fix: bind ipv6 and ipv4 by default #31168 [bitnami/clickhouse] Fix: bind ipv6 and ipv4 by default Jan 2, 2025
@JulesdeCube JulesdeCube force-pushed the clickhouse-ipv6-support branch from 8606f09 to af80ed4 Compare January 2, 2025 15:43
@bitnami-bot bitnami-bot added verify Execute verification workflow for these changes in-progress labels Jan 2, 2025
@JulesdeCube
Copy link
Author

hello I manage to fix the probleme by running the CI localy and push the change.

@carrodher carrodher added in-progress and removed in-progress triage Triage is needed labels Jan 3, 2025
@github-actions github-actions bot removed the request for review from javsalgar January 3, 2025 11:36
@github-actions github-actions bot requested a review from fmulero January 3, 2025 11:36
@fmulero
Copy link
Collaborator

fmulero commented Jan 7, 2025

The 403 error is still happening. Please check if the @bitnami-bot user appears as collaborator in your repository https://github.com/JulesdeCube/bitnami-charts and you don't have protection rule set for this branch

@JulesdeCube JulesdeCube force-pushed the clickhouse-ipv6-support branch 3 times, most recently from 5da4b15 to 7781421 Compare January 7, 2025 16:16
@JulesdeCube

This comment was marked as resolved.

Copy link
Collaborator

@fmulero fmulero left a comment

Choose a reason for hiding this comment

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

Hi @JulesdeCube
The 403 error was in our side, the invitation was not managed properly.

About your changes I think we should keep IPv4 by default. If we enable IPV6 by default, the containers won't start properly in those systems without IPv6 support. Users who need IPv6 could enable it changing the value defaultConfigurationOverrides, setting the value extraOverrides or adding -- --listen_host=:: to the args value

bitnami/clickhouse/values.yaml Show resolved Hide resolved
@JulesdeCube JulesdeCube force-pushed the clickhouse-ipv6-support branch 2 times, most recently from bf8aa9c to b58c53a Compare January 8, 2025 14:09
@JulesdeCube
Copy link
Author

Hello,

I add listen_reuse_port and listen_try (ClickHouse/ClickHouse#21591 (comment)) to defaultConfigurationOverrides to fix 2 issues.

I also properly test the new solution on a ipv4 only server.

But still have some error on the CI:

Error: Task helm-package with ID e948fb94-776e-4a67-a636-1da00c492323 has failed. Error: Error executing action, program exited with exit code: 1, stderr: ERROR: Packaging chart: Couldn't package chart. Error: found in Chart.yaml, but missing in charts/ directory: zookeeper, common

@fmulero
Copy link
Collaborator

fmulero commented Jan 13, 2025

Enable listen_reuse_port is not recommended by mongodb. I think we should keep it simple and just allow users to enable easily IPv6 keeping IPv4 by default.

NOTE: The CI error was caused by a problem in our configuration. It is now working.

@JulesdeCube JulesdeCube force-pushed the clickhouse-ipv6-support branch 2 times, most recently from 75a2994 to afd982a Compare January 13, 2025 09:37
@JulesdeCube
Copy link
Author

JulesdeCube commented Jan 13, 2025

I remove listen_reuse_port (as it's not recommended).

But i think that native ipv4 and ipv6 support must be the default behavior. Moreover this work on only ipv4 only stack

@JulesdeCube JulesdeCube force-pushed the clickhouse-ipv6-support branch from 8ff1ed4 to 58a437c Compare January 13, 2025 09:56
Copy link
Collaborator

@fmulero fmulero left a comment

Choose a reason for hiding this comment

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

Then please bump the chart version and fix the conflicts. I'll keep an eye on this to merge it

bitnami/clickhouse/Chart.yaml Outdated Show resolved Hide resolved
@JulesdeCube JulesdeCube force-pushed the clickhouse-ipv6-support branch from b5f5824 to 758589b Compare January 15, 2025 09:32
fmulero
fmulero previously approved these changes Jan 15, 2025
@JulesdeCube
Copy link
Author

hello,

I was wondering what is blocking this pr to be merge ?

Jules Lefebvre added 2 commits January 17, 2025 19:27
Modify `defaultConfigurationOverrides` to listen on ipv4 and ipv6 and
replace the default launch args of the `setup.sh` to accept args

Signed-off-by: Jules Lefebvre <[email protected]>
Increase clickhouse version number from 7.1.5 to 7.2.0

Signed-off-by: Jules Lefebvre <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clickhouse in-progress verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants