search

pulumi / pulumi-azure-native

Azure Native Provider
Apache License 2.0
126 stars 34 forks source link

keyvault.get_vault throws exception when vault does not exist #1072

Open jancespivo opened 3 years ago

jancespivo commented 3 years ago

When keyvault.get_vault tries to get non-existing vault the function throws error:

      File ".venv/lib/python3.9/site-packages/pulumi_azure_native/keyvault/get_vault.py", line 127, in get_vault
        id=__ret__.id,
    AttributeError: 'NoneType' object has no attribute 'id'
    error: an unhandled error occurred: Program exited with non-zero exit code: 1

IMO it should return None instead.

mikhailshilkov commented 3 years ago

Do you have an example of a function that behaves as you would expect in this scenario?

jancespivo commented 3 years ago

@mikhailshilkov Actually it is ok to expect this kind of error: Exception: invoke of azure-native:keyvault:getVault failed: invocation of azure-native:keyvault:getVault returned an error: request failed ...

I've found:

keyvault.get_vault(
    resource_group_name='existing_rg_name',
    vault_name='not_existing_vault_name',
)

works properly.

However:

keyvault.get_vault(
    resource_group_name='existing_rg_name',
    vault_name='', #<-- empty!
)

fails with the exception I reported.

Thx!

mikhailshilkov commented 1 year ago

I spent some time looking into what is going on and a potential fix here.

The sequence basically goes like this:

  1. An empty name is passed to vault_name.
  2. We build an Azure URL with it. vaultName URL segment is empty, so we end up with something like /subscriptions/12344/resourceGroups/rg/providers/Microsoft.KeyVault/vaults/?api-version=2023-02-01.
  3. It's actually a valid ARM URL! The service responds with a 200 with a list of vaults.
  4. We don't find any properties of a vault, so we end up returning a vault with no values.
  5. Python SDK freaks out about that and fails with an exception.

We could make it more strict and catch the problem on a few different levels:

  1. Don't allow empty values for required properties.
  2. Don't allow empty values for properties that are used to build up the URL.
  3. Require id to be inside the response payload to consider it successful.

I don't feel good about any of these changes, actually. I can't confidently tell that any of (1) to (3) wouldn't have a side effect and wouldn't break some legit scenario that works today. What if there are scenarios where an empty value is a valid value?

I'll mark this issue as an enhancement and label it as 3.0. We can entertain doing (1) or (2) then and clearly mark it as a breaking change.

cleverguy25 commented 3 months ago

Added to epic https://github.com/pulumi/home/issues/3552