Add TLS support to the Scalar Manager Helm chart #303
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
thongdk8
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR adds end-to-end TLS support to the Scalar Manager Helm chart, including inbound (client-to-server) and outbound (server-to-service) encryption, cert-manager integration, and updated docs.
- Introduces a unified
scalarManager.tlsvalues block for downstream (inbound) and upstream (outbound) TLS settings - Adds a
certmanager.yamltemplate to provision CA-based or self-signed certificates via cert-manager - Updates the deployment template to inject TLS environment variables and mount certificate volumes based on values
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| charts/scalar-manager/values.yaml | Added TLS keys under scalarManager.tls and injection into applicationProperties |
| charts/scalar-manager/values.schema.json | Extended the JSON schema to validate tls.certManager, tls.downstream, and tls.upstream |
| charts/scalar-manager/templates/scalar-manager/deployment.yaml | Injected TLS-related env vars and volume mounts for API and Web containers |
| charts/scalar-manager/templates/scalar-manager/certmanager.yaml | New cert-manager Issuer and Certificate resources for automated cert lifecycles |
| charts/scalar-manager/README.md | Documented the new TLS configuration parameters and defaults |
Comments suppressed due to low confidence (5)
charts/scalar-manager/values.yaml:130
- Environment variable names in applicationProperties do not match those set in the Deployment (e.g., using
${app.tls.inbound.enabled}instead of${APP_TLS_INBOUND_ENABLED}). Update the placeholders to the correct uppercase env var names.
server.ssl.enabled=${app.tls.inbound.enabled}
charts/scalar-manager/values.yaml:143
- Outbound TLS settings are only injected as environment variables and are not referenced in
applicationProperties. Ensure Spring Boot client SSL properties or application code use these vars to enable TLS for outbound connections.
# Outbound TLS configuration (Client connections to external services)
charts/scalar-manager/values.yaml:137
- [nitpick] Mixing the term
inboundin applicationProperties withdownstreamin the values structure may cause confusion. Standardize on one term across all configs.
app.tls.inbound.enabled=${APP_TLS_INBOUND_ENABLED}
charts/scalar-manager/templates/scalar-manager/deployment.yaml:88
- [nitpick] Repeated Helm blocks for mounting TLS volumes per service introduce duplication. Consider iterating over a list of services or extracting a named template to DRY up this logic.
{{- if .Values.scalarManager.tls.downstream.enabled }}
charts/scalar-manager/README.md:36
- [nitpick] The default value for
scalarManager.tlsis embedded as a large inline JSON and is hard to read. Consider splitting it into separate documented fields or moving the default object into a fenced code block for clarity.
| scalarManager.tls | object | `{"certManager":{"dnsNames":["localhost","scalar-manager"],"duration":"8760h0m0s","enabled":false,"issuerRef":{},"privateKey":{"algorithm":"RSA","encoding":"PKCS8","size":2048},"renewBefore":"360h0m0s","selfSigned":{"enabled":false,"type":"ca"},"usages":["server auth","key encipherment","signing"]},"downstream":{"caRootCertSecret":"","certChainSecret":"","enabled":false,"privateKeySecret":""},"upstream":{"grafana":{"caRootCertSecret":"","enabled":false,"overrideAuthority":""},"prometheus":{"caRootCertSecret":"","enabled":false,"overrideAuthority":""},"scalardb":{"caRootCertSecret":"","enabled":false,"overrideAuthority":""},"scalardl":{"caRootCertSecret":"","enabled":false,"overrideAuthority":""}}}` | Unified TLS configuration for both API and Web components. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/gemini review |
kota2and3kan
left a comment
kota2and3kan
left a comment
There was a problem hiding this comment.
Thank you for the updates!
I left several comments.
Please take a look when you have time! 🙇♂️
charts/scalar-manager/values.yaml
Outdated
| # -- Type of self-signed issuer (ca or selfSigned). | ||
| type: ca |
There was a problem hiding this comment.
I'd like to know the details of this scalarManager.tls.certManager.selfSigned.type configuration.
For example, I want to know the following things:
- What happens when we set
cahere? - What happens when we set
selfSignedhere? - When should we use
ca? When should we useselfSigned?
I think other existing Scalar Helm Charts (e.g., ScalarDB Cluster or ScalarDL) do not have this configuration. In other words, it seems that this configuration is the original one in only Scalar Manager. So, I want to confirm (judge) whether this configuration is mandatory or not. I want to keep the UI (configurations) consistent between all Scalar Helm Charts as long as possible. Therefore, if this configuration is not mandatory to run Scalar Manager, I want to remove this configuration for now, if we can, to keep consistency between other Scalar Helm Charts. And, I want to consider supporting this configuration in all Scalar Helm Charts if we need it in the future.
To decide the above things, could you please elaborate on more details of this configuration?
There was a problem hiding this comment.
These options will be available if we want the certs for Scalar Manager to be managed by itself (when the certManager.selfSigned.enabled: true
For the details, I summarized as below:
Two Self-Signed Types:
- ca type - Creates a self-signed Certificate Authority (CA) chain
- selfSigned type - Creates a direct self-signed certificate
For the CA Type (type: ca):
- Creates a self-signed CA certificate first
- Uses that CA to sign the actual TLS certificate
- Results in a proper certificate chain (CA → Server Certificate)
- The CA certificate can be distributed as a trust anchor
For the SelfSigned Type (type: selfSigned):
- Creates a single self-signed certificate directly
- No certificate chain - the certificate signs itself
- The certificate itself serves as the trust anchor
Use Cases:
When to use ca:
- Production-like environments where you want to simulate proper PKI hierarchy
- Multiple services that need certificates from the same CA
- Client trust distribution - easier to distribute one CA cert to clients
- Future flexibility - can issue multiple certificates from the same CA
When to use selfSigned:
- Quick testing where certificate chain doesn't matter
- Single service deployments
- Simplified setup with fewer certificate resources
Both options are valuable because they serve different deployment scenarios. The ca type provides better security practices, while selfSigned offers simplicity for development/testing.
The choice depends on the requirements for certificate management I think.
WDYT?
There was a problem hiding this comment.
Thank you for your explanation!
I understood the difference between type: ca and type: selfSigned.
However, I want to consider and discuss whether this type configuration is necessary or not.
I think there are three user scenarios as follows:
- Use a self-signed certificate. Mainly for a development perspective.
- Use a third-party CA to generate certificates. Mainly for a production environment.
- Use a private (customers' own) CA to generate certificates. Mainly for a production environment.
Also, I think both self-signed certificate and self-signed CA that you mentioned are included in scenario 1.. In other words, from the user scenario perspective, I think we don't need to separate self-signed certificate and self-signed CA.
In addition, I think if users want to use self-signed CA, they can create one Issuer resource by themselves, and they can specify the Issuer (self-signed CA) to scalarManager.tls.certManager.issuerRef. In other words, there are two ways to enable a self-signed CA. I think it might cause confusion for users. It's one of my concerns.
And, I want to keep consistency as long as possible between Scalar Helm Charts. Other Scalar products' Helm Charts do not provide such type configuration under the certManager configuration. If we really need such a type configuration, I want to update all Scalar Helm Charts at the same time to avoid providing different UI/UX for users as long as possible.
As the above reasons, I want to discuss whether we really need the type configuration.
In my opinion, we don't need to provide type configuration. And, we can always provide a self-signed CA (the same manifests that are generated by the type: ca configuration in the current chart) as only one option when users want to use a self-signed CA for testing. This is because:
- I think it's enough to provide only one option,
Use self-signed certificates. In other words, I feel there is no large difference betweentype:caandtype: selfSigned.- As I mentioned, users can specify their own self-signed CA in
scalarManager.tls.certManager.issuerRefif they want to do so for some reason.
- As I mentioned, users can specify their own self-signed CA in
- We can keep the consistency between other Scalar products' Helm Charts.
- Users can use similar configurations between all Scalar products. This can reduce confusion.
- We don't need to explain the difference between
type: caandtype: selfSignedin the document. We can say simplyIf you want to use cert-manager and self-signed certificates, set "scalarManager.tls.certManager.selfSigned.enabled=true".- This can reduce the maintenance costs of both Helm Chart and documents.
As I mentioned above, if there is no strong reason to provide the type configuration, I'd like to remove this type option and make it simpler.
What do you think?
There was a problem hiding this comment.
Thank you for your comments and suggestions. I agree with that, doing it will simplify our chart. I made updates to apply the suggestions, PTAL again when you have time.
charts/scalar-manager/values.yaml
Outdated
| # -- ScalarDB TLS configuration. | ||
| scalardb: |
There was a problem hiding this comment.
It seems that this configuration assumes that there is only one ScalarDB Cluster deployment. Is my understanding correct?
If so, I think we should discuss and decide which we support, Only one ScalarDB Cluster deployment or Several ScalarDB Cluster deployments in Scalar Manager.
If we support Only one ScalarDB Cluster deployment, the current configuration might be fine. But, if we support Several ScalarDB Cluster deployments, we should consider how to set the several TLS configurations per ScalarDB Cluster deployment becase different ScalarDB Cluster deployments might have different TLS configurations.
I think this is related to the design of Scalar Manager itself. Not for Helm Chart.
@feeblefakie
I want to discuss this with you. What do you think?
There was a problem hiding this comment.
Currently, as the Scalar Manager will work with one ScalarDB Cluster deployment only, I think.
We simplified the architecture, and as you can see in the home page of the Scalar Manager UI, we dont have any section for multiple deployments of ScalarDB (the initial version we have a selection for multiple clusters, but we removed that part). If we want to support multiple clusters, then we have to reconsider the architecture.
There was a problem hiding this comment.
I discussed with @feeblefakie and @ypeckstadt in the meeting. As a result, we decided to update Scalar Manager.
You may already know it, but as mentioned above, first, we will update the Scalar Manager side. After that, let's discuss the updates of the Helm Chart based on the new Scalar Manager.
There was a problem hiding this comment.
@kota2and3kan I have made some updates to support multiple namespaces. PTAL again when you have time
charts/scalar-manager/values.yaml
Outdated
| # -- ScalarDL TLS configuration. | ||
| scalardl: |
There was a problem hiding this comment.
As well as the ScalarDB Cluster's one, it seems that this configuration assumes there is only one ScalarDL deployment. Therefore, I think we should discuss and decide which we support, Only one ScalarDL deployment or Several ScalarDL deployments.
Also, in a case of ScalarDL, we need to support both ScalarDL Ledger and ScalarDL Auditor. And, Ledger and Auditor might have different TLS configurations because ScalarDL requires different administrative domains for production use to ensure the BFD works properly.
In summary, we should decide (choose) one of the following options from the perspective of the design of Scalar Manager itself.
- Support only one ScalarDL Ledger and only one ScalarDL Auditor.
- Support several ScalarDL Ledger deployments and several ScalarDL Auditor deployments.
@feeblefakie
I want to discuss this with you. What do you think?
There was a problem hiding this comment.
Same as this comment:
| imagePullPolicy: {{ .Values.scalarManager.api.image.pullPolicy }} | ||
| securityContext: | ||
| {{- toYaml .Values.scalarManager.securityContext | nindent 12 }} | ||
| env: |
There was a problem hiding this comment.
I think we need to configure Scalar Manager Web via environment variables because there is no configuration files like application.properties.
However, I think we can configure Scalar Manager API by using application.properties. And, it seems that there are two ways to configure Scalar Manager API in this update. One is using the application.properties file. Another is via environment variables.
My concern is that mixing two configuration ways might cause confusion for users. Is it possible to use only one of application.properties or environment variables?
Also, we already provide the way to configure Scalar Manager API by using application.properties. Therefore, to keep backward compatibility and consistency of How to configure Scalar Manager API, I think it would be better to use only application.properties instead of mixing both application.properties and environment variables.
What do you think?
There was a problem hiding this comment.
Actually, the main config for the API is still application.properties. For the API we add several properties for the TLS in the application.properties, and those configs will get the TLS configs from the env by default. If user overrides these configs, then the API will accept the values user enters.
This env part of the API is for the TLS only as I aimed for one TLS config block in the values file, and the certs is the same for Scalar Manager API and Web. If we dont do the env mapping here, then users have to configure the TLS separately in the application.properties. Also, we still dont expose any env config for the API in values. WDYT?
There was a problem hiding this comment.
I have one question.
If we don't set app.tls.inbound.enabled=${APP_TLS_INBOUND_ENABLED} in the application.properties file explicitly as follows, and we set scalarManager.tls.downstream.enabled=true, does Scalar Manager API start with TLS enabled configuration?
scalarManager:
api:
applicationProperties: |
spring.datasource.url=jdbc:postgresql://user.postgresql.example.com:5432/postgres
spring.datasource.username=postgres
spring.datasource.password=postgres
spring.datasource.driver-class-name=org.postgresql.Driver
tls:
downstream:
enabled: trueIn this case, I think some default configurations like app.tls.inbound.enabled=${APP_TLS_INBOUND_ENABLED} are not applied because the default application.properties is overwritten by the above user-specified application.properties. Is my understanding correct?
If my understanding is correct, Scalar Manager API does not start with TLS configuration even if we set scalarManager.tls.downstream.enabled=true because there is no configuration about TLS on the overwritten (user-specified) application.properties side.
That's why I want to ask this question. I'd like to know the behavior of the Scalar Manager API in the above configuration.
There was a problem hiding this comment.
Yes, that is expected behavior.
But based on this situation, I made some updates to simplify the default config. With these updates, the TLS config will automatically be bound to the API via environment variables directly from the TLS config block in the values.yml. So users dont have to set the values in the properties for TLS, except custom values if they want to.
PTAL again when have time and let me know your thoughts
charts/scalar-manager/values.yaml
Outdated
| server.ssl.certificate=${app.tls.inbound.cert-path} | ||
| server.ssl.certificate-private-key=${app.tls.inbound.private-key-path} | ||
| # Include both ECDSA and RSA ciphers for maximum compatibility | ||
| server.ssl.ciphers=TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_RSA_WITH_AES_256_GCM_SHA384,TLS_RSA_WITH_AES_128_GCM_SHA256 |
There was a problem hiding this comment.
Do we need to set the ciphers configuration in the application.properties explicitly?
I think it would be better to use default as long as possible if there is no problem.
This is because the default (recommended ciphers) might be updated dynamically by following the trend or some updates on the library side.
My concern is that setting the concrete value might cause Users use the old (non-recommended) cipher in the future version.
Therefore, I think it would be better to use the default (default of library or flamework) if there is no strong reason to set some concrete values.
What do you think?
There was a problem hiding this comment.
Actually, I added these configs as I followed the guide for setting up ScalarDB with TLS and generating the certs by the example command and it is using
"key": {
"algo": "ecdsa",
"size": 256
}
So to also enable the Scalar Manager be flexible in accepting the certs config, then I added this section for more cipher support. It is up to users, if they use a strong one in cert generation, then no problem, but if they use a weak one, (like above) then by default our API will fail to start. Adding these ciphers will resolve it. Of course, we can leave it as the default. WDYT?
There was a problem hiding this comment.
Ah, sorry. I lacked descriptions in the previous comment.
I agree with making the acceptable cipher suites configurable.
However, I think we should not set the default (hard-coded default) value for it on our side.
Regarding the default value of server.ssl.ciphers, I think we should use the library's default as a default value so that we can follow the security trend.
For example, set the default property (default values.yaml) as follows:
scalarManager:
api:
applicationProperties: |
(omit other configurations)
# By default, Scalar Manager API accepts the library default cipher suites. You can configure any cipher suites you prefer if you want.
server.ssl.ciphers=If users do not specify any cipher suites, the library's default (based on the security trend) is set.
If users want to set their preferred cipher suites for some reason, users can overwrite them in the application.properties file.
This can keep the default value secure based on the security trend (library's default), and this can provide flexible options for users.
As above, my suggestions are the following:
- Make the acceptable cipher suites configurable (As you already did).
- However, don't set the default value on our side (i.e., set empty or some special value to use the library's default value). In other words, removing the current default values that are set on the Helm Chart (
values.yamlfile) side.
There was a problem hiding this comment.
Yes, you are correct. I simplify it to leave it be unset as default values and add a comment for it to explain usage if users want to.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@thongdk8 |
|
@kota2and3kan Thank you for checking. I made some updates and replied to your comments as well, PTAL when you get a chance. |
2 participants
Description
This PR adds comprehensive TLS support to the Scalar Manager Helm chart, enabling secure communication for both inbound connections (clients to Scalar Manager) and outbound connections (Scalar Manager to external services). The implementation provides flexible certificate management options, supporting both manual certificate provisioning and automated certificate generation through cert-manager integration.
Related issues and/or PRs
Changes made
1. New TLS Configuration Structure
scalarManager.tlsin values.yaml2. Certificate Management
certmanager.yamltemplate for automated certificate generation3. Deployment Enhancements
4. Application Configuration
5. Documentation Updates
Checklist
Additional notes (optional)
Key Design Decisions:
Testing Recommendations:
Release notes
Added comprehensive TLS support to Scalar Manager Helm chart, including: