-
Notifications
You must be signed in to change notification settings - Fork 3k
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
AKS/ACI - Add install-connector and remove-connector commands #4996
Changes from 16 commits
e5c2164
42c21d3
43f248c
91e7800
ba59b59
d3f90b1
0dd4492
f813da0
6d8cbbf
1d5f2a1
ad27361
23b0dea
09fb6ba
64e1a8c
b36c14b
15d32e5
372cca4
3ee223f
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 |
---|---|---|
|
@@ -311,6 +311,125 @@ def k8s_install_cli(client_version='latest', install_location=None): | |
raise CLIError('Connection error while attempting to download client ({})'.format(ex)) | ||
|
||
|
||
def k8s_install_connector(client, name, resource_group, connector_name, | ||
location=None, service_principal=None, client_secret=None, | ||
chart_url=None, os_type='Linux'): | ||
"""Deploy the ACI-Connector to a AKS cluster | ||
:param name: The name of the AKS cluster. The name is case insensitive. | ||
:type name: str | ||
:param connector_name: The name for the ACI Connector | ||
:type connector_name: str | ||
:param resource_group: Name of the resource group where the AKS is located and where | ||
the ACI connector will be mapped. | ||
:type resource_group: str | ||
:param location: | ||
:type location: str | ||
:param service_principal: The service principal used for ACI authentication | ||
to Azure APIs. If not specified, it is created for you and stored in the | ||
${HOME}/.azure directory. | ||
:type service_principal: str | ||
:param client_secret: The secret associated with the service principal. If | ||
--service-principal is specified, then secret should also be specified. If | ||
--service-principal is not specified, the secret is auto-generated for you | ||
and stored in ${HOME}/.azure/ directory. | ||
:type client_secret: str | ||
""" | ||
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. You can delete this whole docstring. 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. You can still just remove this block of comments. 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. Got it, let me move all of it in the |
||
from subprocess import PIPE, Popen | ||
helm_not_installed = "Helm not detected, please verify if it is installed." | ||
image_tag = 'latest' | ||
url_chart = chart_url | ||
# Check if Helm is installed locally | ||
try: | ||
Popen(["helm"], stdout=PIPE, stderr=PIPE) | ||
except OSError: | ||
raise CLIError(helm_not_installed) | ||
# Validate if the RG exists | ||
groups = _resource_client_factory().resource_groups | ||
# Just do the get, we don't need the result, it will error out if the group doesn't exist. | ||
rgkaci = groups.get(resource_group) | ||
# Auto assign the location | ||
if location is None: | ||
location = rgkaci.location # pylint:disable=no-member | ||
# Get the credentials from a AKS instance | ||
_, browse_path = tempfile.mkstemp() | ||
aks_get_credentials(client, resource_group, name, admin=False, path=browse_path) | ||
subscription_id = _get_subscription_id() | ||
dns_name_prefix = _get_default_dns_prefix(connector_name, resource_group, subscription_id) | ||
# Ensure that the SPN exists | ||
principal_obj = _ensure_service_principal(service_principal, client_secret, subscription_id, | ||
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 have a concern about it: # add role first before save it
if not _add_role_assignment('Contributor', service_principal):
logger.warning('Could not create a service principal with the right permissions. '
'Are you an Owner on this project?') 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. 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. @weinong You are asking @julienstroheker to remove code thats not even part of the commit. If theres a concern with another module/function i suggest submitting a PR or issue for that code. This is also an overall problem that should be addressed as an issue regarding the functionality of that method. Which I agree, we should not be scoping to an entire sub, but we are already doing that for acs/aks, thus this should be its own issue and un-related to thi pr. 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. @listonb np! we can fix that separately. |
||
dns_name_prefix, location, connector_name) | ||
client_secret = principal_obj.get("client_secret") | ||
service_principal = principal_obj.get("service_principal") | ||
# Get the TenantID | ||
profile = Profile() | ||
_, _, tenant_id = profile.get_login_credentials() | ||
# Check if we want the windows connector | ||
if os_type.lower() in ['windows', 'both']: | ||
# The current connector will deploy two connectors, one for Windows and another one for Linux | ||
image_tag = 'canary' | ||
logger.warning('Deploying the aci-connector using Helm') | ||
try: | ||
subprocess.call(["helm", "install", url_chart, "--name", connector_name, "--set", "env.azureClientId=" + | ||
service_principal + ",env.azureClientKey=" + client_secret + ",env.azureSubscriptionId=" + | ||
subscription_id + ",env.azureTenantId=" + tenant_id + ",env.aciResourceGroup=" + rgkaci.name + | ||
",env.aciRegion=" + location + ",image.tag=" + image_tag]) | ||
except subprocess.CalledProcessError as err: | ||
raise CLIError('Could not deploy the ACI Chart: {}'.format(err)) | ||
|
||
|
||
def k8s_uninstall_connector(client, name, connector_name, resource_group, | ||
graceful=False, os_type='Linux'): | ||
"""Undeploy the ACI-Connector from an AKS cluster. | ||
:param name: The name of the AKS cluster. The name is case insensitive. | ||
:type name: str | ||
:param resource_group: The name of the resource group where the AKS cluster is deployed. | ||
:type resource_group: str | ||
:param connector_name: The name for the ACI Connector | ||
:type connector_name: str | ||
""" | ||
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. Can remove this whole docstring. 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. You can still just remove this block of comments. 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. Got it, let me move all of it in the |
||
from subprocess import PIPE, Popen | ||
helm_not_installed = "Error : Helm not detected, please verify if it is installed." | ||
# Check if Helm is installed locally | ||
try: | ||
Popen(["helm"], stdout=PIPE, stderr=PIPE) | ||
except OSError: | ||
raise CLIError(helm_not_installed) | ||
# Get the credentials from a AKS instance | ||
_, browse_path = tempfile.mkstemp() | ||
aks_get_credentials(client, resource_group, name, admin=False, path=browse_path) | ||
if graceful: | ||
logger.warning('Graceful option selected, will try to drain the node first') | ||
kubectl_not_installed = "Kubectl not detected, please verify if it is installed." | ||
try: | ||
Popen(["kubectl"], stdout=PIPE, stderr=PIPE) | ||
except OSError: | ||
raise CLIError(kubectl_not_installed) | ||
try: | ||
if os_type.lower() in ['windows', 'both']: | ||
nodes = ['-0', '-1'] | ||
for n in nodes: | ||
drain_node = subprocess.check_output( | ||
["kubectl", "drain", "aci-connector{}".format(n), "--force"], | ||
universal_newlines=True) | ||
else: | ||
drain_node = subprocess.check_output( | ||
["kubectl", "drain", "aci-connector", "--force"], | ||
universal_newlines=True) | ||
except subprocess.CalledProcessError as err: | ||
raise CLIError('Could not find the node, make sure you' + | ||
'are using the correct --os-type option: {}'.format(err)) | ||
if drain_node: | ||
drain_node = str(drain_node) | ||
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.
if not drain_node:
raise CLIError("Couldn't find the node.") |
||
else: | ||
raise CLIError("Couldn't find the node.") | ||
|
||
logger.warning('Undeploying the aci-connector using Helm') | ||
try: | ||
subprocess.call(["helm", "del", connector_name, "--purge"]) | ||
except subprocess.CalledProcessError as err: | ||
raise CLIError('Could not deploy the ACI Chart: {}'.format(err)) | ||
|
||
|
||
def _build_service_principal(client, name, url, client_secret): | ||
# use get_progress_controller | ||
hook = APPLICATION.get_progress_controller(True) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ | |
logger.warn("Wheel is not available, disabling bdist_wheel hook") | ||
cmdclass = {} | ||
|
||
VERSION = "2.0.20" | ||
VERSION = "2.0.21" | ||
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 think we're supposed to leave this alone and let the maintainers increment it--at least I had a previous PR where that was the case. 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. Oh ok, I did it because it failed the CI. Do you want me to put it back version .20? 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. There is a process change recently that if yours is the first one since the latest release, you should bump up the version here and in the HISTORY.RST; otherwise CI will fail |
||
CLASSIFIERS = [ | ||
'Development Status :: 5 - Production/Stable', | ||
'Intended Audience :: Developers', | ||
|
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.
All help should be defined in params.py and _help.py not in the docstring of the commands.
--os-type
: In params.py, define a choice_list of this param with possible values of ['Windows', 'Linux', 'Both']--chart-url
: In params.py, you can specify the default value as a stringdefault='...'