Support a globally shared CA #5539

pebrc · 2022-03-30T12:56:46Z

This adds support for a new operator parameter that allows users to specify a shared CA that will be used for all managed resources. If users opted to specify separate CAs per resource those configurations will be left untouched, however any resources that are using the default per-resource self-signed CA will use the shared CA.

The CA is specified as a directory via an operator param, currently --ca. The operator expects two files to be present in this directory tls.crt and tls.key. I am thinking of supporting ca.crt and ca.key as well to be consistent with the existing per-resource custom CA feature.

The operator watches the directory and restarts if it detects changes. This is following the behaviour we have for general operator configuration changes and simplifies the implementation IMO considerably.

I am still trying to figure out a testing strategy (unit/integaition tests but maybe also an e2e test) but wanted to raise the PR already in case there is early feedback.

pebrc · 2022-03-31T09:53:54Z

I am running into trouble in the e2e tests with the fsnotify change detection in the e2e tests. I know we observed this before. I might need to switch to simple polling instead for both the CA and the operator configuration.

pebrc · 2022-04-04T12:15:47Z

I am not 💯 sure about supporting the two naming conventions ca.* and tls.* (the latter being the standard in K8s TLS secrets) and therefore happy to back out that last commit 5da3b3e

barkbay

I only did a first review, but looks good so far 👍
Thanks for the FileWatcher, I'm pretty sure it'll make config file detection more reliable.

pkg/controller/common/operator/parameters.go

pkg/utils/fs/watcher.go

barkbay · 2022-04-15T06:33:07Z

pkg/utils/fs/watcher_integration_test.go

+	requireEventEquals := func(c chan []string, expected []string, timeout time.Duration) {
+		select {
+		case event := <-c:
+			require.Equal(t, expected, event)


I don't think we need a strict equality here, my gut feeling is that it can be a cause of flaky tests

Suggested change

require.Equal(t, expected, event)

require.ElementsMatch(t, expected, event)

pkg/utils/fs/watcher_integration_test.go

pkg/utils/fs/watcher.go

pkg/controller/common/operator/flags.go

test/e2e/ca_test.go

cmd/manager/main.go

pkg/controller/common/certificates/ca_reconcile.go

barkbay

I have run a few other tests, including running the E2E test, or enabling/disabling the feature on existing deployments. It seems to work as expected 👍 During these tests I only noticed that the *-ca-internal Secrets are not removed, while my understanding is that they are not required anymore. However it's definitely not a big deal since it's completely harmless.

I'll take the time to do another review later.

test/e2e/ca_test.go

pkg/controller/common/certificates/ca_reconcile.go

pkg/controller/common/certificates/ca_reconcile_integration_test.go

pkg/controller/elasticsearch/certificates/transport/ca.go

pkg/utils/fs/watcher.go

test/e2e/test/elasticsearch/builder.go

pebrc · 2022-04-19T13:53:04Z

Jenkins test this please

Makefile:63: recipe for target 'ci-build-image' failed

cmd/manager/main.go

test/e2e/global_ca_test.go

pebrc · 2022-04-19T17:58:32Z

Unfortunately it looks like the file watcher integration test I added is flaky. Trying to improve on the current implementation.

pebrc · 2022-04-19T19:11:48Z

Sometimes mtime changes more than once on writes. I am not entirely sure why given the test case uses probably a single write syscall. But then I again I am not an expert on how these things work on unix'ish OSs.

/var/folders/1x/dzn9_byd2sn3cpn48j3d14m40000gn/T/file-watcher-test105416277/file2 prev 0001-01-01 00:00:00 +0000 UTC now 2022-04-19 20:52:24.299815768 +0200 CEST
/var/folders/1x/dzn9_byd2sn3cpn48j3d14m40000gn/T/file-watcher-test105416277/file1 prev 0001-01-01 00:00:00 +0000 UTC now 2022-04-19 20:52:24.300063142 +0200 CEST
/var/folders/1x/dzn9_byd2sn3cpn48j3d14m40000gn/T/file-watcher-test105416277/file3 prev 0001-01-01 00:00:00 +0000 UTC now 2022-04-19 20:52:24.300206579 +0200 CEST
/var/folders/1x/dzn9_byd2sn3cpn48j3d14m40000gn/T/file-watcher-test105416277/file4 prev 0001-01-01 00:00:00 +0000 UTC now 2022-04-19 20:52:24.302395466 +0200 CEST
/var/folders/1x/dzn9_byd2sn3cpn48j3d14m40000gn/T/file-watcher-test105416277/file4 prev 2022-04-19 20:52:24.302395466 +0200 CEST now 2022-04-19 20:52:24.302612857 +0200 CEST
/var/folders/1x/dzn9_byd2sn3cpn48j3d14m40000gn/T/file-watcher-test105416277/file2 prev 2022-04-19 20:52:24.299815768 +0200 CEST now 2022-04-19 20:52:24.302811553 +0200 CEST

See how file4 changes twice in the output above.

I modified the test's assumptions to allow for additional events on the simulated writes.

…ut it actually found a bug

barkbay · 2022-04-20T07:17:21Z

I am not entirely sure why given the test case uses probably a single write syscall.

I think that there are always at least 2 underlying syscall (using os.WriteFile):

A first one to create an empty file, or truncate the content if it already exists: mtime is changed and file size is 0
A second one to write the data to the empty file , mtime is changed once again

Running the WriteFile in debug mode, step by step, and using stat at the same time we can see that mtime is changing at least twice:

func WriteFile(name string, data []byte, perm FileMode) error {
	f, err := OpenFile(name, O_WRONLY|O_CREATE|O_TRUNC, perm) // -- File is created or truncated, `stat -f "%Sm" file1 = Apr 20 08:56:10 2022`
	if err != nil {...}
	_, err = f.Write(data) // -- data is written (not sure if it is synced though) -- `stat -f "%Sm" file1 = Apr 20 08:56:16 2022`
	if err1 := f.Close(); err1 != nil && err == nil { // -- data is flushed on disk?
		err = err1
	}
	return err
}

Also if the file is mapped to several pages in memory, I'm not sure there is a guarantee that all the pages are synced at the same time and mtime is changed only once. I don't know if this article is still relevant, but it is still interesting as it describes a behavior that has existed:

So a quick operation to make a page writable turns into a heavyweight filesystem operation, and it happens every time the application attempts to write to a clean page. If the application writes large numbers of pages that have been mapped into memory, the result will be a painful slowdown. And most of that effort is wasted; the timestamp updates overwrite each other, so only the last one will persist for any useful period of time.

pebrc · 2022-04-20T09:06:55Z

A first one to create an empty file, or truncate the content if it already exists: mtime is changed and file size is 0

Of course! I clearly didn't think this through. Thanks for the great explanation!

Also if the file is mapped to several pages in memory, I'm not sure there is a guarantee that all the pages are synced at the same time and mtime is changed only once.

Do you mean this in the sense that the test might still be flaky and we could see yet another event coming from another mtime update?

barkbay

This looks great, I just left some nitpicks 👍 🚀

pkg/controller/common/operator/flags.go

cmd/manager/main.go

pkg/controller/common/certificates/ca_reconcile.go

pkg/controller/common/certificates/ca_reconcile_integration_test.go

pkg/utils/fs/watcher_integration_test.go

barkbay · 2022-04-20T09:25:32Z

Do you mean this in the sense that the test might still be flaky and we could see yet another event coming from another mtime update?

No, such small files are very unlikely to be spread across multiple pages (min. page size is 4KB on Linux IIUC).

ejsmith · 2022-06-24T02:26:44Z

Is it possible to use this feature to get ES, Kibana and APM to use certs that are signed by a trusted authority like Let's Encrypt so that my app trusts the cert without having to configure my app to add a trusted self-signed CA?

jmp601 · 2022-07-08T20:09:42Z

Is it possible to use this feature to get ES, Kibana and APM to use certs that are signed by a trusted authority like Let's Encrypt so that my app trusts the cert without having to configure my app to add a trusted self-signed CA?

We are an elastic enterprise customer and have the same ask. I have a hard time configuring ECK with e2e with Let's encrypt certs.

naemono · 2022-07-09T13:43:16Z

We have some documentation around this feature:

https://www.elastic.co/guide/en/cloud-on-k8s/master/k8s-tls-certificates.html#k8s-setting-up-your-own-certificate

and here an example for cert manager (you can replace the self-signed issue of course with a publicly trusted one like Let's Encrypt)
https://www.elastic.co/guide/en/cloud-on-k8s/master/k8s-custom-http-certificate.html#k8s_custom_self_signed_certificate_using_cert_manager

We also have an issue tracking updating our documentation to be more clear #4812

ejsmith · 2022-07-09T17:21:15Z

Would love to see a tutorial that sets up cert-manager with let's encrypt and then creates an ES cluster with a trusted SSL feet that apps can use without having to do anything as well as exposing a public Kibana and APM instance that an app can use to send front end APM data to.

This adds support for a new operator parameter that allows users to specify a shared CA that will be used for all managed resources. If users opted to specify separate CAs per resource those configurations will be left untouched, however any resources that are using the default per-resource self-signed CA will use the shared CA. The CA is specified as a directory via an operator param, --ca-dir. The operator expects two files to be present in this directory tls.crt and tls.key -- ca.crt and ca.key are supported as well to be consistent with the existing per-resource custom CA feature. The operator watches the directory and restarts if it detects changes.

jbouse · 2023-10-16T19:00:32Z

Is this still available in v2.9.0? I'm unable to find mention of it in the documentation nor in the Helm chart for the operator deployment.

pebrc added 5 commits March 29, 2022 11:19

Initial implemenation of global CA, no tests

da9455e

follow symlinks

bdb692f

rework watcher logic to be CA aware and check mtime

54729cb

additional logging

8b23c7f

add integration test for CA file parsing

d5711af

botelastic bot added the triage label Mar 30, 2022

pebrc added the >feature Adds or discusses adding a feature to the product label Mar 30, 2022

botelastic bot removed the triage label Mar 30, 2022

pebrc added 2 commits March 31, 2022 11:32

e2e test

a16f23b

linting

eeb72b6

pebrc added 4 commits March 31, 2022 13:58

avoid unnecessary pointer in unit test

f85b517

integration test: avoid cross-device links when running in container

53c7035

Remove tempDir

effa067

replace fsnotify with polling

befec6a

pebrc force-pushed the shared-ca branch from 788e1a9 to 171e773 Compare April 1, 2022 17:58

Godoc, lint

49064df

pebrc force-pushed the shared-ca branch 2 times, most recently from b7d3852 to b505883 Compare April 4, 2022 12:14

pebrc marked this pull request as ready for review April 4, 2022 12:14

Support ca* naming convention

5da3b3e

pebrc force-pushed the shared-ca branch from b505883 to 5da3b3e Compare April 4, 2022 13:55

pebrc added the v2.3.0 label Apr 14, 2022

barkbay reviewed Apr 15, 2022

View reviewed changes

test/e2e/ca_test.go Outdated Show resolved Hide resolved

test/e2e/ca_test.go Outdated Show resolved Hide resolved

naemono reviewed Apr 15, 2022

View reviewed changes

review feeback

82c8717

pebrc force-pushed the shared-ca branch from 6d82c70 to 82c8717 Compare April 16, 2022 08:45

consistent naming: global CA

18662fe

pebrc requested review from barkbay and naemono April 19, 2022 15:10

naemono reviewed Apr 19, 2022

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

test/e2e/global_ca_test.go Outdated Show resolved Hide resolved

consistent naming: global CA, missed a few

0ee6cd0

workaround multiple mtime changes in integration test

8680df0

looks like the linter changed its mind quite late in the game here, b…

a212bc1

…ut it actually found a bug

barkbay approved these changes Apr 20, 2022

View reviewed changes

naemono approved these changes Apr 20, 2022

View reviewed changes

pebrc added 3 commits April 20, 2022 15:16

more review feedback, wording, comments and test setup

edbf72d

Merge remote-tracking branch 'upstream/main' into shared-ca

4d7a4d6

reinstate manual temp dir creation

d6cd21f

pebrc merged commit 6e93392 into elastic:main Apr 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support a globally shared CA #5539

Support a globally shared CA #5539

pebrc commented Mar 30, 2022 •

edited

pebrc commented Mar 31, 2022

pebrc commented Apr 4, 2022 •

edited

barkbay left a comment

barkbay Apr 15, 2022

barkbay left a comment

pebrc commented Apr 19, 2022

pebrc commented Apr 19, 2022

pebrc commented Apr 19, 2022 •

edited

barkbay commented Apr 20, 2022 •

edited

pebrc commented Apr 20, 2022

barkbay left a comment

barkbay commented Apr 20, 2022

ejsmith commented Jun 24, 2022

jmp601 commented Jul 8, 2022

naemono commented Jul 9, 2022

ejsmith commented Jul 9, 2022

jbouse commented Oct 16, 2023

	require.Equal(t, expected, event)
	require.ElementsMatch(t, expected, event)

Support a globally shared CA #5539

Support a globally shared CA #5539

Conversation

pebrc commented Mar 30, 2022 • edited

pebrc commented Mar 31, 2022

pebrc commented Apr 4, 2022 • edited

barkbay left a comment

Choose a reason for hiding this comment

barkbay Apr 15, 2022

Choose a reason for hiding this comment

barkbay left a comment

Choose a reason for hiding this comment

pebrc commented Apr 19, 2022

pebrc commented Apr 19, 2022

pebrc commented Apr 19, 2022 • edited

barkbay commented Apr 20, 2022 • edited

pebrc commented Apr 20, 2022

barkbay left a comment

Choose a reason for hiding this comment

barkbay commented Apr 20, 2022

ejsmith commented Jun 24, 2022

jmp601 commented Jul 8, 2022

naemono commented Jul 9, 2022

ejsmith commented Jul 9, 2022

jbouse commented Oct 16, 2023

pebrc commented Mar 30, 2022 •

edited

pebrc commented Apr 4, 2022 •

edited

pebrc commented Apr 19, 2022 •

edited

barkbay commented Apr 20, 2022 •

edited