pulumi-aws icon indicating copy to clipboard operation
pulumi-aws copied to clipboard

Tracking: remove tags related patches

Open corymhall opened this issue 1 year ago • 4 comments

This is a tracking ticket to track what needs to be done in order to remove all of our patches / bridge hooks related to tagging and revert to upstream's tagging behavior.

An initial draft PR was done here https://github.com/pulumi/pulumi-aws/pull/4219.

Background

The original work to fix tagging on the Pulumi side was done in #2655. There was a lot of limitations in using default_tags in Terraform which we decided to circumvent be merging default_tags into the resource's tags via a PreCheckCallback function. Since then most of the upstream issues (https://github.com/hashicorp/terraform-provider-aws/issues/29747, https://github.com/hashicorp/terraform-provider-aws/issues/29842, https://github.com/hashicorp/terraform-provider-aws/issues/24449) have been fixed.

The additional patches / custom code adds to the maintenance burden and makes upgrades much harder due to merge conflicts. Since many of the original upstream issues have been fixed, it may be possible to remove our custom tag handling and revert back to upstream behavior.

Patches to remove

  • patches/0029-Fix-markTagsAllNotComputedForResources-to-recognize-.patch
  • patches/0027-Do-not-compute-tags_all-at-TF-level.patch
  • patches/0001-Add-TagsSchemaTrulyComputed-definition.patch
  • patches/0030-Optimize-startup-performance.patch
  • patches/0032-DisableTagSchemaCheck-for-PF-provider.patch
  • patches/0033-Run-scripts-patch_computed_only.sh-to-patch-eks-pod_.patch
  • patches/0035-Fix-tags_all-Computed-for-PF-resources.patch
  • patches/0039-Patch-osis_pipeline-tags-flags.patch
  • patches/0041-Do-not-Compute-tags_all-of-aws_bedrock_provisioned_m.patch
  • patches/0043-securitylake_subscriber-tags_all-patch.patch
  • patches/0044-Patch-tags-ComputedOnly-for-m2-resources.patch
  • patches/0053-Patch-tags-ComputedOnly-on-bedrockagent-and-other-mo.patch
  • patches/0055-Fix-tags_all-Computed-for-aws_datazone_domain.patch
  • patches/0058-Fix-tags_all-Computed-for-PF-resources.patch
  • patches/0059-Fix-tags_all-Computed-for-PF-resources.patch
  • patches/0060-Fix-tags_all-Computed-for-PF-resources.patch
  • patches/0062-Patch-tags-ComputedOnly-for-appfabric-networkmonitor.patch

Patches to update (switch tags_all back to Computed)

  • patches/0042-fix-legacy-bucket-context.patch
  • patches/0014-add-matchmaking-configuration-72.patch
### Tasks
- [ ] Rollout `PlanResourceChange` for all resources
- [ ] Fix aws_s3_legacy_bucket to work with `PlanResourceChange`
- [ ] [TestTagsCombinationsGo/regress_2](https://github.com/pulumi/pulumi-aws/blob/335cac45a6d895a29b35b0a0c5755736b557d93d/examples/examples_go_test.go#L92-L96) fails. The Tag value is not removed.
- [ ] [TestAccDefaultTags (Don't specify any default tags)](https://github.com/pulumi/pulumi-aws/blob/335cac45a6d895a29b35b0a0c5755736b557d93d/examples/examples_yaml_test.go#L197-L201) fails. The Tag value is not removed.

corymhall avatar Jul 15 '24 19:07 corymhall

The was an attempt to consolidate these patches which reduces the burden a bit https://github.com/pulumi/pulumi-aws/pull/4151 but we decided to first ensure upstream tests run on the result of patching.

I think also extending testing to cover refresh and import scenarios as sketched out in https://github.com/pulumi/pulumi-aws/pull/4169 could be highly advantageous here to avoid surprises with that part of the life-cycle.

t0yv0 avatar Jul 15 '24 21:07 t0yv0

@iwahbe had a comment that upstream tag behavior relies critically on running refresh as part of apply. Since Pulumi bridged providers target imitating terraform -refresh=false behavior this may imply that removing tagging custom code would regress some functionality. Is it possible to file or reference an example TF program that demonstrates tagging bugs under -refresh=false?

t0yv0 avatar Jul 17 '24 15:07 t0yv0

I just checked, and it seems like upstream's behavior has improved radically here. Retrying on the latest version of hashicorp/aws, this now works (with terraform apply -refresh=false):

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

# Configure the AWS Provider
provider "aws" {
  region = "us-east-1"
  default_tags {
    tags = {
      Test = "example"
    }
  }
}

resource "aws_s3_bucket" "example" {}

Updating "example" to "example2" now works as expected without refresh. Neither used to work without refresh. Adding another default tag works as expected as well (even without refresh):

 provider "aws" {
   region = "us-east-1"
   default_tags {
     tags = {
       Test = "example"
+      Test2 = "example2"
     }
   }
 }

Removing all default tags does not work at all (it shows no diff) without refresh:

 provider "aws" {
   region = "us-east-1"
   default_tags {
     tags = {
-       Test = "example"
     }
   }
 }

I didn't get to the point where I manually compared how the tags property on resources interacts with the default_tags property on the provider.

iwahbe avatar Jul 18 '24 19:07 iwahbe

There is still a few failing tests I am slightly worried about around the use of empty values for tags "", possibly aggravated by the underlying TF machinery confusing empty and unknown values. Some of our customers find these behaviors important and we need to tread carefully to not regress. Might be worth revisiting at some point, as the patches are indeed quite uncomfortable to maintain.

t0yv0 avatar Sep 27 '24 20:09 t0yv0

merged into 7.0.0-alpha

corymhall avatar May 16 '25 13:05 corymhall