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 support for setting secure communication between clickhouse instances #938

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

chancez
Copy link
Contributor

@chancez chancez commented May 19, 2022

  • All commits in the PR are squashed. More info
  • The PR is made into dedicated next-release branch, not into master branch1. More info
  • The PR is signed. More info

Fixes #668

Verified this works when configuring clickhouse to use TLS and with no unencrypted ports.

A snippet of my CHI:

  templates:
    hostTemplates:
    - name: host-template
      spec:
        {{- if .Values.clickhouse.tls.enabled }}
        tcpPort: 9440
        secure: true
        httpPort: 8443
        interserverHTTPPort: 9010
        {{- else }}
        tcpPort: 9000
        httpPort: 8123
        interserverHTTPPort: 9009
        {{- end }}

@chancez chancez force-pushed the secure_shard_communications branch from dda6dc7 to c18ca91 Compare May 20, 2022 00:05
Copy link
Member

@alex-zaitsev alex-zaitsev left a comment

Choose a reason for hiding this comment

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

It would be ideal to add a test for that, but even an example manifest that shows the use of this feature would help.

Maybe some extra automation may be useful as well, e.g. automatically change default ports to secure when secure flag is used.

@chancez
Copy link
Contributor Author

chancez commented May 20, 2022

It would be ideal to add a test for that, but even an example manifest that shows the use of this feature would help.

Sure, I can provide an example, and see if I can write a test. I didn't see much regarding unit tests so I wasn't sure where the proper place to test this would be.

Maybe some extra automation may be useful as well, e.g. automatically change default ports to secure when secure flag is used.

That would be nice, but seems out of scope for this PR, and seems more like a separate feature potentially. I'm currently satisfied with just being able to control the configuration directly, and have less magic. Reason for this is that similar functionality for setting the ports in the (chop-generated-ports.xml), and that currently conflicts a bit with trying to disable the non _secure ports (though currently disabling these ports still works, thankfully).

@chancez
Copy link
Contributor Author

chancez commented May 20, 2022

Here's an example based on what we're using to deploy clickhouse:

https://gist.github.com/chancez/3da4b1df4f9942d2a260360a6a762912

@chancez
Copy link
Contributor Author

chancez commented May 20, 2022

For testing: It shouldn't be terribly difficult to test using TLS, but it requires a bit of effort. I'd need some guidance on how you would expect certificates to be generated for testing.

In our project, we deploy clickhouse with TLS by using cert-manager in the cluster to issue certs. Alternatively, we could use openssl, though I'm not familiar with the python crypto APIs at all, so I'd prefer to just exec openssl to generate certificates for tests.

Once certs are generated, you just need to create a secret for them and then deploy using the example CHI resource I provided.

@chancez
Copy link
Contributor Author

chancez commented May 20, 2022

Currently working on the tests. I have a minimal working example that I'm porting to tests atm.

@chancez chancez force-pushed the secure_shard_communications branch from c18ca91 to d983183 Compare May 20, 2022 19:37
@chancez
Copy link
Contributor Author

chancez commented May 20, 2022

I "tried" to write some tests, but I'm having a hard time getting the test image to build via ./tests/image/build_docker.sh, it just seems to fail in different places each time, sometimes pulling from quay, but most recently on:

Package docker-ce is not available, but is referred to by another package.
This may mean that the package is missing, has been obsoleted, or
is only available from another source
However the following packages replace it:
  docker-ce-cli:amd64

Also, it doesn't seem like the test image is multi-arch, so I might have issues regardless, as I'm on an m1 Mac.

I've pushed my attempt at some tests, but unfortunately it doesn't look like CI runs tests automatically, so I can't iterate that way either.

@chancez chancez force-pushed the secure_shard_communications branch from d983183 to bd50e6a Compare May 20, 2022 19:41
@chancez
Copy link
Contributor Author

chancez commented May 20, 2022

Ah I just realized the reason the test image doesn't build is because it's using amd64 repos (and binaries) but I'm pulling the arm64 image. I guess I just have to update it to work for both.

@Slach
Copy link
Collaborator

Slach commented May 21, 2022

You can develop test without building image on arm64 platform

just setup minikube, python3, python3-pip, 'python3-venv`
and run

minikube start
python3 -m venv ~/operator-venv/
~/operator-venv/bin/pip3 install -U -r ./tests/image/requirements.txt
~/operator-venv/bin/python3 ./tests/regression.py --only="/regression/e2e.test_operator/yout_test*" --native

--native mode mean don't use docker-compose and use installed default minikube KUBECONFIG

@chancez

This comment was marked as off-topic.

@chancez

This comment was marked as off-topic.

@chancez

This comment was marked as off-topic.

@chancez

This comment was marked as off-topic.

@chancez

This comment was marked as off-topic.

@chancez

This comment was marked as off-topic.

@chancez chancez force-pushed the secure_shard_communications branch from bd50e6a to 4adf382 Compare May 23, 2022 22:50
@chancez
Copy link
Contributor Author

chancez commented May 23, 2022

Alrighty, I got a local test setup working (built a custom docker image to run tests with) and got tests working. Please take a look.

@chancez chancez force-pushed the secure_shard_communications branch from 4adf382 to 357db18 Compare May 23, 2022 22:58
@Slach
Copy link
Collaborator

Slach commented May 24, 2022

ok. look like your default shell is zsh
usually we run tests under bash in --native mode
@vzakaznikov JSFYI, as testflows author

@chancez
Copy link
Contributor Author

chancez commented May 25, 2022

@Slach Yeah, it's the default on Mac, it seemed to be correctly opening bash, but I got it all sorted out. Running it in docker worked well.

Let me know if there's anything else this PR needs. I'm really going to need this so we can roll out Clickhouse with TLS in our environments.

@chancez
Copy link
Contributor Author

chancez commented May 25, 2022

@Slach I want to be able to configure the cluster secret or internode user user/pass as a way to authenticate inter-node communication, eg: https://github.com/ClickHouse/ClickHouse/blob/b29e877f269e84ae452c446e70b406a695863470/tests/integration/test_distributed_inter_server_secret/configs/remote_servers_n1.xml#L4

Should that be a separate PR?

@chancez chancez force-pushed the secure_shard_communications branch 3 times, most recently from 22fa1bc to 02f6a86 Compare May 27, 2022 18:40
@chancez chancez requested a review from alex-zaitsev May 27, 2022 18:40
@chancez chancez mentioned this pull request May 27, 2022
3 tasks
@sunsingerus sunsingerus changed the base branch from 0.19.0 to 0.20.0 July 15, 2022 09:03
…nces

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez chancez force-pushed the secure_shard_communications branch from 02f6a86 to 95db8e1 Compare August 2, 2022 17:52
@sunsingerus sunsingerus merged commit 6e84f7e into Altinity:0.20.0 Sep 1, 2022
@chancez chancez deleted the secure_shard_communications branch November 16, 2022 00:39
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.

4 participants