Proposal/Feature Request: Enable comments and full language support for ARM templates

[lawrencegripper]

Is your feature request related to a problem?

Currently the following template will fail to deploy via the CLI but will succeed if sent directly to the ARM API endpoints.

https://github.com/opencb/opencga/blob/0d4c942d71894a16aa30b498ecb2c77318449cd8/opencga-app/app/scripts/azure/arm/daemonvm/azuredeploy.jsonc

There are 2 reasons for this failure.

1. The JSON contains comments
2. The some forms of ARM language aren't valid JSON but ARE valid ARM templates.

For example, the following is valid ARM but not valid JSON as the value splits over multiple lines.

"initCmd": "[concat('docker run  --name=opencga-init --mount type=bind,src=/media/primarynfs,dst=/opt/volume', 
            ' -e INIT_OPENCGA_PASS=',parameters('openCgaAdminPassword'), 
            ' -e INIT_HEALTH_CHECK_INTERVAL=',parameters('healthCheckInterval'), 
            ' -e INIT_HBASE_SSH_DNS=',parameters('hdInsightSshDns'), 
            ' -e INIT_HBASE_SSH_USER=',parameters('hdInsightSshUsername'), 
            ' -e INIT_HBASE_SSH_PASS=',parameters('hdInsightSshPassword')]"

When submitted to the ARM endpoint the content of "[ here including multi-line ]" is processed by the ARM language then re-inserted into the template.

Describe the solution you'd like

In Powershell this is handled correctly and when talking to the ARM team they mentioned there is a desire to unify the approaches to give consistency.

Looking at the code in the CLI I'd like to submit a PR moving the ARM template code to using commentsjson to allow the processing of the JSON files correctly.

So I'd update the utils function here to use commentjson (for this change to be handled in all resource not just ARM) or just update the ARM specific code.

Then have a simple regex (sudo code) function which would strip out the ARM specific lines before using the utils loader. This would allow the JSON to be validated but not trip it up on ARM language elements. Then once the JSON is validated re-insert the ARM language chunks before sending to the ARM API.

def _jsonc_arm_safe_parse(json_or_dict_string, preserve_order=False):
    arm_language_regex = re.compile(r"\"\[.*?\]\"")
    json_or_dict_string = arm_language_regex.sub("")  
    return shell_safe_json_parse(json_or_dict_string, preserve_order)

** Why would we want these? **

ARM templates become seriously large for big deployments. Being able to comment and split inputs over multiple lines makes the difference between usable and big ball of mess.

For example see: (hint scroll right on the first one)

initCmd": "[concat('docker run  --name=opencga-init --mount type=bind,src=/media/primarynfs,dst=/opt/volume -e INIT_OPENCGA_PASS=',parameters('openCgaAdminPassword'),' -e INIT_HEALTH_CHECK_INTERVAL=',parameters('healthCheckInterval'),' -e INIT_HBASE_SSH_DNS=',parameters('hdInsightSshDns'),' -e INIT_HBASE_SSH_USER=',parameters('hdInsightSshUsername'),' -e INIT_HBASE_SSH_PASS=',parameters('hdInsightSshPassword'),' -e INIT_SEARCH_HOSTS=',variables('solrHostsCSV'),' -e INIT_CELLBASE_MONGO_HOSTS=',variables('mongoDbHostsCSV'),' -e INIT_CLINICAL_HOSTS=',variables('solrHostsCSV'),' -e INIT_CATALOG_DATABASE_HOSTS=',variables('mongoDbHostsCSV'),' -e INIT_CATALOG_DATABASE_USER=',parameters('mongoDbUser'),' -e INIT_CATALOG_DATABASE_PASSWORD=',parameters('mongoDbPassword'),' -e INIT_CATALOG_SEARCH_HOSTS=',variables('solrHostsCSV'),' -e INIT_CATALOG_SEARCH_USER=',parameters('solrUser'),' -e INIT_CATALOG_SEARCH_PASSWORD=',parameters('solrPassword'),' -e INIT_REST_HOST=\"',variables('restHost'),'\" -e INIT_GRPC_HOST=\"',variables('grpcHost'),'\" -e INIT_BATCH_EXEC_MODE=AZURE ',' -e INIT_BATCH_ACCOUNT_NAME=',parameters('batchAccountName'),' -e INIT_BATCH_ACCOUNT_KEY=',parameters('batchAccountKey'),' -e INIT_BATCH_ENDPOINT=',parameters('batchEndpoint'),' -e INIT_BATCH_POOL_ID=',parameters('batchPoolId'),' -e INIT_BATCH_DOCKER_ARGS=',variables('singleQuote'),parameters('batchDockerArgs'),variables('singleQuote'),' -e INIT_BATCH_DOCKER_IMAGE=',parameters('batchContainerImage'),' -e INIT_BATCH_MAX_CONCURRENT_JOBS=',string(parameters('batchMaxConcurrentJobs')),' -e INIT_CELLBASE_MONGO_HOSTS=',string(parameters('cellbaseMongoDbHosts')), ' -e INIT_CELLBASE_MONGO_HOSTS_USER=',string(parameters('cellbaseMongoDbUser')),' -e INIT_CELLBASE_MONGO_HOSTS_PASSWORD=',string(parameters('cellbaseMongoDbPassword')), ' -e INIT_CELLBASE_REST_URLS=',string(parameters('cellbaseRestUrls')), ' ', parameters('initContainerImage'),' ',parameters('catalogSecretKey'))]",

vs

// This build the command used to setup the configuration file for OpenCGA
        // it takes in inputs from the larger ARM template to configure Mongo, SOLR, Storage and other settings
        // the script used can be found here /opencga-app/app/scripts/docker/opencga-init/*.py
        // NOTE> Editors will show this line as invalid JSON but as the `[]` denote the ARM language 
        //       this functions correctly. 
        "initCmd": "[concat('docker run  --name=opencga-init --mount type=bind,src=/media/primarynfs,dst=/opt/volume', 
            ' -e INIT_OPENCGA_PASS=',parameters('openCgaAdminPassword'), 
            ' -e INIT_HEALTH_CHECK_INTERVAL=',parameters('healthCheckInterval'), 
            ' -e INIT_HBASE_SSH_DNS=',parameters('hdInsightSshDns'), 
            ' -e INIT_HBASE_SSH_USER=',parameters('hdInsightSshUsername'), 
            ' -e INIT_HBASE_SSH_PASS=',parameters('hdInsightSshPassword'), 
            ' -e INIT_SEARCH_HOSTS=',variables('solrHostsCSV'), 
            ' -e INIT_CELLBASE_MONGO_HOSTS=',variables('mongoDbHostsCSV'), 
            ' -e INIT_CLINICAL_HOSTS=',variables('solrHostsCSV'), 
            ' -e INIT_CATALOG_DATABASE_HOSTS=',variables('mongoDbHostsCSV'), 
            ' -e INIT_CATALOG_DATABASE_USER=',parameters('mongoDbUser'), 
            ' -e INIT_CATALOG_DATABASE_PASSWORD=',parameters('mongoDbPassword'), 
            ' -e INIT_CATALOG_SEARCH_HOSTS=',variables('solrHostsCSV'), 
            ' -e INIT_CATALOG_SEARCH_USER=',parameters('solrUser'), 
            ' -e INIT_CATALOG_SEARCH_PASSWORD=',parameters('solrPassword'), 
            ' -e INIT_REST_HOST=', variables('singleQuote'),variables('restHost'), variables('singleQuote'),
            ' -e INIT_GRPC_HOST=', variables('singleQuote'), variables('grpcHost'), variables('singleQuote'),
            ' -e INIT_BATCH_EXECUTION_MODE=AZURE ', 
            ' -e INIT_BATCH_ACCOUNT_NAME=',parameters('batchAccountName'), 
            ' -e INIT_BATCH_ACCOUNT_KEY=',parameters('batchAccountKey'), 
            ' -e INIT_BATCH_ENDPOINT=',parameters('batchEndpoint'), 
            ' -e INIT_BATCH_POOL_ID=',parameters('batchPoolId'), 
            ' -e INIT_BATCH_DOCKER_ARGS=',variables('singleQuote'), parameters('batchDockerArgs'),variables('singleQuote'), 
            ' -e INIT_BATCH_DOCKER_IMAGE=',parameters('batchContainerImage'), 
            ' -e INIT_BATCH_MAX_CONCURRENT_JOBS=',string(parameters('batchMaxConcurrentJobs')), 
            ' -e INIT_CELLBASE_MONGO_HOSTS=',string(parameters('cellbaseMongoDbHosts')),  
            ' -e INIT_CELLBASE_MONGO_HOSTS_USER=',string(parameters('cellbaseMongoDbUser')), 
            ' -e INIT_CELLBASE_MONGO_HOSTS_PASSWORD=',string(parameters('cellbaseMongoDbPassword')),  
            ' -e INIT_CELLBASE_REST_URLS=',string(parameters('cellbaseRestUrls')),  
            ' ', parameters('initContainerImage'), 
' ', parameters('catalogSecretKey'))]",

Describe alternatives you've considered

For comments in the JSON it's possible to use a pre-processing step and I considered this but for the ARM language components this doesn't work as easily and very much feels like it should be supported in the built in tooking.

Additional context

I've been in conversation with @alex-frankel and @bmoore-msft regarding the approach to this issue.

[zikalino]

I think I would rather do this change just for ARM templates somewhere around here:

azure-cli/src/azure-cli/azure/cli/command_modules/resource/custom.py

Line 302 in 5573b5f

if template_uri:

shell_safe_json_parse(_urlretrieve(...))

or
get_file_json(...)

Note: get_file_json() uses shell_safe_json_parse() internally.

So I think I would make change for ARM templates first, or just modify get_file_json to support loading json from both files and uris.

[lawrencegripper]

Thanks for the reply, I've not got any scope to pick this up currently as I'm on a different project.

Happy for anyone else to tackle this or I might try and loop back round at some point in the future.

[zikalino]

Some further investigation:

(1) It seems like commentjson has some issues, I have submitted a bug report here:

vaidik/commentjson#22

Basically it won't be able to parse following line as it interpret # as comment:

        "cloud-init": [
            "#cloud-config",

https://github.com/opencb/opencga/blob/0d4c942d71894a16aa30b498ecb2c77318449cd8/opencga-app/app/scripts/azure/arm/daemonvm/azuredeploy.jsonc#L219

It will return an error, as it thinks there's comment in the line above.

(2) secondly removing ARM specific parts -- "[ … ]" -- is not going to help when loading json from local files, as we would have to somehow reinsert them into template after parsing. (there's some magic going on).

Instead I am going to modify shell_safe_json_parse to:

• remove line-breaks (that should resolve ARM template parsing issues)
• remove comments

[Juliehzl]

dupe of #10354. Fixed in PR #10389.