-
Notifications
You must be signed in to change notification settings - Fork 25
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 ClickHouse server TLS option #348
Conversation
Codecov Report
@@ Coverage Diff @@
## main #348 +/- ##
=======================================
Coverage 71.43% 71.43%
=======================================
Files 40 40
Lines 5220 5220
=======================================
Hits 3729 3729
Misses 1322 1322
Partials 169 169
*This pull request uses carry forward flags. Click here to find out more. |
This PR is still in draft as we haven't adopt the secure connection support at the Client side. Here is a reference to test this ClickHouse TLS support before we add support on the Client side.
./hack/generate-clickhouse-cert.sh --SAN IP:127.0.0.1 --dhparam
kubectl port-forward svc/clickhouse-clickhouse -n flow-visibility 9440:9440
caCert, err := os.ReadFile("ca-cert.pem")
if err != nil {
klog.Fatal(err)
}
caCertPool := x509.NewCertPool()
successful := caCertPool.AppendCertsFromPEM(caCert)
if !successful {
klog.Fatal(err)
}
opt := clickhouse.Options{
Addr: []string{"127.0.0.1:9440"},
Auth: clickhouse.Auth{
Database: "default",
Username: "clickhouse_operator",
Password: "clickhouse_operator_password",
},
TLS: &tls.Config{
InsecureSkipVerify: false,
RootCAs: caCertPool,
},
}
connect := clickhouse.OpenDB(&opt) |
This PR is updated referring to: antrea-io/antrea#5171. It adapts the certs generated by a functionality in helm, and removes the scripts in hack to generate the certs. |
{{- if .Values.clickhouse.service.secureConnection.enable }} | ||
secure: "yes" | ||
settings: | ||
tcp_port: {{ .Values.clickhouse.service.tcpPort }} # keep for localhost |
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.
Do we want to expose tcp_port and http_port when secureConnection is disabled?
I only enclose https_port and tcp_port_secure inisde the double curly braket.
I'm not quite familiar with the settings, are they optional?
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.
Thanks for pointing out this! I've moved it out.
Let me explain about how it works.
The port here defines for the port in ClickHouse pod while the port exposed by the service defined in the Service template:
ports: | |
- name: http | |
port: {{ .Values.clickhouse.service.httpPort }} | |
- name: tcp | |
port: {{ .Values.clickhouse.service.tcpPort }} |
Although we may prefer to get consistency between these two, ClickHouse itself has some internal calls on the tcp port 9000 before the ClickHouse server is ready for the ClickHouse service, i.e. 9000 is hard-coded in ClickHouse code and I do not find a way to overwrite them easily. Thus finally my solution is to make the internal tcp port always 9000 while others will be consistent with the customization. And this will not affect the user experience.
c25f238
to
e9aa192
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.
LGTM
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 don't have strong opinion on refactoring the description of commonName, ipAddresses, dnsNames these three fields. But I suggest to specify ipAddresses, dnsNames are optional.
I notice we didn't include documentation update in this PR. @yuntanghsu will open a PR for adding documentation of supporting TLS connection between FA and CH. Do we plan to add it together in Yun-Tang's PR? |
For the multi-cluster document, as we haven't finished the story of multi-cluster, Elton is thinking to add that document later. |
I suppose we can add documentation on how to configure TLS connection between FA and CH? |
5e21d7c
to
d0debba
Compare
That makes sense. I added some doc for secure connection in the PR. Please take a look when you have time. Thanks! |
test/e2e/framework.go
Outdated
} | ||
pods, err := data.clientset.CoreV1().Pods(kubeNamespace).List(context.TODO(), listOptions) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to list Flow Aggregator Pod: %v", err) |
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.
return nil, fmt.Errorf("failed to list Flow Aggregator Pod: %v", err) | |
return nil, fmt.Errorf("failed to list ClickHouse Operatorr Pod: %v", err) |
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.
Thanks Yuntang! I finally removed the changes in e2e tests and added a discussion below.
test/e2e/framework.go
Outdated
@@ -1278,6 +1293,34 @@ func (data *TestData) deployFlowVisibilityCommon(chOperatorYML, flowVisibilityYM | |||
if err != nil || rc != 0 { | |||
return fmt.Errorf("error when deploying the ClickHouse Operator YML: %v\n is %s available on the control-plane Node?", err, chOperatorYML) | |||
} | |||
// Check for clickhouse operator pod running again for db connection establishment |
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 we can check the deployment directly?
https://github.com/antrea-io/antrea/blob/9c32416d85a919f0ab087e5b564afe808249295a/test/e2e/framework.go#L1084
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.
Thanks! That makes sense to me. But I found simply check the ClickHouse Operator is not enough and removed these parts. For more details, you can see my comments below
I noticed a strange behavior when running the ClickHouse migration e2e test in this PR. When we update the ClickHouse Operator, it will schedule a task to renew the ClickHouse pod, If we update the ClickHouse Operator followed by the new Flow Visibility manifest, usually it However, if we have some changes in the Here is the error may happen in the e2e test. The root cause is in the downgrading tests.
This should not break users usage as after several tries it will start to apply the second task As the changes in |
Signed-off-by: Yanjun Zhou <[email protected]>
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.
LGTM
Generally, we recommend using Ingress/LoadBalancer for secure connection to ClickHouse.
But we also provide this TLS option at ClickHouse server side.
Currently ClickHouse Operator does not support cert-manager, which means users need to
manually do the cert rotation when using this ClickHouse server side TLS option.
This PR also includes a script to generate all required cert files for ClickHouse.You could use it with the following command. It will generate the dh parameter, a server
cert, a server key and a CA cert in
build/charts/theia/provisioning/tls
.You can provision the CA cert at the client side.
This PR also offers an option to use the certs generated by a functionality in helm.