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

Avoid default value diffs upon pulumi import #2436

Open
t0yv0 opened this issue Sep 23, 2024 · 4 comments
Open

Avoid default value diffs upon pulumi import #2436

t0yv0 opened this issue Sep 23, 2024 · 4 comments
Labels
kind/enhancement Improvements or new features

Comments

@t0yv0
Copy link
Member

t0yv0 commented Sep 23, 2024

Hello!

  • Vote on this issue by adding a 👍 reaction
  • If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

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 pulumi/pulumi-aws#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 t0yv0 added kind/enhancement Improvements or new features needs-triage Needs attention from the triage team labels Sep 23, 2024
@t0yv0
Copy link
Member Author

t0yv0 commented Sep 23, 2024

This might just be a variation of #2272 that is specific to pulumi import vs pulumi up with the import option.

@t0yv0
Copy link
Member Author

t0yv0 commented Sep 23, 2024

As for alternative solutions.

pulumi/pulumi#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
Copy link
Member Author

t0yv0 commented Sep 27, 2024

pulumi/pulumi-aws#4510 works surprisingly well, could scale it out as default possibly.

@t0yv0
Copy link
Member Author

t0yv0 commented Oct 16, 2024

I think pulumi/pulumi-aws#4644 is another problem that seems to be caused by this; it is much worse in this case since the program is trying to generate a replace, which the user emphatically cannot accept.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements or new features
Projects
None yet
Development

No branches or pull requests

2 participants