pulumi / pulumi-aws

An Amazon Web Services (AWS) Pulumi resource package, providing multi-language access to AWS
Apache License 2.0
464 stars 155 forks source link

ECS Container definition permanent diff #1738

Closed jonathanmyers closed 4 months ago

jonathanmyers commented 2 years ago

Hello!

Issue details

Past some length or number of elements, the ECS container definition string starts showing current state in an alphabetized format. If your container definition object is not alphabetized in your source code, this results in a persistent diff where every pulumi up results in a task recreation regardless of whether real changes are present.

Also past this length or number of elements, current state starts showing default elements that are not present in source, which also creates a diff. I had to write in empty properties for mountPoint and volumesFrom, and set essential and cpu to prevent the diff.

The lack of a pretty printed diff of this string in pulumi's preview makes this a bit labor intensive to diagnose--with a large number of elements the only practical way to find the problem is to copy the string out, remove escape characters, then pretty print and diff the strings elsewhere.

I'm guessing the underlying AWS api does this for convenience in the UI, but I think there are some options for making it behave better in pulumi.

Steps to reproduce

  1. Create a long container definition object where elements are not alphabetized. Mine is 5800 characters.
  2. Run pulumi up twice and see that there is still a diff.

Expected: There is no diff after a successful pulumi up, even if I'm not alphabetizing or passing optional parameters Actual: The diff persists indefinitely

ixti commented 1 year ago

I believe this is related to: https://github.com/pulumi/pulumi-aws/issues/1985

t0yv0 commented 8 months ago

If anyone is still affected by this, I'd appreciate a quick repro of a Pulumi program demonstrating the problem, it would really helps us narrow this down and look at some resolution options.

t0yv0 commented 6 months ago

So apparently this issue pertains to containerDefinitions of ecs.TaskDefinition that is typed as string but represents a complex JSON structure. #1985 is likely correctly identified as a root cause - there is a lack of upstream normalization for the JSON for the purposes of diff. This is still open upstream. We may consider patching upstream to make this work sooner rather than later.

https://www.pulumi.com/registry/packages/aws/api-docs/ecs/taskdefinition/

smsunarto commented 5 months ago

Still having this issue - happy to help with repro if still needed!

t0yv0 commented 5 months ago

New repros are always welcome! I've been looking at this issue recently and there is a cluster of related issues, looks like upstream is already performing some workarounds but it sounds like they are not sufficient. We could use several programs that reproduce this sort of issue here.

t0yv0 commented 4 months ago

I was able to reproduce permanent diff with this program:

import * as aws from "@pulumi/aws";
import * as awsx from "@pulumi/awsx";

const cluster = new aws.ecs.Cluster("test", {
    name: "example",
});

const svc = new awsx.ecs.FargateService(`foo-bar`, {
    cluster: cluster.arn,
    desiredCount: 1,
    assignPublicIp: true,
    taskDefinitionArgs: {
        containers: {
            "nginx": {
                name: "nginx-name",
                image: "nginx",
                environment: [
                ],
                cpu: 512,
                memory: 2048,
                startTimeout: 10,
                healthCheck: {
                    interval: 5,
                    retries: 10,
                    command: [ "CMD-SHELL", "curl -f http://localhost:8080/health || exit 1" ],
                },
            },
        }
    },
});
t0yv0 commented 4 months ago

CPU, Essential, etc is normalized in https://github.com/hashicorp/terraform-provider-aws/blob/master/internal/service/ecs/task_definition_equivalency.go#L69

Looks like healthCheck.timeout was not normalized, causing the permanent diff here.

smsunarto commented 4 months ago

Confirmed as resolved!

stefan01 commented 3 months ago

I am still experiencing this issue even with the newest version 6.47.0 :( More specifically, it is effecting the ordering of the Environment section and it always wants to remove the Properties:

t0yv0 commented 3 months ago

Can you please open a new issue with a repro @stefan01 thank you.