KeyVaultEnvironmentPostProcessor.isKeyVaultEnabled should check the enabled flag before checking if AZURE_KEYVAULT_VAULT_URI is populated

[jmax01]

KeyVaultEnvironmentPostProcessor.isKeyVaultEnabled should check the enabled flag before checking if AZURE_KEYVAULT_VAULT_URI is populated.

URIs like AZURE_KEYVAULT_VAULT_URI are often constructed from other properties and in many test scenarios resolution will fail due to those properties not being resolved.

Solution:

Reorder and simplify isKeyVaultEnabled's conditionals.

https://github.com/microsoft/azure-spring-boot/blob/4543480ed26976dfb9eec71375855da5ed24fdc6/azure-spring-boot/src/main/java/com/microsoft/azure/keyvault/spring/KeyVaultEnvironmentPostProcessor.java#L31-L38

Patch in PR:

    private boolean isKeyVaultEnabled(ConfigurableEnvironment environment) {
        return environment.getProperty(Constants.AZURE_KEYVAULT_ENABLED, Boolean.class, true)
                && environment.getProperty(Constants.AZURE_KEYVAULT_VAULT_URI) != null && isKeyVaultClientAvailable();
    }

[saragluna]

Thanks for bringing this up. Could you share more details about the test scenarios when resolution will fail due to the URI?

[jmax01]

Our default config defines the current pattern used for our keyvault instance,

We use profiles to set the referenced properties.

We were hoping to avoid having another profile to set the uri and we want to be able to test construction of the uri without having the KeyVaultEnvironmentPostProcessor start.

The patch fixes this issue.

#default config
#this currently fails
azure.keyvault.enabled=false
azure.keyvault.uri=https://${subscription.prefix}-${env.prefix}-${region.code}.vault.azure.net

[saragluna]

@jmax01 Thanks for your response and just for clarification:

When the user specifies azure.keyvault.enabled to false, the KeyVaultEnvironmentPostProcessor should not be started. In which case, the below code snippet does return false. Or do I miss something?

https://github.com/microsoft/azure-spring-boot/blob/4543480ed26976dfb9eec71375855da5ed24fdc6/azure-spring-boot/src/main/java/com/microsoft/azure/keyvault/spring/KeyVaultEnvironmentPostProcessor.java#L31-L38

[jmax01]

should not be started. In which case, the below code snippet does return false. Or do I miss something?

If azure.keyvault.enabled is false KeyVaultEnvironmentPostProcessor should not attempt to resolve AZURE_KEYVAULT_VAULT_URI.

Processing should cease immediately.

To do otherwise is, in my opinion, is a violation of the principle of least surprise

[jmax01]

The issue still exists in the new implementation. I will supply a new PR.

[chenrujun]

This problem already fixed in our new repo, so I'll close this issue.