tpodowd commented on PR #10551: URL: https://github.com/apache/cloudstack/pull/10551#issuecomment-2731390777
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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
