AWS Auth in Terraform #1771
AWS Auth in Terraform #1771
Conversation
[review]
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -1,11 +1,12 @@ | |||
| provider "kubernetes" { | |||
| config_path = "~/.kube/config" | |||
| config_context = "dev" | |||
| config_context = var.env | |||
There was a problem hiding this comment.
The change from a hardcoded context to using var.env is good for flexibility, but ensure that var.env is properly defined and validated to avoid potential issues.
| } | ||
|
|
||
| data "aws_caller_identity" "current" {} | ||
|
|
||
| module "eks" { | ||
| count = var.env != "production" ? 1 : 0 |
There was a problem hiding this comment.
The count parameter is being set based on the environment. Ensure that var.env is correctly set to either 'production' or another value to avoid unexpected behavior.
| @@ -0,0 +1,9 @@ | |||
| terraform { | |||
| source = "${get_env("ENVIRONMENT") == "production" ? "git::https://github.com/cds-snc/notification-terraform//aws/aws-auth?ref=v${get_env("INFRASTRUCTURE_VERSION")}" : "../../../aws//aws-auth"}" | |||
There was a problem hiding this comment.
Consider using a variable for the repository URL to avoid hardcoding it multiple times in the codebase. This will make it easier to update the URL in the future if needed.
env/dev/aws-auth/terragrunt.hcl
Outdated
| path = find_in_parent_folders() | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
There is an extra blank line at the end of the file. It is generally good practice to keep the file clean by removing unnecessary blank lines.
| @@ -0,0 +1,9 @@ | |||
| terraform { | |||
| source = "${get_env("ENVIRONMENT") == "production" ? "git::https://github.com/cds-snc/notification-terraform//aws/aws-auth?ref=v${get_env("INFRASTRUCTURE_VERSION")}" : "../../../aws//aws-auth"}" | |||
There was a problem hiding this comment.
Consider using a variable for the environment check instead of directly using get_env("ENVIRONMENT") for better readability and maintainability.
| path = find_in_parent_folders() | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
There is an extra empty line at the end of the file. Consider removing it to maintain consistent formatting.
| @@ -0,0 +1,9 @@ | |||
| terraform { | |||
| source = "${get_env("ENVIRONMENT") == "production" ? "git::https://github.com/cds-snc/notification-terraform//aws/aws-auth?ref=v${get_env("INFRASTRUCTURE_VERSION")}" : "../../../aws//aws-auth"}" | |||
There was a problem hiding this comment.
Consider using a more readable format for the source attribute to improve maintainability. For example, you can use string interpolation for the URL and the version separately.
| @@ -1,11 +1,12 @@ | |||
| provider "kubernetes" { | |||
| config_path = "~/.kube/config" | |||
| config_context = "dev" | |||
| config_context = var.env | |||
There was a problem hiding this comment.
Consider renaming the variable var.env to something more descriptive, such as var.environment, to improve code readability.
| } | ||
|
|
||
| data "aws_caller_identity" "current" {} | ||
|
|
||
| module "eks" { | ||
| count = var.env != "production" ? 1 : 0 |
There was a problem hiding this comment.
The count parameter logic could be simplified by using a boolean variable for production environment check. For example, var.is_production ? 0 : 1.
| @@ -0,0 +1,7 @@ | |||
| terraform { | |||
| source = "${get_env("ENVIRONMENT") == "production" ? "git::https://github.com/cds-snc/notification-terraform//aws/aws-auth?ref=v${get_env("INFRASTRUCTURE_VERSION")}" : "../../../aws//aws-auth"}" | |||
There was a problem hiding this comment.
Consider breaking the source value into multiple lines for better readability. For example:
source = get_env("ENVIRONMENT") == "production" ? \
"git::https://github.com/cds-snc/notification-terraform//aws/aws-auth?ref=v${get_env("INFRASTRUCTURE_VERSION")}" : \
"../../../aws//aws-auth"
staging: aws-auth✅ Terraform Init: Plan: 1 to add, 0 to change, 0 to destroyShow summary
Show planResource actions are indicated with the following symbols:
+ create
Terraform will perform the following actions:
# module.eks[0].kubernetes_config_map_v1_data.aws_auth[0] will be created
+ resource "kubernetes_config_map_v1_data" "aws_auth" {
+ data = {
+ "mapAccounts" = (sensitive value)
+ "mapRoles" = (sensitive value)
+ "mapUsers" = jsonencode([])
}
+ field_manager = "Terraform"
+ force = true
+ id = (known after apply)
+ metadata {
+ name = "aws-auth"
+ namespace = "kube-system"
}
}
Plan: 1 to add, 0 to change, 0 to destroy.
─────────────────────────────────────────────────────────────────────────────
Saved the plan to: plan.tfplan
To perform exactly these actions, run the following command to apply:
terraform apply "plan.tfplan"
Show Conftest results20 tests, 20 passed, 0 warnings, 0 failures, 0 exceptions
|
Summary | Résumé
Moving AWS auth configuration to Terraform for dev and staging. Eventually will be pushed to prod, but let's test it first.
Removes it from kustomize/helmfile and eliminates the issue we have where migrating it from Kustomize to Helmfile breaks the authentication to K8s.
Related Issues | Cartes liées
Before merging this PR
Read code suggestions left by the
cds-ai-codereviewer bot. Address
valid suggestions and shortly write down reasons to not address others. To help
with the classification of the comments, please use these reactions on each of the
comments made by the AI review:
The classifications will be extracted and summarized into an analysis of how helpful
or not the AI code review really is.
Test instructions | Instructions pour tester la modification
TF Apply in staging
Verify we can still access the k8s cluster
Release Instructions | Instructions pour le déploiement
None.
Reviewer checklist | Liste de vérification du réviseur