feat: Enable support for v5.0 of the AWS provider

[ohmer]

Description

Loosen version constraint for AWS provider. Currently major version is pinned to 4. This change allows using newer major version, like the just released version 5.

Motivation and Context

I want to upgrade my workspaces using this module to AWS provider version 5.
Solves issue #384.

Breaking Changes

No breaking change found.

How Has This Been Tested?

• I have updated at least one of the examples/* to demonstrate and validate my change(s)
• I have tested and validated these changes using one or more of the provided examples/* projects
• I have executed pre-commit run -a on my pull request
• I have tested and validated these changes with my own workspaces using a fork (https://github.com/ohmer/terraform-aws-rds-aurora)

[antonbabenko]

Looks good. Could you also please update the rest of versions.tf in examples?

[b-dean]

I know it's a pain to have to change the version every time a new major version comes out, but I really think the constraint should be:

terraform {
  required_version = ">= 1.0"
  required_providers {
    aws = {
      source  = "hashicorp/aws"
      version = ">= 4.67, < 6"
    }
  }
}

While you know this will work fine with version 5 of the provider, you have no idea what the breaking changes will be in version 6. If this is too relaxed then users of this module could have things break if there's something RDS related in the next major version.

[bryantbiggs]

this is essentially what the current version is protecting from (~> 4.67) - I am skeptical that opening up v5.0 on here, I suspect we'll get more issues if we do without further changes

[b-dean]

Yeah, I'm saying if v5 is safe (which by all means, someone should look over the v5 CHANGELOG.md and test it), that doesn't mean v6, v7, etc will all be safe.

Another option is bump this module up a major version and require ~> 5.0

[ohmer]

Looks good. Could you also please update the rest of versions.tf in examples?

Done in 3171ca3 @antonbabenko, thanks for pointing that out!

[ohmer]

I know it's a pain to have to change the version every time a new major version comes out, but I really think the constraint should be:

terraform {
  required_version = ">= 1.0"
  required_providers {
    aws = {
      source  = "hashicorp/aws"
      version = ">= 4.67, < 6"
    }
  }
}

While you know this will work fine with version 5 of the provider, you have no idea what the breaking changes will be in version 6. If this is too relaxed then users of this module could have things break if there's something RDS related in the next major version.

We will have to disagree here. I think this is not up to the modules (the callee) to make this decision.
I think the best practice is:

• pin major version of providers in root module (a.k.a. workspace, the caller)
• express a minimum version in the modules (>=)

This practice, whether you consider it optimal or not, is what widely used modules adopt as far as I know. This is also the recommendation from https://developer.hashicorp.com/terraform/language/expressions/version-constraints#terraform-core-and-provider-versions as I understand it.

Here is my proposal, up to the maintainers to make a call.

[danlsgiga]

Thanks for pushing a PR for this @ohmer! I'm having the same issue and I agree that its not up to the module to define providers constraints in a strict way but defining a minimum supported version. Your suggestion is to the point, I think the maintainers should consider "supporting" the last 2 major provider releases with the minimum version being the "vetted" one. So, version = ">= 4.67, < 6" makes a lot of sense to me!

[mcantinqc]

I agree with @ohmer. Using the syntax ">= 4.67, < 6" is very very conservative, because it say we know the module is break on the version 6. The standard way used by others AWS modules (like https://github.com/terraform-aws-modules/terraform-aws-s3-bucket/blob/master/versions.tf, https://github.com/terraform-aws-modules/terraform-aws-iam/blob/master/modules/iam-assumable-role-with-oidc/versions.tf, https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/versions.tf) is to use the minimal compatible version.

[bryantbiggs]

Again, this is the way this module is versioned today. We need to validate v5 before opening up to add that. Looking at the changelog, I do not believe this is a no-op of simply opening up the version

[ohmer]

I do not believe this is a no-op of simply opening up the version

This was for me. Here is how I invoke the forked module:

terraform {
  required_version = "~> 1.4.0"

  required_providers {
    aws = {
      source  = "hashicorp/aws"
      version = "~> 5.0"
    }
  }
}

module "database" {
  # Using a fork temporarily because of:
  # https://github.com/terraform-aws-modules/terraform-aws-rds-aurora/pull/385
  source  = "github.com/ohmer/terraform-aws-rds-aurora"
#  source  = "terraform-aws-modules/rds-aurora/aws"
#  version = "~> 8.0"

  for_each = {
    simply = {}
    hive   = {}
  }

  name                                = "${var.environment}-${each.key}"
  engine                              = "aurora-mysql"
  engine_mode                         = "provisioned"
  engine_version                      = data.aws_rds_engine_version.this.version
  master_username                     = "root"
  manage_master_user_password         = true
  allow_major_version_upgrade         = false
  auto_minor_version_upgrade          = false
  backup_retention_period             = var.database_backup_retention_period[each.key]
  create_cloudwatch_log_group         = true
  create_db_cluster_parameter_group   = true
  create_monitoring_role              = false
  create_db_parameter_group           = true
  db_cluster_parameter_group_family   = "aurora-mysql5.7"
  db_parameter_group_family           = "aurora-mysql5.7"
  create_db_subnet_group              = false
  db_subnet_group_name                = module.vpc.database_subnet_group_name
  deletion_protection                 = false
  enabled_cloudwatch_logs_exports     = ["audit", "error", "general", "slowquery"]
  subnets                             = module.vpc.database_subnets
  vpc_id                              = module.vpc.vpc_id
  vpc_security_group_ids              = [module.sg_app_rds.security_group_id]
  create_security_group               = false
  port                                = var.database_port[each.key]
  iam_database_authentication_enabled = false
  publicly_accessible                 = false
  snapshot_identifier                 = var.database_snapshot_identifier[each.key]
  skip_final_snapshot                 = var.database_skip_final_snapshot[each.key]
  storage_encrypted                   = true
  instance_class                      = var.database_instance_class[each.key]

  instances = {
    for i in range(1, var.database_instance_count[each.key] + 1) : i => {
      availability_zone = module.vpc.azs[i - 1]
      promotion_tier    = i
    }
  }
}

[b-dean]

@mcantinqc, restricting to >= 4.67, < 6 is not saying we know it's going to break in 6. It's saying we know it works in 4.x (at least 4.67) and all of 5.x. But we don't know if it works in 6 because we have no idea what's going to break in 6.

And I don't think it's too conservative. There's a lot of people that have very lax version constraints getting bitten by upgrading to 5.x unexpectedly and hitting breaking changes because they had >= 4.0 and now that includes 5.0 and all its 72 breaking changes.

Terraform's docs on best practices for provider versions say that modules should "at least declare the minimum provider version it is known to work with". But anyone that's worked with ruby gems much can tell you the dangers of only giving the minimum versions.

Putting a max known version does put the burden on the module developers to keep that updated. But I think that's fine. Because otherwise how do they even know if it works on those versions? Just because bugs start coming in when it blows up people's infrastructure? That can't be good right?

[mcantinqc]

@b-dean The documentation you follow for Ruby is a recommendation for code that uses the gem. In this context, that makes perfect sense.

In parallel with Terraform, i.e., the code that calls the Terraform module must set the version of the module, and that is strongly recommended. Personally, I recommend freezing the exact versions of all modules and providers and using a tool like renovate and a good CI to automate the update process.

If you check gems like https://github.com/rails/rails/blob/v7.0.5/Gemfile, https://github.com/rspec/rspec-rails/blob/main/Gemfile or https://github.com/sidekiq/sidekiq/blob/main/Gemfile, they always only set the minimum required version.

Normally, in packages or modules, the best way is to set the minimum version (and maximum only if you know the package/module is breaking), and on the caller, you set a pessimistic constraint.

[nikolay]

@bryantbiggs Can we have this merged, please? Thanks!

[estahn]

@bryantbiggs Do you think the test @ohmer has done is sufficient or are there any further requirements to validate that v5 is working?

[bryantbiggs]

validated by:

1. Deploying current examples on master
2. Ensured clean plans following apply with terraform apply -refresh-only
3. Switch to PR changes
4. terraform init -upgrade=true
5. Check that plans are clean - in order to get clean plans I did have to update the AWS VPC module version in the examples but this is a no-op for the module implementation

[antonbabenko]

This PR is included in version 8.3.0 🎉

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.