search

noobaa / noobaa-operator

Operator for NooBaa - object data service for hybrid and multi cloud environments :cloud: :wrench:
https://www.noobaa.io
Apache License 2.0
103 stars 101 forks source link

Azure kms handling for noobaa #1311

Closed vh05 closed 8 months ago

vh05 commented 8 months ago

This patch provides the support for azure keyvault.

We are using libopenstorage/secrets as the wrapper package to integrate with different kms and package provides the abstraction over several kms. It also provides the integraton support for azure and helps communication with azure key vault.

We are required to provide the definition for house keeping calls registered calls with libopenstorage/secrets.

libopenstorage/secrets does the creation of client handle based on the details provided in config data. Noobaa CR will have these details under connectionDetails and these details are popupated on Noobaa CR by Noobaa ocs by reading the configmap provided by ODF. The certificate details present in the secret are copied into a temp file and used to establish the connection with azure key vault as of now.

libopenstorage/secretssupports only secret as string as of now. Our requirement is to authenticate using certificate. To facililate this, we have PR to accomodate certificate related handling and the PR can be found here: https://github.com/libopenstorage/secrets/pull/83/files

Below are the connection details that are going to be populated on Noobaa CR by noobaa_system_reconciler at ocs side and this is the ocs code where connectiondetails on Noobaa CR are built: https://github.com/red-hat-storage/ocs-operator/blob/2d082fc4c1ac4cec961406053cece448f4b07684/controllers/storagecluster/noobaa_system_reconciler.go#L249

ex: configmap data: AZURE_CERT_SECRET_NAME = Secret name from which we extract the certificate data and send it to libopenstorage/secrets to get the client handle for service principle to access the root master key

data:
  AZURE_CERT_SECRET_NAME: azure-ocs-ffwc9o1j
  AZURE_CLIENT_ID: az-client-id1
  AZURE_TENANT_ID: az-tenant-id1
  AZURE_VAULT_URL: az-valut-url1
  KMS_PROVIDER: azure-kv
  KMS_SERVICE_NAME: kms-conn-azure1
vh05 commented 8 months ago

I think we need to add test for azure KMS, also I think we need some more info about what we expect the info sent by the NooBaa CR should look like, I mean what config info we need.

Below are the connection details that are going to be populated on Noobaa CR by noobaa_system_reconciler at ocs side and this is the ocs code where connectiondetails on Noobaa CR are built: https://github.com/red-hat-storage/ocs-operator/blob/2d082fc4c1ac4cec961406053cece448f4b07684/controllers/storagecluster/noobaa_system_reconciler.go#L249

ex: configmap data:

data:
  AZURE_CERT_SECRET_NAME: azure-ocs-ffwc9o1j
  AZURE_CLIENT_ID: az-client-id1
  AZURE_TENANT_ID: az-tenant-id1
  AZURE_VAULT_URL: az-valut-url1
  KMS_PROVIDER: azure-kv
  KMS_SERVICE_NAME: kms-conn-azure1
vh05 commented 8 months ago

I have some comment, the main issue I have is with AZURE_CERT_SECRET_NAME which I don't understand if it needs to be in a file and where this requirement is coming for, what I did find in the secrets was AZURE_CLIENT_SECRET which I'm not sure if that't what you meant, and it looks like a regular string.

There are 2 things.

  1. libopenstorage/secrets currently support "secret" as string.
  2. The requirement is to go for "secret" as certificate.

Currently libopenstorage/secrets dont support secret as certificate. We have (Member from Rook team, Santosh, working for azure kms on rook side and owning this azure kms story) pushed a patch to libopenstorage/sectes support secret certificate and that can be found here: https://github.com/libopenstorage/secrets/pull/83/files#diff-91153976f4824b52d20fd598108e77d2a5d605f6b67dedec4f0ba53e70ca5e49R82-R93

With the certificate handling in place, we need to create a temp file and put the temp path in the map. This is used at libopenstrorage/secrets to create a service principle. The code for the same is here at: https://github.com/libopenstorage/secrets/pull/83/files#diff-feaeae3297b3a2250b55e5307f4f8ef82bf0c6032af4ffe0736c6814ec7855f5R33-R37

The path we store in the map is accessed at the libopenstorage/secrets at here: https://github.com/libopenstorage/secrets/pull/83/files#diff-91153976f4824b52d20fd598108e77d2a5d605f6b67dedec4f0ba53e70ca5e49R74

I have imported the hash for the same in this PR which here: https://github.com/noobaa/noobaa-operator/pull/1311/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R5-R8

jackyalbo commented 8 months ago

I have some comment, the main issue I have is with AZURE_CERT_SECRET_NAME which I don't understand if it needs to be in a file and where this requirement is coming for, what I did find in the secrets was AZURE_CLIENT_SECRET which I'm not sure if that't what you meant, and it looks like a regular string.

There are 2 things.

  1. libopenstorage/secrets currently support "secret" as string.
  2. The requirement is to go for "secret" as certificate.

Currently libopenstorage/secrets dont support secret as certificate. We have (Member from Rook team, Santosh, working for azure kms on rook side and owning this azure kms story) pushed a patch to libopenstorage/sectes support secret certificate and that can be found here: https://github.com/libopenstorage/secrets/pull/83/files#diff-91153976f4824b52d20fd598108e77d2a5d605f6b67dedec4f0ba53e70ca5e49R82-R93

With the certificate handling in place, we need to create a temp file and put the temp path in the map. This is used at libopenstrorage/secrets to create a service principle. The code for the same is here at: https://github.com/libopenstorage/secrets/pull/83/files#diff-feaeae3297b3a2250b55e5307f4f8ef82bf0c6032af4ffe0736c6814ec7855f5R33-R37

The path we store in the map is accessed at the libopenstorage/secrets at here: https://github.com/libopenstorage/secrets/pull/83/files#diff-91153976f4824b52d20fd598108e77d2a5d605f6b67dedec4f0ba53e70ca5e49R74

I have imported the hash for the same in this PR which here: https://github.com/noobaa/noobaa-operator/pull/1311/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R5-R8

I'm a bit worried about us relying on a PR that is not yet merged, and not even properly reviewed... Also think that maybe we should wait for the tests and testing infra before pushing this. @liranmauda @nimrod-becker what do you think?

vh05 commented 8 months ago

I have some comment, the main issue I have is with AZURE_CERT_SECRET_NAME which I don't understand if it needs to be in a file and where this requirement is coming for, what I did find in the secrets was AZURE_CLIENT_SECRET which I'm not sure if that't what you meant, and it looks like a regular string.

There are 2 things.

  1. libopenstorage/secrets currently support "secret" as string.
  2. The requirement is to go for "secret" as certificate.

Currently libopenstorage/secrets dont support secret as certificate. We have (Member from Rook team, Santosh, working for azure kms on rook side and owning this azure kms story) pushed a patch to libopenstorage/sectes support secret certificate and that can be found here: https://github.com/libopenstorage/secrets/pull/83/files#diff-91153976f4824b52d20fd598108e77d2a5d605f6b67dedec4f0ba53e70ca5e49R82-R93 With the certificate handling in place, we need to create a temp file and put the temp path in the map. This is used at libopenstrorage/secrets to create a service principle. The code for the same is here at: https://github.com/libopenstorage/secrets/pull/83/files#diff-feaeae3297b3a2250b55e5307f4f8ef82bf0c6032af4ffe0736c6814ec7855f5R33-R37 The path we store in the map is accessed at the libopenstorage/secrets at here: https://github.com/libopenstorage/secrets/pull/83/files#diff-91153976f4824b52d20fd598108e77d2a5d605f6b67dedec4f0ba53e70ca5e49R74 I have imported the hash for the same in this PR which here: https://github.com/noobaa/noobaa-operator/pull/1311/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R5-R8

I'm a bit worried about us relying on a PR that is not yet merged, and not even properly reviewed... Also think that maybe we should wait for the tests and testing infra before pushing this. @liranmauda @nimrod-becker what do you think?

Quick update to this. Rook is planning to have libopenstorage/secrets as a submodule (cloned) inside rook and merge the PR (https://github.com/rook/secrets/pull/1) for azure certificate as of now and use it for the tech preview. Once the PR is merged at libopenstorage/secrets then we switch to that package.

The cloned branch (submodule at rook: https://github.com/rook/secrets) is libopenstorage/secrets master + PR (for azure certificate handling: https://github.com/rook/secrets/pull/1)

The PR is merged at the rook side. This temp arrangement is required since the PR is not yet reviewed by libopenstorage team.

Changes at Noobaa side that facilitates the above explained approach: https://github.com/noobaa/noobaa-operator/pull/1311/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R5-R9

Your thoughts ? @liranmauda @jackyalbo @nimrod-becker @dannyzaken

sp98 commented 8 months ago

I'm a bit worried about us relying on a PR that is not yet merged, and not even properly reviewed... Also think that maybe we should wait for the tests and testing infra before pushing this. @liranmauda @nimrod-becker what do you think?

Agree that we don't have it reviewed by the maintainers yet. Pinged them a few weeks ago and pinged on slack, but no response yet.

We have review comments from the our own team members that have been addressed.

For rook, we created a fork and plan to use that fork. This is temporary until the PR is reviewed by the library maintainer and merged.

Since the changes were made for azure module, I don't see it affecting the other modules (like hashicorp vault). But we should ensure that there are no issue is manual testing of this fork. We are ensuring that in Rook.