pulumi / pulumi-terraform-bridge

A library allowing Terraform providers to be bridged into Pulumi.
Apache License 2.0
194 stars 43 forks source link

Avoid default value diffs upon pulumi import #2436

Open t0yv0 opened 4 days ago

t0yv0 commented 4 days ago

Hello!

Issue details

When a user runs pulumi import and then inserts the generated code in the program and runs pulumi up, pulumi may generate unwanted diffs that imply editing the resource inputs from undefined to default values.

An excellent example is found in https://github.com/pulumi/pulumi-aws/issues/4457

The imported code does not specify e.g. force_delete=False which is a good thing for aesthetics as the generated code needs to be as small as possible.

        import pulumi
        import pulumi_aws as aws

        newag = aws.autoscaling.Group("newag",
            availability_zones=["us-west-2a"],
            default_cooldown=300,
            desired_capacity=1,
            health_check_type="EC2",
            launch_template={
                "id": "lt-0030fab2c555bb78f",
                "version": "$Latest",
            },
            max_size=1,
            min_size=1,
            name="ag-130c5f3",
            service_linked_role_arn="arn:aws:iam::616138583583:role/aws-service-role/autoscaling.amazonaws.com/AWSServiceRoleForAutoScaling",
            opts = pulumi.ResourceOptions(protect=True))

However the imported state does not have an entry for force_delete=False.

Subsequent preview generates a diff that is distracting to the user:

        @ previewing update....
            aws:ec2:LaunchTemplate lt1  
         ~  aws:autoscaling:Group newag update [diff: +forceDelete,forceDeleteWarmPool,ignoreFailedScalingActivities,waitForCapacityTimeout]

It would be nice if the diff was not there.

Generalization

It would appear that this issue should occur commonly in bridged providers; the affected fields have simple default values defined against SDKv2, but Terraform does not place guarantees that these defaults will be applied in the Read() method.

            names.AttrForceDelete: {
                Type:     schema.TypeBool,
                Optional: true,
                Default:  false,
            },
            "force_delete_warm_pool": {
                Type:     schema.TypeBool,
                Optional: true,
                Default:  false,
            },

It is possible to reproduce the diff in question in Terrraform proper, it appears it's accepted there:

resource "aws_launch_template" "lt1" {
  name_prefix   = "lt1"
  image_id      = "ami-0f6cac0240f22d17e"
  instance_type = "t2.micro"
}

resource "aws_autoscaling_group" "ag1" {
  availability_zones = ["us-west-2a"]
  desired_capacity = 1
  max_size = 1
  min_size = 1
  launch_template {
    id      = aws_launch_template.lt1.id
    version = "$Latest"
  }
}

resource "aws_autoscaling_group" "ag2" {
  availability_zones = ["us-west-2a"]
  desired_capacity = 1
  max_size = 1
  min_size = 1
  launch_template {
    id      = aws_launch_template.lt1.id
    version = "$Latest"
  }
}
  # aws_autoscaling_group.ag2 will be updated in-place
  ~ resource "aws_autoscaling_group" "ag2" {
      + force_delete                     = false
      + force_delete_warm_pool           = false
        id                               = "terraform-20240923153140154700000003"
      + ignore_failed_scaling_activities = false
        name                             = "terraform-20240923153140154700000003"
      + wait_for_capacity_timeout        = "10m"
        # (27 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

Historical Context

One additional bit of information is that our behavior apparently regressed here with the PlanResourceChange rollout, that is we used to be able to do better.

Affected area/feature

import

t0yv0 commented 4 days ago

This might just be a variation of https://github.com/pulumi/pulumi-terraform-bridge/issues/2272 that is specific to pulumi import vs pulumi up with the import option.

t0yv0 commented 4 days ago

As for alternative solutions.

https://github.com/pulumi/pulumi/issues/16886 was requested in this area but it's unclear it's a net win to me; it adds some complication to the protocol . Also https://pulumi-developer-docs.readthedocs.io/latest/developer-docs/architecture/import.html#challenges

The key issue is overloading Read() for refresh, import, R.get scenarios. If we were revisiting import protocol specifically with the understanding that the provider could opt to support a "better" import we could potentially design for that holistically rather than overloading Read() some more with extra flags.

t0yv0 commented 21 hours ago

https://github.com/pulumi/pulumi-aws/pull/4510 works surprisingly well, could scale it out as default possibly.