feat: add public ip address support for `/res/api-management/service` #1795

tony-box · 2024-04-30T17:02:30Z

Description

Adds APIM support for PublicIPAddress

Pipeline Reference

Pipeline

Type of Change

Update to CI Environment or utlities (Non-module effecting changes)
Azure Verified Module updates:
- Bugfix containing backwards compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in version.json:
  - Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description.
  - The bug was found by the module author, and no one has opened an issue to report it yet.
- Feature update backwards compatible feature updates, and I have bumped the MINOR version in version.json.
- Breaking changes and I have bumped the MAJOR version in version.json.
- Update to documentation

Checklist

I'm sure there are no other open Pull Requests for the same update/change
I have run Set-AVMModule locally to generate the supporting module files.
My corresponding pipelines / checks run clean and green without any errors or warnings

AlexanderSehr · 2024-05-03T16:07:54Z

.github/workflows/avm.ptn.security-center.yml

@@ -0,0 +1,86 @@
+name: "avm.ptn.security-center"


Beware, PR invaders

AlexanderSehr · 2024-05-03T16:08:58Z

avm/res/api-management/service/README.md

@@ -1532,39 +1551,6 @@ A list of availability zones denoting where the resource needs to come from.

 _None_

-## Notes


Just doublecheckinf if this was intended. You're more than welcome to remove these notes that originally came from CARML. Just want to make sure you did know about them 😉

AlexanderSehr · 2024-05-03T16:11:30Z

avm/res/api-management/service/main.bicep

@@ -129,6 +128,9 @@ param products array = []
 @description('Optional. Subscriptions.')
 param subscriptions array = []

+@description('Optional. Public Standard SKU IP V4 based IP address to be associated with Virtual Network deployed service in the region. Supported only for Developer and Premium SKU being deployed in Virtual Network.')
+param publicIpAddressId string = ''


Ideally, to be explicit, we'd call this publicIpAddressResourceId. That being said, the parameter sohuld probably be nullable, as the pipeline seems to not like the '' as a value

AlexanderSehr · 2024-05-03T16:12:43Z

avm/res/api-management/service/main.bicep

@@ -141,7 +143,7 @@ var identity = !empty(managedIdentities)
  ? {
      type: (managedIdentities.?systemAssigned ?? false)
        ? (!empty(managedIdentities.?userAssignedResourceIds ?? {}) ? 'SystemAssigned,UserAssigned' : 'SystemAssigned')
-        : (!empty(managedIdentities.?userAssignedResourceIds ?? {}) ? 'UserAssigned' : 'None')
+        : (!empty(managedIdentities.?userAssignedResourceIds ?? {}) ? 'UserAssigned' : null)


Why this change? It seems to bring back the RP warning
/home/runner/work/bicep-registry-modules/bicep-registry-modules/avm/res/api-management/service/main.bicep(142,16) : Warning BCP321: Expected a value of type "string" but the provided value is of type "'SystemAssigned' | 'SystemAssigned,UserAssigned' | 'UserAssigned' | null".

AlexanderSehr · 2024-05-03T16:14:23Z

avm/res/api-management/service/tests/e2e/defaults/main.test.bicep

-      publisherEmail: '[email protected]'
-      publisherName: '${namePrefix}-az-amorg-x-001'
-    }
+module testDeployment '../../../main.bicep' = [for iteration in [ 'init', 'idem' ]: {


Beware, you're not using the latest Bicep version which is why every Bicep file (& JSON file) got re-formatted / updated.

Please upgrade to the latest (check bicep --version) and run Set-AVMModule -Recurse on the root folder path of this module again :)

AlexanderSehr · 2024-05-03T16:18:39Z

avm/res/api-management/service/tests/e2e/max/dependencies.bicep

+@description('VNet name')
+param vnetName string = 'VNet'
+
+@description('Address prefix')
+param vnetAddressPrefix string = '10.0.0.0/16'
+
+@description('Subnet Prefix')
+param subnetPrefix string = '10.0.0.0/24'
+
+@description('Subnet Name')
+param subnetName string = 'Subnet'
+
+@description('DNS Prefix')
+param dnsLabelPrefix string


This and the next is just a recommendation to align with other test cases. Feel free to reject if you prefer the original implementation 😉

The rationale is that some inner resources like the subnet name or network properties don't need to be globally unique. Hence there is no need to have them as parameters.
The same could be set for, for example the VNETName, but these we usually pass in regardless to have a single location where one could find all the main resource names (with namePrefix & all).

Suggested change

@description('VNet name')

param vnetName string = 'VNet'

@description('Address prefix')

param vnetAddressPrefix string = '10.0.0.0/16'

@description('Subnet Prefix')

param subnetPrefix string = '10.0.0.0/24'

@description('Subnet Name')

param subnetName string = 'Subnet'

@description('DNS Prefix')

param dnsLabelPrefix string

@description('Required. The DNS prefix for the Public IP')

param publicIpDnsLabelPrefix string

@description('Required. The name of the Virtual Network to create.')

param virtualNetworkName string

@description('Required. The name of the NSG to create.')

param networkSecurityGroupName string

var addressPrefix = '10.0.0.0/16'

AlexanderSehr · 2024-05-03T16:24:17Z

avm/res/api-management/service/tests/e2e/max/dependencies.bicep

+resource vnet 'Microsoft.Network/virtualNetworks@2023-04-01' = {
+  name: vnetName
+  location: location
+  properties: {
+    addressSpace: {
+      addressPrefixes: [
+        vnetAddressPrefix
+      ]
+    }
+    subnets: [
+      {
+        name: subnetName
+        properties: {
+          addressPrefix: subnetPrefix
+          networkSecurityGroup: {
+            id: nsg.id
+          }
+          serviceEndpoints: [
+            {
+              service: 'Microsoft.Storage'
+            }
+            {
+              service: 'Microsoft.Sql'
+            }
+            {
+              service: 'Microsoft.KeyVault'
+            }
+          ]
+        }
+      }
+    ]
+  }
+}
+
+resource nsg 'Microsoft.Network/networkSecurityGroups@2020-06-01' = {
+  name: 'testNSG'


For rationale, see other comment.

Suggested change

resource vnet 'Microsoft.Network/virtualNetworks@2023-04-01' = {

name: vnetName

location: location

properties: {

addressSpace: {

addressPrefixes: [

vnetAddressPrefix

]

}

subnets: [

{

name: subnetName

properties: {

addressPrefix: subnetPrefix

networkSecurityGroup: {

id: nsg.id

}

serviceEndpoints: [

{

service: 'Microsoft.Storage'

}

{

service: 'Microsoft.Sql'

}

{

service: 'Microsoft.KeyVault'

}

]

}

}

]

}

}

resource nsg 'Microsoft.Network/networkSecurityGroups@2020-06-01' = {

name: 'testNSG'

resource vnet 'Microsoft.Network/virtualNetworks@2023-04-01' = {

name: virtualNetworkName

location: location

properties: {

addressSpace: {

addressPrefixes: [

addressPrefix

]

}

subnets: [

{

name: 'default'

properties: {

addressPrefix: cidrSubnet(addressPrefix, 24, 0)

networkSecurityGroup: {

id: nsg.id

}

serviceEndpoints: [

{

service: 'Microsoft.Storage'

}

{

service: 'Microsoft.Sql'

}

{

service: 'Microsoft.KeyVault'

}

]

}

}

]

}

}

resource nsg 'Microsoft.Network/networkSecurityGroups@2020-06-01' = {

name: networkSecurityGroupName

AlexanderSehr · 2024-05-03T16:25:34Z

avm/res/api-management/service/tests/e2e/max/dependencies.bicep

+    publicIPAllocationMethod: 'Static'
+    publicIPAddressVersion: 'IPv4'
+    dnsSettings: {
+      domainNameLabel: dnsLabelPrefix


See other comment

Suggested change

domainNameLabel: dnsLabelPrefix

domainNameLabel: publicIpDnsLabelPrefix

AlexanderSehr · 2024-05-03T16:26:14Z

avm/res/api-management/service/tests/e2e/max/main.test.bicep

@@ -41,6 +41,8 @@ module nestedDependencies 'dependencies.bicep' = {
  params: {
    managedIdentityName: 'dep-${namePrefix}-msi-${serviceShort}'
    location: resourceLocation
+    publicIPName: 'dep-${namePrefix}-pip-${serviceShort}'
+    dnsLabelPrefix: 'dep-${namePrefix}-dnsprefix-${uniqueString(deployment().name, resourceLocation)}'


Suggested change

dnsLabelPrefix: 'dep-${namePrefix}-dnsprefix-${uniqueString(deployment().name, resourceLocation)}'

publicIpDnsLabelPrefix: 'dep-${namePrefix}-dnsprefix-${uniqueString(deployment().name, resourceLocation)}'

tony-box · 2024-05-20T23:47:35Z

Been a bit busy, but thank you for all the recommendations @AlexanderSehr -- I didn't do a thorough review of the way this was set up during the CARML -> AVM migraiton so there are still a lot of relics from CARML. I will make the changes you suggested. I'm still working on fixing the max test deployment which includes creating a publicIP. have been running into issues with it and have even raised an Azure support ticket. I'm still working on my branch so there will be more updates coming

tony-box · 2024-05-23T03:40:43Z

I am going to re-do this whole pull request now that I have my local branches sorted out and the commits cleaned up. Stay tuned...

eriqua

Hey @tony-box is this PR still valid or should we close it as superseded?

tony-box · 2024-06-15T21:49:55Z

closing this PR which will be superceeded by #2453

Tony Box added 6 commits April 24, 2024 10:52

adding ptn/security-center

58bd4ad

adding security-center workflow

8f63d16

adding publicip capability

2eae14e

moving to right directory structure

09fab8c

Merge branch 'main' into res/apimanagement

e0760ed

adding publicIPAddress support

ced21e1

tony-box requested review from a team as code owners April 30, 2024 17:02

microsoft-github-policy-service bot added Needs: Triage 🔍 Maintainers need to triage still Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue labels Apr 30, 2024

tony-box added Class: Resource Module 📦 This is a resource module Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue and removed Needs: Triage 🔍 Maintainers need to triage still Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue labels Apr 30, 2024

adding more dependencies for max test

402169f

AlexanderSehr reviewed May 3, 2024

View reviewed changes

eriqua assigned tony-box May 12, 2024

eriqua added the Needs: Author Feedback 👂 Awaiting feedback from the issue/PR author label May 12, 2024

removed accidental nsg rule

9aaa689

tony-box changed the title ~~feature: add public ip address support for /res/api-management/service~~ feat: add public ip address support for /res/api-management/service May 20, 2024

microsoft-github-policy-service bot added Needs: Attention 👋 Reply has been added to issue, maintainer to review and removed Needs: Author Feedback 👂 Awaiting feedback from the issue/PR author labels May 20, 2024

changing to Internal vnet deployment to fix comms issue. Update readme

d4daddb

eriqua mentioned this pull request May 22, 2024

fix: v2sku for APIM - avm/res/api-management/service #1989

Closed

11 tasks

eriqua added the Needs: Core Team 🧞 This item needs the AVM Core Team to review it label Jun 6, 2024

eriqua reviewed Jun 6, 2024

View reviewed changes

eriqua added the Needs: Author Feedback 👂 Awaiting feedback from the issue/PR author label Jun 6, 2024

tony-box closed this Jun 15, 2024

tony-box deleted the apim/addPublicIP branch June 15, 2024 21:49

microsoft-github-policy-service bot removed the Needs: Author Feedback 👂 Awaiting feedback from the issue/PR author label Jun 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add public ip address support for `/res/api-management/service` #1795

feat: add public ip address support for `/res/api-management/service` #1795

tony-box commented Apr 30, 2024

AlexanderSehr May 3, 2024

AlexanderSehr May 3, 2024

AlexanderSehr May 3, 2024

AlexanderSehr May 3, 2024

AlexanderSehr May 3, 2024

AlexanderSehr May 3, 2024 •

edited

Loading

AlexanderSehr May 3, 2024 •

edited

Loading

AlexanderSehr May 3, 2024

AlexanderSehr May 3, 2024

tony-box commented May 20, 2024

tony-box commented May 23, 2024

eriqua left a comment

tony-box commented Jun 15, 2024

		@@ -1532,39 +1551,6 @@ A list of availability zones denoting where the resource needs to come from.

		_None_

		## Notes

	domainNameLabel: dnsLabelPrefix
	domainNameLabel: publicIpDnsLabelPrefix

	dnsLabelPrefix: 'dep-${namePrefix}-dnsprefix-${uniqueString(deployment().name, resourceLocation)}'
	publicIpDnsLabelPrefix: 'dep-${namePrefix}-dnsprefix-${uniqueString(deployment().name, resourceLocation)}'

feat: add public ip address support for /res/api-management/service #1795

feat: add public ip address support for /res/api-management/service #1795

Conversation

tony-box commented Apr 30, 2024

Description

Pipeline Reference

Type of Change

Checklist

AlexanderSehr May 3, 2024

Choose a reason for hiding this comment

AlexanderSehr May 3, 2024

Choose a reason for hiding this comment

AlexanderSehr May 3, 2024

Choose a reason for hiding this comment

AlexanderSehr May 3, 2024

Choose a reason for hiding this comment

AlexanderSehr May 3, 2024

Choose a reason for hiding this comment

AlexanderSehr May 3, 2024 • edited Loading

Choose a reason for hiding this comment

AlexanderSehr May 3, 2024 • edited Loading

Choose a reason for hiding this comment

AlexanderSehr May 3, 2024

Choose a reason for hiding this comment

AlexanderSehr May 3, 2024

Choose a reason for hiding this comment

tony-box commented May 20, 2024

tony-box commented May 23, 2024

eriqua left a comment

Choose a reason for hiding this comment

tony-box commented Jun 15, 2024

feat: add public ip address support for `/res/api-management/service` #1795

feat: add public ip address support for `/res/api-management/service` #1795

AlexanderSehr May 3, 2024 •

edited

Loading

AlexanderSehr May 3, 2024 •

edited

Loading