refactor: split cloud providers into separate modules by philip-gai · Pull Request #59 · lucavallin/tf-ghes-oidc

philip-gai · 2025-04-04T18:30:07Z

Summary

This PR refactors the Terraform configuration by splitting each cloud provider (AWS, Azure, GCP) into its own module under src/. This improves maintainability, allows for independent versioning, and makes it easier to use only the cloud providers you need.

Changes

Structure

Created src/aws/ module for AWS OIDC configuration
Created src/azure/ module for Azure OIDC configuration
Created src/gcp/ module for GCP OIDC configuration
Each module is self-contained with its own main.tf, variables.tf, outputs.tf, provider.tf, and versions.tf

Code Quality

AWS: Created aws_client_id_list local to avoid duplicating client IDs
Azure: Removed unnecessary local variables, inlined values used only once
GCP: Fixed variable description and used lowercase region default

Dependabot

Updated .github/dependabot.yml to track provider updates in each module directory

Testing

All modules pass terraform fmt -check
All modules pass terraform validate

lucavallin

Thank you for the contribution! This is a much needed upgrade. I left a few comments. Also, modules normally follow a certain structure (e.g. modules are inside a "modules" directory outside of the main "src" directory). Have a look at https://www.env0.com/blog/terraform-modules

lucavallin · 2025-04-07T09:03:36Z

 resource "aws_iam_openid_connect_provider" "this" {
  url             = local.oidc_issuer_uri
-  client_id_list  = [local.aws_sts_endpoint]
+  client_id_list  = ["sts.amazonaws.com", local.aws_sts_endpoint]


Should sts.amazonaws.com always be in the client list?

Yes, sts.amazonaws.com is now always included. I've created a local variable aws_client_id_list that includes both sts.amazonaws.com and the regional STS endpoint, and this is used in both the OIDC provider and the IAM role policy.

lucavallin · 2025-04-07T09:03:49Z

-            "${aws_iam_openid_connect_provider.this.url}:aud" = local.aws_sts_endpoint
+            "${aws_iam_openid_connect_provider.this.url}:sub" : "${local.ghes_hostname}",
+            "${aws_iam_openid_connect_provider.this.url}:aud" = [
+              "sts.amazonaws.com",


We should create a local var for this

Done! Created aws_client_id_list local variable that's now used in both the client_id_list attribute and the IAM role policy aud condition.

lucavallin · 2025-04-07T09:04:31Z

@@ -1,14 +1,6 @@
-provider "azurerm" {


Do we need to update the dependabot config to keep it updating the providers?

Yes! Updated .github/dependabot.yml to include entries for /src/aws, /src/azure, and /src/gcp directories so providers in each module will be kept up to date.

lucavallin · 2025-04-07T09:05:35Z

+  azure_blob_endpoint_suffix             = "core.windows.net"
+}
+
+resource "random_string" "long" {


Since we use this across modules, we could have modules take in a random prefix as a variable, and generate the random prefix in our main.tf

Given that this is a long random string, presumably by design, it might be worth considering adding validation for its length if you’re accepting it as a variable. Also, I’d recommend reconsidering whether it’s necessary to expose it as a variable—doing so could add unnecessary noise to the module interface. Is there other than the above mentioned reason another reason to make it a variable?

Good point from both of you. I've opted to keep the random_string resource within each module for now to keep them self-contained and avoid adding complexity to the module interface. This way each module is independently usable without requiring external dependencies. Happy to revisit if there's a strong preference otherwise.

bschaatsbergen

Hey 👋🏼, nice job on splitting this out. I left a few drive-by comments. They’re just meant to highlight some things for consideration.

bschaatsbergen · 2025-04-08T12:23:15Z

+locals {
+  ghes_name       = var.GHES_NAME
+  ghes_hostname   = var.GHES_HOSTNAME
+  oidc_issuer_uri = "https://${local.ghes_hostname}/_services/token"


Is this local referenced elsewhere? I see it used just once here (in the context of this PR), it's advised to avoid reserving a local variable for it in this case.

Good catch! Removed oidc_issuer_uri local and inlined the value directly where it's used since it was only referenced once.

bschaatsbergen · 2025-04-08T12:24:58Z

+  azure_region                           = var.AZURE_REGION
+  azure_storage_account_tier             = var.AZURE_STORAGE_ACCOUNT_TIER
+  azure_storage_account_replication_type = var.AZURE_STORAGE_ACCOUNT_REPLICATION_TYPE
+  azure_blob_endpoint_suffix             = "core.windows.net"


Similarly, is this local referenced anywhere other than the existing output block? If not, it might be better to place the string literal ("core.windows.net") directly within the output block.

Done! Removed azure_blob_endpoint_suffix local and now using the literal "core.windows.net" directly in the output block.

bschaatsbergen · 2025-04-08T12:27:01Z

+  azure_blob_endpoint_suffix             = "core.windows.net"
+}
+
+resource "random_string" "long" {


Given that this is a long random string, presumably by design, it might be worth considering adding validation for its length if you’re accepting it as a variable. Also, I’d recommend reconsidering whether it’s necessary to expose it as a variable—doing so could add unnecessary noise to the module interface. Is there other than the above mentioned reason another reason to make it a variable?

bschaatsbergen · 2025-04-08T12:28:56Z

@@ -0,0 +1,20 @@
+variable "GHES_NAME" {


I noticed the variable names are all caps. Is there a particular reason for this choice? In Terraform, the standard convention is to use camelCase or snake_case for variable names, and aligning with this could improve consistency with other publicly available modules. This might also help reduce cognitive load for users who are familiar with the conventions.

Point of thought: since this was likely already released and in use, I would recommend avoiding any breaking changes—unless you haven’t yet reached any end users.

Agreed on avoiding breaking changes. The all-caps variable names are from the original codebase, so I'm keeping them as-is to maintain backward compatibility for existing users.

bschaatsbergen · 2025-04-08T12:30:01Z

+
+variable "GCP_PROJECT_ID" {
+  type        = string
+  description = "Google Cloud: ID of the Project to use"


Suggested change

description = "Google Cloud: ID of the Project to use"

description = "ID of the Google Cloud project to be used."

Applied! Updated description to "ID of the Google Cloud project to be used."

bschaatsbergen · 2025-04-08T12:31:39Z

+variable "GCP_REGION" {
+  type        = string
+  description = "Google Cloud: Region for OIDC Resources"
+  default     = "EUROPE-WEST4"


Does the provider accept the region in all caps? It’s possible that the provider or the underlying Google Cloud SDKs are normalizing the capitalization, but I would recommend sticking to lowercase for consistency. Was there a specific reason for using all caps?

Good point! Changed the default from EUROPE-WEST4 to europe-west4 for consistency with standard conventions.

- AWS: Create local var aws_client_id_list to avoid duplication - Azure: Remove unnecessary local vars (oidc_issuer_uri, azure_blob_endpoint_suffix) - GCP: Fix variable description and use lowercase region default - Update dependabot config to include module directories

- Resolve merge conflicts - Update AWS provider to 6.19.0 - Update Azure provider to 4.51.0, AzureAD to 3.6.0 - Update GCP provider to 7.9.0 - Update random provider to 3.7.2 across all modules

philip-gai

Addressed all feedback - see individual replies on each thread.

lucavallin · 2026-01-25T20:27:42Z

Fantastic, great work! I assume everything is working fine? I'd double check myself normally, but it's hectic times here... ;)

lucavallin · 2026-03-24T11:38:43Z

@philip-gai Just a ping to know I can merge this ;)

lucavallin · 2026-05-06T13:48:09Z

@philip-gai Ping!

philip-gai · 2026-05-06T17:08:28Z

Sorry @lucavallin haven't had the chance to get this all setup and re-test it 😄

Split out modules for each cloud provider

c78ef5c

philip-gai temporarily deployed to production April 4, 2025 18:30 — with GitHub Actions Inactive

philip-gai commented Apr 4, 2025

View reviewed changes

Comment thread .devcontainer/devcontainer.json

philip-gai added 3 commits April 4, 2025 19:48

Make thumbprint lowercase

519ae9a

Add default sts.amazonaws.com" to trust relationship

70ad3de

Add back examples

04f9649

philip-gai temporarily deployed to production April 4, 2025 19:49 — with GitHub Actions Inactive

Add default sts.amazonaws.com to provider

ceca1d0

philip-gai temporarily deployed to production April 4, 2025 19:51 — with GitHub Actions Inactive

Remove extra line

e1ea041

philip-gai temporarily deployed to production April 4, 2025 20:26 — with GitHub Actions Inactive

Set default AWS_STS_ENDPOINT

64b1504

philip-gai temporarily deployed to production April 4, 2025 20:29 — with GitHub Actions Inactive

lucavallin reviewed Apr 7, 2025

View reviewed changes

bschaatsbergen reviewed Apr 8, 2025

View reviewed changes

philip-gai changed the title ~~Split out modules for each cloud provider~~ Split cloud providers into separate modules Jan 20, 2026

Merge main and update provider versions

9ba0e5b

- Resolve merge conflicts - Update AWS provider to 6.19.0 - Update Azure provider to 4.51.0, AzureAD to 3.6.0 - Update GCP provider to 7.9.0 - Update random provider to 3.7.2 across all modules

philip-gai temporarily deployed to production January 20, 2026 19:23 — with GitHub Actions Inactive

philip-gai changed the title ~~Split cloud providers into separate modules~~ refactor: split cloud providers into separate modules Jan 20, 2026

philip-gai commented Jan 20, 2026

View reviewed changes

	description = "Google Cloud: ID of the Project to use"
	description = "ID of the Google Cloud project to be used."

Conversation

philip-gai commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Structure

Code Quality

Dependabot

Testing

Uh oh!

Uh oh!

lucavallin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bschaatsbergen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philip-gai left a comment

Choose a reason for hiding this comment

Uh oh!

lucavallin commented Jan 25, 2026

Uh oh!

lucavallin commented Mar 24, 2026

Uh oh!

lucavallin commented May 6, 2026

Uh oh!

philip-gai commented May 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

philip-gai commented Apr 4, 2025 •

edited

Loading