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

[#DEVEX-116] Create Dev APIM and bind OpenAPI specs and default Policy #22

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BurnedMarshal
Copy link
Contributor

A test development APIM is created under DEV-IO subscription.
Created all the related resources.
Use default empty policy for product and api.
The function OpenAPI specification are divided into internal and external. The internal spec describe how the API are implemented al the application level, instead the external describe as the API are served outside the APIM.
Updated the pipeline to handle the new environment dev/italynorth for the resources.

@@ -10,7 +10,7 @@ on:
- ready_for_review
paths:
# Trigger the workflow when resources are modified
- "infra/resources/**"
- "infra/resources/prod/westeurope/**"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please get rid of line 25 as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, we need to remove the region parameter (westeurope) which is not used in the new workflow

ARM_CLIENT_ID: ${{ secrets.ARM_CLIENT_ID }}
with:
environment: dev
region: italynorth
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

"ARM_SUBSCRIPTION_ID" = data.azurerm_subscription.current.subscription_id
"ARM_TENANT_ID" = data.azurerm_client_config.current.tenant_id,
"ARM_SUBSCRIPTION_ID" = data.azurerm_subscription.current.subscription_id
"ARM_DEV_SUBSCRIPTION_ID" = data.azurerm_subscription.dev.subscription_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need another identity and another github environment to manage DEV-IO as current identities have roles on PROD-IO only

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why dx-typescript runners have roles on PROD-IO ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have moved the state into DEV-ENGINEERING subscription

subscription_required = false # Public API

content_format = "openapi"
content_value = templatefile("../../../../apps/azure-functions-api/api/external.yaml",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it works, probably you need to use ${path.module} or anything similar

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this update the resource during apply if the content changes?

}
)

xml_content = file("../../../../apps/azure-functions-api/api/policy/default.xml")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

infra/resources/dev/italynorth/keyvault.tf Outdated Show resolved Hide resolved
@@ -0,0 +1,18 @@
resource "azurerm_resource_group" "sec_rg" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use another resource group

publisher_email = data.azurerm_key_vault_secret.apim_publisher_email.value
notification_sender_email = data.azurerm_key_vault_secret.apim_publisher_email.value
sku_name = "Developer_1"
virtual_network_type = "Internal"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a private endpoint here?

infra/resources/dev/italynorth/monitor.tf Outdated Show resolved Hide resolved
infra/resources/dev/italynorth/monitor.tf Outdated Show resolved Hide resolved
"ARM_SUBSCRIPTION_ID" = data.azurerm_subscription.current.subscription_id
"ARM_TENANT_ID" = data.azurerm_client_config.current.tenant_id,
"ARM_SUBSCRIPTION_ID" = data.azurerm_subscription.current.subscription_id
"ARM_DEV_SUBSCRIPTION_ID" = data.azurerm_subscription.dev.subscription_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why dx-typescript runners have roles on PROD-IO ?

@@ -32,3 +32,7 @@ provider "github" {
data "azurerm_client_config" "current" {}

data "azurerm_subscription" "current" {}

data "azurerm_subscription" "dev" {
subscription_id = "a4e96bcd-59dc-4d66-b2f7-5547ad157c12" # DEV-IO
Copy link
Contributor

@gunzip gunzip Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use DEV-ENGINEERING

subscription_required = false # Public API

content_format = "openapi"
content_value = templatefile("../../../../apps/azure-functions-api/api/external.yaml",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this update the resource during apply if the content changes?

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarCloud

@@ -10,7 +10,7 @@ on:
- ready_for_review
paths:
# Trigger the workflow when resources are modified
- "infra/resources/**"
- "infra/resources/prod/westeurope/**"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, we need to remove the region parameter (westeurope) which is not used in the new workflow

ARM_CLIENT_ID: ${{ secrets.ARM_CLIENT_ID }}
with:
environment: dev
region: italynorth
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
region: italynorth

"ARM_SUBSCRIPTION_ID" = data.azurerm_subscription.current.subscription_id
"ARM_TENANT_ID" = data.azurerm_client_config.current.tenant_id,
"ARM_SUBSCRIPTION_ID" = data.azurerm_subscription.current.subscription_id
"ARM_DEV_SUBSCRIPTION_ID" = data.azurerm_subscription.dev.subscription_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have moved the state into DEV-ENGINEERING subscription

protocols = ["http", "https"]
product_ids = [module.apim_product_example_api.product_id]

service_url = null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this? (I'd add a comment)

]
}

resource "azurerm_network_security_group" "nsg_apim" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a comment on what's happening here

allocation_method = "Static"
sku = "Standard"
domain_name_label = "apimdx"
zones = ["1", "2", "3"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this?

Comment on lines +106 to +113
default_ssl_binding = false
host_name = format("%s-apim-api.azure-api.net", local.project)
key_vault_id = null
},
]
developer_portal = null
management = null
portal = null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add some comments to clarify the reason of this params


autoscale = local.apim_autoscale

alerts_enabled = local.apim_alerts_enabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as an example? probably. A nice to have would be to have this logic inside a more abstract APIM module (with presets for autoscaling + preconfigured alerts)

resource_group_name = "terraform-state-rg"
storage_account_name = "tfdevdx"
container_name = "terraform-state"
key = "dx-typescript.resources.dev.italynorth.tfstate"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants