Conversation
|
|
||
|
|
||
| def get_msi_token(resource, port=50342): | ||
| def get_msi_token(resource, port=50342, client_id=None): |
There was a problem hiding this comment.
FYI, msi's resource id and identity's service principal's object id are also supported
There was a problem hiding this comment.
I saw your email about this naming. Will wait for official suggestion before merging.
yugangw-msft
left a comment
yugangw-msft
left a comment
There was a problem hiding this comment.
Left a few questions, LGTM otherwise
|
|
||
| def _is_app_service(): | ||
| # Might be discussed if we think it's not robust enough | ||
| return 'APPSETTING_WEBSITE_SITE_NAME' in os.environ |
There was a problem hiding this comment.
I am not familiar with this env variable, but I know we have MSI_ENDPOINT and MSI_SECRET to leverage
There was a problem hiding this comment.
When I did the MSI sample of app service, I realized that I need first to detect if I'm on a WebApp. For that, MSI_ENDPOINT is not good (if MSI is not activated).
APPSETTING_WEBSITE_SITE_NAME is always there, it's a WebApp requirement. This makes possible to detect: if it's a WebApp, MSI_ENDPOINT is not there, tell the customer.
There was a problem hiding this comment.
Got it. I don't know whether APPSETTING_WEBSITE_SITE_NAME is reliable moving forward for both Linux and Windows. //CC: @panchagnula
My opinion was it is the client applications' responsibility to ensure the webapp was enabled with MSI before execute the code, so it is fine for our libraries to make such assumption on the existence of env varaibles
There was a problem hiding this comment.
@lmazuel, verify in both linux and windows webapp, APPSETTING_WEBSITE_SITE_NAME does exist in both
|
|
||
| :param str resource: The resource where the token would be use. | ||
| :param int port: The port is not the default 50342 is used. | ||
| :param dict[str, str] msi_conf: msi_conf if User Assigned (if not specified, assume System Assigned) |
There was a problem hiding this comment.
Suggest wording of if to request a token through a user assigned identity. But up to you
| super(MSIAuthentication, self).__init__(None) | ||
|
|
||
| self.port = port | ||
| self.msi_conf = {k:v for k,v in kwargs.items() if k in ["client_id", "object_id", "msi_res_id"]} |
There was a problem hiding this comment.
still debating a bit, but to get interface clear, we should throw when user assigned identity info is supplied for a webapp
There was a problem hiding this comment.
Let's do that for now, but I won't be surprised if this comes to WebApp at some point...
lmazuel
force-pushed
the
explicit_msi
branch
from
a02f183 to
7ebd894
Compare
yugangw-msft
left a comment
yugangw-msft
left a comment
There was a problem hiding this comment.
I cc'd Sisira for the right env variable we can depend on.
|
|
||
| def _is_app_service(): | ||
| # Might be discussed if we think it's not robust enough | ||
| return 'APPSETTING_WEBSITE_SITE_NAME' in os.environ |
There was a problem hiding this comment.
Got it. I don't know whether APPSETTING_WEBSITE_SITE_NAME is reliable moving forward for both Linux and Windows. //CC: @panchagnula
My opinion was it is the client applications' responsibility to ensure the webapp was enabled with MSI before execute the code, so it is fine for our libraries to make such assumption on the existence of env varaibles
|
CC:@ahmedelnably can you comment on the 'APPSETTING_WEBSITE_SITE_NAME' setting if that is a reliable setting we can use. |
|
@panchagnula App Service always set the 'APPSETTING_WEBSITE_SITE_NAME' we should be able to reliably use it |
|
Thanks for your confirmation @ahmedelnably @panchagnula ! |
4 participants
FYI @yugangw-msft