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

feat: User Managed Identity federated within GitHub for CI/CD module #165

Merged
merged 20 commits into from
Oct 31, 2023

Conversation

Krusty93
Copy link
Contributor

@Krusty93 Krusty93 commented Oct 3, 2023

List of changes

Adds a module to make easier and reusable the creation of a user managed identity federated with GitHub.

Motivation and context

This change adds a new module that can be used by repositories that need to run GitHub Actions and logging in Azure using a password-less approach. Since potentially every repository might need to run Actions against in Azure, I think that creating a new module is worthing because it can help developers maximising code reusability.

The module allows the consumer to set either ci or cd as identity_role value: depending on which one is used, the module updates subscription IAM permissions with a set of Azure built-in roles.

The consumer can also limit the scope of the identity over a single repository environment or branch or even tags.

Module forces the consumer to use a consistent naming style.

Type of changes

  • Add new module
  • Update existing module
  • Remove existing module

Does this introduce a breaking change?

  • Yes
  • No

Other information

Run checks

Useful commands to run checks on local machine

bash .utils/terraform_run_all.sh init local
pre-commit run -a

identity_github_runner/tests/main.tf Outdated Show resolved Hide resolved
identity_github_runner/variables.tf Outdated Show resolved Hide resolved
identity_github_runner/variables.tf Outdated Show resolved Hide resolved
@pasqualedevita
Copy link
Member

What is the scope of this module? federation for all github actions or only for infrastructure repos?

pasqualedevita
pasqualedevita previously approved these changes Oct 4, 2023
identity_github_runner/variables.tf Outdated Show resolved Hide resolved
@@ -0,0 +1,4 @@
locals {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm aware this is a strong assumption, but the goal here is to maintain a naming consistency since the beginning and removing responsibilities on devs - especially in the next days when they will start a migration "losing time for devops things they don't care about" (hope you get the message).

If you agree on this change, I'd also define the resource group in this module instead of referencing it using data for two reasons:

  • if we are forcing the name, we already know the name and some work on dev side can be eliminated; of course, they would need to provide location as input variable
  • if rg definition was outside of the module, devs needed to "guess" how the module internally forces rg name (ok there's README.md but it is not enough in my opinion)

For devs, providing prefix and env_short is really easy and convenient.

Anyway, let's talk about this. It's just a proposal

cc @diegolagospagopa @pasqualedevita @gunzip

Copy link

@gunzip gunzip Oct 10, 2023

Choose a reason for hiding this comment

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

I agree with the principle of shifting the burden from devs minimizing the setup steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to make a change in the last commit.

Unfortunately, declaring a resource group in a module is a nightmare and considered a bad practice. However, I still want to enforce the rg name by looking for the right name using data resource in the module - if the rg is not found a failure is thrown. This is documented both in code example and readme file.

@Krusty93 Krusty93 marked this pull request as ready for review October 11, 2023 16:08
@Krusty93 Krusty93 requested a review from a team October 11, 2023 16:08
@Krusty93 Krusty93 requested a review from a team as a code owner October 11, 2023 16:08
@gunzip
Copy link

gunzip commented Oct 13, 2023

consider that infrastructure (hcl) files will go into the code repository when using a monorepo.
does this works with monorepos that contain multiple projects and needs different environments?

@lucacavallaro is this your concern?

@Krusty93
Copy link
Contributor Author

consider that infrastructure (hcl) files will go into the code repository when using a monorepo. does this works with monorepos that contain multiple projects and needs different environments?

@lucacavallaro is this your concern?

Today I got in touch with @lucacavallaro and with the last commit, it does.

I've added some documentation for naming and resource management in README.md file

@Krusty93 Krusty93 merged commit b9c9bfc into main Oct 31, 2023
3 checks passed
@Krusty93 Krusty93 deleted the gh-identity-module branch October 31, 2023 14:27
Copy link

🎉 This PR is included in version 7.19.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants