.*: TLS and Etcd v3 support #195

FlorinPeter · 2020-11-10T11:23:39Z

First of all thx for this awesome etcd operator ❤️ and sorry for this huge update.
I started first to add tls support and ended later on by adding support vor etcd v3 api that allows to run the latest etcd v3.4.x versions.
My focus was on keeping the changes as minimum as possible and to maintain compatibility with etcd v2.

Changes

Added tls support for peer and client
Added etcd v3 support
Improved tests

Verification

Deploy a cluster like:

apiVersion: etcd.improbable.io/v1alpha1
kind: EtcdCluster
metadata:
  name: my-cluster
spec:
  replicas: 3
  version: 3.4.13
  tls:
    enabled: true
  storage:
    volumeClaimTemplate:
      storageClassName: standard
      resources:
        requests:
          storage: 1Mi
  podTemplate:
    resources:
      requests:
        cpu: 200m
        memory: 200Mi
      limits:
        cpu: 200m
        memory: 200Mi
    affinity:
      podAntiAffinity:
        preferredDuringSchedulingIgnoredDuringExecution:
          - weight: 100
            podAffinityTerm:
              labelSelector:
                matchExpressions:
                  - key: etcd.improbable.io/cluster-name
                    operator: In
                    values:
                      - my-cluster
              topologyKey: kubernetes.io/hostname

improbable-prow-robot · 2020-11-10T11:24:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cheahjs

Hey, thanks for the pull request. Sorry that I haven't been able to review this in a while.

The PR generally looks good with some comments.

Some comments for myself:

etcd v3.4 disables the v2 API by default. We were using it for the version and membership API. This PR adds support for the v3 API instead. I don't remember why we opted to use the v2 API (when the operator only supports v3 and above), but note to self we could probably simplify this in the future and remove the v2 codepath.
- There were references to v2 structs, so a shim was made to provide compatibility for other bits of the codebase.
There's a drive-by pprof option for the controller.

cheahjs · 2020-12-08T14:24:41Z

controllers/constants.go

@@ -3,7 +3,8 @@ package controllers
 const (
 	etcdClientPort          = 2379
 	etcdPeerPort            = 2380
-	etcdDataMountPath       = "/var/lib/etcd"
+	EtcdDataMountPath       = "/var/lib/etcd"


Does this need to be an exported variable?

cheahjs · 2020-12-08T15:02:34Z

internal/etcd/etcd.go


-	etcdclient "go.etcd.io/etcd/client"
-	"go.etcd.io/etcd/version"
+	"github.com/blang/semver"


small nit, there's an existing dependency on github.com/coreos/go-semver which should be used instead of another semver lib.

cheahjs · 2020-12-08T15:05:51Z

vendor/cloud.google.com/go/LICENSE

@@ -0,0 +1,202 @@
+


We don't currently vendor dependencies in this repo, is there a need for them?

FlorinPeter · 2020-12-18T06:57:19Z

@cheahjs thx for having a look on this PR.
I will take care of your comments around mid of January.

cheahjs · 2021-02-15T10:35:08Z

@FlorinPeter hey, any updates?

FlorinPeter · 2021-02-16T15:43:04Z

@cheahjs worked on some stuff, I hope to make progress in the next week, sorry for the delay.

gottwald · 2021-05-05T15:27:15Z

@FlorinPeter @cheahjs Any updates on this? Anything I could maybe help you with?

gottwald · 2021-05-12T08:55:39Z

controllers/etcdcluster_controller.go

+func clientCertForCluster(cluster *etcdv1alpha1.EtcdCluster, caCert, caKey []byte) (*v1.Secret, error) {
+	name := clientSecretName(cluster.Name, cluster.Namespace)
+
+	clientCert, clientKey, err := tls.Issue([]string{name.Name}, caCert, caKey)


AFAIU this creates a client cert with the DNS name clusterName-client.
Shouldn't there be a svc with the same name going along with it? How else would you connect to the cluster as a client and have a correct name verification?

@FlorinPeter @cheahjs Any updates on this? Anything I could maybe help you with?

Sorry for delaying this PR, there are to many topics on my desk, my plan was to find a couple of hours next week to go over the review.

You might be thinking of the peer's client certificate which requires that the Subject Alternative Name (SAN) includes DNS hostnames and/or IPs that match that of the peer. That certificate is generated in the EtcdPeer reconciler over here: https://github.com/improbable-eng/etcd-cluster-operator/pull/195/files#diff-2e9c68c9fc75a29d6beb2fe742c18d4b97a48e76796ccea4da15e663c3cfe6a4R659-R681

This bit of code is generating the client certificate for etcd clients, and the Common Name (CN) is used for logging in as the username in the CN (in this case, hardcoded to be Etcd Cluster Operator as per the Subject field).

Reference: https://etcd.io/docs/v3.4/op-guide/security/

gottwald · 2021-07-21T19:08:57Z

@FlorinPeter Would you mind if I carry this PR further?

FlorinPeter · 2021-10-06T10:55:08Z

@FlorinPeter Would you mind if I carry this PR further?

@gottwald sure please go ahead as I have no time to work on that at the moment.

FlorinPeter force-pushed the upstream-etcd-update branch 4 times, most recently from 32da207 to 6b8633d Compare November 10, 2020 19:43

Added TLS and Etcd v3 support

99687be

FlorinPeter force-pushed the upstream-etcd-update branch from 6b8633d to 99687be Compare November 11, 2020 09:13

cheahjs reviewed Dec 8, 2020

View reviewed changes

cheahjs self-assigned this Dec 8, 2020

cheahjs linked an issue Dec 8, 2020 that may be closed by this pull request

TLS between cluster peers #100

Open

gottwald reviewed May 12, 2021

View reviewed changes

FlorinPeter closed this Nov 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.*: TLS and Etcd v3 support #195

.*: TLS and Etcd v3 support #195

FlorinPeter commented Nov 10, 2020

improbable-prow-robot commented Nov 10, 2020

cheahjs left a comment

cheahjs Dec 8, 2020

cheahjs Dec 8, 2020

cheahjs Dec 8, 2020

FlorinPeter commented Dec 18, 2020

cheahjs commented Feb 15, 2021

FlorinPeter commented Feb 16, 2021

gottwald commented May 5, 2021

gottwald May 12, 2021

FlorinPeter May 12, 2021

cheahjs May 12, 2021 •

edited

Loading

gottwald commented Jul 21, 2021

FlorinPeter commented Oct 6, 2021

.*: TLS and Etcd v3 support #195

.*: TLS and Etcd v3 support #195

Conversation

FlorinPeter commented Nov 10, 2020

Changes

Verification

improbable-prow-robot commented Nov 10, 2020

cheahjs left a comment

Choose a reason for hiding this comment

cheahjs Dec 8, 2020

Choose a reason for hiding this comment

cheahjs Dec 8, 2020

Choose a reason for hiding this comment

cheahjs Dec 8, 2020

Choose a reason for hiding this comment

FlorinPeter commented Dec 18, 2020

cheahjs commented Feb 15, 2021

FlorinPeter commented Feb 16, 2021

gottwald commented May 5, 2021

gottwald May 12, 2021

Choose a reason for hiding this comment

FlorinPeter May 12, 2021

Choose a reason for hiding this comment

cheahjs May 12, 2021 • edited Loading

Choose a reason for hiding this comment

gottwald commented Jul 21, 2021

FlorinPeter commented Oct 6, 2021

cheahjs May 12, 2021 •

edited

Loading