Skip to content

DT-436 adding tags to aws resources #25

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

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

Conversation

christopher-comet
Copy link
Contributor

@christopher-comet christopher-comet commented Feb 10, 2025

https://comet-ml.atlassian.net/browse/DT-436

This also includes edits to make the tfvars ready to go with defaults and updates the redis version.

darenjacobs
darenjacobs previously approved these changes Feb 10, 2025
Copy link
Contributor

@darenjacobs darenjacobs left a comment

Choose a reason for hiding this comment

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

LGTM

@darenjacobs darenjacobs self-requested a review February 19, 2025 12:37
@christopher-comet
Copy link
Contributor Author

Ready for review. @darenjacobs @liyaka

liyaka
liyaka previously requested changes Feb 22, 2025
@christopher-comet
Copy link
Contributor Author

There are a couple of modules wherein the resource explicitly sets tags.
eg. in terraform-aws-comet/modules/comet_s3/main.tf

resource "aws_s3_bucket" "comet_airflow_bucket" {
  count = var.enable_mpm_infra ? 1 : 0

  bucket = "comet-airflow-${local.suffix}"

  force_destroy = var.s3_force_destroy

  tags = {
    Name = "comet-airflow-${local.suffix}"
  }
}

In this situation the S3 bucket will only get the Name tag, and it will NOT inherit common_tags automatically because the tags block inside aws_s3_bucket is explicitly defined, and default_tags from providers.tf does not override manually set tags.

Therefore for these cases we need to merge the common_tags.

@christopher-comet christopher-comet marked this pull request as draft March 6, 2025 16:13
@christopher-comet
Copy link
Contributor Author

christopher-comet commented May 8, 2025

Summary of Terraform Changes in This PR
Provider Updates

Bumped versions for the following providers:

    aws → 5.97.0

    helm → 2.17.0

    kubernetes → 2.36.0

    Others: tls, time, cloudinit, and added null

Why: Modernization, bug fixes, and compatibility with new module versions.

EKS Module & Provider Refactor

Migrated to terraform-aws-modules/eks v20.33.1

Set authentication_mode = "API_AND_CONFIG_MAP" for streamlined auth

Replaced deprecated aws_eks_cluster_auth with output-based kubeconfig logic

Moved helm.eks and kubernetes.eks provider definitions into eks_providers.tf

Added explicit providers = { ... } block for comet_eks

Enables one-stage apply without Helm/Kubernetes provider race conditions.
Unified Tagging Across Modules

Replaced hardcoded tags = { Terraform = "true", Environment = var.environment }

Now using common_tags passed into all modules

Centralized in local.all_tags and merged with per-resource tags

Improves tag consistency across all infrastructure components.
Module Improvements

Updated modules for:

    comet_eks (with conditional merge for node groups)

    comet_ec2, comet_vpc, comet_elasticache, comet_rds, comet_s3

Added missing common_tags variables and passed them to resources

Output Cleanup

Removed obsolete comet_eks_token output (no longer available via token-based auth)

Updated comet_eks_cert and comet_eks_endpoint to match new module structure

IRSA + Addons

Added IRSA role creation for EBS CSI driver

Integrated aws-ia/eks-blueprints-addons module to install:

    CoreDNS, kube-proxy, vpc-cni, aws-ebs-csi-driver

    AWS Load Balancer Controller (with Helm release)

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