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.

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

Arm-ttk failing the usage of concat() in ARM-templates #590

Open
MadhurangaWije opened this issue Feb 10, 2022 · 12 comments
Open

Arm-ttk failing the usage of concat() in ARM-templates #590

MadhurangaWije opened this issue Feb 10, 2022 · 12 comments
Labels
discussion Can use some help thinking through this one... enhancement New feature or request

Comments

@MadhurangaWije
Copy link

We have faced an issue while using the Arm-ttk runner to static scan some of the ARM-templates which were infact exported from the Azure portal it self and directly used, which contains concat() in some of the URI attributes. Following is the failure message which shows in the Pipeline run logs.

[-] URIs Should Be Properly Constructed (51 ms)
Function'concat' found within 'iconUri Line: 45, Column: 3533
Function'concat' found within 'requestUri Line: 52, Column: 4575

@ghost ghost added the Needs: triage 🔍 label Feb 10, 2022
@bmoore-msft
Copy link
Contributor

I suspect that's intentional - can you share the template (or resource snippet)?

@riclandy
Copy link

We are facing a similar issue that only started today.
The ARM templates for ADF, Databricks and Synapse are all failing the Static testing.

Here's an Example....

ERROR MESSAGE
Microsoft.PowerShell.Commands.WriteErrorException: Function'concat' found within 'url in template file deploy.dataFactory.json

STACK TRACE
_tasks\RunARMTTKTests_\1.5.1\arm-ttk\testcases\deploymentTemplate\URIs-Should-Be-Properly-Constructed.test.ps1: line 34
_tasks\RunARMTTKTests_\1.5.1\arm-ttk\Test-AzTemplate.ps1: line 249
_tasks\RunARMTTKTests_\1.5.1\arm-ttk\Test-AzTemplate.ps1: line 313
_tasks\RunARMTTKTests_\1.5.1\arm-ttk\Test-AzTemplate.ps1: line 553
_tasks\RunARMTTKTests_\1.5.1\arm-ttk\Test-AzTemplate.ps1: line 699
_tasks\RunARMTTKTests_\1.5.1\invoke-ttk.psm1: line 17
_tasks\RunARMTTKTests_\1.5.1\invoke-ttk.psm1: line 120
_tasks\RunARMTTKTests_\1.5.1\powershell.ps1: line 40

Here's a snippet of the ARM template showing the use of concat().

  "name": "[variables('dataFactoryName')]",
  "type": "Microsoft.DataFactory/factories",
  "apiVersion": "2018-06-01",
  "location": "[variables('location')]",
  "identity": {
    "type": "SystemAssigned"
  },
  "properties": {
    "publicNetworkAccess": "Disabled",
    "purviewConfiguration": {
      "purviewResourceId": "[parameters('purviewResourceId')]"
    }
  },
  "tags": {
    "catalogUri": "[concat(variables('purviewAccountName'), '.catalog.purview.azure.com')]"
  },
  "resources": [
    {
      "name": "default",
      "type": "managedVirtualNetworks",
      "apiVersion": "2018-06-01",
      "dependsOn": [
        "[resourceId('Microsoft.DataFactory/factories', variables('dataFactoryName'))]"
      ],
      "properties": {}
    },

@bmoore-msft
Copy link
Contributor

@riclandy - any chance you could attach (or send me) a complete template? I know ADF/Datbricks can be a bit unwieldy but I'm not able to repro the stack trace with just your snippet above. The flagging of the concat function is expected...

@erik-source
Copy link

We ran into this as well, but in our case they were valid test failures. We just didn't realize a new test had been added and when it started failing in CI it caught us off guard.

Example snippet of the change to pass the test:
Fails:

"uri": "[concat('https://mystorageaccount.blob.', environment().suffixes.storage, '/templates/v2/')]"

Passes

"uri": "[uri(concat('https://mystorageaccount.blob.', environment().suffixes.storage), '/templates/v2/')]"

@riclandy
Copy link

@erik-source
Thanks for sharing, I'll give this a go and report back with my findings.

@bmoore-msft
I'll try the fix above first and then send the ARM template if this doesn't fix the issue. Thanks

@riclandy
Copy link

@erik-source and @bmoore-msft
Thank you for you help, the fix above partially works.
However, in the majority of cases where I need to update the code, the value only needs to be a baseurl

Therefore, if I need to use the URI function I have to pass in 2 values.

This works but leaves me with a trailing '/' , which I don't need in the ARM template. For example,
"baseUrl": "[uri(concat('https://', variables('keyVaultName'), '.vault.azure.net'), '/')]"

This example, also passed the test but the URL function adds a trailing '/'
"baseUrl": "[uri(concat('https://', variables('keyVaultName'), '.vault.azure.net'), '')]"

Is this the best way to focus on fixing the proper constructed url and uri's ?
Just thought I'd check with you incase there is a better option.

@erik-source
Copy link

That is a good point, the docs for the uri function state "Creates an absolute URI by combining the baseUri and the relativeUri string." which is not how the concat function is being used in this case.

I noticed in one of the passing test cases a dot is used as the second parameter. I'm not an expert at URIs by any means but I suppose that's still a valid DNS name, albeit not a common pattern. And maybe I'm totally misinterpreting the usage of the dot there.

It would be easier to have the tests allow for URIs without path segments, but still need interpolation/format/concat. The DeploymentTemplate-Must-Not-Contain-Hardcoded-Uri test checks for this, so I imagine it will be very common.

@bmoore-msft
Copy link
Contributor

I believe the trailing slash is part of the canonical representation of the URI - any code that deals with a uri would take that into account, any string manipulation would have to "manually".

Is the problem that you don't want/need the trailing slash or something in ARM doesn't allow it (which wouldn't surprise me).

Aside from that I'm not sure what we could do in the test, that would account for both cases - so the only option may be to skip it.

@riclandy
Copy link

riclandy commented Feb 17, 2022

Thanks for the speedy replies!

We've decided to go the the trailing forward slash.
"baseUrl": "[uri(concat('https://', variables('keyVaultName'), '.vault.azure.net'), '/')]"

It's working OK, the Arm-Ttk tests are all passing and the ARM templates are deploying without any errors.
Also tested that the ADF Pipelines are running fine without any errors.
(we had to alter the Post-deployment pester testcases to include the trailing / in the tests, but that isn't related to the Pre-deployment Arm-Ttk testing)

So far, so good.
Cheers

@erik-source
Copy link

@bmoore-msft I think the only issue in our case is that we don't want/need it. If we don't need to combine a base and relative uri, I wouldn't expect to have to call the uri function, whose purpose is to combine a base and relative uri. Just feels unnecessary.

As you say, the result is still a valid uri and it has not broken anything for us. If this can't be accounted for in the test, I suppose we just have to deal with this quirk when writing our templates.

@mtetenkin
Copy link
Contributor

mtetenkin commented Feb 18, 2022

I also have issue with trailing slash in uri() function. Solved by using variables, instead of using

"catalogUri": "[concat(variables('purviewAccountName'), '.catalog.purview.azure.com')]"

I added variable:

"variables": {
  "catalogHost": "[concat(variables('purviewAccountName'), '.catalog.purview.azure.com')]"
}

and used it in resource parameter:

"catalogUri": "[variables('catalogHost')]"

@bmoore-msft
Copy link
Contributor

We might be able to get creative and check to see if it's just a hostName you're after... e.g. is the last arguement in the function fit a regex that looks like a TLD (...) of something like that...

Would have to think through it a bit to see if that has unintended consequences -- would also be a breaking change... but I do agree with the scenario. A hostname and a URI are arguably different things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Can use some help thinking through this one... enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants