[Extensions] Enable CLI extensions to include packages in the 'azure' namespace

[zooba]

Description

Extensions currently cannot modules in the azure namespace, which prevents us building extensions to support preview SDKs that are not yet part of the CLI.

This change extends the submodule import path for both azure and azure.mgmt modules when adding an extension to sys.path. This allows extensions to include submodules that do not exist in the CLI's base install (if they do exist, they'll be preferred).

Testing Guide

Create an extension with a dependency on, say, azure-storage-file-share and include import azure.storage.fileshare.

Add the extension and try to invoke its command. It will fail without this fix, and succeed with it.

---

This checklist is used to make sure that common guidelines for a pull request are followed.

• The PR title and description has followed the guideline in Submitting Pull Requests.

• I adhere to the Command Guidelines.

[yonzhan]

add to S170

[zooba]

Not sure why CI isn't running, but I'm going to stop trying. Feel free to do whatever you need to wake it up.

[yonzhan]

@fengzhou-msft could you help?

[noelbundick]

@yonzhan @fengzhou-msft

Any updates on this? My extension is blocked from using the latest SDK's until this issue is resolved

[fengzhou-msft]

@zooba can you take a look at the CI failure? test_add_extension_azure_to_path failed.

[fengzhou-msft]

LGTM, will merge after CI pass.

[fengzhou-msft]

@zooba The CI log shows azure.mgmt.__path__ is an object of _NamespacePath. The old path will have the new value when the object gets updated.

Suggested change

	old_path_1 = azure.__path__
	old_path_2 = azure.mgmt.__path__
	old_path_1 = list(azure.__path__)
	old_path_2 = list(azure.mgmt.__path__)

[fengzhou-msft]

I used pytest to test it directly, the issue does not exist and debug shows azure.mgmt.__path__ is a list. CI uses tox and it fails.

[zooba]

It's going to depend on whether you've used the pkg_resources call in your __init__.py files. You really shouldn't need those anywhere, though you do need to be careful about not using old versions of the tools, because some of those still assume they're necessary.

You also need to be careful about not putting an __init__.py anywhere during development, as that will prevent you importing the other copies elsewhere on sys.path. Or you can use the same __path__ trick to make local development easier. (Installs with pip use the nspkg packages to ensure the final directory structure is correct, and installs with setuptools are not supported.)

[fengzhou-msft]

We do use pkg_resources now and we're planning to follow PEP 420 to remove it. This does not affect the __path__ trick you add, right? If so, let's fix the CI tests to unblock this PR first.

[jiasli]

Linking to #13293 to following PEP 420

[zooba]

The __path__ trick is only necessary with the nspkg packages, which I expect you'll keep to avoid the hijacking issues I mentioned below.

If you remove those, then yeah, extensions can just use the normal hijacking approach to extend the azure namespace. So no need for this change.

[zooba]

I was wrong above - it's a _NamespacePath because of PEP 420. So because it's no longer a real list, we have to remove the extension and trigger an update. Hopefully that will restore the right value.

[jiasli]

This may be relevant to #12778 (comment). Any reason this has to been done in the azure namespace? Why can't the SDK be vendored in the extension?

Another concern is that allowing extensions to override SDKs from venv may cause a security breach - a malicious extension can inject tampered SDKs to intercept the calls thus overriding the behavior, also impacting other command modules that rely on the SDKs.

So as an ultimate solution, the extension should vendor the SDK by putting it under vendored_sdks to avoid the conflict. Then they can use something like from .vendored_sdks.timeseriesinsights.models import LongTermEnvironmentUpdateParameters.

[jiasli]

Please provide more explanations about why this change is necessary.

[Juliehzl]

Actually this function is just used in specific module and doesn't influence all modules.
But one more thing to note is that it seems it only help import azure.mgmt. What about other azure modules such as azure.storage?

[zooba]

It may be worth doing for azure.storage as well, assuming that it's also a faux namespace package that doesn't install an azure/storage/__init__.py file anywhere and it's installed by azure-cli. But apparently that one worked for the user I was working with, even though he was using one of the new azure.storage libraries.

Both azure and azure.mgmt lock down their packages on install by adding the __init__.py file. This prevents applications from hijacking the core libraries via the PYTHONPATH environment variable (because regular namespace packages will merge all matching directory names). It also prevents users from breaking themselves with an azure/ folder in their own sources, as they'll get a much more obvious error. It's only while the SDK team is developing it that you want it to act like a namespace package, as that will save you from having to do an install just to test parts of it.

But once it's locked down, it becomes impossible to use a different part of the namespace separately. And so extension authors have to do the vendoring.

Now, the problem with that is, it's not very discoverable. So while it's a viable solution, you will still have expert developers getting angry at you for making it so hard. When they eventually find the little tiny note hidden away in dev docs, they're not going to be much happier. So your choices are either make the workaround really obvious (put it in every sample, prominent documentation, templates, etc.) or just do it once in your extension loading code and never let anyone worry about it again.

By the way, the "injecting libraries" argument is a red herring - an extension is an injected library. There's no need for more trickery once you're already running arbitrary code. Though advertising how to work around this issue may lead more people to think about actively trying to override parts of the CLI via an extension, whereas simply letting things work won't encourage that and it leaves you with the ability to control the priority (as I've done in this PR, by making the extension's azure submodules lower priority than the ones installed with the CLI).

So it's up to you guys, really. I personally really dislike giving advice on how to work around things that could just be fixed, but I'll certainly do it. And since this one is not very discoverable, if the issue comes up much more then it may be worth publishing a blog post about it - that looks pretty bad, but not as bad as people posting terrible workarounds.

[noelbundick]

@jiasli

Any updates on this? My CLI extensions are still unable to use the latest Azure SDK for Python without terrible workarounds. I do not want to vendor the library and take on the burden of maintenance & updates. Azure SDK's should work seamlessly with the Azure CLI

More and more teams pilot their functionality (AKS, web apps, etc) or completely launch feature sets (Azure ML, AzDO, etc) via CLI extensions. None of those services will be able to use the latest SDK's until this change or something similar is implemented. imo it would be smart to fix this ahead of time so they never notice a problem rather than blocking multiple services on a fix that's already been written

[fengzhou-msft]

@noelbundick I have verified that make azure-cli follow PEP420 would achieve the same goal as this PR. @arrownj has started the work on #13293. The code change is simple, we just need to do more testing.
Update: Draft PR submitted: #13903

[zooba]

@fengzhou-msft Moving entirely to PEP 420 will open up the risk of (intentional or otherwise) package hijacking, so be aware of the security implications, especially since the CLI handles high-value credentials for many users.

Also, you may need to do a manual step to make sure your packaged azure\__init__.py and azure\mgmt\__init__.py are deleted, since those are the problem files. The ones deleted by the PR you linked don't have any impact on people using the SDKs - they'll only affect people trying to hijack CLI modules ;)

[yungezz]

@fengzhou-msft Moving entirely to PEP 420 will open up the risk of (intentional or otherwise) package hijacking, so be aware of the security implications, especially since the CLI handles high-value credentials for many users.

Also, you may need to do a manual step to make sure your packaged azure\__init__.py and azure\mgmt\__init__.py are deleted, since those are the problem files. The ones deleted by the PR you linked don't have any impact on people using the SDKs - they'll only affect people trying to hijack CLI modules ;)

Exactly, we see the risk and evaluating it now. add @arrownj who works on it now.

[fengzhou-msft]

@zooba we will merge this PR first while evalutaing the impact of PEP 420. Can you fix the CI and update the doc? We would still recommend to use vendored SDK if possible.

[zooba]

I'll take a look.

Note that "vendored" SDK is exactly what this enables. I think what you mean is "custom generated SDK", which is only possible for management SDKs (and even then is a really big ask).

dismiss