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

[scalardb-cluster] Support cert-manager in ScalarDB Cluster chart #263

Merged
merged 3 commits into from
May 31, 2024

Conversation

kota2and3kan
Copy link
Collaborator

@kota2and3kan kota2and3kan commented Apr 26, 2024

Description

This PR updates ScalarDB Cluster chart to support cert-manager.

Please take a look!

Related issues and/or PRs

Changes made

Mainly, this PR adds/updates the following features (manifests):

  1. Deploy a Certificate resource for cert-manager by using charts/scalardb-cluster/templates/scalardb-cluster/certmanager.yaml.

  2. Deploy self-signed CA and create certificate for ScalarDB Cluster if you don't specify any Issuer resources in issuerRef configuration.

    • If you deploy Scalar Envoy as a sub chart of this ScalarDB Cluster chart without issuerRef configuration, the Envoy chart will use self-signed CA that is deployed by this chart to create certificate for Envoy.
    [ScalarDB Cluster chart (this chart)] ---(Deploy)---> [Issuer (self-signed CA)] <---(Refer via issuerRef configuration)--- [Certificate resource] <---(Deploy)--- [Envoy chart]
    
  3. Update deployment.spec.template.spec.volumes and deployment.spec.template.spec.containers.volumeMounts to remove subPath configuration.

    • Cert-manager creates a secret resource that includes private key and certificate, and we use them by mounting them on pods. And, cert-manager automatically renew (update) the certificate in the secret resource before the certificate will be expired. However, if we use subPath to specify the mounted file name, Kubernetes does not apply the changes of the secret resource. So, to take advantage of cert-manager's automatically renew feature, we should not use subPath. This is because I removed subPath from the volumeMounts configuration.
  4. Update file names of private key and certificate.

    • From the perspective of cert-manager specification (strictly, TLS Secrets specification of Kubernetes), we must use the fixed names for private key and certificate files. This is because cert-manager creates a secret resource that includes private key and certificate files with fixed names based on the TLS Secrets specification. So, I updated the file names in existing manifests.
      • cert.pem -> tls.crt
      • key.pem -> tls.key
      • ca.pem -> ca.crt

Checklist

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

Support cert-manager in ScalarDB Cluster chart. You can manage private key and certificate for TLS connections by using cert-manager.

@kota2and3kan kota2and3kan added improvement scalardb cluster PR for ScalarDB Cluster chart labels Apr 26, 2024
@kota2and3kan kota2and3kan self-assigned this Apr 26, 2024
@@ -0,0 +1,83 @@
{{- if .Values.scalardbCluster.tls.certManager.enabled }}
{{- if not .Values.scalardbCluster.tls.certManager.issuerRef }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you don't specify Issuer in the issurRef configuration, this chart deploys self-signed CA to generate certificates.

If you specify the issuerRef configuration, this chart doesn't deploy self-signed CA.

Comment on lines 77 to 82
issuerRef:
{{- if .Values.scalardbCluster.tls.certManager.issuerRef }}
{{- toYaml .Values.scalardbCluster.tls.certManager.issuerRef | nindent 4 }}
{{- else }}
name: {{ include "scalardb-cluster.fullname" . }}-ca-issuer
{{- end }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you specify issuerRef, this chart uses that configuration in the Certificate resource. If you don't specify issuerRef, this chart uses a self-signed CA that is deployed by this chart.

{{- if .Values.scalardbCluster.tls.caRootCertSecret }}
- -tls-ca-cert=/tls/certs/ca-root-cert.pem
{{- end }}
- -tls-ca-cert=/tls/scalardb-cluster/certs/ca.crt
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When you enable TLS, the -tls-ca-cert option is mandatory. In other words, {{- if .Values.scalardbCluster.tls.caRootCertSecret }} does not make sense. So, I removed that unnecessary condition.

Comment on lines +96 to +98
{{- if .Values.scalardbCluster.tls.enabled }}
- name: scalardb-cluster-tls-volume
mountPath: /tls/scalardb-cluster/certs
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I combine all private keys and certificates into one volume to remove the subPath configuration.

Comment on lines +107 to +110
{{- if and .Values.scalardbCluster.tls.enabled .Values.scalardbCluster.tls.certManager.enabled }}
- name: scalardb-cluster-tls-volume
secret:
secretName: {{ .Values.scalardbCluster.tls.caRootCertSecret }}
secretName: {{ include "scalardb-cluster.fullname" . }}-tls-cert
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you use cert-manager, it creates one secret resource that includes all private keys and certificates. So, it's enough to mount that one secret here.

Comment on lines +112 to +121
{{- if and (.Values.scalardbCluster.tls.enabled) (not .Values.scalardbCluster.tls.certManager.enabled) }}
- name: scalardb-cluster-tls-volume
projected:
sources:
- secret:
name: {{ .Values.scalardbCluster.tls.caRootCertSecret }}
- secret:
name: {{ .Values.scalardbCluster.tls.certChainSecret }}
- secret:
name: {{ .Values.scalardbCluster.tls.privateKeySecret }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you don't use cert-manager (and you want to manage each file separately), you have to create three secrets for the private key, certificate, and CA certificate.

In this case, I want to combine them into one volume to mount them easily under the same directory in the container. So, I use projected volume here.

Comment on lines +309 to +310
dnsNames:
- localhost
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I set localhost as a default value of the dnsNames. This is because:

  • The grpc_health_probe command accesses ScalarDB Cluster by specifying localhost like -addr=localhost:60053.
  • The grpc_health_probe command verifies the certificate by using Subject Alternative Name (SAN) instead of Common Name (CN) of the certificate. In other words, at least one SAN configuration is necessary. (I will update the document in another PR.)

Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

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

LGTM! 👍
I didn't thoroughly review it, though.

Copy link

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@feeblefakie feeblefakie merged commit 503e5a4 into main May 31, 2024
14 checks passed
@feeblefakie feeblefakie deleted the support-cert-manager-scalardb-cluster branch May 31, 2024 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement scalardb cluster PR for ScalarDB Cluster chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants