Merge pull request #186 from arangodb/bugfix/tls-and-sync-fixes

Various TLS & Sync related fixes

Author: ewoutp

docs/Manual/Deployment/Kubernetes/DeploymentReplicationResource.md

@@ -39,7 +39,7 @@ metadata:
   name: "replication-from-a-to-b"
 spec:
   source:
-    endpoint: ["https://163.172.149.229:31888", "https://51.15.225.110:31888", "https://51.15.229.133:31888"]
+    masterEndpoint: ["https://163.172.149.229:31888", "https://51.15.225.110:31888", "https://51.15.229.133:31888"]
     auth:
       keyfileSecretName: cluster-a-sync-auth
     tls:
@@ -67,7 +67,7 @@ with sync enabled.
 
 This cluster configured as the replication source.
 
-### `spec.source.endpoint: []string`
+### `spec.source.masterEndpoint: []string`
 
 This setting specifies zero or more master endpoint URL's of the source cluster.
 
@@ -108,7 +108,7 @@ with sync enabled.
 
 This cluster configured as the replication destination.
 
-### `spec.destination.endpoint: []string`
+### `spec.destination.masterEndpoint: []string`
 
 This setting specifies zero or more master endpoint URL's of the destination cluster.
 

pkg/apis/deployment/v1alpha/sync_external_access_spec.go

@@ -35,7 +35,7 @@ import (
 type SyncExternalAccessSpec struct {
 	ExternalAccessSpec
 	MasterEndpoint           []string `json:"masterEndpoint,omitempty"`
-	AccessPackageSecretNames []string `json:accessPackageSecretNames,omitempty"`
+	AccessPackageSecretNames []string `json:"accessPackageSecretNames,omitempty"`
 }
 
 // GetMasterEndpoint returns the value of masterEndpoint.

pkg/apis/replication/v1alpha/endpoint_spec.go

@@ -36,8 +36,8 @@ type EndpointSpec struct {
 	// DeploymentName holds the name of an ArangoDeployment resource.
 	// If set this provides default values for masterEndpoint, auth & tls.
 	DeploymentName *string `json:"deploymentName,omitempty"`
-	// Endpoint holds a list of URLs used to reach the syncmaster(s).
-	Endpoint []string `json:"endpoint,omitempty"`
+	// MasterEndpoint holds a list of URLs used to reach the syncmaster(s).
+	MasterEndpoint []string `json:"masterEndpoint,omitempty"`
 	// Authentication holds settings needed to authentication at the syncmaster.
 	Authentication EndpointAuthenticationSpec `json:"auth"`
 	// TLS holds settings needed to verify the TLS connection to the syncmaster.
@@ -60,13 +60,13 @@ func (s EndpointSpec) Validate(isSourceEndpoint bool) error {
 	if err := k8sutil.ValidateOptionalResourceName(s.GetDeploymentName()); err != nil {
 		return maskAny(err)
 	}
-	for _, ep := range s.Endpoint {
+	for _, ep := range s.MasterEndpoint {
 		if _, err := url.Parse(ep); err != nil {
 			return maskAny(errors.Wrapf(ValidationError, "Invalid master endpoint '%s': %s", ep, err))
 		}
 	}
 	hasDeploymentName := s.HasDeploymentName()
-	if !hasDeploymentName && len(s.Endpoint) == 0 {
+	if !hasDeploymentName && len(s.MasterEndpoint) == 0 {
 		return maskAny(errors.Wrapf(ValidationError, "Provide a deploy name or at least one master endpoint"))
 	}
 	if err := s.Authentication.Validate(isSourceEndpoint || !hasDeploymentName); err != nil {

pkg/apis/replication/v1alpha/zz_generated.deepcopy.go

@@ -240,8 +240,8 @@ func (in *EndpointSpec) DeepCopyInto(out *EndpointSpec) {
 			**out = **in
 		}
 	}
-	if in.Endpoint != nil {
-		in, out := &in.Endpoint, &out.Endpoint
+	if in.MasterEndpoint != nil {
+		in, out := &in.MasterEndpoint, &out.MasterEndpoint
 		*out = make([]string, len(*in))
 		copy(*out, *in)
 	}

pkg/deployment/context_impl.go

@@ -25,6 +25,8 @@ package deployment
 import (
 	"context"
 	"fmt"
+	"net"
+	"strconv"
 
 	"github.com/arangodb/arangosync/client"
 	"github.com/arangodb/arangosync/tasks"
@@ -175,7 +177,11 @@ func (d *Deployment) GetSyncServerClient(ctx context.Context, group api.ServerGr
 	dnsName := k8sutil.CreatePodDNSName(d.apiObject, group.AsRole(), id)
 
 	// Build client
-	source := client.Endpoint{dnsName}
+	port := k8sutil.ArangoSyncMasterPort
+	if group == api.ServerGroupSyncWorkers {
+		port = k8sutil.ArangoSyncWorkerPort
+	}
+	source := client.Endpoint{"https://" + net.JoinHostPort(dnsName, strconv.Itoa(port))}
 	tlsAuth := tasks.TLSAuthentication{
 		TLSClientAuthentication: tasks.TLSClientAuthentication{
 			ClientToken: monitoringToken,

pkg/deployment/reconcile/action_wait_for_member_up.go

@@ -174,7 +174,7 @@ func (a *actionWaitForMemberUp) checkProgressCluster(ctx context.Context) (bool,
 // of a sync master / worker.
 func (a *actionWaitForMemberUp) checkProgressArangoSync(ctx context.Context) (bool, bool, error) {
 	log := a.log
-	c, err := a.actionCtx.GetSyncServerClient(ctx, a.action.Group, a.action.ID)
+	c, err := a.actionCtx.GetSyncServerClient(ctx, a.action.Group, a.action.MemberID)
 	if err != nil {
 		log.Debug().Err(err).Msg("Failed to create arangosync client")
 		return false, false, maskAny(err)

pkg/deployment/reconcile/plan_builder_tls.go

@@ -63,10 +63,14 @@ func createRotateTLSServerCertificatePlan(log zerolog.Logger, spec api.Deploymen
 					Msg("Failed to get TLS secret")
 				continue
 			}
-			renewalNeeded := tlsKeyfileNeedsRenewal(log, keyfile)
+			tlsSpec := spec.TLS
+			if group.IsArangosync() {
+				tlsSpec = spec.Sync.TLS
+			}
+			renewalNeeded, reason := tlsKeyfileNeedsRenewal(log, keyfile, tlsSpec)
 			if renewalNeeded {
 				plan = append(append(plan,
-					api.NewAction(api.ActionTypeRenewTLSCertificate, group, m.ID)),
+					api.NewAction(api.ActionTypeRenewTLSCertificate, group, m.ID, reason)),
 					createRotateMemberPlan(log, m, group, "TLS certificate renewal")...,
 				)
 			}
@@ -133,8 +137,32 @@ func createRotateTLSCAPlan(log zerolog.Logger, apiObject k8sutil.APIObject,
 
 // tlsKeyfileNeedsRenewal decides if the certificate in the given keyfile
 // should be renewed.
-func tlsKeyfileNeedsRenewal(log zerolog.Logger, keyfile string) bool {
+func tlsKeyfileNeedsRenewal(log zerolog.Logger, keyfile string, spec api.TLSSpec) (bool, string) {
 	raw := []byte(keyfile)
+	// containsAll returns true when all elements in the expected list
+	// are in the actual list.
+	containsAll := func(actual []string, expected []string) bool {
+		for _, x := range expected {
+			found := false
+			for _, y := range actual {
+				if x == y {
+					found = true
+					break
+				}
+			}
+			if !found {
+				return false
+			}
+		}
+		return true
+	}
+	ipsToStringSlice := func(list []net.IP) []string {
+		result := make([]string, len(list))
+		for i, x := range list {
+			result[i] = x.String()
+		}
+		return result
+	}
 	for {
 		var derBlock *pem.Block
 		derBlock, raw = pem.Decode(raw)
@@ -146,7 +174,7 @@ func tlsKeyfileNeedsRenewal(log zerolog.Logger, keyfile string) bool {
 			if err != nil {
 				// We do not understand the certificate, let's renew it
 				log.Warn().Err(err).Msg("Failed to parse x509 certificate. Renewing it")
-				return true
+				return true, "Cannot parse x509 certificate: " + err.Error()
 			}
 			if cert.IsCA {
 				// Only look at the server certificate, not CA or intermediate
@@ -162,42 +190,31 @@ func tlsKeyfileNeedsRenewal(log zerolog.Logger, keyfile string) bool {
 					Str("not-after", cert.NotAfter.String()).
 					Str("expiration-date", expirationDate.String()).
 					Msg("TLS certificate renewal needed")
-				return true
+				return true, "Server certificate about to expire"
+			}
+			// Check alternate names against spec
+			dnsNames, ipAddresses, emailAddress, err := spec.GetParsedAltNames()
+			if err == nil {
+				if !containsAll(cert.DNSNames, dnsNames) {
+					return true, "Some alternate DNS names are missing"
+				}
+				if !containsAll(ipsToStringSlice(cert.IPAddresses), ipAddresses) {
+					return true, "Some alternate IP addresses are missing"
+				}
+				if !containsAll(cert.EmailAddresses, emailAddress) {
+					return true, "Some alternate email addresses are missing"
+				}
 			}
 		}
 	}
-	return false
+	return false, ""
 }
 
 // tlsCANeedsRenewal decides if the given CA certificate
 // should be renewed.
 // Returns: shouldRenew, reason
 func tlsCANeedsRenewal(log zerolog.Logger, cert string, spec api.TLSSpec) (bool, string) {
 	raw := []byte(cert)
-	// containsAll returns true when all elements in the expected list
-	// are in the actual list.
-	containsAll := func(actual []string, expected []string) bool {
-		for _, x := range expected {
-			found := false
-			for _, y := range actual {
-				if x == y {
-					found = true
-					break
-				}
-			}
-			if !found {
-				return false
-			}
-		}
-		return true
-	}
-	ipsToStringSlice := func(list []net.IP) []string {
-		result := make([]string, len(list))
-		for i, x := range list {
-			result[i] = x.String()
-		}
-		return result
-	}
 	for {
 		var derBlock *pem.Block
 		derBlock, raw = pem.Decode(raw)
@@ -227,19 +244,6 @@ func tlsCANeedsRenewal(log zerolog.Logger, cert string, spec api.TLSSpec) (bool,
 					Msg("TLS CA certificate renewal needed")
 				return true, "CA Certificate about to expire"
 			}
-			// Check alternate names against spec
-			dnsNames, ipAddresses, emailAddress, err := spec.GetParsedAltNames()
-			if err == nil {
-				if !containsAll(cert.DNSNames, dnsNames) {
-					return true, "Some alternate DNS names are missing"
-				}
-				if !containsAll(ipsToStringSlice(cert.IPAddresses), ipAddresses) {
-					return true, "Some alternate IP addresses are missing"
-				}
-				if !containsAll(cert.EmailAddresses, emailAddress) {
-					return true, "Some alternate email addresses are missing"
-				}
-			}
 		}
 	}
 	return false, ""

pkg/deployment/resources/certificates_tls.go

@@ -45,20 +45,13 @@ const (
 // specified in the given spec.
 func createTLSCACertificate(log zerolog.Logger, cli v1.CoreV1Interface, spec api.TLSSpec, deploymentName, namespace string, ownerRef *metav1.OwnerReference) error {
 	log = log.With().Str("secret", spec.GetCASecretName()).Logger()
-	dnsNames, ipAddresses, emailAddress, err := spec.GetParsedAltNames()
-	if err != nil {
-		log.Debug().Err(err).Msg("Failed to get alternate names")
-		return maskAny(err)
-	}
 
 	options := certificates.CreateCertificateOptions{
-		CommonName:     fmt.Sprintf("%s Root Certificate", deploymentName),
-		Hosts:          append(dnsNames, ipAddresses...),
-		EmailAddresses: emailAddress,
-		ValidFrom:      time.Now(),
-		ValidFor:       caTTL,
-		IsCA:           true,
-		ECDSACurve:     tlsECDSACurve,
+		CommonName: fmt.Sprintf("%s Root Certificate", deploymentName),
+		ValidFrom:  time.Now(),
+		ValidFor:   caTTL,
+		IsCA:       true,
+		ECDSACurve: tlsECDSACurve,
 	}
 	cert, priv, err := certificates.CreateCertificate(options, nil)
 	if err != nil {

pkg/replication/sync_client.go

@@ -113,7 +113,7 @@ func (dr *DeploymentReplication) createArangoSyncEndpoint(epSpec api.EndpointSpe
 		dnsName := k8sutil.CreateSyncMasterClientServiceDNSName(depl)
 		return client.Endpoint{"https://" + net.JoinHostPort(dnsName, strconv.Itoa(k8sutil.ArangoSyncMasterPort))}, nil
 	}
-	return client.Endpoint(epSpec.Endpoint), nil
+	return client.Endpoint(epSpec.MasterEndpoint), nil
 }
 
 // createArangoSyncTLSAuthentication creates the authentication needed to authenticate