ceph: add support in RGW to communicate vault with TLS

[thotz]

Description of your changes:

From ceph v16.2.6 onwards the vault TLS suppport in RGW was added,
include similar changes for RGW.

Signed-off-by: Jiffin Tony Thottan thottanjiffin@gmail.com

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

• Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
• Skip Tests for Docs: Add the flag for skipping the build if this is only a documentation change. See here for the flag.
• Skip Unrelated Tests: Add a flag to run tests for a specific storage provider. See test options.
• Reviewed the developer guide on Submitting a Pull Request
• Documentation has been updated, if necessary.
• Unit tests have been added, if necessary.
• Integration tests have been added, if necessary.
• Pending release notes updated with breaking and/or notable changes, if necessary.
• Upgrade from previous release is tested and upgrade user guide is updated, if necessary.
• Code generation (make codegen) has been run to update object specifications, if necessary.

[mergify]

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

[thotz]

@leseb: anyidea what is failing here, locally when I tried steps in github actions job it is working fine. Is OSD not coming up os the issue or toolpods is missing I am not able to figure this out

[leseb]

@leseb: anyidea what is failing here, locally when I tried steps in github actions job it is working fine. Is OSD not coming up os the issue or toolpods is missing I am not able to figure this out

2021-08-31 12:51:41.509712 E | ceph-cluster-controller: failed to reconcile CephCluster "rook-ceph/rook-ceph". failed to reconcile cluster "rook-ceph": failed to configure local ceph cluster: failed to perform validation before cluster creation: failed to validate kms connection details: failed to get backend version: failed to initialize vault client: open vault-client-cert: no such file or directory

[leseb]

holding until CI job is fixed

[thotz]

@leseb: anyidea what is failing here, locally when I tried steps in github actions job it is working fine. Is OSD not coming up os the issue or toolpods is missing I am not able to figure this out

2021-08-31 12:51:41.509712 E | ceph-cluster-controller: failed to reconcile CephCluster "rook-ceph/rook-ceph". failed to reconcile cluster "rook-ceph": failed to configure local ceph cluster: failed to perform validation before cluster creation: failed to validate kms connection details: failed to get backend version: failed to initialize vault client: open vault-client-cert: no such file or directory

@leseb : from code reading even though tlsConfig.Insecure is set ConfigureTLS() , setting VAULT_TLS_SECRET may creating the issue here. If understand correctly tlsConfig.other options expects path we but are providing secret names. IMO NewClient() is not ignoring this values even though tlsConfig.Insecure is set

[BlaineEXE]

No unit tests?

[thotz]

No unit tests?
@BlaineEXE I can add unit test for VaultTokenFileVolume() similar to TestTLSSecretVolumeAndMount().

Other addition can done in TestCheckRGWKMS() to validate the tls client certs passed, but similar cases are performed in TestValidateConnectionDetails() and CheckRGWKMS() calls the ValidateConnectionDetails() internally. So do I need to cover these cases?

[BlaineEXE]

No unit tests?
@BlaineEXE I can add unit test for VaultTokenFileVolume() similar to TestTLSSecretVolumeAndMount().

Other addition can done in TestCheckRGWKMS() to validate the tls client certs passed, but similar cases are performed in TestValidateConnectionDetails() and CheckRGWKMS() calls the ValidateConnectionDetails() internally. So do I need to cover these cases?

Yes. All code that adds or modifies a feature should come with added or modified unit tests. Part of development is writing tests to ensure the feature works as intended and to catch corner case issues during development. We have collectively (including myself) slacked off on doing this for Rook in the past, but with as much complexity as Rook has and the importance that it works well for our downstream customers and upstream users, we can't continue to ignore that responsibility.

When I write a new feature, the unit tests I write for it always catch bugs that I make in the code. Every single time. I usually find that I spend more time working on tests than the feature itself and that writing good tests that cover all corner cases is much more difficult than implementing the feature. When I estimate the time it will take to implement a feature, I often estimate the time I think it will take to implement the feature itself, then multiply by three to estimate the time it will take me to implement the feature plus the tests (I assume tests take twice as long).

Questions that every unit test should answer and test (not all are always applicable):

1. What if the feature is enabled?
2. What if the feature is disabled?
3. What if the feature is only partially enabled? (and how many ways can it be partially enabled?)
4. What errors can be encountered when the configuration is being applied? (need test for each kind of error)
5. Is there a slice/array involved? (test slice length=0, length=1, length=3, length=max, larger than max length)
6. What if an input is not specified? (test for every input that can be entered incorrectly)
7. What if an input is specified incorrectly? (test every input that can be incorrect)
8. What if a resource we rely on doesn't exist?

I'm sure there are some questions I missed.

[travisn]

It's great to see all the unit and integration tests, thanks

[BlaineEXE]

We should also run a version of TestPodSpecs for the RGW spec where KMS is enabled.

[mergify]

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

[BlaineEXE]

I flagged a few nit typos, given Seb already requested changes. I think this looks pretty good overall now, with some good questions from Seb.