Load back Elasticsearch certs from secrets

[pavolloffay]

Resolves #320

• avoids generating different certs if the operator restarts
• restores edited secrets by user
• stores certs in /tmp/_cert/namespace/instance_name/uuid

Signed-off-by: Pavol Loffay ploffay@redhat.com

[jpkrohling]

This change is 

[codecov]

Codecov Report

Merging #324 into master will decrease coverage by 0.13%.
The diff coverage is 81.42%.

@@            Coverage Diff            @@
##           master    #324      +/-   ##
=========================================
- Coverage   88.94%   88.8%   -0.14%     
=========================================
  Files          69      69              
  Lines        3112    3154      +42     
=========================================
+ Hits         2768    2801      +33     
- Misses        234     239       +5     
- Partials      110     114       +4

Impacted Files	Coverage Δ
pkg/strategy/controller.go	96.96% <100%> (ø)	⬆️
pkg/strategy/production.go	68.57% <100%> (ø)	⬆️
pkg/storage/elasticsearch.go	81.53% <50%> (ø)	⬆️
pkg/controller/jaeger/jaeger_controller.go	38.61% <58.33%> (+2.44%)	⬆️
pkg/storage/elasticsearch_secrets.go	92.39% <86.53%> (-7.61%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fcc9dd...e9cdfd3. Read the comment docs.

[objectiser]

Only minor comments.

@pavolloffay Happy for this to be merged now if will help complete other tasks before your PTO.

@jpkrohling Even if merged, could you just give it a once over to check you are happy with the change from a security perspective?

[objectiser]

Thought this method would only be called for overwriting the secret data with existing secret - so shouldn't the file already exist? Not a big issue, but just wanted to clarify my understanding.

[pavolloffay]

This method is called on every loop. The previous statement returns if file exists. If the file does not exists we have to create before creating the file.

[objectiser]

@pavolloffay Sorry forgot to mention code coverage needs fixing.

[jpkrohling]

Reviewed 8 of 10 files at r1, 2 of 2 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @objectiser and @pavolloffay)

---

pkg/controller/jaeger/jaeger_controller.go, line 113 at r3 (raw file):

	list := &corev1.SecretList{}
	if err := r.client.List(context.Background(), opts, list); err != nil {
		return reconcile.Result{}, err

Do we have to fail here? Can't we recover from this?

---

pkg/controller/jaeger/jaeger_controller.go, line 115 at r3 (raw file):

		return reconcile.Result{}, err
	}
	str := r.runStrategyChooser(instance, list.Items)

The secrets should be set directly on the strategy afterwards instead of being passed to the strategy chooser. Otherwise, it implies that the secrets are somehow part of the selection algorithm, which is not the case.

Something like:

str := r.runStrategyChooser(instance)
str.ExistingSecrets(list.Items)

Then, each strategy would be able to consume it in case it's relevant.

Ideally, though, we'd have most of the code from this PR inside the applySecrets() function and inject the secret/storage data based on the outcome of this sync, instead of as part of the strategy. This would require a bigger refactoring t hough, so, something for the next version.

---

pkg/storage/elasticsearch_secrets.go, line 21 at r3 (raw file):

const (
	tmpWorkingDir = "/tmp/_certs"

I guess I missed this during the initial review, but for the record: a cleaner solution is to use os.TempDir(), as /tmp might not be the right place.

---

pkg/storage/elasticsearch_secrets.go, line 195 at r3 (raw file):

func writeToWorkingDirFile(dir, toFile string, value []byte) error {
	// first check if file exists - we prefer what is on FS to revert users editing secrets

I'd say the opposite should be true: always sync the local file system with the secret. Users should be instructed to change the secret instead of the Operator's file system.

---

pkg/storage/elasticsearch_secrets.go, line 210 at r3 (raw file):

	_, err = f.Write(value)
	if err != nil {
		return err

If the failure happens during the write (like, an IO error of some sort), then we'd have a partially written file + all the files that were written before that. Later on, you said that this PR keeps the file that exists on disk instead of overwriting it. This would result in a failure that is very, very hard to debug in the future.

Independent from what happens with the partially written file in the next iteration, I think this code here should attempt to remove the file that failed to be written, in case it exists.

[pavolloffay]

Do we have to fail here? Can't we recover from this?

We do fail in the controller if we cannot get objects. So maybe better to keep it the same for consistency.

The secrets should be set directly on the strategy afterwards instead of being passed to the strategy chooser. Otherwise, it implies that the secrets are somehow part of the selection algorithm, which is not the case.

Unfortunately, the str := r.runStrategyChooser(instance) already runs strategy and sets all objects. To do what you propose we would have to refactor to choose the strategy first and then run it.

I guess I missed this during the initial review, but for the record: a cleaner solution is to use os.TempDir(), as /tmp might not be the right place.

Let's address this when it actually will be an issue. Btw we do create the dir if it does not exists.

Users should be instructed to change the secret instead of the Operator's file system.

No, we trust the operator and not the objects. What you propose means that users could also edit deployments and jaeger would change the CR appropriately.

[jpkrohling]

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @objectiser and @pavolloffay)

---

pkg/controller/jaeger/jaeger_controller.go, line 113 at r3 (raw file):

So maybe better to keep it the same for consistency.

If we don't have to fail, then it's better not to fail. The controllers fail because we can't reconcile the state if we can't get the "current" list of objects. If the same is true for this logic, I'm OK to keep it failing, but if we can recover from this, even better.

---

pkg/storage/elasticsearch_secrets.go, line 21 at r3 (raw file):

Let's address this when it actually will be an issue. Btw we do create the dir if it does not exists.

It's alright. The point of using os.TempDir() is because the user might have specified their preferred tmpdir, which might not be /tmp. This is usually the case when dealing with large files, which is not the case here, so, it's OK to leave it as is for now.

---

pkg/storage/elasticsearch_secrets.go, line 195 at r3 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

I'd say the opposite should be true: always sync the local file system with the secret. Users should be instructed to change the secret instead of the Operator's file system.

As we talked on IRC, the best would be to have the secret as the source of truth. This is consistent with other Kubernetes/OpenShift objects. If the secret does not exist, the operator will generate the appropriate certs and update the secret. Later on, the Operator creates the cert in the file system and synchronizes during every reconciliation, in case the backing secret changes.

---

pkg/storage/elasticsearch_secrets_test.go, line 135 at r4 (raw file):

		},
		{
			dir: "/root", file: "bla", err: "open /root/bla: permission denied",

Is the test actually trying to write to /root and /foo? :-)

[pavolloffay]

As we talked on IRC, the best would be to have the secret as the source of truth. This is consistent with other Kubernetes/OpenShift objects. If the secret does not exist, the operator will generate the appropriate certs and update the secret. Later on, the Operator creates the cert in the file system and synchronizes during every reconciliation, in case the backing secret changes.

At the moment we don't officially support users providing their own certs - secrets. That said this PR assures that secrets are what operator created. If a users tries to edit them it will be reverted.

We should track users providing their own certs in a separate issue.

[pavolloffay]

I have created #330 for providing certs by user