Skip to content

Option to merge the JVM truststore with user-supplied truststore #461

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.

Sign up for GitHub

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

Draft
thelabdude wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Option to merge the JVM truststore with user-supplied truststore #461

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dcb59ee
Option to merge the JVM truststore with user-supplied truststore
thelabdude Jul 28, 2022
0420970
Add some bulk to the tests
HoustonPutman Aug 1, 2022
f99abc0
Add resource requests for the merge initContainer
thelabdude Aug 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Add resource requests for the merge initContainer
  • Loading branch information
@thelabdude
thelabdude committed Aug 1, 2022
commit f99abc04a251292f3bffdfc98fa594b55aeadba7
2 changes: 2 additions & 0 deletions api/v1beta1/solrcloud_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1527,9 +1527,11 @@ type SolrTLSOptions struct {
// Path on the Solr image to your JVM's truststore to merge with an external truststore.
// If supplied, Solr will be configured to use the merged truststore.
// The truststore for the JVM in the default Solr image is: $JAVA_HOME/lib/security/cacerts
// +optional
MergeJavaTruststore string `json:"mergeJavaTrustStore,omitempty"`
Copy link
Contributor

@HoustonPutman HoustonPutman Aug 1, 2022

Choose a reason for hiding this comment

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

Can we add the // +optional tag for both of these new options? Just for consistency sake

Copy link
Contributor

@HoustonPutman HoustonPutman Aug 1, 2022

Choose a reason for hiding this comment

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

Does this work with mountedTLSDir?

Copy link
Contributor Author

@thelabdude thelabdude Aug 1, 2022

Choose a reason for hiding this comment

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

Typically, mountedTLSDir will have a csi driver volume and corresponding mount on the mainContainer, which would get used by the merge initContainer, so the init container would get the truststore file. However, that might cause double creation of the cert for each pod, once for the initContainer and once for the main container, so this would likely put a lot of pressure on the Cert issuer. So probably safer to say this feature is not supported with mountedTLSDir option for now.

Copy link
Contributor Author

@thelabdude thelabdude Aug 1, 2022

Choose a reason for hiding this comment

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

No, nvm all that volumesAndMounts doesn't include vols & mounts for the mountedTLSDir option. So this option is not going to work with mountedTLSDir.

So maybe we just punt on this feature for 0.6 and solve it using an init-db script instead? There's already some code in place for mounting a script into the init-db.


// Password for the Java truststore to merge; defaults to "changeit"
// +optional
MergeJavaTruststorePass string `json:"mergeJavaTrustStorePass,omitempty"`
Copy link
Contributor

@HoustonPutman HoustonPutman Aug 1, 2022

Choose a reason for hiding this comment

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

Should this be a secret reference? How bad is it to have this in plain text?

Copy link
Contributor Author

@thelabdude thelabdude Aug 1, 2022

Choose a reason for hiding this comment

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

seemed like overkill to me since it's the truststore pass for the JVM which is most likely "changeit" and isn't used by Solr ... that said, we can pull from a secret too ... doubt many would ever use this option.

Copy link
Contributor

@HoustonPutman HoustonPutman Aug 1, 2022

Choose a reason for hiding this comment

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

ahhh ok, didn't understand it was unusual to change it. Sounds good

}

Expand Down
51 changes: 44 additions & 7 deletions controllers/util/solr_tls_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
solr "github.com/apache/solr-operator/api/v1beta1"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
"strconv"
Expand All @@ -46,6 +47,11 @@ const (
DefaultJvmTruststore = "$JAVA_HOME/lib/security/cacerts"
)

var (
DefaultMergeTruststoreInitContainerMemory = resource.NewScaledQuantity(64, 6)
DefaultMergeTruststoreInitContainerCPU = resource.NewMilliQuantity(50, resource.DecimalExponent)
)

// Helper struct for holding server and/or client cert config
// This struct is intended for internal use only and is only exposed outside the package so that the controllers can access
type TLSCerts struct {
Expand All @@ -55,6 +61,8 @@ type TLSCerts struct {
ClientConfig *TLSConfig
// Image used for initContainers that help configure the TLS settings
InitContainerImage *solr.ContainerImage
// Holds custom resource requests for the initContainer if the defaults are overridden by config
InitContainerResources *corev1.ResourceRequirements
}

// Holds TLS options from the user config as well as other config properties determined during reconciliation
Expand All @@ -78,6 +86,7 @@ type TLSConfig struct {

// Get a TLSCerts struct for reconciling TLS on a SolrCloud
func TLSCertsForSolrCloud(instance *solr.SolrCloud) *TLSCerts {

tls := &TLSCerts{
ServerConfig: &TLSConfig{
Options: instance.Spec.SolrTLS.DeepCopy(),
Expand All @@ -100,6 +109,14 @@ func TLSCertsForSolrCloud(instance *solr.SolrCloud) *TLSCerts {
EnvVarPrefix: "SOLR_SSL_CLIENT",
}
}

if instance.Spec.CustomSolrKubeOptions.PodOptions != nil {
resources := instance.Spec.CustomSolrKubeOptions.PodOptions.DefaultInitContainerResources
if resources.Limits != nil || resources.Requests != nil {
tls.InitContainerResources = &resources
}
}

return tls
}

Expand All @@ -114,7 +131,7 @@ func TLSCertsForExporter(prometheusExporter *solr.SolrPrometheusExporter) *TLSCe
PullPolicy: solr.DefaultPullPolicy,
}
}
return &TLSCerts{
tls := &TLSCerts{
ClientConfig: &TLSConfig{
Options: prometheusExporter.Spec.SolrReference.SolrTLS.DeepCopy(),
KeystorePath: DefaultKeyStorePath,
Expand All @@ -125,6 +142,15 @@ func TLSCertsForExporter(prometheusExporter *solr.SolrPrometheusExporter) *TLSCe
},
InitContainerImage: bbImage,
}

if prometheusExporter.Spec.CustomKubeOptions.PodOptions != nil {
resources := prometheusExporter.Spec.CustomKubeOptions.PodOptions.DefaultInitContainerResources
if resources.Limits != nil || resources.Requests != nil {
tls.InitContainerResources = &resources
}
}

return tls
}

// Enrich the config for a SolrCloud StatefulSet to enable TLS, either loaded from a secret or
Expand Down Expand Up @@ -159,10 +185,10 @@ func (tls *TLSCerts) enableTLSOnSolrCloudStatefulSet(stateful *appsv1.StatefulSe
}

if serverCert.Options.MergeJavaTruststore != "" {
serverCert.addMergeTruststoreInitContainer(&stateful.Spec.Template)
serverCert.addMergeTruststoreInitContainer(&stateful.Spec.Template, tls.InitContainerResources)
}
if tls.ClientConfig != nil && tls.ClientConfig.Options.MergeJavaTruststore != "" {
tls.ClientConfig.addMergeTruststoreInitContainer(&stateful.Spec.Template)
tls.ClientConfig.addMergeTruststoreInitContainer(&stateful.Spec.Template, tls.InitContainerResources)
}
}

Expand All @@ -188,7 +214,7 @@ func (tls *TLSCerts) enableTLSOnExporterDeployment(deployment *appsv1.Deployment

if clientCert.Options.MergeJavaTruststore != "" {
// add an initContainer that merges the truststores together
clientCert.addMergeTruststoreInitContainer(&deployment.Spec.Template)
clientCert.addMergeTruststoreInitContainer(&deployment.Spec.Template, tls.InitContainerResources)
}
}

Expand Down Expand Up @@ -827,7 +853,7 @@ func verifyTLSSecretConfig(client *client.Client, secretName string, secretNames
}

// Adds an initContainer that merges the JVM's truststore with the user-supplied truststore
func (tls *TLSConfig) addMergeTruststoreInitContainer(template *corev1.PodTemplateSpec) {
func (tls *TLSConfig) addMergeTruststoreInitContainer(template *corev1.PodTemplateSpec, initContRes *corev1.ResourceRequirements) {
mainContainer := &template.Spec.Containers[0]

// supports either client or server truststore env var names
Expand All @@ -836,6 +862,9 @@ func (tls *TLSConfig) addMergeTruststoreInitContainer(template *corev1.PodTempla
// build an initContainer that merges the truststores together
initContainer, mergeVol, mergeMount :=
tls.buildMergeTruststoreInitContainer(mainContainer.Image, mainContainer.ImagePullPolicy, mainContainer.Env)
if initContRes != nil {
initContainer.Resources = *initContRes
}
template.Spec.InitContainers = append(template.Spec.InitContainers, *initContainer)
template.Spec.Volumes = append(template.Spec.Volumes, *mergeVol)
mainContainer.VolumeMounts = append(mainContainer.VolumeMounts, *mergeMount)
Expand Down Expand Up @@ -883,7 +912,12 @@ keytool -importkeystore -srckeystore $%s -srcstorepass $%s -destkeystore $%s -de
mergeMount := &corev1.VolumeMount{Name: volName, ReadOnly: false, MountPath: mountPath}
mounts = append(mounts, *mergeMount)

return &corev1.Container{
volumePrepResources := corev1.ResourceList{
corev1.ResourceCPU: *DefaultMergeTruststoreInitContainerCPU,
corev1.ResourceMemory: *DefaultMergeTruststoreInitContainerMemory,
}

initCont := &corev1.Container{
Name: volName,
Image: solrImageName, // we use the Solr image for the initContainer since it has the truststore and keytool
ImagePullPolicy: imagePullPolicy,
Expand All @@ -892,7 +926,10 @@ keytool -importkeystore -srckeystore $%s -srcstorepass $%s -destkeystore $%s -de
Command: []string{"sh", "-c", cmd},
VolumeMounts: mounts,
Env: envVars,
}, mergeVol, mergeMount
Resources: corev1.ResourceRequirements{Requests: volumePrepResources, Limits: volumePrepResources},
}

return initCont, mergeVol, mergeMount
}

func (tls *TLSConfig) trustStoreEnvVarName() string {
Expand Down
8 changes: 8 additions & 0 deletions helm/solr-operator/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,14 @@ annotations:
url: https://github.com/apache/solr-operator/issues/435
- name: Github PR
url: https://github.com/apache/solr-operator/pull/456
- kind: added
description: TLS config provides an option to merge the JVM truststore from the Solr image with a user-supplied truststore.
links:
- name: Github Issue
url: https://github.com/apache/solr-operator/issues/390
- name: Github PR
url: https://github.com/apache/solr-operator/pull/461

artifacthub.io/images: |
- name: solr-operator
image: apache/solr-operator:v0.6.0-prerelease
Expand Down