-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ESTS-Regional support #306
Conversation
apps/confidential/confidential.go
Outdated
@@ -245,6 +253,23 @@ func WithX5C() Option { | |||
} | |||
} | |||
|
|||
// WithAzureRegionsets the region(preferred) or Confidential.AutoDetectRegion() for auto detecting region. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after "WithAzureRegion"
err := c.Comm.JSONCall(ctx, endpoint, http.Header{}, qv, nil, &resp) | ||
return resp, err | ||
resp := "" | ||
err := c.Comm.JSONCall(ctx, imdsEndpoint, http.Header{}, nil, nil, &resp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qv isn't being passed in here anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
qv.Set("api-version", defaultAPIVersion) | ||
qv.Set("format", "text") | ||
resp := "" | ||
err := c.Comm.JSONCall(ctx, imdsEndpoint, http.Header{}, qv, nil, &resp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
design doc indicates that this call should be retried once.
Additionally, do we want to handle discovery here for new versions? Specified as optional in the design doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AzureAD/microsoft-authentication-library-for-python#358 (comment)
I was following this discussion and decided to go with the simpler approach of trying it only once.
I'll update to use the latest API version though as per this doc. https://docs.microsoft.com/en-us/azure/virtual-machines/windows/instance-metadata-service?tabs=linux#supported-api-versions
} | ||
resp.Metadata = []InstanceDiscoveryMetadata{metadata} | ||
} | ||
if region == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
am I reading this wrong, or could this just be an else?
Kudos, SonarCloud Quality Gate passed!
|
This resolves #240
This is the internal design doc: AAD SDK Proposal to Pin Auth to region
I was able to E2E test this by specifying an Azure Region using this sample app.
Also added a wiki page to show usage details.