Skip to content

Azure: Avoid depending on KeyWrapAlgorithm in AzureProperties#16186

Open
ebyhr wants to merge 2 commits intoapache:mainfrom
ebyhr:ebi/azure
Open

Azure: Avoid depending on KeyWrapAlgorithm in AzureProperties#16186
ebyhr wants to merge 2 commits intoapache:mainfrom
ebyhr:ebi/azure

Conversation

@ebyhr
Copy link
Copy Markdown
Member

@ebyhr ebyhr commented May 1, 2026

Without this change, any downstream application that uses AzureProperties will encounter a NoClassDefFound error.
Changing the return type is safe because Iceberg 1.10.x does not include #13186

properties.getOrDefault(
AzureProperties.AZURE_KEYVAULT_KEY_WRAP_ALGORITHM,
KeyWrapAlgorithm.RSA_OAEP_256.getValue());
properties.getOrDefault(AzureProperties.AZURE_KEYVAULT_KEY_WRAP_ALGORITHM, "RSA-OAEP-256");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we extract this into a named constant with a comment noting which Azure SDK value it mirrors? Something like:

> // Must match KeyWrapAlgorithm.RSA_OAEP_256.getValue() from azure-security-keyvault-keys
> private static final String DEFAULT_KEY_WRAP_ALGORITHM = "RSA-OAEP-256";
>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, updated.

Copy link
Copy Markdown
Contributor

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

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

The refactor make sense to me in general since this is the compile time dependency in iceberg-azure, though i am curious its implementation of iceberg-azure-bundle does trino doesn't depends on the bundle ?

@ebyhr
Copy link
Copy Markdown
Member Author

ebyhr commented May 4, 2026

@singhpk234 Trino Iceberg connecrtor doesn't use iceberg-azure module. It leads to a build failure with Maven duplicate-finder plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants