Bugs: wrong address and memory leak (#285)

* Several `defer c.Close()` added to clean up etcd clients after reconciliation.
* DNS names targeting etcd cluster members fixed to resolve properly.
* TLS capability added to etcd clients created during reconciliation.

Author: lllamnyp

internal/controller/etcdcluster_controller.go

@@ -111,6 +111,18 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
+	defer func() {
+		if clusterClient != nil {
+			// nolint:errcheck
+			clusterClient.Close()
+		}
+		for i := range singleClients {
+			if singleClients[i] != nil {
+				// nolint:errcheck
+				singleClients[i].Close()
+			}
+		}
+	}()
 	state.endpointsFound = clusterClient != nil && singleClients != nil
 
 	if !state.endpointsFound {
@@ -286,6 +298,8 @@ func (r *EtcdClusterReconciler) ensureConditionalClusterObjects(
 }
 
 // updateStatusOnErr wraps error and updates EtcdCluster status
+// TODO: refactor this so the linter doesn't complain
+// nolint:unparam
 func (r *EtcdClusterReconciler) updateStatusOnErr(ctx context.Context, cluster *etcdaenixiov1alpha1.EtcdCluster, err error) (ctrl.Result, error) {
 	// The function 'updateStatusOnErr' will always return non-nil error. Hence, the ctrl.Result will always be ignored.
 	// Therefore, the ctrl.Result returned by 'updateStatus' function can be discarded.
@@ -341,9 +355,8 @@ func (r *EtcdClusterReconciler) configureAuth(ctx context.Context, cluster *etcd
 		return err
 	}
 
-	defer func() {
-		err = cli.Close()
-	}()
+	// nolint:errcheck
+	defer cli.Close()
 
 	err = testMemberList(ctx, cli)
 	if err != nil {

internal/controller/factory/etcd_client.go

@@ -2,7 +2,11 @@ package factory
 
 import (
 	"context"
+	"crypto/tls"
+	"crypto/x509"
+	"encoding/pem"
 	"fmt"
+	"time"
 
 	"github.com/aenix-io/etcd-operator/api/v1alpha1"
 	clientv3 "go.etcd.io/etcd/client/v3"
@@ -29,6 +33,8 @@ func NewEtcdClientSet(ctx context.Context, cluster *v1alpha1.EtcdCluster, cli cl
 		cfg.Endpoints = []string{ep}
 		singleClients[i], err = clientv3.New(cfg)
 		if err != nil {
+			// nolint:errcheck
+			clusterClient.Close()
 			return nil, nil, fmt.Errorf("error building etcd single-endpoint client for endpoint %s: %w", ep, err)
 		}
 	}
@@ -56,8 +62,72 @@ func configFromCluster(ctx context.Context, cluster *v1alpha1.EtcdCluster, cli c
 		}
 	}
 	for name := range names {
-		urls = append(urls, fmt.Sprintf("%s:%s", name, "2379"))
+		urls = append(urls, fmt.Sprintf("%s.%s.%s.svc:%s", name, ep.Name, ep.Namespace, "2379"))
 	}
+	etcdClient := clientv3.Config{Endpoints: urls, DialTimeout: 5 * time.Second}
 
-	return clientv3.Config{Endpoints: urls}, nil
+	if s := cluster.Spec.Security; s != nil {
+		if s.TLS.ClientSecret == "" {
+			return clientv3.Config{}, fmt.Errorf("cannot configure client: security enabled, but client certificates not set")
+		}
+		clientSecret := &v1.Secret{}
+		err := cli.Get(ctx, types.NamespacedName{Namespace: cluster.Namespace, Name: s.TLS.ClientSecret}, clientSecret)
+		if err != nil {
+			return clientv3.Config{}, fmt.Errorf("cannot configure client: failed to get client certificate: %w", err)
+		}
+		cert, err := parseTLSSecret(clientSecret)
+		if err != nil {
+			return clientv3.Config{}, fmt.Errorf("cannot configure client: failed to extract keypair from client secret: %w", err)
+		}
+		etcdClient.TLS = &tls.Config{}
+		etcdClient.TLS.Certificates = []tls.Certificate{cert}
+		etcdClient.TLS.GetClientCertificate = func(cri *tls.CertificateRequestInfo) (*tls.Certificate, error) {
+			return &etcdClient.TLS.Certificates[0], nil
+		}
+		caSecret := &v1.Secret{}
+		err = cli.Get(ctx, types.NamespacedName{Namespace: cluster.Namespace, Name: s.TLS.ServerSecret}, caSecret)
+		if err != nil {
+			return clientv3.Config{}, fmt.Errorf("cannot configure client: failed to get server CA secret: %w", err)
+		}
+		ca, err := parseTLSSecretCA(caSecret)
+		if err != nil {
+			return clientv3.Config{}, fmt.Errorf("cannot configure client: failed to extract Root CA from server secret: %w", err)
+		}
+		pool := x509.NewCertPool()
+		pool.AddCert(ca)
+		etcdClient.TLS.RootCAs = pool
+	}
+
+	return etcdClient, nil
+}
+
+func parseTLSSecret(secret *v1.Secret) (cert tls.Certificate, err error) {
+	certPem, ok := secret.Data["tls.crt"]
+	if !ok {
+		return tls.Certificate{}, fmt.Errorf("tls.crt not found in secret")
+	}
+	keyPem, ok := secret.Data["tls.key"]
+	if !ok {
+		return tls.Certificate{}, fmt.Errorf("tls.key not found in secret")
+	}
+	cert, err = tls.X509KeyPair(certPem, keyPem)
+	if err != nil {
+		err = fmt.Errorf("failed to load x509 key pair: %w", err)
+	}
+	return
+}
+
+func parseTLSSecretCA(secret *v1.Secret) (ca *x509.Certificate, err error) {
+	caPemBytes, ok := secret.Data["ca.crt"]
+	if !ok {
+		err = fmt.Errorf("secret does not contain ca.crt")
+		return
+	}
+	block, _ := pem.Decode(caPemBytes)
+	if block == nil {
+		err = fmt.Errorf("failed to decode PEM bytes")
+		return
+	}
+	ca, err = x509.ParseCertificate(block.Bytes)
+	return
 }