Allow updating Object Storage Details

[tpodowd]

Description

Currently, when an administrator edits an Object Storage Pool, only the name or url can be updated.

This change allows the other details of the object store to be updated. The object store details contains the credentials used to connect to the object store as well as other information provided when it was added to the system.

Only updated information is updated. The connection is tested using a new method verifyServiceConnectivity(). If no information has changed, the existing connection is also tested.

In the general case (MinIO/Ceph and Simulator), this change allows you to edit the name and URL of the object storage as before in addition to the credentials used to connect to the object storage. Previously it was not possible to edit them after creation using the GUI. In the case of Cloudian HyperStore Object Storage, this allows you to additionally edit the service URL/ports.

Types of changes

• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Bug fix (non-breaking change which fixes an issue)
• [X] Enhancement (improves an existing feature and functionality)
• [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
• [ ] build/CI
• [ ] test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• [ ] Major
• [X] Minor

Bug Severity

• [ ] BLOCKER
• [ ] Critical
• [ ] Major
• [ ] Minor
• [ ] Trivial

Screenshots (if appropriate):

Before this change:

After the change: General case (editing MinIO is shown):

After the change: Cloudian HyperStore case:

How Has This Been Tested?

1. Unit tests have been added for the new code where possible.
2. I tested both HyperStore and MinIO environments. I tested updating and making changes to the names, passwords, urls, ports etc and confirmed that you get feedback when the configuration is bad and it updates correctly when the configuration is good.
3. I ran all the CloudStack Unit tests with a clean build.
4. I tested adding/removing buckets after changing the object storage details. For MinIO, I created new credentials and deleted the old ones and then updated using the GUI.

How did you try to break this feature and the system with this change?

Tested as above.

[codecov[bot]]

Codecov Report

:x: Patch coverage is 69.09091% with 34 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 17.51%. Comparing base (a137283) to head (ac35460). :warning: Report is 288 commits behind head on main.

Files with missing lines	Patch %	Lines
...river/CloudianHyperStoreObjectStoreDriverImpl.java	65.51%	10 Missing :warning:
...ain/java/com/cloud/storage/StorageManagerImpl.java	82.50%	2 Missing and 5 partials :warning:
...mand/admin/storage/UpdateObjectStoragePoolCmd.java	76.47%	3 Missing and 1 partial :warning:
...oudstack/storage/object/store/ObjectStoreImpl.java	0.00%	3 Missing :warning:
...ge/datastore/driver/CephObjectStoreDriverImpl.java	0.00%	3 Missing :warning:
...e/datastore/driver/MinIOObjectStoreDriverImpl.java	0.00%	3 Missing :warning:
...tastore/driver/SimulatorObjectStoreDriverImpl.java	0.00%	2 Missing :warning:
...ain/java/com/cloud/api/query/QueryManagerImpl.java	66.66%	0 Missing and 1 partial :warning:
...om/cloud/metadata/ResourceMetaDataManagerImpl.java	0.00%	1 Missing :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #10551      +/-   ##
============================================
+ Coverage     17.50%   17.51%   +0.01%     
- Complexity    13924    13939      +15     
============================================
  Files          5318     5318              
  Lines        474629   474685      +56     
  Branches      55748    55756       +8     
============================================
+ Hits          83070    83139      +69     
+ Misses       382279   382259      -20     
- Partials       9280     9287       +7     

Flag	Coverage Δ
unittests	17.51% <69.09%> (+0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bernardodemarco]

@blueorangutan package

[blueorangutan]

@bernardodemarco a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12769

[tpodowd]

Thanks @abh1sar for the review and your questions.

1. Should we allow the url of the object store to be updated? Because to me it looks similar to deleting an object store and adding a new one. Currently we don't allow deleting an object store if there are buckets associated with it. But, there is no such check now.

I believe this is desirable. This allows the admin to update the protocol from http to https or change the port or go from an IP to a hostname or a different IP. Usually once everything is settled, it won't be used (if only rarely). If the admin changes the object store IP and had registered the IP here, all the existing buckets would stop working and he could not delete the buckets using the GUI or remove the object storage in order to re-add it. This lets you update the IP directly here and keep all your buckets functioning. The alternative would be updating the DB directly I guess in such cases.

2. Does list resource details really need to return the api key - secret key? Because the secret key is visible in the logs due to it.

My observation is that secret keys seem to be in the logs a lot more than just for this. That is no excuse of course and I think that logging in general with regards to object storage at least needs looking at in order to remove all the places where keys are logged.

Back to this PR, when any value is updated the UI also sends credentials back with any new details and the connection is validated. The credentials it sends could be the existing credentials untouched by the admin or new credentials. It is desirable to be able to set new credentials as permanent credentials are often rotated for security reasons. I don't think it is too bad currently as it is except for the logging as this UI is just for administrators.

However, I guess it is not 100% necessary for the server to send the old credentials to the UI and that if the UI didn't specify credentials, the server could merge the existing credential details back into any new details the UI did change. This would make the credentials non-mandatory on the UI side and a merge would be needed as well as stripping out the credentials from the details in the initial response. A little complex perhaps. Another approach could be to leave the credentials as mandatory in the UI and have the administrator provide them (without the UI pre-filling them out) such that any update to the cloud storage always required credentials. This is preferable to me I think but it is more hassle for the administrator perhaps.

Another issue with this is that I'm also not sure how to filter out the credentials from the details as the API that is used for getting the details is generic. Thanks again for looking at this and let me know what you think.

[abh1sar]

I believe this is desirable. This allows the admin to update the protocol from http to https or change the port or go from an IP to a hostname or a different IP. Usually once everything is settled, it won't be used (if only rarely). If the admin changes the object store IP and had registered the IP here, all the existing buckets would stop working and he could not delete the buckets using the GUI or remove the object storage in order to re-add it. This lets you update the IP directly here and keep all your buckets functioning. The alternative would be updating the DB directly I guess in such cases.

You are right.

However, I guess it is not 100% necessary for the server to send the old credentials to the UI and that if the UI didn't specify credentials, the server could merge the existing credential details back into any new details the UI did change. This would make the credentials non-mandatory on the UI side and a merge would be needed as well as stripping out the credentials from the details in the initial response. A little complex perhaps.

I would prefer this if it's not too much hassle.

[tpodowd]

Hi @abh1sar - you mentioned:

I would prefer this if it's not too much hassle.

Truth be told, I don't currently have very much bandwidth to look into this particular change. It will likely be on the back burner for a while until I can redirect my attention at it again. If you have any advice to implement this change also, please add it here in the meantime. Thanks.

[abh1sar]

Truth be told, I don't currently have very much bandwidth to look into this particular change. It will likely be on the back burner for a while until I can redirect my attention at it again. If you have any advice to implement this change also, please add it here in the meantime. Thanks.

@tpodowd it's ok. I can raise a PR to fix it once this PR is merged.

[tpodowd]

Hi @abh1sar - Thanks! That would be great. If you do create a PR, please mention me on it so that I can also test against HyperStore.

[github-actions[bot]]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[sureshanaparti]

Hi @tpodowd Can you resolve the conflicts.

[tpodowd]

Hi @sureshanaparti - thanks for pointing this out. I'll try to get to this next week. Seems like it is one file but I think it warrants a local test also.

[DaanHoogland]

@tpodowd , I took the liberty. It looked quite trivial.

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 13668

[github-actions[bot]]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[github-actions[bot]]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.