ceph: fix kms auto-detection when full TLS #8867

leseb · 2021-09-29T13:34:20Z

Description of your changes:

When TLS is used and includes a caert, client key/cert, we need to copy
the content of the secret to a file in the operator's container
filesystem so that we can build the TLS config and thus the HTTP Client,
which reads those files.
Also, removing the files after each API call so they don't persist on
the filesystem forever.

Signed-off-by: Sébastien Han seb@redhat.com

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

.github/workflows/canary-integration-test.yml

pkg/daemon/ceph/osd/kms/kms.go

pkg/daemon/ceph/osd/kms/vault.go

mergify · 2021-10-01T07:47:34Z

This pull request has merge conflicts that must be resolved before it can be merged. @leseb please rebase it. https://rook.io/docs/rook/latest/development-flow.html#updating-your-fork

BlaineEXE

I wonder if the Sleep() calls in the tests are still needed.

If they are, I have generally seen the advice that unit tests that test timeouts should change the timeout value itself so that the test doesn't actually take many seconds to run unnecessarily. I think I've changed them to be on the order of milliseconds (10ms or so) with success on local runs as well as in CI.

pkg/daemon/ceph/osd/kms/vault_test.go

travisn · 2021-10-11T14:56:53Z

pkg/daemon/ceph/osd/kms/vault_api.go

+	// Populate TLS config
+	newConfigWithTLS, removeCertFiles, err := configTLS(clusterdContext, namespace, localSecretConfig)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to initialize vault tls configuration")


Would it be simpler to remove cert files here in the error condition, instead of inside configTLS()?

Suggested change

return nil, errors.Wrap(err, "failed to initialize vault tls configuration")

removeCertFiles()

return nil, errors.Wrap(err, "failed to initialize vault tls configuration")

I think removing the files from this function is the better design so that the calling function doesn't have to worry about handling special behavior. It seems like it's normal practice to me for go functions to clean up after themselves.

But I think what you're seeing is that modifying the function in the future, we could forget to keep removeCertFiles() before returning an error in configTLS()...

The caller already owns calling removeCertFiles() on success, so it seems reasonable to cleanup here on failure as well. Like you said, cleaning up could be missed in an individual error condition inside the method.

The cleanup is not missed anymore since we use defer in configTLS() now.

pkg/daemon/ceph/osd/kms/vault.go

pkg/daemon/ceph/osd/kms/vault_test.go

BlaineEXE · 2021-10-11T15:56:08Z

pkg/daemon/ceph/osd/kms/vault_api.go

+	// Populate TLS config
+	newConfigWithTLS, removeCertFiles, err := configTLS(clusterdContext, namespace, localSecretConfig)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to initialize vault tls configuration")


I think removing the files from this function is the better design so that the calling function doesn't have to worry about handling special behavior. It seems like it's normal practice to me for go functions to clean up after themselves.

But I think what you're seeing is that modifying the function in the future, we could forget to keep removeCertFiles() before returning an error in configTLS()...

pkg/daemon/ceph/osd/kms/vault.go

BlaineEXE · 2021-10-13T14:56:58Z

pkg/daemon/ceph/osd/kms/vault.go

+// The signature has named result parameters to help building 'defer' statements especially for the
+// content of removeCertFiles which needs to be populated by the files to remove if no errors and be
+// nil on errors
+func configTLS(clusterdContext *clusterd.Context, namespace string, config map[string]string) (newCconfig map[string]string, removeCertFiles removeCertFilesFunction, retErr error) {


Nit: typo?

Suggested change

func configTLS(clusterdContext *clusterd.Context, namespace string, config map[string]string) (newCconfig map[string]string, removeCertFiles removeCertFilesFunction, retErr error) {

func configTLS(clusterdContext *clusterd.Context, namespace string, config map[string]string) (newConfig map[string]string, removeCertFiles removeCertFilesFunction, retErr error) {

BlaineEXE · 2021-10-13T15:02:23Z

pkg/daemon/ceph/osd/kms/vault.go

+			err := file.Close()
+			if err != nil {
+				logger.Errorf("failed to close file %q. %v", file.Name(), err)
+			}


All the files are definitely still open when this is called, right?

Yes, we don't close them anywhere else.

BlaineEXE · 2021-10-13T15:06:18Z

pkg/daemon/ceph/osd/kms/vault_test.go

+		assert.NoFileExists(t, config["VAULT_CLIENT_KEY"])
+	})
+
+	t.Run("advanced TLS config with temp file creation error", func(t *testing.T) {


I'm having some trouble following what this test is checking for. Let's discuss in huddle.

Ok but essentially it's testing that previously created files are removed if any invocation in conifgTLS fails. It's testing the cleanup.

When TLS is used and includes a caert, client key/cert, we need to copy the content of the secret to a file in the operator's container filesystem so that we can build the TLS config and thus the HTTP Client, which reads those files. Also, removing the files after each API call so they don't persist on the filesystem forever. Signed-off-by: Sébastien Han <seb@redhat.com>

travisn

Is there an upstream need to backport to 1.6? If not, let's just backport to 1.7.

ceph: fix kms auto-detection when full TLS (backport #8867)

leseb added the backport-release-1.7 label Sep 29, 2021

mergify bot added the ceph main ceph tag label Sep 29, 2021

leseb force-pushed the kv-auto-detect-with-certs branch from e5d0b24 to 94b7e27 Compare September 29, 2021 14:39

leseb requested review from travisn and BlaineEXE September 29, 2021 14:43

leseb force-pushed the kv-auto-detect-with-certs branch 4 times, most recently from e3ca518 to 4c5d090 Compare September 30, 2021 12:33

leseb added this to In progress in v1.7 via automation Sep 30, 2021

BlaineEXE reviewed Sep 30, 2021

View reviewed changes

.github/workflows/canary-integration-test.yml Outdated Show resolved Hide resolved

BlaineEXE reviewed Sep 30, 2021

View reviewed changes

pkg/daemon/ceph/osd/kms/kms.go Outdated Show resolved Hide resolved

pkg/daemon/ceph/osd/kms/vault.go Show resolved Hide resolved

leseb force-pushed the kv-auto-detect-with-certs branch 4 times, most recently from 1691be9 to 4ef2ff3 Compare October 1, 2021 08:48

leseb requested a review from BlaineEXE October 1, 2021 09:16

leseb force-pushed the kv-auto-detect-with-certs branch from 4ef2ff3 to 5f81511 Compare October 1, 2021 09:17

leseb added the backport-release-1.6 label Oct 1, 2021

BlaineEXE reviewed Oct 1, 2021

View reviewed changes

pkg/daemon/ceph/osd/kms/vault_test.go Outdated Show resolved Hide resolved

pkg/daemon/ceph/osd/kms/vault_test.go Outdated Show resolved Hide resolved

leseb force-pushed the kv-auto-detect-with-certs branch from 5f81511 to d9f0249 Compare October 11, 2021 12:57

leseb requested a review from BlaineEXE October 11, 2021 12:58

travisn reviewed Oct 11, 2021

View reviewed changes

BlaineEXE reviewed Oct 11, 2021

View reviewed changes

leseb force-pushed the kv-auto-detect-with-certs branch from d9f0249 to 76214cd Compare October 13, 2021 14:06

leseb requested review from BlaineEXE and travisn October 13, 2021 14:07

leseb force-pushed the kv-auto-detect-with-certs branch from 76214cd to db6413f Compare October 13, 2021 14:08

BlaineEXE reviewed Oct 13, 2021

View reviewed changes

leseb force-pushed the kv-auto-detect-with-certs branch from db6413f to 8a6b332 Compare October 13, 2021 15:12

leseb force-pushed the kv-auto-detect-with-certs branch from 8a6b332 to 61afadd Compare October 13, 2021 16:13

leseb requested a review from BlaineEXE October 13, 2021 22:47

BlaineEXE approved these changes Oct 13, 2021

View reviewed changes

travisn removed the backport-release-1.6 label Oct 14, 2021

travisn approved these changes Oct 14, 2021

View reviewed changes

leseb merged commit 07986bc into rook:master Oct 14, 2021

v1.7 automation moved this from In progress to Done Oct 14, 2021

leseb deleted the kv-auto-detect-with-certs branch October 14, 2021 09:30

mergify bot mentioned this pull request Oct 14, 2021

ceph: fix kms auto-detection when full TLS (backport #8867) #8977

Merged

mergify bot added a commit that referenced this pull request Oct 14, 2021

Merge pull request #8977 from rook/mergify/bp/release-1.7/pr-8867

f704fb2

ceph: fix kms auto-detection when full TLS (backport #8867)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ceph: fix kms auto-detection when full TLS #8867

ceph: fix kms auto-detection when full TLS #8867

leseb commented Sep 29, 2021

mergify bot commented Oct 1, 2021

BlaineEXE left a comment

travisn Oct 11, 2021

BlaineEXE Oct 11, 2021

travisn Oct 13, 2021

leseb Oct 13, 2021

BlaineEXE Oct 11, 2021

BlaineEXE Oct 13, 2021

BlaineEXE Oct 13, 2021

leseb Oct 13, 2021

BlaineEXE Oct 13, 2021

leseb Oct 13, 2021

travisn left a comment

	return nil, errors.Wrap(err, "failed to initialize vault tls configuration")
	removeCertFiles()
	return nil, errors.Wrap(err, "failed to initialize vault tls configuration")

	func configTLS(clusterdContext *clusterd.Context, namespace string, config map[string]string) (newCconfig map[string]string, removeCertFiles removeCertFilesFunction, retErr error) {
	func configTLS(clusterdContext *clusterd.Context, namespace string, config map[string]string) (newConfig map[string]string, removeCertFiles removeCertFilesFunction, retErr error) {

Navigation Menu

ceph: fix kms auto-detection when full TLS #8867

ceph: fix kms auto-detection when full TLS #8867

Conversation

leseb commented Sep 29, 2021

mergify bot commented Oct 1, 2021

BlaineEXE 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

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

travisn left a comment

Choose a reason for hiding this comment