Support secure connections to ClickHouse DB #5171

yuntanghsu · 2023-06-23T00:15:05Z

In this PR, we are adding following changes:

Support TLS when connecting the FlowAggregator to ClickHouse.
Support HTTP/HTTPS. Users can directly modify the clickHouse.databaseURL in order to switch protocol.
Support custom CA certificate. User need to create a clickhouse-ca Secret in flow-aggregator namespace. The custom CA certificate will be used when clickhouse.tls.caCert is true.
Update network-flow-visibility.md
Add e2e test for http/https/tls protocol when connecting the FlowAggregator to ClickHouse.

resolve #4902

Signed-off-by: Yun-Tang Hsu [email protected]

build/charts/flow-aggregator/conf/flow-aggregator.conf

build/charts/flow-aggregator/templates/deployment.yaml

docs/network-flow-visibility.md

pkg/flowaggregator/clickhouseclient/clickhouseclient.go

yanjunz97 · 2023-06-23T22:31:43Z

I notice you did not update the chart doc. You may also need to run
make under build/charts.

heanlan

In the PR description, please consider use the following keywords to link to the issue https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request[…]n-issue-using-a-keyword

build/charts/flow-aggregator/templates/deployment.yaml

pkg/flowaggregator/clickhouseclient/clickhouseclient.go

build/charts/flow-aggregator/conf/flow-aggregator.conf

heanlan · 2023-06-28T17:24:56Z

@yuntanghsu I was trying to reply one of the comment reply, but I cannot find it anymore. Did you accidentally delete that comment? I remember I was suggesting we can add "tls" here. To reply your comment, IMO tls is a secure protocol building on top of tcp. So if user specify tcp, means tcp + tls disabled. If they specify tls, means tcp protocol + tls enabled.

I believe it is fine to put tcp in parallel with tls. And we have something similar in FA:

antrea/build/charts/flow-aggregator/conf/flow-aggregator.conf

Line 16 in a84eb0e

    
           # Provide the transport protocol for the flow aggregator collecting process, which is tls, tcp or udp.

yuntanghsu · 2023-06-28T17:30:01Z

@yuntanghsu I was trying to reply one of the comment reply, but I cannot find it anymore. Did you accidentally delete that comment? I remember I was suggesting we should can add "tls" here. To reply your comment, IMO tls is a secure protocol

I deleted it. Cuz I think you are right. This will be more straightforward to just let user to specify tls/tcp/http/https instead of giving them to options and letting them to make the combinations themselves.

build/charts/flow-aggregator/conf/flow-aggregator.conf

docs/network-flow-visibility.md

pkg/flowaggregator/exporter/clickhouse.go

build/charts/flow-aggregator/values.yaml

pkg/flowaggregator/clickhouseclient/clickhouseclient.go

antoninbas

any thoughts on enabling TLS for e2e tests? cc @heanlan @yanjunz97

build/charts/flow-aggregator/README.md

docs/network-flow-visibility.md

pkg/flowaggregator/clickhouseclient/clickhouseclient.go

pkg/flowaggregator/clickhouseclient/clickhouseclient_test.go

antoninbas · 2023-07-03T21:19:23Z

build/charts/flow-aggregator/templates/deployment.yaml

@@ -79,6 +79,10 @@ spec:
          readOnly: true
        - mountPath: /var/log/antrea/flow-aggregator
          name: host-var-log-antrea-flow-aggregator
+        {{- if and .Values.clickHouse.tls.customCACert .Values.clickHouse.tls.customCACert }}
+        - name: clickhouse-ca
+          mountPath: /etc/ssl/certs


is using this directory really necessary in our case? We are explicitly providing the CA certificate when we create the ClickHouse client connection, so I think using this standard unix directory can only create confusion. I'd recommend just mounting the certificate to /etc/flow-aggregator.

We already use /etc/flow-aggregator for flow-aggregator conifg. I think we are not able to mount two volumes to the same path?
I changed the path to /etc/flow-aggregator/ssl/certs

You could use subPath and have both files in /etc/flow-aggregator.
https://stackoverflow.com/a/68179095/4538702

However, I am fine with using a subdirectory if it works. Maybe rename /etc/flow-aggregator/ssl/certs to /etc/flow-aggregator/certs

I've renamed it to /etc/flow-aggregator/certs. Thanks.

yanjunz97 · 2023-07-04T00:05:57Z

Hi @antoninbas , I have a offline discussion with @heanlan and @yuntanghsu . And here’re our thoughts:

There are basically two ways to do the e2e test.

One way is to use the TLS connection supported by ClickHouse server. As ClickHouse operator does not support cert-manager, we will need to manage our own certs and key. We remember you’ve mentioned the default root CAs, but we did not think of a way to use the default root CAs to sign our ClickHouse server cert. Please correct me if I’m wrong. Thus I believe there are 2 ways to deal with the certs:

We can pre-generate the certs and save it under buld/yamls/flow-aggregator for test purpose. In this case, we need to rotate the certs in the future.
We can generate the certs during setup step in the e2e test, inject them into the flow-visibility-e2e.yaml and create the Secret for CA cert.

Another way is to use the ingress and cert-manager. We haven’t verified it yet, but I think we need to follow the steps below for this strategy:

Enable the LoadBalancer in Antrea. Not sure if Service external IP management by Antrea works in kind as I failed to do this before. But Anlan has verified that MetalLB will work
Install the ingress controller
Install the cert-manager and create the CertificateIssuer
Create a ingress resource for ClickHouse service

I think after these steps, we will be able to test the secure connection between FA client and CH server. But this might introduce quite a lot of resources to our test environment and we are not sure if it is acceptable.

Do you have any suggestion on which way we should select?

yuntanghsu · 2023-07-15T05:25:57Z

Hi,
I update the e2e test by changing the endpoint from the ingress to ClickHouse server. Currently, we test tcp/http when we disable secure option in ClickHouse server and test tls/https when we enable secure option in ClickHouse server.

antoninbas

I assume that all recent changes are for e2e tests, and you haven't changed anything in the FA code?

antoninbas · 2023-07-18T23:02:11Z

test/e2e/charts/flow-visibility/provisioning/tls/settings.xml

+        <server>
+            <certificateFile>/opt/certs/tls.crt</certificateFile>
+            <privateKeyFile>/opt/certs/tls.key</privateKeyFile>
+            <dhParamsFile>/etc/clickhouse-server/config.d/dhparam.pem</dhParamsFile>


Given that providing dhParamsFile is optional, I think we should omit it for testing to keep things simpler

I actually don't even see dhParamsFile mentioned in https://clickhouse.com/docs/en/guides/sre/configuring-ssl. Modern ciphers should not require this. In any case, we should not really be worried about security hardening when it comes to e2e testing.

I assume that all recent changes are for e2e tests, and you haven't changed anything in the FA code?

Yes, that's right.

Removed dhParamsFile. Thanks.

test/e2e/framework.go

test/e2e/flowaggregator_test.go

test/e2e/framework.go

antoninbas

a couple more small comments

@heanlan do you want to take another look?

test/e2e/charts/flow-visibility/templates/clickhouseinstallation.yaml

test/e2e/charts/flow-visibility/templates/namespace.yaml

test/e2e/flowaggregator_test.go

test/e2e/framework.go

yanjunz97

Overall LGTM, only some nits

docs/network-flow-visibility.md

yanjunz97 · 2023-07-20T01:41:27Z

test/e2e/framework.go

@@ -877,7 +892,21 @@ func (data *TestData) deployFlowAggregator(ipfixCollector string) error {
 	if err != nil || rc != 0 {
 		return fmt.Errorf("error when deploying the Flow Aggregator; %s not available on the control-plane Node", flowAggYaml)
 	}
-	if err = data.mutateFlowAggregatorConfigMap(ipfixCollector); err != nil {
+	if o.secureConnection {


Maybe add some comments explaining we need to copy the secret to flow-aggregator namespace to make it accessible by FA?

I added a comment there. Please take a look to see if it makes sense? Thanks.

docs/network-flow-visibility.md

antoninbas · 2023-07-20T17:44:11Z

docs/network-flow-visibility.md

+#### Configuration for ClickHouse pre Antrea v1.13
+
+We don't support secure connection to ClickHouse database prior to Antrea v1.13.
+We only support TCP when connecting to ClickHouse server from Flow Aggregator.


I am still not sure I understand the value of this section, or why it is its own section

I feel like it should just be a quick note at the end of the previous section:

Prior to Antrea v1.13, secure connections to ClickHouse are not supported, and TCP is the only supported protocol when connecting to the ClickHouse server from the Flow Aggregator.

The reason I added this section is because I saw we usually add extra section when we have configuration changes in other document, like node-port-local.
I've updated it. Thanks.

test/e2e/charts/flow-visibility/values.yaml

test/e2e/framework.go

yuntanghsu · 2023-07-20T23:27:48Z

@antoninbas
I found the e2e test keeps failing because of the error: no space left. It happens in other PRs as well.
I also found the available memory is 2G less than what we have yesterday. Not sure if it is github's issue?
Do you think I can add a workaround to free more space before running the flow-visibility e2e test?

antoninbas · 2023-07-20T23:49:57Z

@yuntanghsu I think your workaround is ok. Github is probably installing additional / new software since yesterday, hence the reduction in available disk space. Later on, you could try to find ways to reduce the space used by the e2e visibility test (e.g., delete images from the host after they have been loaded into the Kind Nodes, or use 2 Kind Nodes instead of 3).

antoninbas

one more small comment. Please address and rebase the PR, and we can merge this.

test/e2e/framework.go

Adding following changes: 1. Support TLS when connecting the Flow Aggregator to ClickHouse. 2. Support HTTP/HTTPS. Users can directly modify the clickHouse.databaseURL in order to switch protocol. 3. Support custom CA certificate. User need to create a clickhouse-ca Secret in flow-aggregator namespace. The custom CA certificate will be used when clickhouse.tls.caCert is true. 4. Update network-flow-visibility.md 5. Add e2e test for http/https/tls protocol when connecting the Flow Aggregator to ClickHouse Signed-off-by: Yun-Tang Hsu <[email protected]>

antoninbas · 2023-07-21T17:56:11Z

/test-all

antoninbas · 2023-07-21T20:40:42Z

/test-conformance

yuntanghsu force-pushed the clickhouse_tls branch 7 times, most recently from 1ba2ceb to 0f1e1e0 Compare June 23, 2023 19:50

yuntanghsu requested review from heanlan and yanjunz97 June 23, 2023 19:53

yanjunz97 reviewed Jun 23, 2023

View reviewed changes

yuntanghsu force-pushed the clickhouse_tls branch from 0f1e1e0 to 17f4137 Compare June 26, 2023 17:14

heanlan reviewed Jun 27, 2023

View reviewed changes

yuntanghsu force-pushed the clickhouse_tls branch from 17f4137 to 8f09ccb Compare June 28, 2023 16:47

yuntanghsu force-pushed the clickhouse_tls branch 6 times, most recently from 59557f9 to e16b177 Compare June 28, 2023 18:01

yuntanghsu requested review from yanjunz97 and heanlan July 3, 2023 16:31

yanjunz97 reviewed Jul 3, 2023

View reviewed changes

heanlan reviewed Jul 3, 2023

View reviewed changes

build/charts/flow-aggregator/values.yaml Show resolved Hide resolved

pkg/flowaggregator/clickhouseclient/clickhouseclient.go Outdated Show resolved Hide resolved

yuntanghsu force-pushed the clickhouse_tls branch from e16b177 to fc9a503 Compare July 3, 2023 19:57

yuntanghsu requested a review from antoninbas July 3, 2023 20:03

antoninbas reviewed Jul 3, 2023

View reviewed changes

yanjunz97 mentioned this pull request Jul 14, 2023

Add ClickHouse server TLS option antrea-io/theia#348

Merged

yuntanghsu force-pushed the clickhouse_tls branch 3 times, most recently from c24a395 to 17a7e77 Compare July 18, 2023 17:25

antoninbas reviewed Jul 18, 2023

View reviewed changes

yuntanghsu force-pushed the clickhouse_tls branch from 17a7e77 to 161c062 Compare July 19, 2023 22:43

antoninbas reviewed Jul 19, 2023

View reviewed changes

yanjunz97 previously approved these changes Jul 20, 2023

View reviewed changes

yuntanghsu dismissed yanjunz97’s stale review via 7a1dc5b July 20, 2023 17:29

yuntanghsu force-pushed the clickhouse_tls branch from 1f3e589 to 7a1dc5b Compare July 20, 2023 17:29

antoninbas reviewed Jul 20, 2023

View reviewed changes

yuntanghsu force-pushed the clickhouse_tls branch 3 times, most recently from 0b167ef to 649a84c Compare July 20, 2023 20:12

antoninbas reviewed Jul 20, 2023

View reviewed changes

test/e2e/framework.go Outdated Show resolved Hide resolved

test/e2e/framework.go Show resolved Hide resolved

yuntanghsu force-pushed the clickhouse_tls branch from 649a84c to 58a7139 Compare July 20, 2023 22:29

elton-furtado added this to the Antrea v1.13 release milestone Jul 21, 2023

antoninbas reviewed Jul 21, 2023

View reviewed changes

test/e2e/framework.go Outdated Show resolved Hide resolved

yuntanghsu force-pushed the clickhouse_tls branch from ba436af to 8652a8b Compare July 21, 2023 16:18

antoninbas approved these changes Jul 21, 2023

View reviewed changes

antoninbas merged commit 7919ef4 into antrea-io:main Jul 22, 2023
39 of 43 checks passed

antoninbas added action/release-note Indicates a PR that should be included in release notes. area/flow-visibility Issues or PRs related to flow visibility support in Antrea labels Jul 22, 2023

yuntanghsu mentioned this pull request Oct 4, 2023

Update hack/generate-manifest.sh --mode antrea-e2e antrea-io/theia#321

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support secure connections to ClickHouse DB #5171

Support secure connections to ClickHouse DB #5171

yuntanghsu commented Jun 23, 2023 •

edited

Loading

yanjunz97 commented Jun 23, 2023

heanlan left a comment

heanlan commented Jun 28, 2023 •

edited

Loading

yuntanghsu commented Jun 28, 2023 •

edited

Loading

antoninbas left a comment

antoninbas Jul 3, 2023

yuntanghsu Jul 10, 2023

yuntanghsu Jul 10, 2023

antoninbas Jul 10, 2023

yuntanghsu Jul 10, 2023

yanjunz97 commented Jul 4, 2023

yuntanghsu commented Jul 15, 2023

antoninbas left a comment

antoninbas Jul 18, 2023

yuntanghsu Jul 19, 2023 •

edited

Loading

antoninbas left a comment

yanjunz97 left a comment

yanjunz97 Jul 20, 2023

yuntanghsu Jul 20, 2023

antoninbas Jul 20, 2023

yuntanghsu Jul 20, 2023

yuntanghsu commented Jul 20, 2023 •

edited

Loading

antoninbas commented Jul 20, 2023

antoninbas left a comment

antoninbas commented Jul 21, 2023

antoninbas commented Jul 21, 2023

Support secure connections to ClickHouse DB #5171

Support secure connections to ClickHouse DB #5171

Conversation

yuntanghsu commented Jun 23, 2023 • edited Loading

yanjunz97 commented Jun 23, 2023

heanlan left a comment

Choose a reason for hiding this comment

heanlan commented Jun 28, 2023 • edited Loading

yuntanghsu commented Jun 28, 2023 • edited Loading

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanjunz97 commented Jul 4, 2023

yuntanghsu commented Jul 15, 2023

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuntanghsu Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

yanjunz97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuntanghsu commented Jul 20, 2023 • edited Loading

antoninbas commented Jul 20, 2023

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Jul 21, 2023

antoninbas commented Jul 21, 2023

yuntanghsu commented Jun 23, 2023 •

edited

Loading

heanlan commented Jun 28, 2023 •

edited

Loading

yuntanghsu commented Jun 28, 2023 •

edited

Loading

yuntanghsu Jul 19, 2023 •

edited

Loading

yuntanghsu commented Jul 20, 2023 •

edited

Loading