Skip to content

Complete azure-identity prototype#5547

Merged
chlowell merged 14 commits intoAzure:masterfrom
chlowell:identity
Jun 3, 2019
Merged

Complete azure-identity prototype#5547
chlowell merged 14 commits intoAzure:masterfrom
chlowell:identity

Conversation

@chlowell
Copy link
Member

@chlowell chlowell commented May 30, 2019

This implements the rest of the azure-identity prototype introduced by #5246, adding the remaining credentials with unit tests. With this the package supports sync and async AAD authentication with managed identity or with a certificate or client secret. MSAL is used for token caching and signing the JWT assertion used in cert authentication.

Future work for future PRs includes more documentation, live tests, and maybe more shared code between credentials and between a/sync equivalents. Suggestions around the latter are welcome. Sharing more code is possible but I found everything I tried too difficult to read.

Closes #5251, closes #5149, closes #5150

@chlowell chlowell added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels May 30, 2019
@adxsdk6
Copy link

adxsdk6 commented May 30, 2019

Can one of the admins verify this patch?

@chlowell chlowell self-assigned this May 30, 2019
@chlowell
Copy link
Member Author

/azp run azure-sdk-for-python - client

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chlowell chlowell requested a review from johanste May 31, 2019 00:59
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

404 [](start = 35, length = 3)

Why are you retrying on 404?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right? According to the IMDS docs the endpoint returns 404 when it's updating, and clients should retry.

Copy link
Member

@johanste johanste Jun 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of having to create a list with >100 items. There are some things we can do to improve. Just off the top of my head, something like:

class StatusCodeRange(object):

    def __init__(self, min_value, max_value):
        self.min_value = min_value
        self.max_value = max_value

    def __eq__(self, other):
        return self.min_value <= other and other <= self.max_value


scr = StatusCodeRange(500, 600)

assert 503 in [404, 429, scr]
assert 499 not in [1, 2, scr]

It does bastardize the eq method, though, so another version would be to change the simple in check in the retry policy to understand ranges as well...

class SuperRange(object):

    def __init__(self, *values):
        self.values = values

    def __contains__(self, value):
        for value_in_range in self.values:
            try:
                if value_in_range[0] <= value and value <= value_in_range[1]:
                    return True
            except TypeError:
                if value_in_range == value:
                    return True
        return False


sur = SuperRange(404, 429, (500, 600))


assert 503 in sur
assert 499 not in sur

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right? According to the IMDS docs the endpoint returns 404 when it's updating, and clients should retry.

Per the linked documentation (great find, btw :)), we should make sure our retry policy follows the suggested timeouts by default. We are trying to make sure that our libraries "do the right thing" (tm).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have done. I put #5628 on the backlog to track retry code range support.

@chlowell
Copy link
Member Author

chlowell commented Jun 3, 2019

I plan to merge this once CI is green and open another PR to incorporate the architecture board's feedback.

@chlowell chlowell merged commit b93fd3e into Azure:master Jun 3, 2019
rajivnandivada pushed a commit to rajivnandivada/azure-sdk-for-python that referenced this pull request Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Identity Client This issue points to a problem in the data-plane of the library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement certificate credential provider Credential provider classes Credential classes

5 participants