Don't attempt to create container in Azure BackupReader #62989
Conversation
| @@ -37,7 +37,7 @@ BackupReaderAzureBlobStorage::BackupReaderAzureBlobStorage( | |||
| , data_source_description{DataSourceType::ObjectStorage, ObjectStorageType::Azure, MetadataStorageType::None, configuration_.container, false, false} | |||
| , configuration(configuration_) | |||
| { | |||
| auto client_ptr = StorageAzureBlob::createClient(configuration, /* is_read_only */ false); | |||
| auto client_ptr = StorageAzureBlob::createClient(configuration, /* is_read_only */ true, /*attempt_to_create_container*/ false); | |||
There was a problem hiding this comment.
is_read_only flag is intended to check if we need to create container or not. If its a read only operation, then we don't create container, and if its not read only, then if needed container is created.
I am not clear what is the use of having another flag attempt_to_create_container , could you please clarify this?
There was a problem hiding this comment.
The flag attempt_to_create_container is intended to prevent ClickHouse from using operations that require permissions at the storage account level, such as list containers/create container, and just trust that the container is created already. The name is a bit confusing because I didn't realize that the the backup reader also used list containers in #61785.
We can rename it or tbh just remove it because probably it was a bit excessive to assume such a restricted level of access to the blob service. If you prefer this setting to not be there at all, I can close this PR and open a new one to remove the setting. And while at it, I can document the permissions that ClickHouse needs, is there a place in the docs where that would belong?
There was a problem hiding this comment.
Thanks for the clarification. Yes I would suggest we remove this setting as currently we have 2 parameters to a function which do the same thing.
And while at it, I can document the permissions that ClickHouse needs, is there a place in the docs where that would belong?
https://clickhouse.com/docs/en/engines/table-engines/integrations/azureBlobStorage
Follow-up to #61785, don't require permissions at the storage account level when reading backups either (including for writing incremental backups).
Changelog category (leave one):