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

Structural resources #918

Open
lukehoban opened this issue Feb 12, 2018 · 12 comments
Open

Structural resources #918

lukehoban opened this issue Feb 12, 2018 · 12 comments
Assignees
Labels
area/cli UX of using the CLI (args, output, logs) area/providers kind/enhancement Improvements or new features

Comments

@lukehoban
Copy link
Member

Today, all Pulumi resources are identified by URN. Two resources are different iff they have different URNs, and create/update/deletes are determined based on that.

There are some resources though which do not have any fixed nominal identity in the source provider.

In particular, the AWS provider has several "Attachment" resources, such as aws.iam.RolePolicyAttachment. This resource isn't really a resource in AWS, it's just a request to add (during Create) or remove (during Delete) an association between a Role and a Policy.

That means that these RolePolicyAttachment resources are really "structural" resources. If I already have an association between a Role and a Policy it makes no sense to create a new one - it is only ever associated or not.

Worse though, when we allow these resources to have names, it means the name can change, resulting in a create of a new association followed by a delete of the same association. Because both of these operations affect the same association - this results in going from {A} to {A} to {} (even though the intended result was {A}). Additional details in pulumi/pulumi-cloud#278.

We have tracked this as one of the motivating scenarios for delete-before-create in #450. However, that would lead to downtime with no policy attached, in cases where we really do not intend to make any change.

What feels like the more correct approach is to have a notion of "structural" resources.

A possible sketch of what this would mean:

  • The engine has a bit on a resource indicating whether it is structural or nominal.
  • If that bit indicates a resource is structural, then instead of looking up a URN to identify the "old" value for the resource, we compute a hash of all input properties and look that up (against the similarly hashed properties of all instances of the same type).

Critically, the requirement is that the user is not able to name this resource at all for purposes of identity. We can continue to allow them to provide a name which is used purely for display purposes.

@lukehoban
Copy link
Member Author

We have seen (surprisingly?) relatively few issues related to this in the wild so far - so we'll hold off on making a change here for now.

@sstuddard
Copy link

I believe I'm hitting this.

elb.py:

target_group = ...

def add_instance(instance):
    attachment_id = instance.id.apply(lambda x: x + '_elb_attach')
    attachment = lb.TargetGroupAttachment(attachment_id,
                                          port=9200,
                                          target_group_arn=target_group.arn,
                                          target_id=instance.id)

__main__.py:

import elb

instance = ec2.Instance(...)
elb.add_instance(instance)

Upon doing a preview, I get an error:

    TypeError: Expected resource name to be a string

Am I doing something wrong or do I need a workaround for this issue?

@Max1boy
Copy link

Max1boy commented Aug 27, 2020

I am having the same issue.

for policy in self.policies:
            attach_name = Output.concat(policy.name, "-attachment")
            self.attach = aws.iam.PolicyAttachment(
                attach_name,
                groups=[group_list],
                policy_arn=policy.arn,
                opts=ResourceOptions(provider=self._provider, parent=self._provider)
                )
TypeError: Expected resource name to be a string

I did manage to work around this using:

for num,policy in enumerate(self.policies):
            self.attach = aws.iam.PolicyAttachment(
                f'policy-attachment-{num}',
                groups=group_list,
                policy_arn=policy.arn,
                opts=ResourceOptions(provider=self._provider, parent=self._provider)
                )

Previewing update (dev):
     Type                            Name                 Plan       
     pulumi:pulumi:Stack             account-dev                     
     └─ pulumi:providers:aws         privileged                      
 +      ├─ aws:iam:Group             group1               create     
 +      ├─ aws:iam:Group             group2               create     
 +      ├─ aws:iam:PolicyAttachment  policy-attachment-1  create     
 +      └─ aws:iam:PolicyAttachment  policy-attachment-0  create     
 
Resources:
    + 4 to create
    9 unchanged

but I would prefer giving my resources more recognizable names, is there a solution for this?

@leezen leezen reopened this Sep 14, 2020
@albrechs
Copy link

We have seen (surprisingly?) relatively few issues related to this in the wild so far - so we'll hold off on making a change here for now.

While I don't think this is an incredibly uncommon use case, I can understand why the issue isn't encountered as frequently as one would expect. In reality these attachments tend to exist to prevent the the need to recreate the "root" object, like an IAM Role or Security Group. I'm currently in the process of converting a large terraform project to Pulumi, so I've been mostly deleting and recreating my stack from scratch throughout testing the different components. We are very partial to using our resource name pattern, and understand that in many cases we lose the ability to update certain resources without downtime via deleteBeforeCreate, but we are fine with that.

I only noticed this issue when I realized a typo in the args passed to my IAM role (deploymentName), which would trigger a name change to the Role, and I tried to run the update on existing infrastructure.

Sample Code:

constructor(name: string, iamArgs: AberrantIAMArgs, opts?: pulumi.ResourceOptions) {
    super("aberrant:aws:iam", name, {}, opts);
    const defaultResourceOptions: pulumi.ResourceOptions = { parent:this }

    const ssmReadPolicy = new aws.iam.Policy("iam-ssm-read-policy", { ... }, defaultResourceOptions);

    this.ecsTaskExecutionRole = new aws.iam.Role("iam-task-execution-role", {
        name: `pu-${iamArgs.deploymentEnv}-${iamArgs.deploymentName}-task-role`,
        assumeRolePolicy: JSON.stringify({
            Version: "2012-10-17",
            Statement: [
                {
                    Action: "sts:AssumeRole",
                    Principal: {
                        Service: "ecs-tasks.amazonaws.com"
                    },
                    Effect: "Allow",
                    Sid: ""
                }
            ]
        }),
        tags: {
            Name: `pu-${iamArgs.deploymentEnv}-${iamArgs.deploymentName}-task-role`,
            pulumi: "true"
        }
    }, defaultResourceOptions);

    const taskExecutionPolicyAttachment = new aws.iam.PolicyAttachment("iam-execution-policy-attachment", {
        roles: [ this.ecsTaskExecutionRole.name ],
        policyArn: "arn:aws:iam::aws:policy/service-role/AmazonECSTaskExecutionRolePolicy"
    }, { 
        parent: this.ecsTaskExecutionRole,
        dependsOn: [ this.ecsTaskExecutionRole ]
    });

    const ssmReadPolicyAttachment = new aws.iam.PolicyAttachment("iam-ssm-read-policy-attachment", {
        roles: [ this.ecsTaskExecutionRole.name ],
        policyArn: ssmReadPolicy.arn
    }, { 
        parent: this.ecsTaskExecutionRole,
        dependsOn: [ this.ecsTaskExecutionRole ]
    });
}

Pulumi Preview Output:

     Type                               Name                             Plan        Info
     pulumi:pulumi:Stack                aberrant-io-poc                              
     ├─ aberrant:aws:ecs                poc-ecs                                      
 +-  │  ├─ aws:ecs:TaskDefinition       ecs-task-definition              replace     [diff: ~executionRoleArn,taskRoleArn]
 ~   │  └─ aws:ecs:Service              ecs-service                      update      [diff: ~taskDefinition]
     └─ aberrant:aws:iam                poc-iam                                      
 +-     └─ aws:iam:Role                 iam-task-execution-role          replace     [diff: ~name,tags]
 ~         ├─ aws:iam:PolicyAttachment  iam-execution-policy-attachment  update      [diff: ~roles]
 ~         └─ aws:iam:PolicyAttachment  iam-ssm-read-policy-attachment   update      [diff: ~roles]

Pulumi Up Output:

Type                          Name                     Status                   Info
     pulumi:pulumi:Stack           aberrant-io-poc          **failed**               1 error
     ├─ aberrant:aws:ecs           poc-ecs                                           
 +-  │  └─ aws:ecs:TaskDefinition  ecs-task-definition      replaced                 
     └─ aberrant:aws:iam           poc-iam                                           
 +-     └─ aws:iam:Role            iam-task-execution-role  **replacing failed**     1 error
 
Diagnostics:
  aws:iam:Role (iam-task-execution-role):
    error: deleting urn:pulumi:poc::aberrant-io::aberrant:aws:iam$aws:iam/role:Role::iam-task-execution-role: 1 error occurred:
    	* error deleting IAM Role (pu-prod-pulumi-poc.aberrant.io-task-role): DeleteConflict: Cannot delete entity, must detach all policies first.
    	status code: 409, request id: 9c336b8e-7188-4f10-8bee-f0de4411baf0

So even though I have strongly expressed that the attachment is dependent on the role, and the pulumi engine sees that the resource needs to be updated based on the new role, it consistently attempts to replace the role before deleting the attachments. Throughout my testing I also used aws.iam.RolePolicyAttachment instead of PolicyAttachment, but the same behavior was observed even when the PolicyAttachment was marked for deletion.

Preview Output w/ RolePolicyAttachment

     Type                                   Name                             Plan        Info
     pulumi:pulumi:Stack                    aberrant-io-poc                              
     ├─ aberrant:aws:ecs                    poc-ecs                                      
 +-  │  ├─ aws:ecs:TaskDefinition           ecs-task-definition              replace     [diff: ~executionRoleArn,taskRoleArn]
 ~   │  └─ aws:ecs:Service                  ecs-service                      update      [diff: ~taskDefinition]
     └─ aberrant:aws:iam                    poc-iam                                      
 +-     └─ aws:iam:Role                     iam-task-execution-role          replace     [diff: ~name,tags]
 +         ├─ aws:iam:RolePolicyAttachment  iam-execution-policy-attachment  create      
 +         ├─ aws:iam:RolePolicyAttachment  iam-ssm-read-policy-attachment   create      
 -         ├─ aws:iam:PolicyAttachment      iam-execution-policy-attachment  delete      
 -         └─ aws:iam:PolicyAttachment      iam-ssm-read-policy-attachment   delete      
 

Seeing this I decided to test the behavior of the aws.ec2.SecurityGroup and aws.ec2.SecurityGroupRule resources under the same conditions (all of my security groups are declared ruleless within components, rules are created in index.ts to "glue" everything together) and found the same outcome. If I trigger some change requiring replacement on the security group, it does not know to first delete the security group rules, and an error is thrown by the AWS API.

With all that - its not incredibly common to see things like the actual role or security group require replacement when the rules/policies are not declared in line, that's kind of why they're structured that way in the first place. That said - I've worked on projects where existing resource names needed to be updated across the board (typically for some regulatory/governance reason), and as much as it pains me to say it - not every problem can be solved by deleting and recreating a stack. I would expect that pulumi handles all resource dependencies the same, even though attachments have no "hard" resource to link them to, they are still resources in the world of pulumi, no different than an EC2 or RDS instance.

@albrechs
Copy link

I found a workaround! I still can't 100% explain how/why it works, but it does.

I started breaking down the individual roles and policies, and decided that it would be best to loop through the policy attachments, which required a pulumi.all().apply() to pull off. Of course, forcing the apply told the engine that those policy attachments need to be destroy while the parent role is replaced, and everything worked itself out.

const taskPolicies: aws.iam.Policy[] = [
    new aws.iam.Policy("iam-task-ssm-read-policy", { ... }, defaultResourceOptions),
    new aws.iam.Policy("iam-task-kms-use-policy", { ... }, defaultResourceOptions),
    // SQS,
    // S3,
    // SNS,
    // SSM
]

this.roles = {
    ecsExecution: new aws.iam.Role("iam-execution-role", { ... }, defaultResourceOptions),
    task: new aws.iam.Role("iam-task-role", { ... }, defaultResourceOptions)
};

const taskPolicyAttachments = pulumi.all(taskPolicies).apply((policies) => {
    policies.map((policy,index) => 
        new aws.iam.RolePolicyAttachment(`iam-task-role-policy-attachment-${index}`, {
            role: this.roles.task.name,
            policyArn: policy.arn
        }, { 
            parent: this.roles.task,
            dependsOn: [ this.roles.task ]
        })
    )
})

pulumi up Output:

     Type                                   Name                               Status       Info
     pulumi:pulumi:Stack                    aberrant-io-poc                                 
     ├─ aberrant:aws:ecs                    poc-ecs                                         
 +-  │  ├─ aws:ecs:TaskDefinition           ecs-queue-task-definition          replaced     [diff: ~taskRoleArn]
 ~   │  │  └─ aws:ecs:Service               ecs-queue-service                  updated      [diff: ~taskDefinition]
 +-  │  ├─ aws:ecs:TaskDefinition           ecs-dbMigration-task-definition    replaced     [diff: ~taskRoleArn]
 ~   │  │  └─ aws:ecs:Service               ecs-db-migration-service           updated      [diff: ~taskDefinition]
 +-  │  ├─ aws:ecs:TaskDefinition           ecs-web-task-definition            replaced     [diff: ~taskRoleArn]
 ~   │  │  └─ aws:ecs:Service               ecs-web-service                    updated      [diff: ~taskDefinition]
 +-  │  ├─ aws:ecs:TaskDefinition           ecs-jobs-task-definition           replaced     [diff: ~taskRoleArn]
 ~   │  │  └─ aws:ecs:Service               ecs-jobs-service                   updated      [diff: ~taskDefinition]
 +-  │  └─ aws:ecs:TaskDefinition           ecs-es-task-definition             replaced     [diff: ~taskRoleArn]
 ~   │     └─ aws:ecs:Service               ecs-es-service                     updated      [diff: ~taskDefinition]
     └─ aberrant:aws:iam                    poc-iam                                         
 +-     └─ aws:iam:Role                     iam-task-role                      replaced     [diff: ~name]
 +-        ├─ aws:iam:RolePolicyAttachment  iam-task-role-policy-attachment-1  replaced     [diff: ~role]
 +-        └─ aws:iam:RolePolicyAttachment  iam-task-role-policy-attachment-0  replaced     [diff: ~role]

AaronFriel added a commit to pulumi/pulumi-tailscale that referenced this issue Jan 24, 2023
Fixes #98

The ACL resource is what Pulumi would consider a "structural resource", see:
- pulumi/pulumi#918

This kind of resource is not quite correctly handled in the object model, and
this interacts poorly with the upstream provider requiring the ACL to exactly
match the default for "create" to succeed.

It's a reasonable safety precaution of the upstream provider, so it's better to
work around that by using a regular resource in the test.
@benlongo
Copy link

benlongo commented Jun 8, 2023

It seems like most resources of this nature are joining two (or more) resources together, which forms a natural primary key so they don't have an additional key. Would it be possible to make the urn for such resources contain all relevant IDs instead?

@WonderPanda
Copy link

Just went down a huge rabbit hole with the related issues to this one today because we realized that some role policy attachments had disappeared from AWS even though they were still in Pulumi. This caused some cascading failures in our Staging environment because a bunch of Fargate tasks could no longer start up since they were missing required permissions

@t0yv0
Copy link
Member

t0yv0 commented Dec 15, 2023

Would allowing the provider to control URN allocation for the resource fix the issue cleanly (though that could perhaps be difficult to carry out)? Coming here from
pulumi/pulumi-aws#1923 for the QueuePolicy resource the identity of the cloud resource is clearly tied to queueUrl parameter. The AWS provider knows this. The engine does not need to know this or assume hashing strategies. The idea here is that if the provider could influence the selection of URN during Create, it could encode the correct notion of identity that the engine then would use.

@Frassle
Copy link
Member

Frassle commented Dec 16, 2023

Would allowing the provider to control URN allocation for the resource fix the issue cleanly (though that could perhaps be difficult to carry out)?

We'd need to have a way of sending the provider just the type and current inputs and for it to tell us the URN, because this would have to be before any state lookup.
Also that then makes some horrible oddities of URN not having the resource logical name in it. But it might be that these resources shouldn't have user given logical names in the first place because they aren't unique entities.

@t0yv0
Copy link
Member

t0yv0 commented Dec 20, 2023

pulumi/pulumi-aws#3166 adds some interesting implications to refresh. Adding BucketVersioningV2 has the side-effect that Bucket is no longer clean with respect to refresh, as its pulumi-tracked state differs from cloud-tracked state once the sidecar resource has been added.

This is a bit of a moonshot possibility, but Pulumi providers could map these resources to properties of the main resource (Bucket)? This would make it explicit that not only identity but also lifecycle of these is tied to the owning resource.

@Frassle
Copy link
Member

Frassle commented Dec 20, 2023

This is a bit of a moonshot possibility, but Pulumi providers could map these resources to properties of the main resource (Bucket)? This would make it explicit that not only identity but also lifecycle of these is tied to the owning resource.

I did have the same thought. Just get rid of all structural resources are map them to properties on other resources.
Makes some amount of sense for things like bucket settings like this, but trickier for structural resources that link two different resources together. You've then got to decide on which side of the relationship the resource is going to go.

@VenelinMartinov
Copy link
Contributor

VenelinMartinov commented Jan 19, 2024

pulumi/pulumi-terraform-bridge#1631 Related issue where resources with DeleteBeforeReplace in the schema get silently deleted when being replaced because their parent provider is replaced. Pretty sure most of them would be "structural"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli UX of using the CLI (args, output, logs) area/providers kind/enhancement Improvements or new features
Projects
None yet
Development

No branches or pull requests