core: azure kms support #13852

sp98 · 2024-03-04T02:16:06Z

Add support for store OSD encryption Keys in Azure KMS

Store OSD encryption key in Azure KMS
Fetch OSD encryption key from Azure KMS
Delete OSD encryption keys from Azure KMS when the cluster is deleted. (Deleting doesn't purge the OSD, use has to do that manually)

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

mergify · 2024-03-04T02:16:41Z

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

subhamkrai · 2024-03-04T11:28:10Z

go.mod

+	github.com/kubernetes-incubator/external-storage => github.com/libopenstorage/external-storage v0.20.4-openstorage-rc3
+	// github.com/libopenstorage/secrets => github.com/sp98/secrets v0.0.0-20240220060802-e2bb123d06b9
+	github.com/libopenstorage/secrets => github.com/sp98/secrets v0.0.0-20240304064404-1ca2f1c12a8b
+	github.com/portworx/sched-ops => github.com/portworx/sched-ops v0.20.4-openstorage-rc3


Suggested change

github.com/portworx/sched-ops => github.com/portworx/sched-ops v0.20.4-openstorage-rc3

github.com/portworx/sched-ops => github.com/portworx/sched-ops v0.20.4-openstorage-rc3

any reason for this change? As we have put the portworx import in exclude section.

We originally had this in the go.mod in Rook

// portworx dependencies are a mess, and we don't use portworx code, so skip it github.com/portworx/sched-ops v1.20.4-rc1

The reason for this is that the portworx/sched-ops repo is still referring to this incorrect version of kubernetes-incubator/external-storage. This version dependency needs to be fixed in sched-ops.

sched-ops is only used by Azure KMS. So we excluded it since we were not using Azure.

Now that we are using Azure, we need to use Azure KMS, we need to repo. And solution was the replace it like this, since the issue is still not fixed.

github.com/portworx/sched-ops => github.com/portworx/sched-ops v0.20.4-openstorage-rc3

I need to remove what we have in exclude section. That does not make sense now.

Thanks for pointing this out.

thanks for explaining the uses.

Thanks for explaining. I marked this it as unresolved to make sure that we keep the conversation more easily visible in case we have to reference it in the future. I don't see a way to mark it as resolved but still see the convo.

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

BlaineEXE · 2024-03-04T15:30:52Z

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

+	if c.IsAzure() {
+		v, err := InitAzure(c.ClusterInfo.Context, c.context, c.ClusterInfo.Namespace, c.clusterSpec.Security.KeyManagementService.ConnectionDetails)
+		if err != nil {
+			return errors.Wrap(err, "failed to init azure key vault")
+		}
+		err = put(v, GenerateOSDEncryptionSecretName(secretName), secretValue, map[string]string{})
+		if err != nil {
+			return errors.Wrap(err, "failed to put secret in azure key vault")
+		}
+	}
+


I see 3 instances of this same code block. What is the reason for that?

They are for put, get and delete operations.

Got it. Thanks for explaining.

In general, I see this as a code smell. Methods like put/get shouldn't be doing KMS type checking. I think this is highlighting that our KMS support was hastily implemented and has been clunkily extended over time. I don't think this needs to be resolved in this PR, but it would be good to consider cleaning up the KMS code afterwards.

The better implementation would be to use a Golang interface that defines all the KMS methods, and then have each KMS implement all the methods. That would allow us to implement new KMSes more easily, it would make unit testing easier, and it would allow us to do functional e2e testing of KMSes without having to do a full install of Rook.

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

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

travisn

some small suggestions

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

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

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

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

travisn · 2024-03-07T22:33:37Z

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

+	if err != nil && err != secrets.ErrInvalidSecretId && err != secrets.ErrSecretNotFound {
+		return errors.Wrapf(err, "failed to get secret %q in kms", secretName)
+	}
+	if key != "" {


Are updates supported? Or should we return an error if the value doesn't match? If the value changes it looks like the method succeeds, but ignores the new value. The caller will believe the latest value was set.

I really don't think its supported.
This is an existing function used by hashcorp vault as well. The key is created for each PVC and then stored in the KMS.

Maybe we don't need to worry about an update, since we don't expect it to change. How about logging an error if it doesn't match, but don't fail the reconcile?

update to code the add error message and return if the new secret value is not same when compared to existing secret value in the KMS. .

mergify · 2024-03-11T13:35:20Z

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

travisn

Please add a note about this feature in the PendingReleaseNotes.md. Hopefully we can get traction on the dependent PR soon, but otherwise this looks good.

mergify · 2024-03-14T01:02:35Z

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

travisn · 2024-03-14T23:08:48Z

@sp98 A fork of the libopenstorage/secrets is created now. How about opening your PR against that repo and we can move this forward for the short term? Hopefully this is only temporary.

sp98 · 2024-03-15T02:08:28Z

@sp98 A fork of the libopenstorage/secrets is created now. How about opening your PR against that repo and we can move this forward for the short term? Hopefully this is only temporary.

cherrypicked my PR in the fork - rook/secrets#1

Add support for store OSD encryption Keys in Azure KMS Signed-off-by: sp98 <sapillai@redhat.com>

sp98 · 2024-03-15T06:08:13Z

@sp98 A fork of the libopenstorage/secrets is created now. How about opening your PR against that repo and we can move this forward for the short term? Hopefully this is only temporary.

Updated the go.mod to use the rook fork of the secrets library. Manually tested it. Looks good. Able store, retrieve and delete OSD encryption keys.

parth-gr

nit

parth-gr · 2024-03-18T05:00:18Z

pkg/apis/go.mod

+	github.com/googleapis/gnostic => github.com/googleapis/gnostic v0.4.1
+	github.com/kubernetes-incubator/external-storage => github.com/libopenstorage/external-storage v0.20.4-openstorage-rc3
+
+	// TODO: remove this replace once https://github.com/libopenstorage/secrets/pull/83 is merged


replace once libopenstorage/secrets#83 is merged

iPraveenParihar

LGTM

The build was failing locally for me with a go module checksum mismatch error: ```console verifying github.com/portworx/sched-ops@v0.20.4-openstorage-rc3: checksum mismatch downloaded: h1:46EZ+vYCJ3qmQolvgDCrGuPz8Tf0vIds41RuF0dqVEw= go.sum: h1:tXnHsjZT2wZ2BCXf8avDoya7zGyCgLNUC8Upt+WEQrY= `` The last change to go.sum related to this module was from PR rook#13852 This change to go.sum fixes the issue for me. It is the result of the following commands: ````console rm go.sum go mod tidy `` Signed-off-by: Michael Adam <obnox@samba.org>

The build was failing locally for me with a go module checksum mismatch error: ```console verifying github.com/portworx/sched-ops@v0.20.4-openstorage-rc3: checksum mismatch downloaded: h1:46EZ+vYCJ3qmQolvgDCrGuPz8Tf0vIds41RuF0dqVEw= go.sum: h1:tXnHsjZT2wZ2BCXf8avDoya7zGyCgLNUC8Upt+WEQrY= `` The last change to go.sum related to this module was from PR rook#13852 This change to go.sum fixes the issue for me. Fixes: rook#14282 It is the result of the following commands: ````console rm go.sum go mod tidy `` Signed-off-by: Michael Adam <obnox@samba.org>

sp98 force-pushed the support-azure-kms branch from ff75055 to e9774cf Compare March 4, 2024 07:39

subhamkrai reviewed Mar 4, 2024

View reviewed changes

BlaineEXE reviewed Mar 4, 2024

View reviewed changes

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

BlaineEXE reviewed Mar 4, 2024

View reviewed changes

sp98 force-pushed the support-azure-kms branch 2 times, most recently from 0677351 to bd8d741 Compare March 5, 2024 06:14

sp98 marked this pull request as ready for review March 5, 2024 10:11

sp98 changed the title ~~core: azure kms support~~ [WIP]core: azure kms support Mar 5, 2024

sp98 force-pushed the support-azure-kms branch 2 times, most recently from cfa0af7 to 2fedef3 Compare March 7, 2024 06:40

sp98 changed the title ~~[WIP]core: azure kms support~~ core: azure kms support Mar 7, 2024

sp98 changed the title ~~core: azure kms support~~ [WIP]core: azure kms support Mar 7, 2024

sp98 force-pushed the support-azure-kms branch 4 times, most recently from 2999a5f to 80d6d98 Compare March 7, 2024 07:44

sp98 requested review from subhamkrai and BlaineEXE March 7, 2024 07:44

sp98 force-pushed the support-azure-kms branch 4 times, most recently from 75bb98d to d99fa19 Compare March 7, 2024 13:43

sp98 changed the title ~~[WIP]core: azure kms support~~ core: azure kms support Mar 7, 2024

iPraveenParihar requested changes Mar 7, 2024

View reviewed changes

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

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

sp98 force-pushed the support-azure-kms branch from d99fa19 to 9155253 Compare March 7, 2024 14:16

sp98 requested a review from iPraveenParihar March 7, 2024 15:34

travisn requested changes Mar 7, 2024

View reviewed changes

sp98 force-pushed the support-azure-kms branch 2 times, most recently from 076c834 to 6d7c549 Compare March 8, 2024 13:35

sp98 force-pushed the support-azure-kms branch from 0850212 to 6863077 Compare March 11, 2024 14:54

sp98 requested a review from travisn March 11, 2024 15:09

sp98 force-pushed the support-azure-kms branch 4 times, most recently from 3137314 to 0049f48 Compare March 13, 2024 16:00

travisn reviewed Mar 13, 2024

View reviewed changes

sp98 force-pushed the support-azure-kms branch from 0049f48 to 7722aa1 Compare March 14, 2024 01:01

sp98 force-pushed the support-azure-kms branch from 7722aa1 to dbf0ab7 Compare March 14, 2024 01:03

sp98 requested a review from travisn March 14, 2024 01:03

sp98 force-pushed the support-azure-kms branch from dbf0ab7 to ccd524b Compare March 14, 2024 01:18

core: azure kms support

4efe398

Add support for store OSD encryption Keys in Azure KMS Signed-off-by: sp98 <sapillai@redhat.com>

sp98 force-pushed the support-azure-kms branch from ccd524b to 4efe398 Compare March 15, 2024 05:53

travisn approved these changes Mar 15, 2024

View reviewed changes

subhamkrai approved these changes Mar 18, 2024

View reviewed changes

parth-gr approved these changes Mar 18, 2024

View reviewed changes

sp98 requested review from BlaineEXE and iPraveenParihar and removed request for BlaineEXE and iPraveenParihar March 18, 2024 05:01

iPraveenParihar approved these changes Mar 18, 2024

View reviewed changes

sp98 merged commit b8ee8a4 into rook:master Mar 18, 2024
51 checks passed

obnoxxx mentioned this pull request Jun 3, 2024

build: fix go mod checksum mismatch error #14283

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: azure kms support #13852

core: azure kms support #13852

sp98 commented Mar 4, 2024 •

edited

Loading

mergify bot commented Mar 4, 2024

subhamkrai Mar 4, 2024

sp98 Mar 5, 2024 •

edited

Loading

subhamkrai Mar 5, 2024

BlaineEXE Mar 11, 2024

BlaineEXE Mar 4, 2024

sp98 Mar 7, 2024 •

edited by travisn

Loading

BlaineEXE Mar 11, 2024

travisn left a comment

travisn Mar 7, 2024

sp98 Mar 8, 2024

travisn Mar 8, 2024

sp98 Mar 11, 2024

mergify bot commented Mar 11, 2024

travisn left a comment

mergify bot commented Mar 14, 2024

travisn commented Mar 14, 2024

sp98 commented Mar 15, 2024

sp98 commented Mar 15, 2024 •

edited

Loading

parth-gr left a comment

parth-gr Mar 18, 2024

iPraveenParihar left a comment

	github.com/portworx/sched-ops => github.com/portworx/sched-ops v0.20.4-openstorage-rc3
	github.com/portworx/sched-ops => github.com/portworx/sched-ops v0.20.4-openstorage-rc3

core: azure kms support #13852

core: azure kms support #13852

Conversation

sp98 commented Mar 4, 2024 • edited Loading

mergify bot commented Mar 4, 2024

Choose a reason for hiding this comment

sp98 Mar 5, 2024 • edited Loading

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

sp98 Mar 7, 2024 • edited by travisn Loading

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

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

mergify bot commented Mar 11, 2024

travisn left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 14, 2024

travisn commented Mar 14, 2024

sp98 commented Mar 15, 2024

sp98 commented Mar 15, 2024 • edited Loading

parth-gr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iPraveenParihar left a comment

Choose a reason for hiding this comment

sp98 commented Mar 4, 2024 •

edited

Loading

sp98 Mar 5, 2024 •

edited

Loading

sp98 Mar 7, 2024 •

edited by travisn

Loading

sp98 commented Mar 15, 2024 •

edited

Loading