Skip to content
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.

Sign up for GitHub

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

Dealing with slash in subscription name for terraform and MS quirkiness #2000

Closed
wants to merge 3 commits into from
Closed

Dealing with slash in subscription name for terraform and MS quirkiness #2000

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a14860
Dealing with slash in subscription name for terraform and MS quirkiness
sunshine69 Jan 28, 2021
c17625a
Change logger level
sunshine69 Jan 28, 2021
0de66c8
Use generator expression and next to yield value instead of normal li…
sunshine69 Feb 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions core/src/epicli/cli/engine/terraform/TerraformRunner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def __init__(self, cluster_model, config_docs):
self.new_env = os.environ.copy()
self.terraform.init(env=self.new_env)
if self.cluster_model.provider == 'azure':
self.azure_login()
self.azure_login()

def __enter__(self):
super().__enter__()
Expand All @@ -30,9 +30,9 @@ def build(self):

def delete(self):
self.terraform.destroy(auto_approve=True, env=self.new_env)

def azure_login(self):
# From the 4 methods terraform provides to login to
# From the 4 methods terraform provides to login to
# Azure we support (https://www.terraform.io/docs/providers/azurerm/auth/azure_cli.html):
# - Authenticating to Azure using the Azure CLI
# - Authenticating to Azure using a Service Principal and a Client Secret
Expand All @@ -52,22 +52,27 @@ def azure_login(self):
# Create the service principal, for now we use the default subscription
self.logger.info('Creating service principal')
cluster_name = self.cluster_model.specification.name.lower()
cluster_prefix = self.cluster_model.specification.prefix.lower()
cluster_prefix = self.cluster_model.specification.prefix.lower()
resource_group_name = resource_name(cluster_prefix, cluster_name, 'rg')
sp = apiproxy.create_sp(resource_group_name, subscription['id'])
sp['subscriptionId'] = subscription['id']
save_sp(sp, self.cluster_model.specification.name)
else:
self.logger.info('Using service principal from file')
self.logger.info(f'Using service principal from file {sp_file}')
sp = load_yaml_file(sp_file)

# Login as SP and get the default subscription.
subscription = apiproxy.login_sp(sp)

if 'subscriptionId' in sp:
# Set active subscription if sp contains it.
# Set active subscription if sp contains it.
self.logger.debug(f"subscriptionId from sp file: {sp['subscriptionId']}")
apiproxy.set_active_subscribtion(sp['subscriptionId'])
self.new_env['ARM_SUBSCRIPTION_ID'] = sp['subscriptionId']
arm_subscription_id = sp['subscriptionId']
if "/" in sp['subscriptionId']:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subscriptionId is always UUID. Why would you have slash in it?

self.logger.debug(f"WARN Slash detected in the subscription name {sp['subscriptionId']}. Will parse the ID and use it instead")
arm_subscription_id = next((_sub['id'] for _sub in subscription if _sub['name'] == sp['subscriptionId']), None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU you are fixing incorrectly created service principal file. I assume that service principal file you are using has subscriptionId and name fields filled with name.

Copy link
Contributor Author

@sunshine69 sunshine69 Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review the existing code as I don't have much time on this, but you will see we accept two types of ID, one UUID and the other format which is the subscription name.

It is just takes it too long a simple change like this with all test and uses cases presented!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for inconvenience and late notice but this PR just looks incorrect. Putting name into subscriptionId field looks like misuse. Service principal file is documented here in point 3:

Where the clustername is the name you specified under specification.name in your cluster yaml. Then in the terraform folder add the file named sp.yml and fill it with the service priciple information like so:

appId: "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxx"
displayName: "app-name"
name: "http://app-name"
password: "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxx"
tenant: "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxx"
subscriptionId: "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxx"

From PR correctness point of view, we tested this change and if we put name without / in it, this change will not work, so even if I didn't mind misusing field if "/" in sp['subscriptionId'] is incorrect and some "validate if is UUID" should be placed here instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is up to the team. At the time in ABB RND team, we hit the problems, our sp.yaml is correct (in terms of correctness) and the problems is describe in this pr, and has been fixed by using this change PR.

That is the fact. If you don't ACKed it then when the RND team move to new version they will hit it again (if they do not use my custom build image/branch which has the fix)

I no longer work for ABB thus I do not have time to debug anymore. From my memory by tracing the code, epiphany accept two format, the UUID or the name - and when the name has slash the azure behavior makes it errors. Read again about my description.

I do it with best of considerations and judgement at the time and I retain. If problem goes away then it might be Azure has fixed their issues (that is if they allow slash in name then in API request they would interpret the encode of slash %F02 to be a string rather than a API path separator).

self.new_env['ARM_SUBSCRIPTION_ID'] = arm_subscription_id
else:
# No subscriptionId in sp.yml so use the default one from Azure SP login.
self.new_env['ARM_SUBSCRIPTION_ID'] = subscription[0]['id']
Expand Down