aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.59k stars 3.88k forks source link

[ec2] Token numbers should be content dependent, not just sequential #10251

Closed OlegBoulanov closed 2 years ago

OlegBoulanov commented 4 years ago

Token numbers should be content dependent, not just sequential

Use Case

An innocent change (say, of policy, nothing of EIPs) can trigger tokens renumbering, and result with:

Resources
[-] AWS::EC2::EIP EIPTokenTOKEN445 destroy
[+] AWS::EC2::EIP EIP--Token-TOKEN-450-- EIPTokenTOKEN450
[~] AWS::Route53::RecordSet ARecord-Public-NewAlloc ARecordPublicNewAlloc0474C286
 └─ [~] ResourceRecords
     └─ @@ -1,5 +1,5 @@
        [ ] [
        [ ]   {
        [-]     "Ref": "EIPTokenTOKEN445"
        [+]     "Ref": "EIPTokenTOKEN450"
        [ ]   }
        [ ] ]

Unfortunately, certain systems, even AWS ones, like Chime Voice Connector Termination Allowed hosts list, require IPs to be registered, not allowing for DNS names. So even a minor change may break production system, which is not desirable.

Proposed Solution

Should the tokens be generated by content, the unnecessary change would not happen.

Other


This is a :rocket: Feature Request

rix0rrr commented 4 years ago

Exactly for this reason it is disallowed to use a Token in a logical ID.

That should be checked, and I'm fairly sure we have a test for that. Can I see your code?

Of course you can take a stringified token, manipulate it so it's no longer recognized as such, and then use that in a logical ID, but then you've circumvented the protection we've put in place.

OlegBoulanov commented 4 years ago

@rix0rrr The code is pretty straightforward:

                    var newElasticIPs = hosts.Where(h => h.Group.AllocateNewElasticIPs).Select(h =>
                    {
                        return new CfnEIP(this, $"EIP{h.Instance.InstanceId}".AsCloudFormationId(), new CfnEIPProps
                        {
                            Domain = "vpc",
                            InstanceId = h.Instance.InstanceId,
                        });
                    }).ToList()      
                    if (newElasticIPs.Any())
                    {
                        // Register public Elastic IPs
                        var arNewPublic = new ARecord(this, $"ARecord_Public_NewAlloc".AsCloudFormationId(), new ARecordProps
                        {
                            Zone = theZone,
                            RecordName = $"{schema.SubdomainNewEIPs}.{theZone.ZoneName}",
                            Target = RecordTarget.FromValues(newElasticIPs.Select(eip => eip.Ref).ToArray()),
                            Ttl = Duration.Seconds(300),
                        });
                    } 

The problem is that it's a part of one of several stacks. Changes like adding a resource (maybe in a different stack), requiring yet another token, shift token count, and this results in undesirable EIP change.

OlegBoulanov commented 4 years ago

In related question: https://github.com/aws/aws-cdk/issues/10288 I try to get an advice on how could I register pre-allocated EIPs?

This code works for new EIPs:

var eip = new CfnEIP(...)

Target = RecordTarget.FromValues(newElasticIPs.Select(eip => eip.Ref).ToArray()),

How do I create a target from EIP association?

var eipa = new CfnEIPAssociation(... allocation ID ...)

Target = RecordTarget.?????(elasticIPAssociations.Select(eipa => eipa.???).ToArray()),

Having EIPs allocated, how do I retrieve their actual IPs, to create a record target?

peterwoodworth commented 2 years ago

return new CfnEIP(this, $"EIP{h.Instance.InstanceId}".AsCloudFormationId()

It looks to me like you're using a token in your logical ID - where is that token coming from? Do you know if this still works on the current version of CDK?

As for your second question, I don't believe that's possible since the only return value provided by cloudformation is the CfnEipAssociation name

github-actions[bot] commented 2 years ago

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.