Add local K8S remote cluster support #2543

barkbay · 2020-02-11T16:44:52Z

What this PR does:

It adds the remoteClusters field (as usual naming can be discussed) to the Elasticsearch schema:

apiVersion: elasticsearch.k8s.elastic.co/v1
kind: Elasticsearch
metadata:
  name: cluster-1
  namespace: ns1
spec:
  version: 7.5.2
  remoteClusters:
  - name: ns2-cluster-2
    elasticsearchRef:
      name: cluster-2
      namespace: ns2
    #config:
    #  transport.ping_schedule: 30s
    #  transport.compress: true
    #  skip_unavailable: true

Adds a new controller to copy certificate authorities between clusters. Each remote certificate is copied into the namespace of the peer cluster.
The elasticsearch controller then concatenates all the remote certificates into an new Secret called <cluster-name>-es-remote-ca

Here is a example of the new Secrets:

$ k get es,secrets -n ns1                    
NAME                                                   HEALTH   NODES   VERSION   PHASE   AGE
elasticsearch.elasticsearch.k8s.elastic.co/cluster-1   green    3       7.5.2     Ready   2m

NAME                                          TYPE                                  DATA   AGE
[...]
secret/cluster-1-es-remote-ca                 Opaque                                1      2m21s
secret/cluster-1-ns2-cluster-2-es-remote-ca   Opaque                                1      2m18s

$ k get es,secrets -n ns2
NAME                                                   HEALTH   NODES   VERSION   PHASE   AGE
elasticsearch.elasticsearch.k8s.elastic.co/cluster-2   green    3       7.5.2     Ready   1m

NAME                                          TYPE                                  DATA   AGE
[...]
secret/cluster-2-es-remote-ca                 Opaque                                1      81s
secret/cluster-2-ns1-cluster-1-es-remote-ca   Opaque                                1      80s

The elasticsearch controller also does the right API calls to setup the remote cluster in the settings. The last configuration is stored as an annotation in the Elasticsearch resource in order to avoid to do unnecessary calls:

apiVersion: v1
items:
- apiVersion: elasticsearch.k8s.elastic.co/v1
  kind: Elasticsearch
  metadata:
    annotations:
      elasticsearch.k8s.elastic.co/remote-clusters: '[{"name":"ns2-cluster-2","configHash":"3795207740"}]'

This PR also creates a transport service:

$ k get svc -n ns1
NAME                     TYPE           CLUSTER-IP    EXTERNAL-IP      PORT(S)          AGE
cluster-1-es-default     ClusterIP      None          <none>           <none>           10m
cluster-1-es-http        LoadBalancer   10.28.XX.XX   35.195.XX.XX     9200:32076/TCP   10m
cluster-1-es-transport   ClusterIP      None          <none>           9300/TCP         10m

Side notes:

I have added some spans for performance monitoring but I still need to check if they are relevant.
Don't forget to start a trial or use an Enterprise license if you want to do some tests

barkbay · 2020-02-17T07:26:36Z

jenkins test this please

barkbay · 2020-02-18T07:18:48Z

jenkins test this please

pebrc

Nice work! I just took a quick look.

pebrc · 2020-02-12T10:45:43Z

pkg/apis/elasticsearch/v1/elasticsearch_types.go

+
+// RemoteClusters defines some remote Elasticsearch clusters.
+type RemoteClusters struct {
+	K8sLocal []K8sLocalRemoteCluster `json:"k8sLocal,omitempty"`


I know we discussed this before but I am still not 💯 on the naming here, mostly gut feeling/aesthetics reasons, so apologies if I don't have a better suggestion right now.

I'm trying to find alternatives. If we look at eg. VolumeSources, there's a flat list of optional sources expressed in a single struct. Maybe we could remove one layer here?

type RemoteCluster struct { Config RemoteClusterConfig `json:"config,omitempty"` ElasticsearchRef *commonv1.ObjectSelector ElasticCloudRef *ElasticCloudRef NetworkRef *NetworkRef }

spec: remoteClusters: - elasticsearchRef: name: foo namespace: bar config: transport.ping_schedule: 30s - elasticsearchRef: name: bar namespace: baz config: transport.ping_schedule: 10s - elasticCloudRef: clusterId: 1234 config: transport.ping_schedule: 10s - networkRef: seeds: 1.2.3.4:9300 config: transport.ping_schedule: 10s

ElasticsearchRef is implicitly the "local" ref. If later on we want to extend it to cross-k8s cluster boundaries we can either extend the existing ElasticsearchRef with more fields (eg. elasticsearchref.k8sCluster), either introduce a new type (federatedElasticsearchRef).

pkg/apis/common/v1/common.go

pkg/apis/elasticsearch/v1/elasticsearch_types.go

pkg/controller/elasticsearch/remotecluster/elasticsearch.go

pkg/controller/elasticsearch/remotecluster/remoteca/controller.go

pkg/controller/elasticsearch/remotecluster/remoteca/labels.go

pkg/controller/elasticsearch/remotecluster/remoteca/watches.go

pkg/controller/elasticsearch/services/services.go

pebrc · 2020-02-21T09:04:28Z

pkg/controller/elasticsearch/services/services.go

+			Name:      TransportServiceName(es.Name),
+			Labels:    label.NewLabels(nsn),
+		},
+		Spec: corev1.ServiceSpec{


Should we allow users to control the spec like we do for the HTTP service? For remoting outside of the k8s cluster users probably want this to be of type LoadBalancer for example.

It makes sense. That said this PR is already pretty large and is targeting clusters inside a same k8s cluster. I would do that in a dedicated PR, I can create an issue to track this effort.

sebgl

This is great work!

There is a lot of code so I have a lot of comments, sorry 😄

One concern I have is that I feel lost when reading the association controller reconciliation code. It's hard to figure out what local, what's remote, how the local CA also needs to be remote, etc. I don't know if we can make that clearer? Maybe through smaller functions and more comments about the high-level things we want to do.

config/crds/all-crds.yaml

sebgl · 2020-02-21T13:07:01Z

pkg/apis/elasticsearch/v1/elasticsearch_types.go

+
+// RemoteClusters defines some remote Elasticsearch clusters.
+type RemoteClusters struct {
+	K8sLocal []K8sLocalRemoteCluster `json:"k8sLocal,omitempty"`


I'm trying to find alternatives. If we look at eg. VolumeSources, there's a flat list of optional sources expressed in a single struct. Maybe we could remove one layer here?

type RemoteCluster struct { Config RemoteClusterConfig `json:"config,omitempty"` ElasticsearchRef *commonv1.ObjectSelector ElasticCloudRef *ElasticCloudRef NetworkRef *NetworkRef }

spec: remoteClusters: - elasticsearchRef: name: foo namespace: bar config: transport.ping_schedule: 30s - elasticsearchRef: name: bar namespace: baz config: transport.ping_schedule: 10s - elasticCloudRef: clusterId: 1234 config: transport.ping_schedule: 10s - networkRef: seeds: 1.2.3.4:9300 config: transport.ping_schedule: 10s

ElasticsearchRef is implicitly the "local" ref. If later on we want to extend it to cross-k8s cluster boundaries we can either extend the existing ElasticsearchRef with more fields (eg. elasticsearchref.k8sCluster), either introduce a new type (federatedElasticsearchRef).

pkg/controller/elasticsearch/certificates/remoteca/reconcile.go

pkg/controller/elasticsearch/remotecluster/remoteca/labels.go

sebgl · 2020-02-21T14:46:29Z

pkg/controller/elasticsearch/remotecluster/remoteca/labels.go

+
+const (
+	// RemoteClusterNamespaceLabelName used to represent the namespace of the RemoteCluster in a TrustRelationship.
+	RemoteClusterNamespaceLabelName = "remotecluster.k8s.elastic.co/namespace"


this should probably have elasticsearch.k8s.elastic.co in the name.
Can we do elasticsearch.k8s.elastic.co/remote-cluster: {"namespace": "ns", "name": "cluster-name"}?

I'm not sure we can put some json in a label (also I'm not sure it's easy to create a label selector against it)

pkg/controller/elasticsearch/remotecluster/remoteca/labels.go

pkg/controller/elasticsearch/services/services.go

barkbay · 2020-02-25T13:26:51Z

I have updated the PR description with the new spec format, ICYMI here is an example:

apiVersion: elasticsearch.k8s.elastic.co/v1
kind: Elasticsearch
metadata:
  name: cluster-1
  namespace: ns1
spec:
  version: 7.5.2
  remoteClusters:
  - name: ns2-cluster-2
    elasticsearchRef:
      name: cluster-2
      namespace: ns2
  - elasticsearchRef:
      name: cluster-3
      namespace: ns3
    name: ns3-cluster-3
  nodeSets:
    - name: default
      count: 3
      config:
        node.store.allow_mmap: false

barkbay · 2020-02-26T08:27:00Z

jenkins test this please

barkbay · 2020-02-26T09:03:58Z

@sebgl I split the remote CA reconciler and add some comments, I also tried to address most of your comments.

pebrc

LGTM, this is really impressive work. I think I am 👍 on merging and if necessary follow up with two PRs:

making the transport service user configurable
if we decide to use file based config for remote clusters to switch to that

Leaving the final ✅ to @sebgl who did the more in-depth review

pkg/apis/elasticsearch/v1/elasticsearch_types.go

pkg/controller/elasticsearch/remotecluster/elasticsearch.go

pkg/controller/elasticsearch/remotecluster/remoteca/controller.go

pebrc · 2020-02-26T10:10:17Z

pkg/controller/remoteca/remoteca_controller.go

+)
+
+// Add creates a new RemoteCa Controller and adds it to the manager with default RBAC.
+func Add(mgr manager.Manager, accessReviewer rbac.AccessReviewer, params operator.Parameters) error {


Any particular reason why you did not move everything from elasticsearch/remotecluster/remoteca into this package?

At first it was only in elasticsearch/remotecluster/remoteca The reason is that this controller only makes sense in the context of Elasticsearch.

I moved a part of it to make it "visible", I'm not sure I would go further because this controller is dependent on the Elasticsearch one, but I'm happy to move everything if you think it makes sense.

pkg/apis/elasticsearch/v1/name.go

pkg/controller/elasticsearch/certificates/remoteca/reconcile.go

pkg/controller/elasticsearch/driver/driver.go

pkg/controller/elasticsearch/remotecluster/remoteca/controller.go

pkg/controller/elasticsearch/remotecluster/remoteca/secret.go

sebgl · 2020-02-27T17:30:42Z

Thanks for making the changes, I understood the code much better now regarding what's remote vs. local and why we reconcile 2 secrets per association.

Most of my comments above are nitpicks. Except this one that I'd like to understand. I think we are reconciling the same secret multiple times in different reconciliations targeting different clusters and we may not have to?

sebgl

LGTM. Great work!

barkbay · 2020-03-02T13:35:54Z

@pebrc @sebgl It was a large PR, thank you both for your thorough code review ! ❤️

barkbay added 3 commits February 11, 2020 17:38

Add local K8S remote cluster support

d440787

make linter happy

2891137

Regenerate API doc

2570191

barkbay added >feature Adds or discusses adding a feature to the product v1.1.0 labels Feb 12, 2020

barkbay added 9 commits February 12, 2020 10:55

Merge remote-tracking branch 'origin/master' into remote-clusters-vII

4b5d5df

Fix some typos

3acfd93

Revert NOTICE.txt

b2507ce

Merge with origin/master

e9b585a

Handle remote clusters with no namespace set in the spec

9e655b1

Sort remote CA

117f284

Remove TODO

af90aac

Add an APM span

2d896b6

Refactor rbac package

523a742

barkbay marked this pull request as ready for review February 17, 2020 07:26

barkbay added 2 commits February 17, 2020 09:50

Improve UpdateRemoteCluster unit tests

3959c69

Do a license check for API calls

56f5e8e

pebrc reviewed Feb 19, 2020

View reviewed changes

barkbay added 3 commits February 21, 2020 08:52

fix typos and update from code review

73837c3

Merge with origin/master

fac1ca6

Update remote clusters in one API call

c3a9bf6

pebrc reviewed Feb 21, 2020

View reviewed changes

pebrc mentioned this pull request Feb 21, 2020

Document how to set up remote clusters across k8s boundaries #2593

Merged

sebgl reviewed Feb 21, 2020

View reviewed changes

barkbay added 4 commits February 24, 2020 11:21

Flatten remote clusters spec

f9b2fb0

Store the cfg map in the annotation

3da409c

Let the user specify the remote cluster name

5422aca

Update api doc + add ConfigHash receiver

190a547

barkbay added 6 commits February 25, 2020 09:32

Improve unit tests

64ae337

Move naming function

bcf2c1b

Generate an event if enterprise features are disabled

afb1468

refactoring, add some comments

a033f1e

typos

26bfb20

Update event

0d36a0b

barkbay added 3 commits February 25, 2020 14:56

Add comment about remote cluster name

1f12029

update comment

73fdc26

Merge remote-tracking branch 'origin/master' into remote-clusters-vII

3630138

barkbay requested review from pebrc and sebgl February 26, 2020 09:04

pebrc reviewed Feb 26, 2020

View reviewed changes

barkbay added 2 commits February 27, 2020 08:30

update from code review

be0054c

Fix typo

17e343b

sebgl reviewed Feb 27, 2020

View reviewed changes

barkbay added 2 commits February 28, 2020 08:47

Rename UpdateSettings to UpdateRemoteClusterSettings

5203b5f

update from code review

bcd2da6

sebgl approved these changes Mar 2, 2020

View reviewed changes

barkbay added 2 commits March 2, 2020 13:55

Merge remote-tracking branch 'origin/master' into remote-clusters-vII

bbbe927

Update api doc

5a313c8

barkbay merged commit eefc8af into elastic:master Mar 2, 2020

This was referenced Mar 2, 2020

Users should be able to customize the transport service #2644

Closed

File based configuration for remote clusters #2645

Closed

Add e2e tests to test the "local" remote cluster feature #2647

Closed

idanmo mentioned this pull request Nov 25, 2020

Remote CA cert is created empty when remote clusters are not configured for an ES cluster #3881

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add local K8S remote cluster support #2543

Add local K8S remote cluster support #2543

barkbay commented Feb 11, 2020 •

edited

Loading

barkbay commented Feb 17, 2020

barkbay commented Feb 18, 2020

pebrc left a comment

pebrc Feb 12, 2020

sebgl Feb 21, 2020

pebrc Feb 21, 2020

barkbay Feb 21, 2020

pebrc Feb 21, 2020

sebgl left a comment

sebgl Feb 21, 2020

sebgl Feb 21, 2020

barkbay Feb 25, 2020

barkbay commented Feb 25, 2020

barkbay commented Feb 26, 2020

barkbay commented Feb 26, 2020

pebrc left a comment

pebrc Feb 26, 2020

barkbay Feb 27, 2020

sebgl commented Feb 27, 2020 •

edited

Loading

sebgl left a comment

barkbay commented Mar 2, 2020

Add local K8S remote cluster support #2543

Add local K8S remote cluster support #2543

Conversation

barkbay commented Feb 11, 2020 • edited Loading

barkbay commented Feb 17, 2020

barkbay commented Feb 18, 2020

pebrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebgl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barkbay commented Feb 25, 2020

barkbay commented Feb 26, 2020

barkbay commented Feb 26, 2020

pebrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebgl commented Feb 27, 2020 • edited Loading

sebgl left a comment

Choose a reason for hiding this comment

barkbay commented Mar 2, 2020

barkbay commented Feb 11, 2020 •

edited

Loading

sebgl commented Feb 27, 2020 •

edited

Loading