Added Initial TLS support for ValkeyCluster#91
Added Initial TLS support for ValkeyCluster#91sandeepkunusoth wants to merge 5 commits intovalkey-io:mainfrom
Conversation
Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
| return &tls.Config{ | ||
| Certificates: []tls.Certificate{cert}, | ||
| RootCAs: caCertPool, | ||
| InsecureSkipVerify: true, |
There was a problem hiding this comment.
I guess we aim to remove InsecureSkipVerify soon? Or should it be configurable?
There was a problem hiding this comment.
i tried removing it. getting connection errors due to cert validation will check on this. the same works from valkey-cli but go client throws error
| Enabled bool `json:"enabled,omitempty"` | ||
|
|
||
| // Name of the Secret containing TLS keys | ||
| ExistingSecret string `json:"existingSecret,omitempty"` |
There was a problem hiding this comment.
Since we use the name "existing" here, will the cert-manager create a separate secret which it updates?
Or it named so that a user should know that is needs to exist?
There was a problem hiding this comment.
maybe we can change ExistingSecret to secretRef so that it can be used for both exisiting secret or secret generated by Certificate CRD.
There was a problem hiding this comment.
Could we name it certificateRef to provide a clearer name of intent? And have it be of a custom struct so that we can extend on it later on if we wanted to? This is what the elasticsearch operator does:
type SecretRef struct {
name string `json:"name,omitempty"`
}
|
|
||
| envVars = append(envVars, corev1.EnvVar{ | ||
| Name: "VALKEY_TLS_ARGS", | ||
| Value: fmt.Sprintf("--tls --cert %s --key %s --cacert %s", certPath, keyPath, caPath), |
There was a problem hiding this comment.
Ah, we don't need --insecure here but we have InsecureSkipVerify=true?
| // +kubebuilder:default=false | ||
| Enabled bool `json:"enabled,omitempty"` |
There was a problem hiding this comment.
Could we remove the enabled field and use the presence of tls as whether it should be enabled?
| if tlsConfig == nil || !tlsConfig.Enabled { | ||
| return "", "", "" | ||
| } |
There was a problem hiding this comment.
Would this block ever be hit? Seems we're always checking for this before we call getTLSFileNames
| } | ||
|
|
||
| if cluster.Spec.TLS.ExistingSecret == "" { | ||
| // TODO: cert-manager integration |
There was a problem hiding this comment.
How common is it for operators to depend on cert-manager? I checked ElasticSearch and CloudNativePG and they don't do this. Instead they self-manage their own certs, while allowing the user to override this by providing their own certs (possibly managed by cert-manager).
| // Secret key name containing server public certificate (default=server.crt) | ||
| // +kubebuilder:default=server.crt | ||
| Cert string `json:"cert,omitempty"` | ||
|
|
||
| // Secret key name containing server private key (default=server.key) | ||
| // +kubebuilder:default=server.key | ||
| Key string `json:"key,omitempty"` | ||
|
|
||
| // Secret key name containing Certificate Authority public certificate (default=ca.crt) | ||
| // +kubebuilder:default=ca.crt | ||
| CA string `json:"ca,omitempty"` |
There was a problem hiding this comment.
Are these defaults standard with the kubernetes.io/tls Secret type?
tls.crt
tls.key
ca.crt
| Enabled bool `json:"enabled,omitempty"` | ||
|
|
||
| // Name of the Secret containing TLS keys | ||
| ExistingSecret string `json:"existingSecret,omitempty"` |
There was a problem hiding this comment.
Could we name it certificateRef to provide a clearer name of intent? And have it be of a custom struct so that we can extend on it later on if we wanted to? This is what the elasticsearch operator does:
type SecretRef struct {
name string `json:"name,omitempty"`
}
| // TODO: cert-manager integration and rotation | ||
| // IssuerRef references a cert-manager Issuer or ClusterIssuer. | ||
| IssuerRef *IssuerRef `json:"issuerRef,omitempty"` |
There was a problem hiding this comment.
Similar comment here re cert-manager integration
|
|
||
| if cluster.Spec.TLS.ExistingSecret == "" { | ||
| // TODO: cert-manager integration | ||
| err := errors.New("tls.existingSecret is required when tls.enabled is true") |
There was a problem hiding this comment.
I think we should remove tls.enabled, and have presence on tls.certificateRef (renamed from tls.existingSecret) to determine whether TLS should be enabled. Otherwise tls.enabled isn't really doing a lot here. Plus it can be confusing to the user: "which part of TLS is enabled?"
3 participants
This PR adds initial TLS support to the Valkey Cluster, fixes #59. TLS enabled for ValkeyCluster using pre-created / existing Kubernetes Secrets. Updated e2e tests to test TLS.
Followup features not included in this PR: