From 8f09ccb77d19daf55768085b2ae814fa3ace8d51 Mon Sep 17 00:00:00 2001 From: Yun-Tang Hsu Date: Mon, 26 Jun 2023 10:07:09 -0700 Subject: [PATCH] address comments Signed-off-by: Yun-Tang Hsu --- build/charts/flow-aggregator/README.md | 6 ++++- .../flow-aggregator/conf/flow-aggregator.conf | 2 +- .../flow-aggregator/templates/deployment.yaml | 4 ++++ build/charts/flow-aggregator/values.yaml | 2 +- build/yamls/flow-aggregator.yml | 9 +------- docs/network-flow-visibility.md | 23 +++++++++++-------- pkg/config/flowaggregator/config.go | 2 +- .../clickhouseclient/clickhouseclient.go | 13 ++++++----- 8 files changed, 33 insertions(+), 28 deletions(-) diff --git a/build/charts/flow-aggregator/README.md b/build/charts/flow-aggregator/README.md index d954012977e..5908c623012 100644 --- a/build/charts/flow-aggregator/README.md +++ b/build/charts/flow-aggregator/README.md @@ -26,9 +26,13 @@ Kubernetes: `>= 1.16.0-0` | clickHouse.commitInterval | string | `"8s"` | CommitInterval is the periodical interval between batch commit of flow records to DB. Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h". | | clickHouse.compress | bool | `true` | Compress enables lz4 compression when committing flow records. | | clickHouse.connectionSecret | object | `{"password":"clickhouse_operator_password","username":"clickhouse_operator"}` | Credentials to connect to ClickHouse. They will be stored in a Secret. | -| clickHouse.databaseURL | string | `"tcp://clickhouse-clickhouse.flow-visibility.svc:9000"` | DatabaseURL is the url to the database. TCP protocol is required. | +| clickHouse.databaseURL | string | `"tcp://clickhouse-clickhouse.flow-visibility.svc:9000"` | DatabaseURL is the url to the database. TCP/HTTP/HTTPS protocols are supported. | | clickHouse.debug | bool | `false` | Debug enables debug logs from ClickHouse sql driver. | | clickHouse.enable | bool | `false` | Determine whether to enable exporting flow records to ClickHouse. | +| clickHouse.tls | object | `{"customCACert":false,"enable":false,"insecureSkipVerify":false}` | tls contains tls related configuration options, which is used to connect to clickhouse service. | +| clickHouse.tls.customCACert | bool | `false` | Indicates whether to use custom CA certificate. Default root CAs will be used if this field is false. If true, a Secret named "clickhouse-ca" must be provided with the following keys: ca.crt: | +| clickHouse.tls.enable | bool | `false` | Determine whether to enable tls. | +| clickHouse.tls.insecureSkipVerify | bool | `false` | Determine whether to skip the verification of the server's certificate chain and host name. Default is false. | | flowAggregatorAddress | string | `""` | Provide an extra DNS name or IP address of flow aggregator for generating TLS certificate. | | flowCollector.address | string | `""` | Provide the flow collector address as string with format :[:], where proto is tcp or udp. If no L4 transport proto is given, we consider tcp as default. | | flowCollector.enable | bool | `false` | Determine whether to enable exporting flow records to external flow collector. | diff --git a/build/charts/flow-aggregator/conf/flow-aggregator.conf b/build/charts/flow-aggregator/conf/flow-aggregator.conf index ada47f68dd7..b69370d23c4 100644 --- a/build/charts/flow-aggregator/conf/flow-aggregator.conf +++ b/build/charts/flow-aggregator/conf/flow-aggregator.conf @@ -80,7 +80,7 @@ clickHouse: # Enable is the switch to enable TLS when connecting to ClickHouse server. enable: {{ .Values.clickHouse.tls.enable }} - # InsecureSkipVerify determine whether a client verifies the server's certificate chain and host name. + # InsecureSkipVerify determine whether to skip the verification of the server's certificate chain and host name. # Default is false. insecureSkipVerify: {{ .Values.clickHouse.tls.insecureSkipVerify }} diff --git a/build/charts/flow-aggregator/templates/deployment.yaml b/build/charts/flow-aggregator/templates/deployment.yaml index db755d9bccb..e52507c257d 100644 --- a/build/charts/flow-aggregator/templates/deployment.yaml +++ b/build/charts/flow-aggregator/templates/deployment.yaml @@ -79,8 +79,10 @@ spec: readOnly: true - mountPath: /var/log/antrea/flow-aggregator name: host-var-log-antrea-flow-aggregator + {{- if and .Values.clickHouse.tls.enable .Values.clickHouse.tls.customCACert }} - name: clickhouse-ca mountPath: /etc/ssl/certs + {{- end }} nodeSelector: kubernetes.io/os: linux kubernetes.io/arch: amd64 @@ -93,9 +95,11 @@ spec: hostPath: path: /var/log/antrea/flow-aggregator type: DirectoryOrCreate + {{- if and .Values.clickHouse.tls.enable .Values.clickHouse.tls.customCACert }} # Make it optional as we only read it when customCACert=true. - name: clickhouse-ca secret: secretName: clickhouse-ca defaultMode: 0400 optional: true + {{- end }} diff --git a/build/charts/flow-aggregator/values.yaml b/build/charts/flow-aggregator/values.yaml index 0fa5b9c069a..5d19e23eec2 100644 --- a/build/charts/flow-aggregator/values.yaml +++ b/build/charts/flow-aggregator/values.yaml @@ -51,7 +51,7 @@ clickHouse: tls: # -- Determine whether to enable tls. enable: false - # -- Determine whether a client verifies the server's certificate chain and host name. Default is false. + # -- Determine whether to skip the verification of the server's certificate chain and host name. Default is false. insecureSkipVerify: false # -- Indicates whether to use custom CA certificate. Default root CAs will be used if this field is false. # If true, a Secret named "clickhouse-ca" must be provided with the following keys: diff --git a/build/yamls/flow-aggregator.yml b/build/yamls/flow-aggregator.yml index 6dd6871f1c9..b2c9ea6eca0 100644 --- a/build/yamls/flow-aggregator.yml +++ b/build/yamls/flow-aggregator.yml @@ -232,7 +232,7 @@ data: # Enable is the switch to enable TLS when connecting to ClickHouse server. enable: false - # InsecureSkipVerify determine whether a client verifies the server's certificate chain and host name. + # InsecureSkipVerify determine whether to skip the verification of the server's certificate chain and host name. # Default is false. insecureSkipVerify: false @@ -448,8 +448,6 @@ spec: readOnly: true - mountPath: /var/log/antrea/flow-aggregator name: host-var-log-antrea-flow-aggregator - - mountPath: /etc/ssl/certs - name: clickhouse-ca nodeSelector: kubernetes.io/arch: amd64 kubernetes.io/os: linux @@ -462,8 +460,3 @@ spec: path: /var/log/antrea/flow-aggregator type: DirectoryOrCreate name: host-var-log-antrea-flow-aggregator - - name: clickhouse-ca - secret: - defaultMode: 256 - optional: true - secretName: clickhouse-ca diff --git a/docs/network-flow-visibility.md b/docs/network-flow-visibility.md index f6cb530d1b1..4e8d594a590 100644 --- a/docs/network-flow-visibility.md +++ b/docs/network-flow-visibility.md @@ -291,13 +291,15 @@ the URL for `clickHouse.databaseURL` in the following format: #### Support secure connections to ClickHouse database -Since Antrea v1.13, you can enable TLS when connecting to ClickHouse Server by setting `clickHouse.tls.enable` to `true`. -You can also change the value of `clickHouse.tls.insecureSkipVerify` to determine whether a client verifies the -server's certificate. -If you want to provide a custom CA certificate, you can set `clickHouse.tls.customizedCACert` to `true` and -the flow-aggregator will read the certificate key pair from the `clickhouse-ca` Secret. +Since Antrea v1.13, you can enable TLS when connecting to ClickHouse Server by +setting `clickHouse.tls.enable` to `true`. You can also change the value of +`clickHouse.tls.insecureSkipVerify` to determine whether a client verifies the +server's certificate. If you want to provide a custom CA certificate, you can +set `clickHouse.tls.customizedCACert` to `true` and the flow-aggregator will +read the certificate key pair from the `clickhouse-ca` Secret. -Make sure to follow the following form when creating the `clickhouse-ca` Secret with the custom CA certificate: +Make sure to follow the following form when creating the `clickhouse-ca` Secret +with the custom CA certificate: ```yaml apiVersion: v1 @@ -311,7 +313,8 @@ data: #### Using kubectl -You can use `kubectl apply -f ` to create the above secret, or use `kubectl create secret`: +You can use `kubectl apply -f ` to create the above secret +, or use `kubectl create secret`: ```bash kubectl create secret generic clickhouse-ca -n flow-aggregator --from-file=ca.crt= @@ -319,8 +322,8 @@ kubectl create secret generic clickhouse-ca -n flow-aggregator --from-file=ca.cr #### 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. +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. #### Example of flow-aggregator.conf @@ -404,7 +407,7 @@ flow-aggregator.conf: | # Enable is the switch to enable TLS when connecting to ClickHouse server. enable: false - # InsecureSkipVerify determine whether a client verifies the server's certificate chain and host name. + # InsecureSkipVerify determine whether to skip the verification of the server's certificate chain and host name. # Default is false. insecureSkipVerify: false diff --git a/pkg/config/flowaggregator/config.go b/pkg/config/flowaggregator/config.go index ecffd487453..6bbf36f0d13 100644 --- a/pkg/config/flowaggregator/config.go +++ b/pkg/config/flowaggregator/config.go @@ -113,7 +113,7 @@ type ClickHouseConfig struct { type TLSConfig struct { // Enable is the switch to enable TLS when connecting to ClickHouse server. Enable bool `yaml:"enable,omitempty"` - // InsecureSkipVerify determine whether a client verifies the server's certificate chain and host name. + // InsecureSkipVerify determine whether to skip the verification of the server's certificate chain and host name. // Default is false. InsecureSkipVerify bool `yaml:"insecureSkipVerify,omitempty"` // CustomCACert determine whether to use custom CA certificate. Default root CAs will be used if false. diff --git a/pkg/flowaggregator/clickhouseclient/clickhouseclient.go b/pkg/flowaggregator/clickhouseclient/clickhouseclient.go index 043a521d201..791552e7c92 100644 --- a/pkg/flowaggregator/clickhouseclient/clickhouseclient.go +++ b/pkg/flowaggregator/clickhouseclient/clickhouseclient.go @@ -34,6 +34,7 @@ import ( ) const ( + ProtocolUnknown = -1 maxQueueSize = 1 << 19 // 524288. ~500MB assuming 1KB per record queueFlushTimeout = 10 * time.Second insertQuery = `INSERT INTO flows ( @@ -446,7 +447,7 @@ func ConnectClickHouse(config *ClickHouseConfig) (*sql.DB, error) { caCertPool := x509.NewCertPool() successful := caCertPool.AppendCertsFromPEM(config.CACert) if !successful { - connErr = fmt.Errorf("failed to add custom CA certification") + connErr = fmt.Errorf("failed to add the custom CA certification") return false, nil } opt.TLS.RootCAs = caCertPool @@ -472,25 +473,25 @@ func ConnectClickHouse(config *ClickHouseConfig) (*sql.DB, error) { func parseDatabaseURL(dbUrl string, secure bool) (clickhouse.Protocol, string, error) { u, err := url.Parse(dbUrl) if err != nil { - return -1, "", fmt.Errorf("failed to parse ClickHouse database URL: %w", err) + return ProtocolUnknown, "", fmt.Errorf("failed to parse ClickHouse database URL: %w", err) } if u.Path != "" || u.RawQuery != "" || u.User != nil { - return -1, "", fmt.Errorf("invalid ClickHouse database URL '%s': path, query string or user info should not be set", dbUrl) + return ProtocolUnknown, "", fmt.Errorf("invalid ClickHouse database URL '%s': path, query string or user info should not be set", dbUrl) } switch u.Scheme { case "http": if secure { - return -1, u.Host, fmt.Errorf("invalid ClickHouse setting: http with TLS enabled") + return ProtocolUnknown, u.Host, fmt.Errorf("invalid ClickHouse setting: http with TLS enabled") } return clickhouse.HTTP, u.Host, nil case "https": if !secure { - return -1, u.Host, fmt.Errorf("invalid ClickHouse setting: https without TLS enabled") + return ProtocolUnknown, u.Host, fmt.Errorf("invalid ClickHouse setting: https without TLS enabled") } return clickhouse.HTTP, u.Host, nil case "tcp": return clickhouse.Native, u.Host, nil default: - return -1, "", fmt.Errorf("connection over %s transport protocol is not supported", u.Scheme) + return ProtocolUnknown, "", fmt.Errorf("connection over %s transport protocol is not supported", u.Scheme) } }