-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[WIP] - Add OpenShift support #7385
Changes from 4 commits
c87021b
a403821
cd8e4e6
dda8819
a47fd2a
eeee1a6
e1f47af
2df28e2
6546358
91961c1
04e4788
5f6b0dd
9d9240b
d9b2d41
f889de4
ac36154
efaa154
73ae6ec
3b600a8
0fa3d39
b7156ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -174,6 +174,47 @@ def remove_resource(self, name, **kwargs): | |
execute(self.cli_ctx, 'az ad sp delete --id {}'.format(self.result['appId'])) | ||
|
||
|
||
# Managed Application preparer | ||
|
||
# pylint: disable=too-many-instance-attributes | ||
class ManagedApplicationPreparer(AbstractPreparer, SingleValueReplacer): | ||
def __init__(self, name_prefix='clitest', parameter_name='aad_client_app_id', | ||
julienstroheker marked this conversation as resolved.
Show resolved
Hide resolved
|
||
parameter_secret='aad_client_app_secret', app_name='app_name', | ||
parameter_tenant='aad_tenant_id', | ||
dev_setting_app_name='AZURE_CLI_TEST_DEV_APP_NAME', | ||
dev_setting_app_secret='AZURE_CLI_TEST_DEV_APP_SECRET', | ||
dev_setting_app_tenant='AZURE_CLI_TEST_DEV_APP_TENANT', | ||
key='app'): | ||
super(ManagedApplicationPreparer, self).__init__(name_prefix, 24) | ||
self.cli_ctx = get_dummy_cli() | ||
self.parameter_name = parameter_name | ||
self.parameter_secret = parameter_secret | ||
self.parameter_tenant = parameter_tenant | ||
self.result = {} | ||
self.app_name = app_name | ||
self.dev_setting_app_name = os.environ.get(dev_setting_app_name, None) | ||
self.dev_setting_app_secret = os.environ.get(dev_setting_app_secret, None) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't the default value for |
||
self.dev_setting_app_tenant = os.environ.get(dev_setting_app_tenant, None) | ||
self.key = key | ||
|
||
def create_resource(self, name, **kwargs): | ||
if not self.dev_setting_app_name: | ||
template = 'az ad app create --display-name {} --key-type Password --password {} --identifier-uris ' \ | ||
'http://microsoft.onmicrosoft.com/{}' | ||
self.result = execute(self.cli_ctx, template.format(name, name, name)).get_output_in_json() | ||
self.test_class_instance.kwargs[self.key] = name | ||
# The slice is the easiest way for know to return the Teanant from the same command | ||
return {self.parameter_name: self.result['appId'], self.parameter_secret: name, | ||
self.parameter_tenant: self.result['odata.metadata'][26:62]} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This not very stable. Isn't there any alternative? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, it is not very clean, the response from the command is not returning this field I can add another cli command such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like a more stable solution than this. Please do that and also add comments regarding why it invokes that command to fetch the tenant id. |
||
self.test_class_instance.kwargs[self.key] = name | ||
return {self.parameter_name: self.dev_setting_sp_name, | ||
self.parameter_secret: self.dev_setting_app_secret, | ||
self.parameter_tenant: self.dev_setting_app_tenant} | ||
|
||
def remove_resource(self, name, **kwargs): | ||
if not self.dev_setting_app_name: | ||
execute(self.cli_ctx, 'az ad app delete --id {}'.format(self.result['appId'])) | ||
|
||
# Utility | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,17 @@ | |
from azure.mgmt.containerservice.models import ManagedClusterAADProfile | ||
from azure.mgmt.containerservice.models import ManagedClusterAddonProfile | ||
from azure.mgmt.containerservice.models import ManagedClusterAgentPoolProfile | ||
from azure.mgmt.containerservice.models import OpenShiftManagedClusterAgentPoolProfile | ||
from azure.mgmt.containerservice.models import OSType | ||
from azure.mgmt.containerservice.models import OpenShiftAgentPoolProfileRole | ||
from azure.mgmt.containerservice.models import OpenShiftManagedClusterIdentityProviders | ||
from azure.mgmt.containerservice.models import OpenShiftManagedClusterServiceAADIdentityProvider | ||
from azure.mgmt.containerservice.models import OpenShiftManagedCluster | ||
from azure.mgmt.containerservice.models import OpenShiftManagedClusterMasterPoolProfile | ||
from azure.mgmt.containerservice.models import OpenShiftContainerServiceVMSize | ||
from azure.mgmt.containerservice.models import OpenShiftRouterProfile | ||
from azure.mgmt.containerservice.models import OpenShiftManagedClusterAuthProfile | ||
|
||
from ._client_factory import cf_container_services | ||
from ._client_factory import cf_resource_groups | ||
from ._client_factory import get_auth_management_client | ||
|
@@ -2164,3 +2175,106 @@ def _validate_aci_location(norm_location): | |
if norm_location not in aci_locations: | ||
raise CLIError('Azure Container Instance is not available at location "{}".'.format(norm_location) + | ||
' The available locations are "{}"'.format(','.join(aci_locations))) | ||
|
||
|
||
def openshift_create(cmd, client, resource_group_name, name, # pylint: disable=too-many-locals | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Break this method into smaller methods to avoid There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure to understand how I can do this. Those parameters are from the user. I am doing the same logic as the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "too many locals" means that you defined too many local variables in this method, which indicates that the body of the function is too large. |
||
location=None, | ||
node_vm_size="Standard_D4s_v3", | ||
node_count=3, | ||
fqdn='', | ||
aad_client_app_id=None, | ||
aad_client_app_secret=None, | ||
aad_tenant_id=None, | ||
tags=None, | ||
no_wait=False): | ||
|
||
rg_location = _get_rg_location(cmd.cli_ctx, resource_group_name) | ||
if location is None: | ||
location = rg_location | ||
|
||
agent_pool_profiles = [] | ||
agent_node_pool_profile = OpenShiftManagedClusterAgentPoolProfile( | ||
name='compute', # Must be 12 chars or less before ACS RP adds to it | ||
count=int(node_count), | ||
vm_size=node_vm_size, | ||
os_type="Linux", | ||
role=OpenShiftAgentPoolProfileRole.compute | ||
) | ||
|
||
agent_infra_pool_profile = OpenShiftManagedClusterAgentPoolProfile( | ||
name='infra', # Must be 12 chars or less before ACS RP adds to it | ||
count=int(2), | ||
vm_size="Standard_D4s_v3", | ||
os_type="Linux", | ||
role=OpenShiftAgentPoolProfileRole.infra | ||
) | ||
|
||
agent_pool_profiles.append(agent_node_pool_profile) | ||
agent_pool_profiles.append(agent_infra_pool_profile) | ||
|
||
agent_master_pool_profile = OpenShiftManagedClusterAgentPoolProfile( | ||
name='master', # Must be 12 chars or less before ACS RP adds to it | ||
count=int(3), | ||
vm_size="Standard_D2s_v3", | ||
os_type="Linux" | ||
) | ||
identity_providers = [] | ||
|
||
if any([aad_client_app_id, aad_client_app_secret, aad_tenant_id]): | ||
identity_providers.append( | ||
OpenShiftManagedClusterIdentityProviders( | ||
name='Azure AD', | ||
provider=OpenShiftManagedClusterServiceAADIdentityProvider( | ||
kind='AADIdentityProvider', | ||
client_id=aad_client_app_id, | ||
secret=aad_client_app_secret, | ||
tenant_id=aad_tenant_id | ||
) | ||
) | ||
) | ||
auth_profile = OpenShiftManagedClusterAuthProfile(identity_providers=identity_providers) | ||
|
||
default_router_profile = OpenShiftRouterProfile(name='default') | ||
|
||
osamc = OpenShiftManagedCluster( | ||
location=location, tags=tags, | ||
open_shift_version="v3.10", | ||
fqdn=fqdn, | ||
auth_profile=auth_profile, | ||
agent_pool_profiles=agent_pool_profiles, | ||
master_pool_profile=agent_master_pool_profile, | ||
router_profiles=[default_router_profile]) | ||
|
||
# We don't creating the AADIdentity for the user right now but maybe later so keeping this | ||
# Keeping this Due to SPN replication latency, we do a few retries here | ||
max_retry = 30 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will make the retry last up to 90 seconds. I recommend you print warning to the stderr through logging so as to avoid the impression of command hanging. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed that part since I added the auto creation of the AADIdentity : |
||
retry_exception = Exception(None) | ||
for _ in range(0, max_retry): | ||
try: | ||
# long_running_operation_timeout=300 | ||
return sdk_no_wait(no_wait, client.create_or_update, | ||
resource_group_name=resource_group_name, resource_name=name, parameters=osamc) | ||
except CloudError as ex: | ||
retry_exception = ex | ||
if 'not found in Active Directory tenant' in ex.message: | ||
time.sleep(3) | ||
else: | ||
raise ex | ||
raise retry_exception | ||
|
||
|
||
def openshift_show(cmd, client, resource_group_name, name): | ||
mc = client.get(resource_group_name, name) | ||
return [mc][0] | ||
|
||
|
||
def openshift_scale(cmd, client, resource_group_name, name, node_count, no_wait=False): | ||
instance = client.get(resource_group_name, name) | ||
# TODO: change this approach when we support multiple agent pools. | ||
instance.agent_pool_profiles[0].count = int(node_count) # pylint: disable=no-member | ||
|
||
# null out the AAD profile and add manually the masterAP name because otherwise validation complains | ||
instance.master_pool_profile.name = "master" | ||
instance.auth_profile = None | ||
|
||
return sdk_no_wait(no_wait, client.create_or_update, resource_group_name, name, instance) |
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.
Break the line at 120.