App Config Provider Module Level Load #28241

mrm9084 · 2023-01-09T22:12:50Z

Description

Moves the load method of AppConfigurationProvider from a class method to a module method.

All SDK Contribution checklist:

The pull request does not introduce [breaking changes]
CHANGELOG is updated for new features, bug fixes or other significant changes.
I have read the contribution guidelines.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

…yVaultOptions to be Keyword only.

azure-sdk · 2023-01-09T22:36:58Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-appconfiguration-provider

...appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/__init__.py

...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py

annatisch · 2023-01-09T23:47:08Z

...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py

-
-class AzureAppConfigurationProvider:
+#pylint:disable=protected-access
+def loadProvider(*, connection_string=None, endpoint=None, credential=None, **kwargs):


Type hints should go in the method signature, and we should also add overloads.
The Provider object itself currently has no methods outside of the implemented protocols - so we can simply document the return type as a Mapping. However once we start to add things like the refresh, which may expose other methods, we would need to update this (so that the ref docs link correctly).

Suggested change

def loadProvider(*, connection_string=None, endpoint=None, credential=None, **kwargs):

@overload

def load_provider(endpoint: str, credential: TokenCredential, **kwargs) -> Mapping[str, str]:

...

@overload

def load_provider(connection_string: str, **kwargs) -> Mapping[str, str]:

...

def load_provider(*args, **kwargs) -> Mapping[str, str]:

I have made some of this modification, but something seems wrong. endpoint and the other method variables are put into kwargs, not args.

Also, it seems like Mapping isn't a typing type. I tried making a new one but it didn't work.

annatisch · 2023-01-09T23:51:07Z

...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py

+        return key_vault_options.secret_resolver(config.secret_id)

-        key_vault_identifier = KeyVaultSecretIdentifier(config.secret_id)
+    raise AttributeError(


Not sure if AttributeError is the right pick here - but we can take a closer look at that later.

...appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/__init__.py

annatisch · 2023-01-09T23:58:24Z

...figuration-provider/azure/appconfiguration/provider/_azureappconfigurationkeyvaultoptions.py

-    def __init__(self, credential=None, secret_clients=None, secret_resolver=None):
-        # type: (TokenCredential, List[SecretClient], Callable) -> None
+    def __init__(self, *, credential=None, secret_clients=None, secret_resolver=None):
+        # type: (TokenCredential, List[SecretClient], Callable[[str], str]) -> None


We should use inline type hints and overloads (though I can't see where the Vault endpoint comes from?):

@overload def __init__(self, credential: TokenCredential,*, secret_resolver: Optional[Callable[[str], str]] = None) -> None ... @overload def __init__(self, secret_client: List[SecretClient], *, secret_resolver: Optional[Callcable[[str], str]] = None) -> None ...

If I know what you are talking about, the "vault endpoint" comes from App Configuration inside of the key vault reference.

Also, the options here are, Credential with List of Clients, or List of Clients with Secret Resolver.

The idea is that we default either to the TokenCredential provided or the Secret Resolver.

I also ran into an issue with TokenCredential it works fine locally, but for some reason it breaks in the build pipeline, I got ImportError: cannot import name 'TokenCredential'. I was using from azure.core.credential import TokenCredential. https://dev.azure.com/azure-sdk/public/_build/results?buildId=2105499&view=ms.vss-test-web.build-test-results-tab&runId=38663166&resultId=100000&paneView=debug

annatisch · 2023-01-10T00:00:44Z

...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py

-        sub_types = sub_type.split("+")
-        if "json" in sub_types:
-            return True
+class AzureAppConfigurationProvider:


Given that this object holds an instance of a client, we will need to expose this as a context manger and allow it to be closeable (such that the connection can be cleaned up).
This means that we will probably need to still keep the object public so that it can be properly documented....

Well, this would get a bit awkward when it comes to refresh. Typically it's best to reuse the same clients as it is a costly operation to create new ones.

Also, how would this work given we have one to many clients, for App Configuration (will be with geo-replication) and Key Vault.

...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py

...figuration-provider/azure/appconfiguration/provider/_azureappconfigurationkeyvaultoptions.py

...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py

avanigupta · 2023-01-26T23:31:17Z

...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py

+    if key_vault_options and (
+        key_vault_options.credential or key_vault_options.secret_clients or key_vault_options.secret_resolver
+    ):
+        correlation_context += ",UsesKeyVault"


So we are adding RequestType and UsesKeyVault tracing right now. Is there a way to get host type and dev environment tracing for python apps?

It was already there. This was just moved. I can work on adding those, but not in this branch.

avanigupta · 2023-01-26T23:34:43Z

...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py

+
+            trimmed_key = config.key
+            # Trim the key if it starts with one of the prefixes provided
+            for trim in provider._trim_prefixes:


In .NET, we process KeyVault/Json types first, then trim prefixes. Can we do the same here to maintain the same behavior? Changing the order might produce different results, especially when we start supporting feature flags.

If you look down a bit, the trimmed_key is only used in the dictionary, _resolve_keyvault_reference uses the config which contains the untrimmed key.

Just thinking about how feature flags would look in future - we'd want to maintain the same schema that we use in .net and spring providers, right? So for a feature "Beta", dictionary key would be something like "FeatureManagement:Beta" (or whatever separator makes sense for python).
Since feature flag flattening changes the key being added to dictionary, we shouldn't use trimmed_key as dictionary key.

I see two options:

elif config.content_type == FeatureFlagConfigurationSetting._feature_flag_content_type: #pylint: disable=protected-access feature_management = provider._dict.get("FeatureManagement", {}) feature_management[trimmed_key] = config.value

or

elif config.content_type == FeatureFlagConfigurationSetting._feature_flag_content_type: #pylint: disable=protected-access provider._dict["FeatureManagement." + trimmed_key] = config.value

Discussed with Matt offline. Since the config dictionary structure is not limited to <string,string> in python, what you have right now should be good.

mrm9084 · 2023-01-31T18:30:18Z

Closing, I think I have gotten all of the comments from here and moved them too #28497.

mrm9084 added 5 commits January 5, 2023 16:03

Removes custom exception. Adds missing dictionary methods. Changed Ke…

1c7ba08

…yVaultOptions to be Keyword only.

Update CHANGELOG.md

27661f2

Merge branch 'main' into AppConfigProviderArchReviewChanges

5296487

Moving load logic out of AzureAppConfigurationProvider

f4f7a11

Update CHANGELOG.md

d74cb02

ghost added the App Configuration Azure.ApplicationModel.Configuration label Jan 9, 2023

annatisch reviewed Jan 10, 2023

View reviewed changes

mrm9084 added 5 commits January 10, 2023 12:20

Updating syntax

1c89e43

Fixed import

c8b3c8f

Fixing Lint issue

3d377cd

Fixed Spaces

78c59f6

Temp removing TokenCredential hint

d263059

albertofori reviewed Jan 10, 2023

View reviewed changes

...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py Show resolved Hide resolved

avanigupta reviewed Jan 26, 2023

View reviewed changes

...figuration-provider/azure/appconfiguration/provider/_azureappconfigurationkeyvaultoptions.py Show resolved Hide resolved

avanigupta reviewed Jan 26, 2023

View reviewed changes

...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py Show resolved Hide resolved

avanigupta reviewed Jan 26, 2023

View reviewed changes

Adding validation to KeyVaultOptions

cfdf398

avanigupta approved these changes Jan 30, 2023

View reviewed changes

mrm9084 closed this Jan 31, 2023

-def loadProvider(*, connection_string=None, endpoint=None, credential=None, **kwargs):
+@overload
+def load_provider(endpoint: str, credential: TokenCredential, **kwargs) -> Mapping[str, str]:
+    ...
+ @overload
+ def load_provider(connection_string: str, **kwargs) -> Mapping[str, str]:
+     ...
+ def load_provider(*args, **kwargs) -> Mapping[str, str]:

App Config Provider Module Level Load #28241

App Config Provider Module Level Load #28241

Uh oh!

Conversation

mrm9084 commented Jan 9, 2023

Description

All SDK Contribution checklist:

General Guidelines and Best Practices

Uh oh!

azure-sdk commented Jan 9, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrm9084 commented Jan 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants