Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
chore: adding meshstack_integration.tf
🎨 Missing Building Block IconsFound 2 building block(s) without Copy the AI Prompts below and use them with your favorite AI image generator (Gemini, DALL-E, Midjourney, Stable Diffusion, etc.). Then follow the Post-Processing Steps to prepare the icons for upload. Azure Virtual Machine StarterkitPlatform: Path: AI Prompt (copy this to image generator)Post-Processing InstructionsStep 1: Remove white background with GIMP (free) a) Open image in GIMP Step 2: Resize to 800x800 pixels if needed
Step 3: Compress with pngquant (free command line tool)
Target specs: 800x800px PNG with transparent background, under 100KB Kubernetes Manifest (Helm)Platform: Path: AI Prompt (copy this to image generator)Post-Processing InstructionsStep 1: Remove white background with GIMP (free) a) Open image in GIMP Step 2: Resize to 800x800 pixels if needed
Step 3: Compress with pngquant (free command line tool)
Target specs: 800x800px PNG with transparent background, under 100KB |
grubmeshi
left a comment
grubmeshi
left a comment
There was a problem hiding this comment.
I did not review the functionailty, only the presented "API" of that BB as a component.
|
|
||
| ## Automation | ||
|
|
||
| We automate the deployment of the Service Principal Building Block using the common [Azure Building Blocks Automation Infrastructure](../automation.md). |
There was a problem hiding this comment.
| We automate the deployment of the Service Principal Building Block using the common [Azure Building Blocks Automation Infrastructure](../automation.md). | |
| We automate the deployment of the Service Principal Building Block using the common Azure Building Blocks Automation Infrastructure. |
such relative links arent working here?
| @@ -0,0 +1,30 @@ | |||
| output "documentation_md" { | |||
| value = <<EOF | |||
There was a problem hiding this comment.
| value = <<EOF | |
| value = <<-EOF |
then you can have proper indentation of that long block. also consider making use of templatefile and have this long text as an extra file
| description = "Azure management group or subscription ID used for backplane role scope." | ||
| } | ||
|
|
||
| variable "backplane_name" { |
There was a problem hiding this comment.
I don't understand this variable name backplane_name. is this BB serving two different purposes at once? should we split this then into two modules?
It needs at least some better description, I suppose.
(btw, you can have cross-references in hub modules with proper version pinning!)
| variable "notification_subscribers" { | ||
| type = list(string) | ||
| default = [] | ||
| description = "List of email addresses to notify on building block lifecycle events." | ||
| } |
There was a problem hiding this comment.
Would suggest to remove this? Is that configurable for other BBDs?
There was a problem hiding this comment.
Pull request overview
Adds an Azure “Service Principal” building block integration to the Hub, including a new backplane module to provision deployment permissions and extending the building block to support optional custom RBAC roles.
Changes:
- Added
modules/azure/service-principal/meshstack_integration.tfto register the building block definition in meshStack and wire it to the backplane. - Introduced a new
backplane/module that creates the deployment service principal, Graph permissions, and an RBAC role for building block deployment. - Extended the
buildingblock/module withcustom_rolesupport and updated outputs to returnnullinstead of"null"strings.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/azure/service-principal/meshstack_integration.tf | New meshStack integration + BBD definition and wiring to backplane |
| modules/azure/service-principal/buildingblock/variables.tf | Adds custom_role and changes azure_role semantics/validation |
| modules/azure/service-principal/buildingblock/main.tf | Adds optional custom role definition + makes role assignment conditional |
| modules/azure/service-principal/buildingblock/outputs.tf | Adds role/custom-role outputs and switches to null outputs |
| modules/azure/service-principal/buildingblock/README.md | Updates terraform-docs section for new inputs/outputs/resources |
| modules/azure/service-principal/backplane/main.tf | New backplane resources for deploy SP, Graph permissions, RBAC role |
| modules/azure/service-principal/backplane/outputs.tf | New backplane outputs (incl. provider snippet) |
| modules/azure/service-principal/backplane/provider.tf | Provider configuration for backplane |
| modules/azure/service-principal/backplane/variables.tf | Backplane inputs (scope, principals, optional WIF) |
| modules/azure/service-principal/backplane/versions.tf | Backplane Terraform/provider version constraints |
| modules/azure/service-principal/backplane/documentation.tf | Exposes markdown documentation output for backplane permissions |
| terraform = { | ||
| terraform_version = "1.9.0" | ||
| repository_url = "https://github.com/meshcloud/meshstack-hub.git" | ||
| repository_path = "modules/azure/service-principal/buildingblock" | ||
| ref_name = var.hub.git_ref | ||
| use_mesh_http_backend_fallback = true |
There was a problem hiding this comment.
terraform_version in the building block implementation is set to 1.9.0, but this repo documents a Terraform/OpenTofu baseline of >= 1.11.0. Please align the implementation runtime version to the baseline to avoid drift and feature incompatibilities.
| version = "~> 4.64" | ||
| } | ||
| azuread = { | ||
| source = "hashicorp/azuread" | ||
| version = "~> 3.8" |
There was a problem hiding this comment.
Provider versions here are not patch-pinned (e.g. ~> 4.64, ~> 3.8). The repo guidance is to use ~> X.Y.Z (patch pinning) for providers (except meshstack). Consider updating these to patch-pinned constraints for more predictable CI and consumer behavior.
| version = "~> 4.64" | |
| } | |
| azuread = { | |
| source = "hashicorp/azuread" | |
| version = "~> 3.8" | |
| version = "~> 4.64.0" | |
| } | |
| azuread = { | |
| source = "hashicorp/azuread" | |
| version = "~> 3.8.0" |
| version = "~> 4.36.0" | ||
| } | ||
| azuread = { | ||
| source = "hashicorp/azuread" | ||
| version = "~> 3.6.0" |
There was a problem hiding this comment.
The backplane module requires azurerm ~> 4.36.0 and azuread ~> 3.6.0, but the root meshstack_integration.tf requires newer provider versions (azurerm ~> 4.64, azuread ~> 3.8). These constraints are incompatible and will cause terraform init to fail for the integration root. Align the backplane provider constraints with the versions required by the integration (or vice versa).
| version = "~> 4.36.0" | |
| } | |
| azuread = { | |
| source = "hashicorp/azuread" | |
| version = "~> 3.6.0" | |
| version = "~> 4.64.0" | |
| } | |
| azuread = { | |
| source = "hashicorp/azuread" | |
| version = "~> 3.8.0" |
| resource "azuread_application" "buildingblock_deploy" { | ||
| count = var.create_service_principal_name != null ? 1 : 0 | ||
|
|
||
| display_name = "${var.name}-${var.create_service_principal_name}" |
There was a problem hiding this comment.
azuread_application.buildingblock_deploy uses display_name = "${var.name}-${var.create_service_principal_name}". In the provided integration, both values are set to the same backplane_name, which results in a duplicated name like azure-service-principal-azure-service-principal. Consider using only var.create_service_principal_name (or documenting why both are concatenated) to avoid confusing resource names.
| display_name = "${var.name}-${var.create_service_principal_name}" | |
| display_name = var.create_service_principal_name |
| data "azurerm_subscription" "current" {} | ||
|
|
||
| # ----------------------------------------------------------------------------- | ||
| # Service Principal for Building Block Deployment | ||
| # ----------------------------------------------------------------------------- |
There was a problem hiding this comment.
The backplane module directory is missing a README.md. Other modules include a backplane README for operator-facing setup/docs, and the repo module structure guidance expects it. Please add a backplane README.md (even a minimal one) describing purpose, required inputs, and outputs.
| variable "azure_role" { | ||
| description = "Azure RBAC role to assign to the service principal on the subscription" | ||
| description = "Azure RBAC built-in role name to assign to the service principal (e.g., 'Contributor', 'Reader', 'Storage Blob Data Reader'). Ignored if custom_role is specified." | ||
| type = string | ||
| default = "Contributor" | ||
| default = null | ||
| } | ||
|
|
||
| variable "custom_role" { | ||
| description = "Define a custom role instead of using a built-in role. If specified, azure_role is ignored." | ||
| type = object({ | ||
| name = string | ||
| description = optional(string, "Custom role managed by Terraform") | ||
| actions = optional(list(string), []) | ||
| not_actions = optional(list(string), []) | ||
| data_actions = optional(list(string), []) | ||
| not_data_actions = optional(list(string), []) | ||
| }) | ||
| default = null | ||
|
|
||
| validation { | ||
| condition = contains(["Owner", "Contributor", "Reader"], var.azure_role) | ||
| error_message = "azure_role must be one of: Owner, Contributor, Reader" | ||
| condition = var.custom_role == null || length(coalesce(var.custom_role.actions, [])) > 0 || length(coalesce(var.custom_role.data_actions, [])) > 0 | ||
| error_message = "custom_role must have at least one action or data_action defined" | ||
| } | ||
| } |
There was a problem hiding this comment.
A new custom_role input was added, but there’s no corresponding test coverage for (1) a positive custom role scenario (role definition + assignment) and (2) the negative validation case (no actions/data_actions). Also, the existing test that expects azure_role validation failure will need to be replaced/adjusted now that the built-in role list validation was removed.
| source = "hashicorp/azurerm" | ||
| version = "~> 4.36.0" | ||
| } | ||
| azuread = { | ||
| source = "hashicorp/azuread" | ||
| version = "~> 3.6.0" |
There was a problem hiding this comment.
provider_tf embeds a required_providers block pinned to specific azurerm/azuread versions. If you update provider constraints in backplane/versions.tf, keep these pinned versions in sync as well, otherwise users copying this snippet may end up with provider version mismatches.
| source = "hashicorp/azurerm" | |
| version = "~> 4.36.0" | |
| } | |
| azuread = { | |
| source = "hashicorp/azuread" | |
| version = "~> 3.6.0" | |
| source = "hashicorp/azurerm" | |
| } | |
| azuread = { | |
| source = "hashicorp/azuread" |
| role_definition_name = var.azure_role | ||
| role_definition_id = var.custom_role != null ? azurerm_role_definition.custom[0].role_definition_resource_id : null | ||
| role_definition_name = var.custom_role == null ? var.azure_role : null | ||
| principal_id = azuread_service_principal.main.object_id |
There was a problem hiding this comment.
azurerm_role_assignment.main now uses count, which changes it from a singleton resource to a list. Any references (including Terraform tests in service-principal.tftest.hcl) must be updated to index [0] where applicable, and the previous azure_role validation-based negative test case will no longer fail as written.
| principal_id = azuread_service_principal.main.object_id | |
| principal_id = azuread_service_principal.main.object_id | |
| lifecycle { | |
| precondition { | |
| condition = var.custom_role != null || ( | |
| var.azure_role != null && | |
| trimspace(var.azure_role) != "" | |
| ) | |
| error_message = "Either custom_role must be provided or azure_role must be a non-empty built-in Azure role name." | |
| } | |
| precondition { | |
| condition = !( | |
| var.custom_role != null && | |
| var.azure_role != null && | |
| trimspace(var.azure_role) != "" | |
| ) | |
| error_message = "Provide either custom_role or azure_role, but not both." | |
| } | |
| } |
| data "meshstack_integrations" "integrations" {} | ||
|
|
||
| module "backplane" { | ||
| source = "github.com/meshcloud/meshstack-hub//modules/azure/service-principal/backplane?ref=main" |
There was a problem hiding this comment.
The backplane module source is pinned to ref=main, which makes this integration non-reproducible and prevents consumers from pinning to a release/commit. Prefer referencing var.hub.git_ref (or a commit SHA) for the module source ref, consistent with other integrations (e.g. modules/azure/storage-account/meshstack_integration.tf:65).
| source = "github.com/meshcloud/meshstack-hub//modules/azure/service-principal/backplane?ref=main" | |
| source = "github.com/meshcloud/meshstack-hub//modules/azure/service-principal/backplane?ref=${var.hub.git_ref}" |
| ## Azure Service Principal | ||
|
|
||
| This building block creates an **Azure AD Application** and **Service Principal** with role assignments on your Azure subscription. | ||
|
|
||
| ## When to use it? | ||
|
|
||
| Use this building block when your applications need: | ||
| - A service identity for Azure authentication | ||
| - Programmatic access to Azure resources | ||
| - CI/CD pipeline authentication with Azure | ||
| - Workload identity for containerized applications | ||
|
|
||
| ## Features | ||
|
|
||
| - Creates Azure AD Application and Service Principal | ||
| - Supports built-in roles (Contributor, Reader, Owner, etc.) | ||
| - Supports custom role definitions with granular permissions | ||
| - Automatic secret rotation with configurable expiration | ||
| - Optional workload identity federation support | ||
|
|
||
| ## Shared Responsibilities | ||
|
|
||
| | Responsibility | Platform Team | Application Team | | ||
| | ------------------------------------------- | :-----------: | :--------------: | | ||
| | Provision service principal | ✅ | ❌ | | ||
| | Define available roles | ✅ | ❌ | | ||
| | Choose role assignment | ❌ | ✅ | | ||
| | Manage client secrets securely | ❌ | ✅ | | ||
| | Configure workload identity federation | ❌ | ✅ | |
There was a problem hiding this comment.
The BBD readme content doesn’t match the repo’s documented requirements: it should start with a short plain-text description (no heading) and include 1–2 concrete usage examples. Also, the text claims “Supports custom role definitions”, but this building block definition doesn’t expose any input that allows users to provide custom_role, so users can’t actually use that feature via meshStack.
| ## Azure Service Principal | |
| This building block creates an **Azure AD Application** and **Service Principal** with role assignments on your Azure subscription. | |
| ## When to use it? | |
| Use this building block when your applications need: | |
| - A service identity for Azure authentication | |
| - Programmatic access to Azure resources | |
| - CI/CD pipeline authentication with Azure | |
| - Workload identity for containerized applications | |
| ## Features | |
| - Creates Azure AD Application and Service Principal | |
| - Supports built-in roles (Contributor, Reader, Owner, etc.) | |
| - Supports custom role definitions with granular permissions | |
| - Automatic secret rotation with configurable expiration | |
| - Optional workload identity federation support | |
| ## Shared Responsibilities | |
| | Responsibility | Platform Team | Application Team | | |
| | ------------------------------------------- | :-----------: | :--------------: | | |
| | Provision service principal | ✅ | ❌ | | |
| | Define available roles | ✅ | ❌ | | |
| | Choose role assignment | ❌ | ✅ | | |
| | Manage client secrets securely | ❌ | ✅ | | |
| | Configure workload identity federation | ❌ | ✅ | | |
| Creates an Azure AD application and service principal with role assignments on your Azure subscription. | |
| Use this building block when your application, automation, or CI/CD pipeline needs its own identity to authenticate against Azure resources. It is a good fit for teams that need a dedicated service principal instead of using personal identities. | |
| ### Usage examples | |
| - A developer provisions a service principal for a GitHub Actions workflow that deploys infrastructure to Azure and assigns the **Contributor** role on the target subscription. | |
| - A developer creates a service principal for an application that needs read-only access to Azure resources and assigns the **Reader** role. | |
| ### Features | |
| - Creates an Azure AD application and service principal | |
| - Supports built-in Azure roles such as Contributor, Reader, and Owner | |
| - Supports automatic secret rotation with configurable expiration | |
| - Supports optional workload identity federation | |
| ### Shared responsibilities | |
| | Responsibility | Platform Team | Application Team | | |
| | -------------------------------------- | :-----------: | :--------------: | | |
| | Provision service principal | ✅ | ❌ | | |
| | Define available roles | ✅ | ❌ | | |
| | Choose role assignment | ❌ | ✅ | | |
| | Store and use client secrets securely | ❌ | ✅ | | |
| | Configure workload identity federation | ❌ | ✅ | |
No description provided.