Use the public transport CA as remote CA if the remote CA list is empty #3993
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
idanmo
force-pushed
the
empty-remote-ca-fix
branch
from
6f88299
to
223a347
Compare
pebrc
approved these changes
Dec 2, 2020
There was a problem hiding this comment.
LGTM. I did a quick test and seems to work. The only thing I noticed is that the remote connection is initially disconnected because the CA certificate was not available yet when the remote cluster was set up in ES via the API, but I believe the same could happen before if the CA propagation was too slow.
| @@ -101,14 +104,24 @@ func TestReconcile(t *testing.T) { | |||
| Data: map[string][]byte{certificates.CAFileName: []byte("cert2\n")}, | |||
| }, | |||
| }, | |||
| ca: nil, | |||
There was a problem hiding this comment.
I think we should pass in the testCA here and on L64 as well to make sure it does not get included when there are remote CA certs.
There was a problem hiding this comment.
Yes, that makes sense.
sebgl
reviewed
Dec 2, 2020
pkg/controller/elasticsearch/certificates/remoteca/reconcile.go
Outdated
Show resolved
Hide resolved
sebgl
approved these changes
Dec 3, 2020
| @@ -36,6 +36,7 @@ func Labels(esName string) client.MatchingLabels { | |||
| func Reconcile( | |||
| c k8s.Client, | |||
| es esv1.Elasticsearch, | |||
| ca certificates.CA, | |||
There was a problem hiding this comment.
nit: transportCA would make things even more explicit
| @@ -24,7 +24,9 @@ func TestReconcile(t *testing.T) { | |||
| type args struct { | |||
| es esv1.Elasticsearch | |||
| secrets []runtime.Object | |||
| ca *certificates.CA | |||
|
run full pr build |
idanmo
added a commit
to idanmo/cloud-on-k8s
that referenced
this pull request
Dec 3, 2020
…ty (elastic#3993) * Addresses an issue where remote transport ca.crt is empty if remote clusters are not set for an ES cluster * Preserves the behavior where it is not needed to restart ES when configuring remote clusters, post the initial deployment of the cluster
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If remote clusters are not set for an ES cluster, the remote CA
ca.crtmounted on the ES container is empty, which causes the_ssl/certificatesES REST API call to fail. In this PR, if remote clusters are not set for an ES cluster, the remote CA is populated with the public transport CA as a "dummy" CA.This fix preserves the behavior where adding/setting remote clusters for ES does not require restarting the ES cluster.
Fixes #3881