feat: api-management.service new v0.2 #2453

tony-box · 2024-06-15T21:37:44Z

Description

Closes #2075
Closes #1929
Closes #1124
Closes #1104
Closes #2362
Closes #2444
Fixes #2361
Closes #1473

Big update v0.2 release:

update all APIM apis to newer versions
new feat: Add PublicIP
new feat: add API Loggers
new feat: add API Diagnostics
new feat: add v2sku options
add test case for v2sku
updates to many default params to better align with PSRule suggestions for better reliability and security (big one is multi region and AZ deployment)

Pipeline Reference

Pipeline

Type of Change

Update to CI Environment or utilities (Non-module affecting 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

…s into fix/v2sku

tony-box · 2024-06-15T21:46:58Z

@Azure/avm-core-team-technical-bicep sorry for the large PR but I thought it was the best way to get everything submitted in a timely manner. I promise I won't make this a habit... just trying to get a bunch of these open bugs resolved.

jtracey93

LGTM :rocket

## Description  Closes Azure#2075 Closes Azure#1929 Closes Azure#1124 Closes Azure#1104 Closes Azure#2362 Closes Azure#2444 Big update v0.2 release: - update all APIM apis to newer versions - new feat: Add PublicIP - new feat: add API Loggers - new feat: add API Diagnostics - new feat: add v2sku options - add test case for v2sku - updates to many default params to better align with PSRule suggestions for better reliability and security ## Pipeline Reference  | Pipeline | | -------- | | [![avm.res.api-management.service](https://github.com/tony-box/bicep-registry-modules/actions/workflows/avm.res.api-management.service.yml/badge.svg?branch=v0.2)](https://github.com/tony-box/bicep-registry-modules/actions/workflows/avm.res.api-management.service.yml) | ## Type of Change  - [x] Update to CI Environment or utilities (Non-module affecting changes) - [x] Azure Verified Module updates: - [ ] Bugfix containing backwards-compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in `version.json`: - [x] 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. - [x] 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`. - [x] Update to documentation ## Checklist - [x] I'm sure there are no other open Pull Requests for the same update/change - [x] I have run `Set-AVMModule` locally to generate the supporting module files. - [x] My corresponding pipelines / checks run clean and green without any errors or warnings  --------- Co-authored-by: Tony Box <[email protected]>

AlexanderSehr · 2024-06-20T09:20:46Z

avm/utilities/pipelines/staticValidation/psrule/ps-rule.yaml

@@ -81,3 +81,4 @@ rule:
    - Azure.VM.UseHybridUseBenefit
    - Azure.Storage.UseReplication
    - Azure.AppConfig.PurgeProtect
+    - Azure.APIM.MultiRegion


What was ther reson for this? We should probably start attaching comments with the reason to every suppression

we agreed in Teams to disable this one. It's untenable for most deployments to use multiregion deployment for APIM in every case ($$$), despite it being recommended.

Hey Tony, the reason is absolutely fine. If possible please add it as a comment at the end of the line. Something we should really start doing for later reference 🙂

AlexanderSehr · 2024-06-20T09:22:56Z

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

+}
+
+resource diagnostic 'Microsoft.ApiManagement/service/apis/diagnostics@2022-08-01' = {
+  name: diagnosticName


Should not be called diagnosticName but name as per spec Inputs - Parameter/Variable Naming

AlexanderSehr · 2024-06-20T09:23:30Z

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

-resource api 'Microsoft.ApiManagement/service/apis@2021-08-01' = {
-  name: name
+resource api 'Microsoft.ApiManagement/service/apis@2022-08-01' = {
+  name: apiName


Should be name as per spec Inputs - Parameter/Variable Naming

AlexanderSehr · 2024-06-20T09:24:30Z

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

 @description('Required. Relative URL uniquely identifying this API and all of its resource paths within the API Management service instance. It is appended to the API endpoint base URL specified during the service instance creation to form a public URL for this API.')
-param path string
+param apiPath string


Should be api as per spec Inputs - Parameter/Variable Naming

Background: As it's the path parameter of the api resource type it should be intuitive that it's the api's path. Following this logic it would be redundant to call it apiPath

AlexanderSehr · 2024-06-20T09:26:59Z

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

+  for (diagnostic, index) in diagnostics ?? []: {
+    name: '${deployment().name}-diagnostics-${index}'
+    params: {
+      diagnosticName: !empty(diagnostic.diagnosticName) ? diagnostic.diagnosticName : '${api.name}-diagnostics-${index}'


Why does have a different default than in the child?

There the diagnosticName parameter's default is 'local'. It's very different and it may have been better to use '${apiName}-diagnostics' in the child

Also, just to suggest an improvement to simplify the code, you could write diagnosticName: diagnostic.?diagnosticName ?? '${api.name}-diagnostics-${index}'

AlexanderSehr · 2024-06-20T09:28:48Z

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

+param targetResourceId string
+
+@description('Conditional. Required if loggerType = applicationInsights or azureEventHub. The name and SendRule connection string of the event hub for azureEventHub logger. Instrumentation key for applicationInsights logger.')
+param credentials object


Should this not maybe have the annotation @secure() or are there no secrets in this object?

AlexanderSehr · 2024-06-20T09:29:33Z

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

@@ -54,7 +63,7 @@ param restore bool = false
 @description('Optional. Array of role assignments to create.')
 param roleAssignments roleAssignmentType

-@description('Optional. The pricing tier of this API Management service.')
+@description('Optional. The pricing tier of this API Management service. Default is Premium.')


This is representative of all parameters - the default should not be part of the name because

in the readme.md it's automatically added from the default

in Bicep linter (when using the module) it's automatically suggested

It adds another location (per parameter) where you'd need to maintain the value - so it could run stale

Suggested change

@description('Optional. The pricing tier of this API Management service. Default is Premium.')

@description('Optional. The pricing tier of this API Management service.')

This would be the same comment across all modules, btw :) I'll refrain from commenting every instance though

AlexanderSehr · 2024-06-20T09:31:50Z

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

+}
+
+resource diagnostic 'Microsoft.ApiManagement/service/apis/diagnostics@2022-08-01' = {
+  name: diagnosticName


Same comment as per the product api - should not contain the resource type's name as per Inputs - Parameter/Variable Naming

AlexanderSehr · 2024-06-20T09:35:43Z

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

+      managedIdentities: {
+        systemAssigned: true
+        userAssignedResourceIds: [
+          nestedDependencies.outputs.managedIdentityResourceId
+        ]
+      }


Required for WAF alignment?

yup, required for Azure.APIM.ManagedIdentity

v-xuto · 2024-06-21T09:15:12Z

@tony-box The changes related to this PR seem to have not been published successfully, please confirm.

AlexanderSehr · 2024-06-21T09:39:47Z

Hey @v-xuto that's correct. One of the tests failed due to an internal server error. We'll merge a new PR asap - including the suggested changes above. @tony-box started working on it right away

## Description  Fixes issues from the previous PR: #2453 ## Pipeline Reference  | Pipeline | | -------- | | [![avm.res.api-management.service](https://github.com/tony-box/bicep-registry-modules/actions/workflows/avm.res.api-management.service.yml/badge.svg?branch=fix%2Fapim)](https://github.com/tony-box/bicep-registry-modules/actions/workflows/avm.res.api-management.service.yml) | ## Type of Change  - [X] Update to CI Environment or utilities (Non-module affecting changes) - [X] Azure Verified Module updates: - [X] 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. - [X] 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`. - [X] Update to documentation ## Checklist - [X] I'm sure there are no other open Pull Requests for the same update/change - [X] I have run `Set-AVMModule` locally to generate the supporting module files. - [X] My corresponding pipelines / checks run clean and green without any errors or warnings  --------- Co-authored-by: Tony Box <[email protected]>

Tony Box and others added 30 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

adding more dependencies for max test

402169f

removed accidental nsg rule

9aaa689

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

d4daddb

cleaning up weird main branch stuff

dedaa28

adding apim logger and tests for it

4dad1d6

adding api/diagnostics

19b9575

updating readmes

d842c86

upgrading sku and deployment zones for best practice

7dfdc34

update sku and add zones

c5254b2

Update sku and add zones

04d8184

addin metrics

586e12f

Update main.test.bicep

f8505ca

Update main.test.bicep

da4dd3a

Update main.test.bicep

ad62bf3

adding metrics

19d888c

update readme

854c493

adding additional location to waf test

8b269c4

adding additional location to waf test

be1c7f8

update readme

1ec2bb3

Merge branch 'main' into fix/v2sku

aa2924f

updating all API versions to 2022-08-01

84ebccb

Merge branch 'fix/v2sku' of github.com:tony-box/bicep-registry-module…

e2a018e

…s into fix/v2sku

updating readmes

ef43e88

disable APIM MultiRegion PSRule -- too expensive for most use cases

82113a1

tony-box requested a review from a team as a code owner June 15, 2024 21:45

tony-box enabled auto-merge (squash) June 15, 2024 21:45

jongio mentioned this pull request Jun 17, 2024

Update the infra/core modules to AVM modules Azure/azure-dev#3976

Open

v-xuto mentioned this pull request Jun 18, 2024

[AVM Module Issue]: API Management Module should add the creation of 'Microsoft.ApiManagement/service/loggers' and 'Microsoft.ApiManagement/service/apis/diagnostics' #1124

Closed

1 task

Merge branch 'main' into v0.2

6f3f34e

matebarabas added Class: Resource Module 📦 This is a resource module Needs: Core Team 🧞 This item needs the AVM Core Team to review it and removed Needs: Triage 🔍 Maintainers need to triage still labels Jun 19, 2024

jtracey93 approved these changes Jun 19, 2024

View reviewed changes

tony-box merged commit 8a869fa into Azure:main Jun 19, 2024
11 checks passed

tony-box deleted the v0.2 branch June 19, 2024 17:39

AlexanderSehr reviewed Jun 20, 2024

View reviewed changes

AlexanderSehr assigned tony-box Jun 21, 2024

tony-box mentioned this pull request Jun 23, 2024

fix: apim v0.2 pipeline issues #2530

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: api-management.service new v0.2 #2453

feat: api-management.service new v0.2 #2453

tony-box commented Jun 15, 2024 •

edited

Loading

tony-box commented Jun 15, 2024

jtracey93 left a comment

AlexanderSehr Jun 20, 2024

tony-box Jun 20, 2024

AlexanderSehr Jun 20, 2024

AlexanderSehr Jun 20, 2024

AlexanderSehr Jun 20, 2024

AlexanderSehr Jun 20, 2024

AlexanderSehr Jun 20, 2024

AlexanderSehr Jun 20, 2024

AlexanderSehr Jun 20, 2024

AlexanderSehr Jun 20, 2024

AlexanderSehr Jun 20, 2024

AlexanderSehr Jun 20, 2024

AlexanderSehr Jun 20, 2024

tony-box Jun 20, 2024

v-xuto commented Jun 21, 2024

AlexanderSehr commented Jun 21, 2024 •

edited

Loading

	@description('Optional. The pricing tier of this API Management service. Default is Premium.')
	@description('Optional. The pricing tier of this API Management service.')

feat: api-management.service new v0.2 #2453

feat: api-management.service new v0.2 #2453

Conversation

tony-box commented Jun 15, 2024 • edited Loading

Description

Pipeline Reference

Type of Change

Checklist

tony-box commented Jun 15, 2024

jtracey93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v-xuto commented Jun 21, 2024

AlexanderSehr commented Jun 21, 2024 • edited Loading

tony-box commented Jun 15, 2024 •

edited

Loading

AlexanderSehr commented Jun 21, 2024 •

edited

Loading