Skip to content

Conversation

@SmitaRKulkarni
Copy link
Member

@SmitaRKulkarni SmitaRKulkarni commented Sep 29, 2025

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

In AzureBlobStorage, updated to try native copy first and go to read & write on 'Unauthroized' error (In AzureBlobStorage, if storage accounts are different for source & destination we get 'Unauthorized' error). And fix applying "use_native_copy" when endpoint is defined in configuration.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@SmitaRKulkarni SmitaRKulkarni marked this pull request as draft September 29, 2025 13:03
@clickhouse-gh
Copy link

clickhouse-gh bot commented Sep 29, 2025

Workflow [PR], commit [93d171b]

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Sep 29, 2025
@jkartseva jkartseva self-assigned this Oct 1, 2025
@SmitaRKulkarni
Copy link
Member Author

After some manual tests I see its not possible to compare authentication methods in Azure Blob Storage especially for WorkloadIdentity (& ManagedIdenitty).
GetToken does not work and an equal comparison also does not work. There is a function to compare AccountInfo, but still does not check if its same account or not.

So I have updated the approach, Now we first try native copy & if we get unauthorized error we go for read & write. I have tested manually with WorkloadIdentity that native copy works.

@SmitaRKulkarni SmitaRKulkarni changed the title [WIP]Fix azure GetToken Fix azure GetToken Oct 1, 2025
@SmitaRKulkarni SmitaRKulkarni marked this pull request as ready for review October 1, 2025 20:43
@SmitaRKulkarni SmitaRKulkarni changed the title Fix azure GetToken Fix AzureBlobStorage copy Oct 1, 2025
Copy link
Member

@jkartseva jkartseva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR relaxes the condition under which native copy is performed, which seems Okay.

Copy link
Member

@jkartseva jkartseva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@SmitaRKulkarni
Copy link
Member Author

SmitaRKulkarni commented Oct 3, 2025

Made a small fix for applying setting "use_native_copy", (https://github.com/ClickHouse/ClickHouse/pull/87826/files#diff-efbc3c03efb87660e06a2c7d91d2de5a25ff91611d5f60167636a7e5d169bd9cR630-R634)

Previously the map contained url (as given in config) & settings. When reading it we use endpoint.storage_account_url to fetch settings. So now I updated it to store endpoint.storage_account_url by getting endpoint from config & settings. So in case endpoint is defined we will still be storing only storage_account_url & read using it as well

@jkartseva
Copy link
Member

@clickhouse-gh clickhouse-gh bot added the submodule changed At least one submodule changed in this PR. label Oct 8, 2025
@jkartseva
Copy link
Member

Let's publish the submodule fix as a separate PR?

@SmitaRKulkarni
Copy link
Member Author

Moved submodule change to different PR : #88278

@SmitaRKulkarni
Copy link
Member Author

Unrelated fails :
Stateless tests (amd_debug, sequential) - 03141_fetches_errors_stress
Bugfix validation (integration tests) - the bug couldn't be seen in integration test, so fix couldn't be verified
AST fuzzer (amd_debug) - Logical error: 'Block structure mismatch in function connect between RemovingSparseTransform and MergeTreeSink stream: different types:

@SmitaRKulkarni SmitaRKulkarni added this pull request to the merge queue Oct 9, 2025
Merged via the queue into master with commit 3886ed3 Oct 9, 2025
119 of 123 checks passed
@SmitaRKulkarni SmitaRKulkarni deleted the Fix_azure_token branch October 9, 2025 19:27
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 9, 2025
@SmitaRKulkarni SmitaRKulkarni added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Oct 9, 2025
@robot-clickhouse robot-clickhouse added pr-backports-created-cloud pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR labels Oct 9, 2025
robot-clickhouse added a commit that referenced this pull request Oct 12, 2025
Cherry pick #87826 to 25.7: Fix AzureBlobStorage copy
robot-clickhouse added a commit that referenced this pull request Oct 12, 2025
Cherry pick #87826 to 25.8: Fix AzureBlobStorage copy
robot-clickhouse added a commit that referenced this pull request Oct 12, 2025
Cherry pick #87826 to 25.9: Fix AzureBlobStorage copy
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Oct 12, 2025
clickhouse-gh bot added a commit that referenced this pull request Oct 12, 2025
Backport #87826 to 25.9: Fix AzureBlobStorage copy
clickhouse-gh bot added a commit that referenced this pull request Oct 12, 2025
Backport #87826 to 25.8: Fix AzureBlobStorage copy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud pr-bugfix Pull request with bugfix, not backported by default pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo submodule changed At least one submodule changed in this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants