[Container Registry] Anonymous Access Client #18550
Conversation
|
Dependent on #18392 |
|
/azp run python - containerregistry - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - containerregistry - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| super(ContainerRegistryChallengePolicy, self).__init__() | ||
| self._credential = credential | ||
| self._exchange_client = ACRExchangeClient(endpoint, self._credential) | ||
| if self._credential is None: |
There was a problem hiding this comment.
Shouldn't this check move before line 28? Otherwise we are building the ACRExchangeClient for nothing?
There was a problem hiding this comment.
Yes I can move it, I originally didn't like the if/else flow but I don't see a way around it with this design.
| AUTHENTICATION_CHALLENGE_PARAMS_PATTERN = re.compile('(?:(\\w+)="([^""]*)")+') | ||
|
|
||
| def __init__(self, endpoint, credential=None, **kwargs): | ||
| # type: (str, TokenCredential, Dict[str, Any]) -> None |
There was a problem hiding this comment.
I'm curious why this client takes a credential at all if it must be None?
At a minimum, that type hint should probably be Optional[...]
There was a problem hiding this comment.
It definitely does not need to be there, I will remove it. Thank you
| from azure.core.credentials import TokenCredential | ||
|
|
||
|
|
||
| PASSWORD = u"password" |
There was a problem hiding this comment.
Isn't there already a generated enum for this?
There was a problem hiding this comment.
Yes, added in
| """ | ||
|
|
||
| BEARER = "Bearer" | ||
| AUTHENTICATION_CHALLENGE_PARAMS_PATTERN = re.compile('(?:(\\w+)="([^""]*)")+') |
There was a problem hiding this comment.
These seem to be defined on all 4 ExchangeClients - perhaps they could be refactored out somewhere?
Not a big deal - doesn't need to be changed now.
There was a problem hiding this comment.
They can all be removed, I parse the challenge policy in a separate file with everything properly scoped
| if not endpoint.startswith("https://") and not endpoint.startswith("http://"): | ||
| endpoint = "https://" + endpoint | ||
| self._endpoint = endpoint | ||
| self._credential_scope = "https://management.core.windows.net/.default" |
There was a problem hiding this comment.
This could probably be a constant somewhere to be shared between the ExchangeClients
There was a problem hiding this comment.
The credential scope needs to be fixed in the next release to handle foreign clouds as well. I'm going to address this in our next beta.
mccoyp
left a comment
mccoyp
left a comment
There was a problem hiding this comment.
Nitpicks more than anything -- looks good!
|
|
||
| def __init__(self, endpoint, credential=None, **kwargs): | ||
| # type: (str, TokenCredential, Dict[str, Any]) -> None | ||
| if not endpoint.startswith("https://") and not endpoint.startswith("http://"): |
There was a problem hiding this comment.
This feels like it could be veering into client-side validation territory, but if there are other clients that do this then it's probably okay for now
There was a problem hiding this comment.
I believe we do this in all four tier-1 languages, and the biggest reason being the value given in the portal when you create an account does not prefix the endpoint with https:// or http://
| # ------------------------------------ | ||
| from enum import Enum | ||
| from typing import TYPE_CHECKING | ||
| from typing import TYPE_CHECKING, Dict, Any |
There was a problem hiding this comment.
nit: it looks like Dict and Any can just be imported if type checking
| AUTHENTICATION_CHALLENGE_PARAMS_PATTERN = re.compile('(?:(\\w+)="([^""]*)")+') | ||
|
|
||
| def __init__(self, endpoint: str, credential: "AsyncTokencredential" = None, **kwargs: Dict[str, Any]) -> None: | ||
| if not endpoint.startswith("https://") and not endpoint.startswith("http://"): |
There was a problem hiding this comment.
Same comment about parameter validation, but I realize that this may be kind of ACR-specific
|
|
||
| if "seankaneanon.azurecr.io" in request.uri: | ||
| request.uri = request.uri.replace("seankaneanon.azurecr.io", "fake_url.azurecr.io") | ||
| if "seankaneanon.azurecr.io" in request.url: |
There was a problem hiding this comment.
Looks like an accidental duplication
There was a problem hiding this comment.
The scrubbers are a work in progress, but one checks the uri and the other checks the url
There was a problem hiding this comment.
Ah, good point! I knew there must be a character difference in there somewhere that I was missing
…into fix_confidentialledger_aiohttp * 'master' of https://github.com/Azure/azure-sdk-for-python: regenerate (Azure#18647) [formrecognizer] remove polling interval from doc comments (Azure#18645) [formrecognizer] regenerates on v2.1 (Azure#18551) adding images to anonymous client (Azure#18646) Confidential Ledger: Update README samples and CHANGELOG (Azure#18644) [Container Registry] Anonymous Access Client (Azure#18550)
3 participants
No description provided.