-
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 11 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,142 @@ 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 | ||
:param chart_url: URL to the chart, | ||
Default: https://github.com/Azure/aci-connector-k8s/raw/master/charts/aci-connector.tgz | ||
:type chart_url: str | ||
:param os_type: Os type target, could be Windows, Linux or Both. | ||
:type os_type: 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 = "Error : Helm not detected, please verify if it is installed." | ||
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. No need for the 'Error: ' prefix. |
||
image_tag = 'latest' | ||
if chart_url is None: | ||
url_chart = "https://github.com/Azure/aci-connector-k8s/raw/master/charts/aci-connector.tgz" | ||
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 you define chart_url in params.py with a default, this is not needed. |
||
else: | ||
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 == 'Windows') or (os_type == 'Both'): | ||
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. Define 'Windows', 'Both', 'Linux' as constants and this should be a case-insensitive comparison. |
||
# 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 | ||
:param graceful: Mention if you want to drain/uncordon your aci-connector to move your applications | ||
running on ACI to your others nodes. Default : False | ||
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. Once you move the param descriptions to params.py, you can define the default there. |
||
:type graceful: bool | ||
:param os_type: Os type target, could be Windows, Linux or Both. | ||
:type os_type: 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) | ||
# Validate if the RG exists | ||
# Just do the get, we don't need the result, it will error out if the group doesn't exist. | ||
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. these 2 comments are invalid |
||
if graceful: | ||
logger.warning('Graceful option selected, will try to drain the node first') | ||
kubectl_not_installed = "Error : Kubectl not detected, please verify if it is installed." | ||
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. No need to prefix with |
||
try: | ||
Popen(["kubectl"], stdout=PIPE, stderr=PIPE) | ||
except OSError: | ||
raise CLIError(kubectl_not_installed) | ||
try: | ||
if (os_type == 'Windows') or (os_type == 'Both'): | ||
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. As before, should be a case-insensitive comparison. |
||
drain_node = subprocess.check_output( | ||
["kubectl", "drain", "aci-connector-0", "--force"], | ||
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. should use the "name" parameter instead of aci-connector-x |
||
universal_newlines=True) | ||
drain_node = subprocess.check_output( | ||
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. suggest use a loop to avoid code dupes, something like nodes = ['-0', '-1'] if os_type.lower() in ['windows', 'both'] else ['']
for n in nodes:
drain_node = subprocess.check_output(["kubectl", "drain", "aci-connector{}".format(n), "--force"], universal_newlines=True) |
||
["kubectl", "drain", "aci-connector-1", "--force"], | ||
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. same here. show we use {name}-win, {name}-linux as the node name? 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 is not supported in the current connector codebase, we cannot specify a custom node name 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. see @itowlson last comment |
||
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: | ||
# remove the "pods/" prefix from the name | ||
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 comment might be wrong |
||
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='...'