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
[ServiceConnector] az webapp/spring-cloud connection
: New command group to support service to service connection
#19834
Conversation
ServiceConnector |
AUTH_TYPE.Secret: '--secret name=XX secret=XX', | ||
AUTH_TYPE.SecretAuto: '--secret', | ||
AUTH_TYPE.SystemIdentity: '--system-identity', | ||
AUTH_TYPE.ServicePrincipalSecret: '--service-principal client-id=XX object-id=XX secret=XX', |
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.
Do we support serviceprincipal certificate in CLI?
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.
why need both client id and object id? I mean can we get one from another?
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.
why need both client id and object id? I mean can we get one from another?
Yes, tenant_id
+ client_id
could result in a unique object_id
. If we provide only client_id
, we have to use default tenant, which would sacrifice the cross-tenant ability.
So, i prefer we leave both of them.
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.
Do we support serviceprincipal certificate in CLI?
Not now, we may support this if there's customer ask in the future.
from ._utils import run_cli_cmd | ||
|
||
def _infer_webapp(source_id): | ||
value_type_map = { |
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.
are they case sensitive?
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.
Yes, they are converted to lower cases when comparing.
|
||
all_auth_info = [] | ||
if secret_auth_info is not None: | ||
all_auth_info.append(secret_auth_info) |
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.
what happens if user input has more than one auth_info
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.
We will check it, and an error will be throwed if there are mutiple auth info provided
'and then create the connection. Error details: {}'.format(str(err))) | ||
|
||
target_id = self.get_target_id() | ||
logger.warning('Created, the resource id is: %s', target_id) |
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.
pls help to make the message specific clear, <target_resource_type> is created
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.
Before this log, another log called Start creating a <target resource>
will be print, so i think it's clear enough here
try: | ||
run_cli_cmd(cmd) | ||
except CLIInternalError: | ||
logger.warning('Rollback failed, please manually check and delete the unintended resources ' |
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.
with resource id?
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.
sure
def _retrive_source_rg(self): | ||
'''Retrive the resource group name in source resource id | ||
''' | ||
regex = '.*/resourceGroups/([^/]*)' |
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.
pls use the common lib in msrest.azure to parse resource id
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.
good point
AUTH_TYPE.Secret: '--secret name=XX secret=XX', | ||
AUTH_TYPE.SecretAuto: '--secret', | ||
AUTH_TYPE.SystemIdentity: '--system-identity', | ||
AUTH_TYPE.ServicePrincipalSecret: '--service-principal client-id=XX object-id=XX secret=XX', |
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.
why need both client id and object id? I mean can we get one from another?
Go = 'go' | ||
Php = 'php' | ||
Ruby = 'ruby' | ||
SpringBoot = 'springBoot' |
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.
what's the convention? all small case or camel?
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.
These values are defined in swagger, i think they are camel
def _infer_springcloud(source_id): | ||
client_type = None | ||
try: | ||
regex = '*/resourceGroups/([^/]*)/providers/Microsoft.AppPlatform/Spring/([^/]*)/apps/([^/]*)/*' |
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.
pls use the common lib, avoid regex :)
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.
sure~
@houk-ms Do you need to update CODEOWNERS file? |
Thanks for reminding, we will add it |
Description
This PR implements Azure ServiceConnector as command group
az {source} connection
.Currently, the supported
{source}
areThe supported target resources are
In the future, more compute services and target resources may be added.
Note. As
az {source} connection
is released as CLI core module, so for{source}
which are in CLI extension (e.g,spring-cloud
), we only loadaz {source} connection
when{source}
is installed.Command Interface
Use
az webapp connection
as an example, asaz spring-cloud connection
is similar.This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.