Azure / bicep-registry-modules

Bicep registry modules
MIT License
506 stars 351 forks source link

[AVM Module Issue]: "Cannot change the private link connection on private endpoint" in several modules #1513

Closed JanKnipp closed 5 months ago

JanKnipp commented 7 months ago

Check for previous/existing GitHub issues

Issue Type?

Bug

Module Name

Other, as defined below...

(Optional) Module Name if not listed above

serveral modules, see below

(Optional) Module Version

No response

Description

Hi, i'm using several modules that deploy resources with a private endpoint. As of now the redeployment of these resources has never been a problem. When updating one of the mentioned modules the deployment fails with an "Cannot change the private link connection on private endpoint" error. When the module version is rolled back to the "old" version it is working again so a change in the new module version seems to have triggered this. Im using the default mode for deployment (incremental).

I've noticed it for the following modules: avm/res/key-vault/vault:0.3.5 -> avm/res/key-vault/vault:0.4.0 avm/res/storage/storage-account:0.6.0 -> br/public:avm/res/storage/storage-account:0.8.2 avm/res/service-bus/namespace:0.2.5 -> avm/res/service-bus/namespace:0.3.0

Most of my private endpoints are configured like this with the corresponding service names for that resource:

privateEndpoints: [
      {
        name: resourceNames.keyVaultPrivateEndpointName
        customNetworkInterfaceName : resourceNames.keyVaultPrivateEndpointNetworkInferfaceName
        location: location
        service: 'vault'
        subnetResourceId: privateEndpointSubnetId
        tags: tags
        enableTelemetry: enableTelemetry
      }
    ]

Maybe this is connected to the following issues that I found:

https://github.com/Azure/bicep-types-az/issues/1511 https://github.com/Azure/bicep/issues/6810

Regards, Jan

(Optional) Correlation Id

No response

matebarabas commented 7 months ago

@fblix, @ChrisSidebotham - this issue seems to impact your modules. Can you please have a look? Thank you!

ChrisSidebotham commented 7 months ago

@JanKnipp - Are you able to supply what changes you are making that cause the error? I have just checked the servicebus module and it is using the latest specification of the private endpoint deployment https://azure.github.io/Azure-Verified-Modules/specs/shared/interfaces/#private-endpoints @AlexanderSehr - Not something you have seen is it?

AlexanderSehr commented 7 months ago

@JanKnipp - Are you able to supply what changes you are making that cause the error? I have just checked the servicebus module and it is using the latest specification of the private endpoint deployment https://azure.github.io/Azure-Verified-Modules/specs/shared/interfaces/#private-endpoints @AlexanderSehr - Not something you have seen is it?

Can't say I have. The modules should also run the idempotency test in their corresponding workflows. Thanks for linking the issues @JanKnipp. I wonder if any defaults changed in between the modue versions. Would need to look into what for modules like key vault changed from version 0.3.5 to 0.4.0 - and if the PE version was upgraded. And if it was, then what set change was.

JanKnipp commented 7 months ago

@ChrisSidebotham The only real change that I did was changing the module version. The configuration of the module is identical. So i.e. in the service bus example i can deploy the version that uses module v0.2.5 without any issues and just changing it to v.0.3.0 produces the error. My guess is that there is 1-n properties that have a new default value or a value at all and that this triggers arm to think that we want to change an existing pe which it does not allow. Sadly what-if is still not so very useful in seeing changes. I could try to convert the bicep into arm for both versions and do a comparision if there are any changes for the pe.

JanKnipp commented 7 months ago

@ChrisSidebotham @AlexanderSehr These are the changes that I could identify.

grafik

v.0.2.5

{
  "privateLinkServiceConnections": "[if(not(equals(tryGet(coalesce(parameters('privateEndpoints'), createArray())[copyIndex()], 'manualPrivateLinkServiceConnections'), true())), createObject('value', createArray(createObject('name', coalesce(tryGet(coalesce(parameters('privateEndpoints'), createArray())[copyIndex()], 'privateLinkServiceConnectionName'), format('{0}-{1}-{2}', last(split(resourceId('Microsoft.ServiceBus/namespaces', parameters('name')), '/')), coalesce(tryGet(coalesce(parameters('privateEndpoints'), createArray())[copyIndex()], 'service'), 'namespace'), copyIndex())), 'properties', createObject('privateLinkServiceId', resourceId('Microsoft.ServiceBus/namespaces', parameters('name')), 'groupIds', createArray(coalesce(tryGet(coalesce(parameters('privateEndpoints'), createArray())[copyIndex()], 'service'), 'namespace')))))), createObject('value', null()))]",
  "manualPrivateLinkServiceConnections": "[if(equals(tryGet(coalesce(parameters('privateEndpoints'), createArray())[copyIndex()], 'manualPrivateLinkServiceConnections'), true()), createObject('value', createArray(createObject('name', coalesce(tryGet(coalesce(parameters('privateEndpoints'), createArray())[copyIndex()], 'privateLinkServiceConnectionName'), format('{0}-{1}-{2}', last(split(resourceId('Microsoft.ServiceBus/namespaces', parameters('name')), '/')), coalesce(tryGet(coalesce(parameters('privateEndpoints'), createArray())[copyIndex()], 'service'), 'namespace'), copyIndex())), 'properties', createObject('privateLinkServiceId', resourceId('Microsoft.ServiceBus/namespaces', parameters('name')), 'groupIds', createArray(coalesce(tryGet(coalesce(parameters('privateEndpoints'), createArray())[copyIndex()], 'service'), 'namespace')), 'requestMessage', coalesce(tryGet(coalesce(parameters('privateEndpoints'), createArray())[copyIndex()], 'manualConnectionRequestMessage'), 'Manual approval required.'))))), createObject('value', null()))]"
}

v.0.3.0

{
  "privateLinkServiceConnections": {
    "value": [
      {
        "name": "[parameters('name')]",
        "properties": {
          "privateLinkServiceId": "[resourceId('Microsoft.ServiceBus/namespaces', parameters('name'))]",
          "groupIds": [
            "[coalesce(tryGet(coalesce(parameters('privateEndpoints'), createArray())[copyIndex()], 'service'), 'namespace')]"
          ]
        }
      }
    ]
  },
  "manualPrivateLinkServiceConnections": {
    "value": "[tryGet(coalesce(parameters('privateEndpoints'), createArray())[copyIndex()], 'manualPrivateLinkServiceConnections')]"
  }
}
AlexanderSehr commented 7 months ago

createArray

Hey @JanKnipp, thanks for the investigation. There is one bug that I'm aware of the we'll fix for quite a few modules as part of https://github.com/Azure/bicep-registry-modules/pull/1472, most notably,

the conditions

privateLinkServiceConnections: privateEndpoint.?manualPrivateLinkServiceConnections != true 
manualPrivateLinkServiceConnections: privateEndpoint.?manualPrivateLinkServiceConnections == true

should be

privateLinkServiceConnections: privateEndpoint.?isManualConnection != true 
manualPrivateLinkServiceConnections: privateEndpoint.?isManualConnection== true

as manualPrivateLinkServiceConnections is a property that doesn't exist in the UDT (anymore), so the condition privateEndpoint.?manualPrivateLinkServiceConnections will always be false. But from what you wrote above, I don't think that's the cause for your problem, as you're not trying to deploy manualPrivateLinkServiceConnections. I anyways wanted to mention it as it's a known issue.

cc: @krbar , @eriqua

AlexanderSehr commented 7 months ago

This is the change when comparing the 2 git tags for the service-bus namespace: image

What's notable is the change in the name which, by default, is now NOT the name of the main resource (there was an issue complaining about that, hence the change), but a calculated default.

That said, @JanKnipp, you should be able to solve your issue by setting the property privateLinkServiceConnectionName explicitely on the parameter. The name would then be the same that you use for the main resource's name (i.e., the service-bus namespace's name parameter`).

Could you please give that a shot and let us know the outcome?

JanKnipp commented 7 months ago

Thansk @AlexanderSehr for looking at the issue. Currently I don't understand how to set this value when using the service bus module as the value does not seem to be exposed in the service bus module - are you suggesting to split and use the private-endpoint module at this point or am I missing something?

AlexanderSehr commented 7 months ago

Hey @JanKnipp, that may or may not have given me a heart attack. But you're absolutely right, the User-Defined-Type used by the PrivateEndpoint parameter is missing the privateLinkServiceConnectionName in it's declaration. How unfortunate that we just merged a PR for private endpoints in yesterday. Ah well - I'll look into it and raise a PR asap.

Edit: Ref https://github.com/Azure/bicep-registry-modules/pull/1614 & https://github.com/Azure/Azure-Verified-Modules/pull/828

AlexanderSehr commented 6 months ago

All modules will non-failing workflows were updated. The remaining are tracked here #1583

Julicher commented 6 months ago

Hi,

just to validate as i have the same issue while migrating to avm from a forked Azure resource modules repo. Now with the above solution i got it to work for multiple modules, so thanks! But not for the storage account module unfortunately, even with the version 0.6.0 as @JanKnipp points out in the original message.

@AlexanderSehr I saw as well that the solution regarding privateLinkServiceConnectionName is not a part of the storage account module in the avm repository yet, is this intended behavior and is there perhaps a workaround, or is this currently still in development?

highly apricated, Thanks Cas

ChrisSidebotham commented 6 months ago

@Julicher - Please can you check again with the latest version of the module - version 0.8.3 is now available

Julicher commented 6 months ago

Hi @ChrisSidebotham thank you for the reply, but I've tried 0.8.3 but generates the same error unfortunately.

Please ensure you are not updating the details of existing private link connection: '/subscriptions/xxxx/resourceGroups/xxxx/providers/Microsoft.Network/privateEndpoints/xxxx/privateLinkServiceConnections/xxxxx'. That is not allowed. (Code: CannotChangePrivateLinkConnectionOnPrivateEndpoint)

example code:

module storage 'br/public:avm/res/storage/storage-account:0.8.3' = {
  name: storageAccountName
  scope: resourceGroup(privateSubscriptionId, privateResourceGroupName)
  params: {
    name: storageAccountName
    location: location
    tags: tags
    roleAssignments: roleAssignmentAccessConnector
    enableHierarchicalNamespace: enableHierarchicalNamespace
    blobServices: blobServicesAtlasStorage
    allowBlobPublicAccess: allowBlobPublicAccess
    skuName: storageRedundancy
    requireInfrastructureEncryption: enableInfrastructureEncryption
    publicNetworkAccess: 'Disabled'
    networkAcls: defaultNetworkAclsBypassAzureServices
    privateEndpoints: [
      {
        service: 'dfs'
        name: privateEndpointNameStorageAccountDfs
        location: location
        tags: tags
        subnetResourceId: privateLinkSubnetResourceId
        customNetworkInterfaceName: networkInterfaceNameStorageAccountDfs
      }
    ]
    customerManagedKey: {
      keyName: cmkName
      keyVaultResourceId:  keyVaultOnPrem.outputs.resourceId
      userAssignedIdentityResourceId: cmkUserAssignedIdentityIdStorage
    }
    managedIdentities: {
      systemAssigned: false
      userAssignedResourceIds: [cmkUserAssignedIdentityIdStorage]
    }
  }
}

And the privateLinkServiceConnectionName parameter is not available.

AlexanderSehr commented 6 months ago

Hi @ChrisSidebotham thank you for the reply, but I've tried 0.8.3 but generates the same error unfortunately.

Please ensure you are not updating the details of existing private link connection: '/subscriptions/xxxx/resourceGroups/xxxx/providers/Microsoft.Network/privateEndpoints/xxxx/privateLinkServiceConnections/xxxxx'. That is not allowed. (Code: CannotChangePrivateLinkConnectionOnPrivateEndpoint)

example code:

module storage 'br/public:avm/res/storage/storage-account:0.8.3' = {
  name: storageAccountName
  scope: resourceGroup(privateSubscriptionId, privateResourceGroupName)
  params: {
    name: storageAccountName
    location: location
    tags: tags
    roleAssignments: roleAssignmentAccessConnector
    enableHierarchicalNamespace: enableHierarchicalNamespace
    blobServices: blobServicesAtlasStorage
    allowBlobPublicAccess: allowBlobPublicAccess
    skuName: storageRedundancy
    requireInfrastructureEncryption: enableInfrastructureEncryption
    publicNetworkAccess: 'Disabled'
    networkAcls: defaultNetworkAclsBypassAzureServices
    privateEndpoints: [
      {
        service: 'dfs'
        name: privateEndpointNameStorageAccountDfs
        location: location
        tags: tags
        subnetResourceId: privateLinkSubnetResourceId
        customNetworkInterfaceName: networkInterfaceNameStorageAccountDfs
      }
    ]
    customerManagedKey: {
      keyName: cmkName
      keyVaultResourceId:  keyVaultOnPrem.outputs.resourceId
      userAssignedIdentityResourceId: cmkUserAssignedIdentityIdStorage
    }
    managedIdentities: {
      systemAssigned: false
      userAssignedResourceIds: [cmkUserAssignedIdentityIdStorage]
    }
  }
}

And the privateLinkServiceConnectionName parameter is not available.

Hey @Julicher & @ChrisSidebotham, while almost all modules have been updated there are still a few outliers that are tracked here #1583. Storage Acocunt is one of them. The reason it isn't updated yet is because the pipeline has been failing the WAF-Reliablity tests by PSRule for a while now and we opted to not introduce new features until this is fixed as they'd not be published to the BRM anyways as the pipeline would just fail again. @fblix is working on the issue (#1731) and once it's resolved we'll update the interface right away. It's a very unfortunate blocking state :/

ChrisSidebotham commented 5 months ago

New version of the module published in #1987 - Closing this as completed

jonasssa commented 3 months ago

This is still an issue for the avm/res/sql/server module I believe @ChrisSidebotham . Can this be reopened until that is solved?