-
Notifications
You must be signed in to change notification settings - Fork 366
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
Support secure connections to ClickHouse DB #5171
Conversation
1ba2ceb
to
0f1e1e0
Compare
I notice you did not update the chart doc. You may also need to run |
0f1e1e0
to
17f4137
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
17f4137
to
8f09ccb
Compare
@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:
|
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. |
59557f9
to
e16b177
Compare
e16b177
to
fc9a503
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any thoughts on enabling TLS for e2e tests? cc @heanlan @yanjunz97
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed it to /etc/flow-aggregator/certs
. Thanks.
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:
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:
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? |
Hi, |
c24a395
to
17a7e77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that all recent changes are for e2e tests, and you haven't changed anything in the FA code?
<server> | ||
<certificateFile>/opt/certs/tls.crt</certificateFile> | ||
<privateKeyFile>/opt/certs/tls.key</privateKeyFile> | ||
<dhParamsFile>/etc/clickhouse-server/config.d/dhparam.pem</dhParamsFile> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
17a7e77
to
161c062
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple more small comments
@heanlan do you want to take another look?
test/e2e/charts/flow-visibility/templates/clickhouseinstallation.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, only some nits
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add some comments explaining we need to copy the secret to flow-aggregator namespace to make it accessible by FA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment there. Please take a look to see if it makes sense? Thanks.
1f3e589
to
7a1dc5b
Compare
docs/network-flow-visibility.md
Outdated
#### 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
0b167ef
to
649a84c
Compare
649a84c
to
58a7139
Compare
@antoninbas |
@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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more small comment. Please address and rebase the PR, and we can merge this.
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]>
ba436af
to
8652a8b
Compare
/test-all |
/test-conformance |
In this PR, we are adding following changes:
resolve #4902
Signed-off-by: Yun-Tang Hsu [email protected]