-
Notifications
You must be signed in to change notification settings - Fork 697
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support a globally shared CA #5539
Conversation
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. |
b7d3852
to
b505883
Compare
I am not 💯 sure about supporting the two naming conventions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
requireEventEquals := func(c chan []string, expected []string, timeout time.Duration) { | ||
select { | ||
case event := <-c: | ||
require.Equal(t, expected, event) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need a strict equality here, my gut feeling is that it can be a cause of flaky tests
require.Equal(t, expected, event) | |
require.ElementsMatch(t, expected, event) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pkg/controller/common/certificates/ca_reconcile_integration_test.go
Outdated
Show resolved
Hide resolved
Jenkins test this please
|
Unfortunately it looks like the file watcher integration test I added is flaky. Trying to improve on the current implementation. |
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.
See how I modified the test's assumptions to allow for additional events on the simulated writes. |
…ut it actually found a bug
I think that there are always at least 2 underlying syscall (using
Running the 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
|
Of course! I clearly didn't think this through. Thanks for the great explanation!
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, I just left some nitpicks 👍 🚀
pkg/controller/common/certificates/ca_reconcile_integration_test.go
Outdated
Show resolved
Hide resolved
pkg/controller/common/certificates/ca_reconcile_integration_test.go
Outdated
Show resolved
Hide resolved
No, such small files are very unlikely to be spread across multiple pages (min. page size is 4KB on Linux IIUC). |
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. |
We have some documentation around this feature: 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) We also have an issue tracking updating our documentation to be more clear #4812 |
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.
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. |
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 directorytls.crt
andtls.key
. I am thinking of supportingca.crt
andca.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.