Dealing with slash in subscription name for terraform and MS quirkiness #2000
Conversation
Azure allow / in the sub name however the API endpoint return wrongly even we encode the / into %2F. Get https://management.azure.com/subscriptions/CALM-WFM-Development-Dev%2FTest/providers?api-version=2016-02-01 into your browser and see, azure will return sub not found and the sub name is the first part of the slash only. However ID seems to work. This changes put some more debugging and try to parse and use the ID if found a slash.
Ouch! Will remove it :) |
Hey it is just subscriptions! Is it regarded as secrets though? |
…st comprehension and index operator Looks like more pythonic!
|
/azp run |
| apiproxy.set_active_subscribtion(sp['subscriptionId']) | ||
| self.new_env['ARM_SUBSCRIPTION_ID'] = sp['subscriptionId'] | ||
| arm_subscription_id = sp['subscriptionId'] | ||
| if "/" in sp['subscriptionId']: |
There was a problem hiding this comment.
subscriptionId is always UUID. Why would you have slash in it?
| arm_subscription_id = sp['subscriptionId'] | ||
| if "/" in sp['subscriptionId']: | ||
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
|
Putting |
Azure allow / in the sub name however the API endpoint return wrongly even we
encode the / into %2F. Get
https://management.azure.com/subscriptions/CALM-WFM-Development-Dev%2FTest/providers?api-version=2016-02-01
into your browser and see, azure will return sub not found and the sub name is
the first part of the slash only.
However ID seems to work.
This changes put some more debugging and try to parse and use the ID if found a slash.