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

rds.ParameterGroup diff not clear #2442

Closed
dirien opened this issue Mar 30, 2023 · 10 comments · Fixed by #3638
Closed

rds.ParameterGroup diff not clear #2442

dirien opened this issue Mar 30, 2023 · 10 comments · Fixed by #3638
Assignees
Labels
area/providers bug/diff kind/bug related to Pulumi generating wrong diffs on preview or up. impact/usability Something that impacts users' ability to use the product easily and intuitively kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Milestone

Comments

@dirien
Copy link

dirien commented Mar 30, 2023

What happened?

Hi,

when doing a up on a stack with a rds.ParameterGroup, Pulumi will always show a diff. This is due to the order the array gets saved in the Pulumi service.

This makes the detection of an actuall very difficult, as everything is marked in red in the Pulumi output.

Demo code:

import * as pulumi from "@pulumi/pulumi";
import * as aws from "@pulumi/aws";
import * as awsx from "@pulumi/awsx";
import {rds} from "@pulumi/aws";

const vpc = new awsx.ec2.Vpc("vpc", {
    cidrBlock: "10.0.0.0/16",
    enableDnsHostnames: true,
    enableDnsSupport: true,
})

const group = new rds.SubnetGroup("group", {
    subnetIds: vpc.publicSubnetIds,
    name: "group",
})

const securityGroup = new aws.ec2.SecurityGroup("securityGroup", {
    name: "securityGroup",
    vpcId: vpc.vpcId,
    ingress: [
        {
            protocol: "tcp",
            fromPort: 5432,
            toPort: 5432,
            cidrBlocks: [
                "0.0.0.0/0",
            ],
        }
    ],
    egress: [
        {
            protocol: "tcp",
            fromPort: 5432,
            toPort: 5432,
            cidrBlocks: [
                "0.0.0.0/0",
            ],
        }
    ],
})

const backyardParameterGroup = new aws.rds.ParameterGroup(
    "parameterGroup",
    {
        family: "postgres14",
        parameters: [
            {name: "max_connections", value: "500", applyMethod: "pending-reboot"},
            {name: "wal_buffers", value: "2048", applyMethod: "pending-reboot"}, // in 8kB
            {
                name: "default_statistics_target",
                value: "100",
                applyMethod: "immediate",
            },
            {name: "random_page_cost", value: "1.1", applyMethod: "immediate"},
            {
                name: "effective_io_concurrency",
                value: "200",
                applyMethod: "immediate",
            },
            {name: "work_mem", value: "65536", applyMethod: "immediate"}, // in kB
            {
                name: "max_parallel_workers_per_gather",
                value: "4",
                applyMethod: "immediate",
            },
            {
                name: "max_parallel_maintenance_workers",
                value: "4",
                applyMethod: "immediate",
            },
            {
                name: "pg_stat_statements.track",
                value: "ALL",
                applyMethod: "immediate",
            },
            {
                name: "shared_preload_libraries",
                value: "pg_stat_statements,auto_explain",
                applyMethod: "pending-reboot",
            },
            {
                name: "track_io_timing",
                value: "1",
                applyMethod: "immediate",
            },
            {
                name: "log_min_duration_statement",
                value: "1000",
                applyMethod: "immediate",
            },
            {
                name: "log_lock_waits",
                value: "1",
                applyMethod: "immediate",
            },
            {
                name: "log_temp_files",
                value: "1",
                applyMethod: "immediate",
            },
            {
                name: "log_checkpoints",
                value: "1",
                applyMethod: "immediate",
            },
            {
                name: "log_connections",
                value: "1",
                applyMethod: "immediate",
            },
            {
                name: "log_disconnections",
                value: "1",
                applyMethod: "immediate",
            },
            {
                name: "log_autovacuum_min_duration",
                value: "0",
                applyMethod: "immediate",
            },
            {
                name: "auto_explain.log_format",
                value: "json",
                applyMethod: "immediate",
            },
            {
                name: "auto_explain.log_min_duration",
                value: "1000",
                applyMethod: "immediate",
            },
            {
                name: "auto_explain.log_analyze",
                value: "1",
                applyMethod: "immediate",
            },
            {
                name: "auto_explain.log_buffers",
                value: "1",
                applyMethod: "immediate",
            },
            {
                name: "auto_explain.log_timing",
                value: "0",
                applyMethod: "immediate",
            },
            {
                name: "auto_explain.log_triggers",
                value: "1",
                applyMethod: "immediate",
            },
            {
                name: "auto_explain.log_verbose",
                value: "1",
                applyMethod: "immediate",
            },
            {
                name: "auto_explain.log_nested_statements",
                value: "1",
                applyMethod: "immediate",
            },
            {
                name: "auto_explain.sample_rate",
                value: "1",
                applyMethod: "immediate",
            },
            {
                name: "rds.logical_replication",
                value: "1",
                applyMethod: "pending-reboot",
            },
        ],
    },
);

const db = new aws.rds.Instance("db", {
    identifier: "db",
    engine: "postgres",
    engineVersion: "14.1",
    instanceClass: "db.t3.micro",
    allocatedStorage: 5,
    username: "edu",
    password: "e12345678",
    dbSubnetGroupName: group.name,
    vpcSecurityGroupIds: [securityGroup.id],
    parameterGroupName: backyardParameterGroup.name,
    publiclyAccessible: true,
    skipFinalSnapshot: true,
})

Expected Behavior

Shows only diffs of fields, which actually changed.

Steps to reproduce

See above the code!

Output of pulumi about

pulumi about -s xxx/prod
CLI
Version      3.60.0
Go Version   go1.20.2
Go Compiler  gc

Plugins
NAME    VERSION
aws     5.33.0
awsx    1.0.2
docker  3.6.1
nodejs  unknown

Host
OS       darwin
Version  13.2.1
Arch     arm64

Current Stack: xxx/xxx/prod

TYPE                                   URN
pulumi:pulumi:Stack                    urn:pulumi:prod::xxx::pulumi:pulumi:Stack::xxx-prod
pulumi:providers:aws                   urn:pulumi:prod::xxx::pulumi:providers:aws::default_5_33_0
pulumi:providers:awsx                  urn:pulumi:prod::xxx::pulumi:providers:awsx::default_1_0_2
awsx:ec2:DefaultVpc                    urn:pulumi:prod::xxx::awsx:ec2:DefaultVpc::default
pulumi:providers:aws                   urn:pulumi:prod::xxx::pulumi:providers:aws::default_5_16_2
aws:rds/subnetGroup:SubnetGroup        urn:pulumi:prod::xxx::aws:rds/subnetGroup:SubnetGroup::xxx-backyard
aws:ec2/securityGroup:SecurityGroup    urn:pulumi:prod::xxx::aws:ec2/securityGroup:SecurityGroup::xxx-backyard-rds
aws:rds/parameterGroup:ParameterGroup  urn:pulumi:prod::xxx::aws:rds/parameterGroup:ParameterGroup::xxx-backyard
aws:rds/instance:Instance              urn:pulumi:prod::xxx::aws:rds/instance:Instance::xxx-backyard


Found no pending operations associated with prod

Backend
Name           [pulumi.com](http://pulumi.com/)


Dependencies:
NAME            VERSION
@types/node     16.18.23
prettier        2.8.7
@pulumi/aws     5.33.0
@pulumi/awsx    1.0.2
@pulumi/pulumi  3.60.0

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@dirien dirien added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Mar 30, 2023
@rquitales
Copy link
Member

@dirien Thanks for logging this. To confirm the severity of this, this is isolated to only the diff preview right? It doesn't cause the cloud resource to be recreated?

@rquitales rquitales removed the needs-triage Needs attention from the triage team label Mar 30, 2023
@dirien
Copy link
Author

dirien commented Apr 3, 2023

@rquitales: no it does not trigger a recreated.

@kpitzen kpitzen added impact/usability Something that impacts users' ability to use the product easily and intuitively area/providers labels Apr 3, 2023
@stepan-romankov
Copy link

@dirien Thanks for logging this. To confirm the severity of this, this is isolated to only the diff preview right? It doesn't cause the cloud resource to be recreated?

Same for me. When I run multiple time pulumi up every time I get

     pulumi:pulumi:Stack     harbour-dev             
 ~   └─ aws:rds:SubnetGroup  rds          update     [diff: ~subnetIds]

@kmosher
Copy link

kmosher commented May 12, 2023

I've found this extends beyond just sort order.

When asked, AWS will always say the applyMethod is "pending-reboot". (Which makes sense, as applyMethod is basically a flag on how to handle a parameter change, not a long-term part of the resource state)

So if you set a parameter applyMethod to "immediate". Pulumi will try to set that, read back "pending-reboot", store that in the resource state, and then generate a diff and repeat the whole cycle on the next update, ad infitum.

applyMethod should basically be ignored for purposes of diffs.

@t0yv0
Copy link
Member

t0yv0 commented Mar 12, 2024

Summary

Contrary to the original hypothesis, set element reordering is not affecting diffs as both TF and bridged providers
apply canonical ordering based on the custom set hash function. The issue here is indeed with implicit refreshing that
changes applyMethod, that is pulumi up results in a different applyMethod in the cloud state from the inputs. This
change affects the parameter hash and ordering when more than 1 parameter is involved, adding to the confusion.

  • currently Pulumi matches default TF behavior on the same program which also has a perpetual diff

  • aws_db_parameter_group
    offers a note about potential perpetual diffs

  • Pulumi docs have a similar NOTE

  • TF does not have the perpetual diff when run with -refresh=false

  • There is currently no supported way for the Pulumi bridged provider to match the -refresh=false behavior of
    Terraform

Resolution

Currently leaning toward closing this as working as advertised, although the NOTE in the docs is a little vague about
the possibility of perpetual diffs right after creating the resource and not making any changes. If you are affected by
this, you might be able to tweak applyMethod values in your program until the perpetual diff disappears.

A more involved possibility would be to build support in pulumi-terraform-bridge for matching -refresh=false behavior
of Terraform.

Minimal repro

import * as pulumi from "@pulumi/pulumi";
import * as aws from "@pulumi/aws";
import * as awsx from "@pulumi/awsx";
import {rds} from "@pulumi/aws";

const backyardParameterGroup = new aws.rds.ParameterGroup(
    "parametergroupmy",
    {
        family: "postgres14",
        parameters: [
            {
                name: "track_io_timing",
                value: "1",
                applyMethod: "immediate", // Changing this to pending-reboot fixes the diff
            },
        ],
    },
);

This yields:

$ pulumi preview --diff

 ~ parameters: [
     ~ [0]: {
             ~ applyMethod: "pending-reboot" => "immediate"
             ~ name       : "track_io_timing" => "track_io_timing"
             ~ value      : "1" => "1"
           }
   ]

Recommended workaround: edit your program source to mach applyMethod to the target values suggested by the pulumi preview --diff.

Minimal repro in TF

Write infra.tf:

resource "aws_db_parameter_group" "default" {
  name   = "rds-pg"
  family = "postgres14"

  parameter {
    name  = "track_io_timing"
    value = "1"
    apply_method = "immediate"
  }
}

And then:

$ terraform apply
$ terraform plan
aws_db_parameter_group.default: Refreshing state... [id=rds-pg]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # aws_db_parameter_group.default will be updated in-place
  ~ resource "aws_db_parameter_group" "default" {
        id          = "rds-pg"
        name        = "rds-pg"
        tags        = {}
        # (4 unchanged attributes hidden)

      - parameter {
          - apply_method = "pending-reboot" -> null
          - name         = "track_io_timing" -> null
          - value        = "1" -> null
        }
      + parameter {
          + apply_method = "immediate"
          + name         = "track_io_timing"
          + value        = "1"
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

$ terraform plan -refresh=false

No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration and found no differences, so no changes are needed.

@t0yv0
Copy link
Member

t0yv0 commented Mar 12, 2024

@stepan-romankov could you please open a separate issue with a repro for [diff: ~subnetIds] as I'm not currently able to reproduce this from this program? Much appreciated.

@t0yv0
Copy link
Member

t0yv0 commented Mar 12, 2024

FWIW on the larger program I get:

pulumi preview --diff                                                                                                                                                                                                                                                                                                                                                                     ~/tmp/2024/03/aws-2442
Previewing update (dev)

View Live: https://app.pulumi.com/anton-pulumi-corp/aws-2442/dev/previews/146208b6-2bba-45b4-b6d8-910a0900f83e

  pulumi:pulumi:Stack: (same)
    [urn=urn:pulumi:dev::aws-2442::pulumi:pulumi:Stack::aws-2442-dev]
    ~ aws:rds/parameterGroup:ParameterGroup: (update)
        [id=parametergroupmy-f2e2fcd]
        [urn=urn:pulumi:dev::aws-2442::aws:rds/parameterGroup:ParameterGroup::parametergroupmy]
        [provider=urn:pulumi:dev::aws-2442::pulumi:providers:aws::default_6_25_0::e55fd59a-44b2-4e82-94e2-7a8f07c2bad1]
      ~ parameters: [
          ~ [2]: {
                  - applyMethod: "pending-reboot"
                  - name       : "track_io_timing"
                  - value      : "1"
                }
          ~ [3]: {
                  - applyMethod: "pending-reboot"
                  - name       : "log_checkpoints"
                  - value      : "1"
                }
          ~ [10]: {
                  + applyMethod: "immediate"
                  + name       : "track_io_timing"
                  + value      : "1"
                }
          ~ [14]: {
                  + applyMethod: "immediate"
                  + name       : "log_checkpoints"
                  + value      : "1"
                }
        ]
Resources:
    ~ 1 to update
    1 unchanged

And editing these listed entries gets rid of the perpetual diff.

@t0yv0
Copy link
Member

t0yv0 commented Mar 12, 2024

For completeness, here are some upstream issues essentially closed as won't fix:

hashicorp/terraform-provider-aws#22028
hashicorp/terraform-provider-aws#20660

@t0yv0
Copy link
Member

t0yv0 commented Mar 12, 2024

I've tried applying an ignoreChanges workaround but it does not apply to this problem, unfortunately. This is now tracked in pulumi/pulumi-terraform-bridge#1756

@t0yv0
Copy link
Member

t0yv0 commented Mar 12, 2024

It remains an option to consider @kmosher suggestion regarding applyMethod should basically be ignored for purposes of diffs, ideally this is something that would happen upstream but we can just do this in the bridged provider as well. One approach would be patching the upstream provider and coding a special CustomizeDiff CustomizeDiffFunc that removes diffs that pertain changes that shift apply_method only without changing the value. The CustomizeDiff interface is a little tricky but at a glance it seems like it could be done correctly to solve this problem and make the resource work as you would expect.

@t0yv0 t0yv0 added this to the 0.102 milestone Mar 14, 2024
t0yv0 added a commit that referenced this issue Mar 14, 2024
@t0yv0 t0yv0 mentioned this issue Mar 14, 2024
t0yv0 added a commit that referenced this issue Mar 18, 2024
t0yv0 added a commit that referenced this issue Mar 19, 2024
t0yv0 added a commit that referenced this issue Mar 19, 2024
Fixes #2442 

This adds a diff customizer that ignores changes to parameters that only
change the apply_method and not the value for the aws_db_parameter_group
resource. To make this work, the change also needs to modify the set
element hashing function to identify parameters that differ only by
apply_method as identical.

As a side-effect of the set hashing change, upgrading stacks to the
newer version of the provider with this change will show a reordering
update diff on the ParameterGroup resource.
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/providers bug/diff kind/bug related to Pulumi generating wrong diffs on preview or up. impact/usability Something that impacts users' ability to use the product easily and intuitively kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants